Closed Bug 1454445 Opened 6 years ago Closed 6 years ago

Support [windowtype] / Services.wm.getMostRecentWindow when running the webconsole.html directly as the Browser Console

Categories

(DevTools :: Console, enhancement, P3)

Product:
DevTools
Component:
Console
Type:
enhancement

Tracking

(firefox61 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: bgrins, Assigned: bgrins)

References

Details

Attachments

(1 file)

Bug 1454445 - Put windowtype/positioning attributes on the webconsole html tag to support opening as top-level;
(deleted), text/x-review-board-request
mossop
: review+
Details 
In order to support top-level HTML windows, I'd like to have a way to get ahold of an already-opened window with that URL.

The current setup is:
- We use a browserconsole.xul wrapper that iframes webconsole.html
- It uses <window type="devtools:webconsole"> [0]
- We have other code that does Services.wm.getMostRecentWindow("devtools:webconsole") and expects to get back a reference to the window [1]

I'd like to support this behavior when directly loading webconsole.html. A couple of ideas:

1) Support a [windowtype] or similar attribute on <html> tags and implement relevant wm APIs
2) Stop using [windowtype] altogether and instead use/create an API to get window(s) by URL.

As for (2), that would definitely work for the Browser Console case but as per https://searchfox.org/mozilla-central/search?q=windowtype&path=.xul it does look like there at least a couple times where we set the same windowtype on different URLs (i.e. devtools:toolbox in two places but that seems to be unused entirely, and navigator:browser in browser.xul and mobile/android/chrome/content/PresentationView.xul).

[0]: https://searchfox.org/mozilla-central/rev/bd326a2a6b729dc62a5aee57354a97ceac4d1dc0/devtools/client/webconsole/browserconsole.xul#11
[1]: https://searchfox.org/mozilla-central/rev/bd326a2a6b729dc62a5aee57354a97ceac4d1dc0/devtools/server/actors/webconsole.js#196
What are the use cases here?

Only opening one instance of a given window seems like an obvious one and I think the solution to that is just iterating the windows to find one that matches and focusing it instead of opening.

What other cases are there?
(In reply to Dave Townsend [:mossop] from comment #1)
> Only opening one instance of a given window seems like an obvious one and I
> think the solution to that is just iterating the windows to find one that
> matches and focusing it instead of opening.

For the Browser Console case I suppose we can rewrite it to not use this API and instead iterate all windows - checking the URL to make sure we don't open more than one. I was hoping we could come up with a solution that will work for future HTML windows as well, but if we don't think this feature is worth supporting in HTML we can turn this into a DevTools bug.

> What other cases are there?

The most frequently checked [windowtype] seems to be "navigator:browser" (it looks like at least 178 out of 254 usages as per [0][1]). I haven't looked through them all but they generally seem to be looking for the frontmost browser window and doing something with it. Given how common finding the most recent "navigator:browser" window is, maybe we should have a separate function for something like getMostRecentBrowserWindow.

I'm not sure if the remaining cases could easily be re-written as enumerators - looking at https://searchfox.org/mozilla-central/rev/bd326a2a6b729dc62a5aee57354a97ceac4d1dc0/xpfe/appshell/nsIWindowMediator.idl#44 I guess maybe getZOrderDOMWindowEnumerator would do the trick (since both refer to 'front to back order'). Although the enumerator is pretty verbose and worse to read/write.

[0]: https://searchfox.org/mozilla-central/search?q=Services.wm.getMostRecentWindow%28%22navigator%3Abrowser%22%29&path=
[1]: https://searchfox.org/mozilla-central/search?q=Services.wm.getMostRecentWindow%28&path=
(In reply to Brian Grinstead [:bgrins] from comment #2)
> (In reply to Dave Townsend [:mossop] from comment #1)
> > Only opening one instance of a given window seems like an obvious one and I
> > think the solution to that is just iterating the windows to find one that
> > matches and focusing it instead of opening.
> 
> For the Browser Console case I suppose we can rewrite it to not use this API
> and instead iterate all windows - checking the URL to make sure we don't
> open more than one. I was hoping we could come up with a solution that will
> work for future HTML windows as well, but if we don't think this feature is
> worth supporting in HTML we can turn this into a DevTools bug.

Easy enough to put that enumerator code in a module that can be used elsewhere, like RecentWindow.jsm.

> > What other cases are there?
> 
> The most frequently checked [windowtype] seems to be "navigator:browser" (it
> looks like at least 178 out of 254 usages as per [0][1]). I haven't looked
> through them all but they generally seem to be looking for the frontmost
> browser window and doing something with it. Given how common finding the
> most recent "navigator:browser" window is, maybe we should have a separate
> function for something like getMostRecentBrowserWindow.

RecentWindow.jsm seems to have something like this already.
(In reply to Dave Townsend [:mossop] from comment #3)
> (In reply to Brian Grinstead [:bgrins] from comment #2)
> > (In reply to Dave Townsend [:mossop] from comment #1)
> > > Only opening one instance of a given window seems like an obvious one and I
> > > think the solution to that is just iterating the windows to find one that
> > > matches and focusing it instead of opening.
> > 
> > For the Browser Console case I suppose we can rewrite it to not use this API
> > and instead iterate all windows - checking the URL to make sure we don't
> > open more than one. I was hoping we could come up with a solution that will
> > work for future HTML windows as well, but if we don't think this feature is
> > worth supporting in HTML we can turn this into a DevTools bug.
> 
> Easy enough to put that enumerator code in a module that can be used
> elsewhere, like RecentWindow.jsm.
>
> > > What other cases are there?
> > 
> > The most frequently checked [windowtype] seems to be "navigator:browser" (it
> > looks like at least 178 out of 254 usages as per [0][1]). I haven't looked
> > through them all but they generally seem to be looking for the frontmost
> > browser window and doing something with it. Given how common finding the
> > most recent "navigator:browser" window is, maybe we should have a separate
> > function for something like getMostRecentBrowserWindow.
> 
> RecentWindow.jsm seems to have something like this already.

Huh - didn't know about that module! I could see adding a helper that takes in a URL to support the HTML case.

By the way, is there a reason not to migrate all calls of `Services.wm.getMostRecentWindow("navigator:browser");` to use `RecentWindow.getMostRecentBrowserWindow()`? It looks like there's some special handling in there (esp for OSX) that presumably the Services.wm doesn't take into account.
Flags: needinfo?(dtownsend)
(In reply to Brian Grinstead [:bgrins] from comment #4)
> By the way, is there a reason not to migrate all calls of
> `Services.wm.getMostRecentWindow("navigator:browser");` to use
> `RecentWindow.getMostRecentBrowserWindow()`? It looks like there's some
> special handling in there (esp for OSX) that presumably the Services.wm
> doesn't take into account.

I'm not sure anyone has looked at it. I'm guessing that converting them all would probably be a good thing (particularly if we think about the private/non-private aspect of each as we do it).
Flags: needinfo?(dtownsend)
I'll add that the only other annoying thing here is that while a window is loading, there's no way to really know what it is / what the URL is. I think this is also already an issue with `windowtype` (that is, I assume the windowmediator just ignores windows that are in the process of loading) but it'd be nice if we could fix that. For toplevel window loads, ideally the window mediator or *someone* should know what's supposed to be in there (soon).
Looks like RecentWindow.jsm is moving to BrowserWindowTracker.jsm in Bug 1034036
Depends on: 1034036
(In reply to Dave Townsend [:mossop] from comment #5)
> (In reply to Brian Grinstead [:bgrins] from comment #4)
> > By the way, is there a reason not to migrate all calls of
> > `Services.wm.getMostRecentWindow("navigator:browser");` to use
> > `RecentWindow.getMostRecentBrowserWindow()`? It looks like there's some
> > special handling in there (esp for OSX) that presumably the Services.wm
> > doesn't take into account.
> 
> I'm not sure anyone has looked at it. I'm guessing that converting them all
> would probably be a good thing (particularly if we think about the
> private/non-private aspect of each as we do it).

Mike, since you've recently been looking at this in Bug 1034036 - what do you think about migrating calls from `Services.wm.getMostRecentWindow("navigator:browser")` to `BrowserWindowTracker.getTopWindow()`? Are there cases where we wouldn't want to do that (i.e. they return different things and we'd particularly want the return value from Services.wm)?
Flags: needinfo?(mdeboer)
No I think it's be perfectly fine to do that. The one thing I want to look at next is to make `BrowserWindowTracker.getTopWindow()` not use Services.wm under the hood, but that's orthogonal to this bug.
Flags: needinfo?(mdeboer)
Priority: -- → P3
Another idea is to add a meta tag in the html doc to specify [windowtype] and then look for that from Services.wm APIs. Something like <meta name="windowtype" content="devtools:webconsole"> or <meta data-windowtype="devtools:webconsole">.
Oh, so I did some more testing and I think this works already! We just need to put the attribute on the HTML node. A bunch of things expect nsIXULWindow and I was thinking that in the HTML case we weren't one of those, but I guess we are. I tried to trace through what happens when we load a new window:

- JS calls into Services.ww.openWindow (with a parent window)
- nsWindowWatcher::OpenWindow calls into nsWindowWatcher::OpenWindowInternal
- We eventually end up in some variation of CreateChromeWindow (nsWindowWatcher::CreateChromeWindow or nsAppStartup::CreateChromeWindow)... depends on if we have nsIWindowCreator2
- nsAppStartup::CreateChromeWindow2 then calls either nsAppShellService::CreateTopLevelWindow or nsXULWindow::CreateNewWindow (which in turn calls nsAppShellService::CreateTopLevelWindow)
- nsAppShellService::CreateTopLevelWindow takes in a xul window then calls:
- nsAppShellService::RegisterTopLevelWindow which then calls:
- mediator->RegisterWindow(aWindow) which then sets up nsWindowInfo with it
- MostRecentWindowInfo searches through those nsWindowInfos and calls nsWindowInfo::TypeEquals which then reads the 'windowtype' attr from the document element (fetched from GetDOMNodeFromDocShell).

Ultimately, if I run this in the Browser Console:
`Services.ww.openWindow(null, "data:text/html,<html windowtype='devtools:foo'><body style='width: 100px; height: 100px;'></body></html>", "_blank", "chrome", null);`

Then:
`Services.wm.getMostRecentWindow("devtools:foo")`

Returns the window we want
Moving this to Firefox since the fix looks to be frontend only
Component: DOM → Developer Tools
Product: Core → Firefox
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Component: Developer Tools → Developer Tools: Console
Summary: Figure out what to do with [windowtype] / Services.wm.getMostRecentWindow and HTML windows → Support [windowtype] / Services.wm.getMostRecentWindow when running the webconsole.html directly as the Browser Console
Comment on attachment 8970753 [details]
Bug 1454445 - Put windowtype/positioning attributes on the webconsole html tag to support opening as top-level;

https://reviewboard.mozilla.org/r/239496/#review245888
Attachment #8970753 - Flags: review?(dtownsend) → review+
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2adfda24e3cd
Put windowtype/positioning attributes on the webconsole html tag to support opening as top-level;r=mossop
https://hg.mozilla.org/mozilla-central/rev/2adfda24e3cd
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Blocks: 1456663
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: