Last Comment Bug 723328 - Move the privacy preferences to in-content UI
: Move the privacy preferences to in-content UI
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: 949589
Blocks: 718011
  Show dependency treegraph
 
Reported: 2012-02-01 15:32 PST by (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back)
Modified: 2013-12-12 11:06 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
in-content privacy (14.88 KB, patch)
2012-02-02 10:46 PST, Jon Rietveld
bmcbride: feedback+
Details | Diff | Review
Includes "Landing" page, general, tabs, and privacy (56.19 KB, patch)
2012-02-14 16:19 PST, Jon Rietveld
jaws: feedback+
bmcbride: feedback+
Details | Diff | Review
Includes new directory, landing, tabs, privacy, general, advanced (537.96 KB, patch)
2012-02-22 09:29 PST, Jon Rietveld
jaws: feedback+
bmcbride: feedback+
Details | Diff | Review
in-content privacy pane (32.84 KB, patch)
2012-03-06 11:28 PST, Jon Rietveld
jaws: feedback+
Details | Diff | Review
privacy in-content pane (31.78 KB, patch)
2012-03-13 14:12 PDT, Jon Rietveld
jaws: feedback+
Details | Diff | Review
privacy in-content pane (30.19 KB, patch)
2012-03-22 14:42 PDT, Jon Rietveld
jaws: feedback+
Details | Diff | Review
privacy in-content pane (30.16 KB, patch)
2012-03-22 15:41 PDT, Jon Rietveld
jaws: feedback+
Details | Diff | Review
privacy in-content pane (27.30 KB, patch)
2012-03-24 20:05 PDT, Jon Rietveld
bmcbride: feedback+
Details | Diff | Review
privacy in-content pane (27.70 KB, patch)
2012-03-25 23:02 PDT, Jon Rietveld
jaws: feedback+
Details | Diff | Review
privacy in-content patch (27.82 KB, patch)
2012-03-28 18:36 PDT, Jon Rietveld
jaws: feedback+
Details | Diff | Review
privacy in-content patch (26.14 KB, patch)
2012-04-08 22:49 PDT, Jon Rietveld
jaws: feedback+
Details | Diff | Review
Privacy with updated data-category (26.16 KB, patch)
2012-04-10 11:05 PDT, MSU Capstone Team
no flags Details | Diff | Review
privacy in-content patch for new landing patch (26.14 KB, patch)
2012-04-12 12:37 PDT, Jon Rietveld
no flags Details | Diff | Review
privacy in-content patch for new landing patch (26.24 KB, patch)
2012-04-12 13:18 PDT, Jon Rietveld
jaws: feedback+
bmcbride: feedback+
Details | Diff | Review
patch for testing (26.53 KB, patch)
2012-04-18 18:12 PDT, Jon Rietveld
no flags Details | Diff | Review
privacy in-content patch (26.53 KB, patch)
2012-04-20 09:24 PDT, Jon Rietveld
jaws: review+
bmcbride: review-
Details | Diff | Review
privacy in-content patch (26.38 KB, patch)
2012-05-07 09:29 PDT, Jon Rietveld
bmcbride: review+
Details | Diff | Review

