Last Comment Bug 735471 - Add a pref to switch between window'd preferences and in-content preferences
: Add a pref to switch between window'd preferences and in-content preferences
Status: VERIFIED FIXED
[testday-20120622]
:
Product: Firefox
Classification: Client Software
Component: Preferences (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 15
Assigned To: Devan Sayles
:
Mentors:
Depends on: 741047
Blocks: 718011
  Show dependency treegraph
 
Reported: 2012-03-13 15:11 PDT by Jared Wein [:jaws] (please needinfo? me)
Modified: 2012-06-22 04:18 PDT (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
first attempt patch for toggling prefs (1.42 KB, patch)
2012-03-13 18:48 PDT, Devan Sayles
jaws: feedback+
Details | Diff | Splinter Review
toggle-prefs patch (1.87 KB, patch)
2012-03-14 17:59 PDT, Devan Sayles
no flags Details | Diff | Splinter Review
toggle-prefs patch with more context (2.61 KB, patch)
2012-03-14 18:48 PDT, Devan Sayles
no flags Details | Diff | Splinter Review
toggle-prefs with change to utilityOverlay.js (4.94 KB, patch)
2012-03-14 20:06 PDT, Devan Sayles
jaws: feedback+
bmcbride: feedback+
Details | Diff | Splinter Review
feedback incorporated, test started (6.11 KB, patch)
2012-03-19 13:21 PDT, Devan Sayles
no flags Details | Diff | Splinter Review
more in test, test added to makefile.in (8.28 KB, patch)
2012-03-20 19:23 PDT, Devan Sayles
jaws: feedback+
Details | Diff | Splinter Review
toggle-prefs updated test (8.03 KB, patch)
2012-03-23 08:03 PDT, Devan Sayles
bmcbride: feedback-
Details | Diff | Splinter Review
toggle-prefs with tests that pass! (8.20 KB, patch)
2012-03-26 19:41 PDT, Devan Sayles
bmcbride: review-
dao+bmo: review-
Details | Diff | Splinter Review
patch with modified nsBrowserContentHandler.js (6.97 KB, patch)
2012-04-10 18:52 PDT, Jon Rietveld
bmcbride: feedback-
Details | Diff | Splinter Review
patch with modified nsBrowserContentHandler.js (7.53 KB, patch)
2012-04-11 09:41 PDT, Jon Rietveld
jaws: feedback+
Details | Diff | Splinter Review
patch with registerCleanupFunction (7.53 KB, patch)
2012-04-12 12:51 PDT, Jon Rietveld
no flags Details | Diff | Splinter Review
patch with registerCleanupFunction (7.56 KB, patch)
2012-04-12 12:55 PDT, Jon Rietveld
jaws: feedback+
bmcbride: feedback+
Details | Diff | Splinter Review
pref to switch patch (7.48 KB, patch)
2012-04-20 09:29 PDT, Jon Rietveld
jaws: review+
bmcbride: review+
Details | Diff | Splinter Review

Description Jared Wein [:jaws] (please needinfo? me) 2012-03-13 15:11:41 PDT
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.
Comment 1 Jared Wein [:jaws] (please needinfo? me) 2012-03-13 15:13:36 PDT
Going with sentence case as other prefs do, we should actually call it:
"browser.preferences.inContent"
Comment 2 Devan Sayles 2012-03-13 18:48:08 PDT
Created attachment 605628 [details] [diff] [review]
first attempt patch for toggling prefs

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 3 Jared Wein [:jaws] (please needinfo? me) 2012-03-13 19:54:31 PDT
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.
Comment 4 Devan Sayles 2012-03-14 17:31:48 PDT
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.
Comment 5 Devan Sayles 2012-03-14 17:59:47 PDT
Created attachment 606042 [details] [diff] [review]
toggle-prefs patch

previous feedback incorporated
Comment 6 Devan Sayles 2012-03-14 18:48:05 PDT
Created attachment 606060 [details] [diff] [review]
toggle-prefs patch with more context
Comment 7 Devan Sayles 2012-03-14 20:06:24 PDT
Created attachment 606090 [details] [diff] [review]
toggle-prefs with change to utilityOverlay.js

made changes to openPreferences in browser/base/content/utilityOverlay.js
Comment 8 Jared Wein [:jaws] (please needinfo? me) 2012-03-15 16:05:06 PDT
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 9 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-03-15 21:07:34 PDT
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.
Comment 10 Devan Sayles 2012-03-19 13:21:44 PDT
Created attachment 607277 [details] [diff] [review]
feedback incorporated, test started

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 11 Jared Wein [:jaws] (please needinfo? me) 2012-03-19 14:13:56 PDT
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");|
Comment 12 Devan Sayles 2012-03-20 19:23:11 PDT
Created attachment 607837 [details] [diff] [review]
more in test, test added to makefile.in

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 13 Jared Wein [:jaws] (please needinfo? me) 2012-03-20 19:55:25 PDT
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.
Comment 14 Devan Sayles 2012-03-23 08:03:07 PDT
Created attachment 608711 [details] [diff] [review]
toggle-prefs updated test

Latest feedback integrated.
Comment 15 Jared Wein [:jaws] (please needinfo? me) 2012-03-23 16:00:36 PDT
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 16 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-03-25 04:16:55 PDT
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".
Comment 17 Devan Sayles 2012-03-26 19:41:06 PDT
Created attachment 609588 [details] [diff] [review]
toggle-prefs with tests that pass!

More feedback incorporated, plus browser_bug735471.js has tests that pass now.
Comment 18 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-03-30 18:44:20 PDT
Comment on attachment 609588 [details] [diff] [review]
toggle-prefs with tests that pass!

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

Good to go!
Comment 19 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-03-30 18:48:03 PDT
Comment on attachment 609588 [details] [diff] [review]
toggle-prefs with tests that pass!

Er, *this* is the flag I wanted.
Comment 20 Dão Gottwald [:dao] 2012-03-30 18:59:33 PDT
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.
Comment 21 Dão Gottwald [:dao] 2012-03-30 19:03:56 PDT
What happens to the paneID argument with browser.preferences.inContent set to true?
Comment 22 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-03-30 22:58:39 PDT
(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 23 Dão Gottwald [:dao] 2012-03-31 01:29:52 PDT
Comment on attachment 609588 [details] [diff] [review]
toggle-prefs with tests that pass!

Also, nsBrowserContentHandler.js can't call openUILinkIn.
Comment 24 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-03-31 05:33:22 PDT
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.
Comment 25 Devan Sayles 2012-04-04 13:39:25 PDT
(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.
Comment 26 Jon Rietveld 2012-04-10 18:52:25 PDT
Created attachment 613846 [details] [diff] [review]
patch with modified nsBrowserContentHandler.js

Modified nsBrowserContentHandler.js OpenPreferences() to do something more like doSearch()
Comment 27 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-04-10 19:45:00 PDT
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.
Comment 28 Jon Rietveld 2012-04-11 09:41:41 PDT
Created attachment 614036 [details] [diff] [review]
patch with modified nsBrowserContentHandler.js

Opens in-content preferences when command line argument -preferences is passed
Comment 29 Jared Wein [:jaws] (please needinfo? me) 2012-04-11 21:17:24 PDT
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 30 Jon Rietveld 2012-04-12 12:51:25 PDT
Created attachment 614512 [details] [diff] [review]
patch with registerCleanupFunction
Comment 31 Jon Rietveld 2012-04-12 12:55:24 PDT
Created attachment 614516 [details] [diff] [review]
patch with registerCleanupFunction
Comment 32 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-04-15 19:46:16 PDT
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).
Comment 33 Jon Rietveld 2012-04-20 09:29:18 PDT
Created attachment 617000 [details] [diff] [review]
pref to switch patch

removed comment
Comment 34 Jared Wein [:jaws] (please needinfo? me) 2012-05-08 22:41:29 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b4f86c85565
Comment 35 Ed Morley [:emorley] 2012-05-09 07:43:51 PDT
https://hg.mozilla.org/mozilla-central/rev/9b4f86c85565
Comment 36 Alfredos-Panagiotis Damkalis [:fredy] 2012-06-22 04:18:18 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.