Bug 56384 – XML viewer is not shown when frame has non-null opener

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Vsevolod Vlasov]

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).

[Alexey Proskuryakov]

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.

[Vsevolod Vlasov]

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>

[Alexey Proskuryakov]

Yes, I understand. Is there any use case suggesting that displaying a tree in those cases is a good thing?

[Vsevolod Vlasov]

For example it happens when you open a link in some search engines.

[Alexey Proskuryakov]

This is a counter-example, in my opinion. Clicking on a search result should never display developer-only information such as the XML tree.

[Adam Barth]

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.

[Alexey Proskuryakov]

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.

[Adam Barth]

(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.

[Adam Barth]

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.

[Alexey Proskuryakov]

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.

[Adam Barth]

> 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.

[Pavel Feldman]

> 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.

[Vsevolod Vlasov]

As discussed before xml viewer is already hidden behind developerExtras flag.

[Pavel Feldman]

(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?

[Alexey Proskuryakov]

Just one consideration: it could be damaging to web developers to see regular Web content differently than users do.

[Pavel Feldman]

(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.

[Vsevolod Vlasov]

Created attachment 87337 [details]
Patch

[Pavel Feldman]

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?

[Vsevolod Vlasov]

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.

[Vsevolod Vlasov]

Created attachment 87735 [details]
New patch

[WebKit Commit Bot]

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

[Vsevolod Vlasov]

Created attachment 87739 [details]
Patch with fixed changelog

[WebKit Commit Bot]

Comment on attachment 87739 [details]
Patch with fixed changelog

Clearing flags on attachment: 87739

Committed r82604: <http://trac.webkit.org/changeset/82604>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

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