Closed
Bug 735091
Opened 12 years ago
Closed 12 years ago
Sync pane for In-Content preferences
Categories
(Firefox :: Settings UI, defect)
Firefox
Settings UI
Tracking
()
VERIFIED
FIXED
Firefox 15
People
(Reporter: saylesd1, Assigned: saylesd1)
References
Details
(Whiteboard: [testday-20120622])
Attachments
(1 file, 10 obsolete files)
13.56 KB,
patch
|
jaws
:
review+
Unfocused
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 2•12 years ago
|
||
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)
Updated•12 years ago
|
Attachment #605210 -
Attachment is patch: true
Comment 3•12 years ago
|
||
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+
Updated•12 years ago
|
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 4•12 years ago
|
||
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•12 years ago
|
||
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 6•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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)
Updated•12 years ago
|
Attachment #605618 -
Attachment is obsolete: true
Attachment #605618 -
Flags: feedback?(bmcbride)
Comment 9•12 years ago
|
||
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•12 years ago
|
||
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)
Updated•12 years ago
|
Attachment #609228 -
Flags: feedback?(jwein) → feedback+
Comment 11•12 years ago
|
||
vboxs within the deck element have been removed for purposes of pane switching
Attachment #609966 -
Flags: feedback?(jwein)
Attachment #609966 -
Flags: feedback?(bmcbride)
Updated•12 years ago
|
Attachment #609966 -
Flags: feedback?(jwein) → feedback+
Updated•12 years ago
|
Attachment #609228 -
Attachment is obsolete: true
Attachment #609228 -
Flags: feedback?(bmcbride)
Comment 12•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
Attachment #609966 -
Attachment is obsolete: true
Updated•12 years ago
|
Attachment #613233 -
Flags: feedback?(jwein) → feedback+
Comment 15•12 years ago
|
||
Attachment #613233 -
Attachment is obsolete: true
Attachment #614505 -
Flags: feedback?(jwein)
Attachment #614505 -
Flags: feedback?(bmcbride)
Attachment #613233 -
Flags: feedback?(bmcbride)
Comment 16•12 years ago
|
||
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)
Updated•12 years ago
|
Attachment #614537 -
Flags: feedback?(jwein) → feedback+
Comment 17•12 years ago
|
||
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)
Updated•12 years ago
|
Attachment #613685 -
Attachment is obsolete: true
Updated•12 years ago
|
Attachment #616999 -
Flags: review+
Updated•12 years ago
|
Attachment #616999 -
Flags: feedback?(bmcbride) → review+
Comment 18•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/29de131322c5
Target Milestone: --- → Firefox 15
Comment 19•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/29de131322c5
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 20•12 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.
Comment 21•12 years ago
|
||
(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•12 years ago
|
||
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.
Description
•