Bug 25911 – Apply href in base elements to anchors shown on the source viewer

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Takeshi Yoshino]

In rendering HTML sources, parse base elements to apply the base URI to anchors shown on the source viewer.

This issue was originally reported to the Chromium issue tracker.
http://code.google.com/p/chromium/issues/detail?id=2418

[Takeshi Yoshino]

Created attachment 30523 [details]
Proposed fix for 25911

[Takeshi Yoshino]

Comment on attachment 30523 [details]
Proposed fix for 25911

Hi,

Could someone review this fix?

Thanks,

[Alexey Proskuryakov]

Even after running WebKit with this patch applied, I don't see any changes in Web Inspector. Is this supposed to do something for Safari, or only for Chrome?

[Takeshi Yoshino]

(In reply to comment #3)
> Even after running WebKit with this patch applied, I don't see any changes in
> Web Inspector. Is this supposed to do something for Safari, or only for Chrome?
> 

Thank you for review.

We can click URLs of anchors' href values in Chrome's source viewer. Not inspector. It uses HTMLViewSourceDocument to render sources.

I didn't intended to improve something in Safari and I didn't know where HTMLViewSourceDocument is used in Safari. But I found that the resources tab in Safari Web Inspector also uses HTMLViewSourceDocument (Element tab doesn't) after your comment. Yes, there must be some change on the behavior of anchors in the resource tab when I view http://backup.gene-it.nl/welcome/persoonsgegevens since this document contains a base element.

Actually, after patching this fix, the result of "Copy Link" changes. For example, the result for gene_global.css change from file:///Users/testing/webkit_trunk/WebKitBuild/Release/WebCore.framework/Resources/inspector/gene_global.css to http://backup.gene-it.nl/gene_global.css . This is a side effect of my patch.

It's a new bug that we cannot "Copy Link" on the resource tab correctly (file:/// ... /inspector returns).

[Maciej Stachowiak]

Are the described changes regressions or improvements?

[Takeshi Yoshino]

It's not a regression. It's a little improvement.

- For anchors with RELATIVE URLs in its href attribute on the Web Inspector
-- For documents with base elements
--- This is improvement not regression. We will be able to click them to switch to the resource in resource viewer and copy correct URLs from them.
-- For documents with no base element
--- This is neither improvement nor regression. There is no change on behavior.

- For anchors with ABSOLUTE URLs in its href attribute on the Web Inspector:
-- This is neither improvement nor regression. Anchors are now working well and still work well with this patch.

[Eric Seidel (no email)]

Comment on attachment 30523 [details]
Proposed fix for 25911

You should be able to make a test case using a viewsource attribute.

<iframe viewsource src="data:text/html,<p>hello world</p>"></iframe>

see fast/frames/viewsource-attribute.html

r- for lack of test case.

[Takeshi Yoshino]

Created attachment 30644 [details]
Proposed fix for 25911 (rev2)

Added a test following Eric's suggestion.
Thank you.

[Darin Adler]

Comment on attachment 30644 [details]
Proposed fix for 25911 (rev2)

> +                            if (token->tagName == baseTag && equalIgnoringCase(attr->name().localName(), "href")) {

I'm not sure why the code in this class checks attribute names in this inefficient way. In normal WebCore code we would just compare with hrefAttr using == instead.

r=me

[Takeshi Yoshino]

Created attachment 30697 [details]
Proposed fix for 25911 (rev3)

[Takeshi Yoshino]

(In reply to comment #9)
> (From update of attachment 30644 [details] [review])
> > +                            if (token->tagName == baseTag && equalIgnoringCase(attr->name().localName(), "href")) {
> 
> I'm not sure why the code in this class checks attribute names in this
> inefficient way. In normal WebCore code we would just compare with hrefAttr
> using == instead.
> 
> r=me
> 

Thank you. After your comment, I investigated and understood the reason. In order to show HTML sources as is on the view-source window, HTMLTokenizer doesn't lower names when inViewSourceMode() is true. So, we have to lower names before comparison in HTMLViewSourceDocument class. Patch revision 2 was comparing tagName and baseTag case sensitively, so I updated to revision 3. Please take a look again.

[Darin Adler]

Comment on attachment 30697 [details]
Proposed fix for 25911 (rev3)

I think the test case should try both lowercase and uppercase, since the code has to be written carefully to be case insensitive.

But even without that, r=me

[Takeshi Yoshino]

Created attachment 30769 [details]
Proposed fix for 25911 (rev4)

Modified so that we can check the code works case-insensitively.

[David Levin]

Committed as http://trac.webkit.org/changeset/44278.

[Takeshi Yoshino]

Thank you all.