Bug 25911 - Apply href in base elements to anchors shown on the source viewer
: Apply href in base elements to anchors shown on the source viewer
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: PC Windows Vista
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-05-21 00:23 PST by
Modified: 2009-05-31 19:37 PST (History)


Attachments
Proposed fix for 25911 (2.02 KB, patch)
2009-05-21 00:32 PST, Takeshi Yoshino
eric: review-
Review Patch | Details | Formatted Diff | Diff
Proposed fix for 25911 (rev2) (6.36 KB, patch)
2009-05-25 00:31 PST, Takeshi Yoshino
darin: review+
Review Patch | Details | Formatted Diff | Diff
Proposed fix for 25911 (rev3) (6.58 KB, patch)
2009-05-26 23:30 PST, Takeshi Yoshino
darin: review+
Review Patch | Details | Formatted Diff | Diff
Proposed fix for 25911 (rev4) (7.48 KB, patch)
2009-05-29 02:29 PST, Takeshi Yoshino
darin: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-05-21 00:23:43 PST
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 From 2009-05-21 00:32:16 PST -------
Created an attachment (id=30523) [details]
Proposed fix for 25911
------- Comment #2 From 2009-05-21 00:47:22 PST -------
(From update of attachment 30523 [details])
Hi,

Could someone review this fix?

Thanks,
------- Comment #3 From 2009-05-21 06:07:40 PST -------
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 From 2009-05-21 21:18:12 PST -------
(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 From 2009-05-21 22:54:14 PST -------
Are the described changes regressions or improvements?
------- Comment #6 From 2009-05-21 23:42:12 PST -------
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 From 2009-05-22 07:03:57 PST -------
(From update of attachment 30523 [details])
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 From 2009-05-25 00:31:29 PST -------
Created an attachment (id=30644) [details]
Proposed fix for 25911 (rev2)

Added a test following Eric's suggestion.
Thank you.
------- Comment #9 From 2009-05-26 09:16:28 PST -------
(From update of attachment 30644 [details])
> +                            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 From 2009-05-26 23:30:25 PST -------
Created an attachment (id=30697) [details]
Proposed fix for 25911 (rev3)
------- Comment #11 From 2009-05-26 23:39:14 PST -------
(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 From 2009-05-27 07:38:38 PST -------
(From update of attachment 30697 [details])
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 From 2009-05-29 02:29:30 PST -------
Created an attachment (id=30769) [details]
Proposed fix for 25911 (rev4)

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