[webkit-reviews] review denied: [Bug 71510] [Microdata] fast/dom/Microdata/itemid-attte-test.html assertion failure in Element::getURLAttribute() : [Attachment 113561] 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] fast/dom/Microdata/itemid-attte-test.html assertion
failure in Element::getURLAttribute()
https://bugs.webkit.org/show_bug.cgi?id=71510

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

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


> Source/WebCore/html/HTMLElement.cpp:997
> +bool HTMLElement::isURLAttribute(Attribute *attr) const

Formatting here is wrong. The * goes next to the word “Attribute”.

In new code we should not use abbreviations. This should be "attribute" instead
of "attr".

Should this be unconditional? I don’t think this is right if ENABLE(MICRODATA)
is not set.

> Source/WebCore/html/HTMLElement.h:128
> +    virtual bool isURLAttribute(Attribute*) const;

All the classes derived from HTMLElement that currently do not call through to
the base class need to do so, otherwise they will return false for itemid. That
includes HTMLAnchorElement::isURLAttribute, HTMLBaseElement::isURLAttribute,
HTMLBodyElement::isURLAttribute, HTMLButtonElement::isURLAttribute,
HTMLEmbedElement::isURLAttribute, HTMLFormElement::isURLAttribute,
HTMLFrameElementBase::isURLAttribute, HTMLHtmlElement::isURLAttribute,
HTMLIFrameElement::isURLAttribute (actually, that function should just be
removed, because HTMLFrameElementBase already takes care of it),
HTMLImageElement::isURLAttribute, HTMLInputElement::isURLAttribute,
HTMLLinkElement::isURLAttribute, HTMLMediaElement::isURLAttribute,
HTMLModElement::isURLAttribute, HTMLObjectElement::isURLAttribute,
HTMLParamElement::isURLAttribute, HTMLQuoteElement::isURLAttribute,
HTMLScriptElement::isURLAttribute, HTMLSourceElement::isURLAttribute,
HTMLTableCellElement::isURLAttribute, HTMLTableElement::isURLAttribute, and
HTMLTrackElement::isURLAttribute.

I also wonder if we need this itemid support for SVG elements too (separate
issue).