Transplant getOuterWindowWithId from nsIDOMWindowUtils to a window-related service

RESOLVED FIXED in mozilla23

Status

()

Core
DOM
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: crussell, Assigned: crussell)

Tracking

({dev-doc-needed})

unspecified
mozilla23
dev-doc-needed
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(10 attachments, 2 obsolete attachments)

8.03 KB, patch
crussell
: review+
Details | Diff | Splinter Review
1.74 KB, patch
Dolske
: review+
Details | Diff | Splinter Review
1.68 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
1.72 KB, patch
myk
: review+
Details | Diff | Splinter Review
1.39 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
3.00 KB, patch
msucan
: review+
Details | Diff | Splinter Review
5.59 KB, patch
msucan
: review+
Details | Diff | Splinter Review
2.83 KB, patch
jgriffin
: review+
Details | Diff | Splinter Review
1.18 KB, patch
crussell
: review+
Details | Diff | Splinter Review
3.11 KB, patch
Details | Diff | Splinter Review
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).
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.  :(
(In reply to Boris Zbarsky (:bz) from comment #1)
> of course we need to keep the old way too.  :(

Wherefore?
Summary: Transplate getOuterWindowWithId from nsIDOMWindowUtils to a window-related service → Transplant getOuterWindowWithId from nsIDOMWindowUtils to a window-related service
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?
Blocks: 852687
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...
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #4)
> Russel

My name's Colby.

> are you working on this?

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

Sorry about that.
Created attachment 741853 [details] [diff] [review]
part 1: add nsIWindowMediator.getOuterWindowWithId and warn on nsIDOMWindowUtils.getOuterWindowWithId use)
Attachment #741853 - Flags: review?(bzbarsky)
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.
> I don't know how to use nsGlobalWindow.h from this file.

Add dom/base to LOCAL_INCLUDES?
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".
Attachment #741853 - Flags: review?(bzbarsky) → review+
Created attachment 741992 [details] [diff] [review]
part 1v2: add nsIWindowMediator.getOuterWindowWithId and warn on nsIDOMWindowUtils.getOuterWindowWithId use

Thanks.
Attachment #741853 - Attachment is obsolete: true
Attachment #741992 - Flags: review+
Created attachment 742008 [details] [diff] [review]
part 2 webrtcUI.jsm
Attachment #742008 - Flags: review?(dolske)
Created attachment 742017 [details] [diff] [review]
part 2: signInToWebsite.jsm
Attachment #742017 - Flags: review?(dolske)
Created attachment 742018 [details] [diff] [review]
part 2: webappsUI.jsm
Attachment #742018 - Flags: review?(gavin.sharp)
Created attachment 742020 [details] [diff] [review]
part 2: WebappsHandler.jsm
Attachment #742020 - Flags: review?(myk)
Created attachment 742021 [details] [diff] [review]
part 2: mobile WebrtcUI.js
Attachment #742021 - Flags: review?(mark.finkle)
Created attachment 742022 [details] [diff] [review]
part 2: WebConsoleUtils.jsm
Attachment #742022 - Flags: review?(mihai.sucan)
Created attachment 742023 [details] [diff] [review]
part 2: ConsoleAPITests.js
Attachment #742023 - Flags: review?(mihai.sucan)
Attachment #742018 - Flags: review?(gavin.sharp) → review+
Created attachment 742025 [details] [diff] [review]
part 2: marionette-actors.js
Attachment #742025 - Flags: review?(jgriffin)
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...)
Attachment #742025 - Flags: review?(jgriffin) → review+
Attachment #742020 - Flags: review?(myk) → review+
Attachment #742021 - Flags: review?(mark.finkle) → review+
Comment on attachment 742022 [details] [diff] [review]
part 2: WebConsoleUtils.jsm

This is a very much welcomed change! Thank you!
Attachment #742022 - Flags: review?(mihai.sucan) → review+

Updated

4 years ago
Attachment #742023 - Flags: review?(mihai.sucan) → review+
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.
Attachment #742008 - Flags: review?(dolske) → review+
Attachment #742017 - Flags: review?(dolske) → review+
Created attachment 746441 [details] [diff] [review]
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.
Attachment #742017 - Attachment is obsolete: true
Attachment #746441 - Flags: review+
Attachment #742008 - Attachment is obsolete: true
Attachment #742017 - Attachment is obsolete: false
Keywords: checkin-needed
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
Keywords: checkin-needed
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.
Whiteboard: [leave open]
And mochitest b-c.
https://tbpl.mozilla.org/php/getParsedLog.php?id=22695405&tree=Birch
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
Created attachment 746908 [details] [diff] [review]
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?
Landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/1c05e7a9e51c
https://hg.mozilla.org/integration/mozilla-inbound/rev/55b00015a660
Status: NEW → ASSIGNED
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/55b00015a660
https://hg.mozilla.org/mozilla-central/rev/1c05e7a9e51c
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Depends on: 893266

Updated

4 years ago
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.