Last Comment Bug 732125 - Content pane for In-Content preferences
: Content pane for 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:
Blocks: 718011
  Show dependency treegraph
 
Reported: 2012-03-01 11:56 PST by Devan Sayles
Modified: 2012-06-22 04:29 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
first patch for content pane (22.15 KB, patch)
2012-03-12 15:53 PDT, Devan Sayles
jaws: feedback+
Details | Diff | Splinter Review
updated Content patch based on Jared's feedback (19.20 KB, patch)
2012-03-13 17:24 PDT, Devan Sayles
bmcbride: feedback+
Details | Diff | Splinter Review
content in-content pane (17.16 KB, patch)
2012-03-22 15:58 PDT, Jon Rietveld
no flags Details | Diff | Splinter Review
content in-content pane (17.28 KB, patch)
2012-03-24 20:10 PDT, Jon Rietveld
bmcbride: feedback+
Details | Diff | Splinter Review
content in-content pane (17.25 KB, patch)
2012-03-25 23:09 PDT, Jon Rietveld
bmcbride: review+
jaws: feedback+
Details | Diff | Splinter Review
content in-content patch (16.94 KB, patch)
2012-04-08 22:52 PDT, Jon Rietveld
jaws: feedback+
Details | Diff | Splinter Review
Content with updated data-category (16.96 KB, patch)
2012-04-10 11:07 PDT, MSU Capstone Team
no flags Details | Diff | Splinter Review
content in-content patch for new landing patch (16.97 KB, patch)
2012-04-12 12:39 PDT, Jon Rietveld
no flags Details | Diff | Splinter Review
content in-content patch for new landing patch (17.07 KB, patch)
2012-04-12 13:19 PDT, Jon Rietveld
jaws: review+
bmcbride: review+
jaws: feedback+
Details | Diff | Splinter Review

Description Devan Sayles 2012-03-01 11:56:45 PST
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_2) AppleWebKit/535.11 (KHTML, like Gecko) Chrome/17.0.963.56 Safari/535.11
Comment 1 Jared Wein [:jaws] (please needinfo? me) 2012-03-01 21:36:16 PST
(In reply to Devan Sayles from comment #0)
> AppleWebKit/535.11 (KHTML, like Gecko) Chrome/17.0.963.56 Safari/535.11

Sigh...
Comment 2 Devan Sayles 2012-03-12 15:53:30 PDT
Created attachment 605182 [details] [diff] [review]
first patch for content pane

Patch for content pane, to be applied on top of Jon's new separated landing patch.
Specific questions I have regard the dialog boxes that pop up when the buttons on the right of the pane are pressed. What changes (including styling) should these have, and should I get rid of the Help icon in each?
Comment 3 Jared Wein [:jaws] (please needinfo? me) 2012-03-13 00:44:43 PDT
Comment on attachment 605182 [details] [diff] [review]
first patch for content pane

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

::: browser/components/preferences/in-content/content.js
@@ +1,3 @@
> +# -*- Mode: Java; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*-
> +# ***** BEGIN LICENSE BLOCK *****
> +# Version: MPL 1.1/GPL 2.0/LGPL 2.1

New files should use MPL 2.0.

::: browser/components/preferences/in-content/content.xul
@@ +65,5 @@
> +
> +    <stringbundle id="bundlePreferences" src="chrome://browser/locale/preferences/preferences.properties"/>
> +
> +    <!-- various checkboxes, font-fu -->
> +   <vbox id="content-content" hidden="true">

Please remove the surrounding <vbox> and replace it with <vbox>s around each preference.

::: browser/components/preferences/in-content/home.js
@@ +42,5 @@
>    gPrivacyPane.init();
>    gTabsPane.init();
>    gAdvancedPane.init();
>    gApplicationsPane.init();
> +  

nit: Please remove this blank line.

::: browser/locales/en-US/chrome/browser/preferences/content.dtd
@@ +35,5 @@
>  <!ENTITY chooseLanguage.label         "Choose your preferred language for displaying pages">
>  <!ENTITY chooseButton.label           "Choose…">
>  <!ENTITY chooseButton.accesskey       "o">
> +
> +<!ENTITY contentHeader.label          "Content">

Please use &paneContent.title; instead (https://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/preferences/preferences.dtd#12)
Comment 4 Devan Sayles 2012-03-13 17:24:45 PDT
Created attachment 605606 [details] [diff] [review]
updated Content patch based on Jared's feedback

Changes: uses MPL 2.0 in content.xul and content.js, surrounding vbox removed (Owen to put in individual vboxes with search patch), extra blank line removed, uses &paneContent.title; in dtd.
Comment 5 Blair McBride [:Unfocused] (UNAVAILABLE) 2012-03-22 03:37:25 PDT
Comment on attachment 605606 [details] [diff] [review]
updated Content patch based on Jared's feedback

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

Needs updated for Jon's changes to the page structure/landing page for me to be able to comment on how it integrates there.

::: browser/components/preferences/in-content/content.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 the BEGIN/END LICENSE BLOCK bits - see https://www.mozilla.org/MPL/headers/ for exact text to use. Ditto for content.xul.

@@ +39,5 @@
> +   * The exceptions types which may be passed to this._showExceptions().
> +   */
> +  _exceptionsParams: {
> +    popup:   { blockVisible: false, sessionVisible: false, allowVisible: true, prefilledHost: "", permissionType: "popup"   },
> +    image:   { blockVisible: true,  sessionVisible: false, allowVisible: true, prefilledHost: "", permissionType: "image"   }

Wrap this to 80 characters a line.

@@ +53,5 @@
> +    var params = this._exceptionsParams[aPermissionType];
> +    params.windowTitle = bundlePreferences.getString(aPermissionType + "permissionstitle");
> +    params.introText = bundlePreferences.getString(aPermissionType + "permissionstext");
> +    
> +    openDialog("chrome://browser/content/preferences/permissions.xul", "Permissions", "resizable=yes", params);

The second argument to openDialog() is equivalent to the first argument for openWindow(), so should be kept the same (ie, "Browser:Permissions").

@@ +125,5 @@
> +   * various annoying behaviors.
> +   */
> +  showAdvancedJS: function ()
> +  {
> +    openDialog("chrome://browser/content/preferences/advanced-scripts.xul", "", null);  

Second argument should be something like "Browser:AdvancedScripts".

@@ +216,5 @@
> +   * configured.
> +   */  
> +  configureFonts: function ()
> +  {
> +    openDialog("chrome://browser/content/preferences/fonts.xul", "", null);

Second argument should be something like "Browser:FontPreferences".

@@ +225,5 @@
> +   * configured.
> +   */
> +  configureColors: function ()
> +  {
> +    openDialog("chrome://browser/content/preferences/colors.xul", "", null);  

Second argument should be something like "Browser:ColorPreferences".

@@ +235,5 @@
> +   * Shows a dialog in which the preferred language for web content may be set.
> +   */
> +  showLanguages: function ()
> +  {
> +    openDialog("chrome://browser/content/preferences/languages.xul", "", null);

Second argument should be something like "Browser:LanguagePreferences".

::: browser/components/preferences/in-content/content.xul
@@ +1,2 @@
> +
> +# -*- Mode: Java; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*-

Remove the blank line above.

Better remove that modeline too... given that it's the wrong settings.

@@ +6,5 @@
> +   - You can obtain one at http://mozilla.org/MPL/2.0/.
> +   -
> +   - ***** END LICENSE BLOCK ***** -->
> +
> +    <preferences id="contentPreferences">

This shouldn't be indented (reduce indentation of entire file by 4 spaces).

@@ +7,5 @@
> +   -
> +   - ***** END LICENSE BLOCK ***** -->
> +
> +    <preferences id="contentPreferences">
> +      <!--XXX buttons prefs -->

Remove this comment, it's not useful.

@@ +9,5 @@
> +
> +    <preferences id="contentPreferences">
> +      <!--XXX buttons prefs -->
> +
> +      <!-- POPUPS, IMAGES, JAVASCRIPT -->

Lets fix up these comments. Proper English sentences, AND NO CAPS LOCK. (Applies to all comments here.)

@@ +36,5 @@
> +    
> +    <script type="application/javascript" src="chrome://mozapps/content/preferences/fontbuilder.js"/>
> +    <script type="application/javascript" src="chrome://browser/content/preferences/in-content/content.js"/>
> +
> +    <stringbundle id="bundlePreferences" src="chrome://browser/locale/preferences/preferences.properties"/>

Wrap these lines to 80 characters a line.

::: browser/components/preferences/in-content/jar.mn
@@ +13,5 @@
>  *   content/browser/preferences/in-content/advanced.js
>  *   content/browser/preferences/in-content/applications.xul
> +*   content/browser/preferences/in-content/applications.js
> +*   content/browser/preferences/in-content/content.xul
> +*   content/browser/preferences/in-content/content.js
\ No newline at end of file

Remove the * from these new lines - those files don't need to be run through the text preprocessor (that's what the * does).

::: browser/components/preferences/in-content/preferences.xul
@@ +81,4 @@
>    <script type="application/javascript"
>       src="chrome://browser/content/preferences/in-content/tabs.js"/>
>    <script type="application/javascript"
> +     src="chrome://browser/content/preferences/in-content/content.js"/>

This is already loaded in content.xul - remove this instance.
Comment 6 Jared Wein [:jaws] (please needinfo? me) 2012-03-22 11:17:22 PDT
Comment on attachment 605606 [details] [diff] [review]
updated Content patch based on Jared's feedback

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

I'll clear my feedback request since Blair has provided a pretty thorough feedback pass already :)
Comment 7 Jon Rietveld 2012-03-22 15:58:11 PDT
Created attachment 608515 [details] [diff] [review]
content in-content pane

since I already have the correct landing structure I figured I would quick make the changes. Blair feedback has been fixed.
Comment 8 Devan Sayles 2012-03-23 08:22:46 PDT
Thanks Jon!

(In reply to Jon Rietveld from comment #7)
> Created attachment 608515 [details] [diff] [review]
> content in-content pane
> 
> since I already have the correct landing structure I figured I would quick
> make the changes. Blair feedback has been fixed.
Comment 9 Jon Rietveld 2012-03-24 20:10:01 PDT
Created attachment 609065 [details] [diff] [review]
content in-content pane

added consistent title
Comment 10 Blair McBride [:Unfocused] (UNAVAILABLE) 2012-03-25 05:20:15 PDT
Comment on attachment 609065 [details] [diff] [review]
content in-content pane

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

::: browser/components/preferences/in-content/content.js
@@ +37,5 @@
> +  _exceptionsParams: {
> +    popup:   { blockVisible: false, sessionVisible: false, allowVisible: true, 
> +               prefilledHost: "", permissionType: "popup"   },
> +    image:   { blockVisible: true,  sessionVisible: false, allowVisible: true, 
> +               prefilledHost: "", permissionType: "image"   }

Only one space before the "}".

@@ +52,5 @@
> +    params.windowTitle = bundlePreferences.getString(aPermissionType + "permissionstitle");
> +    params.introText = bundlePreferences.getString(aPermissionType + "permissionstext");
> +    
> +    openDialog("chrome://browser/content/preferences/permissions.xul", 
> +               "Browser:Permissions", "resizable=yes", params);

Remove the blank line after this. Also, the blank line before this has 4 space characters that shouldn't be there.

::: browser/components/preferences/in-content/content.xul
@@ +30,5 @@
> +
> +<script type="application/javascript" 
> +  src="chrome://mozapps/content/preferences/fontbuilder.js"/>
> +<script type="application/javascript" 
> +  src="chrome://browser/content/preferences/in-content/content.js"/>

The wrapped lines don't have correct indentation here.

@@ +32,5 @@
> +  src="chrome://mozapps/content/preferences/fontbuilder.js"/>
> +<script type="application/javascript" 
> +  src="chrome://browser/content/preferences/in-content/content.js"/>
> +
> +<!-- various checkboxes, font-fu -->

This comment isn't useful, remove it.

::: browser/components/preferences/in-content/jar.mn
@@ +12,5 @@
>  *  content/browser/preferences/in-content/advanced.js
>  *  content/browser/preferences/in-content/applications.xul
>  *  content/browser/preferences/in-content/applications.js
> +*  content/browser/preferences/in-content/content.xul
> +*  content/browser/preferences/in-content/content.js

Neither of these files need to use the text preprocessor (neither use #ifdef, etc) - remove the *.

::: browser/locales/en-US/chrome/browser/preferences/content.dtd
@@ +35,5 @@
>  <!ENTITY chooseLanguage.label         "Choose your preferred language for displaying pages">
>  <!ENTITY chooseButton.label           "Choose…">
>  <!ENTITY chooseButton.accesskey       "o">
> +
> +<!ENTITY paneContent.title            "Content">

This already exists in preferences.dtd
Comment 11 Blair McBride [:Unfocused] (UNAVAILABLE) 2012-03-25 05:21:02 PDT
Comment on attachment 605606 [details] [diff] [review]
updated Content patch based on Jared's feedback

(Remember to mark old patches as obsolete.)
Comment 12 Jon Rietveld 2012-03-25 23:09:18 PDT
Created attachment 609226 [details] [diff] [review]
content in-content pane

fixed feedback, included search vbox's, and fixed title
Comment 13 Jon Rietveld 2012-04-08 22:52:43 PDT
Created attachment 613232 [details] [diff] [review]
content in-content patch

removed vbox from heading and removed vbox around 1st child elements and added data-category and hidden to 1st child element instead
Comment 14 MSU Capstone Team 2012-04-10 11:07:22 PDT
Created attachment 613678 [details] [diff] [review]
Content with updated data-category
Comment 15 Jon Rietveld 2012-04-12 12:39:35 PDT
Created attachment 614504 [details] [diff] [review]
content in-content patch for new landing patch
Comment 16 Jon Rietveld 2012-04-12 13:19:42 PDT
Created attachment 614535 [details] [diff] [review]
content in-content patch for new landing patch
Comment 17 Jared Wein [:jaws] (please needinfo? me) 2012-05-08 22:40:34 PDT
\o/ Pushed to mozilla-inbound. These patches should be merged to mozilla-central within the next day or two. Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/4953a39c52e6
Comment 18 Ed Morley [:emorley] 2012-05-09 07:43:13 PDT
https://hg.mozilla.org/mozilla-central/rev/4953a39c52e6
Comment 19 Alfredos-Panagiotis Damkalis [:fredy] 2012-06-22 04:29:25 PDT
I verify fixed this bug, tested at Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20120621 Firefox/15.0a2.

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