1243435 - Remove unused prefs "toolkit.telemetry.prompted" / "toolkit.telemetry.notifiedOptOut" from test suites

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P4)

Description

[Alessio Placitelli [:Dexter]]

I stumbled upon a couple of Telemetry preferences that doesn't seem to be used anymore.

'toolkit.telemetry.prompted' - [0]
'toolkit.telemetry.notifiedOptOut' - [1]

Should we remove them? Is there any reason to keep them around?

[0] - https://dxr.mozilla.org/mozilla-central/search?q=%22toolkit.telemetry.prompted%22&redirect=false&case=false
[1] - https://dxr.mozilla.org/mozilla-central/search?q=%22toolkit.telemetry.notifiedOptOut%22&redirect=false&case=false

Comment 1

[Alessio Placitelli [:Dexter]]

By looking at bug 825464, they seem to relate to MOZ_TELEMETRY_DISPLAY_REV which is going away with bug 1242667.

Comment 2

[Alessio Placitelli [:Dexter]]

Chaitanya, would you be interested in working on this one?

Comment 3

[chaithanya]

(In reply to Alessio Placitelli [:Dexter] from comment #2)
> Chaitanya, would you be interested in working on this one?

Yes. Gladly!

Comment 4

[Alessio Placitelli [:Dexter]]

(In reply to chaithanya from comment #3)
> (In reply to Alessio Placitelli [:Dexter] from comment #2)
> > Chaitanya, would you be interested in working on this one?
> 
> Yes. Gladly!

Cool, I've assigned the bug to you. Please follow the instructions in the bug description and don't hesitate to ask if there's any problem!

Comment 5

[chaithanya]

Attachment: Made changes as per description. Please review.

Comment 6

[Alessio Placitelli [:Dexter]]

Comment on attachment 8714287 [details] [diff] [review]
Made changes as per description. Please review.

Review of attachment 8714287 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for your patch chaithanya!

In addition to the changes below, would you mind setting a different commit message? You can take a look at the commit messages conventions that we use: https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#Commit_Message_Conventions

::: addon-sdk/source/python-lib/cuddlefish/prefs.py
@@ +209,5 @@
>      # Need to client auth test be w/o any dialogs
>      'security.default_personal_cert': 'Select Automatically',
>      'network.http.prompt-temp-redirect': False,
>      'security.warn_viewing_mixed': False,
>      # Set a future policy version to avoid the telemetry prompt.

You should also remove this comment, as it's related to the prefs you removed.

::: layout/tools/reftest/remotereftest.py
@@ +222,5 @@
>          prefs["browser.firstrun.show.localepicker"] = False
>          prefs["font.size.inflation.emPerLine"] = 0
>          prefs["font.size.inflation.minTwips"] = 0
>          prefs["reftest.remote"] = True
>          # Set a future policy version to avoid the telemetry prompt.

Please remove this comment.

@@ -444,5 @@
>      return retVal
>  
>  if __name__ == "__main__":
>      sys.exit(main())
> -

Please do not remove this empty line.

::: layout/tools/reftest/runreftestb2g.py
@@ +248,5 @@
>          prefs["network.dns.localDomains"] = "app://test-container.gaiamobile.org"
>          prefs["reftest.browser.iframe.enabled"] = False
>          prefs["reftest.remote"] = True
>  
>          # Set a future policy version to avoid the telemetry prompt.

Please remove this comment.

::: layout/tools/reftest/runreftestmulet.py
@@ +135,5 @@
>          prefs["font.size.inflation.minTwips"] = 0
>          prefs["network.dns.localDomains"] = "app://test-container.gaiamobile.org"
>          prefs["reftest.browser.iframe.enabled"] = False
>          prefs["reftest.remote"] = False
>          # Set a future policy version to avoid the telemetry prompt.

Ditto.

::: mobile/android/chrome/content/WebappRT.js
@@ +31,5 @@
>      // Auto-disable any add-ons that are "dropped in" to the profile
>      pref("extensions.autoDisableScopes", 1),
>      // Disable add-on installation via the web-exposed APIs
>      pref("xpinstall.enabled", false),
>      // Set a future policy version to avoid the telemetry prompt.

Please remove this comment.

::: testing/profiles/prefs_general.js
@@ +49,5 @@
>  user_pref("dom.w3c_touch_events.enabled", 1);
>  user_pref("dom.undo_manager.enabled", true);
>  user_pref("dom.webcomponents.enabled", true);
>  user_pref("dom.htmlimports.enabled", true);
>  // Set a future policy version to avoid the telemetry prompt.

Same here.

::: testing/talos/tests/test_talosconfig.py
@@ -6,5 @@
>  
>  #globals
>  ffox_path = 'test/path/to/firefox'
>  command_args = [ffox_path, '-profile', 'pathtoprofile', '-tp', 'pathtotpmanifest', '-tpchrome', '-tpmozafterpaint', '-tpnoisy', '-rss', '-tpcycles', '1', '-tppagecycles', '1']
> -browser_config = {'deviceroot': '', 'dirs': {}, 'test_name_extension': '_paint', 'repository': 'http://hg.mozilla.org/releases/mozilla-release', 'buildid': '20131205075310', 'results_log': 'pathtoresults_log', 'symbols_path': None, 'bcontroller_config': 'pathtobcontroller', 'host': '', 'browser_name': 'Firefox', 'sourcestamp': '39faf812aaec', 'remote': False, 'child_process': 'plugin-container', 'branch_name': '', 'browser_version': '26.0', 'extra_args': '', 'develop': True, 'preferences': {'browser.display.overlaynavbuttons': False, 'extensions.getAddons.get.url': 'http://127.0.0.1/extensions-dummy/repositoryGetURL', 'dom.max_chrome_script_run_time': 0, 'network.proxy.type': 1, 'extensions.update.background.url': 'http://127.0.0.1/extensions-dummy/updateBackgroundURL', 'network.proxy.http': 'localhost', 'plugins.update.url': 'http://127.0.0.1/plugins-dummy/updateCheckURL', 'dom.max_script_run_time': 0, 'extensions.update.enabled': False, 'browser.safebrowsing.keyURL': 'http://127.0.0.1/safebrowsing-dummy/newkey', 'media.navigator.permission.disabled': True, 'app.update.enabled': False, 'extensions.blocklist.url': 'http://127.0.0.1/extensions-dummy/blocklistURL', 'browser.EULA.override': True, 'extensions.checkCompatibility': False, 'talos.logfile': 'pathtofile', 'browser.safebrowsing.gethashURL': 'http://127.0.0.1/safebrowsing-dummy/gethash', 'extensions.hotfix.url': 'http://127.0.0.1/extensions-dummy/hotfixURL', 'dom.disable_window_move_resize': True, 'network.proxy.http_port': 80, 'browser.dom.window.dump.enabled': True, 'extensions.update.url': 'http://127.0.0.1/extensions-dummy/updateURL', 'browser.chrome.dynamictoolbar': False, 'toolkit.telemetry.notifiedOptOut': 999, 'browser.link.open_newwindow': 2, 'extensions.getAddons.search.url': 'http://127.0.0.1/extensions-dummy/repositorySearchURL', 'browser.cache.disk.smart_size.first_run': False, 'security.turn_off_all_security_so_that_viruses_can_take_over_this_computer': True, 'dom.disable_open_during_load': False, 'extensions.getAddons.search.browseURL': 'http://127.0.0.1/extensions-dummy/repositoryBrowseURL', 'browser.cache.disk.smart_size.enabled': False, 'extensions.getAddons.getWithPerformance.url': 'http://127.0.0.1/extensions-dummy/repositoryGetWithPerformanceURL', 'hangmonitor.timeout': 0, 'extensions.getAddons.maxResults': 0, 'dom.send_after_paint_to_content': True, 'security.fileuri.strict_origin_policy': False, 'media.capturestream_hints.enabled': True, 'extensions.update.notifyUser': False, 'extensions.blocklist.enabled': False, 'browser.bookmarks.max_backups': 0, 'browser.shell.checkDefaultBrowser': False, 'media.peerconnection.enabled': True, 'dom.disable_window_flip': True, 'security.enable_java': False, 'toolkit.telemetry.prompted': 999, 'browser.warnOnQuit': False, 'media.navigator.enabled': True, 'browser.safebrowsing.updateURL': 'http://127.0.0.1/safebrowsing-dummy/update', 'dom.allow_scripts_to_close_windows': True, 'extensions.webservice.discoverURL': 'http://127.0.0.1/extensions-dummy/discoveryURL'}, 'test_timeout': 1200, 'title': 'qm-pxp01', 'error_filename': 'pathtoerrorfile', 'webserver': 'localhost:15707', 'browser_path':ffox_path, 'port': 20701, 'browser_log': 'browser_output.txt', 'process': 'firefox.exe', 'xperf_path': 'C:/Program Files/Microsoft Windows Performance Toolkit/xperf.exe', 'extensions': ['pathtopageloader'], 'fennecIDs': '', 'env': {'NO_EM_RESTART': '1'}, 'init_url': 'http://localhost:15707/getInfo.html', 'browser_wait': 5}

You should not remove the whole browser_config. Just drop the two configuration details you need to remove :)

Comment 7

[chaithanya]

Attachment: Made changes as per comment 6. Kindly review.

Comment 8

[Alessio Placitelli [:Dexter]]

Comment on attachment 8714814 [details] [diff] [review]
Made changes as per comment 6. Kindly review.

Review of attachment 8714814 [details] [diff] [review]:
-----------------------------------------------------------------

This looks better, thanks. We're almost there!

Would you change the commit message to "Bug 1243435: Remove unused prefs "toolkit.telemetry.prompted" / "toolkit.telemetry.notifiedOptOut" from test suites" (I've added the bug number to the beginning of the commit message).

::: layout/tools/reftest/runreftestmulet.py
@@ +135,5 @@
>          prefs["font.size.inflation.minTwips"] = 0
>          prefs["network.dns.localDomains"] = "app://test-container.gaiamobile.org"
>          prefs["reftest.browser.iframe.enabled"] = False
>          prefs["reftest.remote"] = False
>          # Set a future policy version to avoid the telemetry prompt.

This comment needs to be removed.

::: testing/profiles/prefs_general.js
@@ +49,5 @@
>  user_pref("dom.w3c_touch_events.enabled", 1);
>  user_pref("dom.undo_manager.enabled", true);
>  user_pref("dom.webcomponents.enabled", true);
>  user_pref("dom.htmlimports.enabled", true);
>  // Set a future policy version to avoid the telemetry prompt.

This comment needs to be removed.

Comment 9

[chaithanya]

(In reply to Alessio Placitelli [:Dexter] from comment #8)
> Comment on attachment 8714814 [details] [diff] [review]
> Made changes as per comment 6. Kindly review.
> 
> Review of attachment 8714814 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks better, thanks. We're almost there!
> 
> Would you change the commit message to "Bug 1243435: Remove unused prefs
> "toolkit.telemetry.prompted" / "toolkit.telemetry.notifiedOptOut" from test
> suites" (I've added the bug number to the beginning of the commit message).
> 
> ::: layout/tools/reftest/runreftestmulet.py
> @@ +135,5 @@
> >          prefs["font.size.inflation.minTwips"] = 0
> >          prefs["network.dns.localDomains"] = "app://test-container.gaiamobile.org"
> >          prefs["reftest.browser.iframe.enabled"] = False
> >          prefs["reftest.remote"] = False
> >          # Set a future policy version to avoid the telemetry prompt.
> 
> This comment needs to be removed.
> 
> ::: testing/profiles/prefs_general.js
> @@ +49,5 @@
> >  user_pref("dom.w3c_touch_events.enabled", 1);
> >  user_pref("dom.undo_manager.enabled", true);
> >  user_pref("dom.webcomponents.enabled", true);
> >  user_pref("dom.htmlimports.enabled", true);
> >  // Set a future policy version to avoid the telemetry prompt.
> 
> This comment needs to be removed.

Thanks for that review. I'm working on it. My exams have just begun, so I might take sometime to submit the patch.

Comment 10

[chaithanya]

Attachment: Thanks for the review.Made changes as by comment 8, please review.

Comment 11

[Alessio Placitelli [:Dexter]]

Comment on attachment 8719154 [details] [diff] [review]
Thanks for the review.Made changes as by comment 8, please review.

This new patch discards a big chunk of the changes you correctly made in the previous version.

Please make sure your patch contains all your changes before requesting a new review.

Comment 12

[chaithanya]

Attachment: Made changes as by comment 11, please review.

Comment 13

[Alessio Placitelli [:Dexter]]

Comment on attachment 8719749 [details] [diff] [review]
Made changes as by comment 11, please review.

Review of attachment 8719749 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good now, thanks.

Comment 14

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=567ad5530e5d

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/53b0b5b29776

Comment 16

[Carsten Book [:Tomcat]]

backed this out in https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=3b8c69102f12 since one of this 2 pushes caused https://treeherder.mozilla.org/logviewer.html#?job_id=7344923&repo=fx-team

Comment 17

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/4cd21a52ff41

Comment 18

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=38833c34f240

Comment 19

[Alessio Placitelli [:Dexter]]

This new try push doesn't seem to show any failure for m5.

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/86fbe58122a2

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/86fbe58122a2