Use a typedef instead of inheriting WTF::Vector for AttributeVector
Created attachment 138031 [details] Patch
Trying to address comment https://bugs.webkit.org/show_bug.cgi?id=79963#c16.
Better wait until more complex bug 83440 lands to minimize disruption.
Ack.
Created attachment 138577 [details] Patch
Comment on attachment 138577 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=138577&action=review Looks good, but let's fix up some things and make it great! > Source/WebCore/html/parser/HTMLTreeBuilder.cpp:579 > - attributes.removeAttribute(nameAttr); > - attributes.removeAttribute(actionAttr); > - attributes.removeAttribute(promptAttr); > + removeAttribute(&attributes, nameAttr); > + removeAttribute(&attributes, actionAttr); > + removeAttribute(&attributes, promptAttr); Since this is the only user of removeAttribute(), we could write it as a loop that does a single pass across the attribute vector instead of 3 passes. > Source/WebCore/html/parser/HTMLTreeBuilder.cpp:583 > - attributes.insertAttribute(Attribute(nameAttr, isindexTag.localName())); > + if (!getAttributeFromVector(&attributes, nameAttr)) > + attributes.append(Attribute(nameAttr, isindexTag.localName())); Given that the nameAttr is always removed just above, you don't need the getAttributeFromVector() check here. > Source/WebCore/html/parser/TextDocumentParser.cpp:64 > - attributes.insertAttribute(Attribute("style", "word-wrap: break-word; white-space: pre-wrap;")); > + attributes.append(Attribute("style", "word-wrap: break-word; white-space: pre-wrap;")); It would be slightly more efficient to pass HTMLNames::styleAttr rather than "style" here.
Comment on attachment 138577 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=138577&action=review > Source/WebCore/dom/ElementAttributeData.h:39 > +typedef Vector<Attribute> AttributeVector; I think you should get rid of the AttributeVector type completely. Vector<Attribute>is not any harder to write and way less opaque.
Created attachment 138582 [details] Patch
Created attachment 138594 [details] Patch
Comment on attachment 138594 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=138594&action=review r=me with these fixes: > Source/WebCore/dom/ElementAttributeData.h:39 > +inline Attribute* getAttributeFromVector(const Vector<Attribute>* attributes, const QualifiedName& name) - getAttributeFromVector doesn't sound very WebKit. findAttributeInVector would be slightly better. - The 'attributes' argument could be a const Vector<Attribute>&, that would remove the need for the ASSERT. - Also no need for the inline keyword.
Comment on attachment 138594 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=138594&action=review >> Source/WebCore/dom/ElementAttributeData.h:39 >> +inline Attribute* getAttributeFromVector(const Vector<Attribute>* attributes, const QualifiedName& name) > > - getAttributeFromVector doesn't sound very WebKit. findAttributeInVector would be slightly better. > - The 'attributes' argument could be a const Vector<Attribute>&, that would remove the need for the ASSERT. > - Also no need for the inline keyword. Yup, we use "get" prefix iff we have an out argument. > Source/WebCore/dom/ElementAttributeData.h:151 > +inline size_t ElementAttributeData::getAttributeItemIndex(const QualifiedName& name) const Ditto. > Source/WebCore/html/parser/HTMLTreeBuilder.cpp:571 > + if (name.matches(nameAttr) || name.matches(actionAttr) || name.matches(promptAttr)) { > + attributes.remove(i); > + --i; > + } Would it be better to remember the indexes and bulk remove? Modifying a vector while iterating over the vector might be too clever.
http://cdn.memegenerator.net/instances/400x/19307123.jpg awesomekling beats rniwa
Committed r115099: <http://trac.webkit.org/changeset/115099>
You appear to have broken the debug build :-( /Volumes/Data/git/WebKit/OpenSource/Source/WebCore/dom/ElementAttributeData.h:41:5: error: invalid argument type 'const Vector<WebCore::Attribute>' to unary expression ASSERT(attributes);
Comment on attachment 138594 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=138594&action=review > Source/WebCore/dom/ElementAttributeData.h:41 > + ASSERT(attributes); Reopen, because it broke the debug build: ../../../../Source/WebCore/dom/ElementAttributeData.h: In function ‘WebCore::Attribute* WebCore::findAttributeInVector(const WTF::Vector<WebCore::Attribute, 0ul>&, const WebCore::QualifiedName&)’: ../../../../Source/WebCore/dom/ElementAttributeData.h:41: error: no match for ‘operator!’ in ‘!attributes’ ../../../../Source/WebCore/dom/ElementAttributeData.h:41: note: candidates are: operator!(bool) <built-in>
Sorry folks. This should be fixed in http://trac.webkit.org/changeset/115102
(In reply to comment #16) > Sorry folks. This should be fixed in http://trac.webkit.org/changeset/115102 Not problem, but I have to share this with you :)) http://webkitmemes.tumblr.com/post/21640883992/scumbag-good-guy-awesomekling
Reopening since this is being rolled out. My blind fix stopped the crashing but left html5lib/runner.html with incorrect results.
Comment on attachment 138594 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=138594&action=review > Source/WebCore/xml/parser/MarkupTokenBase.h:542 > + const QualifiedName name = nameForAttribute(attribute); I think you mean to say: const QualifiedName& name
Created attachment 138838 [details] Patch
Created attachment 139457 [details] Patch for landing
Comment on attachment 139457 [details] Patch for landing Clearing flags on attachment: 139457 Committed r115645: <http://trac.webkit.org/changeset/115645>
All reviewed patches have been landed. Closing bug.