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
Takeshi Yoshino
2009-05-21 00:23:43 PDT
Created attachment 30523 [details]
Proposed fix for 25911
Comment on attachment 30523 [details]
Proposed fix for 25911
Hi,
Could someone review this fix?
Thanks,
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? (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). Are the described changes regressions or improvements? 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 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.
Created attachment 30644 [details]
Proposed fix for 25911 (rev2)
Added a test following Eric's suggestion.
Thank you.
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 Created attachment 30697 [details]
Proposed fix for 25911 (rev3)
(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 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
Created attachment 30769 [details]
Proposed fix for 25911 (rev4)
Modified so that we can check the code works case-insensitively.
Committed as http://trac.webkit.org/changeset/44278. Thank you all. |