[webkit-reviews] review denied: [Bug 98184] HTMLBaseElement href attribute binding returns wrong URL : [Attachment 166734] Change to use fastGetAttribute

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Erik Arvidsson <arv at chromium.org>'s
request for review:
Bug 98184: HTMLBaseElement href attribute binding returns wrong URL
https://bugs.webkit.org/show_bug.cgi?id=98184

Attachment 166734: Change to use fastGetAttribute
https://bugs.webkit.org/attachment.cgi?id=166734&action=review

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


> Source/WebCore/html/HTMLBaseElement.cpp:82
> +String HTMLBaseElement::href() const
> +{
> +    return
document()->completeURL(stripLeadingAndTrailingHTMLSpaces(fastGetAttribute(href
Attr)), document()->url());
> +}

How does this compile? completeURL returns a KURL but this function returns a
String.

This needs a comment explaining how it differs from the getURLAttribute
function. It’s not enough to have it in the change log.

> Source/WebCore/html/HTMLBaseElement.cpp:84
> +void HTMLBaseElement::setHref(const String& v)

Please use words rather than single letters for argument names.

> Source/WebCore/html/HTMLBaseElement.h:35
> +    void setHref(const String&);

Type here should be const AtomicString& rather than const String&, because the
former is the type that setAttribute takes. The code generated by [Reflect]
correctly uses AtomicString.