Bug 25911

Summary: Apply href in base elements to anchors shown on the source viewer
Product: WebKit Reporter: Takeshi Yoshino <tyoshino>
Component: WebCore Misc.Assignee: David Levin <levin>
Status: RESOLVED FIXED    
Severity: Normal CC: ap
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows Vista   
Attachments:
Description Flags
Proposed fix for 25911
eric: review-
Proposed fix for 25911 (rev2)
darin: review+
Proposed fix for 25911 (rev3)
darin: review+
Proposed fix for 25911 (rev4) darin: review+

Description Takeshi Yoshino 2009-05-21 00:23:43 PDT
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
Comment 1 Takeshi Yoshino 2009-05-21 00:32:16 PDT
Created attachment 30523 [details]
Proposed fix for 25911
Comment 2 Takeshi Yoshino 2009-05-21 00:47:22 PDT
Comment on attachment 30523 [details]
Proposed fix for 25911

Hi,

Could someone review this fix?

Thanks,
Comment 3 Alexey Proskuryakov 2009-05-21 06:07:40 PDT
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?
Comment 4 Takeshi Yoshino 2009-05-21 21:18:12 PDT
(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).
Comment 5 Maciej Stachowiak 2009-05-21 22:54:14 PDT
Are the described changes regressions or improvements?

Comment 6 Takeshi Yoshino 2009-05-21 23:42:12 PDT
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.

Comment 7 Eric Seidel (no email) 2009-05-22 07:03:57 PDT
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.
Comment 8 Takeshi Yoshino 2009-05-25 00:31:29 PDT
Created attachment 30644 [details]
Proposed fix for 25911 (rev2)

Added a test following Eric's suggestion.
Thank you.
Comment 9 Darin Adler 2009-05-26 09:16:28 PDT
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
Comment 10 Takeshi Yoshino 2009-05-26 23:30:25 PDT
Created attachment 30697 [details]
Proposed fix for 25911 (rev3)
Comment 11 Takeshi Yoshino 2009-05-26 23:39:14 PDT
(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.

Comment 12 Darin Adler 2009-05-27 07:38:38 PDT
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
Comment 13 Takeshi Yoshino 2009-05-29 02:29:30 PDT
Created attachment 30769 [details]
Proposed fix for 25911 (rev4)

Modified so that we can check the code works case-insensitively.
Comment 14 David Levin 2009-05-29 16:06:53 PDT
Committed as http://trac.webkit.org/changeset/44278.
Comment 15 Takeshi Yoshino 2009-05-31 19:37:08 PDT
Thank you all.