Bug 77800 – Provide more attribute methods in Element

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

[Caio Marcelo de Oliveira Filho]

Provide more attribute methods in Element

[Caio Marcelo de Oliveira Filho]

Created attachment 125468 [details]
Patch

[Caio Marcelo de Oliveira Filho]

Created attachment 125470 [details]
Patch

[Early Warning System Bot]

Comment on attachment 125470 [details]
Patch

Attachment 125470 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11420609

[WebKit Review Bot]

Comment on attachment 125470 [details]
Patch

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

[Gyuyoung Kim]

Comment on attachment 125470 [details]
Patch

Attachment 125470 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11420610

[Caio Marcelo de Oliveira Filho]

Comment on attachment 125470 [details]
Patch

Gathered some feedback on IRC. WIll make an updated version of the patch.

[Gustavo Noronha (kov)]

Comment on attachment 125470 [details]
Patch

Attachment 125470 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11419595

[Caio Marcelo de Oliveira Filho]

Created attachment 125480 [details]
Patch

[Ryosuke Niwa]

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.

[Caio Marcelo de Oliveira Filho]

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

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

(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.

[Caio Marcelo de Oliveira Filho]

Created attachment 125541 [details]
Patch

[Caio Marcelo de Oliveira Filho]

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?

[WebKit Review Bot]

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

[Caio Marcelo de Oliveira Filho]

Created attachment 125628 [details]
Patch

[Caio Marcelo de Oliveira Filho]

Created attachment 125630 [details]
Patch

[Caio Marcelo de Oliveira Filho]

(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.

[Ryosuke Niwa]

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.

[Caio Marcelo de Oliveira Filho]

Committed r106833: <http://trac.webkit.org/changeset/106833>

[Caio Marcelo de Oliveira Filho]

(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.

[Csaba Osztrogonác]

(In reply to comment #20)
> Committed r106833: <http://trac.webkit.org/changeset/106833>

Reopen, because it broke the Mac builds.

[Csaba Osztrogonác]

Sorry, but it is absolutely innocent, r106932 is the culprit.
( r106932 and r106833 are so similar and they modified the problematic file :) )