Closed Bug 735091 Opened 12 years ago Closed 12 years ago

Sync pane for In-Content preferences

Categories

(Firefox :: Settings UI, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 15

People

(Reporter: saylesd1, Assigned: saylesd1)

References

Details

(Whiteboard: [testday-20120622])

Attachments

(1 file, 10 obsolete files)

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
Attached patch first patch for sync pane (obsolete) — Splinter Review
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?
Attachment #605210 - Flags: feedback?(jwein)
Attachment #605210 - Flags: feedback?(bmcbride)
Attachment #605210 - Attachment is patch: true
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.
Attachment #605210 - Flags: feedback?(jwein) → feedback+
Assignee: nobody → saylesd1
Blocks: 718011
Status: UNCONFIRMED → ASSIGNED
Component: Untriaged → Preferences
Ever confirmed: true
OS: Mac OS X → All
QA Contact: untriaged → preferences
Hardware: x86 → All
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
Changes: uses MPL 2.0, surrounding vbox removed (Owen to add vboxes for individual prefs in search patch), uses correct dtd entry (paneSync.title).
Attachment #605210 - Attachment is obsolete: true
Attachment #605618 - Flags: feedback?(jwein)
Attachment #605618 - Flags: feedback?(bmcbride)
Attachment #605210 - Flags: feedback?(bmcbride)
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.
Attachment #605618 - Flags: feedback?(jwein) → feedback+
Attached patch sync in-content pane (obsolete) — Splinter Review
Fixed to be based off new landing pane, and made changes from Jareds feedback
Attachment #608520 - Flags: feedback?(jwein)
Attachment #608520 - Flags: feedback?(bmcbride)
Attached patch sync in-content pane (obsolete) — Splinter Review
added consistent title
Attachment #608520 - Attachment is obsolete: true
Attachment #609067 - Flags: feedback?(jwein)
Attachment #609067 - Flags: feedback?(bmcbride)
Attachment #608520 - Flags: feedback?(jwein)
Attachment #608520 - Flags: feedback?(bmcbride)
Attachment #605618 - Attachment is obsolete: true
Attachment #605618 - Flags: feedback?(bmcbride)
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).
Attachment #609067 - Flags: feedback?(bmcbride) → feedback+
Attached patch sync in-content pane (obsolete) — Splinter Review
fixed feedback, included search vbox's, and fixed title
Attachment #609067 - Attachment is obsolete: true
Attachment #609228 - Flags: feedback?(jwein)
Attachment #609228 - Flags: feedback?(bmcbride)
Attachment #609067 - Flags: feedback?(jwein)
Attachment #609228 - Flags: feedback?(jwein) → feedback+
vboxs within the deck element have been removed for purposes of pane switching
Attachment #609966 - Flags: feedback?(jwein)
Attachment #609966 - Flags: feedback?(bmcbride)
Attachment #609966 - Flags: feedback?(jwein) → feedback+
Attachment #609228 - Attachment is obsolete: true
Attachment #609228 - Flags: feedback?(bmcbride)
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.
Attachment #609966 - Flags: feedback?(bmcbride) → feedback+
Attached patch sync in-content patch (obsolete) — Splinter Review
removed vbox from heading, fixed blairs feedback, removed vbox around 1st child elements and added data-category and hidden to 1st child element instead
Attachment #613233 - Flags: feedback?(jwein)
Attachment #613233 - Flags: feedback?(bmcbride)
Attached patch Sync with updated data-category (obsolete) — Splinter Review
Attachment #609966 - Attachment is obsolete: true
Attachment #613233 - Flags: feedback?(jwein) → feedback+
Attachment #613233 - Attachment is obsolete: true
Attachment #614505 - Flags: feedback?(jwein)
Attachment #614505 - Flags: feedback?(bmcbride)
Attachment #613233 - Flags: feedback?(bmcbride)
Attachment #614505 - Attachment is obsolete: true
Attachment #614537 - Flags: feedback?(jwein)
Attachment #614537 - Flags: feedback?(bmcbride)
Attachment #614505 - Flags: feedback?(jwein)
Attachment #614505 - Flags: feedback?(bmcbride)
Attachment #614537 - Flags: feedback?(jwein) → feedback+
updated this to include import("resource://services-sync/service.js"); since it was taken out of landing patch
Attachment #614537 - Attachment is obsolete: true
Attachment #616999 - Flags: feedback?(bmcbride)
Attachment #614537 - Flags: feedback?(bmcbride)
Attachment #613685 - Attachment is obsolete: true
Attachment #616999 - Flags: feedback?(bmcbride) → review+
https://hg.mozilla.org/mozilla-central/rev/29de131322c5
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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.
(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?
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]
You need to log in before you can comment on or make changes to this bug.