Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Sync pane for In-Content preferences

VERIFIED FIXED in Firefox 15

Status

()

Firefox
Preferences
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: Devan Sayles, Assigned: Devan Sayles)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 15
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [testday-20120622])

Attachments

(1 attachment, 10 obsolete attachments)

(Assignee)

Description

5 years ago
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

Updated

5 years ago
Duplicate of this bug: 735092
(Assignee)

Comment 2

5 years ago
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?
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
(Assignee)

Comment 5

5 years ago
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).
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+

Comment 7

5 years ago
Created attachment 608520 [details] [diff] [review]
sync in-content pane

Fixed to be based off new landing pane, and made changes from Jareds feedback
Attachment #608520 - Flags: feedback?(jwein)
Attachment #608520 - Flags: feedback?(bmcbride)

Comment 8

5 years ago
Created attachment 609067 [details] [diff] [review]
sync in-content pane

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+

Comment 10

5 years ago
Created attachment 609228 [details] [diff] [review]
sync in-content pane

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+

Comment 11

5 years ago
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
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+

Comment 13

5 years ago
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
Attachment #613233 - Flags: feedback?(jwein)
Attachment #613233 - Flags: feedback?(bmcbride)

Comment 14

5 years ago
Created attachment 613685 [details] [diff] [review]
Sync with updated data-category
Attachment #609966 - Attachment is obsolete: true
Attachment #613233 - Flags: feedback?(jwein) → feedback+

Comment 15

5 years ago
Created attachment 614505 [details] [diff] [review]
sync in-content patch for new landing patch
Attachment #613233 - Attachment is obsolete: true
Attachment #614505 - Flags: feedback?(jwein)
Attachment #614505 - Flags: feedback?(bmcbride)
Attachment #613233 - Flags: feedback?(bmcbride)

Comment 16

5 years ago
Created attachment 614537 [details] [diff] [review]
sync in-content patch for new landing patch
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+

Comment 17

5 years ago
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
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: review+
Attachment #616999 - Flags: feedback?(bmcbride) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/29de131322c5
Target Milestone: --- → Firefox 15
https://hg.mozilla.org/mozilla-central/rev/29de131322c5
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Comment 20

5 years ago
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?
Depends on: 754342
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.