Add a webidl getter to get the DOMWindowUtils from a given window

RESOLVED FIXED in Firefox 63

Status

()

enhancement
P3
normal
RESOLVED FIXED
Last year
4 months ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

(Blocks 1 bug)

unspecified
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(9 attachments)

8.14 KB, patch
Nika
: review+
Details | Diff | Splinter Review
5.22 KB, patch
kmag
: review+
Details | Diff | Splinter Review
89.60 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
47.52 KB, patch
bgrins
: review+
Details | Diff | Splinter Review
56.40 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
55.09 KB, patch
mossop
: review+
Details | Diff | Splinter Review
28.38 KB, patch
snorp
: review+
Details | Diff | Splinter Review
71.33 KB, patch
kmag
: review+
Details | Diff | Splinter Review
8.17 KB, patch
Nika
: review+
Details | Diff | Splinter Review
See bug 1341546 comment 6.

This will also nix a bunch if QI and getInterface calls, which is always good.  We can additionally think about moving some getters from nsIDOMWindowUtils to Window, but shouldn't block on that.
Priority: -- → P3
The new attribute is not [Cached] because we would need to bump
JSCLASS_GLOBAL_APPLICATION_SLOTS for that and it's not obvious that we should do
that.
Attachment #8993781 - Flags: review?(nika)
This is not quite a mechanical change, because some places have a .top or
whatnot snuck in there, so please review carefully!
Attachment #8993786 - Flags: review?(dtownsend)
Attachment #8993784 - Flags: review?(bgrinstead) → review+
Comment on attachment 8993785 [details] [diff] [review]
part 5.  Stop using getInterface(nsIDOMWindowUtils) in DOM code

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

I was going to say this looked like a bug I filed a while ago, but I see you already made this one blocking that one!

::: dom/push/Push.js
@@ +196,5 @@
>        cancel: cancelCallback,
>        window: this._window,
>      };
>  
>      // Using askPermission from nsIDOMWindowUtils that takes care of the

This comment needs to be updated, but I guess if you are on the path to remove that interface anyways, it can be left for that patch.
Attachment #8993785 - Flags: review?(continuation) → review+
(In reply to Andrew McCreight [:mccr8] from comment #10)
> This comment needs to be updated, but I guess if you are on the path to
> remove that interface anyways, it can be left for that patch.

Never mind, I guess the interface is still the same. I'd assumed that the WebIDLization of the utils getter implied the WebIDLization of the utils interface.

Updated

11 months ago
Attachment #8993782 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8993788 [details] [diff] [review]
part 8.  Stop using getInterface(nsIDOMWindowUtils) in various test code

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

::: docshell/test/navigation/NavigationUtils.js
@@ +187,5 @@
>          (win.document.body.textContent.trim() == body ||
>           win.document.body.textContent.trim() == popup_body) && 
>          win.document.readyState == "complete") {
>  
> +      var util = win.windowUtils;

Probably may as well get rid of the `util` variable at this point.

::: layout/style/test/chrome/test_bug1157097.html
@@ +9,5 @@
>  </style>
>  <body onload=run()>
>  <p><span id=s1 class=blue><b></b></span><span id=s2 class=red><b></b></span></p>
>  <script>
> +var windowUtils = window.windowUtils;

This should basically be a no-op... Maybe just remove?

::: testing/marionette/listener.js
@@ +701,5 @@
>    }
>  
>    // we get here if we're not in asyncPacZoomEnabled land, or if we're
>    // the main process
> +  let domWindowUtils = win.windowUtils;

Can probably just get rid of this variable too.

::: testing/specialpowers/content/MockFilePicker.jsm
@@ -226,5 @@
>      };
>    },
>    get domFileOrDirectoryEnumerator() {
> -    this.parent.QueryInterface(Ci.nsIInterfaceRequestor)
> -               .getInterface(Ci.nsIDOMWindowUtils);

Well, that's odd...

::: testing/talos/talos/pageloader/chrome/pageloader.js
@@ +146,5 @@
>        }
>      }
>  
>      if (forceCC &&
> +        !window.windowUtils.garbageCollect) {

Maybe move to the previous line?
Attachment #8993788 - Flags: review?(kmaglione+bmo) → review+
> I'd assumed that the WebIDLization of the utils getter implied the WebIDLization of the utils interface.

That's bug 1341546, which may or may not happen.

> Probably may as well get rid of the `util` variable at this point.

Fair.  I got into a fairly mechanical groove... ;)  Got rid of it.

> This should basically be a no-op... Maybe just remove?

Good catch.  Done.

> Can probably just get rid of this variable too.

Done.

> Well, that's odd...

Yeah, it used to be an assignment to an unused variable, and then one of our lint fixes removed the unused variable and the assignment but left the potentially-side-effecty expression.  Or something like that.

> Maybe move to the previous line?

Done.
Attachment #8993781 - Flags: review?(nika) → review+
Comment on attachment 8993789 [details] [diff] [review]
part 9.  Drop support for getting window utils via getInterface

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

::: dom/ipc/TabParent.cpp
@@ +2480,5 @@
>  
>  mozilla::ipc::IPCResult
>  TabParent::RecvIsParentWindowMainWidgetVisible(bool* aIsVisible)
>  {
> +  // XXXbz This looks unused; can we just remove it?

I think so...
Attachment #8993789 - Flags: review?(nika) → review+
> I think so...

I filed bug 1477343 on that earlier today.
Comment on attachment 8993783 [details] [diff] [review]
part 3.  Stop using getInterface(nsIDOMWindowUtils) in browser/

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

LGTM, just a few nits...

::: browser/base/content/browser-fullScreenAndPointerLock.js
@@ +492,5 @@
>      return gMultiProcessBrowser && aBrowser.getAttribute("remote") == "true";
>    },
>  
>    get _windowUtils() {
> +    return window.windowUtils;

Nit: please remove the getter and use `window.windowUtils` in its 2 consumers in this file.

::: browser/base/content/browser-pageActions.js
@@ +385,5 @@
>        action && this.urlbarButtonNodeIDForActionID(action.id),
>        this.mainButtonNode.id,
>        "identity-icon",
>      ];
> +    let dwu = window.windowUtils;

Nit: rm this line and then update line 394 to use `window.windowUtils` instead of `dwu`. This code isn't hot so I doubt there'll be any perf difference.

::: browser/base/content/browser-places.js
@@ +1211,1 @@
>        let iconBounds = dwu.getBoundsWithoutFlushing(libraryIcon);

Nit: remove the temp var here.

::: browser/base/content/browser.js
@@ +2564,5 @@
>  
> +    let win = doc.defaultView;
> +    let browser = win.getInterface(Ci.nsIWebNavigation)
> +                     .QueryInterface(Ci.nsIDocShell)
> +                     .chromeEventHandler;

While we're here, couldn't this just be `doc.docShell.chromeEventHandler` or is there a reason that doesn't work?

@@ +3243,5 @@
>    // Reset temporary permissions on the current tab. This is done here
>    // because we only want to reset permissions on user reload.
>    SitePermissions.clearTemporaryPermissions(gBrowser.selectedBrowser);
>  
> +  let windowUtils = window.windowUtils;

Nit: if instead you do `let handlingUserInput: window.windowUtils.isHandlingUserInput` then the options param to `Browser:Reload` just becomes `{ flags: reloadFlags, handlingUserInput }`.

::: browser/base/content/tab-content.js
@@ +409,1 @@
>    },

You can remove this getter entirely and just replace the line in `receiveMessage` with:

let windowUtils = content && content.windowUtils;

::: browser/base/content/tabbrowser.js
@@ +2841,1 @@
>          windowUtils.disableDialogs();

Nit: please combine these 2 lines.

@@ +3086,1 @@
>        dwu.suppressAnimation(true);

Nit: please combine these 2 lines

::: browser/components/translation/TranslationDocument.jsm
@@ +38,5 @@
>     * @param document  The document to be translated
>     */
>    _init(document) {
>      let window = document.defaultView;
> +    let winUtils = window.windowUtils;

Nit: `window` is unused after this, so may as well unify this with the line above.

::: browser/modules/FormSubmitObserver.jsm
@@ +214,5 @@
>      this._mm.sendAsyncMessage("FormValidation:HidePopup", {});
>    },
>  
>    _getWindowUtils() {
> +    return this._content.windowUtils;

There's only 1 callsite of this; can you remove the faux getter and just insert this in the 1 callsite?
Attachment #8993783 - Flags: review?(gijskruitbosch+bugs) → review+
Anthony, what will need to happen to merge the pocket change into the github repo? Do you or does someone else merge changes back from m-c into the pocket github repo, or something else?
Flags: needinfo?(anthony)
Comment on attachment 8993786 [details] [diff] [review]
part 6.  Stop using getInterface(nsIDOMWindowUtils) in toolkit

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

Looks good!
Attachment #8993786 - Flags: review?(dtownsend) → review+
> Nit: please remove the getter and use `window.windowUtils` in its 2 consumers in this file.

Done.

> Nit: rm this line and then update line 394 to use `window.windowUtils` instead of `dwu`. This code isn't hot so I doubt there'll be any perf difference.

Done.

> Nit: remove the temp var here.

Done.

> While we're here, couldn't this just be `doc.docShell.chromeEventHandler`

I think it could, yes.  See bug 1446940 which tracks making that change in general.  I'd be happier making that change explicitly instead of as part of this change.

> Nit: if instead you do `let handlingUserInput: window.windowUtils.isHandlingUserInput` then the options param to `Browser:Reload` just becomes `{ flags: reloadFlags, handlingUserInput }`.

Much cleaner.  Done.

> You can remove this getter entirely and just replace the line in `receiveMessage` with:

Done.

> Nit: please combine these 2 lines.

Done, for both copies of that nit.

> Nit: `window` is unused after this, so may as well unify this with the line above.

Done.

> There's only 1 callsite of this; can you remove the faux getter and just insert this in the 1 callsite?

Done.
Attachment #8993787 - Flags: review?(snorp) → review+

Comment 20

11 months ago
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b48b718c3a1
part 1.  Add a getter to get the nsIDOMWindowUtils from a window.  r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e6977be5455
part 2.  Stop using getInterface(nsIDOMWindowUtils) in accessible/.  r=kmag
https://hg.mozilla.org/integration/mozilla-inbound/rev/6daa67c0cae4
part 3.  Stop using getInterface(nsIDOMWindowUtils) in browser/.  r=gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/640fd7808af5
part 4.  Stop using getInterface(nsIDOMWindowUtils) in devtools.  r=bgrins
https://hg.mozilla.org/integration/mozilla-inbound/rev/0fb80adfe470
part 5.  Stop using getInterface(nsIDOMWindowUtils) in DOM code.  r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/6bd61a056a63
part 6.  Stop using getInterface(nsIDOMWindowUtils) in toolkit.  r=mossop
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed72643ae3ea
part 7.  Stop using getInterface(nsIDOMWindowUtils) in mobile/.  r=snorp
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2df9ae93b11
part 8.  Stop using getInterface(nsIDOMWindowUtils) in various test code.  r=kmag
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e9e04da5cb9
part 9.  Drop support for getting window utils via getInterface.  r=nika
Yes, done.
Summary: Add a webidl getter to get the DOMWindowUtils from a given window → Add a webidl getter to get the WindowUtils from a given window
Summary: Add a webidl getter to get the WindowUtils from a given window → Add a webidl getter to get the DOMWindowUtils from a given window

Updated

11 months ago
Blocks: 1480735

Updated

11 months ago
Blocks: 1480970

Updated

11 months ago
Blocks: 1480982

Updated

11 months ago
Blocks: 1481401

Comment 24

9 months ago
Pocket will be using code from m-c, no backwards merging needed.
Flags: needinfo?(anthony)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.