1439021 - Fix fallout from bug 1418403: Remove viewing source in a stand-alone window (view source not working)

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Jorg K (CEST = GMT+2)]

Bug 1418403 has landed, so it's time to act.

Comment 1

[Jorg K (CEST = GMT+2)]

Related to bug 1163707, I suppose.

Comment 2

[:aceman]

(In reply to Jorg K (GMT+1) from comment #1)
> Related to bug 1163707, I suppose.

I don't think so, we are migrated. The last users of deprecated APIs are in m-c.

Comment 3

[:aceman]

In the current c-c, attempt to open View source produces this:

XML Parsing Error: undefined entity
Location: chrome://global/content/viewSource.xul
Line Number 17, Column 1:<window id="viewSource"
^

Comment 4

[Jorg K (CEST = GMT+2)]

Well, I see
XML Parsing Error: undefined entity
Location: jar:file:///C:/Temp/2018-02-17/thunderbird/omni.ja!/chrome/toolkit/content/global/netError.xhtml
Line Number 313, Column 41: netError.xhtml:313:41
(see bug 1439032)

and when clicking in the link to the file I get:
TypeError: window.top.openUILinkIn is not a function viewSourceUtils.js:73:5

So I suppose we could make the second one work by supplying that function.

Comment 5

[Jorg K (CEST = GMT+2)]

With bug 1439032 now fixed, when trying to view the source of a message, I get a window that says:
===
File not found
The file /C:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/bin/chrome/toolkit/content/global/viewSource.xul cannot be found. Please check the location and try again.
===

Clicking on a link in the Browser/Error Console, I now get:
TypeError: win.gBrowser is undefined viewSourceUtils.js:69:9
viewSource/onOpen/< chrome://global/content/viewSourceUtils.js:69:9

So it looks like we're dealing with two problems here. The first one can be fixed by forking viewSource.xul to C-C's common/ as suggested by FRG in a private message.

The second problem is the JS source view from the Browser/Error Console and here we need to mimic the browser sufficiently to get this to work since that's working in FF.

Comment 6

[Frank-Rainer Grahl (:frg)]

> So it looks like we're dealing with two problems here.

For 1: If we fork should the Should we fork with history intact? 

For 2: Might work in SeaMonkey out of the box but not sure if we have a selectedTab in which case some minor work in tabbrowser might be needed. For Thunderbird this probably needs a change in toolkit. Somewhat browser centric. Reminds me of the password prompting code in 51+ ...

Comment 7

[Jorg K (CEST = GMT+2)]

For 1: Before we fork, we should see what FF does and whether we can get the source view going in a tab.

For 2: Yes, it also reminds me of the password prompting code, but we also mimic browser here:
       https://hg.mozilla.org/comm-central/rev/f45975e98daa#l2.17

Comment 8

[Jorg K (CEST = GMT+2)]

I'm looking at a Firefox Nightly. View source opens in a tab, for example:
view-source:https://www.mozilla.org/de/firefox/60.0a1/whatsnew/?oldversion=58.0.2

And opening something from the Browser console gives:
view-source:resource://gre/modules/TelemetryStopwatch.jsm

Frankly, this is quite neat, it would be great if you could do something similar.

Comment 9

[Jorg K (CEST = GMT+2)]

Snooping around the M-C source, they use:
https://hg.mozilla.org/mozilla-central/rev/fcfdf000a8f3#l2.95
gViewSourceUtils.viewSourceInBrowser(args);
https://searchfox.org/mozilla-central/rev/cac28623a15ace458a8f4526e107a71db1519daf/toolkit/components/viewsource/content/viewSourceUtils.js#71

I wonder what would be required to use this for our message URLs.

Comment 12

[Jorg K (CEST = GMT+2)]

Attachment: 1439021-view-source.patch - [landed comment #21]

This tries to restore stuff from
https://hg.mozilla.org/mozilla-central/rev/fcfdf000a8f3

Somehow I'm missing something. This shows the message source, but I get this error on the console:
JavaScript error: resource://gre/modules/ViewSourceBrowser.jsm, line 95: TypeError: WeakSet value must be an object, got null

And when exercising most menu item, I get:
JavaScript error: chrome://messenger/content/viewSource.xul, line 1: TypeError: viewSourceChrome is undefined

Richard: Is there something that I've obviously missed? - or -
Aceman: Can you take a look.

I wanted to fix the bustage, but this is not good enough :-(

Comment 13

[Richard Marti (:Paenglab)]

Comment on attachment 8954233 [details] [diff] [review]
1439021-view-source.patch - [landed comment #21]

Thanks for looking into it.

When I revert, or remove this lines, it works: https://hg.mozilla.org/mozilla-central/diff/fcfdf000a8f3/toolkit/components/viewsource/ViewSourceBrowser.jsm#l1.33

The this.loadFrameScript(); shouldn't be executed.

Comment 14

[Jorg K (CEST = GMT+2)]

Thanks for looking into it. So we're talking about this:

-    // If we have a known <browser> already, load the frame script here.  This
-    // is not true for the window case, as the element does not exist until the
-    // XUL document loads.  For that case, the frame script is loaded by
-    // viewSource.js.
-    if (this._browser) {
-      this.loadFrameScript();
-    }
+    // If we have a known <browser> already, load the frame script here.
+    this.loadFrameScript();

It will be hard to convince M-C to restore this. Well, they told us to fork what we need, and we need this snipped changed for the fork to work, so let's see.

Comment 15

[Jorg K (CEST = GMT+2)]

Attachment: 1439021-M-C-part.patch

Brian, would you accept restoring the hunk which was removed here:
https://hg.mozilla.org/mozilla-central/rev/fcfdf000a8f3#l9.33

That way the forked viewSource.{xul|js|css|dtd|properties} would still work in TB for the moment.

The medium term plan is to get the "view source in tab" going for TB, but we're short in resources to investigate the feasibility and what would be involved to get that to work.

We also need to fix the problem that clicking on a source file in the developer console doesn't work.

Comment 16

[Brian Grinstead [:bgrins]]

Comment on attachment 8954307 [details] [diff] [review]
1439021-M-C-part.patch

Review of attachment 8954307 [details] [diff] [review]:
-----------------------------------------------------------------

Seems OK to me, but forwarding review to jryans

Comment 17

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8954307 [details] [diff] [review]
1439021-M-C-part.patch

Review of attachment 8954307 [details] [diff] [review]:
-----------------------------------------------------------------

Seems reasonable to me, thanks for working on it! :)

::: toolkit/components/viewsource/ViewSourceBrowser.jsm
@@ +75,5 @@
>  
> +    // If we have a known <browser> already, load the frame script here. This
> +    // is not true for the window case (still used by other applications like
> +    // Thunderbird), as the element does not exist until the XUL document loads.
> +    if (this._browser) {

I would suggest moving these details into `loadFrameScript`, and have it return early when there's no `browser`.  Also, this file seems to use the getter version throughout (`this.browser`), so seems best to follow that trend.

Comment 18

[Jorg K (CEST = GMT+2)]

Attachment: 1439021-M-C-part.patch (v2) [landed comment #29]

Thanks for the quick review. Here's a patch that incorporates your suggestions.

I removed the original comment
  If we have a known <browser> already, load the frame script here.
since it appeared out of place in front of an unconditional call.

Comment 19

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8954518 [details] [diff] [review]
1439021-M-C-part.patch (v2) [landed comment #29]

Review of attachment 8954518 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, looks great to me! :)

Comment 20

[Jorg K (CEST = GMT+2)]

Thanks for the quick turn-around.

Dear sheriff, could you please land "1439021-M-C-part.patch (v2)" on inbound.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2caecab6e8c0
restore 'view source' by forking M-C code. r=me (fork-onky)

Comment 22

[Jorg K (CEST = GMT+2)]

Dear sheriff, could you please land "1439021-M-C-part.patch (v2)" on inbound.

Comment 23

[Jorg K (CEST = GMT+2)]

Comment on attachment 8954233 [details] [diff] [review]
1439021-view-source.patch - [landed comment #21]

So this was no WIP after all thanks to Richard's suggestion. This already shows the message source again and will work even better with the M-C part landed as well :-)

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/172a4b9b4974
Follow-up: remove import-globals-from linter directives. rs=bustage-fix DONTBUILD

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6188bb22f81e
Check for browser in loadFrameScript() to support Thunderbird. r=jryans

Comment 26

[Andreea Pavel [:apavel]]

Jorg, the patch has been landed.

Comment 27

[Richard Marti (:Paenglab)]

The edit menu in the source view will be affected by bug 1439766 when it lands. I can look at it this evening.

Comment 28

[Jorg K (CEST = GMT+2)]

Thanks. The M-C part hasn't been merged, so it will be broken before it ever worked :-(

Comment 29

[Jorg K (CEST = GMT+2)]

Oops, ignore that, it has been merged:
https://hg.mozilla.org/mozilla-central/rev/6188bb22f81e
Check for browser in loadFrameScript() to support Thunderbird. r=jryans

Comment 30

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/6188bb22f81e

Comment 31

[:aceman]

Comment on attachment 8954233 [details] [diff] [review]
1439021-view-source.patch - [landed comment #21]

Review of attachment 8954233 [details] [diff] [review]:
-----------------------------------------------------------------

OK, if this is restoration of the original code from m-c, then probably nothing to review here.

But clicking on errors in the console doesn't open the source file, with a message like:
Error: An error was thrown inside one of your components, but React doesn't know what it was. This is likely due to browser flakiness. React does its best to preserve the "Pause on exceptions" behavior of the DevTools, which requires some DEV-mode only tricks. It's possible that these don't work in your browser. Try triggering the error in production mode, or switching to a modern browser. If you suspect that this is actually an issue with React, please file an issue. 1 react-dom-dev.js:624:19

And I also sometimes get:
TypeError: this.gViewSourceUtils is undefined 1, hudservice.js:400:5

gViewSourceUtils is not mentioned in this patch. Do we need to somehow provide it too, similar to gViewSourceBundle ?

Comment 32

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #31)
> OK, if this is restoration of the original code from m-c, then probably
> nothing to review here.
Thanks. The restored code only ever worked for message source, not JS source. I made a mistake that Richard corrected for me later: https://hg.mozilla.org/comm-central/rev/5e07cff9cd9c422239f82edc33e2aec5f00f45b7#l2.51

> But clicking on errors in the console doesn't open the source file, with a
> message like:
> Error: An error was thrown inside one of your components, but React doesn't
> know what it was. This is likely due to browser flakiness. React does its
> best to preserve the "Pause on exceptions" behavior of the DevTools, which
> requires some DEV-mode only tricks. It's possible that these don't work in
> your browser. Try triggering the error in production mode, or switching to a
> modern browser. If you suspect that this is actually an issue with React,
> please file an issue. 1 react-dom-dev.js:624:19
Oh, I saw that with pref view_source.editor.external set to true. Try with false. It's all here:
https://searchfox.org/mozilla-central/source/toolkit/components/viewsource/content/viewSourceUtils.js#48

Comment 33

[:aceman]

Attachment: load viewSourceUtils.js [landed comment #35]

This makes gViewSourceUtils defined.
Clicking a source link in the Error console still fails with win.gBrowser not being defined at viewSourceUtils.js:69.

But, setting view_source.editor.external=true and view_source.editor.path to your preferred viewer/editor makes the source file of the error open in that editor. Could be a temporary measure for devs :)

There is still "aCallBack is not a function 1 viewSourceUtils.js:268" output, but I file that as a separate bug as that seems to be a real bug in m-c.

Comment 34

[Jorg K (CEST = GMT+2)]

Comment on attachment 8955821 [details] [diff] [review]
load viewSourceUtils.js [landed comment #35]

Yes, that works. For the record, I set view_source.editor.path to
C:\Program Files (x86)\Notepad++\notepad++.exe
Yes, with the backslashes and the spaces and it still works ;-)

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/248a595bf6bc
load viewSourceUtils.js into Thunderbird to make 'view source' work for more cases. r=jorgk

Comment 36

[Jorg K (CEST = GMT+2)]

Hmm, I see
JavaScript error: chrome://messenger/content/mailCore.js, line 20: ReferenceError: XPCOMUtils is not defined

Comment 37

[:aceman]

Attachment: import XPCOMUtils [landed comment #40]

Yes, I see it too now. I wander why I didn't see it before.
Strangely it still works (makes the View source open) even with that error, which I would expect to not load gViewSourceUtils and thus do nothing.

So this should solve the error.

Comment 38

[Jorg K (CEST = GMT+2)]

Comment on attachment 8956971 [details] [diff] [review]
import XPCOMUtils [landed comment #40]

Looks OK, I assume you tried it ;-)

Comment 39

[:aceman]

I did, but so did I the previous patch :)

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b5ddfa0a002c
Follow-up: import missing XPCOMUtils in mailCore.js. r=jorgk

Comment 41

[Jorg K (CEST = GMT+2)]

Aceman, the trick with the external browser still works in TB 60 beta, but not in Daily any more. Can you take a look?

Also see bug 1448689.

Comment 42

[Jorg K (CEST = GMT+2)]

Hmm, works in a local build, but not in Daily 61.0a1 (2018-03-26) (64-bit).

Comment 43

[Jorg K (CEST = GMT+2)]

Filed bug 1450205 for that.

Comment 44

[Jorg K (CEST = GMT+2)]

In this bug here we forked "view (message) source" for mail messages and added a kludge so "view (JS/XML/etc) source" from the error/browser console will work if view_source.editor.external and view_source.editor.path are set.

That worked for a while, at least in TB 60, and then went broken again, at least in server builds, see bug 1450205.

Since this bug here has become too long and twisted, I'm moving the remaining work over to bug 1454041.

All patches here landed on version 60 of C-C and M-C, no uplifts.