Closed Bug 741047 Opened 12 years ago Closed 10 years ago

Implement opening in-content preferences to a given view

Categories

(Firefox :: Settings UI, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 32

People

(Reporter: Unfocused, Assigned: jaws)

References

Details

(Whiteboard: p=5 s=it-32c-31a-30b.2 [qa!] )

Attachments

(1 file, 6 obsolete files)

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.
OS: Windows 7 → All
Hardware: x86_64 → All
Assignee: nobody → owenccarpenter
Status: NEW → ASSIGNED
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.
Attachment #615333 - Flags: feedback?(jwein)
Attachment #615333 - Flags: feedback?(bmcbride)
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.
Attachment #615333 - Flags: feedback?(jwein) → feedback+
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 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.
Attachment #615333 - Flags: feedback?(bmcbride)
Blocks: 718011
Attached patch specific pane patch with test (obsolete) — Splinter Review
Includes browser_bug741047.js to test the new functionality
Attachment #619419 - Flags: feedback?(jwein)
Attachment #619419 - Flags: feedback?(bmcbride)
Comment on attachment 619419 [details] [diff] [review]
specific pane patch with test

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

Looks good to me :)
Attachment #619419 - Flags: feedback?(jwein) → review+
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.
Attachment #619419 - Flags: review-
Comment on attachment 619419 [details] [diff] [review]
specific pane patch with test

Thanks for catching that Dao.
Attachment #619419 - Flags: review+
Attachment #619419 - Flags: feedback?(bmcbride)
Attachment #615333 - Attachment is obsolete: true
Also, utilityOverlay.js is used in more windows than browser.xul, but gBrowser only exists in browser.xul.
Attached patch pane view patch with update test (obsolete) — Splinter Review
modified browser.js to have BrowerOpenAddonsMgr use BrowserOpenView and openPreferences calls BrowserOpenView
Attachment #619419 - Attachment is obsolete: true
Attachment #619708 - Flags: feedback?(dao)
Blocks: 750519
Blocks: 750524
Attachment #619708 - Flags: feedback?(dao) → review?(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.
Attachment #619708 - Flags: review?(dao) → review-
Assignee: owenccarpenter → jon.rietveld
Attached patch pane view patch (obsolete) — Splinter Review
moved openPreferences to brower.js and fixed feedback in comment 11
Attachment #619708 - Attachment is obsolete: true
Attachment #621780 - Flags: feedback?(dao)
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.
Attachment #621780 - Flags: feedback?(dao)
Attached patch pane view patch (obsolete) — Splinter Review
added openChromeTab in utilityOverlay.js and have openPreferences calling it. Have BrowerOpenAddonsMrg in browser.js calling openChromeTab as well
Attachment #621780 - Attachment is obsolete: true
Attachment #622164 - Flags: feedback?(dao)
Attached patch pane view patch (obsolete) — Splinter Review
added openChromeTab in utilityOverlay.js and have openPreferences calling it. Have BrowerOpenAddonsMrg in browser.js calling openChromeTab as well. Removed random ++ / -- lines from patch
Attachment #622164 - Attachment is obsolete: true
Attachment #622164 - Flags: feedback?(dao)
Attachment #622185 - Flags: feedback?(dao)
Attachment #622185 - Attachment is patch: true
Attachment #622185 - Flags: feedback?(dao) → review?(dao)
Attachment #622185 - Flags: review?(dao) → review?(bmcbride)
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?
Attachment #622185 - Flags: review?(bmcbride) → review-
(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.
Whiteboard: p=0
Bug 754304 could be one way to do this.
(or maybe that bug depends on this one)
Whiteboard: p=0 → p=5
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+
Blocks: 738797
Attached patch PatchSplinter Review
Most of this was covered in the refactoring for the new in-content design, but there were a couple bits missing.
Assignee: jon.rietveld → jaws
Attachment #622185 - Attachment is obsolete: true
Attachment #8418355 - Flags: review?(ttaubert)
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)
Attachment #8418355 - Flags: review?(gijskruitbosch+bugs)
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
Attachment #8418355 - Flags: review?(ttaubert)
Attachment #8418355 - Flags: review?(gijskruitbosch+bugs)
Attachment #8418355 - Flags: review+
(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
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
Whiteboard: p=5 [fixed-in-fx-team] → p=5
Whiteboard: p=5 → p=5 s=it-32c-31a-30b.2
Whiteboard: p=5 s=it-32c-31a-30b.2 → p=5 s=it-32c-31a-30b.2 [qa?]
Whiteboard: p=5 s=it-32c-31a-30b.2 [qa?] → p=5 s=it-32c-31a-30b.2 [qa-]
This will be testable on a new profile by clicking on the "Choose What I Share" button.
Whiteboard: p=5 s=it-32c-31a-30b.2 [qa-] → p=5 s=it-32c-31a-30b.2 [qa+]
https://hg.mozilla.org/integration/fx-team/rev/49859608dbb8
Whiteboard: p=5 s=it-32c-31a-30b.2 [qa+] → p=5 s=it-32c-31a-30b.2 [qa+] [fixed-in-fx-team]
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
Whiteboard: p=5 s=it-32c-31a-30b.2 [qa+] [fixed-in-fx-team] → p=5 s=it-32c-31a-30b.2 [qa+]
Try push: https://tbpl.mozilla.org/?tree=Try&rev=466ff66e63c2

Relanded with the leak fixed: https://hg.mozilla.org/integration/fx-team/rev/8480626f3af3
Whiteboard: p=5 s=it-32c-31a-30b.2 [qa+] → p=5 s=it-32c-31a-30b.2 [qa+] [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/8480626f3af3
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: p=5 s=it-32c-31a-30b.2 [qa+] [fixed-in-fx-team] → p=5 s=it-32c-31a-30b.2 [qa+]
Target Milestone: --- → Firefox 32
QA Contact: camelia.badau
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?
Flags: needinfo?(jaws)
(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.
Flags: needinfo?(jaws)
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).
Status: RESOLVED → VERIFIED
Whiteboard: p=5 s=it-32c-31a-30b.2 [qa+] → p=5 s=it-32c-31a-30b.2 [qa!]
You need to log in before you can comment on or make changes to this bug.