1444133 - Have an option to have view-source open in a separate (browser) window, not separate tab

Status: RESOLVED FIXED

(Toolkit :: View Source, enhancement, P3)

Description

[Boris Zbarsky [:bzbarsky]]

This is not a suggestion to bring back the dedicated view-source UI.  Just to do what we do now, but in a new browser window, not new browser tab.  See bug 1418403 comment 27.

Comment 1

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

Any thoughts about the right trigger for window mode?  Should it be a pref like before?  Maybe a different shortcut, like adding Shift similar to links, so Cmd-Shift-U would open in a window?

Comment 2

[Boris Zbarsky [:bzbarsky]]

I personally would prefer a pref...

Comment 3

[Brian Grinstead [:bgrins]]

Should we re-add the `view_source.tab` pref?

Comment 4

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

(In reply to Brian Grinstead [:bgrins] from comment #3)
> Should we re-add the `view_source.tab` pref?

Assuming we go with a pref, then probably so, since it's presumably still set for those who had set it before.

Comment 5

[Boris Zbarsky [:bzbarsky]]

One thing we could do is have a pref _and_ have Cmd-Shift-U do the "reverse of the pref" behavior, by the way.  If we want that complexity.

Comment 6

[Jordan Brown]

Preference, please.  And I don't feel any need for an alternate keystroke to invert it.

Comment 7

[mtness]

(In reply to J. Ryan Stinnett [:jryans] from comment #4)
> (In reply to Brian Grinstead [:bgrins] from comment #3)
> > Should we re-add the `view_source.tab` pref?
> 
> Assuming we go with a pref, then probably so, since it's presumably still
> set for those who had set it before.

Yes, that would be the most unobtrusive behavior.
(I was very annoyed today to find outr that thios behavior had been changed)

pretty please with sugar on top, just a preference setting. Keep it simple.


The one thing I liked very much about the gygone XUL window is, that iot was disatraction-free.
Just the source, nothing more, nothing less. - I hope we ca restore this feature again.

Comment 8

[tom]

(In reply to Brian Grinstead [:bgrins] from comment #3)
> Should we re-add the `view_source.tab` pref?

Yes, please do so.

Comment 9

[Jason Laster [:jlast]]

Out of curiosity, is there a specific use case you had in mind or just more comfortable with the previous behavior.

Comment 10

[Jason Laster [:jlast]]

Ah, i see where you mentioned it.

https://bugzilla.mozilla.org/show_bug.cgi?id=1418403#c26

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

This might be good first bug since it's actually a revert + a new pref
But, moving to the backlog (P3) since nobody is working on it now.

Honza

Comment 12

[Brian Grinstead [:bgrins]]

(In reply to Jan Honza Odvarko [:Honza] (need-info? me) from comment #11)
> This might be good first bug since it's actually a revert + a new pref
> But, moving to the backlog (P3) since nobody is working on it now.

This bug isn't intended as a revert. Instead of restoring the old behavior exactly this would cause the current `view-source:` uri to open in a window instead of a tab. Though I hope it will still be a relatively easy bug if we handle it with a pref.

Comment 13

[Dhyey Thakore [:dhyey35]]

Can I work on this bug ? I already have mozilla-unified successfully setup.

Comment 14

[Jason Laster [:jlast]]

Sure! I'd be happy to mentor.

Comment 15

[Dhyey Thakore [:dhyey35]]

(In reply to Jason Laster [:jlast] from comment #14)

Sure! I'd be happy to mentor.

For starting I will be reviewing commits:

1. https://hg.mozilla.org/mozreview/gecko/rev/8b2584caa54f47bb60a9393837c5941e1b555fce#index_header
2. https://hg.mozilla.org/mozreview/gecko/rev/fe3bb8fb5c60de3a9dfd2cca0a3a171fa7ccbd58#index_header

and try to understand what I need to restore and where in current code.
Should I be doing anything else too ?

Comment 16

[Jason Laster [:jlast]]

makes sense

Comment 17

[Akash Srivastava]

Hey, can I get assigned to this bug?

Comment 18

[u632215]

Hi Jason, dhyey35 here.

I had to create a duplicate account as I had factory reset my phone and have lost access to my 2 Factor Authentication codes on google authenticator and I mailed [e-mail] as mentioned here: https://wiki.mozilla.org/BMO/UserGuide/Two-Factor_Authentication#Help.21_My_phone_has_been_destroyed have not received since last 9 days. Can you please help me get back to my account ?

I am also facing an issue when I try to checkout code at https://hg.mozilla.org/mozreview/gecko/rev/8b2584caa54f47bb60a9393837c5941e1b555fce#index_header by doing hg up 8f678eee80e9.
The error is abort: unknown revision '8f678eee80e9'!. Is the commit hash something else or is there some other issue ?

Comment 19

[Brian Grinstead [:bgrins]]

(In reply to dhyey35+moz from comment #18)

I had to create a duplicate account as I had factory reset my phone and have lost access to my 2 Factor Authentication codes on google authenticator and I mailed [e-mail] as mentioned here: https://wiki.mozilla.org/BMO/UserGuide/Two-Factor_Authentication#Help.21_My_phone_has_been_destroyed have not received since last 9 days. Can you please help me get back to my account ?

Sorry about that - sounds like it may have been caught in a spam filter. Forwarding the needinfo to see if we can get it sorted out.

Comment 20

[Dylan Hardison [:dylan] (he/him)]

This is sorted -- see email reply.

Comment 21

[Boris Zbarsky [:bzbarsky]]

by doing hg up 8f678eee80e9

You probably want "hg pull -u -r 8f678eee80e9 https://hg.mozilla.org/mozreview/gecko" or so.

Comment 22

[Dhyey Thakore [:dhyey35]]

(In reply to Dylan Hardison [:dylan] (he/him) from comment #20)

This is sorted -- see email reply.

Thanks :)

Comment 23

[Dhyey Thakore [:dhyey35]]

(In reply to Brian Grinstead [:bgrins] from comment #12)

(In reply to Jan Honza Odvarko [:Honza] (need-info? me) from comment #11)

This might be good first bug since it's actually a revert + a new pref
But, moving to the backlog (P3) since nobody is working on it now.

This bug isn't intended as a revert. Instead of restoring the old behavior
exactly this would cause the current view-source: uri to open in a window
instead of a tab. Though I hope it will still be a relatively easy bug if we
handle it with a pref.

I checked the older version and when 'view_source.tab' is set to false a new browser window opens up and when set to true it opens source in new tab. Isn't this exactly what we want now ?

Comment 24

[Brian Grinstead [:bgrins]]

(In reply to dhyey35 from comment #23)

(In reply to Brian Grinstead [:bgrins] from comment #12)

(In reply to Jan Honza Odvarko [:Honza] (need-info? me) from comment #11)

This might be good first bug since it's actually a revert + a new pref
But, moving to the backlog (P3) since nobody is working on it now.

This bug isn't intended as a revert. Instead of restoring the old behavior
exactly this would cause the current view-source: uri to open in a window
instead of a tab. Though I hope it will still be a relatively easy bug if we
handle it with a pref.

I checked the older version and when 'view_source.tab' is set to false a new browser window opens up and when set to true it opens source in new tab. Isn't this exactly what we want now ?

We now want it to open in a new "normal" browser window instead of a special view source window (with no chrome, additional shortcuts, etc). See https://bugzilla.mozilla.org/show_bug.cgi?id=1418403#c27. So we won't want to fully back out bug 1418403 but there may be some relevant parts that we want to revert around https://searchfox.org/mozilla-central/rev/5c8ea961d04767db723a0a15e3a8f7fbca154129/toolkit/components/viewsource/content/viewSourceUtils.js#57. I think practically we'll now want to open a new browser window instead of getting the most recent one, before calling BrowserViewSourceOfDocument on it.

Comment 25

[Dhyey Thakore [:dhyey35]]

Attachment: Bug 1444133 - Have an option to have view-source open in a separate (browser) window, not separate tab, r=mconley

Comment 26

[Dhyey Thakore [:dhyey35]]

I am not able to figure out how to pass browser object to BrowserViewSourceOfDocument.

1. Should I be using window.openDialog ?
2. Should the code to open a new normal window be placed where it is currently ?

Comment 27

[Brian Grinstead [:bgrins]]

(In reply to dhyey35 from comment #26)

I am not able to figure out how to pass browser object to BrowserViewSourceOfDocument.

1. Should I be using window.openDialog ?
2. Should the code to open a new normal window be placed where it is currently ?

Forwarding to mconley, who's reviewed a bunch of the view source stuff in the past.

Comment 28

[Mike Conley (:mconley) (:⚙️)]

I think the problem here is that browserToViewSrcIn is supposed to be a <xul:browser>, and window.openDialog is returning a DOM window.

The other problem is that the browser argument to BrowserViewSourceOfDocument is the browser that we want to view the source of, not the one that we want to view the source in.

What I suggest, if we want to do this, is to handle creating the new window inside of BrowserViewSourceOfDocument. Pass some new argument, like, "inNewWindow" to it if the prefs are true, and then have BrowserViewSourceOfDocument be the one responsible for opening the new browser window, getting at the initial selected browser inside of it, and then setting that as the viewSourceBrowser on the "args" object we pass to viewSourceInBrowser.

Note that we'll also need to make sure that the initial browser tab in the new window is hosted in the same content process as the one we're viewing the source of. I believe you can do that by passing the browser-we-want-to-view-the-source of's <xul:tab> as the first argument to the window.openDialog call.

Comment 29

[Dhyey Thakore [:dhyey35]]

After struggling quite a bit with getting <xul:browser> object instead of DOM window from window.openDialog I have resorted to a hack to get further help from you ( updated phabricator ). Right now I have kept tabBrowser = newWindow.gBrowser in a timeout as gBrowser is not instantly available in the window object after creating a new window by using window.openDialog.

1. Should I use some other method for opening a new normal browser window which returns me <xul:browser> ?

2. I explored the object of args.browser ( which is browser-we-want-to-view-the-source of ) but couldn't find it's <xul:tab>, can you please let me know how can I get it ?

Comment 30

[Mike Conley (:mconley) (:⚙️)]

Sorry for the delay.

I briefly considered making it possible to pass something to the window.openDialog call that would allow the opened window to know that it needs to load a View Source... but I think I have a simpler suggestion.

Perhaps we can open a new tab with View Source, but then immediately tear it out to a new window.

I think you could potentially do that here:

https://searchfox.org/mozilla-central/rev/93905b660fc99a5d52b683690dd26471daca08c8/browser/base/content/browser.js#2589

Right after top.gViewSourceUtils.viewSourceInBrowser(args);, try:

tabBrowser.replaceTabsWithWindow(tab);

Does that open View Source in a new window correctly?

Comment 31

[Dhyey Thakore [:dhyey35]]

I did that but the user experience does not seem good. It feels like a hack.
I have uploaded a gif here about what happens when I press Ctrl+U

Comment 32

[Dhyey Thakore [:dhyey35]]

I was thinking maybe we can do something like create a new browser window and have an event dispatched when it has loaded ( i.e gBrowser object is available ) so that we can load a new tab for view-source just like existing code does at: https://searchfox.org/mozilla-central/source/browser/base/content/browser.js#2552 .

In case of opening in new window we would have an eventListener which calls the view-source tab opening code.

Pseudocode:

    
async function BrowserViewSourceOfDocument(aArgsOrDocument, inNewWindow) {
    ...
    if(inNewWindow) {
        var win = openDialog() // or some relevant method here
        win.addEventListener("gBrowserNowAvailable", () => openViewSourceTab(args, win.gBrowser))
    } else {
        openViewSourceTab(args, tabBrowser)
    }

}

function openViewSourceTab(args, browserToViewSourceIn) {
    // relevant code, something like https://searchfox.org/mozilla-central/source/browser/base/content/browser.js#2583
}

I am not sure if this is a correct way and which methods should I be calling. Please let me know your views.

Comment 33

[Mike Conley (:mconley) (:⚙️)]

I'm just wary of adding additional complexity for this niche case...

Going back to the replaceTabsWithWindow experiment, when opening the tab, can you open it without animation and hide it immediately? Something like:

  let tab = tabBrowser.loadOneTab("about:blank", {
    relatedToCurrent: true,
    inBackground: false,
    skipAnimation: true, // <-- THIS IS NEW
    preferredRemoteType,
    sameProcessAsFrameLoader: args.browser ? args.browser.frameLoader : null,
    triggeringPrincipal: Services.scriptSecurityManager.getSystemPrincipal(),
  });

  tabBrowser.hideTab(tab);
  tabBrowser.replaceTabsWithWindow(tab);

How does that look?

Comment 34

[Dhyey Thakore [:dhyey35]]

Smooth :)

I have updated the code. inBackground also needed to be true to hide the tab because of the !aTab.selected condition: https://searchfox.org/mozilla-central/source/browser/base/content/tabbrowser.js#3596

Do I need to write tests ?

Comment 35

[Mike Conley (:mconley) (:⚙️)]

(In reply to dhyey35 from comment #34)

Do I need to write tests ?

Glad it's working! Yes, let's write tests for this. You can find a selection of pre-existing tests at toolkit/components/viewsource/test/browser.

You can run the set of tests with:

./mach test toolkit/components/viewsource/test/browser

or an individual test by passing the path:

./mach test <path>

You'll want to add a new test file, and then add that test file to the test manifest here: toolkit/components/viewsource/test/browser/browser.ini

head.js has a number of utility functions that will automatically be available inside of your test that you might find useful. BrowserTestUtils will also be available to you:

https://searchfox.org/mozilla-central/rev/b36e97fc776635655e84f2048ff59f38fa8a4626/testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm

BrowserTestUtils is particularly useful for doing things like rigging Promise's that wait until new windows are opened.

Comment 36

[Dhyey Thakore [:dhyey35]]

Should we also open View Selection Source in new window too ?

Comment 37

[Mike Conley (:mconley) (:⚙️)]

(In reply to dhyey35 from comment #36)

Should we also open View Selection Source in new window too ?

Yeah, for completeness, I think that'd make sense.

If you hold a reference to tabBrowser.getBrowserForTab(tab);, and then use replaceTabsWithWindow here: https://searchfox.org/mozilla-central/rev/b36e97fc776635655e84f2048ff59f38fa8a4626/browser/base/content/nsContextMenu.js#903

immediately after, before returning the held browser reference, does that work?

Comment 38

[Dhyey Thakore [:dhyey35]]

Yes it did work, I have updated code on phab. Will start writing tests in a couple of days and will ask if I have any doubts.

Comment 39

[Dhyey Thakore [:dhyey35]]

While testing the promise that source has loaded in the new tab never resolves as the listener is on the old window while we have moved our tab to the new browser. This happens due to https://searchfox.org/mozilla-central/source/toolkit/components/viewsource/test/browser/head.js#135 which is called by the waitForViewSourceTab function in the same file as I am using the openDocument function ( Updated code in phab ).

1. Can you please tell me what should I do to resolve the promise even after we change the window ?

2. What should I check in assertion ? I was thinking about comparing the browser object of original tab with browser object of view source tab, if they aren't equal test passes.

Comment 40

[Mike Conley (:mconley) (:⚙️)]

(In reply to dhyey35 from comment #39)

1. Can you please tell me what should I do to resolve the promise even after we change the window ?

I recommend waiting for the new browser window instead, with something like:

await SpecialPowers.pushPrefEnv({
  set: [["view_source.tab", false]],
});

const PAGE = "http://example.com";

await BrowserTestUtils.withNewTab({
  url: PAGE,
  gBrowser,
}, browser => {
  BrowserViewSource(browser);
  let win = await BrowserTestUtils.waitForNewWindow({ url: "view-source:" + PAGE });
  ok(win, "View Source opened up in a new window.");
  await BrowserTestUtils.closeWindow(win);
});

See this documentation for waitForNewWindow.

I think that'd be enough as a test, honestly - we just need to ensure that a new window opens up, and that it's initial tab's URL is pointed at view-source: plus the page we're viewing the source of. I think that should be sufficient.

Comment 41

[Dhyey Thakore [:dhyey35]]

I tried doing what you said but the promise returned by waitForNewWindow never resolves. I added dump statements in observe:

      let observe = async (win, topic, data) => {
        dump("Observe called " + topic + "\n");
        if (topic != "domwindowopened") {
          return;
        }
        dump("In Observer\n");

        if (!anyWindow) {
          Services.ww.unregisterNotification(observe);
        }
        ...

It is never called when a new window opens, but when I close the window manually or the test times out and closes window it does log domwindowclosed in topic. I also tried window.openDialog in place of tabBrowser.replaceTabWithWindow(tab); but that also doesn't result in observer being called.

1. Do you have any idea why it would not detect new window ?

2. Is dump the only way to debug ? Can I set breakpoints or something similar ?

Comment 42

[Mike Conley (:mconley) (:⚙️)]

(In reply to dhyey35 from comment #41)

It is never called when a new window opens, but when I close the window manually or the test times out and closes window it does log domwindowclosed in topic. I also tried window.openDialog in place of tabBrowser.replaceTabWithWindow(tab); but that also doesn't result in observer being called.

Perhaps the notification fires before the observer has a chance to register itself. What happens if you re-order it, like:

await SpecialPowers.pushPrefEnv({
  set: [["view_source.tab", false]],
});

const PAGE = "http://example.com";

await BrowserTestUtils.withNewTab({
  url: PAGE,
  gBrowser,
}, browser => {
  let winPromise = BrowserTestUtils.waitForNewWindow({ url: "view-source:" + PAGE });
  BrowserViewSource(browser);
  let win = await winPromise;

  ok(win, "View Source opened up in a new window.");
  await BrowserTestUtils.closeWindow(win);
});

? Does that work?

Comment 43

[Dhyey Thakore [:dhyey35]]

Yes it did work. I also added a test for view partial source ( updated phab ). For view partial source I tried calling viewPartialSource from test file using viewPartialSource() ( taking inspiration from BrowserViewSource(browser) ) and nsContextMenu.viewPartialSource(), but none of them worked. I don't know which functions are available globally so I used code from openViewPartialSource

I am thinking now we can either directly call viewPartialSource in some way or we should add a flag inNewWindow in openViewPartialSource which would call a new method waitForViewSourceWindow when set to true instead of waitForViewSourceTab which it does right now because we definitely don't want so much code duplication.

Comment 44

[Mike Conley (:mconley) (:⚙️)]

Reviewed (please see my comments in Phabricator).

Comment 45

[Dhyey Thakore [:dhyey35]]

Do you have any idea about the issue that I mentioned in phab ?

Comment 46

[Mike Conley (:mconley) (:⚙️)]

Hi dhyey35,

Sorry for the delay. I was able to reproduce and examined the test failures today[1].

Here's the problem - the code opens and starts loading a view-source: URI in it, and then immediately does a tab tearout on it. The BrowserTestUtils.waitForNewWindow code is not prepared to handle a window that's being created due to a tab tear out for a tab that's still in the midst of loading.

That's why sometimes it fails, and sometimes it doesn't - it depends on the loaded state of the tab by the time the tearout completes, and there's some non-determinism there.

So here's what I suggest - suggest we stop using BrowserTestUtils.waitForNewWindow, and instead write our own small helper function before the add_tasks() that looks like this:

/**
 * Waits for a View Source window to be opened at a particular
 * URL.
 *
 * @param {string} expectedURL The view-source: URL that's expected.
 * @resolves {DOM Window} The window that was opened.
 * @returns {Promise}
 */
async function waitForNewViewSourceWindow(expectedURL) {
  let win = await BrowserTestUtils.domWindowOpened();
  let event = await BrowserTestUtils.waitForEvent(win, "EndSwapDocShells", true);
  let browser = win.gBrowser.selectedBrowser;
  if (browser.currentURI.spec != expectedURL) {
    await BrowserTestUtils.browserLoaded(browser, false, expectedURL);
  }
  return win;
}

Then replace all of your usages of BrowserTestUtils.waitForNewWindow with that, passing in the URL directly.

That fixed the issue for me. If you put that patch together, I can then land this I think.

Comment 47

[Dhyey Thakore [:dhyey35]]

I have updated the code :)

Comment 48

[Mike Conley (:mconley) (:⚙️)]

Pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ce8ac9c71692a5b48f7bf00606f544db5bd6bc1

Comment 49

[Daniel Varga [:dvarga]]

Hi! I got the following error while trying to land your patch: Repository is not supported by Lando.
Please take a look.

Comment 50

[Dhyey Thakore [:dhyey35]]

As this bug was inactive for a few days in middle, I was working on another bug so I used to shelve the files using hg shelve and when I resumed working arc diff --update 18071tried to create a new commit instead of updating this one. So I just copy pasted the hg diff --git here: https://phabricator.services.mozilla.com/differential/revision/update/18071/

Now should I go ahead and create a new commit with arc diff or is there some way to fix this ?

Also if you have some guide on working with multiple bugs at once in mozilla-unified repo, please let me know as I am new to this tools :)

Comment 51

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

When you reactivate the patch, check with hg commit --amend if it contains the phabricator url. If it does, arc diff will update it. It might be possible that the format of the patch is the issue. The following lines in the .hgrc file in my home directory create patches accepted by phabricator:

[diff]
git = 1
showfunc = 1
unified = 8

About working on multiple bugs:
Get the base revision to work on: hg update central
Make your edits.
Commit: hg commit -m "Bug <number> - <commit message goes here>
If it's ready for review: arc diff
Bookmark the commit to find back easily: hg bookmark bug<number>-<descriptiveWords>, e.g. hg bookmark bug1444133-viewSourceWindow
Work on the next bug: hg update central etc.

Get back to the previous work: hg bookmarks to find the bookmark
Update to the bookmark: hg update bug1444133-viewSourceWindow
Update the patch: hg commit --amend
Update it on Phabricator: arc diff

After the patch landed (optional but recommended to keep the heads and bookmarks low):
hg update bug1444133-viewSourceWindow
Delete bookmark: hg bookmark -d bug1444133-viewSourceWindow
Delete local commit: hg histedit, replace pick with drop

Comment 52

[Dhyey Thakore [:dhyey35]]

Hi Sebastian, I added the unified = 8 to my .hgrc and it now allowed me to update the diff. But it also added the 2 files from the other commit that I do not want. In the diff the devtools files are for https://bugzilla.mozilla.org/show_bug.cgi?id=1258809. Also doing hg commit --amend shows me commit for bug 1258809. Can you pls let me know how I should remove this two files from this diff ?

Comment 53

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

Update to your changes, the run hg revert -I "devtools/**" -r .~1 to revert the devtools changes to the state of the parent commit. Then hg commit --amend to save. You can also update your change before, do the same for the browser changes and commit as new revision (to keep both changes with differente phabricator IDs).

Comment 54

[Dhyey Thakore [:dhyey35]]

Thanks Sebastian, that worked :)
dvarga, Can you please try to land again ?

Comment 55

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/88b47716a875
Have an option to have view-source open in a separate (browser) window, not separate tab, r=mconley

Comment 56

[Daniel Varga [:dvarga]]

:dhyey, done, successfully landed

Comment 57

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/88b47716a875