Status

()

enhancement
P2
normal
RESOLVED FIXED
Last year
4 months ago

People

(Reporter: kmag, Assigned: kmag, NeedInfo)

Tracking

(Blocks 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(9 attachments)

59 bytes, text/x-review-board-request
Nika
: review+
Details
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
59 bytes, text/x-review-board-request
Nika
: review+
Details
59 bytes, text/x-review-board-request
Nika
: review+
Details
59 bytes, text/x-review-board-request
Nika
: review+
Details
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Nika
: review+
Details
59 bytes, text/x-review-board-request
Nika
: review+
Details
This is an empty stub interface that's currently only used in any useful way for getInterface calls.

Getting rid of it is a bit tricky, since JS consumers currently pass it to getInterface, and can't pass nsPIDOMWindowInner/nsPIDOMWindowOuter. I'm not sure what the best solution here is, but I'm thinking it would be nice to pass a Window constructor and convert that to nsPIDOMWindowInner internally.
Comment on attachment 8979146 [details]
Bug 1463016: Part 2 - Allow importing Window constructor in system scopes.

https://reviewboard.mozilla.org/r/245392/#review251548

::: commit-message-cc786:1
(Diff revision 1)
> +Bug 1463016: Part 2 - Allow importing Window constructor in system scopes. r?bz

s/constructor/interface object/, because it's not a constructor.

::: dom/base/nsGlobalWindowInner.cpp:3080
(Diff revision 1)
>  
>  /* static */ bool
>  nsGlobalWindowInner::IsPrivilegedChromeWindow(JSContext* aCx, JSObject* aObj)
>  {
>    // For now, have to deal with XPConnect objects here.
> -  return xpc::WindowOrNull(aObj)->IsChromeWindow() &&
> +  auto* win = xpc::WindowOrNull(aObj);

It's worth documenting when win might be null: when we are setting up a Window.prototype in a non-window global, right?
Attachment #8979146 - Flags: review?(bzbarsky) → review+
Comment on attachment 8979146 [details]
Bug 1463016: Part 2 - Allow importing Window constructor in system scopes.

https://reviewboard.mozilla.org/r/245392/#review251548

> It's worth documenting when win might be null: when we are setting up a Window.prototype in a non-window global, right?

Yup. Will add a comment.
Comment on attachment 8979149 [details]
Bug 1463016: Part 5 - Add domWindow property to DocShellTreeItem and update callers to use it.

https://reviewboard.mozilla.org/r/245398/#review251566

It might make sense to just add a chromeonly .docShell on windows instead of needing to go through document...  Conceptually the docshell is attached to the window, not the document, anyway.
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #12)
> Comment on attachment 8979149 [details]
> Bug 1463016: Part 5 - Add domWindow property to DocShellTreeItem and update
> callers to use it.
> 
> https://reviewboard.mozilla.org/r/245398/#review251566
> 
> It might make sense to just add a chromeonly .docShell on windows instead of
> needing to go through document...  Conceptually the docshell is attached to
> the window, not the document, anyway.

Agree, this felt weird when I was updating the existing callers. But that felt like a separate bug. Happy to do it in this patch if you prefer, though.
Comment on attachment 8979150 [details]
Bug 1463016: Part 6 - Allow JS callers to Window constructors as IID.

https://reviewboard.mozilla.org/r/245400/#review251580

::: commit-message-2d6bb:1
(Diff revision 1)
> +Bug 1463016: Part 6 - Allow JS callers to Window constructors as IID. r?bz

"to pass Window constructors", right?

The other option would be to leave the nsIDOMWindow interface and just make the relevant GetInterface implementations return whatever they want there...  I guess that would be an issue if people getInterface in C++, though.

I'm probably OK with this if we file a followup to remove things that do getInterface(nsIDOMWindow) and then to remove this code.

::: js/xpconnect/src/XPCJSID.cpp:805
(Diff revision 1)
> +    }
>      return nullptr;
>  }
>  
>  bool
>  xpc_JSObjectIsID(JSContext* cx, JSObject* obj)

