Bug 56384 - XML viewer is not shown when frame has non-null opener
Summary: XML viewer is not shown when frame has non-null opener
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-03-15 09:48 PDT by Vsevolod Vlasov
Modified: 2011-03-31 13:44 PDT (History)
8 users (show)

See Also:


Attachments
Patch (7.86 KB, patch)
2011-03-29 08:23 PDT, Vsevolod Vlasov
pfeldman: review-
Details | Formatted Diff | Diff
Patch with fixes (7.30 KB, patch)
2011-03-29 08:43 PDT, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
New patch (7.30 KB, patch)
2011-03-31 08:54 PDT, Vsevolod Vlasov
pfeldman: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Patch with fixed changelog (6.12 KB, patch)
2011-03-31 09:05 PDT, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vsevolod Vlasov 2011-03-15 09:48:16 PDT
Rendering of XML without style (https://bugs.webkit.org/show_bug.cgi?id=13807) is done only when frame opener is not set.
This is done to ensure that it is not possible to modify xml document after.

Adam Barth suggested that this check is not needed since it is not done in firefox anyway and this case should be extremely rare.
Quote:
> pfeldman and I discussed this in IRC.  Firefox survives without looking at the opener, 
> so hopefully we'll be able to as well.  The problem is > that the opener property isn't 
> dispositive w.r.t. whether someone else is going to interact with this document.  
> Any pages that have the behavior pfeldman is worried about are already broken in 
> Firefox, so it sounds like we should be ok breaking them as well (he's opening another bug on that topic).
Comment 1 Alexey Proskuryakov 2011-03-15 10:02:46 PDT
I'm not strongly opposed to this, but I question the benefits of making this change. What use case does displaying an XML tree in programmatically opened documents serve?

You are arguing that downsides are very small, but some benefit is needed to outweigh it.
Comment 2 Vsevolod Vlasov 2011-03-15 10:09:19 PDT
Opener is set suprisingly often:
 - every time you do window.open() from javascript (even with '_self' as a target,
 - when you click on a link with target other than '_self' (E.g. <a href="..." target="_blank">...</a>
Comment 3 Alexey Proskuryakov 2011-03-15 10:22:25 PDT
Yes, I understand. Is there any use case suggesting that displaying a tree in those cases is a good thing?
Comment 4 Vsevolod Vlasov 2011-03-15 10:26:44 PDT
For example it happens when you open a link in some search engines.
Comment 5 Alexey Proskuryakov 2011-03-15 10:43:32 PDT
This is a counter-example, in my opinion. Clicking on a search result should never display developer-only information such as the XML tree.
Comment 6 Adam Barth 2011-03-15 10:52:32 PDT
This came up yesterday when a (non-technical) friend of mine clicked a link on a blog to an RSS feed.  Because they were using a WebKit-based browser without an integrated RSS reader, they got a screen full of garbage text and were sad.  They would have been happier with a pretty tree view and a short description at the top explaining that the web site did not provide any presentational information with this data, which is what the new XML tree view does.
Comment 7 Alexey Proskuryakov 2011-03-15 11:28:06 PDT
A WebKit-based browser without an integrated RSS reader should probably provide a better experience in a case like that. It's out of scope for WebKit.

It's not clear if other non-technical users would be happier to see an XML tree view with a non-localized text blurb on top.
Comment 8 Adam Barth 2011-03-15 12:28:48 PDT
(In reply to comment #7)
> A WebKit-based browser without an integrated RSS reader should probably provide a better experience in a case like that. It's out of scope for WebKit.

Why is it out of scope for WebKit?

> It's not clear if other non-technical users would be happier to see an XML tree view with a non-localized text blurb on top.

That's an argument for localizing the text, which I think is a good idea.
Comment 9 Adam Barth 2011-03-15 12:31:01 PDT
In any case, looking at window.opener is a giant hack.  There should be either a compile-time or run-time setting to enable the XML viewer.  If enabled, the XML viewer should be used for all unstyled XML regardless of window.opener.
Comment 10 Alexey Proskuryakov 2011-03-15 13:15:32 PDT
What I'm saying is that developer oriented UI should never be inflicted on users. It's not right to drop into a debugger when you click on a search results link.

If Chrome wants to do that, please add an opt-in preference for that behavior.
Comment 11 Adam Barth 2011-03-15 13:26:20 PDT
> If Chrome wants to do that, please add an opt-in preference for that behavior.

Sounds like we agree that the XML viewer should be enabled/disabled by a runtime setting.
Comment 12 Pavel Feldman 2011-03-15 13:29:29 PDT
> Sounds like we agree that the XML viewer should be enabled/disabled by a runtime setting.

How about using existing "developersExtras" as the preference? Ironically (or not), it is on by default in Chrome as is enabled with Developer menu in Safari.
Comment 13 Vsevolod Vlasov 2011-03-15 13:34:45 PDT
As discussed before xml viewer is already hidden behind developerExtras flag.
Comment 14 Pavel Feldman 2011-03-15 13:37:02 PDT
(In reply to comment #13)
> As discussed before xml viewer is already hidden behind developerExtras flag.

Heh. Are we all good about nuking the opener check then? Alexey?
Comment 15 Alexey Proskuryakov 2011-03-15 13:52:47 PDT
Just one consideration: it could be damaging to web developers to see regular Web content differently than users do.
Comment 16 Pavel Feldman 2011-03-15 14:39:32 PDT
(In reply to comment #15)
> Just one consideration: it could be damaging to web developers to see regular Web content differently than users do.

To summarize: ap wants a runtime preference to opt-in. It will go the usual WebPreferences / Settings path. I see two options for the preference:

a) xmlViewer(Enabled) that controls xml viewer toggling at all times (no matter with or without opener)
b) xmlViewerForFramesWithOpener(Enabled) that controls xml viewer toggling where there is opener.

We will leave menu items implementation / title captions to the port owners. I think we'll have it on at all times for Chrome.
Comment 17 Vsevolod Vlasov 2011-03-29 08:23:59 PDT
Created attachment 87337 [details]
Patch
Comment 18 Pavel Feldman 2011-03-29 08:32:04 PDT
Comment on attachment 87337 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=87337&action=review

> Source/WebCore/xml/XMLViewer.xsl:268
> +                var domSubTreeModifiedListener;

This variable can be declared in the onWebKitXMLViewerLoad.

> Source/WebCore/xml/XMLViewer.xsl:274
> +                        if (!domSubTreeModifiedListener) {

Can the opposite happen?

> Source/WebCore/xml/XMLViewer.xsl:282
> +                    if (!window.onWebKitXMLViewerSourceXMLLoaded)

I don't follow the code. Can you explain what is happening in this load handler?
Comment 19 Vsevolod Vlasov 2011-03-29 08:43:09 PDT
Created attachment 87338 [details]
Patch with fixes

I removed this listener variable.

The other part is not relevant to this bug, so I removed it also.
Comment 20 Vsevolod Vlasov 2011-03-31 08:54:14 PDT
Created attachment 87735 [details]
New patch
Comment 21 WebKit Commit Bot 2011-03-31 09:01:19 PDT
Comment on attachment 87735 [details]
New patch

Rejecting attachment 87735 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'apply-..." exit_code: 2

Last 500 characters of output:
 to patch at input line 5
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|Index: Source/WebCore/xml/XMLViewer.xsl
|index 4ee3ca8..a2981cf 100644
|--- Source/WebCore/xml/XMLViewer.xsl
|+++ Source/WebCore/xml/XMLViewer.xsl
--------------------------
No file to patch.  Skipping patch.
1 out of 1 hunk ignored

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Pavel Feldman', u'--fo..." exit_code: 1

Full output: http://queues.webkit.org/results/8311357
Comment 22 Vsevolod Vlasov 2011-03-31 09:05:10 PDT
Created attachment 87739 [details]
Patch with fixed changelog
Comment 23 WebKit Commit Bot 2011-03-31 12:27:46 PDT
Comment on attachment 87739 [details]
Patch with fixed changelog

Clearing flags on attachment: 87739

Committed r82604: <http://trac.webkit.org/changeset/82604>
Comment 24 WebKit Commit Bot 2011-03-31 12:27:53 PDT
All reviewed patches have been landed.  Closing bug.
Comment 25 WebKit Review Bot 2011-03-31 13:44:23 PDT
http://trac.webkit.org/changeset/82604 might have broken SnowLeopard Intel Release (WebKit2 Tests)
The following tests are not passing:
http/tests/xmlhttprequest/response-encoding.html