861495 - Transplant getOuterWindowWithId from nsIDOMWindowUtils to a window-related service

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Colby Russell :crussell (no longer Mozilla-ing)]

nsIDOMWindowUtils.getOuterWindowWithId was added in bug 605492.  It would make more sense, though, for it to live on one of the window-mediator or window-watcher components, since it doesn't need to do anything with the underlying window.  It just delegates to the static method.
<http://hg.mozilla.org/mozilla-central/annotate/e8bbd291396a/dom/base/nsDOMWindowUtils.cpp#l2510>
<http://hg.mozilla.org/mozilla-central/annotate/e8bbd291396a/dom/base/nsGlobalWindow.h#l591>

Boris, was there a reason for this that I'm missing, or just oversight?

It turns out that everything in m-c that uses the method just ends up grabbing an arbitrary window with getMostRecentWindow, anyway.  (The Web Console at least takes an optional window hint parameter and only uses getMostRecentWindow as a fallback.)

Look:
<http://hg.mozilla.org/mozilla-central/file/e8bbd291396a/toolkit/devtools/webconsole/WebConsoleUtils.jsm#l172>
<http://hg.mozilla.org/mozilla-central/file/e8bbd291396a/browser/modules/webappsUI.jsm#l89>
<http://hg.mozilla.org/mozilla-central/file/e8bbd291396a/browser/modules/webrtcUI.jsm#l59>
<http://hg.mozilla.org/mozilla-central/file/e8bbd291396a/browser/modules/SignInToWebsite.jsm#l138>
<http://hg.mozilla.org/mozilla-central/file/e8bbd291396a/webapprt/WebappsHandler.jsm#l44>
<http://hg.mozilla.org/mozilla-central/file/e8bbd291396a/mobile/android/chrome/content/WebrtcUI.js#l16>

If this were on one of the window services, all those wrapper methods could go away and just use, e.g.,

  Services.wm.getWindowWithOuterId(id)

I haven't profiled anything, but eliminating the getMostRecentWindow + getInterface overhead would seem to be a performance gain for |console| object method calls, too.

(See also: bug 597136, comment 42).

Comment 1

[Boris Zbarsky [:bzbarsky]]

No particular reason for the way it is, frankly; I didn't even think about the fact that people would want this in JS components, but of course it makes sense.

