We'll need to add a pref that toggles between the two preference implementations. The pref should be added to: /browser/app/profile/firefox.js Then, in openPreferences(), we'll need to check the preference flag and see open the relevant preference implementation (http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserContentHandler.js#292). Use |Services.prefs.getBoolPref("pref-name-here");| to read a pref. Let's call the pref "browser.preferences.in-content" and default the pref to false.

Going with sentence case as other prefs do, we should actually call it: "browser.preferences.inContent"

Added the new pref with correct name and default value of false in /browser/app/profile/firefox.js Created a check for the preference flag in openPreferences() but was confused on what needed to be inside of the if-else statement. Feedback/advice appreciated!

Comment on attachment 605628 [details] [diff] [review] first attempt patch for toggling prefs Review of attachment 605628 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/app/profile/firefox.js @@ +1128,5 @@ > // (this pref has no effect if more than 6 hours have passed since the last crash) > pref("toolkit.startup.max_resumed_crashes", 2); > + > +// Toggles between the two Preferences implementations, pop-up window and in-content > +pref("browser.preferences.inContent", false) Please move this preference to be next to the other browser.preferences.* preferences. ::: browser/components/nsBrowserContentHandler.js @@ +300,5 @@ > + if (Services.prefs.getBoolPref("browser.preferences.inContent")) { > + //open in-content prefs > + } > + else { > + //open pop-up prefs This is the right way to do it here, but you should move all the code for opening up the pop-up preferences in to this branch.

By "all the code for opening up the pop-up preferences", do you mean the entire contents of the browser/components/preferences directory (minus the new in-content stuff of course)? Also, this brought up a new question - some of the new in-content files still reference and use unchanged original files in the regular prefs directory (for example, in-content content.js uses fonts.xul, colors.xul, etc) We were wondering how these dependencies should be handled, especially if all of the old prefs code is going to be moved.

previous feedback incorporated

made changes to openPreferences in browser/base/content/utilityOverlay.js

Comment on attachment 606090 [details] [diff] [review] toggle-prefs with change to utilityOverlay.js Review of attachment 606090 [details] [diff] [review]: ----------------------------------------------------------------- This looks great! r+ from me with the below nits fixed, but technically I can't grant r+ so I'll wait for Blair to rubberstamp this. ::: browser/base/content/utilityOverlay.js @@ +459,5 @@ > + if (Services.prefs.getBoolPref("browser.preferences.inContent")) { > + //open in-content prefs > + openUILinkIn("about:preferences", "tab"); > + } > + else { nit: please place the curly brackets on the same line as the else, like, } else {, so it is more consistent with the rest of the file and also do the same for your change in nsBrowserContentHandler.js. ::: browser/components/nsBrowserContentHandler.js @@ +293,5 @@ > return wwatch.openWindow(parent, url, target, features, argArray); > } > > function openPreferences() { > + nit: please remove this blank line @@ +295,5 @@ > > function openPreferences() { > + > + if (Services.prefs.getBoolPref("browser.preferences.inContent")) { > + //open in-content prefs nit: i think these comments ("open in-content prefs", and "open pop-up prefs") are redundant since the if condition is basically saying that already, so I would just remove them from this file and nsBrowserContentHandler.js.

Comment on attachment 606090 [details] [diff] [review] toggle-prefs with change to utilityOverlay.js Review of attachment 606090 [details] [diff] [review]: ----------------------------------------------------------------- This will need a test. This bug isn't tightly coupled with all the other work, so the test should be added here and now. The test should go in browser/base/content/test/ ::: browser/base/content/utilityOverlay.js @@ +456,5 @@ > > function openPreferences(paneID, extraArgs) > { > + if (Services.prefs.getBoolPref("browser.preferences.inContent")) { > + //open in-content prefs What Jared said about removing these comments, but in general: Observe the established style for comments, the format should be: // This is a comment. @@ +482,4 @@ > } > > + return openDialog("chrome://browser/content/preferences/preferences.xul", > + "Preferences", features, paneID, extraArgs); Second line should be lined up with the first argument on the first line.

Feedback from Blair and Jared's previous comments incorporated. I started a test, browser/base/content/test/browser_bug735471.js with a commented outline of what I think I need to do based on talking with Jared, and code where I thought I knew how to accomplish a step. Questions on other steps of the test are in the comments I wrote.

Comment on attachment 607277 [details] [diff] [review] feedback incorporated, test started Review of attachment 607277 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/test/browser_bug735471.js @@ +13,5 @@ > + Services.prefs.setBoolPref("browser.preferences.inContent", true); > + openPreferences(); > + > + > + // Get location.href of the tab, compare to "about:preferences" You can add an event listener for "TabOpen" and within that event listener add an event listener for "PageShow" and within *that* event listener check the location.href of the tab. :) See http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/browser_bug565667.js#61 for an example of what I'm talking about. @@ +14,5 @@ > + openPreferences(); > + > + > + // Get location.href of the tab, compare to "about:preferences" > + // If check comes back false, how to report/log this? In mochitest, you can use the |is()| function to test these cases. Using |is()| will correctly report back the failure and log it. For example, |is(location.href, "about:preferences", "The tab opened did not have the expected location.");| @@ +28,5 @@ > + // Close tab: Browser.closeTab(prefsTab) > + > + > + // Reset pref to its default of false > + Services.prefs.setBoolPref("browser.preferences.inContent", false); This should use |Services.prefs.clearUserPref("browser.preferences.inContent");|

Attempting to use events to check for the prefs tab when opened, and observer to check if preferences.xul is opened. Still feeling a bit lost on this test... I know exactly what needs to be done, just not how to implement it.

Comment on attachment 607837 [details] [diff] [review] more in test, test added to makefile.in This is mainly just feedback on the test since everything else was good the last time I looked at it. It's kind of hard to explain this in written form, so forgive me if this is confusing and ask me on IRC for clarification if I'm not clear. ># HG changeset patch ># Parent 445e1040bfcf5372fb573dc0fdd61ea760667ca9 ># User Joe Chen <joejoevictor@gmail.com> Owen Carpenter <owenccarpenter@gmail.com>; Devan Sayles <devan.sayles@gmail.com>; Jon Reitveld <jon.retiveld@gmail.com> >Patch for Bug-73571, toggle between pop-up and in-content prefs This has the wrong bug number in the description of the patch. Please do |hg qref -e| to fix the description. >diff --git a/browser/base/content/test/Makefile.in b/browser/base/content/test/Makefile.in > browser_bug719271.js \ >+ browser_bug735471.js \ > browser_canonizeURL.js \ nit: Can you replace these tab characters with spaces? >diff --git a/browser/base/content/test/browser_bug735471.js b/browser/base/content/test/browser_bug735471.js >+ >+function test() { >+ waitForExplicitFinish(); // From example browser_bug565667.js >+ >+ // Verify that about:preferences tab is displayed when >+ // browser.preferences.inContent is set to true >+ Services.prefs.setBoolPref("browser.preferences.inContent", true); >+ >+ gBrowser.tabContainer.addEventListener("TabOpen", function(aEvent) { >+ gBrowser.tabContainer.removeEventListener("TabOpen", arguments.callee, true); >+ let browser = aEvent.originalTarget.linkedBrowser; >+ browser.addEventListener("pageshow", function(event) { >+ >+ browser.removeEventListener("pageshow", arguments.callee, true); >+ >+ is(location.href, "about:preferencees", "Checking if the preferences tab was opened"); This is good up to this point. So the test, as I'm reading it, works by enabling the preference, then opening the preferences and checking that a tab has opened and it's href is about:preferences. Now, the next step is to run the second part of the test. So at this point in the code is where you should close the tab, |gBrowser.removeCurrentTab();|, and then flip the pref to false. After flipping the pref to false, then you can call openPreferences() again. Calling openPreferences() again should now run the |observe| function that you have defined in the prototype of myObserver. The current |observe| function definition looks good-so-far (however, it should be changed[1]), but after the is() check[2], you should call this.unregister(), Services.pref.clearUserPref(...), close the window, then call finish() since at the beginning of the test you called waitForExplicitFinish(). [1] Please use "domwindowopened" and see here (https://developer.mozilla.org/en/nsIWindowWatcher) for an example of how to create the observer for that specific notification (note that it uses nsIWindowWatcher and not nsIObserverService. The subject of the notification will be an nsISupports object, which will need to be converted to a nsIDOMWindow. You can use this |let win = subject.QueryInterface(Components.interfaces.nsIDOMWindow);| to get the window object from the subject, and then check win.location.href to see the href of the window. [2] You'll need to use the full chrome:// url here. You can place a dump() or alert() statement before the is() call to see what the chrome:// url is.

Latest feedback integrated.

Comment on attachment 608711 [details] [diff] [review] toggle-prefs updated test Review of attachment 608711 [details] [diff] [review]: ----------------------------------------------------------------- The test here doesn't seem like it would work. Did you run the test locally?

Comment on attachment 608711 [details] [diff] [review] toggle-prefs updated test Review of attachment 608711 [details] [diff] [review]: ----------------------------------------------------------------- Indeed, this test looks broken. Some notes from before I saw the brokenness: ::: browser/base/content/test/browser_bug735471.js @@ +3,5 @@ > + * 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 file, You can > + * obtain one at http://mozilla.org/MPL/2.0/. > + * > + * ***** END LICENSE BLOCK ***** */ MPL2 doesn't use see BEGIN/END LICENSE BLOCK - see https://www.mozilla.org/MPL/headers/ for exact text to use. @@ +7,5 @@ > + * ***** END LICENSE BLOCK ***** */ > + > + > +function test() { > + waitForExplicitFinish(); // From example browser_bug565667.js Remove this comment, it's not useful. @@ +16,5 @@ > + > + gBrowser.tabContainer.addEventListener("TabOpen", function(aEvent) { > + gBrowser.tabContainer.removeEventListener("TabOpen", arguments.callee, true); > + let browser = aEvent.originalTarget.linkedBrowser; > + browser.addEventListener("pageshow", function(event) { aEvent (be consistent - you use aEvent just a few lines above) ::: browser/components/nsBrowserContentHandler.js @@ +296,5 @@ > function openPreferences() { > + if (Services.prefs.getBoolPref("browser.preferences.inContent")) { > + openUILinkIn("about:preferences", "tab"); > + } > + else { No newline between "}" and "else".

More feedback incorporated, plus browser_bug735471.js has tests that pass now.

Comment on attachment 609588 [details] [diff] [review] toggle-prefs with tests that pass! Review of attachment 609588 [details] [diff] [review]: ----------------------------------------------------------------- Good to go!

Comment on attachment 609588 [details] [diff] [review] toggle-prefs with tests that pass! Er, *this* is the flag I wanted.

Comment on attachment 609588 [details] [diff] [review] toggle-prefs with tests that pass! openPreferences doesn't always return a value. You can just make it never return a value.

What happens to the paneID argument with browser.preferences.inContent set to true?

(In reply to Dão Gottwald [:dao] from comment #21) > What happens to the paneID argument with browser.preferences.inContent set > to true? Nothing for now, that's intentional. Will be handled in a followup (filed bug 741047, which I forgot to do earlier).

Comment on attachment 609588 [details] [diff] [review] toggle-prefs with tests that pass! Also, nsBrowserContentHandler.js can't call openUILinkIn.

Comment on attachment 609588 [details] [diff] [review] toggle-prefs with tests that pass! (In reply to Dão Gottwald [:dao] from comment #23) > Comment on attachment 609588 [details] [diff] [review] > toggle-prefs with tests that pass! > > Also, nsBrowserContentHandler.js can't call openUILinkIn. Ugh, indeed - thanks for catching that. Devan: in nsBrowserContentHandler.js, you'll need to do something similar to what the doSearch() function in that file does.

(In reply to Blair McBride (:Unfocused) from comment #24) > Devan: in nsBrowserContentHandler.js, you'll need to do something similar to > what the doSearch() function in that file does. Which part of the doSearch function is applicable to opening a tab? From what I have gathered, the function does quite a but if setup work and then makes a call to openWindow (which is the same function I use in openPreferences to open the prefs pop-up if browser.preferences.inContent is set to false). I'm having trouble identifying an alternative to using openUILinkIn or anything related to gBrowser (such as gBrowser.addTab) in order to open a tab.

Modified nsBrowserContentHandler.js OpenPreferences() to do something more like doSearch()

Comment on attachment 613846 [details] [diff] [review] patch with modified nsBrowserContentHandler.js Review of attachment 613846 [details] [diff] [review]: ----------------------------------------------------------------- As discussed on IRC, this doesn't actually open the preferences.

Opens in-content preferences when command line argument -preferences is passed

Comment on attachment 614036 [details] [diff] [review] patch with modified nsBrowserContentHandler.js Review of attachment 614036 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/test/browser_bug735471.js @@ +42,5 @@ > + is(win.location.href, "chrome://browser/content/preferences/preferences.xul", "Checking if the preferences window was opened"); > + win.close(); > + > + // Reset pref to its default > + Services.prefs.clearUserPref("browser.preferences.inContent"); Please use registerCleanupFunction() at the beginning of the test to make sure that this preference gets cleared. For example, registerCleanupFunction(function() { Services.prefs.clearUserPref("browser.preferences.inContent"); });. You can put this after you call waitForExplicitFinish(). If you make this change, then we won't need to clear the preference here and we can get a guarantee that the preference will be cleared even if this test unexpectedly exits early.

Comment on attachment 614516 [details] [diff] [review] patch with registerCleanupFunction Review of attachment 614516 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/nsBrowserContentHandler.js @@ +290,5 @@ > } > > function openPreferences() { > + if (Services.prefs.getBoolPref("browser.preferences.inContent")) { > + // fill our nsISupportsArray with uri-as-wstring, null, null, postData Remove this comment - it's pretty obvious what the code does (and the comment is incorrect anyway, you're only adding one thing to the array).

removed comment

I verify fixed this bug, tested at Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20120621 Firefox/15.0a2 and the option works fine.