User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_2) AppleWebKit/535.7 (KHTML, like Gecko) Chrome/16.0.912.77 Safari/535.7 Expected results: In-content Preferences pane - General tab

This is the first patch I've submitted myself, so hopefully it's done correctly! I'm having trouble in a few areas: first, there is a button whose label is not getting populated, and I'm pretty sure I traced it back to the fact that gMainPane.init() (gMainPane is created in general.xul, the function defined in main.js) is somehow not getting called, so then the updateUseCurrentContentButton function call within the init is also not made. I tried putting a dump statement in the init function and got nothing, and I cannot figure out why init is being called. Also, I don't think General has the right fonts, though it's using inContentUI.css. Any other feedback is appreciated. Thanks!

Comment on attachment 595182 [details] [diff] [review] first patch for General pane of in-content prefs Review of attachment 595182 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Devan Sayles from comment #1) > Created attachment 595182 [details] [diff] [review] > first patch for General pane of in-content prefs > > This is the first patch I've submitted myself, so hopefully it's done > correctly! Yep, you've done it correctly and the about:general page looks nice so far. Thanks! > I'm having trouble in a few areas: first, there is a button whose label is > not getting populated, and I'm pretty sure I traced it back to the fact that > gMainPane.init() (gMainPane is created in general.xul, the function defined > in main.js) is somehow not getting called, so then the > updateUseCurrentContentButton function call within the init is also not > made. I tried putting a dump statement in the init function and got nothing, > and I cannot figure out why init is being called. I don't think the event handler for onpaneload is being called. This might happen because the file isn't being included through the use of a <prefpane src="" />. Can you try using onload="gMainPane.init()" here and see if that makes a difference? > Also, I don't think General has the right fonts, though it's using > inContentUI.css. about:general looks like it has the correct fonts to me. Are you specifically talking about the font-size of the captions in the fieldset? I do see that general.css sets the size of that font to 20px. > Any other feedback is appreciated. Thanks! General feedback: Take a read through https://developer.mozilla.org/en/Writing_Efficient_CSS and https://wiki.mozilla.org/DevTools/CSSTips for tips on writing the CSS and what will be accepted as CSS selectors. Thanks for getting the ball rolling on the General preferences part.

