Last Comment Bug 724686 - General pane for in-content Preferences
: General 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: Jon Rietveld
:
Mentors:
Depends on:
Blocks: 718011
  Show dependency treegraph
 
Reported: 2012-02-06 14:30 PST by MSU Capstone Team
Modified: 2012-06-22 04:44 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
first patch for General pane of in-content prefs (13.43 KB, patch)
2012-02-07 14:26 PST, Devan Sayles
jaws: feedback+
bmcbride: feedback+
Details | Diff | Splinter Review
in-content general pane (30.89 KB, patch)
2012-03-06 11:37 PST, Jon Rietveld
jaws: feedback+
Details | Diff | Splinter Review
general in-content pane (30.55 KB, patch)
2012-03-13 14:16 PDT, Jon Rietveld
no flags Details | Diff | Splinter Review
general in-content pane (30.61 KB, patch)
2012-03-14 19:50 PDT, Jon Rietveld
no flags Details | Diff | Splinter Review
general in-content pane (27.34 KB, patch)
2012-03-14 20:11 PDT, Jon Rietveld
jaws: feedback+
Details | Diff | Splinter Review
in-content general pane (27.00 KB, patch)
2012-03-22 13:54 PDT, Jon Rietveld
jaws: feedback+
Details | Diff | Splinter Review
general in-content pane (27.00 KB, patch)
2012-03-22 16:20 PDT, Jon Rietveld
no flags Details | Diff | Splinter Review
general in-content pane (27.16 KB, patch)
2012-03-24 20:06 PDT, Jon Rietveld
no flags Details | Diff | Splinter Review
general in-content pane (27.74 KB, patch)
2012-03-25 23:04 PDT, Jon Rietveld
jaws: feedback+
bmcbride: feedback+
Details | Diff | Splinter Review
general in-content patch (27.20 KB, patch)
2012-04-08 22:47 PDT, Jon Rietveld
jaws: feedback+
Details | Diff | Splinter Review
General with updated data-category (27.19 KB, patch)
2012-04-10 11:06 PDT, MSU Capstone Team
no flags Details | Diff | Splinter Review
general in-content patch (27.09 KB, patch)
2012-04-12 12:36 PDT, Jon Rietveld
no flags Details | Diff | Splinter Review
general in-content patch for new landing patch (27.19 KB, patch)
2012-04-12 13:17 PDT, Jon Rietveld
bmcbride: review+
jaws: feedback+
Details | Diff | Splinter Review

Description MSU Capstone Team 2012-02-06 14:30:31 PST
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
Comment 1 Devan Sayles 2012-02-07 14:26:52 PST
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! 

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 2 Jared Wein [:jaws] (please needinfo? me) 2012-02-07 16:26:29 PST
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.
Comment 3 Devan Sayles 2012-02-08 14:49:26 PST
(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?
Comment 4 Jared Wein [:jaws] (please needinfo? me) 2012-02-08 15:23:00 PST
(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>
Comment 5 Devan Sayles 2012-02-08 17:22:57 PST
(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?
Comment 6 Blair McBride [:Unfocused] (UNAVAILABLE) 2012-02-09 03:59:20 PST
(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.
Comment 7 Blair McBride [:Unfocused] (UNAVAILABLE) 2012-02-09 04:43:09 PST
(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 8 Blair McBride [:Unfocused] (UNAVAILABLE) 2012-02-09 04:52:32 PST
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.
Comment 9 Jon Rietveld 2012-03-06 11:37:06 PST
Created attachment 603386 [details] [diff] [review]
in-content general pane

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 10 Jared Wein [:jaws] (please needinfo? me) 2012-03-08 21:48:38 PST
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.
Comment 11 Jon Rietveld 2012-03-13 14:16:22 PDT
Created attachment 605538 [details] [diff] [review]
general in-content pane

Fixed all issues addressed in Jared's latest feedback.
Comment 12 Jon Rietveld 2012-03-13 14:17:06 PDT
I have not worked on the JavaScript to fix the use current page button, but plan to do that tonight
Comment 13 Jon Rietveld 2012-03-14 19:50:52 PDT
Created attachment 606082 [details] [diff] [review]
general in-content pane

Fixed javascript for Use Current Page Button. Modified javascript is in the function _getTabsForHomePage.
Comment 14 Jon Rietveld 2012-03-14 20:11:29 PDT
Created attachment 606091 [details] [diff] [review]
general in-content pane

Fixed javascript for Use Current Page Button. Modified javascript is in the function _getTabsForHomePage + changed licenses.
Comment 15 Jared Wein [:jaws] (please needinfo? me) 2012-03-22 11:29:25 PDT
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?
Comment 16 Jon Rietveld 2012-03-22 13:54:05 PDT
Created attachment 608456 [details] [diff] [review]
in-content general pane

update javascript to use filter and update about:addon to open in new tab
Comment 17 Jared Wein [:jaws] (please needinfo? me) 2012-03-22 16:00:31 PDT
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";
Comment 18 Jon Rietveld 2012-03-22 16:20:47 PDT
Created attachment 608528 [details] [diff] [review]
general in-content pane

yup, that makes much more sense haha
Comment 19 Jon Rietveld 2012-03-24 20:06:53 PDT
Created attachment 609061 [details] [diff] [review]
general in-content pane

added consistent title
Comment 20 Jon Rietveld 2012-03-25 23:04:20 PDT
Created attachment 609222 [details] [diff] [review]
general in-content pane

included search vbox's, and fixed title
Comment 21 Blair McBride [:Unfocused] (UNAVAILABLE) 2012-04-02 00:18:56 PDT
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.
Comment 22 Jon Rietveld 2012-04-08 22:47:55 PDT
Created attachment 613227 [details] [diff] [review]
general in-content patch

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 23 MSU Capstone Team 2012-04-10 11:06:50 PDT
Created attachment 613677 [details] [diff] [review]
General with updated data-category
Comment 24 Jared Wein [:jaws] (please needinfo? me) 2012-04-11 21:00:46 PDT
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.
Comment 25 Jon Rietveld 2012-04-12 12:36:34 PDT
Created attachment 614498 [details] [diff] [review]
general in-content patch

took in account jareds feedback and made adjustments for new landing patch
Comment 26 Jon Rietveld 2012-04-12 13:17:56 PDT
Created attachment 614530 [details] [diff] [review]
general in-content patch for new landing patch
Comment 27 Jared Wein [:jaws] (please needinfo? me) 2012-05-08 22:38:31 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9262b622112
Comment 28 Ed Morley [:emorley] 2012-05-09 07:42:22 PDT
https://hg.mozilla.org/mozilla-central/rev/c9262b622112
Comment 29 Alfredos-Panagiotis Damkalis [:fredy] 2012-06-22 04:44:41 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.