Bug 113630

Summary: HTMLLinkElement should resolve resource URLs when resources will be fetched
Product: WebKit Reporter: Rafael Weinstein <rafaelw>
Component: DOMAssignee: Rafael Weinstein <rafaelw>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, adamk, ericbidelman, eric, esprehn+autocc, ojan.autocc, ojan, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing
none
Patch for landing none

Description Rafael Weinstein 2013-03-29 17:15:28 PDT 
Currently it resolves urls when the href attribute is processed (either during parse or setAttribute).

This will result in an erroneous url if 

a) the document's base URL has changed since the href attribute was processed
b) the link was adopted from another document
Comment 1 Rafael Weinstein 2013-03-29 17:19:24 PDT 
Created attachment 195823 [details]
Patch
Comment 2 Rafael Weinstein 2013-03-29 17:24:27 PDT 
Created attachment 195825 [details]
Patch
Comment 3 Rafael Weinstein 2013-03-29 17:24:51 PDT 
this time with layout test attached.
Comment 4 Eric Seidel (no email) 2013-03-29 17:49:18 PDT 
Comment on attachment 195825 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=195825&action=review

> Source/WebCore/ChangeLog:6
> +         HTMLLinkElement was resolving its URL when the href attribute was processed and caching it without ever

Extra space.

> Source/WebCore/html/HTMLLinkElement.cpp:138
> +    String url = stripLeadingAndTrailingHTMLSpaces(href);
> +    return url.isEmpty() ? KURL() : document()->completeURL(url);

I'm surprised we need this ternary?  Since I would expect other palces to treat empty urls as urls to the document?  I guess this si so the later isValid() check works?

> Source/WebCore/html/HTMLLinkElement.h:108
> +    KURL getURL() const;

I believe webkit style is to avoid using "get" in getters.
Comment 5 Rafael Weinstein 2013-03-30 12:35:32 PDT 
Created attachment 195865 [details]
Patch
Comment 6 Eric Seidel (no email) 2013-03-30 12:45:24 PDT 
Comment on attachment 195865 [details]
Patch

I believe WebKIt style would have you rename "URL()" to "url()".  Otherwise this LGTM.
Comment 7 Rafael Weinstein 2013-03-30 13:20:18 PDT 
Created attachment 195867 [details]
Patch
Comment 8 Rafael Weinstein 2013-03-30 13:21:20 PDT 
Created attachment 195868 [details]
Patch for landing
Comment 9 Rafael Weinstein 2013-03-30 13:21:59 PDT 
just noticed getNonEmptyURLAttribute() on Element.cpp. using that instead.
Comment 10 Adam Barth 2013-03-30 14:24:04 PDT 
Comment on attachment 195868 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=195868&action=review

> Source/WebCore/html/HTMLLinkElement.cpp:180
> +    KURL url =  getNonEmptyURLAttribute(hrefAttr);

You've got an extra space after the = on this line.
Comment 11 Rafael Weinstein 2013-03-30 17:28:15 PDT 
Created attachment 195878 [details]
Patch for landing
Comment 12 Rafael Weinstein 2013-03-30 17:28:44 PDT 
Comment on attachment 195868 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=195868&action=review

>> Source/WebCore/html/HTMLLinkElement.cpp:180
>> +    KURL url =  getNonEmptyURLAttribute(hrefAttr);
> 
> You've got an extra space after the = on this line.

fixed.
Comment 13 WebKit Review Bot 2013-03-31 16:25:55 PDT 
Comment on attachment 195878 [details]
Patch for landing

Clearing flags on attachment: 195878

Committed r147291: <http://trac.webkit.org/changeset/147291>
Comment 14 WebKit Review Bot 2013-03-31 16:25:59 PDT 
All reviewed patches have been landed.  Closing bug.