Last Comment Bug 735091 - Sync pane for In-Content preferences
: Sync 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: Devan Sayles
:
:
Mentors:
: 735092 (view as bug list)
Depends on: 754342
Blocks: 718011
  Show dependency treegraph
 
Reported: 2012-03-12 16:00 PDT by Devan Sayles
Modified: 2012-06-22 04:25 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
first patch for sync pane (18.57 KB, patch)
2012-03-12 16:18 PDT, Devan Sayles
jaws: feedback+
Details | Diff | Splinter Review
updated Sync patch based on Jared's feedback (15.47 KB, patch)
2012-03-13 17:51 PDT, Devan Sayles
jaws: feedback+
Details | Diff | Splinter Review
sync in-content pane (14.11 KB, patch)
2012-03-22 16:10 PDT, Jon Rietveld
no flags Details | Diff | Splinter Review
sync in-content pane (14.22 KB, patch)
2012-03-24 20:12 PDT, Jon Rietveld
blair: feedback+
Details | Diff | Splinter Review
sync in-content pane (13.78 KB, patch)
2012-03-25 23:10 PDT, Jon Rietveld
jaws: feedback+
Details | Diff | Splinter Review
Sync pane with inner vboxs removed (13.68 KB, patch)
2012-03-27 17:35 PDT, MSU Capstone Team
jaws: feedback+
blair: feedback+
Details | Diff | Splinter Review
sync in-content patch (13.44 KB, patch)
2012-04-08 22:53 PDT, Jon Rietveld
jaws: feedback+
Details | Diff | Splinter Review
Sync with updated data-category (13.45 KB, patch)
2012-04-10 11:09 PDT, MSU Capstone Team
no flags Details | Diff | Splinter Review
sync in-content patch for new landing patch (13.46 KB, patch)
2012-04-12 12:40 PDT, Jon Rietveld
no flags Details | Diff | Splinter Review
sync in-content patch for new landing patch (13.56 KB, patch)
2012-04-12 13:20 PDT, Jon Rietveld
jaws: feedback+
Details | Diff | Splinter Review
sync in-content patch (13.56 KB, patch)
2012-04-20 09:28 PDT, Jon Rietveld
jaws: review+
blair: review+
Details | Diff | Splinter Review

Description Devan Sayles 2012-03-12 16:00:58 PDT
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_2) AppleWebKit/535.11 (KHTML, like Gecko) Chrome/17.0.963.56 Safari/535.11




Expected results:

new sync pane for in-content preferences
Comment 1 Tanner Filip [:tanner] 2012-03-12 16:08:31 PDT
*** Bug 735092 has been marked as a duplicate of this bug. ***
Comment 2 Devan Sayles 2012-03-12 16:18:03 PDT
Created attachment 605210 [details] [diff] [review]
first patch for sync pane

sync patch to be applied on top of jon's newly separated landing patch.

When sync is off, the two links shown on the preferences pane bring up dialog boxes, which seem to work ok. What is the best way to test when sync is on - should I create an account to test with, or use another method?
Comment 3 Jared Wein [:jaws] (please needinfo? me) 2012-03-13 00:48:44 PDT
Comment on attachment 605210 [details] [diff] [review]
first patch for sync pane

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

