[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





--- Comment #7 from Gavin Peters <gavinp at chromium.org>  2011-01-05 14:08:06 PST ---
Thanks for the review, Adam!

In my reply, I'm deleting nits I'll address on my next upload, and only leaving in questions from you that get an answer other than "Done."


(In reply to comment #6)
>
> > 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.

Yes.   This was a bug fix I copied in from r74995; if none of those tests touch this, I'll add one.

> > WebCore/loader/LinkLoader.cpp:56
> > +    , m_node()
> > +    , m_document()
> 
> You should be able to initialize this unconditionally, right?

I think so.  I'm still waffling about how to handle the m_node* in the LinkLoader: not always present, but needed in CSSStyleSheet::create (called twice).  Perhaps a method in LinkLoaderClient that returns a Node*, or calls CSSStyleSheet::create itself?  This is a bigger deal if we add stylesheet rel types.

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

I know.

> > 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.

Will do, as it is puzzling.

> 
> > 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?

I think only in the case of linkInDocument().  Each of linkBeforeLoad, linkLoaded and linkError may not be called (say in the Link header case, I'm thinking the first rev doesn't put JS in the HTTP headers at least...), and so a reasonable default makes sense.  linkInDocument not being pure though is just sloppiness on my part.

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