Bug 113630 - HTMLLinkElement should resolve resource URLs when resources will be fetched
Summary: HTMLLinkElement should resolve resource URLs when resources will be fetched
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rafael Weinstein
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-29 17:15 PDT by Rafael Weinstein
Modified: 2013-03-31 16:25 PDT (History)
8 users (show)

See Also:

Attachments
Patch (4.87 KB, patch)
2013-03-29 17:19 PDT, Rafael Weinstein 		no flags Details | Formatted Diff | Diff
Patch (8.35 KB, patch)
2013-03-29 17:24 PDT, Rafael Weinstein 		no flags Details | Formatted Diff | Diff
Patch (8.34 KB, patch)
2013-03-30 12:35 PDT, Rafael Weinstein 		no flags Details | Formatted Diff | Diff
Patch (7.82 KB, patch)
2013-03-30 13:20 PDT, Rafael Weinstein 		no flags Details | Formatted Diff | Diff
Patch for landing (7.82 KB, patch)
2013-03-30 13:21 PDT, Rafael Weinstein 		no flags Details | Formatted Diff | Diff
Patch for landing (7.82 KB, patch)
2013-03-30 17:28 PDT, Rafael Weinstein 		no flags Details | Formatted Diff | Diff
Show Obsolete (5) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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.