Closed Bug 1425494 Opened 6 years ago Closed 6 years ago

Use mapping from user events to browsers to set load info

Categories

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

Product:
Firefox
Component:
New Tab Page
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 62
Iteration:
62.1 - May 21
Tracking Flags:
Tracking Status
firefox59 --- wontfix
firefox60 --- wontfix
firefox62 --- fixed

People

(Reporter: Mardak, Assigned: nanj)

References

(Blocks 3 open bugs)

Details

(Whiteboard: [reporting] [AS61MVP])

Attachments

(1 file)

Bug 1425494 - Make browser-open-newtab-start notify with extra info.
59 bytes, text/x-review-board-request
dmosedale
: review+
mstriemer
: review+
Details 
Split from #3118 that was worked around by #3128. @dmose and I discussed with mconley about having BrowserOpenTab@browser.js notify with a Promise that resolves with the browser that is used to open the about:newtab page.
Iteration: 1.26 → 1.27
Iteration: 1.27 → 60.1 - Jan 29
Iteration: 60.1 - Jan 29 → 60.2 - Feb 12
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.
See Also: → https://github.com/mozilla/activity-stream/issues/3556
Severity: normal → enhancement
status-firefox60: --- → affected
Whiteboard: [reporting] → [reporting] [AS60MVP]
Iteration: 60.2 - Feb 12 → 60.3 - Feb 26
Iteration: 60.3 - Feb 26 → 60.4 - Mar 12
Iteration: 60.4 - Mar 12 → ---
status-firefox60: affected → wontfix
Priority: P2 → P3
Blocks: 1437659
Iteration: --- → 61.1 - Mar 26
Whiteboard: [reporting] [AS60MVP] → [reporting] [AS61MVP]
Iteration: 61.1 - Mar 26 → ---
Iteration: --- → 61.2 - Apr 9
Priority: P3 → P2
Blocks: 1445083
Blocks: 1448356
Assignee: nobody → najiang
Iteration: 61.2 - Apr 9 → 61.3 - Apr 23
Iteration: 61.3 - Apr 23 → 61.4 - May 7
Iteration: 61.4 - May 7 → 62.1 - May 21
No longer blocks: 1437659
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.
Attachment #8971405 - Flags: review?(dmose) → review-
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 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 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.
Attachment #8971405 - Flags: review?(mstriemer) → review+
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 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!
Attachment #8971405 - Flags: review?(dmose) → review+
See Also: → 1458043
Pushed by edilee@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9111dee5d501
Make browser-open-newtab-start notify with extra info. r=dmose,mstriemer
https://hg.mozilla.org/mozilla-central/rev/9111dee5d501
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Component: Activity Streams: Newtab → New Tab Page
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: