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

RESOLVED FIXED in Thunderbird 60.0

Status

RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: jorgk, Assigned: jorgk)

Tracking

(Blocks: 1 bug, {regression})

Trunk
Thunderbird 60.0
regression
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird60+)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

a year ago
Bug 1418403 has landed, so it's time to act.
(Assignee)

Comment 1

a year ago
Related to bug 1163707, I suppose.

Comment 2

a year ago
(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

a year ago
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"
^
(Assignee)

Comment 4

a year ago
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.
(Assignee)

Comment 5

a year ago
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.
> 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+ ...
(Assignee)

Comment 7

a year ago
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
(Assignee)

Comment 8

a year ago
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.
(Assignee)

Comment 9

a year ago
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.
(Assignee)

Updated

a year ago
tracking-thunderbird60: --- → +
Keywords: regression
(Assignee)

Updated

a year ago
Duplicate of this bug: 1439328
(Assignee)

Updated

a year ago
Summary: Fix fallout from bug 1418403: Remove viewing source in a stand-alone window → Fix fallout from bug 1418403: Remove viewing source in a stand-alone window (view source not working)
(Assignee)

Updated

a year ago
Duplicate of this bug: 1439539
(Assignee)

Comment 12

a year ago
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 :-(
Attachment #8954233 - Flags: feedback?(richard.marti)
Attachment #8954233 - Flags: feedback?(acelists)
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.
Attachment #8954233 - Flags: feedback?(richard.marti) → feedback+
(Assignee)

Comment 14

a year ago
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.
(Assignee)

Comment 15

a year ago
Posted patch 1439021-M-C-part.patch (obsolete) — Splinter Review
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.
Attachment #8954307 - Flags: review?(bgrinstead)
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
Attachment #8954307 - Flags: review?(bgrinstead) → review?(jryans)
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.
Attachment #8954307 - Flags: review?(jryans) → review+
(Assignee)

Comment 18

a year ago
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.
Attachment #8954307 - Attachment is obsolete: true
Attachment #8954518 - Flags: review?(jryans)
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! :)
Attachment #8954518 - Flags: review?(jryans) → review+
(Assignee)

Comment 20

a year ago
Thanks for the quick turn-around.

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

Updated

a year ago
Keywords: leave-open

Comment 21

a year ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/2caecab6e8c0
restore 'view source' by forking M-C code. r=me (fork-onky)
Keywords: checkin-needed
(Assignee)

Comment 22

a year ago
Dear sheriff, could you please land "1439021-M-C-part.patch (v2)" on inbound.
Flags: needinfo?(aryx.bugmail)
Flags: needinfo?(apavel)
Keywords: checkin-needed
(Assignee)

Comment 23

a year ago
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 :-)
Attachment #8954233 - Attachment description: 1439021-view-source.patch - WIP (v1) → 1439021-view-source.patch - [landed comment #21]
Attachment #8954233 - Flags: feedback?(acelists)

Comment 24

a year ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/172a4b9b4974
Follow-up: remove import-globals-from linter directives. rs=bustage-fix DONTBUILD
Keywords: checkin-needed

Comment 25

a year ago
Pushed by aciure@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6188bb22f81e
Check for browser in loadFrameScript() to support Thunderbird. r=jryans
Jorg, the patch has been landed.
Flags: needinfo?(aryx.bugmail)
Flags: needinfo?(apavel)
The edit menu in the source view will be affected by bug 1439766 when it lands. I can look at it this evening.
(Assignee)

Comment 28

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

Comment 29

a year ago
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 31

a year ago
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 ?
Attachment #8954233 - Flags: review+
(Assignee)

Comment 32

a year ago
(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

a year ago
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.
Attachment #8955821 - Flags: review?(jorgk)
(Assignee)

Comment 34

a year ago
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 ;-)
Attachment #8955821 - Flags: review?(jorgk) → review+

Comment 35

a year ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/248a595bf6bc
load viewSourceUtils.js into Thunderbird to make 'view source' work for more cases. r=jorgk
(Assignee)

Comment 36

a year ago
Hmm, I see
JavaScript error: chrome://messenger/content/mailCore.js, line 20: ReferenceError: XPCOMUtils is not defined
Flags: needinfo?(acelists)

Comment 37

a year ago
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.
Flags: needinfo?(acelists)
Attachment #8956971 - Flags: review?(jorgk)
(Assignee)

Comment 38

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

Looks OK, I assume you tried it ;-)
Attachment #8956971 - Flags: review?(jorgk) → review+
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 39

a year ago
I did, but so did I the previous patch :)

Comment 40

a year ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/b5ddfa0a002c
Follow-up: import missing XPCOMUtils in mailCore.js. r=jorgk
Keywords: checkin-needed
(Assignee)

Comment 41

a year ago
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.
Flags: needinfo?(acelists)
(Assignee)

Comment 42

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

Comment 43

a year ago
Filed bug 1450205 for that.
Flags: needinfo?(acelists)
(Assignee)

Updated

11 months ago
Blocks: 1454041
(Assignee)

Updated

11 months ago
Attachment #8954518 - Attachment description: 1439021-M-C-part.patch (v2) → 1439021-M-C-part.patch (v2) [landed comment #29]
(Assignee)

Updated

11 months ago
Attachment #8955821 - Attachment description: load viewSourceUtils.js → load viewSourceUtils.js [landed comment #35]
(Assignee)

Updated

11 months ago
Attachment #8956971 - Attachment description: import XPCOMUtils → import XPCOMUtils [landed comment #40]
(Assignee)

Comment 44

11 months ago
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.
Assignee: nobody → jorgk
Status: NEW → RESOLVED
Last Resolved: 11 months ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 60.0
You need to log in before you can comment on or make changes to this bug.