Bug 174348

Comment on attachment 315595 [details] Patch Attachment 315595 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4129383 New failing tests: security/contentSecurityPolicy/video-with-data-url-allowed-by-media-src-star.html storage/indexeddb/modern/new-database-after-user-delete.html

Comment on attachment 315595 [details] Patch Attachment 315595 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4129732 New failing tests: http/tests/canvas/canvas-tainted-after-draw-image.html

Comment on attachment 315595 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315595&action=review Awesome! My biggest takeaway is that we have a lot of static maps / sets / vectors that never change and that perhaps we should have specialized data structures for them. And, it reaffirmed my curiosity on determining the right size of a list deserves what data structure (e.g. does a list of 4 items really need to be in a HashSet, or would lookups in Vector be just as fast). > Source/WTF/wtf/NeverDestroyed.h:76 > +template<typename T> NeverDestroyed<T> makeNeverDestroyed(T&&); I'm not sure I see the point of this forward declaration. > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2868 > + for (auto& expectedName : names) { > + if (*expectedName == name) > + return true; > + } > + return false; This could be written as: std::any_of(names.begin(), names.end(), [&name] (auto& expectedName){ return *expectedName == name; }); Not sure it's better. If this idiom is common enough, maybe we should an overload of Vector::contains that takes a predicate. (It kind of already exists in the form of Vector::findMatching). > Source/WebCore/editing/EditingStyle.cpp:901 > +static const Vector<const HTMLElementEquivalent*>& htmlElementEquivalents() Maybe we need a std::array_ref (http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3334.html) for things like this. Or perhaps std::array could do it. > Source/WebCore/loader/CrossOriginAccessControl.cpp:70 > bool isOnAccessControlResponseHeaderWhitelist(const String& name) > { > - static std::once_flag onceFlag; > - static LazyNeverDestroyed<HTTPHeaderSet> allowedCrossOriginResponseHeaders; > - > - std::call_once(onceFlag, []{ > - allowedCrossOriginResponseHeaders.construct<std::initializer_list<String>>({ > - "cache-control", > - "content-language", > - "content-type", > - "expires", > - "last-modified", > - "pragma" > - }); > + static const auto allowedCrossOriginResponseHeaders = makeNeverDestroyed(HTTPHeaderSet { > + "cache-control", > + "content-language", > + "content-type", > + "expires", > + "last-modified", > + "pragma" > }); This function seems to be unused. So you can just remove the whole thing. > Source/WebCore/page/DebugPageOverlays.cpp:165 > + HashMap<String, Color> map; > + for (auto& entry : entries) > + map.add(entry.name, Color { entry.r, entry.g, entry.b, 80 }); > + return map; Do initializer_lists for HashMaps like this not work? Something like: map = { { "touchstart", Color(191, 191, 63, 80) }, ... }? Would be nice if we could get that to work if it doesn't. Also, elsewhere you do things like: map.add(ASCIILiteral {entry.name }, ...). Might be good to be consistent. > Source/WebCore/platform/MIMETypeRegistry.cpp:385 > + return { 1, type }; While the terseness is nice, I find this a very confusing construction. That might be because I find the Vector constructor that it calls confusing. Perhaps something like { { type } } (if that compiles)? > Source/WebCore/platform/SchemeRegistry.cpp:137 > + static const auto schemes = makeNeverDestroyedStringVector({ Why not just makeNeverDestroyed(Vector<String> { ? Why the extra function here? > Source/WebCore/platform/SchemeRegistry.cpp:151 > + static auto secureSchemes = makeNeverDestroyedSchemeSet(builtinSecureSchemes); Seems like this could be build with an initialize_list as well. > Source/WebCore/platform/graphics/FontCascade.cpp:497 > + static const auto map = makeNeverDestroyed([] { > + static const char* const fontFamiliesWithInvalidCharWidth[] = { Can this just us an initializer_list directly? > Source/WebCore/rendering/RenderThemeMac.mm:115 > +@interface NSTextFieldCell () > +- (CFDictionaryRef)_coreUIDrawOptionsWithFrame:(NSRect)cellFrame inView:(NSView *)controlView includeFocus:(BOOL)includeFocus; > +@end This, a others in this file, should have a FIXME about moving it to one of the ..SPI.h files. > Source/WebCore/rendering/svg/RenderSVGResource.cpp:178 > + static NeverDestroyed<HashSet<SVGElement*>> invalidatingDependencies; Ugh, this still bothers me that this is static. Maybe it could live off renderer.document().accessSVGExtensions()? Clearly moving it down here is an improvement, as it shows much more clearly that the element is both added and removed. > Source/WebCore/svg/SVGPatternElement.cpp:103 > + SVGNames::patternUnitsAttr, SVGNames::patternContentUnitsAttr, SVGNames::patternTransformAttr, > + SVGNames::xAttr, SVGNames::yAttr, SVGNames::widthAttr, SVGNames::heightAttr, would put these each on their own line.

(In reply to Sam Weinig from comment #25) > My biggest takeaway is that we have a lot of static maps / sets / > vectors that never change and that perhaps we should have specialized data > structures for them. And, it reaffirmed my curiosity on determining the > right size of a list deserves what data structure (e.g. does a list of 4 > items really need to be in a HashSet, or would lookups in Vector be just as > fast). Yes, eventually I think we might want to use linear search for some cases, binary search for others, and hash tables for others.

I don’t yet understand the test failure.

Comment on attachment 315595 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315595&action=review Still unsure about why there are test failures on EWS! >> Source/WTF/wtf/NeverDestroyed.h:76 >> +template<typename T> NeverDestroyed<T> makeNeverDestroyed(T&&); > > I'm not sure I see the point of this forward declaration. I like to define classes and functions, even templates, in a terse way without interleaving implementation. Then have implementations be separate. So I think of this as the interface to makeNeverDestroyed. I’m not planning to remove this, but I don’t feel strongly. I will remove it if you want me to. >> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2868 >> + return false; > > This could be written as: > > std::any_of(names.begin(), names.end(), [&name] (auto& expectedName){ return *expectedName == name; }); > > Not sure it's better. > > If this idiom is common enough, maybe we should an overload of Vector::contains that takes a predicate. (It kind of already exists in the form of Vector::findMatching). I will use any_of, I think. >> Source/WebCore/editing/EditingStyle.cpp:901 >> +static const Vector<const HTMLElementEquivalent*>& htmlElementEquivalents() > > Maybe we need a std::array_ref (http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3334.html) for things like this. Or perhaps std::array could do it. Sure. Maybe it will be named std::span or std::array_view in the C++ standard some day. Or should we come up with our own? Quite similar to WTF::StringView, I think. >> Source/WebCore/loader/CrossOriginAccessControl.cpp:70 >> }); > > This function seems to be unused. So you can just remove the whole thing. Good catch. Done. >> Source/WebCore/page/DebugPageOverlays.cpp:165 >> + return map; > > Do initializer_lists for HashMaps like this not work? Something like: > > map = { { "touchstart", Color(191, 191, 63, 80) }, ... }? > > Would be nice if we could get that to work if it doesn't. > > Also, elsewhere you do things like: > > map.add(ASCIILiteral {entry.name }, ...). Might be good to be consistent. I think an initializer_list for a HashMap will probably work. The reason I wrote it the way I did it was that I was tempted to make a static const array that I was sure the compiler would generate as data with no code, kind of "all POD", and the initializer_list with all the Color in it almost certainly will be code rather than just data. I am not saying I am making the tradeoff correctly. And, yes, I know I am not consistent about ASCIILiteral. That’s because I don’t have a good understanding of what the tradeoff is. Is having the string inside the StringImpl better for performance than having a pointer that points all the way to the data segment? Does saving a few bytes from a memory allocation add up to enough to be worth it? Since it is simpler to not use ASCIILiteral, it’s always tempting to not use it. I really am unclear about what we should do! I am tempted to just add ASCIILiteral here. But I am not sure. And then there’s the way these two issues intersect. It would be really ugly to have to write ASCIILiteral on every line, which I suppose is what I’d need if I was using the HashMap with an initializer_list. I suppose we’d want it even in that case. I really wish there was a different type for immortal string literals rather than const char* so I could do all of this with the type system. >> Source/WebCore/platform/MIMETypeRegistry.cpp:385 >> + return { 1, type }; > > While the terseness is nice, I find this a very confusing construction. That might be because I find the Vector constructor that it calls confusing. Perhaps something like { { type } } (if that compiles)? Yes, we should probably eliminate that Vector constructor. Without something to indicate what the arguments mean, it is not very good. I will try to see if the two sets of braces workw >> Source/WebCore/platform/SchemeRegistry.cpp:137 >> + static const auto schemes = makeNeverDestroyedStringVector({ > > Why not just makeNeverDestroyed(Vector<String> { ? Why the extra function here? I wrote this before I realized I could do that. I will change it. >> Source/WebCore/platform/SchemeRegistry.cpp:151 >> + static auto secureSchemes = makeNeverDestroyedSchemeSet(builtinSecureSchemes); > > Seems like this could be build with an initialize_list as well. There is a Vector<String> here and I don’t know how to turn that into an initializer_list. You did make a good point earlier about the fact that these say Vector but they should really be std::array_view or whatever. In fact, std::intiializer_list seems a lot like std::array_view. >> Source/WebCore/platform/graphics/FontCascade.cpp:497 >> + static const char* const fontFamiliesWithInvalidCharWidth[] = { > > Can this just us an initializer_list directly? Probably. I will try. Honestly, I am a little worried that I should be using ConstructFromLiteral here (see all my comments about my ASCIILiteral uncertainty above). If I do decide I need that then I will be sorry if I use an initializer_list; or I suppose I can make a function that takes the initializer_list and does the good thing. >> Source/WebCore/rendering/RenderThemeMac.mm:115 >> +@end > > This, a others in this file, should have a FIXME about moving it to one of the ..SPI.h files. Done. The wording I chose: // FIXME: This should go into an SPI.h file in the spi directory. >> Source/WebCore/rendering/svg/RenderSVGResource.cpp:178 >> + static NeverDestroyed<HashSet<SVGElement*>> invalidatingDependencies; > > Ugh, this still bothers me that this is static. Maybe it could live off renderer.document().accessSVGExtensions()? Clearly moving it down here is an improvement, as it shows much more clearly that the element is both added and removed. I’d be happy to move this. But maybe not in this patch? >> Source/WebCore/svg/SVGPatternElement.cpp:103 >> + SVGNames::xAttr, SVGNames::yAttr, SVGNames::widthAttr, SVGNames::heightAttr, > > would put these each on their own line. I thought I had grouped them into two themed sets: The pattern ones, then the geometry ones. When I put each on its own line, I usually also just sort instead of trying to use a logical order. I want to leave this one as is. Not that it’s perfect, but I am not so motivated to change it. What do you think?

Created attachment 315668 [details] Patch for landing (and to re-run EWS)

Committed r219595: <http://trac.webkit.org/changeset/219595>

This broke some internal builds, fix incoming.

Internal build fixed in <http://trac.webkit.org/changeset/219615>. Note that this build fix is untested on iOS 10.3.