::: browser/components/preferences/in-content/sync.js
@@ +1,2 @@
> +/* ***** BEGIN LICENSE BLOCK *****
> + * Version: MPL 1.1/GPL 2.0/LGPL 2.1

MPL 2.0 on new files please.

::: browser/components/preferences/in-content/sync.xul
@@ +52,5 @@
> +            src="chrome://browser/content/preferences/in-content/sync.js"/>
> +    <script type="application/javascript"
> +            src="chrome://browser/content/sync/utils.js"/>
> +
> +    <vbox id="sync-content" hidden="true">

Please remove this <vbox> and replace it with <vbox>s around each preference.

::: browser/locales/en-US/chrome/browser/preferences/sync.dtd
@@ +41,5 @@
>  <!-- Footer stuff -->
>  <!ENTITY prefs.tosLink.label        "Terms of Service">
>  <!ENTITY prefs.ppLink.label         "Privacy Policy">
> +
> +<!ENTITY syncHeader.label           "Sync">

Please use https://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.dtd#598 instead of adding it here.
Comment 4 Jared Wein [:jaws] (please needinfo? me) 2012-03-13 17:37:17 PDT
Comment on attachment 605210 [details] [diff] [review]
first patch for sync pane

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

::: browser/locales/en-US/chrome/browser/preferences/sync.dtd
@@ +41,5 @@
>  <!-- Footer stuff -->
>  <!ENTITY prefs.tosLink.label        "Terms of Service">
>  <!ENTITY prefs.ppLink.label         "Privacy Policy">
> +
> +<!ENTITY syncHeader.label           "Sync">

Sorry, the used should be this one: http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/preferences/preferences.dtd#19
Comment 5 Devan Sayles 2012-03-13 17:51:26 PDT
Created attachment 605618 [details] [diff] [review]
updated Sync patch based on Jared's feedback

Changes: uses MPL 2.0, surrounding vbox removed (Owen to add vboxes for individual prefs in search patch), uses correct dtd entry (paneSync.title).
Comment 6 Jared Wein [:jaws] (please needinfo? me) 2012-03-22 11:22:39 PDT
Comment on attachment 605618 [details] [diff] [review]
updated Sync patch based on Jared's feedback

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

::: browser/components/preferences/in-content/preferences.xul
@@ +122,1 @@
>  <!-- Put sync options in this #ifdef -->

I think this Todo can be removed now that it has been accomplished :)

::: browser/components/preferences/in-content/sync.js
@@ +3,5 @@
> +   This Source Code Form is subject to the terms of the Mozilla Public License,
> +   v. 2.0. If a copy of the MPL was not distributed with this file,
> +   you can obtain one at http://mozilla.org/MPL/2.0/.
> +   
> + * ***** END LICENSE BLOCK ***** */

No begin/end license block for MPL2. See https://www.mozilla.org/MPL/headers/ for exact usage.

::: browser/components/preferences/in-content/sync.xul
@@ +6,5 @@
> +   -
> +   - ***** END LICENSE BLOCK ***** -->
> +
> +
> +    <preferences>

This whole file has 4 extra spaces at the beginning of each line. Can you please remove them?

::: browser/locales/en-US/chrome/browser/preferences/sync.dtd
@@ +40,4 @@
>  
>  <!-- Footer stuff -->
>  <!ENTITY prefs.tosLink.label        "Terms of Service">
> +<!ENTITY prefs.ppLink.label         "Privacy Policy">
\ No newline at end of file

I'm not sure what changed here, maybe line endings? Either way, this file shouldn't need any changes.
Comment 7 Jon Rietveld 2012-03-22 16:10:42 PDT
Created attachment 608520 [details] [diff] [review]
sync in-content pane

Fixed to be based off new landing pane, and made changes from Jareds feedback
Comment 8 Jon Rietveld 2012-03-24 20:12:57 PDT
Created attachment 609067 [details] [diff] [review]
sync in-content pane

added consistent title
Comment 9 Blair McBride [:Unfocused] (UNAVAILABLE) 2012-03-25 02:40:50 PDT
Comment on attachment 609067 [details] [diff] [review]
sync in-content pane

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

Indenting still isn't fixed - most of sync.xul is indented by 2 spaces too many, and most wrapped lines are indented by 1 additional space too many.

::: browser/components/preferences/in-content/jar.mn
@@ +14,5 @@
>  *  content/browser/preferences/in-content/applications.js
>  *  content/browser/preferences/in-content/content.xul
>  *  content/browser/preferences/in-content/content.js
> +*  content/browser/preferences/in-content/sync.xul
> +*  content/browser/preferences/in-content/sync.js

These don't need to be run through the preprocessor - remove the *

::: browser/locales/en-US/chrome/browser/preferences/sync.dtd
@@ +40,4 @@
>  
>  <!-- Footer stuff -->
>  <!ENTITY prefs.tosLink.label        "Terms of Service">
> +<!ENTITY prefs.ppLink.label         "Privacy Policy">
\ No newline at end of file

