Bug 84413 - Use Vector<Attribute> directly instead of encapsulating it in AttributeVector
Summary: Use Vector<Attribute> directly instead of encapsulating it in AttributeVector
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Caio Marcelo de Oliveira Filho
URL:
Keywords:
Depends on: 83440 84809
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-19 19:32 PDT by Caio Marcelo de Oliveira Filho
Modified: 2012-04-30 09:57 PDT (History)
6 users (show)

See Also:


Attachments
Patch (19.39 KB, patch)
2012-04-19 19:52 PDT, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch (8.78 KB, patch)
2012-04-24 09:26 PDT, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch (8.48 KB, patch)
2012-04-24 09:51 PDT, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch (19.39 KB, patch)
2012-04-24 10:45 PDT, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch (19.45 KB, patch)
2012-04-25 10:34 PDT, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch for landing (19.43 KB, patch)
2012-04-30 08:45 PDT, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Caio Marcelo de Oliveira Filho 2012-04-19 19:32:55 PDT
Use a typedef instead of inheriting WTF::Vector for AttributeVector
Comment 1 Caio Marcelo de Oliveira Filho 2012-04-19 19:52:23 PDT
Created attachment 138031 [details]
Patch
Comment 2 Caio Marcelo de Oliveira Filho 2012-04-19 19:54:32 PDT
Trying to address comment https://bugs.webkit.org/show_bug.cgi?id=79963#c16.
Comment 3 Antti Koivisto 2012-04-19 20:00:12 PDT
Better wait until more complex bug 83440 lands to minimize disruption.
Comment 4 Caio Marcelo de Oliveira Filho 2012-04-20 09:44:43 PDT
Ack.
Comment 5 Caio Marcelo de Oliveira Filho 2012-04-24 09:26:49 PDT
Created attachment 138577 [details]
Patch
Comment 6 Andreas Kling 2012-04-24 09:41:46 PDT
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 7 Antti Koivisto 2012-04-24 09:48:52 PDT
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.
Comment 8 Caio Marcelo de Oliveira Filho 2012-04-24 09:51:14 PDT
Created attachment 138582 [details]
Patch
Comment 9 Caio Marcelo de Oliveira Filho 2012-04-24 10:45:57 PDT
Created attachment 138594 [details]
Patch
Comment 10 Andreas Kling 2012-04-24 10:51:52 PDT
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 11 Ryosuke Niwa 2012-04-24 10:54:12 PDT
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.
Comment 12 Ryosuke Niwa 2012-04-24 11:04:11 PDT
http://cdn.memegenerator.net/instances/400x/19307123.jpg
awesomekling beats rniwa
Comment 13 Caio Marcelo de Oliveira Filho 2012-04-24 12:59:38 PDT
Committed r115099: <http://trac.webkit.org/changeset/115099>
Comment 14 Oliver Hunt 2012-04-24 13:18:25 PDT
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 15 Csaba Osztrogonác 2012-04-24 13:41:59 PDT
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>
Comment 16 Caio Marcelo de Oliveira Filho 2012-04-24 13:46:18 PDT
Sorry folks. This should be fixed in http://trac.webkit.org/changeset/115102
Comment 17 Csaba Osztrogonác 2012-04-24 13:48:07 PDT
(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
Comment 18 Andreas Kling 2012-04-24 17:20:43 PDT
Reopening since this is being rolled out. My blind fix stopped the crashing but left html5lib/runner.html with incorrect results.
Comment 19 Darin Adler 2012-04-24 17:51:59 PDT
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
Comment 20 Caio Marcelo de Oliveira Filho 2012-04-25 10:34:59 PDT
Created attachment 138838 [details]
Patch
Comment 21 Caio Marcelo de Oliveira Filho 2012-04-30 08:45:26 PDT
Created attachment 139457 [details]
Patch for landing
Comment 22 WebKit Review Bot 2012-04-30 09:57:32 PDT
Comment on attachment 139457 [details]
Patch for landing

Clearing flags on attachment: 139457

Committed r115645: <http://trac.webkit.org/changeset/115645>
Comment 23 WebKit Review Bot 2012-04-30 09:57:40 PDT
All reviewed patches have been landed.  Closing bug.