[webkit-reviews] review denied: [Bug 71510] [Microdata] LayoutTests/fast/dom/MicroData/itemid-attribute-test.html assertion failure in Element::getURLAttribute() : [Attachment 113652] Updated patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Arko Saha <nghq36 at motorola.com>'s
request for review:
Bug 71510: [Microdata]
LayoutTests/fast/dom/MicroData/itemid-attribute-test.html assertion failure in
Element::getURLAttribute()
https://bugs.webkit.org/show_bug.cgi?id=71510

Attachment 113652: Updated patch
https://bugs.webkit.org/attachment.cgi?id=113652&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=113652&action=review


This is the wrong way to do it. We don’t want to put an #if into all these
places that are not microdata-related.

And we need to add a new test that covers all these different types of
elements. The existing test does not cover enough.

> Source/WebCore/html/HTMLAnchorElement.cpp:244
> +#if ENABLE(MICRODATA)
> +    return attr->name() == hrefAttr || attr->name() == itemidAttr;
> +#else
>      return attr->name() == hrefAttr;
> +#endif

This code should say:

    return attr->name() == hrefAttr || HTMLElement::isURLAttribute(attr);

Since HTMLAnchorElement’s immediate base class is HTMLElement.

No #if.

Same for all the other functions, but please make sure to use the appropriate
base class and not just call straight to HTMLElement each time.