Closed
Bug 668760
Opened 13 years ago
Closed 13 years ago
Desktop Tab doesn't display any content if Sync is performed
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(fennec+)
VERIFIED
FIXED
Firefox 9
Tracking | Status | |
---|---|---|
fennec | + | --- |
People
(Reporter: xti, Assigned: lucasr)
Details
(Whiteboard: [inbound])
Attachments
(6 files, 4 obsolete files)
32.59 KB,
image/png
|
Details | |
1.82 KB,
patch
|
mbrubeck
:
review+
|
Details | Diff | Splinter Review |
2.67 KB,
patch
|
mbrubeck
:
review+
|
Details | Diff | Splinter Review |
1.87 KB,
patch
|
lucasr
:
review+
|
Details | Diff | Splinter Review |
2.73 KB,
patch
|
lucasr
:
review+
|
Details | Diff | Splinter Review |
2.90 KB,
patch
|
mbrubeck
:
review+
|
Details | Diff | Splinter Review |
Build id : Mozilla/5.0 (Android;Linux armv7l;rv:7.0a1)Gecko/20110630 Firefox/7.0a1 Fennec/7.0a1 Device: Motorola Droid 2 OS: Android 2.2 Steps to reproduce: 1. Open Fennec app 2. Go to Preferences > Sync 3. Connect to Sync with a valid Sync account 4. Tap back button 5. Tap on URL Bar 6. Tap on Desktop Tab Expected result: After step 6, there are listed tabs opened on Desktop Firefox. Actual result: After step 6, Desktop Tab remains blank. Tabs opened on Desktop Firefox are not synced.
Updated•13 years ago
|
tracking-fennec: --- → ?
Comment 1•13 years ago
|
||
Please check the sync log for errors if this is still occurring, as I could not reproduce on 07/04 Nightly, and re-file under Sync if such errors exist.
Comment 2•13 years ago
|
||
Madhava, it seems like if there are not desktop tabs to display there should be some status message such as "sync not set up", "sync'ing your tabs" or "error while sync'ing". Thoughts?
tracking-fennec: ? → 8+
Updated•13 years ago
|
Assignee: nobody → lucasr.at.mozilla
tracking-fennec: 8+ → +
Comment 3•13 years ago
|
||
Yes - if we really have nothing to display, we should show a message. I could have sworn that we had something for this situation. Mfinkle - don't we? More broadly, though, do we know why we have nothing to show on that tab at the moment? I vaguely remember from when we put this together that by the time a user can get back over to the tabs pane there should be content>
Assignee | ||
Comment 4•13 years ago
|
||
According to the code, we do show an error dialog if sync fails for different reasons. However, there's no code to show a message when sync succeeds but there's nothing to display. As Madhava said, is that even possible to happen?
Assignee | ||
Comment 5•13 years ago
|
||
Attachment #554431 -
Flags: review?(mbrubeck)
Assignee | ||
Comment 6•13 years ago
|
||
Attachment #554432 -
Flags: review?(mbrubeck)
Assignee | ||
Comment 7•13 years ago
|
||
Attachment #554434 -
Flags: review?(mbrubeck)
Assignee | ||
Comment 8•13 years ago
|
||
Attachment #554435 -
Flags: review?(mbrubeck)
Assignee | ||
Comment 9•13 years ago
|
||
Here's how it looks like. I think we should use same type of UI for sync errors/warnings. Much better than using alert dialogs. It would also centralize all sync UI handling on one place. Right now, "error" UI is handled in WeaveGlue and "success" UI is handled in remotetabs-list.
Comment 10•13 years ago
|
||
Comment on attachment 554431 [details] [diff] [review] (1/4) Add message label element to remotetabs-list >+remotetabslist > hbox.remotetabs-throbber-box, >+remotetabslist > hbox.remotetabs-message-box { Just use ".remotetabs-message-box" as the selector: https://developer.mozilla.org/en/writing_efficient_css#Don.e2.80.99t_qualify_Class_Rules_with_tag_names You can probably clean up the existing ".remotetabs-throbber-box" selector while you're at it. >+.remotetabs-message-box > label { Just apply these styles to the box; they should be inherited by the label: https://developer.mozilla.org/en/writing_efficient_css#Tag.c2.a0Category_rules_should_never_contain_a_child_selector https://developer.mozilla.org/en/writing_efficient_css#Rely_on_inheritance >+remotetabslist[loading="true"] > hbox.remotetabs-throbber-box, >+remotetabslist[message="true"] > hbox.remotetabs-message-box { Remove "hbox" from these rules. Also, you can use XBL inheritance to avoid the child selector here: https://developer.mozilla.org/en/writing_efficient_css#Question_all_usages_of_the_child_selector >+remotetabslist[loading="true"] > richlistbox.remotetabs-list-children, >+remotetabslist[message="true"] > richlistbox.remotetabs-list-children { Similar changes here. The perf impact of these changes is pretty minimal (I don't think we've seen a big impact in our perf tests from making similar changes in the past), but it's much easier to make them now than to try to clean them up later. This patch looks good, but I'd like to re-review it with the changes above.
Attachment #554431 -
Flags: review?(mbrubeck) → review-
Updated•13 years ago
|
Attachment #554432 -
Flags: review?(mbrubeck) → review+
Comment 11•13 years ago
|
||
Comment on attachment 554435 [details] [diff] [review] (4/4) Show message when there are no desktop tabs to display >+ <constructor> >+ <![CDATA[ >+ this._bundle = Services.strings.createBundle("chrome://browser/locale/sync.properties"); >+ ]]> >+ </constructor> Drive by: I'd like to avoid doing any file access if we don't need to. Can we just make the bundle when we need the string? or use a <field>, since fields are only executed when first used.
Updated•13 years ago
|
Attachment #554434 -
Flags: review?(mbrubeck) → review+
Comment 12•13 years ago
|
||
Comment on attachment 554435 [details] [diff] [review] (4/4) Show message when there are no desktop tabs to display >+ this._messageLabel.value = this._bundle.GetStringFromName("sync.message.notabs"); >+ this.setAttribute("message", "true"); You can either get the bundle on demand as mfinkle suggests, or since only one message is even used, move it to a DTD file and include it directly in the XBL. If you do want the message to be changeable, I think it would be nice to combine the two lines above. You could put xbl:inherits="value=message" on the <label>, and then it won't need an anonid or a field. And in the CSS selector you can just use "[message]" instead of "[message=true]". Looking good; please re-request review with the above comments addressed.
Attachment #554435 -
Flags: review?(mbrubeck) → review-
Comment 13•13 years ago
|
||
(In reply to Matt Brubeck (:mbrubeck) from comment #12) > Comment on attachment 554435 [details] [diff] [review] > (4/4) Show message when there are no desktop tabs to display > > >+ this._messageLabel.value = this._bundle.GetStringFromName("sync.message.notabs"); > >+ this.setAttribute("message", "true"); > > You can either get the bundle on demand as mfinkle suggests, or since only > one message is even used, move it to a DTD file and include it directly in > the XBL. Let's not start this pattern if possible. > If you do want the message to be changeable, I think it would be nice to > combine the two lines above. You could put xbl:inherits="value=message" on > the <label>, and then it won't need an anonid or a field. And in the CSS > selector you can just use "[message]" instead of "[message=true]". Yes, Yes, Yes - great catch.
Assignee | ||
Comment 14•13 years ago
|
||
Attachment #555109 -
Flags: review?(mbrubeck)
Assignee | ||
Comment 15•13 years ago
|
||
Attachment #554431 -
Attachment is obsolete: true
Attachment #555110 -
Flags: review?(mbrubeck)
Assignee | ||
Comment 16•13 years ago
|
||
Just updated to latest patch series. No real changes. Keeping the review+.
Attachment #554432 -
Attachment is obsolete: true
Attachment #555111 -
Flags: review+
Assignee | ||
Comment 17•13 years ago
|
||
Updated for the latest patch series. No real changes. Keeping the review+.
Attachment #554434 -
Attachment is obsolete: true
Attachment #555112 -
Flags: review+
Assignee | ||
Comment 18•13 years ago
|
||
Attachment #554435 -
Attachment is obsolete: true
Attachment #555113 -
Flags: review?(mbrubeck)
Updated•13 years ago
|
Attachment #555109 -
Flags: review?(mbrubeck) → review+
Updated•13 years ago
|
Attachment #555110 -
Flags: review?(mbrubeck) → review+
Updated•13 years ago
|
Attachment #555113 -
Flags: review?(mbrubeck) → review+
Assignee | ||
Comment 19•13 years ago
|
||
Decided to attach patch 6/6 to a separate bug (see bug 681356) so don't wait for it :-)
Assignee | ||
Updated•13 years ago
|
Keywords: uiwanted → checkin-needed
Comment 20•13 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/35413f020d21 https://hg.mozilla.org/integration/mozilla-inbound/rev/ac0b1fc28e3d https://hg.mozilla.org/integration/mozilla-inbound/rev/f532be24c7c8 https://hg.mozilla.org/integration/mozilla-inbound/rev/4a130dda0ed8 https://hg.mozilla.org/integration/mozilla-inbound/rev/4a4b93a6a734
Comment 21•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/35413f020d21 http://hg.mozilla.org/mozilla-central/rev/ac0b1fc28e3d http://hg.mozilla.org/mozilla-central/rev/f532be24c7c8 http://hg.mozilla.org/mozilla-central/rev/4a130dda0ed8 http://hg.mozilla.org/mozilla-central/rev/4a4b93a6a734
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 9
Comment 22•13 years ago
|
||
Verified fixed on build: Mozilla /5.0 (Android;Linux armv7l;rv:9.0a1) Gecko/20110902 Firefox/9.0a1 Fennec/9.0a1 Device: LG Optimus 2X (Android 2.2)
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•