1425494 - Use mapping from user events to browsers to set load info

Status: RESOLVED FIXED

(Firefox :: New Tab Page, enhancement, P2)

Description

[Ed Lee :Mardak]

Split from #3118 that was worked around by #3128. @dmose and I discussed with mconley about having [e-mail] notify with a Promise that resolves with the browser that is used to open the about:newtab page.

Comment 1

[Ed Lee :Mardak]

From https://github.com/mozilla/activity-stream/issues/3556

The most important, "first_window_open", was implemented heuristically in #2658. The rest will probably need to be implemented in a style similar to that for about:newtab described in #3138, and should probably happen after that and #2165 are finished, as they can probably leverage a bunch of the code changes that will happen there.

Note that there are, as of this writing, extensive comments in TelemetryFeed.jsm describing some of the cases that will want to be handled.

Comment 2

[Nan Jiang [:nanj]]

Attachment: Bug 1425494 - Make browser-open-newtab-start notify with extra info.

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

Comment 3

[Nan Jiang [:nanj]]

Comment on attachment 8971405 [details]
Bug 1425494 - Make browser-open-newtab-start notify with extra info.

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

Comment 4

[Nan Jiang [:nanj]]

Comment on attachment 8971405 [details]
Bug 1425494 - Make browser-open-newtab-start notify with extra info.

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

Comment 5

[Nan Jiang [:nanj]]

Comment on attachment 8971405 [details]
Bug 1425494 - Make browser-open-newtab-start notify with extra info.

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

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8971405 [details]
Bug 1425494 - Make browser-open-newtab-start notify with extra info.

https://reviewboard.mozilla.org/r/240160/#review249814

This is looking good, but there some tweaks needed!  Sorry for the delay on the review.  Feel free to ping my to chat about this stuff by Vidyo if you want faster feedback about any of my comments.

::: browser/base/content/browser.js:2363
(Diff revision 4)
> +  // entry points, this won't catch every single tab created, but most
> +  // initiated by the user should go through here.
> +  //
> +  // Note 1: This notification gets notified with following information:
> +  //  * isWindowPrivate: A boolean indicating if this happens in the private
> +  //    window

Why do we want isWindowPrivate?

