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

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, enhancement)

Description

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

I think I can do this.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1474662 - Add chromeOuterWindowID to TabContext.

Review commit: https://reviewboard.mozilla.org/r/256258/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/256258/

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1474662 - Add tests for chromeOuterWindowID.

Review commit: https://reviewboard.mozilla.org/r/256338/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/256338/

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8991397 [details]
Bug 1474662 - Add chromeOuterWindowID to TabContext.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256258/diff/1-2/

Comment 6

[Mike Conley (:mconley) (:⚙️)]

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

[Kris Maglione [:kmag]]

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.

Comment 8

[Andrea Marchesini [:baku]]

Comment on attachment 8991397 [details]
Bug 1474662 - Add chromeOuterWindowID to TabContext.

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

Comment 9

[Andrea Marchesini [:baku]]

Comment on attachment 8991454 [details]
Bug 1474662 - Add tests for chromeOuterWindowID.

https://reviewboard.mozilla.org/r/256338/#review263340

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8991397 [details]
Bug 1474662 - Add chromeOuterWindowID to TabContext.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256258/diff/2-3/

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8991454 [details]
Bug 1474662 - Add tests for chromeOuterWindowID.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256338/diff/1-2/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
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

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/5862122c0f0c
https://hg.mozilla.org/mozilla-central/rev/439d4db9f0a4