[webkit-reviews] review granted: [Bug 77800] Provide more attribute methods in Element : [Attachment 125480] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Caio Marcelo de Oliveira Filho
<cmarcelo at webkit.org>'s request for review:
Bug 77800: Provide more attribute methods in Element
https://bugs.webkit.org/show_bug.cgi?id=77800

Attachment 125480: Patch
https://bugs.webkit.org/attachment.cgi?id=125480&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
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.