Closed Bug 1330315 Opened 7 years ago Closed 7 years ago

Add a telemetry probe to track how the Preferences are opened

Categories

(Firefox :: Settings UI, defect, P1)

Product:
Firefox
Component:
Settings UI
Version:
52 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: jaws, Assigned: Fischer)

References

(Blocks 1 open bug)

Details

(Whiteboard: [photon-preference])

Attachments

(1 file, 1 obsolete file)

Bug 1330315 - Add a telemetry probe to track how the Preferences are opened,
59 bytes, text/x-review-board-request
mconley
: review+
Details 
This telemetry probe should track how the Preferences are opened.

This could be one of the following (comment if I missed one):
- typed about:preferences in to location bar
- Firefox menu > Preferences/Options
- (Title bar) Tools menu > Options
- Preferences/Options button on about:home
Avalon, could you please work on this bug instead of bug 1330122? You can start by looking at the patch for bug 1324167 [1] to see what is needed to add a telemetry probe.

You can also read https://wiki.mozilla.org/Telemetry to learn more about Telemetry.

[1] https://hg.mozilla.org/mozilla-central/rev/551c356821b4
Assignee: nobody → lzylong
Status: NEW → ASSIGNED
Flags: needinfo?(lzylong)
Comment on attachment 8830534 [details]
Bug 1330315 - Add a telemetry probe to track how the Preferences are opened.

https://reviewboard.mozilla.org/r/107290/#review108618

Something got screwed up here with your commits. This latest revision that you pushed removed your previous changes.

::: browser/base/content/browser-data-submission-info-bar.js:70
(Diff revision 2)
>        callback: () => {
>          this._actionTaken = true;
>          window.openAdvancedPreferences("dataChoicesTab");
> +	Services.telemetry
> +                .getHistogramById("FX_PREFERENCES_WHAY_OPENED")
> +                .add("other"); 

This one should be "dataReportingNotification".

::: browser/base/content/browser.js:6162
(Diff revision 2)
>  
>    manage() {
>      openAdvancedPreferences("networkTab");
> +    Services.telemetry
> +            .getHistogramById("FX_PREFERENCES_WHAY_OPENED")
> +            .add("other");  

We shouldn't use "other" for this category. This function is called when someone clicks the button to show the settings for offline apps. So we can call this one "offlineApps".

Also, please remove any trailing whitespace from lines that you have added.

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/Preferences.jsm:77
(Diff revision 2)
> +      Services.telemetry
> +              .getHistogramById("FX_PREFERENCES_WHAY_OPENED")
> +              .add("other");      

These changes can be removed since this code (located under /browser/tools/) only runs in test environments and doesn't get shipped to users.

::: toolkit/components/telemetry/Histograms.json:5205
(Diff revision 2)
>      "kind": "categorical",
>      "labels": ["unknown", "general", "search", "content", "applications", "privacy", "security", "sync", "advancedGeneral", "advancedDataChoices", "advancedNetwork", "advancedUpdates", "advancedCerts"],
>      "releaseChannelCollection": "opt-out",
>      "description": "Count how often each preference category is opened."
>    },
>    "FX_PREFERENCES_WHAY_OPENED": {

Please rename this probe to "FX_PREFERENCES_OPENED_VIA"

::: toolkit/components/telemetry/Histograms.json:5208
(Diff revision 2)
>      "description": "Count how often each preference category is opened."
>    },
>    "FX_PREFERENCES_WHAY_OPENED": {
>      "bug_numbers": [11330315],
>      "alert_emails": ["jaws@mozilla.com"],
>      "expires_in_version": "56",

Since this will be landing in version 54 can you change this expiration to 57?

::: toolkit/components/telemetry/Histograms.json:5210
(Diff revision 2)
>    "FX_PREFERENCES_WHAY_OPENED": {
>      "bug_numbers": [11330315],
>      "alert_emails": ["jaws@mozilla.com"],
>      "expires_in_version": "56",
>      "kind": "categorical",
> -    "labels": ["aboutPreferences", "menu", "titleBar", "aboutHome"],
> +      "labels": ["aboutPreferences", "menu", "titleBar", "aboutHome", "other"],

It looks like this line got indented by a couple spaces, please line up this line with the other lines.

::: toolkit/content/aboutTelemetry.js:247
(Diff revision 2)
>            // Show the data choices preferences on desktop.
>            let mainWindow = getMainWindowWithPreferencesPane();
>            mainWindow.openAdvancedPreferences("dataChoicesTab");
> +	  Services.telemetry
> +                  .getHistogramById("FX_PREFERENCES_WHAY_OPENED")
> +                  .add("other"); 

Please use "aboutTelemetry" here instead of "other".
Attachment #8830534 - Flags: review?(jaws) → review-
Comment on attachment 8830534 [details]
Bug 1330315 - Add a telemetry probe to track how the Preferences are opened.

https://reviewboard.mozilla.org/r/107290/#review109372

You're missing a few other places still. Go to searchfox.org and search for "openPreferences". You should see that there are results in browser-menubar.inc and browser-sets.inc.

::: browser/base/content/browser-fxaccounts.js:286
(Diff revision 3)
>      switch (this.panelUIFooter.getAttribute("fxastatus")) {
>      case "signedin":
>        this.openPreferences();
> +      Services.telemetry
> +              .getHistogramById("FX_PREFERENCES_OPENED_VIA")
> +              .add("titleBar"); 

Can you rename this one to "fxa-signedin"

::: browser/base/content/browser-fxaccounts.js:293
(Diff revision 3)
>      case "error":
>        if (this.panelUIFooter.getAttribute("unverified")) {
>          this.openPreferences();
> +	Services.telemetry
> +                .getHistogramById("FX_PREFERENCES_OPENED_VIA")
> +                .add("titleBar"); 

and this one should be "fxa-error"

::: browser/base/content/browser-fxaccounts.js:303
(Diff revision 3)
>        break;
>      default:
>        this.openPreferences();
> +      Services.telemetry
> +              .getHistogramById("FX_PREFERENCES_OPENED_VIA")
> +              .add("titleBar"); 

and this one should be "fxa"

::: browser/base/content/browser.js:6162
(Diff revision 3)
>  
>    manage() {
>      openAdvancedPreferences("networkTab");
> +    Services.telemetry
> +            .getHistogramById("FX_PREFERENCES_OPENED_VIA")
> +            .add("offlineApps"); 

You need to run our eslint tool on all of the files you changed.

You can do this by running:
./mach eslint path/to/file

You should run that for each of the files that you have changed here. In this particular file it will complain because of the trailing spaces.

If your changes don't pass eslint then they can't get checked in.

::: browser/components/customizableui/CustomizableWidgets.jsm:1194
(Diff revision 3)
>    onCommand(aEvent) {
>      let win = aEvent.target.ownerGlobal;
>      win.openPreferences();
> +    Services.telemetry
> +            .getHistogramById("FX_PREFERENCES_OPENED_VIA")
> +            .add("menu");  

This should be "preferencesButton"

::: browser/components/nsBrowserContentHandler.js:365
(Diff revision 3)
>        if (chromeParam == "chrome://browser/content/pref/pref.xul" ||
>            chromeParam == "chrome://browser/content/preferences/preferences.xul") {
>          openPreferences();
> +	Services.telemetry
> +                .getHistogramById("FX_PREFERENCES_OPENED_VIA")
> +                .add("aboutPreferences");  

This should be "commandLine-legacy"

::: browser/components/nsBrowserContentHandler.js:393
(Diff revision 3)
>      }
>      if (cmdLine.handleFlag("preferences", false)) {
>        openPreferences();
> +      Services.telemetry
> +              .getHistogramById("FX_PREFERENCES_OPENED_VIA")
> +              .add("aboutPreferences");  	

This should be "commandLine"
Attachment #8830534 - Flags: review?(jaws) → review-
Comment on attachment 8830534 [details]
Bug 1330315 - Add a telemetry probe to track how the Preferences are opened.

https://reviewboard.mozilla.org/r/107290/#review109690

Discussed on IRC, Avalon is going to put up a new patch that adds an extra option to openPreferences and then we will add to the telemetry histogram from within openPreferences.
Attachment #8830534 - Flags: review?(jaws) → review-
Comment on attachment 8830534 [details]
Bug 1330315 - Add a telemetry probe to track how the Preferences are opened.

https://reviewboard.mozilla.org/r/107290/#review110602

::: browser/base/content/browser-fxaccounts.js:1
(Diff revision 5)
> -/* This Source Code Form is subject to the terms of the Mozilla Public
> +g/* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this

I don't think you tested this, because the 'g' that is placed before the comment here would be a syntax error.

Please test your patches before submitting them.

::: browser/base/content/browser-fxaccounts.js:295
(Diff revision 5)
> -      this.openPreferences();
> -      Services.telemetry
> +      var extra = {origin: "fxa"};	
> +      this.openPreferences(undefined, extra); 

You updated the openPreferences function in utilityOverlay.js, but `this.openPreferences` refers to a different function, which is defined about 10 lines below this line in the same file.

That function would need to be updated to forward this extra parameter to utilityOverlay.js' openPreferences.

::: browser/base/content/browser-media.js:1
(Diff revision 5)
> -/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*-
> +17/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*-

This is a syntax error and shows that it wasn't tested before submitting.

::: browser/base/content/browser-media.js:170
(Diff revision 5)
>      }
>  
>      let mainAction = {
>        label: gNavigatorBundle.getString(btnLabelId),
>        accessKey: gNavigatorBundle.getString(btnAccessKeyId),
> -      callback() { openPreferences("paneContent"); },
> +      callback() {openPreferences("paneContent", var extra = {origin: "browserMedia"); },

This is also a syntax error.

::: browser/base/content/browser.js:6159
(Diff revision 5)
> -    openAdvancedPreferences("networkTab");
> -    Services.telemetry
> +    var extra = {origin: "offlineApps"};
> +    openAdvancedPreferences("networkTab", extra); 

I don't see anything in the patch that updates openAdvancedPreferences to use this second argument. You've only updated openPreferences.

Also, as a side note, you don't need to decleare the `extra` object outside of the function call. You can just write:
`openAdvancedPreferences("networkTab", {origin: "offlineApps"});`

::: toolkit/mozapps/extensions/content/extensions.js:1
(Diff revision 5)
> -/* This Source Code Form is subject to the terms of the Mozilla Public
> +1;4601;0c/* This Source Code Form is subject to the terms of the Mozilla Public

This is another syntax error and shows that the code wasn't tested before it was submitted.

::: toolkit/mozapps/extensions/content/extensions.js:1461
(Diff revision 5)
>        doCommand() {
>          let mainWindow = getMainWindowWithPreferencesPane();
>          mainWindow.openAdvancedPreferences("dataChoicesTab");
>  	Services.telemetry
>                  .getHistogramById("FX_PREFERENCES_OPENED_VIA")
> -                .add("other"); 
> +                .add("aboutTelemetry"); 

I said this in an earlier review comment...

You need to run our eslint tool on all of the files you changed.

You can do this by running:
./mach eslint path/to/file

You should run that for each of the files that you have changed here. In this particular file it will complain because of the trailing spaces.
Attachment #8830534 - Flags: review?(jaws) → review-
Flags: needinfo?(lzylong)
Comment on attachment 8830534 [details]
Bug 1330315 - Add a telemetry probe to track how the Preferences are opened.

https://reviewboard.mozilla.org/r/107290/#review111586

::: browser/base/content/utilityOverlay.js:676
(Diff revisions 5 - 6)
>  
>    window.openDialog("chrome://browser/content/aboutDialog.xul", "", features);
>  }
>  
>  function openPreferences(paneID, extraArgs) {
> -  if (extraArgs.origin) {
> +  if (extraArgs.origin) { Services.telemetry.getHistogramById("FX_PREFERENCES_OPENED_VIA").add(extraArgs.origin); }

We should always log to the histogram. If extraArgs.origin isn't defined then we should use "other" as the origin. We will then be able to look at the data to make sure that we covered all entry points by making sure there are no "other" entries.

::: browser/base/content/utilityOverlay.js:742
(Diff revisions 5 - 6)
> -function openAdvancedPreferences(tabID) {
> +function openAdvancedPreferences(tabID, extraArgs) {
>    openPreferences("paneAdvanced", { "advancedTab" : tabID });
> +  if (extraArgs.origin) { Services.telemetry.getHistogramById("FX_PREFERENCES_OPENED_VIA").add(extraArgs.origin); }

The extraArgs.origin argument here should just be forwarded to openPreferences.

For example,
> function openAdvancedPreferences(tabID, origin) {
>   openPreferences("paneAdvanced", { advancedTab: tabID, origin });
> }

::: toolkit/components/telemetry/Histograms.json:5210
(Diff revisions 5 - 6)
>    "FX_PREFERENCES_OPENED_VIA": {
>      "bug_numbers": [11330315],
>      "alert_emails": ["jaws@mozilla.com"],
>      "expires_in_version": "57",
>      "kind": "categorical",
> -    "labels": ["preferencesButton", "fxaSignedin", "fxaError", "aboutHome", "dataReporting", "offlineApps", "aboutTelemetry", "commandLine", "commandLineLegacy", "fxa", "browserMedia"],
> +    "labels": ["preferencesButton", "fxaSignedin", "fxaError", "aboutHome", "dataReporting", "offlineApps", "aboutTelemetry", "commandLine", "commandLineLegacy", "fxa", "browserMedia", "paneSearch"],

I don't see where paneSearch is used. Can you go through this list and make sure that all of these labels are used?

::: browser/base/content/browser-menubar.inc:188
(Diff revision 6)
>  #ifndef XP_MACOSX
>                  <menuseparator/>
>                  <menuitem id="menu_preferences"
>                            label="&preferencesCmdUnix.label;"
>                            accesskey="&preferencesCmdUnix.accesskey;"
> -                          oncommand="openPreferences();"/>
> +                          oncommand="openPreferences(undefined, {origin: "preferencesButton"});"/>

This will generate a syntax error because the double-quote that you use inside of the function call will terminate the attribute string. You need to use single-quotes inside of a double-quote, or escape the double-quotes.

Please test your patches before requesting review, or ask for help if you are having trouble understanding why a piece of code isn't working.

::: browser/base/content/browser-menubar.inc:539
(Diff revision 6)
>  #ifndef XP_UNIX
>                <menuseparator id="prefSep"/>
>                <menuitem id="menu_preferences"
>                          label="&preferencesCmd2.label;"
>                          accesskey="&preferencesCmd2.accesskey;"
> -                        oncommand="openPreferences();"/>
> +                        oncommand="openPreferences(undefined, {origin: "preferencesButton"});"/>

Same issue here.
Attachment #8830534 - Flags: review?(jaws) → review-
I found paneSearch here: browser/components/newtab/NewTabSearchProvider.jsm (line75).    browserWin.openPreferences("paneSearch");

Should I keep it?
Comment on attachment 8830534 [details]
Bug 1330315 - Add a telemetry probe to track how the Preferences are opened.

https://reviewboard.mozilla.org/r/107290/#review112604

Your mercurial commits here are quite messed up. I have spent time working with your patches in previous review requests but the added time for me to keep doing that on each review request takes away from time that I could be reviewing other patches. Please meet with your team members or ask for help in #introduction as to how you can roll up all your patches to one patch.

The commit message has regressed as well.

Also, the line that you referenced in comment 16 would still need the second parameter used.
Attachment #8830534 - Flags: review?(jaws) → review-
Comment on attachment 8830534 [details]
Bug 1330315 - Add a telemetry probe to track how the Preferences are opened.

https://reviewboard.mozilla.org/r/107290/#review113092

::: browser/base/content/browser-menubar.inc:188
(Diff revision 10)
>  #ifndef XP_MACOSX
>                  <menuseparator/>
>                  <menuitem id="menu_preferences"
>                            label="&preferencesCmdUnix.label;"
>                            accesskey="&preferencesCmdUnix.accesskey;"
> -                          oncommand="openPreferences();"/>
> +                          oncommand="openPreferences(undefined, {origin: 'preferencesButton'});"/>

This is not the preferences button but in the menubar. Change this to menubar.

::: browser/base/content/browser-menubar.inc:539
(Diff revision 10)
>  #ifndef XP_UNIX
>                <menuseparator id="prefSep"/>
>                <menuitem id="menu_preferences"
>                          label="&preferencesCmd2.label;"
>                          accesskey="&preferencesCmd2.accesskey;"
> -                        oncommand="openPreferences();"/>
> +                        oncommand="openPreferences(undefined, {origin: 'preferencesButton'});"/>

Same here.

::: browser/base/content/browser-sets.inc:96
(Diff revision 10)
>      <command id="cmd_gestureRotateRight" oncommand="gGestureSupport.rotate(event.sourceEvent)"/>
>      <command id="cmd_gestureRotateEnd" oncommand="gGestureSupport.rotateEnd()"/>
>      <command id="Browser:OpenLocation" oncommand="openLocation();"/>
>      <command id="Browser:RestoreLastSession" oncommand="restoreLastSession();" disabled="true"/>
>      <command id="Browser:NewUserContextTab" oncommand="openNewUserContextTab(event.sourceEvent);"/>
> -    <command id="Browser:OpenAboutContainers" oncommand="openPreferences('paneContainers');"/>
> +    <command id="Browser:OpenAboutContainers" oncommand="openPreferences('paneContainers', {origin: 'paneContainers'});"/>

This origin name isn't right. It should be something like openAboutContainersCommand

::: browser/base/content/utilityOverlay.js:679
(Diff revision 10)
>  
>  function openPreferences(paneID, extraArgs) {
> +  if (extraArgs.origin) {
> +    Services.telemetry.getHistogramById("FX_PREFERENCES_OPENED_VIA").add(extraArgs.origin);
> +  } else {
> +    Services.telemetry.getHistogramById("FX_PREFERENCES_OPENED_VIA").add("ohter");

typo

::: browser/modules/ContentSearch.jsm:423
(Diff revision 10)
>      Services.search.currentEngine = Services.search.getEngineByName(data);
>    },
>  
>    _onMessageManageEngines(msg, data) {
>      let browserWin = msg.target.ownerGlobal;
> -    browserWin.openPreferences("paneSearch");
> +    browserWin.openPreferences("paneSearch", {origin: "paneSearch"});

This origin name doesn't seem right. It should be contentSearch

::: toolkit/mozapps/extensions/content/extensions.js:1503
(Diff revision 10)
>        isEnabled() {
>          return !!getMainWindowWithPreferencesPane();
>        },
>        doCommand() {
>          let mainWindow = getMainWindowWithPreferencesPane();
> -        mainWindow.openAdvancedPreferences("dataChoicesTab");
> +        mainWindow.openAdvancedPreferences("dataChoicesTab", {origin: "aboutTelemetry"});

This isn't the right name, since it is using the same origin name as another place. This origin is really about experiments, as the function name above it says. This should be experimentsOpenTelemetryPreferences.
Comment on attachment 8830534 [details]
Bug 1330315 - Add a telemetry probe to track how the Preferences are opened.

https://reviewboard.mozilla.org/r/107290/#review113386

Comment #19 covers what should be changed.
Attachment #8830534 - Flags: review?(jaws) → review-
Blocks: 1330552
 (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #20)
> Comment on attachment 8830534 [details]
> Bug 1330315 - Add a telemetry probe to track how the Preferences are opened.
> 
> https://reviewboard.mozilla.org/r/107290/#review113386
> 
> Comment #19 covers what should be changed.
Hi Ziyan,
Product, Cindy, is asking about the telemetry probe in the bug 1330552.
Are you going to complete the patch, thank you.
Flags: needinfo?(lzylong)
(In reply to Fischer [:Fischer] from comment #21)
>  (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #20)
> > Comment on attachment 8830534 [details]
> > Bug 1330315 - Add a telemetry probe to track how the Preferences are opened.
> > 
> > https://reviewboard.mozilla.org/r/107290/#review113386
> > 
> > Comment #19 covers what should be changed.
> Hi Ziyan,
> Product, Cindy, is asking about the telemetry probe in the bug 1330552.
> Are you going to complete the patch, thank you.

Yea. What can I do for you.
Flags: needinfo?(lzylong)
(In reply to Ziyan Long from comment #22)
> (In reply to Fischer [:Fischer] from comment #21)
> >  (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #20)
> > > Comment on attachment 8830534 [details]
> > > Bug 1330315 - Add a telemetry probe to track how the Preferences are opened.
> > > 
> > > https://reviewboard.mozilla.org/r/107290/#review113386
> > > 
> > > Comment #19 covers what should be changed.
> > Hi Ziyan,
> > Product, Cindy, is asking about the telemetry probe in the bug 1330552.
> > Are you going to complete the patch, thank you.
> 
> Yea. What can I do for you.
Thanks, just check if this bug is still under your radar.
Since Bug 1330552 already blocked Bug 1328561, don't need to block Bug 1328561 anymore.
No longer blocks: 1328561
Sorry, for clear, I think I need to add the Bug 1328561 back.
Blocks: 1328561
Attachment #8830534 - Flags: review?(fliu)
Comment on attachment 8830534 [details]
Bug 1330315 - Add a telemetry probe to track how the Preferences are opened.

https://reviewboard.mozilla.org/r/107290/#review126410

::: toolkit/components/telemetry/Histograms.json:5302
(Diff revision 11)
>      "labels": ["unknown", "general", "search", "content", "applications", "privacy", "security", "sync", "advancedGeneral", "advancedDataChoices", "advancedNetwork", "advancedUpdates", "advancedCerts"],
>      "releaseChannelCollection": "opt-out",
>      "description": "Count how often each preference category is opened."
>    },
> +  "FX_PREFERENCES_OPENED_VIA": {
> +    "bug_numbers": [11330315],

Please fix the bug number. It should be 1330315.

::: toolkit/components/telemetry/Histograms.json:5304
(Diff revision 11)
>      "description": "Count how often each preference category is opened."
>    },
> +  "FX_PREFERENCES_OPENED_VIA": {
> +    "bug_numbers": [11330315],
> +    "alert_emails": ["jaws@mozilla.com"],
> +    "expires_in_version": "57",

Please update this to 59.
Attachment #8830534 - Flags: review?(jaws)
Comment on attachment 8830534 [details]
Bug 1330315 - Add a telemetry probe to track how the Preferences are opened.

Benjamin, can you provide a telemetry review here? Please let me know if I or Avalon can provide more information to you.
Attachment #8830534 - Flags: feedback?(benjamin)
Comment on attachment 8830534 [details]
Bug 1330315 - Add a telemetry probe to track how the Preferences are opened.

Benjamin, can you provide a telemetry review here? Please let me know if I or Avalon can provide more information to you.
Attachment #8830534 - Flags: feedback?(benjamin)
Attachment #8830534 - Flags: review?(fliu)
Comment on attachment 8830534 [details]
Bug 1330315 - Add a telemetry probe to track how the Preferences are opened.

https://reviewboard.mozilla.org/r/107290/#review126488

data-r=me
Attachment #8830534 - Flags: review+
Attachment #8830534 - Flags: feedback?(benjamin)
Comment on attachment 8830534 [details]
Bug 1330315 - Add a telemetry probe to track how the Preferences are opened.

https://reviewboard.mozilla.org/r/107290/#review126668

It looks like there are still some places missing. Please search "openPreferences" and "openAdvancedPreferences" on searchfox or dxr again.
Below are the missings I found currently 
- baseMenuOverlay.xul
- browser-syncui.js
- browser.js
- NewTabSearchProvider.jsm
- nsBrowserGlue.js
- search.xml
- translation-infobar.xml
- UITour.jsm
Thanks

::: browser/base/content/browser-fxaccounts.js:306
(Diff revision 13)
>  
>      PanelUI.hide();
>    },
>  
>    openPreferences() {
>      openPreferences("paneSync", { urlParams: { entrypoint: "menupanel" } });

The calls of this.openPreferences(...) inside onMenuPanelCommand are invoking the openPreferences here. 
So you might need
```
openPreferences(origin) {
  penPreferences("paneSync", { origin, urlParams: { entrypoint: "menupanel" } });
```
And modify the onMenuPanelCommand inside as
```
this.openPreferences("fxa");
```

::: browser/components/nsBrowserContentHandler.js:226
(Diff revision 13)
>    argArray.appendElement(null, /* weak =*/ false); // allowThirdPartyFixup
>  
>    return Services.ww.openWindow(parent, url, target, features, argArray);
>  }
>  
>  function openPreferences() {

Below `openPreferences(undefined, {origin: "commandLineLegacy"});` and `openPreferences(undefined, {origin: "commandLine"});` are actually calling this openPreferences funciton. SO you may want to update this openPreferences here to add telemetry like what you do in the utilityOverlay.js

::: browser/modules/AboutHome.jsm:144
(Diff revision 13)
>        case "AboutHome:Addons":
>          window.BrowserOpenAddonsMgr();
>          break;
>  
>        case "AboutHome:Sync":
>          window.openPreferences("paneSync", { urlParams: { entrypoint: "abouthome" } });

Why this openPreferences has no origin like the case "AboutHome:Settings" one below?
Attachment #8830534 - Flags: review?(fliu) → review-
Comment on attachment 8830534 [details]
Bug 1330315 - Add a telemetry probe to track how the Preferences are opened.

https://reviewboard.mozilla.org/r/107290/#review126874
Attachment #8830534 - Flags: review?(jaws)
Attachment #8830534 - Flags: review?(jaws) → review?(fliu)
(In reply to Fischer [:Fischer] from comment #33)
> Comment on attachment 8830534 [details]
> Bug 1330315 - Add a telemetry probe to track how the Preferences are opened.
> 
> https://reviewboard.mozilla.org/r/107290/#review126668
> 
> It looks like there are still some places missing. Please search
> "openPreferences" and "openAdvancedPreferences" on searchfox or dxr again.
> Below are the missings I found currently 
> - baseMenuOverlay.xul
> - browser-syncui.js
> - browser.js
> - NewTabSearchProvider.jsm
> - nsBrowserGlue.js
> - search.xml
> - translation-infobar.xml
> - UITour.jsm
> Thanks
> 
> ::: browser/base/content/browser-fxaccounts.js:306
> (Diff revision 13)
> >  
> >      PanelUI.hide();
> >    },
> >  
> >    openPreferences() {
> >      openPreferences("paneSync", { urlParams: { entrypoint: "menupanel" } });
> 
> The calls of this.openPreferences(...) inside onMenuPanelCommand are
> invoking the openPreferences here. 
> So you might need
> ```
> openPreferences(origin) {
>   penPreferences("paneSync", { origin, urlParams: { entrypoint: "menupanel"
> } });
> ```
> And modify the onMenuPanelCommand inside as
> ```
> this.openPreferences("fxa");
> ```
> 
> ::: browser/components/nsBrowserContentHandler.js:226
> (Diff revision 13)
> >    argArray.appendElement(null, /* weak =*/ false); // allowThirdPartyFixup
> >  
> >    return Services.ww.openWindow(parent, url, target, features, argArray);
> >  }
> >  
> >  function openPreferences() {
> 
> Below `openPreferences(undefined, {origin: "commandLineLegacy"});` and
> `openPreferences(undefined, {origin: "commandLine"});` are actually calling
> this openPreferences funciton. SO you may want to update this
> openPreferences here to add telemetry like what you do in the
> utilityOverlay.js
> 
> ::: browser/modules/AboutHome.jsm:144
> (Diff revision 13)
> >        case "AboutHome:Addons":
> >          window.BrowserOpenAddonsMgr();
> >          break;
> >  
> >        case "AboutHome:Sync":
> >          window.openPreferences("paneSync", { urlParams: { entrypoint: "abouthome" } });
> 
> Why this openPreferences has no origin like the case "AboutHome:Settings"
> one below?

I upated my bug. Can you review my bug again?
Fischer, review ping?
Flags: needinfo?(fliu)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #37)
> Fischer, review ping?
Yes, looking into the patch.
Flags: needinfo?(fliu)
Comment on attachment 8830534 [details]
Bug 1330315 - Add a telemetry probe to track how the Preferences are opened.

https://reviewboard.mozilla.org/r/107290/#review128804

Thanks for updating. Please some feedbacks below.

::: browser/components/nsBrowserGlue.js:208
(Diff revision 14)
>  
>    // nsIObserver implementation
>    observe: function BG_observe(subject, topic, data) {
>      switch (topic) {
>        case "notifications-open-settings":
>          this._openPreferences("content");

`this._openPreferences("notifications-open-settings", "content");` or `this._openPreferences({ origin: "notifications-open-settings" }, "content");` could clearly tell the origin is from notification asking to open Preferences

::: browser/components/nsBrowserGlue.js:1678
(Diff revision 14)
>                                              [productName], 1);
>  
>      let clickCallback = (subject, topic, data) => {
>        if (topic != "alertclickcallback")
>          return;
>        this._openPreferences("sync");

`this._openPreferences("sync-started-doorhanger", "sync");` could clearly tell that the origin is from user's clicking on the sync-started doorhanger. BTW, `_showSyncStartedDoorhanger` is called by the "fxaccounts:onverified" observer. You could try to run `Services.obs.notifyObservers(null, "fxaccounts:onverified", null)` to see what it is. Same for other observers.

::: browser/components/nsBrowserGlue.js:2206
(Diff revision 14)
>  
>    /**
>     * Open preferences even if there are no open windows.
>     */
>    _openPreferences(...args) {
> +    Services.telemetry.getHistogramById("FX_PREFERENCES_OPENED_VIA").add("nsIObserver");

nsIObserver is an interface so maybe taking it as origin is vague. Tracing the calling origin then add telemetry would be more precise. Please refer to the above 2 exmples and update the rest. And the `_openPreferences` here might need some update on Rest parameter as well. See the example for usage:  https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/rest_parameters#Examples
Attachment #8830534 - Flags: review?(fliu) → review-
Attachment #8830534 - Flags: review?(jaws)
Comment on attachment 8830534 [details]
Bug 1330315 - Add a telemetry probe to track how the Preferences are opened.

https://reviewboard.mozilla.org/r/107290/#review129948

Looks almost ready. Few updates are required, thanks.

::: browser/base/content/browser.js:513
(Diff revision 15)
>            // be removed by bug 1349689.
>            let win = gBrowser.ownerGlobal;
>            if (Preferences.get("browser.preferences.useOldOrganization", false)) {
> -            win.openAdvancedPreferences("networkTab");
> +            win.openAdvancedPreferences("networkTab", {origin: "offlineApps"});
>            } else {
> -            win.openPreferences("panePrivacy");
> +            win.openPreferences("panePrivacy", {origin: "offlineApps"});

This origin should be storage-pressure-notification. Could see an notification for "storage-pressure-notification" is appended from the below codes

::: browser/components/nsBrowserGlue.js:2092
(Diff revision 15)
> +      if (args[i].origin) {
> +        Services.telemetry.getHistogramById("FX_PREFERENCES_OPENED_VIA").add(args[i].origin);
> +      } else {
> +        Services.telemetry.getHistogramById("FX_PREFERENCES_OPENED_VIA").add("other");
> +      }
> +    }

Sorry, didn't notice the last time. Below calls would invoke `openPreferences` in the utilityOverlay.js and the extraArgs would passed down along the way so nothing has to be done here. Please remove the telemetry codes.

::: browser/components/nsBrowserGlue.js:2223
(Diff revision 15)
>      let body = bundle.GetStringFromName("deviceDisconnectedNotification.body");
>  
>      let clickCallback = (subject, topic, data) => {
>        if (topic != "alertclickcallback")
>          return;
> -      this._openPreferences("sync");
> +      this._openPreferences("sync", { origin: "deviceDisconnected"});

"deviceDisconnectedAlert" would be more meaningful, could tell the origin is from alert when device is disconnected. Could see this is triggered by AlertsService below.

::: toolkit/components/telemetry/Histograms.json:5692
(Diff revision 15)
> +    "bug_numbers": [1330315],
> +    "alert_emails": ["jaws@mozilla.com"],
> +    "expires_in_version": "59",
> +    "kind": "categorical",
> +    "labels": ["aboutHome", "aboutTelemetry", "browserMedia", "commandLine", "commandLineLegacy", "ContainersCommand", "contentSearch", "dataReporting", "doorhanger", "deviceDisconnected", "experimentsOpenPre", "fxa", "fxaSignedin", "fxaError", "offlineApps", "prefserviceDefaults", "preferencesButton", "paneSync", "storageManager", "UITour", "menubar", "notifOpenSettings", "other"],
> +    "releaseChannelCollection": "opt-out",

Guess "storageManager" here is for "storage-pressure-notification". I would suggest "storage-pressure-notification" because it is more precise and the UX team for that project really would like to know how many people would enter Preferences to clear some data when seeing notification of not enough storage space.

::: toolkit/mozapps/extensions/content/extensions.js:1548
(Diff revision 15)
>          // The advanced subpanes are only supported in the old organization, which will
>          // be removed by bug 1349689.
>          if (Preferences.get("browser.preferences.useOldOrganization", false)) {
> -          mainWindow.openAdvancedPreferences("dataChoicesTab");
> +          mainWindow.openAdvancedPreferences("dataChoicesTab", {origin: "experimentsOpenPre"});
>          } else {
> -          mainWindow.openPreferences("paneAdvanced");
> +          mainWindow.openPreferences("paneAdvanced", {origin: "experimentsOpenPre"});

nit: "experimentsOpenPref" would be clearly about "Pref".
Attachment #8830534 - Flags: review?(fliu) → review-
Comment on attachment 8830534 [details]
Bug 1330315 - Add a telemetry probe to track how the Preferences are opened.

https://reviewboard.mozilla.org/r/107290/#review130854

Some missed updates there.

::: browser/base/content/browser.js:511
(Diff revision 16)
>          callback(notificationBar, button) {
>            // The advanced subpanes are only supported in the old organization, which will
>            // be removed by bug 1349689.
>            let win = gBrowser.ownerGlobal;
>            if (Preferences.get("browser.preferences.useOldOrganization", false)) {
> -            win.openAdvancedPreferences("networkTab");
> +            win.openAdvancedPreferences("networkTab", {origin: "offlineApps"});

Update missed. Here should be "storagePressure" not "offlineApps".

::: toolkit/mozapps/extensions/content/extensions.js:1546
(Diff revision 16)
>        doCommand() {
>          let mainWindow = getMainWindowWithPreferencesPane();
>          // The advanced subpanes are only supported in the old organization, which will
>          // be removed by bug 1349689.
>          if (Preferences.get("browser.preferences.useOldOrganization", false)) {
> -          mainWindow.openAdvancedPreferences("dataChoicesTab");
> +          mainWindow.openAdvancedPreferences("dataChoicesTab", {origin: "experimentsOpenPre"});

"experimentsOpenPre" is missed here
Attachment #8830534 - Flags: review?(fliu) → review-
Attachment #8830534 - Flags: review?(jaws)
Comment on attachment 8830534 [details]
Bug 1330315 - Add a telemetry probe to track how the Preferences are opened.

https://reviewboard.mozilla.org/r/107290/#review130854

I got it done.
Comment on attachment 8830534 [details]
Bug 1330315 - Add a telemetry probe to track how the Preferences are opened.

https://reviewboard.mozilla.org/r/107290/#review131778

One missed, please fix. Since here is doing lots of changes, please run a try to make sure not breaking something before check-in.

::: browser/base/content/browser-data-submission-info-bar.js:72
(Diff revision 17)
>        callback: () => {
>          this._actionTaken = true;
>          // The advanced subpanes are only supported in the old organization, which will
>          // be removed by bug 1349689.
>          if (Preferences.get("browser.preferences.useOldOrganization", false)) {
>            window.openAdvancedPreferences("dataChoicesTab");

Missed origin here
Attachment #8830534 - Flags: review?(fliu) → review+
Comment on attachment 8830534 [details]
Bug 1330315 - Add a telemetry probe to track how the Preferences are opened.

https://reviewboard.mozilla.org/r/107290/#review131778

Ok. So I need to run test cases?
Comment on attachment 8830534 [details]
Bug 1330315 - Add a telemetry probe to track how the Preferences are opened.

I pushed the patch to tryserver, you can see results at:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=933d7efa091a7212a2769974cd461cd57dc6fb73
Attachment #8830534 - Flags: review?(jaws)
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 5264965cd419 -d f524336df20e: rebasing 389681:5264965cd419 "Bug 1330315 - Add a telemetry probe to track how the Preferences are opened. r=bsmedberg,Fischer" (tip)
merging browser/base/content/browser-data-submission-info-bar.js
merging browser/base/content/browser-fxaccounts.js
merging browser/base/content/browser-media.js
merging browser/base/content/browser.js
merging browser/components/customizableui/CustomizableWidgets.jsm
merging browser/components/nsBrowserContentHandler.js
merging browser/components/nsBrowserGlue.js
merging browser/components/search/content/search.xml
merging toolkit/components/telemetry/Histograms.json
warning: conflicts while merging browser/base/content/browser-data-submission-info-bar.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/components/search/content/search.xml! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Avalon, can you rebase your patch?
Flags: needinfo?(lzylong)
Flags: needinfo?(lzylong)
Attachment #8830534 - Flags: review?(jaws)
Avalon, please see the tryserver results in the link above. This is failing on all test suites.
Flags: needinfo?(lzylong)
Attachment #8830534 - Flags: review?(jaws)
Hey avalon - I guess we didn't land this in time. It's conflicting on rebase. :/ Sorry, that was our fault for not landing it soon enough!

Now that the MSU course is over, I'm going to unassign you from this one. A full-timer will drive this one through to a resolution. Thanks for your work!
Assignee: lzylong → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(lzylong)
I will continue the rebasing work.
Assignee: nobody → fliu
(In reply to Fischer [:Fischer] from comment #60)
> Comment on attachment 8864126 [details]
> Bug 1330315 - Add a telemetry probe to track how the Preferences are opened,
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/135816/diff/1-2/
Hi Mike,
Rebase avalon's patch. However, it seems i cannot set r+ on my own push by myself.
That would be grate that you could help this, thanks
TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=60b7cd095b1183b718bf4a74fc5dd29e17cda7b6
Comment on attachment 8830534 [details]
Bug 1330315 - Add a telemetry probe to track how the Preferences are opened.

Set this patch as obsolete because this patch has been rebased on the latest m-c and repushed in the attachment 8864126 [details] to solve the autoland conflicts (See Comment 51)
Attachment #8830534 - Attachment is obsolete: true
Comment on attachment 8864126 [details]
Bug 1330315 - Add a telemetry probe to track how the Preferences are opened,

https://reviewboard.mozilla.org/r/135816/#review139334

Seems okay - just one concern about the translation-infobar.xml binding, and one suggestion for a simplication. Thanks!

::: browser/base/content/utilityOverlay.js:714
(Diff revision 2)
> +  if (extraArgs && extraArgs.origin) {
> +    Services.telemetry.getHistogramById("FX_PREFERENCES_OPENED_VIA").add(extraArgs.origin);
> +  } else {
> +    Services.telemetry.getHistogramById("FX_PREFERENCES_OPENED_VIA").add("other");
> +  }

Since we're accessing this histogram in either case, maybe pull that out of the conditional, like:

```js
let histogram = Services.telemetry.getHistogramById("FX_PREFERENCES_OPENED_VIA");

if (extraArgs && extraArgs.origin) {
  histogram.add(extraArgs.origin);
} else {
  histogram.add("other");
}

```

::: browser/components/translation/translation-infobar.xml:129
(Diff revision 2)
>                <xul:menuitem anonid="neverForSite"
>                              oncommand="document.getBindingParent(this).neverForSite();"
>                              label="&translation.options.neverForSite.label;"
>                              accesskey="&translation.options.neverForSite.accesskey;"/>
>                <xul:menuseparator/>
> -              <xul:menuitem oncommand="openPreferences('paneGeneral');"
> +              <xul:menuitem oncommand="openPreferences('paneGeneral', {origin:'menubar'});"

I actually don't know if menubar is the right origin for this. This is for the translation feature binding notification bar.
Attachment #8864126 - Flags: review?(mconley) → review+
Comment on attachment 8864126 [details]
Bug 1330315 - Add a telemetry probe to track how the Preferences are opened,

https://reviewboard.mozilla.org/r/135816/#review139334

> Since we're accessing this histogram in either case, maybe pull that out of the conditional, like:
> 
> ```js
> let histogram = Services.telemetry.getHistogramById("FX_PREFERENCES_OPENED_VIA");
> 
> if (extraArgs && extraArgs.origin) {
>   histogram.add(extraArgs.origin);
> } else {
>   histogram.add("other");
> }
> 
> ```

Sure, updated, thanks

> I actually don't know if menubar is the right origin for this. This is for the translation feature binding notification bar.

Thanks for reminding this. Just followed this and found the bug 1153509 to trigger a translation notification bar. Will switch to the translation-infobar origin since this feature is quite unique.
Keywords: checkin-needed
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s c7a8cddaa54b -d b0ff0c5c0a35: rebasing 394537:c7a8cddaa54b "Bug 1330315 - Add a telemetry probe to track how the Preferences are opened. r=bsmedberg,Fischer" (tip)
other [source] changed browser/base/content/browser-fxaccounts.js which local [dest] deleted
use (c)hanged version, leave (d)eleted, or leave (u)nresolved? u
other [source] changed browser/base/content/browser-syncui.js which local [dest] deleted
use (c)hanged version, leave (d)eleted, or leave (u)nresolved? u
merging browser/base/content/browser-data-submission-info-bar.js
merging browser/base/content/browser-media.js
merging browser/base/content/browser-menubar.inc
merging browser/base/content/browser.js
merging browser/base/content/utilityOverlay.js
merging browser/components/customizableui/CustomizableWidgets.jsm
merging browser/components/newtab/NewTabSearchProvider.jsm
merging browser/components/nsBrowserContentHandler.js
merging browser/components/nsBrowserGlue.js
merging browser/components/search/content/search.xml
merging browser/components/uitour/UITour.jsm
merging browser/modules/AboutHome.jsm
merging browser/modules/ContentSearch.jsm
merging toolkit/components/telemetry/Histograms.json
merging toolkit/content/aboutTelemetry.js
merging toolkit/mozapps/extensions/content/extensions.js
warning: conflicts while merging browser/base/content/browser-data-submission-info-bar.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/components/search/content/search.xml! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 197d4a5f6272 -d 23327af4c4c7: rebasing 394544:197d4a5f6272 "Bug 1330315 - Add a telemetry probe to track how the Preferences are opened, r=mconley" (tip)
other [source] changed browser/base/content/browser-fxaccounts.js which local [dest] deleted
use (c)hanged version, leave (d)eleted, or leave (u)nresolved? u
other [source] changed browser/base/content/browser-syncui.js which local [dest] deleted
use (c)hanged version, leave (d)eleted, or leave (u)nresolved? u
merging browser/base/content/browser-menubar.inc
merging browser/base/content/browser.js
merging browser/components/customizableui/CustomizableWidgets.jsm
merging browser/components/nsBrowserGlue.js
merging browser/components/uitour/UITour.jsm
merging browser/modules/AboutHome.jsm
merging browser/modules/ContentSearch.jsm
merging toolkit/components/telemetry/Histograms.json
merging toolkit/mozapps/extensions/content/extensions.js
unresolved conflicts (see hg resolve, then hg rebase --continue)
Re(In reply to Fischer [:Fischer] from comment #68)
> Comment on attachment 8864126 [details]
> Bug 1330315 - Add a telemetry probe to track how the Preferences are opened,
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/135816/diff/3-4/
Rebased and TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=000474561592f1a9ccc2c80b35568e6fb08a8e1e
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/75a14f02f6f7
Add a telemetry probe to track how the Preferences are opened, r=mconley
Keywords: checkin-needed
This also needs a data-review before it can land. See https://wiki.mozilla.org/Firefox/Data_Collection#Requesting_Approval
(In reply to Alessio Placitelli [:Dexter] from comment #72)
> This also needs a data-review before it can land. See
> https://wiki.mozilla.org/Firefox/Data_Collection#Requesting_Approval

The patch got data-review in comment 32.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #73)
> (In reply to Alessio Placitelli [:Dexter] from comment #72)
> > This also needs a data-review before it can land. See
> > https://wiki.mozilla.org/Firefox/Data_Collection#Requesting_Approval
> 
> The patch got data-review in comment 32.

Woha, thanks :jaws, I missed that :-)
Hi Georg,

We are adding a telemetry probe in the about:preferences.
But hitting the error[1] on autoland[2]
```
INFO -  Histogram "FX_PREFERENCES_OPENED_VIA" must have a "record_in_processes" field.
INFO -  backend.mk:23: recipe for target 'TelemetryHistogramData.inc' failed
INFO -  gmake[5]: *** [TelemetryHistogramData.inc] Error 1
INFO -  gmake[5]: *** Deleting file 'TelemetryHistogramData.inc'
```

Saw you did some thing about `record_in_processes` in Bug 1313326.
Do we introduce some new rule for adding telemetry?

p.s A thing strange, we didn't see this type of error on Try[3]

[1] https://treeherder.mozilla.org/logviewer.html#?job_id=98087541&repo=autoland&lineNumber=3129
[2] https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=75a14f02f6f797926cab0d21d4e924d223369dca&selectedJob=98087541
[3] https://treeherder.mozilla.org/#/jobs?repo=try&revision=000474561592f1a9ccc2c80b35568e6fb08a8e1e

Thanks
Flags: needinfo?(fliu) → needinfo?(gfritzsche)
This is a new field:
http://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/collection/histograms.html#record-in-processes

You probably didn't see it on try because it landed so recently.
You probably want to use:
> record_in_processes: ["main", "content"]
Flags: needinfo?(gfritzsche)
(In reply to Georg Fritzsche [:gfritzsche] from comment #76)
> This is a new field:
> http://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/
> collection/histograms.html#record-in-processes
> 
> You probably didn't see it on try because it landed so recently.
> You probably want to use:
> > record_in_processes: ["main", "content"]
Thanks.
Just found Bug 1335343 updating the Histogram.json was got checked-in a day ago.
I will do rebase and update.
(In reply to Fischer [:Fischer] from comment #78)
> Comment on attachment 8864126 [details]
> Bug 1330315 - Add a telemetry probe to track how the Preferences are opened,
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/135816/diff/4-5/
TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ebb985c1d9dce0f41f2e0966e94df7c1a0c70d95
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/471b374af5ab
Add a telemetry probe to track how the Preferences are opened, r=mconley
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/471b374af5ab
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Whiteboard: [photon-preference]
Flags: qe-verify-
Priority: -- → P1
Blocks: 1372459
Blocks: 1420062
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: