Kris Maglione [:kmag] Assignee
	Description • 6 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 2 • 6 years ago
mozreview-review

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?

	Kris Maglione [:kmag] Assignee
	Comment 3 • 6 years ago
mozreview-review-reply

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.

	Kris Maglione [:kmag] Assignee
	Comment 4 • 6 years ago
mozreview-review-reply

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.

	Boris Zbarsky [:bzbarsky]
	Comment 5 • 6 years ago

Kris, is there something blocking this landing?  It would be pretty useful for bug 1476142.

	Kris Maglione [:kmag] Assignee
	Comment 6 • 6 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 7 • 6 years ago

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...

	Kris Maglione [:kmag] Assignee
	Comment 8 • 6 years ago

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.

	Kris Maglione [:kmag] Assignee
	Comment 9 • 6 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/fcfb99baa0f0fb60a7c420a712c6ae7c72576871
Bug 1463291: Move docShell getter from Document to Window. r=bz

	Narcis Beleuzu [:NarcisB]
	Comment 10 • 6 years ago

Backed out for geckoview failures.

Push link: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=fcfb99baa0f0fb60a7c420a712c6ae7c72576871
Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/aaee68beff800bcf076d9c49b5c1f6531b135579
Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=190748974&repo=mozilla-inbound&lineNumber=2050

	Kris Maglione [:kmag] Assignee
	Comment 11 • 6 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/544320932f97d62f477bedd6296690fb164b846f
Bug 1463291: Move docShell getter from Document to Window. r=bz

	Natalia Csoregi [:nataliaCs]
	Comment 12 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/544320932f97