Bug 77800

Summary: Provide more attribute methods in Element
Product: WebKit Reporter: Caio Marcelo de Oliveira Filho <cmarcelo>
Component: New BugsAssignee: Caio Marcelo de Oliveira Filho <cmarcelo>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, dglazkov, gns, kling, macpherson, menard, ossy, rniwa, tkent, webkit.review.bot, xan.lopez, zoltan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 75069, 77674    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch rniwa: review+

Description Caio Marcelo de Oliveira Filho 2012-02-03 19:51:19 PST
Provide more attribute methods in Element
Comment 1 Caio Marcelo de Oliveira Filho 2012-02-03 19:56:52 PST
Created attachment 125468 [details]
Patch
Comment 2 Caio Marcelo de Oliveira Filho 2012-02-03 20:03:18 PST
Created attachment 125470 [details]
Patch
Comment 3 Early Warning System Bot 2012-02-03 20:29:39 PST
Comment on attachment 125470 [details]
Patch

Attachment 125470 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11420609
Comment 4 WebKit Review Bot 2012-02-03 20:34:09 PST
Comment on attachment 125470 [details]
Patch

Attachment 125470 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11418636
Comment 5 Gyuyoung Kim 2012-02-03 20:34:42 PST
Comment on attachment 125470 [details]
Patch

Attachment 125470 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11420610
Comment 6 Caio Marcelo de Oliveira Filho 2012-02-03 20:36:11 PST
Comment on attachment 125470 [details]
Patch

Gathered some feedback on IRC. WIll make an updated version of the patch.
Comment 7 Gustavo Noronha (kov) 2012-02-03 20:37:41 PST
Comment on attachment 125470 [details]
Patch

Attachment 125470 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11419595
Comment 8 Caio Marcelo de Oliveira Filho 2012-02-03 21:52:57 PST
Created attachment 125480 [details]
Patch
Comment 9 Ryosuke Niwa 2012-02-03 22:43:00 PST
Comment on attachment 125480 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=125480&action=review

> Source/WebCore/ChangeLog:20
> +        No new tests. (OOPS!)

Nit: Remove this line.

> Source/WebCore/editing/EditingStyle.cpp:836
> +    // FIXME: Can it really ignore invalid style or animation svg attributes?
> +    if (!element->hasAttributesWithoutUpdate())

This is definitely not right. Could you just change it to hasAttributes()?

> Source/WebCore/editing/markup.cpp:109
> +            unsigned length = e->attributeCount();

Isn't e->attributeCount() inlined now? Maybe we don't this local variable anymore.

> Source/WebCore/html/parser/HTMLFormattingElementList.cpp:43
> +static inline size_t safeAttributeCount(Element* element)

I don't think "safe" is appropriate adjective here. I'd call this function attributeCountWithoutUpdate instead.

> Source/WebCore/inspector/DOMEditor.cpp:165
> +            while (oldElement->attributeCount())
> +                oldElement->removeAttribute(0);

We don't have some method to remove all attributes? This looks like the most inefficient thing we can do here.

> Source/WebCore/inspector/DOMEditor.cpp:173
> +            size_t numAttrs = newElement->attributeCount();
>              for (size_t i = 0; i < numAttrs; ++i) {
> -                const Attribute* attribute = newAttributeMap->attributeItem(i);
> +                const Attribute* attribute = newElement->attributeItem(i);
>                  oldElement->setAttribute(attribute->name(), attribute->value());
>              }

We should have an Element method for this.
Comment 10 Caio Marcelo de Oliveira Filho 2012-02-05 12:38:37 PST
Comment on attachment 125480 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=125480&action=review

Thanks for the comments Rniwa. I have some questions:

>> Source/WebCore/editing/markup.cpp:109
>> +            unsigned length = e->attributeCount();
> 
> Isn't e->attributeCount() inlined now? Maybe we don't this local variable anymore.

Should I inline other similar cases in this patch as well?

>> Source/WebCore/inspector/DOMEditor.cpp:165
>> +                oldElement->removeAttribute(0);
> 
> We don't have some method to remove all attributes? This looks like the most inefficient thing we can do here.

Could the will/didModifyAttribute calls affect the number of attributes (lead to another attribute addition or removal)? In this case this approach is less worse than you think. I worked with this assumption since I wasn't familiar with the code underneat.

