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)

Firefox 7
ARM
Android
defect
Not set
normal

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.
tracking-fennec: --- → ?
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.
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+
Assignee: nobody → lucasr.at.mozilla
tracking-fennec: 8+ → +
Keywords: uiwanted
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>
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?
Attachment #554431 - Flags: review?(mbrubeck)
Attachment #554432 - Flags: review?(mbrubeck)
Attachment #554435 - Flags: review?(mbrubeck)
Attached image Screenshot
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 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-
Attachment #554432 - Flags: review?(mbrubeck) → review+
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.
Attachment #554434 - Flags: review?(mbrubeck) → review+
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-
(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.
Attachment #554431 - Attachment is obsolete: true
Attachment #555110 - Flags: review?(mbrubeck)
Just updated to latest patch series. No real changes. Keeping the review+.
Attachment #554432 - Attachment is obsolete: true
Attachment #555111 - Flags: review+
Updated for the latest patch series. No real changes. Keeping the review+.
Attachment #554434 - Attachment is obsolete: true
Attachment #555112 - Flags: review+
Attachment #554435 - Attachment is obsolete: true
Attachment #555113 - Flags: review?(mbrubeck)
Attachment #555109 - Flags: review?(mbrubeck) → review+
Attachment #555110 - Flags: review?(mbrubeck) → review+
Attachment #555113 - Flags: review?(mbrubeck) → review+
Decided to attach patch 6/6 to a separate bug (see bug 681356) so don't wait for it :-)
Keywords: uiwantedcheckin-needed
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.

Attachment

General

Created:
Updated:
Size: