Closed
Bug 1368383
Opened 7 years ago
Closed 7 years ago
"send to device" subview on page action menu should trigger clients engine sync showing an animation, and update after the sync
Categories
(Firefox :: Sync, defect, P1)
Firefox
Sync
Tracking
()
RESOLVED
FIXED
Firefox 56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: markh, Assigned: eoger)
References
Details
Attachments
(3 files)
STR: * Within 10 seconds of opening the browser, click on the "send to device" subview. Actual: * "No devices" available is shown and never changes (although it may change if you dismiss the menu and reshow it), even though my account does have other devices. Expected: * The menu shows something to indicate the list of devices is being fetched (eg, "Fetching device list..." with some kind of spinner?) * Within a few seconds the menu updates to the list of devices. (IOW, roughly what the "Synced Tabs" subview does today) The problem here is that Sync doesn't persist the list of clients - it needs to do a first sync to know what they are. Best I can tell, that menu item neither triggers that first sync, nor does it update as the device list changes.
Assignee | ||
Comment 1•7 years ago
|
||
This could probably be done in bug 1359894 since this patch is about supporting more states.
Reporter | ||
Comment 2•7 years ago
|
||
(In reply to Edouard Oger [:eoger] from comment #1) > This could probably be done in bug 1359894 since this patch is about > supporting more states. I'm not sure this makes sense - this bug is more about the "sync is configured" state, whereas bug 1359894 is about the "sync is not configured" state. However, I do agree the page-action menu will want the promo link from bug 1359894 when sync isn't configured.
Assignee | ||
Comment 3•7 years ago
|
||
Talked with rfeeley about that one. For now we should have a consistent experience: - Show a disabled menu item in the context menu (currently hidden) - Show a disabled menu item (without the right arrow) in the page action menu.
Comment 4•7 years ago
|
||
Since disabling provides no context during device fetching and could be considered like a bug to a Sync user, could it not say something like what Mark proposed? "Getting device list..."
Flags: needinfo?(rfeeley)
Comment 5•7 years ago
|
||
We're thinking iteratively here and trying to fix the immediate problem quickly, adding enhancement next. I'm meeting with Aaron, Photon visual designer tomorrow. We will discuss the approach of "getting devices" which is a good one.
Flags: needinfo?(rfeeley)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•7 years ago
|
||
Part 1 of this patch is there for consistency purposes: - We show the Send Tab/Page/Link to Device menu always, we just disable it if certain conditions are not met. - The test coverage of the different states is WAY better than before and the tests have been moved in our dedicated sync tests folder. It makes me a lot more confident supporting more states in bug 1359894 without breaking anything else. Part 2 is the meat of the bug: supporting the "notready" state. See the mockup I attached. We also trigger a background Sync when we enter the submenu, and repaint it when the clients collection is synced.
Assignee: nobody → eoger
Status: NEW → ASSIGNED
Assignee | ||
Updated•7 years ago
|
Priority: -- → P1
Reporter | ||
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8873595 [details] Bug 1368383 part 1 - Remove services.sync.sendTabToDevice.enabled pref. https://reviewboard.mozilla.org/r/144976/#review149068 ::: commit-message-7b893:1 (Diff revision 1) > +Bug 1368383 part 2 - Always show Send Tab to Device in the context menu. r?markh both these patches claim to be "part 2" :) ::: browser/base/content/browser-sync.js:48 (Diff revision 1) > get sendTabToDeviceEnabled() { > return Services.prefs.getBoolPref("services.sync.sendTabToDevice.enabled"); > }, > > + get syncReady() { > + return Components.classes["@mozilla.org/weave/service;1"] Might as well change them to Cc and Ci while you are moving it. ::: browser/base/content/browser-sync.js:366 (Diff revision 1) > } > }, > > + // "Send Tab to Device" menu item > updateTabContextMenu(aPopupMenu, aTargetTab) { > - if (!this.sendTabToDeviceEnabled || !this.weaveService.ready) { > + const sendTabToDeviceEnabled = this.sendTabToDeviceEnabled; I don't mind if/when/where you do this, but you should consider removing |this.sendTabToDeviceEnabled| and the related pref entirely - IIUC the pref isn't used for the pageaction menu, so it's somewhat misleading. ::: browser/base/content/browser-sync.js:381 (Diff revision 1) > - > - ["context_sendTabToDevice", "context_sendTabToDevice_separator"] > + this.isSendableURI(aTargetTab.linkedBrowser.currentURI.spec); > + menuItem.disabled = !enabled; > - .forEach(id => { document.getElementById(id).hidden = !showSendTab }); > }, > > + // "Send Page to Device" and "Sent Link to Device" menu items typo in this comment that might as well be fixed now - s/Sent/Send/ ::: browser/base/content/test/contextMenu/browser_contextmenu_mozextension.js:18 (Diff revision 1) > iconURL: "https://example.com/browser/browser/base/content/test/general/moz.png", > shareURL: "https://example.com/browser/browser/base/content/test/social/share.html" > }; > > +// Disable send tabs to device context menu > +const sendTabEnabled = Services.prefs.getBoolPref("services.sync.sendTabToDevice.enabled"); bugger - I guess this might make it tricky to remove that pref? But it still seems worthwhile given all the test churn below. ::: browser/base/content/test/sync/head.js:2 (Diff revision 1) > +// Mocks a getter or a function > +// This is basically sinon.js (our in-tree version doesn't do getters :/) maybe get a bug on file to upgrade and reference it here?
Attachment #8873595 -
Flags: review?(markh) → review+
Reporter | ||
Comment 10•7 years ago
|
||
mozreview-review |
Comment on attachment 8873596 [details] Bug 1368383 part 2 - Handle the Sync-not-ready state in page action send to device. https://reviewboard.mozilla.org/r/144978/#review149074 looking good, but I think there are a few issues here. ::: browser/base/content/browser.js:7837 (Diff revision 1) > showSendToDeviceView(subviewButton) { > + this.setupSendToDeviceView(); > + PanelUI.showSubView("page-action-sendToDeviceView", subviewButton); > + > + // Force a background sync > + Services.obs.addObserver({ where is this observer removed? It seems we should also tell the observer services to keep a weak-ref or it might end up holding a ref to window. Further, the notification will never come if sync isn't configured (so removing the observer in the notification isn't really an option.) Also, doSync early returning when !UIState.ready seems like it might be a problem here? (Although I guess it should become ready fairly quick) We probably also want to limit the sync to the clients engine. And finally, I probably mislead you before, but unlike SyncedTabs, I don't know if it is necessary to do a sync every time the menu is used, as the list of clients is (presumably) far more static than the list of clients - and the list of clients will only be 10 minutes out of date anyway. So I'd be inclined to do this only if sync is configured and there are no clients. I could be talked out of this though if you felt strongly. ::: browser/base/content/browser.js:7853 (Diff revision 1) > let browser = gBrowser.selectedBrowser; > let url = browser.currentURI.spec; > let title = browser.contentTitle; > let body = this.sendToDeviceBody; > > + if (UIState.get().status != UIState.STATUS_SIGNED_IN) { I think this is leaking the gSync abstraction - a gSync.isConfigured or similar getter seems better (and you'll probably want to use it above) (I only just noticed this existed before, but I still think we should do it - have browser.js reference UIState seems (a) likely to mislead people and (b) difficult for people to work out where UIState comes from.) ::: browser/base/content/test/urlbar/browser_page_action_menu.js:131 (Diff revision 1) > gPanel.hidePopup(); > await hiddenPromise; > }); > }); > > +add_task(async function sendToDevice_syncNotReady() { A test that exercises your observer would also be good (even if it mocks alot of things - ie, simulates sync sending the notification.) Let me know if this gets too painful and we can discuss options.
Attachment #8873596 -
Flags: review?(markh) → review-
Reporter | ||
Comment 11•7 years ago
|
||
(In reply to Mark Hammond [:markh] from comment #10) > know if it is necessary to do a sync every time the menu is used, as the > list of clients is (presumably) far more static than the list of clients - doh! - than the list of *tabs*
Assignee | ||
Comment 12•7 years ago
|
||
Comment on attachment 8873595 [details] Bug 1368383 part 1 - Remove services.sync.sendTabToDevice.enabled pref. https://reviewboard.mozilla.org/r/144976/#review149328
Assignee | ||
Comment 13•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8873595 [details] Bug 1368383 part 1 - Remove services.sync.sendTabToDevice.enabled pref. https://reviewboard.mozilla.org/r/144976/#review149068 > maybe get a bug on file to upgrade and reference it here? Filled bug 1369855
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 16•7 years ago
|
||
mozreview-review |
Comment on attachment 8873596 [details] Bug 1368383 part 2 - Handle the Sync-not-ready state in page action send to device. https://reviewboard.mozilla.org/r/144978/#review149552 ::: browser/base/content/browser.js:7846 (Diff revision 2) > this.panel.hidePopup(); > MailIntegration.sendLinkForBrowser(gBrowser.selectedBrowser); > }, > > showSendToDeviceView(subviewButton) { > + if (!this._initialized) { I think you should initialize/"declare" this._initialized. ::: browser/base/content/browser.js:7846 (Diff revision 2) > this.panel.hidePopup(); > MailIntegration.sendLinkForBrowser(gBrowser.selectedBrowser); > }, > > showSendToDeviceView(subviewButton) { > + if (!this._initialized) { If you stick with using `this._initialized`, you should probably initialize it (but see below, I'm not convinced it should be done like this) ::: browser/base/content/browser.js:7847 (Diff revision 2) > MailIntegration.sendLinkForBrowser(gBrowser.selectedBrowser); > }, > > showSendToDeviceView(subviewButton) { > + if (!this._initialized) { > + Services.obs.addObserver(this, "weave:engine:sync:finish", true); this observer still isn't removed, and I believe it's not actually necessary in most cases - we only need to do this if the menu is shown before the first sync (it doesn't seem worthwhile having it just on the off chance that a subsequent sync that produced a different device list happens to finish exactly as the menu is up.) I expect it will also cause flicker when it does fire (which makes sense when the list goes from no items to a device list, but not when the device list remains the same) ::: browser/base/content/browser.js:7858 (Diff revision 2) > + > + observe(subject, topic, data) { > + if (window.closed || topic != "weave:engine:sync:finish" || data != "clients") { > + return; > + } > + this.setupSendToDeviceView(); we only need to do this is the popup is still open, right? ::: browser/base/content/browser.js:7886 (Diff revision 2) > - body.setAttribute("hasdevices", "true"); > - } else { > - body.removeAttribute("hasdevices"); > + // Could be unconfigured or unverified > + body.setAttribute("state", "notsignedin"); > + return; > } > > - if (UIState.get().status == UIState.STATUS_SIGNED_IN) { > + if (!gSync.syncReady) { I think we should put some comments here indicating why this block exists. ::: browser/base/content/browser.js:7890 (Diff revision 2) > > - if (UIState.get().status == UIState.STATUS_SIGNED_IN) { > - body.setAttribute("signedin", "true"); > - } else { > - body.removeAttribute("signedin"); > + if (!gSync.syncReady) { > + body.setAttribute("state", "notready"); > + // Force a background Sync > + Services.tm.dispatchToMainThread(() => { > + Weave.Service.sync([]); // [] = clients engine only Maybe just having a local observer function here, that removes the observer, checked gSync.syncReady is not true, and just recursively calls back into this function? But - thinking some more, do we actually need an observer at all? .sync() blocks - so why not just repopulate when it returns. Your async patch will turn that into a promise which will still work fine. ::: browser/locales/en-US/chrome/browser/browser.dtd:966 (Diff revision 2) > > <!ENTITY sendToDevice.label "Send to Device…"> > <!ENTITY sendToDevice.viewTitle "Send to Device"> > <!ENTITY sendToDevice.fxaRequired.label "Required"> > <!ENTITY sendToDevice.noDevices.label "No Devices Available"> > +<!ENTITY sendToDevice.syncNotReady.label "Syncing Devices…"> Please check with Ryan, but I think "Fetching device list" or similar would be better - "Syncing Devices" doesn't really convey what is going on IMO.
Attachment #8873596 -
Flags: review?(markh) → review-
Comment 17•7 years ago
|
||
I went through many variations with Michelle and "Syncing Devices…" was preferred. That copy paired with the ellipsis and grey copy definitely communicate that the list is on the way.
Comment 18•7 years ago
|
||
from Michelle: General rules of thumb: If it’s part of Sync, use “Sync” or “Syncing” If it is representing a loading state use the ellipsis
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 21•7 years ago
|
||
Thank you Mark, indeed we don't need that listener at all :)
Reporter | ||
Comment 22•7 years ago
|
||
mozreview-review |
Comment on attachment 8873596 [details] Bug 1368383 part 2 - Handle the Sync-not-ready state in page action send to device. https://reviewboard.mozilla.org/r/144978/#review149874 Thanks! I'm really not sure if this patch qualifies as part of the soft-freeze - I don't think this qualifies as "risky" - maybe needinfo :adw to see what he thinks? ::: browser/base/content/browser.js:7881 (Diff revision 3) > - body.setAttribute("signedin", "true"); > - } else { > - body.removeAttribute("signedin"); > + // of devices will be empty. > + if (!gSync.syncReady) { > + body.setAttribute("state", "notready"); > + // Force a background Sync > + Services.tm.dispatchToMainThread(() => { > + Weave.Service.sync([]); // [] = clients engine only I should have mentioned this before, but can we have gSync.doSync take an optional arg and call that instead? That seems better than referencing Weave directly in browser.js ::: browser/base/content/browser.js:7882 (Diff revision 3) > - } else { > - body.removeAttribute("signedin"); > + if (!gSync.syncReady) { > + body.setAttribute("state", "notready"); > + // Force a background Sync > + Services.tm.dispatchToMainThread(() => { > + Weave.Service.sync([]); // [] = clients engine only > + if (!window.closed) { It should be impossible for gSync.syncReady to be false here, but it seems worth checking - if something goes wrong we will do this infinitely.
Attachment #8873596 -
Flags: review?(markh) → review+
Assignee | ||
Comment 23•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8873596 [details] Bug 1368383 part 2 - Handle the Sync-not-ready state in page action send to device. https://reviewboard.mozilla.org/r/144978/#review149874 > I should have mentioned this before, but can we have gSync.doSync take an optional arg and call that instead? That seems better than referencing Weave directly in browser.js Sadly doSync() wraps the call to sync() in a setTimeout(), which means we would have to go back to observers :/
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 28•7 years ago
|
||
Pushed by eoger@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3ca93081969b part 1 - Always show Send Tab to Device in the context menu. r=markh https://hg.mozilla.org/integration/autoland/rev/4eb7778c4325 part 2 - Handle the Sync-not-ready state in page action send to device. r=markh
I had to back these out for mochitest-a11y failures like https://treeherder.mozilla.org/logviewer.html#?job_id=105301355&repo=autoland https://hg.mozilla.org/integration/autoland/rev/807243861494284b3dc25ebad7bfe3877529b1d4
Flags: needinfo?(eoger)
Assignee | ||
Comment 30•7 years ago
|
||
I honesty have no idea why are these tests failing, I wonder who could help me.
Flags: needinfo?(eoger)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 35•7 years ago
|
||
Pushed by eoger@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/864fa58e6777 part 1 - Remove services.sync.sendTabToDevice.enabled pref. r=markh https://hg.mozilla.org/integration/autoland/rev/83296fc07093 part 2 - Handle the Sync-not-ready state in page action send to device. r=markh
Comment 36•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/864fa58e6777 https://hg.mozilla.org/mozilla-central/rev/83296fc07093
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
You need to log in
before you can comment on or make changes to this bug.
Description
•