(In reply to Jared Wein [:jaws] (Vacation Feb. 8 - Feb. 10) from comment #2) > Comment on attachment 595182 [details] [diff] [review] > first patch for General pane of in-content prefs > > Review of attachment 595182 [details] [diff] [review]: > ----------------------------------------------------------------- > > > > I'm having trouble in a few areas: first, there is a button whose label is > > not getting populated, and I'm pretty sure I traced it back to the fact that > > gMainPane.init() (gMainPane is created in general.xul, the function defined > > in main.js) is somehow not getting called, so then the > > updateUseCurrentContentButton function call within the init is also not > > made. I tried putting a dump statement in the init function and got nothing, > > and I cannot figure out why init is being called. > > I don't think the event handler for onpaneload is being called. This might > happen because the file isn't being included through the use of a <prefpane > src="" />. Can you try using onload="gMainPane.init()" here and see if that > makes a difference? > I'm not sure I understand what you mean by that. I tried adding a src attribute to the prefpane (with the value being the location of the javascript file), and the patch already had an onpaneload="gMainPane.init();" attribute in the prefpane, but neither of these seem to make a difference. I'm not sure how to test if the event handler for onpaneload is being called (besides seeing if the button in question gets a label), or what else I should be experimenting with to try and fix the problem. I did find some documentation here (http://www.openxul.com/references/ref_prefpane.html) that talks about using the src attribute in connection with an overlay file, and I took the overlay out of general.xul, so might that be related to the issue at all?

(In reply to Devan Sayles from comment #3) > (In reply to Jared Wein [:jaws] (Vacation Feb. 8 - Feb. 10) from comment #2) > > Comment on attachment 595182 [details] [diff] [review] > > first patch for General pane of in-content prefs > > > > Review of attachment 595182 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > > > > I'm having trouble in a few areas: first, there is a button whose label is > > > not getting populated, and I'm pretty sure I traced it back to the fact that > > > gMainPane.init() (gMainPane is created in general.xul, the function defined > > > in main.js) is somehow not getting called > > I don't think the event handler for onpaneload is being called. This might > > happen because the file isn't being included through the use of a <prefpane > > src="" />. Can you try using onload="gMainPane.init()" here and see if that > > makes a difference? > > > I'm not sure I understand what you mean by that. The function gMainPane.init() is only called when the onpaneload event is fired. It could be that this event isn't being fired, and that's why your code isn't running. The way that you are testing if the code is running is perfectly sound. Can you try replacing the attribute name of onpaneload with onload? For example, how it currently says <prefpane onpaneload="gMainPane.init()" ... >, it would now say <prefpane onload="gMainPane.init()" ... >. If that still doesn't work, can you try moving the <script src="main.js"/> tag down to the end of the XUL file and adding some inline script after it to call gMainPane.init()? For example: > <script src="main.js" /> > <script> > gMainPane.init(); > </script>

(In reply to Jared Wein [:jaws] (Vacation Feb. 8 - Feb. 10) from comment #4) > (In reply to Devan Sayles from comment #3) > > (In reply to Jared Wein [:jaws] (Vacation Feb. 8 - Feb. 10) from comment #2) > > > Comment on attachment 595182 [details] [diff] [review] > > > first patch for General pane of in-content prefs > > > > > > Review of attachment 595182 [details] [diff] [review]: > > > ----------------------------------------------------------------- > > > > > > Changing onpaneload to onload didn't seem to have an effect, but moving the script src tag to the end of the file and adding an inline call to init finally populated the button label! But I was wondering why it is necessary to explicitly make that call to init, when it is made first in the prefpane tag? Also, upon further testing of the General pane, I have noticed that a number of the button events are not being properly triggered when the buttons are pressed - currently, only the "Choose" button actually appears to react properly when pressed. Do you have advice for troubleshooting this issue?

(In reply to Devan Sayles from comment #5) > Do you have advice for troubleshooting this issue? The Error Console shows (most) uncaught exceptions, as well as various warnings and info messages: https://developer.mozilla.org/en/Error_Console Pressing the "Manage Add-ons" button, for instance, will result in the following exception: Error: openUILinkIn is not defined Source file: chrome://browser/content/preferences/main.js Line: 449 Another handy tool is MXR, which is a searchable index of the source code: https://mxr.mozilla.org/mozilla-central/ Searching for an identifier (function) named "openUILinkIn": https://mxr.mozilla.org/mozilla-central/ident?i=openUILinkIn ... shows that it's defined in utilityOverlay.js, which is loaded by the original preferences dialog.

(In reply to Jared Wein [:jaws] (Vacation Feb. 8 - Feb. 10) from comment #4) > Can you try replacing the attribute name of onpaneload with onload? For > example, how it currently says <prefpane onpaneload="gMainPane.init()" ... > >, it would now say <prefpane onload="gMainPane.init()" ... >. ... (In reply to Devan Sayles from comment #5) > Changing onpaneload to onload didn't seem to have an effect Should be adding onload="gMainPane.init();" to the <page> element, not the <prefpane> element. Just tested that, and it works fine.

Comment on attachment 595182 [details] [diff] [review] first patch for General pane of in-content prefs Review of attachment 595182 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/preferences/general.xul @@ +40,5 @@ > +# > +# ***** END LICENSE BLOCK ***** > + > +<?xml-stylesheet href="chrome://global/skin/global.css" type="text/css"?> > +<?xml-stylesheet href="chrome://global/skin/inContentUI.css" type="text/css"?> inContentUI.css should be loaded via @import in a platform-specific (ie, not /content/) stylesheet (see bug 719717 comment 17 for reasoning). @@ +43,5 @@ > +<?xml-stylesheet href="chrome://global/skin/global.css" type="text/css"?> > +<?xml-stylesheet href="chrome://global/skin/inContentUI.css" type="text/css"?> > +<?xml-stylesheet href="chrome://browser/content/preferences/general.css"?> > + > +<!DOCTYPE overlay [ The document element is a <page>, so the DOCTYPE should be for page (not overlay). @@ +46,5 @@ > + > +<!DOCTYPE overlay [ > + <!ENTITY % brandDTD SYSTEM "chrome://branding/locale/brand.dtd"> > + <!ENTITY % mainDTD SYSTEM "chrome://browser/locale/preferences/main.dtd"> > + <!ENTITY % aboutHomeDTD SYSTEM "chrome://browser/locale/aboutHome.dtd"> Why is aboutHome.dtd here? @@ +63,5 @@ > + > + <script type="application/javascript" src="chrome://browser/content/preferences/main.js"/> > + > + <preferences id="mainPreferences"> > + <!-- XXX Button preferences --> While you're touching this code, this comment can be removed (XXX typically denotes a todo item, which this clearly isn't... and it doesn't seem related to anything). @@ +126,5 @@ > + <menulist id="browserStartupPage" preference="browser.startup.page"> > + <menupopup> > + <menuitem label="&startupHomePage.label;" value="1" id="browserStartupHomePage"/> > + <menuitem label="&startupBlankPage.label;" value="0" id="browserStartupBlank"/> > + <menuitem label="&startupLastSession.label;" value="3" id="browserStartupLastSession"/> This would be a good time to clean up inconsistent indenting like this.

general patch split off form huge privacy bug patch. *Note: I did create an init method to add all the panes init methods in.

Comment on attachment 603386 [details] [diff] [review] in-content general pane Review of attachment 603386 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/preferences/in-content/main.xul @@ +87,5 @@ > + type="int"/> > + > + </preferences> > + > + <vbox id="general-content" hidden="true"> These shouldn't have a containing vbox. There should be a vbox containing each preference with an attribute designating which category it belongs to.

Fixed all issues addressed in Jared's latest feedback.

I have not worked on the JavaScript to fix the use current page button, but plan to do that tonight

Fixed javascript for Use Current Page Button. Modified javascript is in the function _getTabsForHomePage.

Fixed javascript for Use Current Page Button. Modified javascript is in the function _getTabsForHomePage + changed licenses.

Comment on attachment 606091 [details] [diff] [review] general in-content pane Review of attachment 606091 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/preferences/in-content/main.js @@ +1,3 @@ > +/*-- 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/. */ Super nitpicky, but this should be: /* 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/. */ See https://www.mozilla.org/MPL/headers/ for more examples. @@ +158,5 @@ > + // We should only include visible & non-pinned tabs > + > + tabs = win.gBrowser.visibleTabs.slice(win.gBrowser._numPinnedTabs); > + > + for (var i=0; i<tabs.length; i++) { nit: spaces around = and < operators. @@ +159,5 @@ > + > + tabs = win.gBrowser.visibleTabs.slice(win.gBrowser._numPinnedTabs); > + > + for (var i=0; i<tabs.length; i++) { > + if(tabs[i].linkedBrowser.currentURI.spec.toString() == "about:preferences") { nit: space after if. @@ +160,5 @@ > + tabs = win.gBrowser.visibleTabs.slice(win.gBrowser._numPinnedTabs); > + > + for (var i=0; i<tabs.length; i++) { > + if(tabs[i].linkedBrowser.currentURI.spec.toString() == "about:preferences") { > + tabs.splice(i,1); Does |i| need to be decremented here? What happens if you have two about:preference tabs opened next to each other? To make this code simpler and less error-prone, you should use Array.filter here (https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Array/filter). @@ +451,5 @@ > + * Displays the Add-ons Manager. > + */ > + showAddonsMgr: function () > + { > + openUILinkIn("about:addons", "window"); This won't need to open about:addons in a window since we're now in a tab. Can you change this to open about:addons in a tab (instead of a window) now?

update javascript to use filter and update about:addon to open in new tab

Comment on attachment 608456 [details] [diff] [review] in-content general pane Review of attachment 608456 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/preferences/in-content/main.js @@ +170,5 @@ > + * Check to see if a tab is not about:preferences > + */ > + isAboutPreferences: function (element, index, array) > + { > + return (!(element.linkedBrowser.currentURI.spec.toString() == "about:preferences")); could this just be: > return element.linkedBrowser.currentURI.spec.toString() != "about:preferences";

yup, that makes much more sense haha

added consistent title

included search vbox's, and fixed title

Comment on attachment 609222 [details] [diff] [review] general in-content pane Review of attachment 609222 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/preferences/in-content/jar.mn @@ +2,5 @@ > * content/browser/preferences/in-content/preferences.js > * content/browser/preferences/in-content/landing.xul > * content/browser/preferences/in-content/preferences.xul > +* content/browser/preferences/in-content/main.xul > +* content/browser/preferences/in-content/main.js main.js doesn't need run through the preprocessor, remove the * ::: browser/components/preferences/in-content/main.js @@ +109,5 @@ > + setHomePageToBookmark: function () > + { > + var rv = { urls: null, names: null }; > + openDialog("chrome://browser/content/preferences/selectBookmark.xul", > + "Select Bookmark", "resizable=yes, modal=yes", rv); Fix indentation here (probably not lining up because there's a tab there instead of space characters). @@ +159,5 @@ > + // We should only include visible & non-pinned tabs > + > + tabs = win.gBrowser.visibleTabs.slice(win.gBrowser._numPinnedTabs); > + > + filteredTabs = tabs.filter(this.isAboutPreferences); No need to keep around multiple arrays for this, just re-use 'tabs'. @@ +168,5 @@ > + > + /** > + * Check to see if a tab is not about:preferences > + */ > + isAboutPreferences: function (element, index, array) Names of arguments should be prefixed with 'a'. ie, aElement, aIndex, etc. See https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide#Prefixes @@ +170,5 @@ > + * Check to see if a tab is not about:preferences > + */ > + isAboutPreferences: function (element, index, array) > + { > + return (element.linkedBrowser.currentURI.spec.toString() != "about:preferences"); Don't think you should need the .toString() call here, .spec is already a string. ::: browser/components/preferences/in-content/main.xul @@ +57,5 @@ > + > +<vbox data-category="general" hidden="true"> > + <hbox class="heading"> > + <image class="preference-icon" type="general"/> > + <html:h1 class="spacing">&paneGeneral.title;</html:h1> Remove the 'spacing' class (I saw the privacy pane patch has it removed, so it's not used, right?) @@ +69,5 @@ > + > + <hbox align="center"> > + <label value="&startupPage.label;" > + accesskey="&startupPage.accesskey;" > + control="browserStartupPage"/> Indentation is messed up for wrapped lines from here onwards.

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

Comment on attachment 613227 [details] [diff] [review] general in-content patch Review of attachment 613227 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/preferences/in-content/main.js @@ +147,5 @@ > + var win; > + var tabs = []; > + > + const Cc = Components.classes, Ci = Components.interfaces; > + // If we're in instant-apply mode, use the most recent browser window This comment doesn't apply anymore, since instantApply will always be true with in-content preferences.

took in account jareds feedback and made adjustments for new landing patch

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