Don't remove the newline at the end of this file (You seem to be having this trouble in most/all patches - is your editor removing the last newline? It *needs* to stay).
Comment 10 Jon Rietveld 2012-03-25 23:10:44 PDT
Created attachment 609228 [details] [diff] [review]
sync in-content pane

fixed feedback, included search vbox's, and fixed title
Comment 11 MSU Capstone Team 2012-03-27 17:35:47 PDT
Created attachment 609966 [details] [diff] [review]
Sync pane with inner vboxs removed

vboxs within the deck element have been removed for purposes of pane switching
Comment 12 Blair McBride [:Unfocused] (UNAVAILABLE) 2012-04-03 22:10:49 PDT
Comment on attachment 609966 [details] [diff] [review]
Sync pane with inner vboxs removed

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

::: browser/components/preferences/in-content/sync.js
@@ +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/. */
> +
> +Components.utils.import("resource://services-sync/service.js");
> +Components.utils.import("resource://gre/modules/Services.jsm");

Move the Services.jsm import into preferences.js - it's a generic module that's commonly used everywhere.

::: browser/components/preferences/in-content/sync.xul
@@ +7,5 @@
> +  <preference id="engine.bookmarks" name="services.sync.engine.bookmarks" type="bool"/>
> +  <preference id="engine.history"   name="services.sync.engine.history"   type="bool"/>
> +  <preference id="engine.tabs"      name="services.sync.engine.tabs"      type="bool"/>
> +  <preference id="engine.prefs"     name="services.sync.engine.prefs"     type="bool"/>
> +  <preference id="engine.passwords" name="services.sync.engine.passwords" type="bool"/>

Format these like the other patches do (eg, the privacy pane in bug 723328).

@@ +23,5 @@
> +  </hbox>
> +</vbox>
> +
> +<vbox data-category="sync" hidden="true">
> +  <deck id="weavePrefsDeck">

No need to wrap the <deck> in a <vbox> if the desk if the only child. Just have the data-category and hidden attributes on the <deck>, and get rid of the <vbox>.

We really should be doing the same for the heading too - the outer <vbox> is rather redundant :\

@@ +146,5 @@
> +      </hbox>
> +    </vbox>
> +
> +
> +    <vbox id="needsUpdate" align="center" pack="center">

Nit: Only one empty line above this.
Comment 13 Jon Rietveld 2012-04-08 22:53:20 PDT
Created attachment 613233 [details] [diff] [review]
sync 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 14 MSU Capstone Team 2012-04-10 11:09:14 PDT
Created attachment 613685 [details] [diff] [review]
Sync with updated data-category
Comment 15 Jon Rietveld 2012-04-12 12:40:07 PDT
Created attachment 614505 [details] [diff] [review]
sync in-content patch for new landing patch
Comment 16 Jon Rietveld 2012-04-12 13:20:17 PDT
Created attachment 614537 [details] [diff] [review]
sync in-content patch for new landing patch
Comment 17 Jon Rietveld 2012-04-20 09:28:14 PDT
Created attachment 616999 [details] [diff] [review]
sync in-content patch

updated this to include import("resource://services-sync/service.js"); since it was taken out of landing patch
Comment 18 Jared Wein [:jaws] (please needinfo? me) 2012-05-08 22:40:54 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/29de131322c5
Comment 19 Ed Morley [:emorley] 2012-05-09 07:43:25 PDT
https://hg.mozilla.org/mozilla-central/rev/29de131322c5
Comment 20 Daniel Cater 2012-05-10 14:55:36 PDT
Something I noticed when checking out the new about:preferences page: in the Sync panel I see "undefined" where the username should be. That doesn't happen with the preferences dialogue.
Comment 21 Jared Wein [:jaws] (please needinfo? me) 2012-05-10 15:01:19 PDT
(In reply to Daniel Cater from comment #20)
> Something I noticed when checking out the new about:preferences page: in the
> Sync panel I see "undefined" where the username should be. That doesn't
> happen with the preferences dialogue.

Can you please file a bug for this with Steps to Reproduce and mark that bug as blocking this bug?
Comment 22 Alfredos-Panagiotis Damkalis [:fredy] 2012-06-22 04:25:50 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.