1465697 - Add library_open, hamburger_open and dotdotdot_open event telemetry for Savant Shield study

Status: RESOLVED FIXED

(Firefox :: General, enhancement, P1)

Description

[Bianca Danforth [:bdanforth]]

This is one of a series of bugs that together comprise the Savant Shield study (tracking bug 1457226).

This bug will register and record (for the duration of the study only):
* When the user opens the library menu.
* When the user opens the hamburger menu.
* When the user opens the dotdotdot menu.

Comment 1

[Bianca Danforth [:bdanforth]]

Attachment: Bug 1465697 - Add menu open probes for Savant Shield study;

These probes will register and record (for the duration of the study only):
* When the library menu opens.
* When the hamburger menu opens.
* When the dotdotdot menu opens.

These will be captured in existing and new windows.

Note: The library panel is dynamically added and removed from the document (see PanelUI.js for where the panel is created and removed), so a listener can't be added to it in advance. While the 'command' event on the library toolbar button will work for most cases, it does not capture the case when the library menu is opened from the hamburger menu, and there may be edge cases when the panel does not show up from a 'command' event.

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

Comment 2

[Bianca Danforth [:bdanforth]]

:pdehaan, could you provide QA review on this patch? Instructions for obtaining the test build are here: https://github.com/biancadanforth/gecko-dev/issues/5

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8983757 [details]
Bug 1465697 - Add menu open probes for Savant Shield study;

https://reviewboard.mozilla.org/r/249588/#review255950

I downloaded the build from your trypush and was unable to get the events to show up on about:telemetry when opening the dotdotdot, library, and hamburger menu. Am I missing something?