Description (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-02-01 15:32:15 PST

    
Comment 1 Jon Rietveld 2012-02-02 10:46:37 PST
Created attachment 593900 [details] [diff] [review]
in-content privacy

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!
Comment 2 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-02-02 15:23:53 PST
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 3 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-02-03 03:17:25 PST
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.
Comment 4 Jon Rietveld 2012-02-07 15:25:35 PST
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
Comment 5 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-02-07 16:42:08 PST
(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 6 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-02-09 02:31:55 PST
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.
Comment 7 Jon Rietveld 2012-02-14 16:19:25 PST
Created attachment 597233 [details] [diff] [review]
Includes "Landing" page, general, tabs, and privacy
Comment 8 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-02-14 16:46:20 PST
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.
Comment 9 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-02-14 17:01:00 PST
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?
Comment 10 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-02-14 17:03:43 PST
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.
Comment 11 Jon Rietveld 2012-02-14 17:05:15 PST
Sorry, yes that line should be removed.
Comment 12 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-02-14 17:28:20 PST
(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 13 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-02-19 21:17:07 PST
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.
Comment 14 Jon Rietveld 2012-02-22 09:29:23 PST
Created attachment 599654 [details] [diff] [review]
Includes new directory, landing, tabs, privacy, general, advanced

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.
Comment 15 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-02-22 21:33:34 PST
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 16 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-02-23 22:10:39 PST
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?
Comment 17 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-02-27 22:50:10 PST
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).
Comment 18 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-02-28 02:14:22 PST
Forgot to mention, I saw the following error:

Error: pref is null
Source file: chrome://browser/content/incontentPreferences/privacy.js
Line: 78
Comment 19 Jon Rietveld 2012-03-06 11:28:10 PST
Created attachment 603380 [details] [diff] [review]
in-content privacy pane

Patch is now only privacy (no landing, general, tabs, advanced). I fixed the error that Blair mentioned in his feedback (JavaScript error).
Comment 20 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-03-09 14:00:50 PST
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?
Comment 21 Jon Rietveld 2012-03-13 14:12:59 PDT
Created attachment 605535 [details] [diff] [review]
privacy in-content pane

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.
Comment 22 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-03-22 11:05:17 PDT
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.
Comment 23 Jon Rietveld 2012-03-22 14:42:13 PDT
Created attachment 608481 [details] [diff] [review]
privacy in-content pane

all feedback from Jared has been fixed.
Comment 24 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-03-22 15:02:51 PDT
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?
Comment 25 Jon Rietveld 2012-03-22 15:41:57 PDT
Created attachment 608507 [details] [diff] [review]
privacy in-content pane

fixed nits
Comment 26 Jon Rietveld 2012-03-24 20:05:01 PDT
Created attachment 609058 [details] [diff] [review]
privacy in-content pane

added consistent title
Comment 27 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-03-25 04:40:40 PDT
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.)
Comment 28 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-03-25 05:35:42 PDT
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.
Comment 29 Jon Rietveld 2012-03-25 23:02:30 PDT
Created attachment 609219 [details] [diff] [review]
privacy in-content pane

fixed feedback, included search vbox's, and fixed title
Comment 30 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-03-27 11:37:18 PDT
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
Comment 31 Jon Rietveld 2012-03-28 18:36:27 PDT
Created attachment 610390 [details] [diff] [review]
privacy in-content patch

fixed nits and added class for changing cursor when hovering over link
Comment 32 Jon Rietveld 2012-04-08 22:49:40 PDT
Created attachment 613229 [details] [diff] [review]
privacy in-content patch

removed vbox from heading, and removed vbox around 1st child elements and added data-category and hidden to 1st child element instead
Comment 33 MSU Capstone Team 2012-04-10 11:05:16 PDT
Created attachment 613674 [details] [diff] [review]
Privacy with updated data-category
Comment 34 Jon Rietveld 2012-04-12 12:37:56 PDT
Created attachment 614500 [details] [diff] [review]
privacy in-content patch for new landing patch
Comment 35 Jon Rietveld 2012-04-12 13:18:35 PDT
Created attachment 614532 [details] [diff] [review]
privacy in-content patch for new landing patch
Comment 36 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-04-15 22:22:18 PDT
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>.
Comment 37 Jon Rietveld 2012-04-18 18:12:19 PDT
Created attachment 616398 [details] [diff] [review]
patch for testing
Comment 38 Jon Rietveld 2012-04-20 09:24:12 PDT
Created attachment 616996 [details] [diff] [review]
privacy in-content patch

updated vboxs and removed checkbox indent class to make checkboxes line up correctly
Comment 39 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-05-06 22:39:54 PDT
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.
Comment 40 Jon Rietveld 2012-05-07 09:29:34 PDT
Created attachment 621631 [details] [diff] [review]
privacy in-content patch

removed one indent vbox and fixed indentation
Comment 41 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-05-08 22:39:13 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/d849b7493ea2
Comment 42 Ed Morley [:emorley] 2012-05-09 07:42:41 PDT
https://hg.mozilla.org/mozilla-central/rev/d849b7493ea2
Comment 43 Alfredos-Panagiotis Damkalis [:fredy] 2012-06-22 04:46:11 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.