Closed
Bug 1363412
Opened 7 years ago
Closed 7 years ago
Do a quick sync of the clients collection on device connected
Categories
(Firefox :: Sync, enhancement, P1)
Firefox
Sync
Tracking
()
RESOLVED
FIXED
Firefox 56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: eoger, Assigned: eoger)
Details
Attachments
(1 file)
When another device connects, we show a notification saying "X connected to the account", but when I open "Send Tab to device" I don't see that device. This is pretty confusing, we could do a quick sync of the clients collection when another device connects.
Assignee | ||
Comment 1•7 years ago
|
||
Actually when I think about it, that notification is sent when the 2nd device logs-in (BEFORE the first sync), which would lead to race conditions if the 1st device would sync at that moment. I think the 2nd device should send a collection_modified push notification (just like when we send tabs) after it has done its first sync.
Updated•7 years ago
|
Priority: -- → P2
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•7 years ago
|
||
mozreview-review |
Comment on attachment 8876827 [details] Bug 1363412 - Notify other clients when uploading the local clients record for the first time. https://reviewboard.mozilla.org/r/148152/#review152502 ::: services/sync/tests/unit/test_clients_engine.js:1446 (Diff revision 1) > > - let notifiedIds; > engine.sendCommand("wipeAll", []); > engine._tracker.addChangedID(engine.localID); > - engine.getClientFxaDeviceId = (id) => "fxa-" + id; > - engine._notifyCollectionChanged = (ids) => (notifiedIds = ids); > + const getClientFxaDeviceId = sinon.stub(engine, "getClientFxaDeviceId", (id) => "fxa-" + id); > + const engineMock = sinon.mock(engine); Explanation: I made these changes because _notifyCollectionChanged was not restored properly after this test.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → eoger
Status: NEW → ASSIGNED
Priority: P2 → P1
Comment 4•7 years ago
|
||
mozreview-review |
Comment on attachment 8876827 [details] Bug 1363412 - Notify other clients when uploading the local clients record for the first time. https://reviewboard.mozilla.org/r/148152/#review152650 Looks fine if you are happy with using a TTL here and have confirmed the sending device won't get this notification ::: services/fxaccounts/FxAccountsClient.jsm:434 (Diff revision 1) > * Resolves to an empty object: > * {} > */ > notifyDevices(sessionTokenHex, deviceIds, payload, TTL = 0) { > const body = { > - to: deviceIds, > + to: deviceIds || "all", I assume you checked that "all" works on the FxA side, but does it also exclude sending it back to the sender? We don't want to get this notification and re-sync ourself. ::: services/sync/modules/engines/clients.js:400 (Diff revision 1) > // Reconcile the status of the local records with what we just wrote on the > // server > for (let id of succeeded) { > const commandChanges = this._currentlySyncingCommands[id]; > if (id == this.localID) { > + if (this.isFirstSync) { Do we really need an unlimited TTL for this notification? I'd have thought an hour would be fine (which is what we use for tabs) - we could just change the constant name to reflect it's used for all such client collection notifications.
Attachment #8876827 -
Flags: review?(markh) → review+
Assignee | ||
Comment 5•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8876827 [details] Bug 1363412 - Notify other clients when uploading the local clients record for the first time. https://reviewboard.mozilla.org/r/148152/#review152650 > I assume you checked that "all" works on the FxA side, but does it also exclude sending it back to the sender? We don't want to get this notification and re-sync ourself. Good catch, I opened https://github.com/mozilla/fxa-auth-server/issues/1942. > Do we really need an unlimited TTL for this notification? I'd have thought an hour would be fine (which is what we use for tabs) - we could just change the constant name to reflect it's used for all such client collection notifications. Isn't 0 is instant TTL? (no storage whatsoever).
Comment 6•7 years ago
|
||
(In reply to Edouard Oger [:eoger] from comment #5) > > Do we really need an unlimited TTL for this notification? I'd have thought an hour would be fine (which is what we use for tabs) - we could just change the constant name to reflect it's used for all such client collection notifications. > > Isn't 0 is instant TTL? (no storage whatsoever). Good question :) I was assuming it meant infinite (although I guess that wouldn't make much sense)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•7 years ago
|
||
Turns out I implemented an "excluded" parameter on the FxA /notify endpoint :)
Pushed by eoger@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5cf6b350c71c Notify other clients when uploading the local clients record for the first time. r=markh
Comment 10•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5cf6b350c71c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
You need to log in
before you can comment on or make changes to this bug.
Description
•