We should in fact just add it to one of the services, though of course we need to keep the old way too.  :(

Comment 2

[Colby Russell :crussell (no longer Mozilla-ing)]

(In reply to Boris Zbarsky (:bz) from comment #1)
> of course we need to keep the old way too.  :(

Wherefore?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Because https://mxr.mozilla.org/addons/search?string=getOuterWindowWithID&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=addons says "Found 23 matching lines in 14 files".

Once those all get fixed, we can think about removing it...  Maybe we should add a warning when it gets used?

Comment 4

[Gabor Krizsanits (INACTIVE)]

Russel, are you working on this? I would like to land a quite similar API, and would like to avoid double work + unnecessary API change around it...

Comment 5

[Colby Russell :crussell (no longer Mozilla-ing)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #4)
> Russel

My name's Colby.

> are you working on this?

Yes.

Comment 6

[Gabor Krizsanits (INACTIVE)]

(In reply to Colby Russell :crussell from comment #5)
> My name's Colby.

Sorry about that.

Comment 7

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: part 1: add nsIWindowMediator.getOuterWindowWithId and warn on nsIDOMWindowUtils.getOuterWindowWithId use)

Comment 8

[Colby Russell :crussell (no longer Mozilla-ing)]

Comment on attachment 741853 [details] [diff] [review]
part 1: add nsIWindowMediator.getOuterWindowWithId and warn on nsIDOMWindowUtils.getOuterWindowWithId use)

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

::: xpfe/appshell/src/nsWindowMediator.cpp
@@ +18,5 @@
>  #include "nsWindowMediator.h"
>  #include "nsIWindowMediatorListener.h"
>  #include "nsXPIDLString.h"
> +#include "nsContentUtils.h"
> +#include "../../../dom/base/nsGlobalWindow.h"

I don't know how to use nsGlobalWindow.h from this file.  This is clearly wrong.

I tried cargo-culting something like

EXPORTS.mozilla.dom += [
    'nsGlobalWindow.h',
]

into xpfe/appshell/moz.build, but that didn't work.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> I don't know how to use nsGlobalWindow.h from this file.

Add dom/base to LOCAL_INCLUDES?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 741853 [details] [diff] [review]
part 1: add nsIWindowMediator.getOuterWindowWithId and warn on nsIDOMWindowUtils.getOuterWindowWithId use)

>+++ b/xpfe/appshell/src/nsWindowMediator.cpp

>+#include "nsContentUtils.h"

No need.

>+  if (!nsContentUtils::IsCallerChrome()) {
>+    return NS_ERROR_DOM_SECURITY_ERR;
>+  }

Likewise.  Non-chrome script can't get its hands on this object (unlike windowutils, which it _can_ get its hands on).

>+
>+

Extra blank line there.

r=me with the nsContentUtils bits removed and the LOCAL_INCLUDES change made and the corresponding #include "nsGlobalWindow.h".

Comment 11

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: part 1v2: add nsIWindowMediator.getOuterWindowWithId and warn on nsIDOMWindowUtils.getOuterWindowWithId use

Thanks.

Comment 12

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: part 2 webrtcUI.jsm

Comment 13

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: part 2: signInToWebsite.jsm

Comment 14

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: part 2: webappsUI.jsm

Comment 15

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: part 2: WebappsHandler.jsm

Comment 16

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: part 2: mobile WebrtcUI.js

Comment 17

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: part 2: WebConsoleUtils.jsm

Comment 18

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: part 2: ConsoleAPITests.js

Comment 19

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: part 2: marionette-actors.js

Comment 20

[Colby Russell :crussell (no longer Mozilla-ing)]

Reviewers:
All part 2 patches are independent; each should only depend on part 1 (v2, i.e., attachment 741992 [details] [diff] [review]).

Also, nothing is tested; I just did the rewriting.  (Presumably, the respective areas are well-covered by existing tests...)

Comment 21

[Mihai Sucan [:msucan]]

Comment on attachment 742022 [details] [diff] [review]
part 2: WebConsoleUtils.jsm

This is a very much welcomed change! Thank you!

Comment 22

[Justin Dolske [:Dolske]]

Comment on attachment 742008 [details] [diff] [review]
part 2 webrtcUI.jsm

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

::: browser/modules/webrtcUI.jsm
@@ +58,5 @@
>  
>  function getBrowserForWindowId(aWindowID) {
> +  let contentWindow = Services.wm.getOuterWindowWithId(aWindowID);
> +  return contentWindow &&
> +         contentWindow.QueryInterface(Ci.nsIInterfaceRequestor)

I'd probably just let this throw (as it would have before) if you don't get back a valid |contentWindow|. Returning null/false/whatever to the caller isn't so useful.

Comment 23

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: part 2: webrtcUI.jsm v2 (see comment 22)

(In reply to Justin Dolske [:Dolske] from comment #22)
> > +  let contentWindow = Services.wm.getOuterWindowWithId(aWindowID);
> > +  return contentWindow &&
> > +         contentWindow.QueryInterface(Ci.nsIInterfaceRequestor)
> 
> I'd probably just let this throw

Huh.  Yeah.  I don't know why I introduced that, since *I'm* usually advocating for not silently swallowing/preventing exceptions...

Thanks.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/c26f304056f2
https://hg.mozilla.org/projects/birch/rev/fe584ef9c4b6
https://hg.mozilla.org/projects/birch/rev/5e9671d84fa2
https://hg.mozilla.org/projects/birch/rev/bb91089f38a7
https://hg.mozilla.org/projects/birch/rev/bfd76a8d8b5b
https://hg.mozilla.org/projects/birch/rev/7bbc04988e85
https://hg.mozilla.org/projects/birch/rev/bb126ac7b33d
https://hg.mozilla.org/projects/birch/rev/85a400cff75f
https://hg.mozilla.org/projects/birch/rev/9db0e05ead33

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Backed out the two webconsole patches as they were causing mochitest timeouts.
https://hg.mozilla.org/projects/birch/rev/b0f38ca2f70b

https://tbpl.mozilla.org/php/getParsedLog.php?id=22694446&tree=Birch

11:16:15     INFO -  32571 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/devtools/webconsole/test/test_cached_messages.html | Test timed out.
11:21:45     INFO -  32574 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/devtools/webconsole/test/test_consoleapi.html | Test timed out.
11:27:15     INFO -  32934 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/devtools/webconsole/test/test_object_actor.html | Test timed out.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

And mochitest b-c.
https://tbpl.mozilla.org/php/getParsedLog.php?id=22695405&tree=Birch

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c26f304056f2
https://hg.mozilla.org/mozilla-central/rev/fe584ef9c4b6
https://hg.mozilla.org/mozilla-central/rev/5e9671d84fa2
https://hg.mozilla.org/mozilla-central/rev/bb91089f38a7
https://hg.mozilla.org/mozilla-central/rev/bfd76a8d8b5b
https://hg.mozilla.org/mozilla-central/rev/85a400cff75f
https://hg.mozilla.org/mozilla-central/rev/9db0e05ead33

Comment 28

[Mihai Sucan [:msucan]]

Attachment: part 2: fix for WebConsoleUtils.jsm

I'm getting deprecation warnings since these patches landed when I use the Web Console.

I looked into the two patches that were backed out and the failures in the Birch branch (tbpl links). It seems the failure was trivial to fix: the patch got the method name wrong - instead of getOuterWindowWithId() it called getOuterWindowById(). That broke everything.

This is an updated patch that fixes all failures on my machine. Can I land the two patches in inbound or someone else wants to do it?

Comment 29

[Mihai Sucan [:msucan]]

Landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/1c05e7a9e51c
https://hg.mozilla.org/integration/mozilla-inbound/rev/55b00015a660

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/55b00015a660
https://hg.mozilla.org/mozilla-central/rev/1c05e7a9e51c