::: browser/base/content/test/tabs/browser_open_newtab_start_observer_notification.js:6
(Diff revision 4)
>  
>  add_task(async function test_browser_open_newtab_start_observer_notification() {
> -
>    let observerFiredPromise = new Promise(resolve => {
> -    function observe() {
> -      resolve();
> +    function observe(subject) {
> +      Services.obs.removeObserver(observe, "browser-open-newtab-start");

Good catch!

::: browser/base/content/test/tabs/browser_open_newtab_start_observer_notification.js:7
(Diff revision 4)
>  add_task(async function test_browser_open_newtab_start_observer_notification() {
> -
>    let observerFiredPromise = new Promise(resolve => {
> -    function observe() {
> -      resolve();
> +    function observe(subject) {
> +      Services.obs.removeObserver(observe, "browser-open-newtab-start");
> +      resolve(subject);

It might (or might not) be more readable to resolve with subject.wrappedJSObject here and adjust the code below.  Your call.

::: browser/base/content/test/tabs/browser_open_newtab_start_observer_notification.js:29
(Diff revision 4)
> +
> +  BrowserTestUtils.removeTab(tab);
> +});
> +
> +add_task(async function test_browser_open_newtab_start_observer_notification_in_private_window() {
> +  let observerFiredPromise = new Promise(resolve => {

Rather than duplicating this promise, can't it just be hoisted out of the scope to the top of the file?

::: browser/base/content/test/tabs/browser_open_newtab_start_observer_notification.js:53
(Diff revision 4)
> +  ok(true, "browser-open-newtab-start observer not called");
> +  ok(isWindowPrivate, "should return the correct isWindowPrivate flag");
> +  Assert.deepEqual(browser, tab.linkedBrowser, "browser-open-newtab-start notified with the created browser");
>  
>    BrowserTestUtils.removeTab(tab);
> +  await BrowserTestUtils.closeWindow(win);

Also a good catch!

::: browser/base/content/utilityOverlay.js:559
(Diff revision 4)
>      });
>      targetBrowser = tabUsedForLoad.linkedBrowser;
>  
> +    if (aResolveOnNewTabCreated) {
> +      // Resolves the promise with the newly created browser, which could be used as the
> +      // identifier of the "browser-open-newtab-start" notification. See #1425494.

I think you can probably drop this comment.  The code is pretty clear, and anyone who wants to find out more about the etiology of it can find the bug number in searchfox (or hg) blame.

::: browser/components/extensions/ExtensionControlledPopup.jsm:128
(Diff revision 4)
>      // Remove the observer here so we don't get multiple open() calls if we get
>      // multiple observer events in quick succession.
>      this.removeObserver();
>  
>      // Do this work in an idle callback to avoid interfering with new tab performance tracking.
> -    this.topWindow.requestIdleCallback(() => this.open(subject));
> +    this.topWindow.requestIdleCallback(() => this.open());

I don't actually see how this could have ever done been sensible code, since up until this patch, subject has always been called back as null. Which makes me suspect that this change is OK, but we should probably have :aswan take a look.

::: browser/components/extensions/ExtensionControlledPopup.jsm:153
(Diff revision 4)
>          this.onObserverAdded();
>        }
>      }
>    }
>  
> -  async open(targetWindow) {
> +  async open() {

I don't think this change works, however.  open() is exposed to callers of this class, and some of them use the parameter: https://searchfox.org/mozilla-central/source/browser/components/extensions/parent/ext-tabs.js#1234

That said, this line is going to need to change in a rebase before landing anyway.

If this line does change, I suspect it wants a test as well, as a quick skim suggests to me that there isn't currently anything that would ensure that the API actually functions in any particular way (though maybe I've missed something in the test dir).

Whatever we end up doing here (if anything) probably wants :aswan's eyes on it as well.

Comment 7

[Nan Jiang [:nanj]]

Comment on attachment 8971405 [details]
Bug 1425494 - Make browser-open-newtab-start notify with extra info.

https://reviewboard.mozilla.org/r/240160/#review250030

::: browser/base/content/browser.js:2363
(Diff revision 4)
> +  // entry points, this won't catch every single tab created, but most
> +  // initiated by the user should go through here.
> +  //
> +  // Note 1: This notification gets notified with following information:
> +  //  * isWindowPrivate: A boolean indicating if this happens in the private
> +  //    window

Since TelemetryFeed in AS is obeserving this notification regardless of the browsing mode, without this flag, it will have to perform the whole notification handling (i.e mark the timestamp and link it to its browser) although AS will never start a session measurement inside of a private browsing tab.

::: browser/base/content/test/tabs/browser_open_newtab_start_observer_notification.js:7
(Diff revision 4)
>  add_task(async function test_browser_open_newtab_start_observer_notification() {
> -
>    let observerFiredPromise = new Promise(resolve => {
> -    function observe() {
> -      resolve();
> +    function observe(subject) {
> +      Services.obs.removeObserver(observe, "browser-open-newtab-start");
> +      resolve(subject);

Sounds good, will fix it up

::: browser/base/content/utilityOverlay.js:559
(Diff revision 4)
>      });
>      targetBrowser = tabUsedForLoad.linkedBrowser;
>  
> +    if (aResolveOnNewTabCreated) {
> +      // Resolves the promise with the newly created browser, which could be used as the
> +      // identifier of the "browser-open-newtab-start" notification. See #1425494.

Sounds good.

::: browser/components/extensions/ExtensionControlledPopup.jsm:128
(Diff revision 4)
>      // Remove the observer here so we don't get multiple open() calls if we get
>      // multiple observer events in quick succession.
>      this.removeObserver();
>  
>      // Do this work in an idle callback to avoid interfering with new tab performance tracking.
> -    this.topWindow.requestIdleCallback(() => this.open(subject));
> +    this.topWindow.requestIdleCallback(() => this.open());

Agreed, since we have somewhat changed the semantics of this notification, we should reach out to :aswan to confirm it.

::: browser/components/extensions/ExtensionControlledPopup.jsm:153
(Diff revision 4)
>          this.onObserverAdded();
>        }
>      }
>    }
>  
> -  async open(targetWindow) {
> +  async open() {

Nice catch. Will talk to :aswan to sort out the reasonable solution here.

Thanks :dmose for the inputs!

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8971405 [details]
Bug 1425494 - Make browser-open-newtab-start notify with extra info.

https://reviewboard.mozilla.org/r/240160/#review250030

> Since TelemetryFeed in AS is obeserving this notification regardless of the browsing mode, without this flag, it will have to perform the whole notification handling (i.e mark the timestamp and link it to its browser) although AS will never start a session measurement inside of a private browsing tab.

Could TelemetryFeed just call PrivateBrowserUtils.isBrowserPrivate() on the browser that is resolved?

Comment 9

[Nan Jiang [:nanj]]

Comment on attachment 8971405 [details]
Bug 1425494 - Make browser-open-newtab-start notify with extra info.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/240160/diff/4-5/

Comment 10

[Nan Jiang [:nanj]]

Comment on attachment 8971405 [details]
Bug 1425494 - Make browser-open-newtab-start notify with extra info.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/240160/diff/5-6/

Comment 11

[Mark Striemer [:mstriemer]]

Comment on attachment 8971405 [details]
Bug 1425494 - Make browser-open-newtab-start notify with extra info.

https://reviewboard.mozilla.org/r/240160/#review250420

The ExtensionControlledPopup changes look good to me.

::: browser/components/extensions/ExtensionControlledPopup.jsm:133
(Diff revision 6)
>      await ExtensionSettingsStore.initialize();
>      return ExtensionSettingsStore.removeSetting(id, this.confirmedType, id);
>    }
>  
>    observe(subject, topic, data) {
> +    let targetWindow = null;

nit: may as well define this just before the `if`. Could leave it as `undefined` rather than assigning `null` as well.

Comment 12

[Nan Jiang [:nanj]]

Comment on attachment 8971405 [details]
Bug 1425494 - Make browser-open-newtab-start notify with extra info.

https://reviewboard.mozilla.org/r/240160/#review250420

> nit: may as well define this just before the `if`. Could leave it as `undefined` rather than assigning `null` as well.

fixed!

Comment 13

[Nan Jiang [:nanj]]

Comment on attachment 8971405 [details]
Bug 1425494 - Make browser-open-newtab-start notify with extra info.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/240160/diff/6-7/

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8971405 [details]
Bug 1425494 - Make browser-open-newtab-start notify with extra info.

https://reviewboard.mozilla.org/r/240160/#review250514

I just triaged the treeherder failure, and they all appeared to be known intermittents.  Looking with the profile, this notification does still get sent, when opening a new tab, so that hasn't be regressed.  :-)  And the API has been simplified.  r=dmose, thanks for doing this!

Comment 15

[Nan Jiang [:nanj]]

Comment on attachment 8971405 [details]
Bug 1425494 - Make browser-open-newtab-start notify with extra info.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/240160/diff/7-8/

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9111dee5d501
Make browser-open-newtab-start notify with extra info. r=dmose,mstriemer

Comment 17

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/9111dee5d501