::: browser/modules/SavantShieldStudy.jsm:190
(Diff revision 1)
> +  constructor(studyCategory) {
> +    this.STUDY_TELEMETRY_CATEGORY = studyCategory;
> +    this.LIBRARY_TOOLBARBUTTON_ID = "library-button";
> +    this.HAMBURGER_PANEL_ID = "appMenu-popup";
> +    this.DOTDOTDOT_PANEL_ID = "pageActionPanel";
> +    this.WindowWatcher = new WindowWatcherClass();

s/this.WindowWatcher/this.windowWatcher/

CapitalCase is usually reserved for class definitions, and camelCase is usually reserved for class instances.

::: browser/modules/SavantShieldStudy.jsm:194
(Diff revision 1)
> +    this.handleCommandRef = this.handleCommand.bind(this);
> +    this.handlePopupShownRef = this.handlePopupShown.bind(this);
> +
> +    this.WindowWatcher.init(this.loadIntoWindow.bind(this),
> +                            this.unloadFromWindow.bind(this),
> +                            this.onWindowError.bind(this));

Instead of doing the `this.handleCommandRef = this.handleCommand.bind(this);` dance, you can just pass `this` to `addEventListener` for the handler, then implement a `handleEvent` function that will redirect the event based on the event.type.

::: browser/modules/SavantShieldStudy.jsm:270
(Diff revision 1)
> +
> +/*
> +* The WindowWatcher is used to add/remove listeners from MenuListener
> +* to/from all windows.
> +*/
> +class WindowWatcherClass {

s/WindowWatcherClass/WindowWatcher/

::: browser/modules/SavantShieldStudy.jsm:276
(Diff revision 1)
> +  constructor() {
> +    this._isActive = false;
> +    this._loadCallback = null;
> +    this._unloadCallback = null;
> +    this._errback = null;
> +    this._onWindowLoadedRef = this._onWindowLoaded.bind(this);

Same comment here about using `handleEvent`.

::: browser/modules/SavantShieldStudy.jsm:281
(Diff revision 1)
> +    this._onWindowLoadedRef = this._onWindowLoaded.bind(this);
> +  }
> +
> +  // It is expected that loadCallback, unloadCallback, and errback are bound
> +  // to a `this` value.
> +  init(loadCallback, unloadCallback, errback) {

Can you please rename errback to errorCallback to match the naming convention of the other arguments?

::: browser/modules/SavantShieldStudy.jsm:282
(Diff revision 1)
> +  }
> +
> +  // It is expected that loadCallback, unloadCallback, and errback are bound
> +  // to a `this` value.
> +  init(loadCallback, unloadCallback, errback) {
> +

nit, remove this blank line.

::: browser/modules/SavantShieldStudy.jsm:325
(Diff revision 1)
> +    // This will call the observe method on WindowWatcher
> +    Services.ww.unregisterNotification(this);

The comment here is misleading, calling unregisterNotification will make it so `observe` is no longer called. I assume this is just a copy/paste error :)

::: browser/modules/SavantShieldStudy.jsm:348
(Diff revision 1)
> +        break;
> +    }
> +  }
> +
> +  _onWindowOpened(win) {
> +    win.addEventListener("load", this._onWindowLoadedRef);

Pass {once: true} as the third argument so you don't have to remove the event listener in the onWindowLoaded function.

::: browser/modules/SavantShieldStudy.jsm:363
(Diff revision 1)
> +    // make sure we only remove window listeners from a DOMWindow (browser.xul)
> +    const winType = win.document.documentElement.getAttribute("windowtype");

Why would we want to keep window listeners on a window that is not a navigator:browser?

::: browser/modules/SavantShieldStudy.jsm:370
(Diff revision 1)
> +  _onError(msg) {
> +    this._errback(msg);
> +  }

This function doesn't seem useful. You can replace each `this._onError(msg)` call with `this._errback(msg)`.

::: toolkit/components/telemetry/Events.yaml:135
(Diff revision 1)
> +    notification_emails:
> +      - "[e-mail]"
> +      - "[e-mail]"
> +    expiry_version: "65"
> +    extra_keys:
> +      subcategory: The broad event category for this probe. E.g. navigation

The subcategory description here appears very generic. Can you describe what possible values would appear here? It looks like only "menu" is a possible value. Since that is the case, is the subcategory extra_key necessary?

Comment 4

[Bianca Danforth [:bdanforth]]

I neglected to mention that for the event telemetry to show up in `about:telemetry#events-tab`, the study pref `shield.savant.enabled` needs to be set to `true`. Normandy will take care of turning this pref ON and OFF when the study is officially launched.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8983757 [details]
Bug 1465697 - Add menu open probes for Savant Shield study;

https://reviewboard.mozilla.org/r/249588/#review255962

::: browser/modules/SavantShieldStudy.jsm:216
(Diff revision 1)
> +    log.error(msg);
> +  }
> +
> +  addListeners(win) {
> +    const doc = win.document;
> +    const libraryToolbarButton = doc.getElementById(this.LIBRARY_TOOLBARBUTTON_ID);

Okay, I've tested this out now with the pref enabled, but nothing gets logged if the library button is placed in the Overflow panel, and then the library menu is opened via Overflow.

Comment 6

[Bianca Danforth [:bdanforth]]

Comment on attachment 8983757 [details]
Bug 1465697 - Add menu open probes for Savant Shield study;

https://reviewboard.mozilla.org/r/249588/#review255950

> Why would we want to keep window listeners on a window that is not a navigator:browser?

This is me copy and pasting to undo what was done in the "load" callback (now in `handleEvent`), which only adds listeners on windows of type "navigator:browser". I guess the only risk of trying to remove listeners that are not present on any other window type would be that it does nothing? Are you just saying this is extra lines/extra work for nothing?

> The subcategory description here appears very generic. Can you describe what possible values would appear here? It looks like only "menu" is a possible value. Since that is the case, is the subcategory extra_key necessary?

The subcategory `extra` key was requested by this study's data analyst, :shong. You are right that it is constant (i.e. "menu" in this case)--in fact, it's constant for every probe.

These subcategories were originally intended to be the top level category for each probe. At the recommendation of our Data Steward :chutten (who also happened to write much if not all of the event telemetry API), we opted for a single study category "savant" instead and subordinate the original top level categories, so that the analyst could still query based on them.

Reasons for the single "savant" category approach:
* Ease of development/testing (one category versus five; enabling collection is done by category)
* Probe isolation concerns (we duplicate the "navigation" category "search" event, so that we don't enable/disable that category for others and vice versa)
* Clarity in Events.yaml for which probes are part of the study

Comment 7

[Bianca Danforth [:bdanforth]]

Comment on attachment 8983757 [details]
Bug 1465697 - Add menu open probes for Savant Shield study;

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

Comment 8

[Bianca Danforth [:bdanforth]]

Comment on attachment 8983757 [details]
Bug 1465697 - Add menu open probes for Savant Shield study;

https://reviewboard.mozilla.org/r/249588/#review255962

> Okay, I've tested this out now with the pref enabled, but nothing gets logged if the library button is placed in the Overflow panel, and then the library menu is opened via Overflow.

I changed the listeners here to listen for "ViewShowing" both for the case where the library menu toolbar button is in the default location and moved to the overflow panel.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8983757 [details]
Bug 1465697 - Add menu open probes for Savant Shield study;

https://reviewboard.mozilla.org/r/249588/#review256206

::: browser/modules/SavantShieldStudy.jsm:224
(Diff revisions 1 - 2)
> -    libraryToolbarButton.addEventListener("command", this.handleCommandRef);
> -    hamburgerPanel.addEventListener("popupshown", this.handlePopupShownRef);
> -    dotdotdotPanel.addEventListener("popupshown", this.handlePopupShownRef);
> +    /*
> +    * the library menu "ViewShowing" event bubbles up on the navBar in its
> +    * default location. A separate listener is needed if it is moved to the
> +    * overflow panel via Hamburger > Customize
> +    */
> +    navBar.addEventListener("ViewShowing", this);

The library button can be moved to the TabsToolbar through customization, but oddly enough when I do that and open the menu I do get the library_menu/open event recorded in about:telemetry. I'm not sure how that is working.

Can you add the listener to the navigator-toolbox instead of nav-bar just to be more correct?

::: browser/modules/SavantShieldStudy.jsm:310
(Diff revisions 1 - 2)
>      while (windows.hasMoreElements()) {
>        const win = windows.getNext();
>        try {
>          this._loadCallback(win);
>        } catch (ex) {
> -        this._onError(`WindowWatcher code loading callback failed: ${ ex }`);
> +        this.errorCallback(`WindowWatcher code loading callback failed: ${ ex }`);

this._errorCallback

::: browser/modules/SavantShieldStudy.jsm:321
(Diff revisions 1 - 2)
>      Services.ww.registerNotification(this);
>    }
>  
>    uninit() {
>      if (!this._isActive) {
> -      this._onError("Called uninit, but WindowWatcher was already uninited");
> +      this.errorCallback("Called uninit, but WindowWatcher was already uninited");

this._errorCallback

::: browser/modules/SavantShieldStudy.jsm:331
(Diff revisions 1 - 2)
>      while (windows.hasMoreElements()) {
>        const win = windows.getNext();
>        try {
>          this._unloadCallback(win);
>        } catch (ex) {
> -        this._onError(`WindowWatcher code unloading callback failed: ${ ex }`);
> +        this.errorCallback(`WindowWatcher code unloading callback failed: ${ ex }`);

this._errorCallback

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Bianca Danforth [:bdanforth] from comment #6)
> Comment on attachment 8983757 [details]
> Bug 1465697 - Add menu open probes for Savant Shield study;
> 
> https://reviewboard.mozilla.org/r/249588/#review255950
> 
> > Why would we want to keep window listeners on a window that is not a navigator:browser?
> 
> This is me copy and pasting to undo what was done in the "load" callback
> (now in `handleEvent`), which only adds listeners on windows of type
> "navigator:browser". I guess the only risk of trying to remove listeners
> that are not present on any other window type would be that it does nothing?

Yeah, it is completely safe to remove listeners that weren't added.

> Are you just saying this is extra lines/extra work for nothing?

It's doing work for nothing, but maybe in the future it might end up not doing work when it should do work. In this case, it's always safe to clean up things that might not exist :)

Comment 11

[Bianca Danforth [:bdanforth]]

Comment on attachment 8983757 [details]
Bug 1465697 - Add menu open probes for Savant Shield study;

https://reviewboard.mozilla.org/r/249588/#review256206

> The library button can be moved to the TabsToolbar through customization, but oddly enough when I do that and open the menu I do get the library_menu/open event recorded in about:telemetry. I'm not sure how that is working.
> 
> Can you add the listener to the navigator-toolbox instead of nav-bar just to be more correct?

Sure I can change it.

I noticed when Firefox shuts down, and I try to remove the listeners on these elements, I get a `TypeError: element is null.` I wrapped this block in a try..catch statement to handle the error; not sure if anything else needs to be done.

Comment 12

[Bianca Danforth [:bdanforth]]

Comment on attachment 8983757 [details]
Bug 1465697 - Add menu open probes for Savant Shield study;

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

Comment 13

[Peter deHaan [:pdehaan]]

Comment on attachment 8983757 [details]
Bug 1465697 - Add menu open probes for Savant Shield study;

https://reviewboard.mozilla.org/r/249588/#review256638

::: browser/modules/SavantShieldStudy.jsm:250
(Diff revision 3)
> +          case this.DOTDOTDOT_PANEL_ID:
> +            log.debug("Dotdotdot panel opened.");
> +            this.recordEvent("dotdotdot_menu");
> +            break;
> +          default:
> +            throw new Error(`Popupshown event target not recognized.`);

What happens when this throws, is something upstream catching it and handling it? Or will things get weird?

::: browser/modules/SavantShieldStudy.jsm:357
(Diff revision 3)
> +        break;
> +      case "domwindowclosed":
> +        this._onWindowClosed(win);
> +        break;
> +      default:
> +        break;

Did we want to do something here (like logging)? This seems like a semi-fancy noop.

Comment 14

[Bianca Danforth [:bdanforth]]

Comment on attachment 8983757 [details]
Bug 1465697 - Add menu open probes for Savant Shield study;

https://reviewboard.mozilla.org/r/249588/#review256638

> What happens when this throws, is something upstream catching it and handling it? Or will things get weird?

This will actually never happen, since I only have listeners for the three elements, and I have a special case for all three. Not sure why I have it in the first place, but it's not doing any harm.

> Did we want to do something here (like logging)? This seems like a semi-fancy noop.

Yeah this is a no-op, basically I only want to do something in those two topics. If it's any other topic, I don't care.

Comment 15

[Peter deHaan [:pdehaan]]

R+ from QA.

I managed to get the "hamburger_menu", "dotdotdot_menu", and "library_menu" methods in about:telemetry (all 3 with `{subcategory: "menu"}`).

:+1:

Comment 16

[Bianca Danforth [:bdanforth]]

Comment on attachment 8983757 [details]
Bug 1465697 - Add menu open probes for Savant Shield study;

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

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/58e58e8e3601
Add menu open probes for Savant Shield study; r=jaws

Comment 18

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/58e58e8e3601

Comment 19

[Bianca Danforth [:bdanforth]]

Comment on attachment 8983757 [details]
Bug 1465697 - Add menu open probes for Savant Shield study;

Approval Request Comment
[Feature/Bug causing the regression]: There is no regression. The tracking bug for this study is 1457226.
[User impact if declined]: None. This patch is part of a priority Shield study.
[Is this code covered by automated tests?]: There are no study-specific tests, but all code has been run on the try server for regression testing and has been approved by both a Firefox peer and QA (pdehaan).
[Has the fix been verified in Nightly?]: The study’s functionality has been verified in Nightly.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: This request applies to all patches in this bug. In general, here is a comprehensive list of bugs whose patches need to be uplifted (in this order) to complete the study: 1462725, 1465698, 1465707, 1465703, 1465704, 1465697, 1465685, 1465694
[Is the change risky?]: No.
[Why is the change risky/not risky?]: This is an observational pref-flip study with no branches and no UI. It adds some event telemetry probes in various places and a study module to enable/disable collection. Each patch has been QA tested and R+ by pdehaan.
[String changes made/needed]: None.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8983757 [details]
Bug 1465697 - Add menu open probes for Savant Shield study;

Telemetry code needed to support the Fx61 Savant work. Approved for 61.0b13.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ffc838d82507