741047 - Implement opening in-content preferences to a given view

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Bug 735471 adds the pref to switch between dialog and in-content prefs, but doesn't handle the case where it wants to show a specific view.

The Add-ons Manager does this in an awkward way, that was meant to be only temporary (see bug 593687 for history). It'd be good to have a standard mechanism for this.

Comment 1

[MSU Capstone Team]

Attachment: In-Content Prefs opening in a given view

Hey, sorry I had this patch ready on Friday and I apparently forgot to upload it.  Once Jon made those changes to the init_all function in preferences.js it was a simple fix.

Comment 2

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

Comment on attachment 615333 [details] [diff] [review]
In-Content Prefs opening in a given view

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

It would be good if you could add a test for this.

::: browser/base/content/utilityOverlay.js
@@ +496,4 @@
>  function openPreferences(paneID, extraArgs)
>  {
>    if (Services.prefs.getBoolPref("browser.preferences.inContent")) {  
> +    var newTabBrowser = gBrowser.getBrowserForTab(gBrowser.selectedTab = gBrowser.addTab("about:preferences"));  

Please move the expression outside of the function arguments. If you do gBrowser.selectedTab = gBrowser.addTab, then can you use gBrowser.selectedBrowser instead of running getBrowserForTab?

@@ +496,5 @@
>  function openPreferences(paneID, extraArgs)
>  {
>    if (Services.prefs.getBoolPref("browser.preferences.inContent")) {  
> +    var newTabBrowser = gBrowser.getBrowserForTab(gBrowser.selectedTab = gBrowser.addTab("about:preferences"));  
> +    newTabBrowser.addEventListener("Initialized", function () {  

This event listener is never removed.

Comment 3

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

Comment on attachment 615333 [details] [diff] [review]
In-Content Prefs opening in a given view

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

::: browser/base/content/utilityOverlay.js
@@ +498,5 @@
>    if (Services.prefs.getBoolPref("browser.preferences.inContent")) {  
> +    var newTabBrowser = gBrowser.getBrowserForTab(gBrowser.selectedTab = gBrowser.addTab("about:preferences"));  
> +    newTabBrowser.addEventListener("Initialized", function () {  
> +	  if(paneID) {
> +	    newTabBrowser.contentWindow.gotoPref(paneID);

Can you please make sure to remove any tab characters and replace them with spaces? The tab characters make indentation look awkward when doing a code review.

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 615333 [details] [diff] [review]
In-Content Prefs opening in a given view

Waiting for new patch that addresses comment 2 and comment 3.

Comment 5

[Jon Rietveld]

Attachment: specific pane patch with test

Includes browser_bug741047.js to test the new functionality

Comment 6

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

Comment on attachment 619419 [details] [diff] [review]
specific pane patch with test

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

Looks good to me :)

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 619419 [details] [diff] [review]
specific pane patch with test

This doesn't look good to me. openUILinkIn is smarter than addTab: it avoids adding tabs to popups and closing the added tab will select the previously selected tab rather than the last tab in the tab strip.

Comment 8

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

Comment on attachment 619419 [details] [diff] [review]
specific pane patch with test

Thanks for catching that Dao.

Comment 9

[Dão Gottwald [:dao]]

Also, utilityOverlay.js is used in more windows than browser.xul, but gBrowser only exists in browser.xul.

Comment 10

[Jon Rietveld]

Attachment: pane view patch with update test

modified browser.js to have BrowerOpenAddonsMgr use BrowserOpenView and openPreferences calls BrowserOpenView

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 619708 [details] [diff] [review]
pane view patch with update test

>+++ b/browser/base/content/browser.js
>@@ -6898,6 +6898,10 @@ var MailIntegration = {
> };
> 
> function BrowserOpenAddonsMgr(aView) {
>+  BrowserOpenView(aView, "about:addons", "EM");
>+}
>+
>+function BrowserOpenView(aView, aPage, aPingPrefix) {

Please find a better name than "BrowserOpenView". "openChromeTab" or something like that.

>   if (aView) {
>     let emWindow;

The "emWindow" variable name doesn't make sense anymore for this function.

>     let browserWindow;
>@@ -6914,10 +6918,9 @@ function BrowserOpenAddonsMgr(aView) {
>         browserWindow = browserWin;
>       }
>     }
>-    Services.obs.addObserver(receivePong, "EM-pong", false);
>-    Services.obs.notifyObservers(null, "EM-ping", "");
>-    Services.obs.removeObserver(receivePong, "EM-pong");
>-
>+    Services.obs.addObserver(receivePong, (aPingPrefix+"-pong"), false);
>+    Services.obs.notifyObservers(null, (aPingPrefix+"-ping"), "");
>+    Services.obs.removeObserver(receivePong, (aPingPrefix+"-pong"));
>     if (emWindow) {

Don't remove the blank line.

>+++ b/browser/base/content/utilityOverlay.js
>@@ -495,8 +495,9 @@ function openAboutDialog() {
> 
> function openPreferences(paneID, extraArgs)
> {
>-  if (Services.prefs.getBoolPref("browser.preferences.inContent")) {  
>-    openUILinkIn("about:preferences", "tab");
>+  if (Services.prefs.getBoolPref("browser.preferences.inContent")) {
>+    BrowserOpenView(paneID, "about:preferences", "PM");

utilityOverlay.js can't depend on browser.js.

I don't understand what "PM" means. Use something more descriptive, e.g. "about:preferences".

>+
>   } else {

Remove spurious blank line.

Comment 12

[Jon Rietveld]

Attachment: pane view patch

moved openPreferences to brower.js and fixed feedback in comment 11

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 621780 [details] [diff] [review]
pane view patch

I don't think you want to move openPreferences() to browser.js (it's been in utilityOverlay.js for a while, AIUI). But you should be able to move openChromTab to utilityOverlay, and then have BrowserOpenAddonsMgr() and openPreferences() call it.

Comment 14

[Jon Rietveld]

Attachment: pane view patch

added openChromeTab in utilityOverlay.js and have openPreferences calling it. Have BrowerOpenAddonsMrg in browser.js calling openChromeTab as well

Comment 15

[Jon Rietveld]

Attachment: pane view patch

added openChromeTab in utilityOverlay.js and have openPreferences calling it. Have BrowerOpenAddonsMrg in browser.js calling openChromeTab as well. Removed random ++ / -- lines from patch

Comment 16

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 622185 [details] [diff] [review]
pane view patch

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

::: browser/base/content/test/browser_bug741047.js
@@ +20,5 @@
> +    gBrowser.tabContainer.removeEventListener("TabOpen", tabOpened, true);
> +    let browser = aEvent.originalTarget.linkedBrowser;
> +    browser.addEventListener("Initialized", function checkSyncPane(aEvent) {
> +      browser.removeEventListener("Initialized", checkSyncPane, true);
> +      setTimeout(function(){

Use executeSoon() instead of setTimeout() if you just want to wait for some JS to run. It takes a function as it's only argument.

::: browser/base/content/utilityOverlay.js
@@ +522,4 @@
>    }
>  }
>  
> +function openChromeTab(aView, aPage, aPingPrefix)

Re-order these arguments - it makes more sense to have aPage be the first argument, since aView both depends on aPage and is optional.

And I think aPingPrefix should be removed - we don't need two different strings to identify a page, it should be enough to only know the URL (ie, aPage). Then the observer topics can be |aPage + "-ping"| etc.

@@ +541,5 @@
> +      }
> +    }
> +    Services.obs.addObserver(receivePong, (aPingPrefix+"-pong"), false);
> +    Services.obs.notifyObservers(null, (aPingPrefix+"-ping"), "");
> +    Services.obs.removeObserver(receivePong, (aPingPrefix+"-pong"));

Nit: Space on either side of operators. ie, aPage + "-pong". Don't need the brackets here either. Same for the "-loaded" notification below.

::: browser/components/preferences/in-content/preferences.js
@@ +30,4 @@
>    var initFinished = document.createEvent("Event");
>    initFinished.initEvent("Initialized", true, true);
>    document.dispatchEvent(initFinished);
> +  Services.obs.addObserver(sendPrefsPong, "prefs-ping", false);

Need to remove this observer when the page is unloaded. Would be generally handy to have a shutdown() function like what the Add-ons Manager has, and do it in there.

@@ +78,5 @@
>    }
>  }
> +
> +function loadView(page) {
> +  gotoPref(page);

Can we just rename gotoPage to loadView?

Comment 17

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Blair McBride (:Unfocused) from comment #16)
> And I think aPingPrefix should be removed - we don't need two different
> strings to identify a page, it should be enough to only know the URL (ie,
> aPage). Then the observer topics can be |aPage + "-ping"| etc.

Which of course means updating the observer topics for the Add-ons Manager too.

Comment 18

[:Gavin Sharp [email: [e-mail]]]

Bug 754304 could be one way to do this.

Comment 19

[:Gavin Sharp [email: [e-mail]]]

(or maybe that bug depends on this one)

Comment 20

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

Attachment: Patch

Most of this was covered in the refactoring for the new in-content design, but there were a couple bits missing.

Comment 21

[:Gijs (out for now; he/him)]

Comment on attachment 8418355 [details] [diff] [review]
Patch

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

I have nits, but I need more time to look at the details here. :-(

::: browser/base/content/utilityOverlay.js
@@ +504,5 @@
>        switchToAdvancedSubPane(browser.contentDocument);
>      }
>  
>      if (newLoad) {
> +      Services.obs.addObserver(function observer(prefWin, topic, data) {

Nit: rename to something more specific, please (but take care to rename both this and the removeObserver call below)

::: toolkit/mozapps/extensions/test/browser/browser_experiments.js
@@ +145,3 @@
>  
> +      deferred.resolve();
> +    }, 0);

As this is in a test, use executeSoon() instead of the manual setTimeout(, 0)

Comment 22

[:Gijs (out for now; he/him)]

Comment on attachment 8418355 [details] [diff] [review]
Patch

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

Blargh. But yeah, I can see why it'd work. Also, I can haz try run before relanding? :-)

::: browser/base/content/utilityOverlay.js
@@ +504,5 @@
>        switchToAdvancedSubPane(browser.contentDocument);
>      }
>  
>      if (newLoad) {
> +      Services.obs.addObserver(function observer(prefWin, topic, data) {

Nit: for my sanity, verify that prefWin == browser.contentWindow

Comment 23

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

(In reply to :Gijs Kruitbosch from comment #22)
> Blargh. But yeah, I can see why it'd work. Also, I can haz try run before
> relanding? :-)

https://tbpl.mozilla.org/?tree=Try&rev=44c498849bba

Comment 24

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

https://hg.mozilla.org/integration/fx-team/rev/0e11a16741bf

Comment 25

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

Backed out because my try push above didn't catch a timeout in browser_datareporting_notification.js on debug builds.

https://hg.mozilla.org/integration/fx-team/rev/7242b733b9e9

Comment 26

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

This will be testable on a new profile by clicking on the "Choose What I Share" button.

Comment 27

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

https://hg.mozilla.org/integration/fx-team/rev/49859608dbb8

Comment 28

[Carsten Book [:Tomcat]]

backed this out in https://tbpl.mozilla.org/?tree=Fx-Team&rev=9ec1824900eb since one of this 2 patches seems to caused frequent memory leaks like:

https://tbpl.mozilla.org/php/getParsedLog.php?id=39711492&tree=Fx-Team

which is in retrigger frequent to permanent

Comment 29

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

Try push: https://tbpl.mozilla.org/?tree=Try&rev=466ff66e63c2

Relanded with the leak fixed: https://hg.mozilla.org/integration/fx-team/rev/8480626f3af3

Comment 30

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8480626f3af3

Comment 31

[Camelia Badau [:cbadau], Desktop Test Engineering]

I've tried to verify this bug using a new profile. Is there any specific method to bring up the "Choose what I share" button? It was not displayed after starting Firefox. 
Are there any other scenarios I could use in testing this?

Comment 32

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

(In reply to Camelia Badau, QA [:cbadau] from comment #31)
> I've tried to verify this bug using a new profile. Is there any specific
> method to bring up the "Choose what I share" button? It was not displayed
> after starting Firefox. 
> Are there any other scenarios I could use in testing this?

Open about:config and change the datareporting.policy.firstRunTime by decreasing the third from the left by 2 and then either wait two minutes or restart Firefox.

Comment 33

[Camelia Badau [:cbadau], Desktop Test Engineering]

Thank you for your answer, Jared!

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0
Mozilla/5.0 (X11; Linux i686; rv:32.0) Gecko/20100101 Firefox/32.0 
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:32.0) Gecko/20100101 Firefox/32.0

Verified fixed on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.7.5 using latest Nightly 32.0a1 (buildID: 20140519030202).