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)

defect

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.
This could probably be done in bug 1359894 since this patch is about supporting more states.
(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.
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.
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)
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)
Attached image mockup.png
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
Priority: -- → P1
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+
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-
(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*
Comment on attachment 8873595 [details]
Bug 1368383 part 1 - Remove services.sync.sendTabToDevice.enabled pref.

https://reviewboard.mozilla.org/r/144976/#review149328
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 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-
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.
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
Thank you Mark, indeed we don't need that listener at all :)
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+
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 :/
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 honesty have no idea why are these tests failing, I wonder who could help me.
Flags: needinfo?(eoger)
Depends on: 1372419
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
https://hg.mozilla.org/mozilla-central/rev/864fa58e6777
https://hg.mozilla.org/mozilla-central/rev/83296fc07093
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
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.

Attachment

General

Created:
Updated:
Size: