Last Comment Bug 861495 - Transplant getOuterWindowWithId from nsIDOMWindowUtils to a window-related service
: Transplant getOuterWindowWithId from nsIDOMWindowUtils to a window-related se...
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: mozilla23
Assigned To: Colby Russell :crussell (no longer Mozilla-ing)
:
Mentors:
Depends on: 893266
Blocks: 865664 852687
  Show dependency treegraph
 
Reported: 2013-04-13 04:48 PDT by Colby Russell :crussell (no longer Mozilla-ing)
Modified: 2013-07-27 15:04 PDT (History)
9 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1: add nsIWindowMediator.getOuterWindowWithId and warn on nsIDOMWindowUtils.getOuterWindowWithId use) (7.73 KB, patch)
2013-04-25 07:31 PDT, Colby Russell :crussell (no longer Mozilla-ing)
bzbarsky: review+
Details | Diff | Splinter Review
part 1v2: add nsIWindowMediator.getOuterWindowWithId and warn on nsIDOMWindowUtils.getOuterWindowWithId use (8.03 KB, patch)
2013-04-25 12:48 PDT, Colby Russell :crussell (no longer Mozilla-ing)
bugzilla: review+
Details | Diff | Splinter Review
part 2 webrtcUI.jsm (1.27 KB, patch)
2013-04-25 13:52 PDT, Colby Russell :crussell (no longer Mozilla-ing)
dolske: review+
Details | Diff | Splinter Review
part 2: signInToWebsite.jsm (1.74 KB, patch)
2013-04-25 13:59 PDT, Colby Russell :crussell (no longer Mozilla-ing)
dolske: review+
Details | Diff | Splinter Review
part 2: webappsUI.jsm (1.68 KB, patch)
2013-04-25 14:00 PDT, Colby Russell :crussell (no longer Mozilla-ing)
gavin.sharp: review+
Details | Diff | Splinter Review
part 2: WebappsHandler.jsm (1.72 KB, patch)
2013-04-25 14:03 PDT, Colby Russell :crussell (no longer Mozilla-ing)
myk: review+
Details | Diff | Splinter Review
part 2: mobile WebrtcUI.js (1.39 KB, patch)
2013-04-25 14:05 PDT, Colby Russell :crussell (no longer Mozilla-ing)
mark.finkle: review+
Details | Diff | Splinter Review
part 2: WebConsoleUtils.jsm (3.00 KB, patch)
2013-04-25 14:08 PDT, Colby Russell :crussell (no longer Mozilla-ing)
mihai.sucan: review+
Details | Diff | Splinter Review
part 2: ConsoleAPITests.js (5.59 KB, patch)
2013-04-25 14:08 PDT, Colby Russell :crussell (no longer Mozilla-ing)
mihai.sucan: review+
Details | Diff | Splinter Review
part 2: marionette-actors.js (2.83 KB, patch)
2013-04-25 14:11 PDT, Colby Russell :crussell (no longer Mozilla-ing)
jgriffin: review+
Details | Diff | Splinter Review
part 2: webrtcUI.jsm v2 (see comment 22) (1.18 KB, patch)
2013-05-07 08:33 PDT, Colby Russell :crussell (no longer Mozilla-ing)
bugzilla: review+
Details | Diff | Splinter Review
part 2: fix for WebConsoleUtils.jsm (3.11 KB, patch)
2013-05-08 05:20 PDT, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review

Description Colby Russell :crussell (no longer Mozilla-ing) 2013-04-13 04:48:51 PDT
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 [:bz] 2013-04-15 07:03:32 PDT
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) 2013-04-15 14:49:09 PDT
(In reply to Boris Zbarsky (:bz) from comment #1)
> of course we need to keep the old way too.  :(

Wherefore?
Comment 3 Boris Zbarsky [:bz] 2013-04-15 15:01:19 PDT
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 [:krizsa :gabor] 2013-04-25 04:40:28 PDT
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) 2013-04-25 06:46:28 PDT
(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 [:krizsa :gabor] 2013-04-25 07:23:01 PDT
(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) 2013-04-25 07:31:42 PDT
Created attachment 741853 [details] [diff] [review]
part 1: add nsIWindowMediator.getOuterWindowWithId and warn on nsIDOMWindowUtils.getOuterWindowWithId use)
Comment 8 Colby Russell :crussell (no longer Mozilla-ing) 2013-04-25 07:36:11 PDT
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 [:bz] 2013-04-25 09:40:48 PDT
> I don't know how to use nsGlobalWindow.h from this file.

Add dom/base to LOCAL_INCLUDES?
Comment 10 Boris Zbarsky [:bz] 2013-04-25 09:44:18 PDT
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) 2013-04-25 12:48:29 PDT
Created attachment 741992 [details] [diff] [review]
part 1v2: add nsIWindowMediator.getOuterWindowWithId and warn on nsIDOMWindowUtils.getOuterWindowWithId use

Thanks.
Comment 12 Colby Russell :crussell (no longer Mozilla-ing) 2013-04-25 13:52:29 PDT
Created attachment 742008 [details] [diff] [review]
part 2 webrtcUI.jsm
Comment 13 Colby Russell :crussell (no longer Mozilla-ing) 2013-04-25 13:59:08 PDT
Created attachment 742017 [details] [diff] [review]
part 2: signInToWebsite.jsm
Comment 14 Colby Russell :crussell (no longer Mozilla-ing) 2013-04-25 14:00:50 PDT
Created attachment 742018 [details] [diff] [review]
part 2: webappsUI.jsm
Comment 15 Colby Russell :crussell (no longer Mozilla-ing) 2013-04-25 14:03:17 PDT
Created attachment 742020 [details] [diff] [review]
part 2: WebappsHandler.jsm
Comment 16 Colby Russell :crussell (no longer Mozilla-ing) 2013-04-25 14:05:57 PDT
Created attachment 742021 [details] [diff] [review]
part 2: mobile WebrtcUI.js
Comment 17 Colby Russell :crussell (no longer Mozilla-ing) 2013-04-25 14:08:21 PDT
Created attachment 742022 [details] [diff] [review]
part 2: WebConsoleUtils.jsm
Comment 18 Colby Russell :crussell (no longer Mozilla-ing) 2013-04-25 14:08:53 PDT
Created attachment 742023 [details] [diff] [review]
part 2: ConsoleAPITests.js
Comment 19 Colby Russell :crussell (no longer Mozilla-ing) 2013-04-25 14:11:47 PDT
Created attachment 742025 [details] [diff] [review]
part 2: marionette-actors.js
Comment 20 Colby Russell :crussell (no longer Mozilla-ing) 2013-04-25 14:17:37 PDT
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] 2013-04-26 04:23:20 PDT
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] 2013-05-06 15:31:07 PDT
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) 2013-05-07 08:33:45 PDT
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.
Comment 25 Ryan VanderMeulen [:RyanVM] 2013-05-07 11:40:53 PDT
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] 2013-05-07 12:11:12 PDT
And mochitest b-c.
https://tbpl.mozilla.org/php/getParsedLog.php?id=22695405&tree=Birch
Comment 28 Mihai Sucan [:msucan] 2013-05-08 05:20:26 PDT
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?

Note You need to log in before you can comment on or make changes to this bug.