Closed Bug 723328 Opened 12 years ago Closed 12 years ago

Move the privacy preferences to in-content UI

Categories

(Firefox :: Settings UI, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 15

People

(Reporter: jaws, Assigned: jon.rietveld)

References

Details

(Whiteboard: [testday-20120622])

Attachments

(1 file, 16 obsolete files)

26.38 KB, patch
Unfocused
: review+
Details | Diff | Splinter Review
      No description provided.
Attached patch in-content privacy (obsolete) — Splinter Review
I made some css styles for Windows, and took a look on OSX, and have not looked at Linux yet. This is my first patch submit so hopefully it works and I did it correctly!
Attachment #593900 - Flags: feedback?(jwein)
Attachment #593900 - Flags: feedback?(bmcbride)
Comment on attachment 593900 [details] [diff] [review]
in-content privacy

Thanks for requesting feedback. I will do a full feedback pass tomorrow when I have more time.
Comment on attachment 593900 [details] [diff] [review]
in-content privacy

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

I didn't focus too much on the preferences section of aboutPrivacy.xul, as I suspect that it was just copy-and-paste from the current privacy preferences. Let me know if anything was changed.

Please read https://wiki.mozilla.org/DevTools/CSSTips for tips on how you should be writing your CSS code.

::: browser/components/about/AboutRedirector.cpp
@@ +107,5 @@
>      nsIAboutModule::ALLOW_SCRIPT },
>    { "permissions", "chrome://browser/content/preferences/aboutPermissions.xul",
>      nsIAboutModule::ALLOW_SCRIPT },
> +  { "preferences", "chrome://browser/content/preferences/preferences.xul",
> +    nsIAboutModule::ALLOW_SCRIPT },

I don't think these two lines for "preferences" are needed. Is adding about:privacy just a temporary step until the containing page is complete? If that is your intention, then this is OK. If not, then I am missing some extra information and will need to get informed of the plan.

::: browser/components/preferences/aboutPrivacy.xul
@@ +13,5 @@
> +<?xml-stylesheet href="chrome://browser/content/preferences/preferences.css"?>
> +<?xml-stylesheet href="chrome://browser/skin/preferences/preferences.css"?>
> +
> +<?xml-stylesheet href="chrome://browser/skin/preferences/privacy.css"?>
> +<?xml-stylesheet href="chrome://browser/content/preferences/privacy.css"?>

The skin file should always come after the content file, so that the skin file can override any styles.

@@ +26,5 @@
> +
> +  <vbox id="privacy-content" class="main-content" flex="1">
> +    <header class="heading">
> +        <image class="pref-icon" type="privacy"></image>
> +        <p>Privacy</p>

This text belongs in a DTD file so that it is localizable.

::: browser/components/preferences/privacy.css
@@ +12,5 @@
> +    padding: 0;*/
> +}
> +
> +#acceptCookies {
> +/*    border: 1px dashed aqua;*/

I assume that these commented out CSS properties were just used for debugging. No problems with that, just make sure to remove them before requesting review.

