Closed
Bug 1463291
Opened 6 years ago
Closed 6 years ago
Move Document.docShell getter to Window
Categories
(Core :: DOM: Core & HTML, enhancement, P2)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla63
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: kmag, Assigned: kmag)
References
(Depends on 1 open bug)
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 hidden (mozreview-request) |
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?
Attachment #8979407 -
Flags: review?(bzbarsky) → review+
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.
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.
Updated•6 years ago
|
Priority: -- → P2
Comment 5•6 years ago
|
||
Kris, is there something blocking this landing? It would be pretty useful for bug 1476142.
Flags: needinfo?(kmaglione+bmo)
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.
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...
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.
Flags: needinfo?(kmaglione+bmo)
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
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
Flags: needinfo?(kmaglione+bmo)
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
Comment 12•6 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(kmaglione+bmo)
You need to log in
before you can comment on or make changes to this bug.
Description
•