Regardless of that, I do agree we can wrap this in a function and at least improve to be equivalent to: "while (hasAttributes) removeAttribute(attributeCount() -1);".
Comment 11 Ryosuke Niwa 2012-02-05 12:43:38 PST
(In reply to comment #10)
> (From update of attachment 125480 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=125480&action=review
> >> Source/WebCore/editing/markup.cpp:109
> >> +            unsigned length = e->attributeCount();
> > 
> > Isn't e->attributeCount() inlined now? Maybe we don't this local variable anymore.
> 
> Should I inline other similar cases in this patch as well?

Maybe. It's your call.

> >> Source/WebCore/inspector/DOMEditor.cpp:165
> >> +                oldElement->removeAttribute(0);
> > 
> > We don't have some method to remove all attributes? This looks like the most inefficient thing we can do here.
> 
> Could the will/didModifyAttribute calls affect the number of attributes (lead to another attribute addition or removal)? In this case this approach is less worse than you think. I worked with this assumption since I wasn't familiar with the code underneat.

That's a good point.

> Regardless of that, I do agree we can wrap this in a function and at least improve to be equivalent to: "while (hasAttributes) removeAttribute(attributeCount() -1);".

Yeah, wrapping these logics in some helper function will help us understand various assumptions in a more centralized way.
Comment 12 Ryosuke Niwa 2012-02-05 12:44:16 PST
(In reply to comment #11)
> Yeah, wrapping these logics in some helper function will help us understand various assumptions in a more centralized way.

Note I'm just suggesting this for a follow up, not to be merged with this patch.
Comment 13 Caio Marcelo de Oliveira Filho 2012-02-05 16:30:51 PST
Created attachment 125541 [details]
Patch
Comment 14 Caio Marcelo de Oliveira Filho 2012-02-05 16:39:15 PST
Rebased version incorporating some of rniwa suggestions. Let's see if the EWS are fine with it.

Regarding the attributeCount(), I think is a bit soon to assume this function will be always just an accessor to a member variable. Since "attribute count" conceptually depends on lazily updated data (the invalid attributes), we may want in the future to make attributeCount() make sure the data is updated.

Ryosuke, could you take another look?
Comment 15 WebKit Review Bot 2012-02-05 17:34:39 PST
Comment on attachment 125541 [details]
Patch

Attachment 125541 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11432078

New failing tests:
inspector/elements/set-outer-html-2.html
inspector/elements/set-outer-html.html
Comment 16 Caio Marcelo de Oliveira Filho 2012-02-06 06:24:07 PST
Created attachment 125628 [details]
Patch
Comment 17 Caio Marcelo de Oliveira Filho 2012-02-06 06:30:43 PST
Created attachment 125630 [details]
Patch
Comment 18 Caio Marcelo de Oliveira Filho 2012-02-06 06:31:08 PST
(In reply to comment #15)
> (From update of attachment 125541 [details])
> Attachment 125541 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/11432078
> 
> New failing tests:
> inspector/elements/set-outer-html-2.html
> inspector/elements/set-outer-html.html

I've tried to reuse existing logic of Element::setAttributesFromElement() to avoid manually copying, but its code inside NamedNodeMap doesn't take Inspector into account (will/didModifyAttribute). I prefer to handle this as a separate change too. So the current patch includes a FIXME for this case too.
Comment 19 Ryosuke Niwa 2012-02-06 11:03:21 PST
Comment on attachment 125630 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=125630&action=review

> Source/WebCore/dom/Element.h:135
> +    // This variant will not update the potentially invalid attributes. To be used when not interested
> +    // in style attribute or one of the SVG animation attributes.
> +    bool hasAttributesWithoutUpdate() const;

Instead of adding comments, should we rename the function to hasAttributesWithoutUpdatingStyleAndSVGAnimations()?

> Source/WebCore/dom/Element.h:160
> +    // Internal methods that assume the existence of attribute storage, one should use hasAttributes()
> +    // before calling them.
> +    size_t attributeCount() const;
> +    Attribute* attributeItem(unsigned index) const;
> +    Attribute* getAttributeItem(const QualifiedName&) const;
> +    void removeAttribute(unsigned index);

Can we eventually make them private/protected?

> Source/WebCore/xml/XPathNodeSet.cpp:216
> +        Element* e = toElement(n);

Please spell-out element.
Comment 20 Caio Marcelo de Oliveira Filho 2012-02-06 12:50:41 PST
Committed r106833: <http://trac.webkit.org/changeset/106833>
Comment 21 Caio Marcelo de Oliveira Filho 2012-02-06 12:53:04 PST
(In reply to comment #19)
> (From update of attachment 125630 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=125630&action=review
> 
> > Source/WebCore/dom/Element.h:135
> > +    // This variant will not update the potentially invalid attributes. To be used when not interested
> > +    // in style attribute or one of the SVG animation attributes.
> > +    bool hasAttributesWithoutUpdate() const;
> 
> Instead of adding comments, should we rename the function to hasAttributesWithoutUpdatingStyleAndSVGAnimations()?

We had a follow up discussion in IRC and I created bug 77892, adding Darin to the CC.


> > Source/WebCore/dom/Element.h:160
> > +    // Internal methods that assume the existence of attribute storage, one should use hasAttributes()
> > +    // before calling them.
> > +    size_t attributeCount() const;
> > +    Attribute* attributeItem(unsigned index) const;
> > +    Attribute* getAttributeItem(const QualifiedName&) const;
> > +    void removeAttribute(unsigned index);
> 
> Can we eventually make them private/protected?

Ack. I'll keep this in mind.


> > Source/WebCore/xml/XPathNodeSet.cpp:216
> > +        Element* e = toElement(n);
> 
> Please spell-out element.

Fixed.
Comment 22 Csaba Osztrogon√°c 2012-02-07 07:36:03 PST
(In reply to comment #20)
> Committed r106833: <http://trac.webkit.org/changeset/106833>

Reopen, because it broke the Mac builds.
Comment 23 Csaba Osztrogon√°c 2012-02-07 07:59:25 PST
Sorry, but it is absolutely innocent, r106932 is the culprit.
( r106932 and r106833 are so similar and they modified the problematic file :) )