@@ +16,5 @@
> +/*    border: 1px dashed aqua;*/
> +}
> +
> +.heading {
> +    font-family: georgia,serif;

Styles like these should be moved to a theme CSS file.

@@ +22,5 @@
> +/*    background-color: #D5DDE9;*/
> +    background-color: rgba(192, 199, 210, 0.70);
> +    font-size: 2.75em;
> +    font-weight: bold;
> +    padding-left: 25px;

Anytime that a padding-left is different than a padding-right (or margin-left/margin-right, etc), there is going to be problems with right-to-left locales such as Hebrew and Arabic. The two options here are to set both the left and right values, which in this case doesn't seem necessary, or to use -moz-padding-start so that it will be applied to the starting padding for both LTR and RTL locales.
Attachment #593900 - Flags: feedback?(jwein)
Hey Jared,

Thanks for the feedback. I made all of those changes to the aboutPrivacy.xul. To answer your questions, I did modify the structure of the aboutPrivacy.xul from privacy.xul. It had a bunch of hbox and vbox elements to center the text that I got rid of. The about:privacy is just a temporary step until the containing page is complete.

I am getting close to submitting another patch, but I have made a skeleton page that does the pre-processing to include some other preferences. Should I still submit that patch under this bug or the main bug?

Thanks
(In reply to Jon Rietveld from comment #4)
> Hey Jared,
> 
> Thanks for the feedback. I made all of those changes to the
> aboutPrivacy.xul. To answer your questions, I did modify the structure of
> the aboutPrivacy.xul from privacy.xul. It had a bunch of hbox and vbox
> elements to center the text that I got rid of. The about:privacy is just a
> temporary step until the containing page is complete.

OK cool.

> I am getting close to submitting another patch, but I have made a skeleton
> page that does the pre-processing to include some other preferences. Should
> I still submit that patch under this bug or the main bug?

I'm fine with it being in this bug. Blair may have a different opinion though. For now, lets just leave it in the same bug and make sure to bring this up in our next meeting.
Comment on attachment 593900 [details] [diff] [review]
in-content privacy

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

::: browser/components/preferences/aboutPrivacy.xul
@@ +7,5 @@
> +%privacyDTD;
> +]>
> +
> +<?xml-stylesheet href="chrome://global/skin/global.css"?>
> +<?xml-stylesheet href="chrome://global/skin/inContentUI.css"?>

inContentUI.css should be loaded via @import in the /skin/ stylesheet (see bug 719717 comment 17 for reasoning).

@@ +21,5 @@
> +         xmlns:html="http://www.w3.org/1999/xhtml">
> +
> +  <prefpane id="panePrivacy"
> +            onpaneload="gPrivacyPane.init();"
> +            helpTopic="prefs-privacy">

<prefpane> should be a child of .main-content (it will end up being a requirement to implement the landing page).

@@ +25,5 @@
> +            helpTopic="prefs-privacy">
> +
> +  <vbox id="privacy-content" class="main-content" flex="1">
> +    <header class="heading">
> +        <image class="pref-icon" type="privacy"></image>

Since XUL is a XML markup, you can close empty tags like so:
  <image class="pref-icon" type="privacy"/>

@@ +36,5 @@
> +      <preference id="privacy.donottrackheader.enabled"
> +                  name="privacy.donottrackheader.enabled"
> +                  type="bool"/>
> +
> +      <!-- XXX button prefs -->

Since you're touching this code anyway, it's a good time to clean up comments like this (XXX typically denotes a todo item, which this clearly isn't).

@@ +67,5 @@
> +
> +      <!-- Cookies -->
> +      <preference id="network.cookie.cookieBehavior"      name="network.cookie.cookieBehavior"      type="int"/>
> +      <preference id="network.cookie.lifetimePolicy"      name="network.cookie.lifetimePolicy"      type="int"/>
> +      <preference id="network.cookie.blockFutureCookies"  name="network.cookie.blockFutureCookies"  type="bool"/>

Again, since you're here anyway... good time to clean up the indentation/line-wrapping style, and make it all consistent.
Attachment #593900 - Flags: feedback?(bmcbride) → feedback+
Attachment #597233 - Flags: feedback?(jwein)
Attachment #597233 - Flags: feedback?(bmcbride)
Comment on attachment 597233 [details] [diff] [review]
Includes "Landing" page, general, tabs, and privacy

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

::: browser/components/preferences/aboutPreferences.xul
@@ +1,3 @@
> +<?xml version="1.0"?>
> +<!-- ***** BEGIN LICENSE BLOCK *****
> +   - Version: MPL 1.1/GPL 2.0/LGPL 2.1

Please use MPL 2 on all new files.

@@ +124,5 @@
> +              onpaneload="gPrivacyPane.init();">
> +#include preferences.xul
> +#include aboutPrivacy.xul
> +#include tabs.xul
> +#include general.xul

These should probably be:
#include preferences.xul
#include general.xul
#include tabs.xul
#include aboutPrivacy.xul

::: browser/components/preferences/aboutPrivacy.xul
@@ +43,5 @@
> +  <header class="heading">
> +      <image class="pref-icon" type="privacy"/>
> +      <p>&header.label;</p>
> +  </header>
> +    <script type="application/javascript" src="chrome://browser/content/utilityOverlay.js"/>

utilityOverlay.js will be included by all of the preference pages, so we could probably just move this <script> tag to aboutPreferences.xul and remove it from all of the subpages.

::: browser/components/preferences/home.js
@@ +1,3 @@
> +function gotoPref(page) {
> +  document.getElementById("landing-content").style.display = "none";
> +  //document.getElementById("landing-content").node.hidden = true;

No need for |.node| here, you should be able to just do:
> document.getElementById("landing-content").hidden = true;
Using .hidden = true will also allow you to remove the setting of display to "none"/"-moz-box".

::: browser/components/preferences/main.js
@@ +145,5 @@
>    setHomePageToBookmark: function ()
>    {
>      var rv = { urls: null, names: null };
> +    openDialog("chrome://browser/content/preferences/selectBookmark.xul",
> +    	    "Select Bookmark", "resizable=yes, modal=yes", rv);

This will need to use a string from a DTD here.

@@ +162,4 @@
>    _updateUseCurrentButton: function () {
>      let useCurrent = document.getElementById("useCurrent");
>  
> +

nit: This extra line isn't needed.

::: browser/components/preferences/preferences.xul
@@ +18,5 @@
> +   - Portions created by the Initial Developer are Copyright (C) 2010
> +   - the Initial Developer. All Rights Reserved.
> +   -
> +   - Contributor(s):
> +   -   Jonathan Rietveld <jon.rietveld@gmail.com>

Since this file isn't new, we shouldn't change the license here. Feel free to add your name to the previous author list though.

::: browser/components/preferences/privacy.css
@@ +81,5 @@
> +  background-position: -192px 0;
> +}
> +
> +.heading {
> +  font-family: georgia,serif;

The font-family here should be in a theme-specific file.

::: browser/components/preferences/tabs.css
@@ +1,1 @@
> +/*@import url("chrome://global/skin/inContentUI.css");*/

If this is commented out, can this line be deleted?

@@ +1,3 @@
> +/*@import url("chrome://global/skin/inContentUI.css");*/
> +
> +.content-prefs{

nit: space between class name and curly bracket.

@@ +1,4 @@
> +/*@import url("chrome://global/skin/inContentUI.css");*/
> +
> +.content-prefs{
> +  margin-left: 150px;

We can't use margin-left and margin-right when they have different values. Please use -moz-margin-start and -moz-margin-end instead.

@@ +3,5 @@
> +.content-prefs{
> +  margin-left: 150px;
> +  margin-top: 25px;
> +  margin-right:150px;
> +  margin-bottom:300px;

nit: Space between colon and value.

@@ +7,5 @@
> +  margin-bottom:300px;
> +  background-color: rgba(255, 255, 255, 0.35);
> +  background-image: -moz-linear-gradient(top,
> +                                         rgba(255, 255, 255, 0),
> +                                         rgba(255, 255, 255, 0.75));

nit: no spaces between color values within rgba(), for example |rgba(255,255,255,0)|.

::: browser/components/preferences/tabs.xul
@@ -1,5 @@
> -<?xml version="1.0"?>
> -
> -# -*- 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

Since the file is staying the same, we should keep the license unchanged. We could create a new file and then use MPL2 on that new file, but AFAIK this file has to keep MPL1 license until it is either deleted or has the OK from the contributors listed.

@@ +19,5 @@
> +
> +	<!-- XXX flex below is a hack because wrapping checkboxes don't reflow
> +			 properly; see bug 349098 -->
> +	<vbox id="tabPrefsBox" align="start" flex="1">
> +	  <xhtml:h1>Tabs</xhtml:h1>

This string will need to be localizable.

::: browser/locales/en-US/chrome/browser/preferences/preferences.dtd
@@ +14,4 @@
>  <!ENTITY  panePrivacy.title       "Privacy">
>  <!ENTITY  paneSecurity.title      "Security">
>  <!ENTITY  paneAdvanced.title      "Advanced">
> +<!ENTITY  search.placeholder       "Search preferences">

nit: keep the spacing consistent between the strings above it.
Attachment #597233 - Flags: feedback?(jwein) → feedback+
Comment on attachment 597233 [details] [diff] [review]
Includes "Landing" page, general, tabs, and privacy

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

::: browser/components/preferences/jar.mn
@@ +21,5 @@
>  *   content/browser/preferences/connection.js
>  *   content/browser/preferences/fonts.xul
>  *   content/browser/preferences/fonts.js
> +*   content/browser/preferences/general.xul
> +*   content/browser/preferences/general.css

There is no general.css file found in this patch, so the creation of the jar file fails. Was this file supposed to be |hg add|'ed or should this line be removed?
As another follow-up, I think we should be doing this work in new files so as not to break the current preferences implementation. This will allow us to enable/disable the new preferences feature using an about:config flag.

Applying the patch and opening the current preferences window is currently broken since the same files are being edited.
Sorry, yes that line should be removed.
(In reply to Jared Wein [:jaws] from comment #8)
> ::: browser/components/preferences/main.js
> @@ +145,5 @@
> >    setHomePageToBookmark: function ()
> >    {
> >      var rv = { urls: null, names: null };
> > +    openDialog("chrome://browser/content/preferences/selectBookmark.xul",
> > +    	    "Select Bookmark", "resizable=yes, modal=yes", rv);
> 
> This will need to use a string from a DTD here.

No - that argument is the name (or ID) of the dialog, not a user-facing string (so shouldn't be localized). It needs to be unique to that dialog. The convention is to use "namespace:dialogname" - so use something like "Preferences:SelectBookmark".
Comment on attachment 597233 [details] [diff] [review]
Includes "Landing" page, general, tabs, and privacy

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

::: browser/components/preferences/aboutPreferences.xul
@@ +90,5 @@
> +#define USE_WIN_TITLE_STYLE
> +#endif
> +
> +<page class="center"
> +      ondialoghelp="openPrefsHelp()"

Remove the ondialoghelp attribute - it won't work here.

@@ +95,5 @@
> +#ifdef USE_WIN_TITLE_STYLE
> +            title="&prefWindow.titleWin;"
> +#else
> +#ifdef XP_UNIX
> +#ifndef XP_MACOSX

Remove the "#ifndef XP_MACOSX" (and related #endif). Options dialogs on OSX may not have a title, but tabs sure do.

@@ +107,5 @@
> +#else
> +#ifdef XP_MACOSX
> +            style="&prefWinMinSize.styleMac;">
> +#else
> +            style="&prefWinMinSize.styleGNOME;">

Remove all these styles using the prefWinMinSize entities - they're to ensure a minimum window size on various platforms, which isn't relevant for an in-content UI.

@@ +115,5 @@
> +<toolbar>
> +<textbox id="header-search" type="search" searchbutton="true"
> +             placeholder="&search.placeholder;"/>
> +</toolbar>
> +    <container class="main-content" id="pref-box" felx="1">

felx?

Also, container isn't a standard element. Whenever you hear anyone say "container element", they mean an element that acts as a container for other elements - like <hbox> or <vbox>.
See https://developer.mozilla.org/en/XUL/vbox and https://developer.mozilla.org/en/XUL/hbox
Both are really just <box> elements with a specific orientation, but we prefer not to use <box> directly - better to be explicit about what orientation you want.

@@ +119,5 @@
> +    <container class="main-content" id="pref-box" felx="1">
> +        <prefpane
> +              xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
> +              xmlns:html="http://www.w3.org/1999/xhtml"
> +              xmlns:xhtml="http://www.w3.org/1999/xhtml"

These namespaces should only be on the <page> element. Also, be consistent with namespace names - don't use both html and xhtml (remove xhtml).

@@ +120,5 @@
> +        <prefpane
> +              xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
> +              xmlns:html="http://www.w3.org/1999/xhtml"
> +              xmlns:xhtml="http://www.w3.org/1999/xhtml"
> +              onpaneload="gPrivacyPane.init();">

This should be in the onload handler for the <page> element.

@@ +127,5 @@
> +#include tabs.xul
> +#include general.xul
> +        </prefpane>
> +    </container>
> +#ifdef MOZ_SERVICES_SYNC

I assume this is a to-do item? You'll want to put the sync options in this #ifdef (and move it to inside the <prefpane>) once you convert that pane.

@@ +131,5 @@
> +#ifdef MOZ_SERVICES_SYNC
> +#endif
> +
> +#ifdef XP_MACOSX
> +#include ../../base/content/browserMountPoints.inc

This can be removed - it's only used for windows/dialogs (it makes sure the main menu works as expected on OSX), and isn't relevant to an in-content UI.

::: browser/components/preferences/aboutPrivacy.xul
@@ +44,5 @@
> +      <image class="pref-icon" type="privacy"/>
> +      <p>&header.label;</p>
> +  </header>
> +    <script type="application/javascript" src="chrome://browser/content/utilityOverlay.js"/>
> +    <script type="application/javascript" src="chrome://browser/content/privacy.js"/>

I don't see this in the original privacy pane... and for that matter, that file doesn't even exist.

::: browser/components/preferences/privacy.css
@@ +76,5 @@
> +  background-position: -128px 0;
> +}
> +.security-img {
> +  background-position: -160px 0;
> +}.advanced-img {

Missing newline here.

@@ +81,5 @@
> +  background-position: -192px 0;
> +}
> +
> +.heading {
> +  font-family: georgia,serif;

*Most* of the CSS here should be in a theme-specific file.
Attachment #597233 - Flags: feedback?(bmcbride) → feedback+
I added a new directory with all the files for the preferences. I haven't been able to check and make sure everything is working 100% for all the pages, but I wanted to get feedback first and make sure this was the right idea.
Attachment #599654 - Flags: feedback?(jwein)
Attachment #599654 - Flags: feedback?(bmcbride)
Comment on attachment 599654 [details] [diff] [review]
Includes new directory, landing, tabs, privacy, general, advanced

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

::: browser/components/Makefile.in
@@ +65,4 @@
>    feeds \
>    places \
>    preferences \
> +  incontentPreferences \

drive-by feedback: the in-content folder should be a child of the preferences folder, not a sibling. as a child folder, it can be named just "in-content" since it is implied that it is part of preferences.
Comment on attachment 599654 [details] [diff] [review]
Includes new directory, landing, tabs, privacy, general, advanced

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

This is in the right direction, but let's move the in-content preferences to a subfolder of the preferences folder. Also, see if you can only copy the files that we'll change (for example, we probably won't need to move any of the about:permissions files).

::: browser/components/incontentPreferences/aboutPermissions.css
@@ +1,2 @@
> +.site {
> +  -moz-binding: url("chrome://browser/content/preferences/aboutPermissions.xml#site");

I don't think this file needs to be copied to the new directory. We probably only need to copy the preference panes and related tests. Pages such as about:permissions are already in-content, so this is a little confusing.

::: browser/components/incontentPreferences/advanced.xul
@@ +1,2 @@
> +# In-Content "Advanced" tab
> +# Created by Michigan State University Capstone Team

This needs a proper license header.

::: browser/components/incontentPreferences/landing.xul
@@ +34,5 @@
> +   -
> +   - ***** END LICENSE BLOCK ***** -->
> +
> +<vbox id="landing-content">
> +  <xhtml:h1>Firefox</xhtml:h1>

This text should be moved to a DTD file.

@@ +76,5 @@
> +  </hbox>
> +
> +
> +  <xhtml:h1>Add-ons</xhtml:h1>
> +  <hbox id="addon-home">

Let's remove the Add-ons section for now.

::: browser/components/incontentPreferences/preferences.xul
@@ +1,3 @@
> +<?xml version="1.0"?>
> +<!-- ***** BEGIN LICENSE BLOCK *****
> +   - Version: MPL 1.1/GPL 2.0/LGPL 2.1

Since this file is new, let's use the MPL2 license here.

::: browser/components/incontentPreferences/tests/Makefile.in
@@ +37,5 @@
> +DEPTH		= ../../../..
> +topsrcdir	= @top_srcdir@
> +srcdir		= @srcdir@
> +VPATH		= @srcdir@
> +relativesrcdir  = browser/components/preferences/tests

This should reference the in-content path.

::: toolkit/themes/winstripe/global/preferences.css
@@ +51,3 @@
>    padding-bottom: 10px;
> +  min-width: 800px;
> +  max-width: 800px;

What are these for? How will this work on screens that are 640x480?
Attachment #599654 - Flags: feedback?(jwein) → feedback+
Comment on attachment 599654 [details] [diff] [review]
Includes new directory, landing, tabs, privacy, general, advanced

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

Some general notes:
* Reviewing a 540kb patch is not the most fun experience, really need to have this as separate patches in their respective bugs.
* Going forward, I'd be great if you could summarize changes since the last patch revision.
* Observing the style guide while you're developing, rather than after, will save all of us a lot of time (https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide). Line lengths, indentation, consistency, etc. Long lines particularly make it harder to review, as it's difficult to see what's changed.
* I assume the dialogs haven't changed, so I didn't go over them. Alas, copying everything into a new directory makes that harder for us to review, but we'll have to live with it (having you summarize changes will help there).
* Landing page looks good :)

::: browser/components/incontentPreferences/home.js
@@ +1,1 @@
> +var currentPageState;

Do you plan on using this for anything? If so, it should only be updated in gotoPref(), as it's inaccurate at the moment.

Style nit: Global variables like this should start with a 'g'. eg, in this case: gCurrentPageState

@@ +6,5 @@
> +    document.getElementById(page).hidden = false;
> +    document.getElementById("back-btn").disabled = false;
> +}
> +
> +function setHistory() {

This should be named initialize() or something similar, and handle all initialization tasks such as calling gPrivacyPane.init etc (rather than having them in the onload attribute, which will just get too long).

@@ +12,5 @@
> +}
> +
> +function cmd_back() {
> +    currentPageState = window.history.state;
> +    document.getElementById("forward-btn").disabled = false;

Back/forward should be enabled based on the canGoBack and canGoForward properties of nsIWebNaviation.
See https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions.js#189

Both commands needs to be updated any time the view changes (which should only happen in gotoPref).

@@ +22,5 @@
> +    currentPageState = window.history.state;
> +    window.history.forward();
> +}
> +
> +window.onpopstate = function(event) {

addEventListener is the proper way of adding an event handler in JS.

This should really just be calling gotoPref()

::: browser/components/incontentPreferences/incontentPreferences.css
@@ +9,5 @@
> +
> +}
> +
> +#header {
> +    position: fixed;

Don't use position:fixed; for this, it causes all sorts of issues with the layout. You'll also need to remove align="center from this, and remove class="center" from <page>. That will make main-content take up more space than you want, so wrap that element in a vbox, and apply class="center" to that. I also want to tweak main-content, but lets fix this issue first :)

::: browser/components/incontentPreferences/landing.xul
@@ +13,5 @@
> +   - The Original Code is the Extension Manager UI.
> +   -
> +   - The Initial Developer of the Original Code is
> +   - the Mozilla Foundation.
> +   - Portions created by the Initial Developer are Copyright (C) 2010

How to tell someone copied the license from another file...

Also, this should be MPL2.

@@ +34,5 @@
> +   -
> +   - ***** END LICENSE BLOCK ***** -->
> +
> +<vbox id="landing-content">
> +  <xhtml:h1>Firefox</xhtml:h1>

If we're using the application name ("Firefox") it should just use &brandShortName; (which is in brand.dtd, already used in preferences.xul)

@@ +37,5 @@
> +<vbox id="landing-content">
> +  <xhtml:h1>Firefox</xhtml:h1>
> +  <hbox id="preferences-home">
> +
> +    <vbox class="options-box" onclick="gotoPref('general-content')">

For accessibility reasons, these need to be buttons. That allows them to be keyboard navigated, and exposed correctly to screen readers. 

Similarly, use oncommand="whatever" instead of onclick="whatever", so they're not restricted to just mouse clicks.

Need to make it so these icons wrap onto two lines if the screen isn't wide enough to fit them in (imagine if we eventually add more categories). There's a trick to achieving that:
* preferences-home and all it's parents need flex="1" added
* preferences-home needs display:block; set in CSS

@@ +38,5 @@
> +  <xhtml:h1>Firefox</xhtml:h1>
> +  <hbox id="preferences-home">
> +
> +    <vbox class="options-box" onclick="gotoPref('general-content')">
> +      <image class="options-icon general-img" id="paneGeneral" type="general"/>

Do you need both the general-img class and the type attribute?

::: browser/components/incontentPreferences/preferences.xul
@@ +50,5 @@
> +   -->
> +<?xml-stylesheet href="chrome://browser/content/incontentPreferences/handlers.css"?>
> +<?xml-stylesheet href="chrome://browser/skin/preferences/applications.css"?>
> +
> +<!DOCTYPE prefwindow [

This should be <!DOCTYPE page [
(The root element is a <page>, not <prefwindow>)

@@ +94,5 @@
> +                gAdvancedPane.init();
> +                gApplicationsPane.init();"
> +        xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
> +        xmlns:html="http://www.w3.org/1999/xhtml"
> +        xmlns:xhtml="http://www.w3.org/1999/xhtml"

xmlns:xhtml still needs removed.

@@ +103,5 @@
> +#endif
> +
> +<hbox id="header" align="center">
> +    <toolbarbutton id="back-btn" class="nav-button header-button" onclick="cmd_back()" tooltiptext="Go back one page" disabled="true"/>
> +    <toolbarbutton id="forward-btn" class="nav-button header-button" onclick="cmd_forward()" tooltiptext="Go forward one page" disabled="true"/>

Use oncommand="whatever" instead of onclick="whatever", so it can be keyboard accessible.

Also, wrap long lines to 80 characters, as per the style guide (https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide).

@@ +106,5 @@
> +    <toolbarbutton id="back-btn" class="nav-button header-button" onclick="cmd_back()" tooltiptext="Go back one page" disabled="true"/>
> +    <toolbarbutton id="forward-btn" class="nav-button header-button" onclick="cmd_forward()" tooltiptext="Go forward one page" disabled="true"/>
> +    <spacer flex="1"/>
> +    <textbox id="header-search" type="search" searchbutton="true" placeholder="&search.placeholder;"/>
> +</hbox>

There's some messed up indentation to fix here.

@@ +107,5 @@
> +    <toolbarbutton id="forward-btn" class="nav-button header-button" onclick="cmd_forward()" tooltiptext="Go forward one page" disabled="true"/>
> +    <spacer flex="1"/>
> +    <textbox id="header-search" type="search" searchbutton="true" placeholder="&search.placeholder;"/>
> +</hbox>
> +    <hbox class="main-content" id="pref-box">

id="pref-box" is a lie. I don't think it needs an ID, just use .main-content in the CSS file if you need to style it.

@@ +133,5 @@
> +
> +        <stringbundleset id="appManagerBundleset">
> +        <stringbundle id="appManagerBundle"
> +                    src="chrome://browser/locale/preferences/applicationManager.properties"/>
> +        </stringbundleset>

Scripts and stringbundles should be the first children of the root element (ie, <page>), so they're easy to find.

::: browser/components/incontentPreferences/tabs.xul
@@ +1,4 @@
> +<hbox flex="1" id="tabs-content" hidden="true"
> +#ifdef XP_WIN
> +
> +#endif

Eh?

::: browser/themes/pinstripe/preferences/preferences.css
@@ +38,4 @@
>  #
>  # ***** END LICENSE BLOCK *****
>  */
> +@import url("chrome://global/skin/inContentUI.css");

Style nit: Add a blank line above and below this (applies to all cases of this).
Attachment #599654 - Flags: feedback?(bmcbride) → feedback+
Forgot to mention, I saw the following error:

Error: pref is null
Source file: chrome://browser/content/incontentPreferences/privacy.js
Line: 78
Attached patch in-content privacy pane (obsolete) — Splinter Review
Patch is now only privacy (no landing, general, tabs, advanced). I fixed the error that Blair mentioned in his feedback (JavaScript error).
Attachment #603380 - Flags: feedback?(jwein)
Attachment #603380 - Flags: feedback?(bmcbride)
Attachment #593900 - Attachment is obsolete: true
Attachment #597233 - Attachment is obsolete: true
Attachment #599654 - Attachment is obsolete: true
Comment on attachment 603380 [details] [diff] [review]
in-content privacy pane

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

::: browser/components/preferences/in-content/privacy.xul
@@ +96,5 @@
> +                  type="bool"/>
> +
> +    </preferences>
> +
> +<vbox id="privacy-content" hidden="true">

We should remove this <vbox> and place a <vbox> around each preference as part of the search implementation.

::: browser/locales/en-US/chrome/browser/preferences/privacy.dtd
@@ +1,1 @@
> +<!ENTITY header.label                   "Privacy">

Do we need to add this here? Can we just use http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/preferences/preferences.dtd#14 instead?
Attachment #603380 - Flags: feedback?(jwein) → feedback+
Attached patch privacy in-content pane (obsolete) — Splinter Review
Fixed all issues addressed in Jared's feedback. I did not add the vbox's around the preference elements since I figured it would be better for Owen to do that in his patch since I don't know how he is going to want to wrap those.
Attachment #605535 - Flags: feedback?(jwein)
Attachment #605535 - Flags: feedback?(bmcbride)
Attachment #603380 - Attachment is obsolete: true
Attachment #603380 - Flags: feedback?(bmcbride)
Comment on attachment 605535 [details] [diff] [review]
privacy in-content pane

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

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

Please remove the empty line at the beginning of the file and this above line too since it's incorrect anyways :)

@@ +38,5 @@
> +# the provisions above, a recipient may use your version of this file under
> +# the terms of any one of the MPL, the GPL or the LGPL.
> +#
> +# ***** END LICENSE BLOCK *****
> +*/

This should be MPL2.

::: browser/components/preferences/in-content/privacy.xul
@@ +36,5 @@
> +# and other provisions required by the GPL or the LGPL. If you do not delete
> +# the provisions above, a recipient may use your version of this file under
> +# the terms of any one of the MPL, the GPL or the LGPL.
> +#
> +# ***** END LICENSE BLOCK *****

Same feedback for this file too.

@@ +121,5 @@
> +            accesskey="&historyHeader.pre.accesskey;">&historyHeader.pre.label;</label>
> +    <menulist id="historyMode"
> +                oncommand="gPrivacyPane.updateHistoryModePane();
> +                            gPrivacyPane.updateHistoryModePrefs();
> +                            gPrivacyPane.updatePrivacyMicroControls();">

Please go through this file and fix up whitespace issues so it will be easier to read/review/edit.
Attachment #605535 - Flags: feedback?(jwein) → feedback+
Attached patch privacy in-content pane (obsolete) — Splinter Review
all feedback from Jared has been fixed.
Attachment #605535 - Attachment is obsolete: true
Attachment #605535 - Flags: feedback?(bmcbride)
Attachment #608481 - Flags: feedback?(jwein)
Attachment #608481 - Flags: feedback?(bmcbride)
Comment on attachment 608481 [details] [diff] [review]
privacy in-content pane

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

Other than the two nits below, this looks fine to me.

::: browser/components/preferences/in-content/jar.mn
@@ +6,5 @@
>  *  content/browser/preferences/in-content/main.js
>  *   content/browser/preferences/in-content/tabs.xul
>  *   content/browser/preferences/in-content/tabs.js
> +*   content/browser/preferences/in-content/privacy.xul
> +*   content/browser/preferences/in-content/privacy.js

An extra space got added to this patch too. Probably from just trying to be consistent with the two lines above them.

::: browser/components/preferences/in-content/privacy.js
@@ +411,5 @@
> +   */
> +  showCookies: function (aCategory)
> +  {
> +    openDialog("chrome://browser/content/incontentPreferences/cookies.xul",
> +                        "model=yes", null);

Can you fix the whitespace indentation here and for the other openDialog calls in this file?
Attachment #608481 - Flags: feedback?(jwein) → feedback+
Attached patch privacy in-content pane (obsolete) — Splinter Review
fixed nits
Attachment #608481 - Attachment is obsolete: true
Attachment #608481 - Flags: feedback?(bmcbride)
Attachment #608507 - Flags: feedback?(jwein)
Attachment #608507 - Flags: feedback?(bmcbride)
Attachment #608507 - Flags: feedback?(jwein) → feedback+
Attached patch privacy in-content pane (obsolete) — Splinter Review
added consistent title
Attachment #608507 - Attachment is obsolete: true
Attachment #608507 - Flags: feedback?(bmcbride)
Attachment #609058 - Flags: feedback?(jwein)
Attachment #609058 - Flags: feedback?(bmcbride)
Attachment #609058 - Attachment is patch: true
Comment on attachment 609058 [details] [diff] [review]
privacy in-content pane

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

::: browser/components/preferences/in-content/jar.mn
@@ +6,5 @@
>  *  content/browser/preferences/in-content/main.js
>  *  content/browser/preferences/in-content/tabs.xul
>  *  content/browser/preferences/in-content/tabs.js
> +*  content/browser/preferences/in-content/privacy.xul
> +*  content/browser/preferences/in-content/privacy.js

Neither of these files need to be run through the text preprocessor (neither contain any #ifdefs, etc) - remove the *.

::: browser/components/preferences/in-content/privacy.js
@@ +1,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/. */
> +
> +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");

XPCOMUtils is a generic module that's used everywhere (I remember at least one other of these patches also loading it). As such, move this to preferences.js in the patch in bug 733473.

::: browser/components/preferences/in-content/privacy.xul
@@ +2,5 @@
> +   - 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/.  -->
> +
> +<script type="application/javascript"
> +  src="chrome://browser/content/preferences/in-content/privacy.js"/>

Indent the wrapped line correctly.

@@ +58,5 @@
> +              instantApply="true"
> +              type="bool"/>
> +
> +</preferences>
> +<header class="heading">

<header> isn't a real XUL element - just use <hbox>. Unless you meant to use the HTML element, in which case <html:header>
(This applies to all patches where you've added this.)
Attachment #609058 - Flags: feedback?(bmcbride) → feedback+
Comment on attachment 609058 [details] [diff] [review]
privacy in-content pane

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

::: browser/components/preferences/in-content/privacy.xul
@@ +60,5 @@
> +
> +</preferences>
> +<header class="heading">
> +  <image class="preference-icon indent-small" type="privacy"/>
> +  <html:h1 class="indent-small">&panePrivacy.title;</html:h1>

Had a think about this - the use of the indent-small class really bugs me. Better to remove it's usage, and add a margin to the preference-icon class instead.

Also, add a blank line above and below this block - will make it easier to read.
Attached patch privacy in-content pane (obsolete) — Splinter Review
fixed feedback, included search vbox's, and fixed title
Attachment #609058 - Attachment is obsolete: true
Attachment #609058 - Flags: feedback?(jwein)
Attachment #609219 - Flags: feedback?(jwein)
Attachment #609219 - Flags: feedback?(bmcbride)
Comment on attachment 609219 [details] [diff] [review]
privacy in-content pane

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

::: browser/components/preferences/in-content/privacy.xul
@@ +38,5 @@
> +              type="bool"/>
> +  <!-- Cookies -->
> +  <preference id="network.cookie.cookieBehavior"      name="network.cookie.cookieBehavior"      type="int"/>
> +  <preference id="network.cookie.lifetimePolicy"      name="network.cookie.lifetimePolicy"      type="int"/>
> +  <preference id="network.cookie.blockFutureCookies"  name="network.cookie.blockFutureCookies"  type="bool"/>

nit: please fix this indentation
Attachment #609219 - Flags: feedback?(jwein) → feedback+
Attached patch privacy in-content patch (obsolete) — Splinter Review
fixed nits and added class for changing cursor when hovering over link
Attachment #609219 - Attachment is obsolete: true
Attachment #609219 - Flags: feedback?(bmcbride)
Attachment #610390 - Flags: feedback?(jwein)
Attachment #610390 - Flags: feedback?(bmcbride)
Attachment #610390 - Flags: feedback?(jwein) → feedback+
Attached patch privacy 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 #610390 - Attachment is obsolete: true
Attachment #610390 - Flags: feedback?(bmcbride)
Attachment #613229 - Flags: feedback?(jwein)
Attachment #613229 - Flags: feedback?(bmcbride)
Attachment #613229 - Flags: feedback?(jwein) → feedback+
Attachment #613229 - Attachment is obsolete: true
Attachment #613229 - Flags: feedback?(bmcbride)
Attachment #614500 - Flags: feedback?(jwein)
Attachment #614500 - Flags: feedback?(bmcbride)
Attachment #614500 - Attachment is obsolete: true
Attachment #614500 - Flags: feedback?(jwein)
Attachment #614500 - Flags: feedback?(bmcbride)
Attachment #614532 - Flags: feedback?(jwein)
Attachment #614532 - Flags: feedback?(bmcbride)
Attachment #614532 - Flags: feedback?(jwein) → feedback+
Attachment #613674 - Attachment is obsolete: true
Comment on attachment 614532 [details] [diff] [review]
privacy in-content patch for new landing patch

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

::: browser/components/preferences/in-content/privacy.xul
@@ +139,5 @@
> +                label="&privateBrowsingPermanent2.label;"
> +                accesskey="&privateBrowsingPermanent2.accesskey;"
> +                preference="browser.privatebrowsing.autostart"/>
> +
> +      <vbox class="indent">

This was originally 2 nested <vbox>'s, both with class="indent". With only one, the position of the checkboxes isn't correct.
See https://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/privacy.xul#191

However, that original code wasn't the nicest. Better to wrap both <vbox> and the <checkbox> above it in another <vbox class="indent"/>. And remove the class="indent" from the <checkbox>.
Attachment #614532 - Flags: feedback?(bmcbride) → feedback+
Attached patch patch for testing (obsolete) — Splinter Review
Attached patch privacy in-content patch (obsolete) — Splinter Review
updated vboxs and removed checkbox indent class to make checkboxes line up correctly
Attachment #614532 - Attachment is obsolete: true
Attachment #616398 - Attachment is obsolete: true
Attachment #616996 - Flags: feedback?(bmcbride)
Attachment #616996 - Flags: review+
Comment on attachment 616996 [details] [diff] [review]
privacy in-content patch

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

::: browser/components/preferences/in-content/privacy.xul
@@ +141,5 @@
> +                  accesskey="&privateBrowsingPermanent2.accesskey;"
> +                  preference="browser.privatebrowsing.autostart"/>
> +
> +        <vbox class="indent">
> +          <vbox class="indent">

In comment 36, I meant the original code had two of these, but that you shouldn't do that to fix the indentation here. Now the checkboxes are double indented. Everything else is fine now, just need to remove one of these <vbox>s.
Attachment #616996 - Flags: feedback?(bmcbride) → review-
removed one indent vbox and fixed indentation
Attachment #616996 - Attachment is obsolete: true
Attachment #621631 - Flags: feedback?(bmcbride)
Attachment #621631 - Flags: feedback?(bmcbride) → review+
https://hg.mozilla.org/mozilla-central/rev/d849b7493ea2
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]
Depends on: 949589
You need to log in before you can comment on or make changes to this bug.