Might be worth a comment about why we're not checking for Window here.
Attachment #8979150 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #14)
> The other option would be to leave the nsIDOMWindow interface and just make
> the relevant GetInterface implementations return whatever they want there...
> I guess that would be an issue if people getInterface in C++, though.

Yeah, I thought about that, but it made me nervous. C++ callers are definitely a concern. A bunch of C++ users have crept back in since khuey removed most of them, and it's not clear which of those are dealing with inner or outer windows.

The only semi-reasonable option I could think of was to return a tear-off that returned the window object when QIed to nsWrapperCache. I think that would probably be sane enough, but it requires sprinkling a bunch of nsIDOMWindow references in all of the notificationCallback objects that return DOM windows, which I didn't like. It also adds to the confusion on the JS side, where we already have a bunch of cargo culted code that tries to QI things to nsIDOMWindow when not necessary. It would be nice if JS and XPIDL code never had to care whether Windows were nsIThings at all, and just reference them by their WebIDL types.

> I'm probably OK with this if we file a followup to remove things that do
> getInterface(nsIDOMWindow) and then to remove this code.

Agreed. I'll file a follow-up. I'm still thinking about options for the notificationCallbacks uses.
Comment on attachment 8979151 [details]
Bug 1463016: Part 7 - Update remaining getInterface(nsIDOMWindow) callers to use Window.

https://reviewboard.mozilla.org/r/245402/#review251586

::: toolkit/mozapps/update/tests/unit_aus_update/uiAutoPref.js:16
(Diff revision 1)
>  };
>  
>  const WindowMediator = {
>    getMostRecentWindow(aWindowType) {
>      executeSoon(check_status);
> -    return { getInterface: ChromeUtils.generateQI([Ci.nsIDOMWindow]) };
> +    return { getInterface: ChromeUtils.generateQI([]) };

Hmm.  How does this work when someone does a getInterface(Window) on this thing?  Or does no one do that?  In which case, why do we need a getInterface at all?
> Agree, this felt weird when I was updating the existing callers. But that felt
> like a separate bug. Happy to do it in this patch if you prefer, though.

Separate bug is fine.

> Agreed. I'll file a follow-up.

Sounds great.
Comment on attachment 8979151 [details]
Bug 1463016: Part 7 - Update remaining getInterface(nsIDOMWindow) callers to use Window.

https://reviewboard.mozilla.org/r/245402/#review251586

> Hmm.  How does this work when someone does a getInterface(Window) on this thing?  Or does no one do that?  In which case, why do we need a getInterface at all?

It doesn't. I fixed the few callers that were calling getInterface(nsIDOMWindow) on things that were already windows. I'll double-check where exactly this gets used and see if the getInterface method can be removed entirely.
See Also: → 1463291
Priority: -- → P2
Comment on attachment 8979145 [details]
Bug 1463016: Part 1 - Remove unnecessary JS QueryInterface(nsIDOMWindow) calls.

https://reviewboard.mozilla.org/r/245390/#review264280
Attachment #8979145 - Flags: review?(nika) → review+
Comment on attachment 8979147 [details]
Bug 1463016: Part 3 - Remove (mostly unnecessary) instanceof nsIDOMWindow checks.

https://reviewboard.mozilla.org/r/245394/#review264282

::: browser/base/content/test/general/browser_tabfocus.js:75
(Diff revision 1)
>    function eventListener(event) {
>      // Stop the shim code from seeing this event process.
>      event.stopImmediatePropagation();
>  
>      var id;
> -    if (event.target instanceof Ci.nsIDOMWindow)
> +    if (Window.isInstance(event.target))

IIRC we should be able to also do `event.target instanceof Window` because we define `[Symbol.hasInstance]` on the `Window` object.

::: browser/components/downloads/DownloadsCommon.jsm:59
(Diff revision 1)
>      prefix: "Downloads"
>    };
>    return new ConsoleAPI(consoleOptions);
>  });
>  
> +Cu.importGlobalProperties(["Window"]);

I presume support for 'Window' in importGlobalProperties was added in part 2 :-)

