TabChild's should be able to identify which window they're in

RESOLVED FIXED in Firefox 63

Status

()

enhancement
RESOLVED FIXED
Last year
11 months ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

58 Branch
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(2 attachments)

The theme-ing API makes it possible to theme the browser, and also some in-content pages like about:newtab which run in content processes.

The theme-ing API also makes it possible to theme individual windows differently.

In bug 1474163, we hope to use the SharedData structure from bug 1463587 to store the in-content theme-ing data, mapped on the window IDs.

However, as it stands, TabChild's don't have a notion of window IDs. We should add that notion, and make sure that it tracks across frameloader swaps.
I think we should be okay to send down the chrome-window's top-level outer window ID as the identifier rather than inventing some kind of new ID scheme.
I think I can do this.
Assignee: nobody → mconley
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Hey baku, if you're not the right person to review this stuff, let me know. I'm hoping, in particular, that you can look at the TabContext changes in that first patch.

Comment 7

Last year
mozreview-review
Comment on attachment 8991397 [details]
Bug 1474662 - Add chromeOuterWindowID to TabContext.

https://reviewboard.mozilla.org/r/256258/#review263244

::: dom/base/nsFrameLoader.cpp:3359
(Diff revision 2)
>        showAccelerators =
>          root->ShowAccelerators() ? UIStateChangeType_Set : UIStateChangeType_Clear;
>        showFocusRings =
>          root->ShowFocusRings() ? UIStateChangeType_Set : UIStateChangeType_Clear;
> +
> +      nsCOMPtr<nsPIDOMWindowOuter> outerWin = root->GetWindow();

Nit: This can be a raw pointer. No need for the refcount overhead.

::: dom/base/nsInProcessTabChildGlobal.cpp:233
(Diff revision 2)
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    aError.Throw(rv);
> +    return 0;
> +  }
> +
> +  nsCOMPtr<nsPIDOMWindowOuter> topWin = root->GetWindow();

Again, can just be a raw pointer.

::: dom/base/nsInProcessTabChildGlobal.cpp:235
(Diff revision 2)
> +    return 0;
> +  }
> +
> +  nsCOMPtr<nsPIDOMWindowOuter> topWin = root->GetWindow();
> +  if (!topWin) {
> +    aError.Throw(NS_ERROR_NULL_POINTER);

Should probably be NS_ERROR_UNEXPECTED. The null pointer is an implementation detail callers don't need to know about.

::: dom/chrome-webidl/MessageManager.webidl:487
(Diff revision 2)
> +
> +  /**
> +   * Returns the outerWindowID of the browser window hosting the frame.
> +   */
> +  [Throws]
> +  readonly attribute long long chromeOuterWindowID;

Honestly, there's probably not much point in having this throw. The OOP case will never throw, and just returns 0 when it can't be mapped to a window. The in-process case should do the same for consistency.

Please also document what returning 0 means.
Attachment #8991397 - Flags: review?(kmaglione+bmo) → review+

Comment 8

Last year
mozreview-review
Comment on attachment 8991397 [details]
Bug 1474662 - Add chromeOuterWindowID to TabContext.

https://reviewboard.mozilla.org/r/256258/#review263338
Attachment #8991397 - Flags: review?(amarchesini) → review+

Comment 9

Last year
mozreview-review
Comment on attachment 8991454 [details]
Bug 1474662 - Add tests for chromeOuterWindowID.

https://reviewboard.mozilla.org/r/256338/#review263340
Attachment #8991454 - Flags: review?(amarchesini) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 12

Last year
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5862122c0f0c
Add chromeOuterWindowID to TabContext. r=baku,kmag
https://hg.mozilla.org/integration/autoland/rev/439d4db9f0a4
Add tests for chromeOuterWindowID. r=baku

Comment 13

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/5862122c0f0c
https://hg.mozilla.org/mozilla-central/rev/439d4db9f0a4
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63

Updated

11 months ago
Depends on: 1475978

Updated

11 months ago
Blocks: 1475978
No longer depends on: 1475978
You need to log in before you can comment on or make changes to this bug.