Bug 84413 – Use Vector<Attribute> directly instead of encapsulating it in AttributeVector

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Caio Marcelo de Oliveira Filho]

Use a typedef instead of inheriting WTF::Vector for AttributeVector

[Caio Marcelo de Oliveira Filho]

Created attachment 138031 [details]
Patch

[Caio Marcelo de Oliveira Filho]

Trying to address comment https://bugs.webkit.org/show_bug.cgi?id=79963#c16.

[Antti Koivisto]

Better wait until more complex bug 83440 lands to minimize disruption.

[Caio Marcelo de Oliveira Filho]

Ack.

[Caio Marcelo de Oliveira Filho]

Created attachment 138577 [details]
Patch

[Andreas Kling]

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.

[Antti Koivisto]

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.

[Caio Marcelo de Oliveira Filho]

Created attachment 138582 [details]
Patch

[Caio Marcelo de Oliveira Filho]

Created attachment 138594 [details]
Patch

[Andreas Kling]

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.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

http://cdn.memegenerator.net/instances/400x/19307123.jpg
awesomekling beats rniwa

[Caio Marcelo de Oliveira Filho]

Committed r115099: <http://trac.webkit.org/changeset/115099>

[Oliver Hunt]

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);

[Csaba Osztrogonác]

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>

[Caio Marcelo de Oliveira Filho]

Sorry folks. This should be fixed in http://trac.webkit.org/changeset/115102

[Csaba Osztrogonác]

(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

[Andreas Kling]

Reopening since this is being rolled out. My blind fix stopped the crashing but left html5lib/runner.html with incorrect results.

[Darin Adler]

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

[Caio Marcelo de Oliveira Filho]

Created attachment 138838 [details]
Patch

[Caio Marcelo de Oliveira Filho]

Created attachment 139457 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 139457 [details]
Patch for landing

Clearing flags on attachment: 139457

Committed r115645: <http://trac.webkit.org/changeset/115645>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.