::: browser/components/feeds/WebContentConverter.js:368
(Diff revision 1)
> -    let haveWindow = (aBrowserOrWindow instanceof Ci.nsIDOMWindow);
> +    let haveWindow = !(aBrowserOrWindow instanceof Ci.nsIBrowser);
>      let uri;
>      if (haveWindow) {
>        uri = Utils.checkAndGetURI(aURIString, aBrowserOrWindow);
>      } else {
>        // aURIString must not be a relative URI.
>        uri = Utils.makeURI(aURIString, null);
>      }

Can we flip this if statement?

::: browser/components/feeds/WebContentConverter.js:457
(Diff revision 1)
>      let haveWindow = aWindowOrBrowser &&
> -                     (aWindowOrBrowser instanceof Ci.nsIDOMWindow);
> +                     !(aWindowOrBrowser instanceof Ci.nsIBrowser);
>      let uri;
>      if (haveWindow) {
>        uri = Utils.checkAndGetURI(aURIString, aWindowOrBrowser);
>      } else if (aWindowOrBrowser) {
>        // uri was vetted in the content process.
>        uri = Utils.makeURI(aURIString, null);
>      }

Same here

::: toolkit/modules/addons/WebRequestContent.js:141
(Diff revision 1)
>        node = node.contentWindow;
>      }
>  
>      if (node) {
>        let window;
> -      if (node instanceof Ci.nsIDOMWindow) {
> +      if (node.Window && node instanceof node.Window) {

'Window' isn't exposed in this context I suppose? It might be clearer to import the Window interface object.
Attachment #8979147 - Flags: review?(nika) → review+
Comment on attachment 8979148 [details]
Bug 1463016: Part 4 - Update XPIDL interfaces to use nsPIDOMWindow(Inner|Outer) rather than nsIDOMWindow.

https://reviewboard.mozilla.org/r/245396/#review264286

rs=me, though I think you'll need to change some of this due to my request to use 'nsGlobalWindowInner' instead of 'nsPIDOMWindowInner' in xpidl to make it match WebIDL.

::: dom/commandhandler/nsPICommandUpdater.idl:28
(Diff revision 1)
>     
>    /*
>     * Notify the command manager that the status of a command
>     * changed. It may have changed from enabled to disabled,
>     * or vice versa, or become toggled etc.
>     */
>  	void		commandStatusChanged(in string aCommandName);
>  	

Can we fix this trailing whitespace?
Attachment #8979148 - Flags: review?(nika) → review+
Comment on attachment 8979149 [details]
Bug 1463016: Part 5 - Add domWindow property to DocShellTreeItem and update callers to use it.

https://reviewboard.mozilla.org/r/245398/#review264288

rs=me - I scanned through a few files & scrolled through to make sure nothing seemed super weird.

::: browser/components/feeds/WebContentConverter.js:497
(Diff revision 1)
> -                         .getInterface(Ci.nsIWebNavigation)
> -                         .QueryInterface(Ci.nsIDocShellTreeItem)
> -                         .rootTreeItem
> -                         .QueryInterface(Ci.nsIInterfaceRequestor)
> -                         .getInterface(Ci.nsIDOMWindow)
>                           .wrappedJSObject;

Why are you accessing 'wrappedJSObject' on the domWindow object here?

I'm not sure what the point of that is.
Attachment #8979149 - Flags: review?(nika) → review+
Comment on attachment 8979152 [details]
Bug 1463016: Part 8 - Remove remaining C++ uses of nsIDOMWindow.

https://reviewboard.mozilla.org/r/245404/#review264292

::: dom/base/nsGlobalWindowInner.cpp:366
(Diff revision 1)
>    }
>    void Forget() { mWindow = nullptr; }
>    NS_IMETHOD GetInterface(const nsIID& aIID, void** aResult) override
>    {
>      if (mWindow) {
> -      if (aIID.Equals(NS_GET_IID(nsIDOMWindow))) {
> +      if (aIID.Equals(NS_GET_IID(nsIDOMWindow)) ||

Shouldn't we be removing this usage here?
Attachment #8979152 - Flags: review?(nika) → review+
Comment on attachment 8979153 [details]
Bug 1463016: Part 9 - Remove nsIDOMWindow.

https://reviewboard.mozilla.org/r/245406/#review264294

::: testing/mochitest/tests/Harness_sanity/test_SpecialPowersExtension.html:85
(Diff revision 1)
> -  ok(SpecialPowers.do_QueryInterface(window, "nsIDOMWindow"));
> -  is(SpecialPowers.getPrivilegedProps(SpecialPowers.do_QueryInterface(window, "nsIDOMWindow"), "document.nodeName"), "#document");
> +  ok(SpecialPowers.do_QueryInterface(window, "nsISupports"));
> +  is(SpecialPowers.getPrivilegedProps(SpecialPowers.do_QueryInterface(window, "nsISupports"), "document.nodeName"), "#document");

This feels like a slightly unnecessary test now...
Attachment #8979153 - Flags: review?(nika) → review+
Comment on attachment 8979150 [details]
Bug 1463016: Part 6 - Allow JS callers to Window constructors as IID.

https://reviewboard.mozilla.org/r/245400/#review264290

::: commit-message-2d6bb:12
(Diff revision 1)
> +I took the simple approach here, and hard coded the handling of the Window
> +prototype. That's the general approach a lot of the rest of XPCConvert takes,
> +and I don't immediately forsee us needing to add other interfaces in the
> +future, so it seems sufficient.

Only handling one type is reasonable here, but it seems like putting this capability inside of XPConnect rather than in JS might be overly complex  :-/.

::: js/xpconnect/src/XPCJSID.cpp:796
(Diff revision 1)
> +    if (dom::IsDOMConstructor(obj)) {
> +        const js::Class* clasp = js::GetObjectClass(obj);
> +        auto iface = dom::DOMIfaceAndProtoJSClass::FromJSClass(clasp);
> +        switch (iface->mPrototypeID) {
> +        case dom::prototypes::id::Window:
> +            return &NS_GET_IID(nsPIDOMWindowOuter);

Why are we getting the outer window's IID when we get a 'Window' object's prototype?

It also might be an option to just define a 

    const INNER_WINDOW_IID = Components.ID("{775dabc9-8f43-4277-9adb-f1990d77cffb}")

in XPCOMUtils.jsm?
Comment on attachment 8979151 [details]
Bug 1463016: Part 7 - Update remaining getInterface(nsIDOMWindow) callers to use Window.

https://reviewboard.mozilla.org/r/245402/#review264296

::: dom/base/nsGlobalWindowInner.cpp:369
(Diff revision 1)
> +      if (aIID.Equals(NS_GET_IID(nsPIDOMWindowOuter))) {
> +        return mWindow->GetOuterWindow()->QueryInterface(aIID, aResult);
> +      }
> +    }

I think this should be a nsPIDOMWindowInner query, and that we should use nsPIDOMWindowInner's IID in the previous XPCJSID patch.
Attachment #8979151 - Flags: review?(nika)
> IIRC we should be able to also do `event.target instanceof Window` because we define `[Symbol.hasInstance]` on the `Window` object.

We want to stop defining that symbol, in the long run.
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #27)
> > IIRC we should be able to also do `event.target instanceof Window` because we define `[Symbol.hasInstance]` on the `Window` object.
> 
> We want to stop defining that symbol, in the long run.

Even if we didn't, I'd be worried about using instanceof checks here. Even with the `hasInstance` hook, I'd expect it to touch Window.prototype, which we really want to lazify (bug 1475700). Window.isInstance shouldn't have that problem.
Blocks: 1463291
Keywords: leave-open
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d69b4fb1ed43751cfcbc0b4f2fe3b6a49bc0494
Bug 1463016: Part 5 - Add domWindow property to DocShellTreeItem and update callers to use it. r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/60c1c783f5e100e1e43b09be2f11b4d1d0b82bdf
Bug 1463016: Part 5 - Add domWindow property to DocShellTreeItem and update callers to use it. r=nika
Depends on: 1479288
Status: NEW → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Flags: needinfo?(kmaglione+bmo)

Any plans to land the other parts? As far as I can see, only part 5 landed so far.

Flags: needinfo?(kmaglione+bmo)
Depends on: 1523020
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.