Closed Bug 732125 Opened 12 years ago Closed 12 years ago

Content pane for In-Content preferences

Categories

(Firefox :: Settings UI, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 15

People

(Reporter: saylesd1, Assigned: saylesd1)

References

Details

(Whiteboard: [testday-20120622])

Attachments

(1 file, 8 obsolete files)

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
Component: Untriaged → Preferences
OS: Mac OS X → All
Hardware: x86 → All
(In reply to Devan Sayles from comment #0)
> AppleWebKit/535.11 (KHTML, like Gecko) Chrome/17.0.963.56 Safari/535.11

Sigh...
Assignee: nobody → saylesd1
Blocks: 718011
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
QA Contact: untriaged → preferences
Attached patch first patch for content pane (obsolete) — Splinter Review
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?
Attachment #605182 - Flags: feedback?(jwein)
Attachment #605182 - Flags: feedback?(bmcbride)
Attachment #605182 - Attachment is patch: true
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)
Attachment #605182 - Flags: feedback?(jwein) → 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.
Attachment #605182 - Attachment is obsolete: true
Attachment #605182 - Flags: feedback?(bmcbride)
Attachment #605606 - Attachment is patch: true
Attachment #605606 - Flags: feedback?(jwein)
Attachment #605606 - Flags: feedback?(bmcbride)
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.
Attachment #605606 - Flags: feedback?(bmcbride) → feedback+
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 :)
Attachment #605606 - Flags: feedback?(jwein)
Attached patch content in-content pane (obsolete) — Splinter Review
since I already have the correct landing structure I figured I would quick make the changes. Blair feedback has been fixed.
Attachment #608515 - Flags: feedback?(jwein)
Attachment #608515 - Flags: feedback?(bmcbride)
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.
Attached patch content in-content pane (obsolete) — Splinter Review
added consistent title
Attachment #608515 - Attachment is obsolete: true
Attachment #609065 - Flags: feedback?(jwein)
Attachment #609065 - Flags: feedback?(bmcbride)
Attachment #608515 - Flags: feedback?(jwein)
Attachment #608515 - Flags: feedback?(bmcbride)
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
Attachment #609065 - Flags: feedback?(bmcbride) → feedback+
Comment on attachment 605606 [details] [diff] [review]
updated Content patch based on Jared's feedback

(Remember to mark old patches as obsolete.)
Attachment #605606 - Attachment is obsolete: true
Attached patch content in-content pane (obsolete) — Splinter Review
fixed feedback, included search vbox's, and fixed title
Attachment #609065 - Attachment is obsolete: true
Attachment #609226 - Flags: feedback?(jwein)
Attachment #609226 - Flags: feedback?(bmcbride)
Attachment #609065 - Flags: feedback?(jwein)
Attachment #609226 - Flags: feedback?(jwein) → feedback+
Attachment #609226 - Flags: feedback?(bmcbride) → review+
Attached patch content in-content patch (obsolete) — Splinter Review
removed vbox from heading and removed vbox around 1st child elements and added data-category and hidden to 1st child element instead
Attachment #609226 - Attachment is obsolete: true
Attachment #613232 - Flags: feedback?(jwein)
Attachment #613232 - Flags: feedback?(bmcbride)
Attachment #613232 - Flags: feedback?(jwein) → feedback+
Attachment #613232 - Attachment is obsolete: true
Attachment #614504 - Flags: feedback?(jwein)
Attachment #614504 - Flags: feedback?(bmcbride)
Attachment #613232 - Flags: feedback?(bmcbride)
Attachment #614504 - Attachment is obsolete: true
Attachment #614535 - Flags: feedback?(jwein)
Attachment #614535 - Flags: feedback?(bmcbride)
Attachment #614504 - Flags: feedback?(jwein)
Attachment #614504 - Flags: feedback?(bmcbride)
Attachment #614535 - Flags: feedback?(jwein) → feedback+
Attachment #613678 - Attachment is obsolete: true
Attachment #614535 - Flags: feedback?(bmcbride) → review+
\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
Target Milestone: --- → Firefox 15
https://hg.mozilla.org/mozilla-central/rev/4953a39c52e6
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
I verify fixed this bug, tested at Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20120621 Firefox/15.0a2.
Status: RESOLVED → VERIFIED
Whiteboard: [testday-20120622]
You need to log in before you can comment on or make changes to this bug.