Closed Bug 1463291 Opened Last year Closed Last year

Move Document.docShell getter to Window

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

Details

Attachments

(1 file)

I ran into the weirdness of the current setup in bug 1463016, when I was getting rid of docShell.getInterface(nsIDOMWindow) calls.

DocShells are tied to outer windows (or, really, outer windows are tied to docShells), not documents, so having the docShell getter on the document is kind of odd to begin with. But most of the time we want to get a docShell, we're dealing with a window object, rather than a document, and having to do `window.document.docShell` rather than just `window.docShell` makes it even weirder.
Comment on attachment 8979407 [details]
Bug 1463291: Move docShell getter from Document to Window.

https://reviewboard.mozilla.org/r/245578/#review251660

r=me

::: browser/base/content/test/general/browser_domFullscreen_fullscreenMode.js:31
(Diff revision 1)
>        inDOMFullscreen: !!content.document.fullscreenElement,
>        inFullscreen: content.fullScreen
>      });
>    });
>    function waitUntilActive() {
> -    let doc = content.document;
> +    if (docShell.isActive && content.document.hasFocus()) {

Should be content.docShell.isActive, right?

::: dom/html/test/browser_fullscreen-api-keys.js:41
(Diff revision 1)
>    doc.addEventListener("keydown", keyHandler, true);
>    doc.addEventListener("keyup", keyHandler, true);
>    doc.addEventListener("keypress", keyHandler, true);
>  
>    function waitUntilActive() {
> -    if (doc.docShell.isActive && doc.hasFocus()) {
> +    if (docShell.isActive && doc.hasFocus()) {

content.docShell.isActive?

::: dom/html/test/browser_fullscreen-contextmenu-esc.js:16
(Diff revision 1)
>    addMessageListener("Test:QueryFullscreenState", () => {
>      sendAsyncMessage("Test:FullscreenState",
>                       !!content.document.fullscreenElement);
>    });
>    function waitUntilActive() {
> -    let doc = content.document;
> +    if (docShell.isActive && content.document.hasFocus()) {

content.docShell.isActive?

::: dom/webidl/Document.webidl
(Diff revision 1)
>    [ChromeOnly, Throws]
>    void obsoleteSheet(URI sheetURI);
>    [ChromeOnly, Throws]
>    void obsoleteSheet(DOMString sheetURI);
>  
> -  [ChromeOnly] readonly attribute nsIDocShell? docShell;

How sure are you that you found all the callers?
Attachment #8979407 - Flags: review?(bzbarsky) → review+
Comment on attachment 8979407 [details]
Bug 1463291: Move docShell getter from Document to Window.

https://reviewboard.mozilla.org/r/245578/#review251660

> Should be content.docShell.isActive, right?

Nope. This is a frame script, so there's already a docShell global that does what we want. I initially did make all of these content.docShell.isActive, but updated them when I realized it didn't make sense.
Comment on attachment 8979407 [details]
Bug 1463291: Move docShell getter from Document to Window.

https://reviewboard.mozilla.org/r/245578/#review251660

> How sure are you that you found all the callers?

I missed a couple that showed up in my try run, but I'll double check before I land.

I did manually check all of the JS .docShell references before I submitted for review, but I missed one that used destructuring, and a couple of others in JSMs, because apparently my `ag` got updated and lost my modifications to search JSMs when I use --js.
Priority: -- → P2
Kris, is there something blocking this landing?  It would be pretty useful for bug 1476142.
Flags: needinfo?(kmaglione+bmo)
A lot of the changes are on top of some of the patches from bug 1463016, since this started as a follow-up to that. I was hoping to get it all landed at once, but I'll try rebase this and see how messy it gets.
Ah, I thought this was under bug 1463016, not on top.

If the other is pretty close (days?) to landing, I wouldn't worry about rebasing...
Most of the locations that I updated for https://reviewboard.mozilla.org/r/245398/diff/1#index_header also needed to be updated for this, so there wasn't really much choice other than doing it on top. I suspect I can extract that part of the patch set and land this on top of it without too much trouble.
Flags: needinfo?(kmaglione+bmo)
Depends on: 1463016
https://hg.mozilla.org/mozilla-central/rev/544320932f97
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Depends on: 1479288
Flags: needinfo?(kmaglione+bmo)
Depends on: 1523020
No longer depends on: 1523020
You need to log in before you can comment on or make changes to this bug.