Closed Bug 1476145 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: DOM: Core & HTML, enhancement, P3)

Product:
Core
Component:
DOM: Core & HTML
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(9 files)

part 1. Add a getter to get the nsIDOMWindowUtils from a window
(deleted), patch
nika
: review+
Details | Diff | Splinter Review
part 2. Stop using getInterface(nsIDOMWindowUtils) in accessible/
(deleted), patch
kmag
: review+
Details | Diff | Splinter Review
part 3. Stop using getInterface(nsIDOMWindowUtils) in browser/
(deleted), patch
Gijs
: review+
Details | Diff | Splinter Review
part 4. Stop using getInterface(nsIDOMWindowUtils) in devtools
(deleted), patch
bgrins
: review+
Details | Diff | Splinter Review
part 5. Stop using getInterface(nsIDOMWindowUtils) in DOM code
(deleted), patch
mccr8
: review+
Details | Diff | Splinter Review
part 6. Stop using getInterface(nsIDOMWindowUtils) in toolkit
(deleted), patch
mossop
: review+
Details | Diff | Splinter Review
part 7. Stop using getInterface(nsIDOMWindowUtils) in mobile/
(deleted), patch
snorp
: review+
Details | Diff | Splinter Review
part 8. Stop using getInterface(nsIDOMWindowUtils) in various test code
(deleted), patch
kmag
: review+
Details | Diff | Splinter Review
part 9. Drop support for getting window utils via getInterface
(deleted), 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)
Attachment #8993783 - Flags: review?(gijskruitbosch+bugs)
Attachment #8993785 - Flags: review?(continuation)
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.
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+
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
Blocks: 1480735
Blocks: 1480970
Blocks: 1480982
Blocks: 1481401
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.

Attachment

General

Creator:
Created:
Updated:
Size: