[Webkit-unassigned] [Bug 51941] refactor HTMLLinkElement to allow Link header implementation

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=51941


Adam Barth <abarth at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #78023|review?                     |review-
               Flag|                            |




--- Comment #6 from Adam Barth <abarth at webkit.org>  2011-01-05 13:56:02 PST ---
(From update of attachment 78023)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.