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.

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 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 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.

Includes browser_bug741047.js to test the new functionality

Comment on attachment 619419 [details] [diff] [review] specific pane patch with test Review of attachment 619419 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me :)

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 on attachment 619419 [details] [diff] [review] specific pane patch with test Thanks for catching that Dao.

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

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

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.

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

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.

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

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 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?

(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.

Bug 754304 could be one way to do this.

(or maybe that bug depends on this one)

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

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 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

(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

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

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

Try push: https://tbpl.mozilla.org/?tree=Try&rev=466ff66e63c2 Relanded with the leak fixed: https://hg.mozilla.org/integration/fx-team/rev/8480626f3af3

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?

(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.

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).