Closed Bug 633681 Opened 13 years ago Closed 13 years ago

Sync doesn't init all-tabs dropdown menu for new windows

Categories

(Firefox :: Sync, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: Dolske, Assigned: philikon)

References

Details

(Whiteboard: [softblocker])

Attachments

(3 files)

Found in bug 625320 comment 29. gSyncUI.initUI() is only called upon weave:service:ready. Any browser windows opened after this notification won't have the alltabs-popup initialized for Sync (ie, the "Tabs From Other Computers" entry will be missing.)

Indeed, the new browser window I have open right now is missing this entry, whereas the older window that's open does have it.

Presumably per-window things handled by the other observers in initUI() are also broken in new windows.
Well F. I'm pretty sure that was working :/

We need to check if Sync is ready before adding that observer and otherwise just proceed to call initUI.

We might want to block on this (why am I saying that!?) - the toolbar button is going broken and presumably the sync now button in the menu.
So I'm thinking: on weave:service:ready register an observer for new windows created. If it's a browser window, call gSync.initUI() in the new window.
Assignee: nobody → philipp
blocking2.0: --- → ?
Whiteboard: [softblocker?]
(In reply to comment #2)
> So I'm thinking: on weave:service:ready register an observer for new windows
> created. If it's a browser window, call gSync.initUI() in the new window.

Well, we'd need to do that in a window-agnostic place in browser/, which I don't think we have for sync. I suppose we could do it in browserGlue, but I don't really see the need to have another domwindowopened observer
(In reply to comment #2)
> So I'm thinking: on weave:service:ready register an observer for new windows
> created. If it's a browser window, call gSync.initUI() in the new window.

Yeah that's not gonna work becuase we'd have one of those per window still. I'm going to add a Weave.Status.ready flag to indicate whether weave:service:ready has been fired or not, and just check that in gSyncUI.init(). If true, we'll proceed to call gSyncUI.initUI() immediately. My only worry here is a potential talos regression, so I'll definitely get some 'try' coverage.
Add a Status.ready flag that indicates whether weave:service:ready has been fired yet or not.
Attachment #512348 - Flags: review?(mconnor)
Try build: http://tbpl.mozilla.org/?tree=MozillaTry&rev=b15dd087b144
Status: NEW → ASSIGNED
Whiteboard: [softblocker?] → [softblocker?][has patch][needs review mconnor]
blocking2.0: ? → final+
Whiteboard: [softblocker?][has patch][needs review mconnor] → [softblocker][has patch][needs review mconnor]
I was further looking at the whole about:sync-tabs mess and came across the various isLoggedIn checks. First we shouldn't really depend in isLoggedIn anymore, so I added a comment referring to bug 583344. Also noticed that there's some unused code we can remove and some clean up we can do.
Attachment #512709 - Flags: review?(mconnor)
compare-talos results for the try build: http://bit.ly/gLgC6k. Compares it to the 10 pushes to mozilla-central prior to the revision that the try build is based on.

No regression on twinopen which sounds like the important test here. I'm not terribly familiar with the talos suite...

There are some "red" values for tp4 and ts which had me worried at first. But then I looked at the compare-talos source and realized it doesn't actually compute a standard deviation. It just looks at whether the try value is in the min/max range which isn't statistically meaningful[1]. If I compute the std deviation for the 10 m-c values, the try build is nearly compatible with 95% confidence level (2 sigma).


[1] Also, 10 samples are barely statistically meaningful. I wish we could automate those "old revisions" somehow, it's a pain to track them down manually. Then we could automatically compare a try build against its 100 or so previous m-c pushes. Maybe the graph server could do that for us, incl. the computation of std devation etc.? And then automatically attach the compare-talos results to the bug? Anyway, I'm rambling...
Attachment #512348 - Flags: review?(mconnor) → review+
Attachment #512350 - Flags: review?(mconnor) → review+
Attachment #512709 - Flags: review?(mconnor) → review+
Attachment #512348 - Flags: approval2.0?
Attachment #512350 - Flags: approval2.0?
Attachment #512709 - Flags: approval2.0?
Whiteboard: [softblocker][has patch][needs review mconnor] → [softblocker][has patch][has review]
Attachment #512348 - Flags: approval2.0? → approval2.0+
Attachment #512350 - Flags: approval2.0? → approval2.0+
Attachment #512709 - Flags: approval2.0? → approval2.0+
Part 1 landed in fx-sync:
https://hg.mozilla.org/services/fx-sync/rev/4924bb609838

Landed on places:
Part 1: http://hg.mozilla.org/projects/places/rev/51ac1d179a84
Part 2: http://hg.mozilla.org/projects/places/rev/5cc8a9310cde
Part 3: http://hg.mozilla.org/projects/places/rev/393de498864f
Whiteboard: [softblocker][has patch][has review] → [softblocker][fixed in fx-sync][fixed in places]
Merged to m-c:

Part 1: http://hg.mozilla.org/mozilla-central/rev/51ac1d179a84
Part 2: http://hg.mozilla.org/mozilla-central/rev/5cc8a9310cde
Part 3: http://hg.mozilla.org/mozilla-central/rev/393de498864f
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [softblocker][fixed in fx-sync][fixed in places] → [softblocker]
Verified using Mozilla/5.0 (Windows NT 6.1; rv:2.0b12) Gecko/20100101 Firefox/4.0b12 and Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b12pre) Gecko/20110222 Firefox/4.0b12pre.
Status: RESOLVED → VERIFIED
Blocks: 684160
No longer blocks: 615950
Component: Firefox Sync: UI → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: