[webkit-reviews] review denied: [Bug 51941] refactor HTMLLinkElement to allow Link header implementation : [Attachment 78023] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Gavin Peters <gavinp at chromium.org>'s
request for review:
Bug 51941: refactor HTMLLinkElement to allow Link header implementation
https://bugs.webkit.org/show_bug.cgi?id=51941

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=78023&action=review

Looks like a great start.  Some comments below.  Mostly nits / questions about
existing code.

> WebCore/html/HTMLLinkElement.cpp:55
> +    , m_linkLoader(*this, document, this)

|*this| ?  We'd usually just pass |this|.

> WebCore/html/HTMLLinkElement.cpp:152
> +    RefPtr<Document> originalDocument = document();
> +    if (!dispatchBeforeLoadEvent(href))
> +	   return false;
> +    // A beforeload handler might have removed us from the document or
changed the document.
> +    if (!inDocument() || document() != originalDocument)
> +	   return false;
> +    return true;

We should definitely test these conditions.

> WebCore/html/HTMLLinkElement.cpp:178
> +

extra space

> WebCore/html/parser/HTMLPreloadScanner.cpp:35
>  #include "HTMLLinkElement.h"

Can we remove this include?

> WebCore/loader/LinkLoader.cpp:53
> +LinkLoader::LinkLoader(LinkLoaderClient& client, Document* doc,
ContainerNode* node)

LinkLoaderClient& => LinkLoaderClient*

We usually hold pointers to clients.

doc => document.

> WebCore/loader/LinkLoader.cpp:56
> +    , m_node()
> +    , m_document()

You should be able to initialize this unconditionally, right?

> WebCore/loader/LinkLoader.cpp:77
> +    if (m_cachedSheet) {
> +	   m_cachedSheet->removeClient(this);
> +    if (isLoading() && !isDisabled() && !isAlternate())
> +	   document()->removePendingSheet();
> +    }

Bad indent.  Tab?

> WebCore/loader/LinkLoader.cpp:89
> +

Extra blank line.

> WebCore/loader/LinkLoader.cpp:108
> +	   if (baseURL.string().contains("mcafee.com/japan/", false))
> +	       enforceMIMEType = false;

OMG

> WebCore/loader/LinkLoader.cpp:172
> +    relAttribute.m_isStyleSheet = false;
> +    relAttribute.m_isIcon = false;
> +    relAttribute.m_isAlternate = false;
> +    relAttribute.m_isDNSPrefetch = false;
> +#if ENABLE(LINK_PREFETCH)
> +    relAttribute.m_isLinkPrefetch = false;
> +#endif

Should this work be done in the RelAttribute constructor?

> WebCore/loader/LinkLoader.cpp:175
> +    else if (equalIgnoringCase(rel, "icon") || equalIgnoringCase(rel,
"shortcut icon"))

"shortcut icon" => really exactly one space between "shortcut" and "icon"?

> WebCore/loader/LinkLoader.cpp:183
> +    else if (equalIgnoringCase(rel, "alternate stylesheet") ||
equalIgnoringCase(rel, "stylesheet alternate")) {

Same question.	I realize you're just moving this code, but might be worth
investigating.

> WebCore/loader/LinkLoader.cpp:257
> +	   if (!m_client.linkBeforeLoad(m_url))
> +	     return;

Bad indent.

> WebCore/loader/LinkLoader.cpp:271
> +

Extra blank line.

> WebCore/loader/LinkLoader.cpp:279
> +	   if (m_cachedSheet)
> +	       m_cachedSheet->addClient(this);
> +	   else {

Prefer early return.

> WebCore/loader/LinkLoader.h:70
> +	       { 
> +	       }

Bad indent.

> WebCore/loader/LinkLoader.h:80
> +    //

Dangling comment.

> WebCore/loader/LinkLoader.h:99
> +    { m_title = title;

These should be on separate lines.  Also, this probably shouldn't be inline.

> WebCore/loader/LinkLoader.h:112
> +    //    virtual void parseMappedAttribute(Attribute*);

Please don't comment out code.

> WebCore/loader/LinkLoader.h:149
> +

extra blank line.

> WebCore/loader/LinkLoaderClient.h:48
> +    virtual bool linkBeforeLoad(const KURL&) { return true; }
> +
> +    virtual void linkLoaded() { }
> +    virtual void linkError() { }
> +
> +    virtual bool linkInDocument() { return true; }

Seems like this should be a pure virtual interface, right?