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

(In reply to Devan Sayles from comment #0) > AppleWebKit/535.11 (KHTML, like Gecko) Chrome/17.0.963.56 Safari/535.11 Sigh...

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 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)

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 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 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 :)

since I already have the correct landing structure I figured I would quick make the changes. Blair feedback has been fixed.

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.

added consistent title

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 on attachment 605606 [details] [diff] [review] updated Content patch based on Jared's feedback (Remember to mark old patches as obsolete.)

fixed feedback, included search vbox's, and fixed title

removed vbox from heading and removed vbox around 1st child elements and added data-category and hidden to 1st child element instead

\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

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