Streamline handling of attributes, using references as much as possible
Created attachment 311944 [details] Patch
Comment on attachment 311944 [details] Patch Attachment 311944 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3867812 Number of test failures exceeded the failure limit.
Created attachment 311949 [details] Archive of layout-test-results from ews115 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 311959 [details] Patch
This new version is passing all the tests on EWS.
Comment on attachment 311959 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=311959&action=review r=me with comment > Source/WebCore/dom/ScriptElement.cpp:320 > + auto& nonce = m_element.attributeWithoutSynchronization(HTMLNames::nonceAttr); This looks unsafe given the dispatchBeforeLoadEvent() call below. We execute JS which could modify the attributes and invalidate the reference. > Source/WebCore/html/HTMLAudioElement.cpp:54 > + element->setAttributeWithoutSynchronization(preloadAttr, "auto"); ASCIILiteral("auto") ?
Comment on attachment 311959 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=311959&action=review Thanks for reviewing. >> Source/WebCore/dom/ScriptElement.cpp:320 >> + auto& nonce = m_element.attributeWithoutSynchronization(HTMLNames::nonceAttr); > > This looks unsafe given the dispatchBeforeLoadEvent() call below. We execute JS which could modify the attributes and invalidate the reference. Glad you caught that. How did I miss it? >> Source/WebCore/html/HTMLAudioElement.cpp:54 >> + element->setAttributeWithoutSynchronization(preloadAttr, "auto"); > > ASCIILiteral("auto") ? I am torn. My apologies in advance for using { } and spaces instead of () below, I don’t know why I am doing this all the time, except that I like the type strictness. 1) "auto", implicitly turned into AtomicString { "auto" } 2) ASCIILiteral { "auto" }, implicitly turned into AtomicString { ASCIILiteral { "auto" } } 3) AtomicString { "auto", AtomicString::ConstructFromLiteral } I am pretty sure (2) doesn’t work. See this code from AtomicString.h: // The explicit constructors with AtomicString::ConstructFromLiteral must be used for literals. AtomicString(ASCIILiteral); I don’t really understand the tradeoffs here. None of these do any memory allocation if the literal is already in the string table. (3) avoids making a call to strlen or doing the equivalent by checking each character to see if it is null. (2) and (3) avoid copying the characters into the StringImpl if one is allocated. (3) has more code at each call site than (1). Then there are the preallocation strategies: 4) static NeverDestroyed<AtomicString> autoString { "auto", AtomicString::ConstructFromLiteral }; Ugly when used just for a constant, with the NeverDestroyed and having to figure out what to name it. Why do we do this in the style of (3) rather (1)? 5) A global variable somewhere. For example, the five special globals in AtomicString.h itself, the ones for all the attribute names in HTMLNames.h. We could add globals for common attribute values as well as attribute names, and if so, I’m sure we would have one for "auto". But would that be a good optimization, or would it just slow things down by spending more time filling the AtomicString hash table with more strings and having a more crowded table. Given the above, I think I will stick with (1). But maybe you can convince me otherwise.
Committed r217774: <http://trac.webkit.org/changeset/217774>
Comment on attachment 311959 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=311959&action=review >>> Source/WebCore/html/HTMLAudioElement.cpp:54 >>> + element->setAttributeWithoutSynchronization(preloadAttr, "auto"); >> >> ASCIILiteral("auto") ? > > I am torn. My apologies in advance for using { } and spaces instead of () below, I don’t know why I am doing this all the time, except that I like the type strictness. > > 1) "auto", implicitly turned into AtomicString { "auto" } > 2) ASCIILiteral { "auto" }, implicitly turned into AtomicString { ASCIILiteral { "auto" } } > 3) AtomicString { "auto", AtomicString::ConstructFromLiteral } > > I am pretty sure (2) doesn’t work. See this code from AtomicString.h: > > // The explicit constructors with AtomicString::ConstructFromLiteral must be used for literals. > AtomicString(ASCIILiteral); > > I don’t really understand the tradeoffs here. None of these do any memory allocation if the literal is already in the string table. (3) avoids making a call to strlen or doing the equivalent by checking each character to see if it is null. (2) and (3) avoid copying the characters into the StringImpl if one is allocated. (3) has more code at each call site than (1). > > Then there are the preallocation strategies: > > 4) static NeverDestroyed<AtomicString> autoString { "auto", AtomicString::ConstructFromLiteral }; > > Ugly when used just for a constant, with the NeverDestroyed and having to figure out what to name it. Why do we do this in the style of (3) rather (1)? > > 5) A global variable somewhere. > > For example, the five special globals in AtomicString.h itself, the ones for all the attribute names in HTMLNames.h. We could add globals for common attribute values as well as attribute names, and if so, I’m sure we would have one for "auto". But would that be a good optimization, or would it just slow things down by spending more time filling the AtomicString hash table with more strings and having a more crowded table. > > Given the above, I think I will stick with (1). But maybe you can convince me otherwise. Oh, I was wrong about using ASCIILiteral() here indeed. I seem to remember Ben asking me to use AtomicString { "auto", AtomicString::ConstructFromLiteral } in such cases though. I do not remember the benefits but I did not look into it in much details. 1) or 3), I do not really mind either way.
Mass moving XML DOM bugs to the "DOM" Component.