1420285 - Change <browser> attribute isPreloadBrowser to preloadedState

Status: RESOLVED FIXED

(Firefox :: New Tab Page, defect, P1)

Description

[Ursula Sarracini (:ursula)]

In order for Activity Stream to update the preloaded page to fix ticket: https://github.com/mozilla/activity-stream/issues/3331 , we first want to allow the RemotePageManager to know which message port belongs to the preloaded browser so we can directly target that one.

As discussed with :mconley, the approach we decided to take is to change the browser's attribute "isPreloadBrowser" to instead be something along the lines of "preloadedState" where the preloaded browser will have a "preloaded" state that will change to "consumed" once we use it for the newtab and then finally be removed altogether when we navigate away. That way the RemotePageManager can just check if the browser has the correct preloaded state and pass that info to Activity Stream.

Comment 1

[Ed Lee :Mardak]

Alternatively, from a xul:browser, one could check if its gBrowser ancestor has _preloadedBrowser pointing at it.

Comment 2

[Ursula Sarracini (:ursula)]

Attachment: Bug 1420285 - Change <browser> attribute isPreloadBrowser to preloadedState

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

Comment 3

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

Comment on attachment 8939927 [details]
Bug 1420285 - Change <browser> attribute isPreloadBrowser to preloadedState

https://reviewboard.mozilla.org/r/209984/#review215952

Works for me, thanks! Just a few small notes below.

::: browser/base/content/tabbrowser.xml:2254
(Diff revision 1)
> +             *   3. When we then navigate away from the new tab, the 'preloadedState'
> +             *      attribute is removed from the browser altogether and the
> +             *      process starts over again for the next preloaded browser

It's also possible to have multiple browsers in the "preloadedState" == "consumed" state though, right?

Perhaps instead of saying here in point 3 that the process starts over again, perhaps we should say that "consumed" browsers will attempt to switch to a new content process when navigating away from about:newtab.

::: dom/base/test/browser_aboutnewtab_process_selection.js:93
(Diff revision 1)
> +  let preloadedTabState = gBrowser._preloadedBrowser.getAttribute("preloadedState");
> +  is(preloadedTabState, PRELOADED_STATE, "The preloaded browser has the correct attribute");

We might want to check for preloadedState to be PRELOADED_STATE on the first preloaded tab before it gets consumed too.

::: dom/base/test/browser_aboutnewtab_process_selection.js:99
(Diff revision 1)
> +  is(preloadedTabState, PRELOADED_STATE, "The preloaded browser has the correct attribute");
> +
> +  // Navigate away and check that the attribute has been removed altogether
> +  gBrowser.selectedBrowser.loadURI(TEST_URL);
> +  let navigatedTabHasState = gBrowser.selectedBrowser.hasAttribute("preloadedState");
> +  is(navigatedTabHasState, false, "Correctly removed the preloadState attribute when navigating away");

I think it's more idiomatic in our testing code to do something like:

```js
ok(!navigatedTabHasState, "Correctly removed the preloadState attribute when navigating away");
```

Comment 4

[Ursula Sarracini (:ursula)]

Comment on attachment 8939927 [details]
Bug 1420285 - Change <browser> attribute isPreloadBrowser to preloadedState

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

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b30d693096e3
Change <browser> attribute isPreloadBrowser to preloadedState r=mconley

Comment 6

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/b30d693096e3

Comment 7

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/activity-stream/pull/3919

Comment 8

[[github robot]]

Commits pushed to master at https://github.com/mozilla/activity-stream

https://github.com/mozilla/activity-stream/commit/4453f5de2aeaa9c350cc50064a5694664cb41a70
chore(telemetry): Backport Bug 1420285 - Changes to TelemetryFeed for preloaded sessions

https://github.com/mozilla/activity-stream/commit/595beeebe6882d0d3989b12230616ee62e3130b6
Merge pull request #3919 from sarracini/gh3916

chore(telemetry): Backport Bug 1420285 - Changes to TelemetryFeed for preloaded sessions