Closed
Bug 861495
Opened 11 years ago
Closed 11 years ago
Transplant getOuterWindowWithId from nsIDOMWindowUtils to a window-related service
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla23
People
(Reporter: crussell, Assigned: crussell)
References
Details
(Keywords: dev-doc-needed)
Attachments
(10 files, 2 obsolete files)
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).
Comment 1•11 years ago
|
||
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. :(
Assignee | ||
Comment 2•11 years ago
|
||
(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
Comment 3•11 years ago
|
||
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•11 years ago
|
||
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...
Assignee | ||
Comment 5•11 years ago
|
||
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #4) > Russel My name's Colby. > are you working on this? Yes.
Comment 6•11 years ago
|
||
(In reply to Colby Russell :crussell from comment #5) > My name's Colby. Sorry about that.
Assignee | ||
Comment 7•11 years ago
|
||
Attachment #741853 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 8•11 years ago
|
||
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•11 years ago
|
||
> I don't know how to use nsGlobalWindow.h from this file.
Add dom/base to LOCAL_INCLUDES?
Comment 10•11 years ago
|
||
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+
Assignee | ||
Comment 11•11 years ago
|
||
Thanks.
Attachment #741853 -
Attachment is obsolete: true
Attachment #741992 -
Flags: review+
Assignee | ||
Comment 12•11 years ago
|
||
Attachment #742008 -
Flags: review?(dolske)
Assignee | ||
Comment 13•11 years ago
|
||
Attachment #742017 -
Flags: review?(dolske)
Assignee | ||
Comment 14•11 years ago
|
||
Attachment #742018 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 15•11 years ago
|
||
Attachment #742020 -
Flags: review?(myk)
Assignee | ||
Comment 16•11 years ago
|
||
Attachment #742021 -
Flags: review?(mark.finkle)
Assignee | ||
Comment 17•11 years ago
|
||
Attachment #742022 -
Flags: review?(mihai.sucan)
Assignee | ||
Comment 18•11 years ago
|
||
Attachment #742023 -
Flags: review?(mihai.sucan)
Updated•11 years ago
|
Attachment #742018 -
Flags: review?(gavin.sharp) → review+
Assignee | ||
Comment 19•11 years ago
|
||
Attachment #742025 -
Flags: review?(jgriffin)
Assignee | ||
Comment 20•11 years ago
|
||
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...)
Updated•11 years ago
|
Attachment #742025 -
Flags: review?(jgriffin) → review+
Updated•11 years ago
|
Attachment #742020 -
Flags: review?(myk) → review+
Updated•11 years ago
|
Attachment #742021 -
Flags: review?(mark.finkle) → review+
Comment 21•11 years ago
|
||
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•11 years ago
|
Attachment #742023 -
Flags: review?(mihai.sucan) → review+
Comment 22•11 years ago
|
||
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+
Updated•11 years ago
|
Attachment #742017 -
Flags: review?(dolske) → review+
Assignee | ||
Comment 23•11 years ago
|
||
(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+
Assignee | ||
Updated•11 years ago
|
Attachment #742008 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #742017 -
Attachment is obsolete: false
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 24•11 years ago
|
||
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
Comment 25•11 years ago
|
||
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]
Comment 26•11 years ago
|
||
And mochitest b-c. https://tbpl.mozilla.org/php/getParsedLog.php?id=22695405&tree=Birch
Comment 27•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
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]
Comment 30•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/55b00015a660 https://hg.mozilla.org/mozilla-central/rev/1c05e7a9e51c
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Updated•11 years ago
|
Keywords: dev-doc-needed
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•