Last Comment Bug 735557 - Add about:preferences to the inContentWhitelist
: Add about:preferences to the inContentWhitelist
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Preferences (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: Firefox 15
Assigned To: Jon Rietveld
:
Mentors:
Depends on:
Blocks: 718011
  Show dependency treegraph
 
Reported: 2012-03-13 20:01 PDT by (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back)
Modified: 2012-05-09 07:41 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
in-content preferences disable toolbar test (1.47 KB, patch)
2012-03-14 21:08 PDT, Jon Rietveld
jaws: feedback+
Details | Diff | Review
remove toolbar (5.04 KB, patch)
2012-03-15 17:15 PDT, Jon Rietveld
no flags Details | Diff | Review
remove toolbar for in-content preferences (4.78 KB, patch)
2012-03-15 18:16 PDT, Jon Rietveld
bmcbride: feedback+
bmcbride: feedback+
Details | Diff | Review
remove toolbar for in-content preferences (1.71 KB, patch)
2012-03-16 07:27 PDT, Jon Rietveld
bmcbride: review+
jaws: feedback+
Details | Diff | Review

Description (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-03-13 20:01:07 PDT
The in-content preferences should be rendered the same way as about:addons, without the navigation toolbar.

To do this

1) Add a testcase for about:preferences to test_disablechrome.js (http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/browser_disablechrome.js#141). This is similar to the work that was done for bug 716464.
2) Run the test to make sure that the test fails for about:preferences.
3) Add about:preferences to the inContentWhitelist here: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#4425
4) Run the test to make sure that the test passes for about:preferences.
Comment 1 Jon Rietveld 2012-03-14 21:08:47 PDT
Created attachment 606105 [details] [diff] [review]
in-content preferences disable toolbar test

In this patch I believe I have the test failing correctly.
Comment 2 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-03-15 16:41:36 PDT
Comment on attachment 606105 [details] [diff] [review]
in-content preferences disable toolbar test

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

This looks good.
Comment 3 Jon Rietveld 2012-03-15 17:15:14 PDT
Created attachment 606407 [details] [diff] [review]
remove toolbar

I have added about:preferences to the inContentWhitelist, but the new tests are still failing and I cannot figure out why. Any feedback would be greatly appreciated!
Comment 4 Jon Rietveld 2012-03-15 18:16:59 PDT
Created attachment 606416 [details] [diff] [review]
remove toolbar for in-content preferences

Everything appears to be working correctly! All tests pass :)
Comment 5 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-03-15 18:30:47 PDT
Comment on attachment 606416 [details] [diff] [review]
remove toolbar for in-content preferences

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

Gah, so many whitespace changes! They are for the better though, so we could undo the whitespace fixes or leave them. I'm fine with the fixes staying. r=me with Blair's approval.
Comment 6 Jon Rietveld 2012-03-15 18:33:23 PDT
yea, Blair said the same thing, duno how those happened :/
Comment 7 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-03-15 18:33:35 PDT
Comment on attachment 606416 [details] [diff] [review]
remove toolbar for in-content preferences

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

Lots of unrelated whitespace changes in browser.js :(

::: browser/base/content/browser.js
@@ +4422,5 @@
>    startTime: 0,
>    statusText: "",
>    isBusy: false,
> +  inContentWhitelist: ["about:addons", "about:permissions",
> +    "about:sync-progress", "about:preferences"],

Indent the second line to be lined up with the first item in the array on the first line.

::: browser/base/content/test/browser_disablechrome.js
@@ +195,5 @@
> +  test_url(HTTPSRC + "disablechrome.html", false, run_chrome_about_preferences_test_5);
> +}
> +
> +// Should not hide the chrome
> +function run_chrome_about_preferences_test_5() {

Keep the function naming consistent with the rest of the file - run_chrome_about_test_5.
Ditto for run_chrome_about_test_6.
Comment 8 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-03-15 18:36:31 PDT
(In reply to Jared Wein [:jaws] from comment #5)
> They are for the better though, so we could
> undo the whitespace fixes or leave them.

I'd rather not have them land with this, on the odd change that this gets backed out at any stage (unnecessary churn in related code).
Comment 9 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-03-15 18:39:35 PDT
Comment on attachment 606416 [details] [diff] [review]
remove toolbar for in-content preferences

(Stupid bugzilla...)
Comment 10 Jon Rietveld 2012-03-16 07:27:54 PDT
Created attachment 606571 [details] [diff] [review]
remove toolbar for in-content preferences

Fixed naming issues and removed unrelated whitespace from browser.js :)
Comment 11 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-03-16 13:00:51 PDT
Comment on attachment 606571 [details] [diff] [review]
remove toolbar for in-content preferences

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

Looks good. r=me with Blair's approval :P
Comment 12 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-05-08 22:37:39 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2d23fc61c6f
Comment 13 Ed Morley [:emorley] 2012-05-09 07:41:54 PDT
https://hg.mozilla.org/mozilla-central/rev/b2d23fc61c6f

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