We want : - Notifications when devices join the account - Add the "synced tabs" button directly to the hamburger menu when 2nd device connected. TLDR: implement slides 6 to 9 on the attached "Synced-Tabs-UX.pdf" file.

This wasn't done as part of the initial work for the new Synced Tabs button.

(In reply to Edouard Oger [:eoger] from comment #0) > We want : > - Notifications when devices join the account This sounds good. > - Add the "synced tabs" button directly to the hamburger menu when 2nd > device connected. I don't think we need to do this as the "synced tabs" button is now in the hamburger panel by default. I'm changing the description accordingly, but please change it back if I got something wrong.

> I'm changing the description accordingly, but please change it back if I got something wrong. LGTM. I was going to do something similar.

This patch depends on bug 1267760 patch. You can try this patch against my FxA-side changes using these prefs: > identity.fxaccounts.allowHttp;true > identity.fxaccounts.auth.uri;https://pushpayloads.dev.lcip.org/auth/v1 > identity.fxaccounts.remote.force_auth.uri;https://pushpayloads.dev.lcip.org/force_auth?service=sync&context=fx_desktop_v3 > identity.fxaccounts.remote.oauth.uri;https://oauth-pushpayloads.dev.lcip.org/v1 > identity.fxaccounts.remote.profile.uri;https://pushpayloads.dev.lcip.org/ > identity.fxaccounts.remote.signin.uri;https://pushpayloads.dev.lcip.org/signin?service=sync&context=fx_desktop_v3 > identity.fxaccounts.remote.signup.uri;https://pushpayloads.dev.lcip.org/signup?service=sync&context=fx_desktop_v3 > identity.fxaccounts.remote.webchannel.uri;https://pushpayloads.dev.lcip.org/ > identity.fxaccounts.settings.uri;https://pushpayloads.dev.lcip.org/settings?service=sync&context=fx_desktop_v3 > identity.sync.tokenserver.uri;https://pushpayloads.dev.lcip.org/syncserver/token/1.0/sync/1.5 We should also talk about what kind of push payload do we want (see attachment)

Comment on attachment 8746252 [details] [diff] [review] bug-1201335.patch Review of attachment 8746252 [details] [diff] [review]: ----------------------------------------------------------------- Looking good! ::: browser/locales/en-US/chrome/browser/accounts.properties @@ +24,5 @@ > verificationNotSentTitle = Unable to Send Verification > verificationNotSentFull = We are unable to send a verification mail at this time, please try again later. > + > +newDeviceTitle = %S is now syncing. Your synced tabs are now available in the menu. > +newDeviceSubtitle = To move synced tabs elsewhere in the browser Customize Firefox We may need to break this up into two strings, if we'd like to make "Customize Firefox" open the customize UI. It might be nice to have another link (or button) that opens the synced tabs panel or sidebar, too. But that's an rfeeley question. :-)

Comment on attachment 8746252 [details] [diff] [review] bug-1201335.patch Review of attachment 8746252 [details] [diff] [review]: ----------------------------------------------------------------- I think this looks good, but probably needs love from rfeeley before we can land it - specifically, "Customize Firefox" should do something (not clear what exactly, but probably magically leveraging the UI tour library - see, eg, https://dxr.mozilla.org/mozilla-central/source/browser/components/uitour/test/browser_UITour2.js#18, and pasting |UITour.getTarget(window, "customize").then(target => {UITour.initForBrowser(gBrowser.selectedBrowser, window); UITour.showInfo(window, target, "string 1", "string 2");})| into a browser console should give some idea of what we might be able to do. My other concern is with the nature of doorhangers and how they are so transient - eg, imagine a future where we have 2 links - one to "customize" and one to "devices" - as soon as the user clicks one the doorhanger will close - so how do they get back to the panel so they can click on the second? Another option rfeeley might like to explore is using the hamburger menu with a new badge - eg, maybe when a new device is attached we could use the menu status area and badge the button itself (much like the "needs verify" state, but with a friendlier badge) and keep this area alive for some period (say 24 hours) after attaching the new device? Could you please attach a screenshot of what this patch looks like, and then we can schedule a quick meeting with rfeeley to discuss it further. (so f+ on the patch itself, but I think there's still quite a bit more work) ::: browser/base/content/browser.xul @@ +405,5 @@ > flip="slide"> > <hbox class="sync-panel-outer"> > <image class="sync-panel-icon"/> > <vbox class="sync-panel-inner"> > + <description id="sync-panel-title" As Kit mentions, it seems likely that over time this will change from a simple "title + subtitle" model to one with links (eg, I think that we should land the "Customize Firefox" string doing *something* in this patch, and later I expect there will be an additional link to "manage devices" or similar. So I'm wondering if we should make this a deck (or otherwise create multiple elements and ensure only 1 is ever visible) so we get more flexibility in the layout). ::: services/fxaccounts/FxAccountsPush.js @@ +141,5 @@ > * Fired when the Push server sends a notification. > * > * @private > */ > + _onPushMessage(maybeMessage) { this should grow a test, probably in the existing services/fxaccounts/tests/xpcshell/test_push_service.js

(In reply to Mark Hammond [:markh] from comment #6) > Could you please attach a screenshot of what this patch looks like, and then > we can schedule a quick meeting with rfeeley to discuss it further. Ah - I see you uploaded one to http://recordit.co/wQeIrYsitp - Ryan, what do you think? To summarize my concerns above: * Should the "customize firefox" be a link that does something, such as opening the hamburger menu and highlighting the "customze" button? * Should we eventually show a link to manage their devices (once a UI for this exists?). If so (and in general), is the transient nature of the doorhanger a problem? eg, once they click on a link (or accidentally click somewhere else), the doorhanger will vanish with no way to get it back - so, eg, they click on the "customize" link to learn how to move the button, but are then unable to get back and discover the "manage devices" link. I'm concerned this might be confusing to the user. Do you have any immediate thoughts, and/or would you like to schedule a time to chat about this?

Patch updated, I added a test in services/fxaccounts/tests/xpcshell/test_push_service.js like mark suggested. I will update the UI parts after Ryan gives us his directions

I lost track of this issue, and should have updated my deck. I think it's best that we just use standard notifications like this one. Can notifications like this open native features? We could do a few things when users click the notification: 1. Open accounts.firefox.com Devices section (when it’s ready) 2. Open about:preferences#sync 3. Open Synced Tabs panel 4. Open Synced Tabs sidebar

If you're talking about native notifications, I think we can do pretty much everything. > new Notification("Test notification").addEventListener('click', function() { PanelUI.show(); }) This code opens the hamburger menu for eg.

(In reply to Ryan Feeley [:rfeeley] from comment #9) > I lost track of this issue, and should have updated my deck. I think it's > best that we just use standard notifications like this one. I'm concerned it's too transient (I think the cool-kids prefer "ephemeral"). I'm sure there's some other bug I can't find with a similar requirement where Chris and I discussed the same basic issue - that the user isn't sitting at the machine receiving the notification, they are on the device *sending* it - so it's tricky to make them aware. But meh - if Ryan goes for it, I'm not going to complain too much :) > We could do a few things when users click the notification: > 1. Open accounts.firefox.com Devices section (when it’s ready) > 2. Open about:preferences#sync > 3. Open Synced Tabs panel > 4. Open Synced Tabs sidebar Actually we can only do 1 - we just have to choose :) Is this really something we can do before the place we go has a way of managing devices? If so, we need a decision on the 1-4 (and if not, this is blocked by a device UI bug which we may or may not already have)

(In reply to Mark Hammond [:markh] from comment #11) > I'm concerned it's too transient (I think the cool-kids prefer "ephemeral"). I think it's the appropriate level of ephemerality. :) We do email users as well; this notification would be an extra measure. On a Mac at least, it appears alongside other system/application notifications, and persists in notification center until deleted. > > We could do a few things when users click the notification: > > 1. Open accounts.firefox.com Devices section (when it’s ready) > > 2. Open about:preferences#sync > > 3. Open Synced Tabs panel > > 4. Open Synced Tabs sidebar > > Actually we can only do 1 - we just have to choose :) Is this really > something we can do before the place we go has a way of managing devices? If > so, we need a decision on the 1-4 (and if not, this is blocked by a device > UI bug which we may or may not already have) This is a great question. Is there a point in having browser notifications about about new devices when we can't show them devices? We can only link to one thing, and in my mind, the only thing that makes sense is to take the user to a list of devices.

> the only thing that makes sense is to take the user to a list of devices. I agree, this seems like by far the best option. Given this as a clear rationale for "why", I think we can get the devices view shipped this quarter, and can work on that in parallel with the finer details of the push payloads here.

Review commit: https://reviewboard.mozilla.org/r/54380/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/54380/

Comment on attachment 8755063 [details] Bug 1201335 - Display notification when a new device is added to Sync account. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54380/diff/1-2/

Updated patch with native notifications (http://imgur.com/RdD7rPB)

We discussed this with Chris, he thinks that we should try to ship this as a nightly-only feature and I agree with him, it would allow us to find early bugs, validate our infrastructure and iterate easily. Ryan: Are you satisfied with the native notification (see screenshot in comment 16)? Is that okay to open the device manager which is also behind a flag (https://accounts.firefox.com/settings/devices?forceDeviceList=true) when clicking on the notification? Markh: How do you feel about landing this as a nightly-only feature?

https://reviewboard.mozilla.org/r/54380/#review51318 Looks good! I think we probably want a browser_fxaccounts test too to ensure that a notification UI is actually created on receipt of that observer notification. Regarding the nightly pref, I think that's fine, but for reasons I mention below I think we'd probably want that in browser-fxaccounts.js. ::: services/fxaccounts/FxAccountsPush.js:156 (Diff revision 2) > + > + if (message && message.command === ON_DEVICE_CONNECTED_NOTIFICATION) { > + Services.obs.notifyObservers(null, ON_DEVICE_CONNECTED_NOTIFICATION, message.data.deviceName); > + } else { > + // Use the empty signal to check the verification state of the account right away > - this.fxAccounts.checkVerificationStatus(); > + this.fxAccounts.checkVerificationStatus(); I'm thinking we should make the checkVerificationStatus only be called when there is no message and log for unrecognized commands. This would mean that in the future the server could happily send a new command that we don't yet handle and we would avoid entering checkVerificationStatus() for it. For this reason, I don't think we would want the nightly-only-pref-check code here - if we disable via that pref we probably *do not* want us to enter checkVerificationStatus() when we get the message.

Comment on attachment 8755063 [details] Bug 1201335 - Display notification when a new device is added to Sync account. As mentioned in IRC, the code will probably need to be in nsBrowserGlue.js so we don't get one notification for every window opened.

Comment on attachment 8755063 [details] Bug 1201335 - Display notification when a new device is added to Sync account. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54380/diff/2-3/

(In reply to Edouard Oger [:eoger] from comment #17) > We discussed this with Chris, he thinks that we should try to ship this as a > nightly-only feature and I agree with him, it would allow us to find early > bugs, validate our infrastructure and iterate easily. > > Ryan: Are you satisfied with the native notification (see screenshot in > comment 16)? Is that okay to open the device manager which is also behind a > flag (https://accounts.firefox.com/settings/devices?forceDeviceList=true) > when clicking on the notification? This seems like a great approach to me, and also drives the device management forward more.

https://reviewboard.mozilla.org/r/54380/#review51552 ::: browser/components/nsBrowserGlue.js:2449 (Diff revision 3) > + let win = RecentWindow.getMostRecentBrowserWindow({private: false}); > + if (!win) { > + return; > + } > + let Notification = win.Notification; > + let notification = new Notification(title, { body }); You can also use `AlertsService.showAlertNotification` from chrome code. It's XPCOM-y, so not nearly as elegant as the notification API (check out `_notifyNotificationsUpgrade` for an example). That way, you can show a notification even if no windows are open...assuming that's desirable, of course. :-) ::: services/fxaccounts/FxAccountsPush.js:148 (Diff revision 3) > * @private > */ > - _onPushMessage() { > + _onPushMessage(maybeMessage) { > this.log.trace("FxAccountsPushService _onPushMessage"); > - // Use this signal to check the verification state of the account right away > + let message; > + if (maybeMessage) { I'm sorry to do this to you, but the API is slightly different now. :-/ if (message.QueryInterface(Ci.nsIPushMessage).data) { message = message.data.json(); } `nsIPushMessage` is a wrapper around the `data` and `principal` (and always passed to `_onPushMessage`), and `nsIPushMessage.data` holds the data.

Comment on attachment 8755063 [details] Bug 1201335 - Display notification when a new device is added to Sync account. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54380/diff/3-4/

Thanks Kit!

(In reply to Ryan Feeley [:rfeeley] from comment #21) > > Ryan: Are you satisfied with the native notification (see screenshot in > > comment 16)? Is that okay to open the device manager which is also behind a > > flag (https://accounts.firefox.com/settings/devices?forceDeviceList=true) > > when clicking on the notification? > > This seems like a great approach to me, and also drives the device > management forward more. Oops - I missed that detail before. Ryan Kelly has some reservations about exposing that UI even to only nightly users: > rfkelly> some stuff on that view is half-done, e.g. the "last connected" timestamps do not get updated correctly right now > rfkelly> markh: I'm not opposed to it in principle, but we should talk explicitly about what MVP is for that page and whether the current version meets that criteria So it sounds like we would need to land this without that feature, then have another bug on file for deciding the MVP before we link to it. rfeeley, are you still OK with this feature landing without that?

Comment on attachment 8755063 [details] Bug 1201335 - Display notification when a new device is added to Sync account. https://reviewboard.mozilla.org/r/54380/#review51644 We need to work out if we can actually open the link yet :/. Also, weren't you working on a mochitest-browser test for this? ::: browser/components/nsBrowserGlue.js:48 (Diff revision 4) > "resource://gre/modules/NetUtil.jsm"); > > XPCOMUtils.defineLazyModuleGetter(this, "FileUtils", > "resource://gre/modules/FileUtils.jsm"); > > +var FxAccountsCommon = {}; I don't think we should import this module here just for the 1 constant. Tricking things to make it lazy also doesn't help in practice as it will still be used immediately after login. I think it would be fine to define ON_DEVICE_CONNECTED_NOTIFICATION as a constant string here with a comment that it's copied from FxAccountCommon to avoid dragging that module in. ::: browser/components/nsBrowserGlue.js:2448 (Diff revision 4) > + let url = Services.urlFormatter.formatURLPref("identity.fxaccounts.settings.devices.uri"); > + > + function clickCallback(subject, topic, data) { > + if (topic != "alertclickcallback") > + return; > + let win = RecentWindow.getMostRecentBrowserWindow(); As Kit mentions, there might be no window - and I think that this code should probably/possibly arrange to open one in that case - but I'm surprised I can't find code already in this file to handle that case (although most of the other cases are handling the case when the user must have been interacting with a window to get there in the first place) IOW, I believe a user would simply consider things broken if clicking that sometimes does something and sometimes silently does nothing. However, given the fact that rfkelly isn't comfortable with exposing that yet, that might all be moot. ::: browser/locales/en-US/chrome/browser/accounts.properties:28 (Diff revision 4) > verificationSentFull = A verification link has been sent to %S. Please check your email and click the link to begin syncing. > verificationNotSentTitle = Unable to Send Verification > verificationNotSentFull = We are unable to send a verification mail at this time, please try again later. > + > +deviceConnectedTitle = Firefox Sync > +deviceConnectedBody = You are now syncing with %S. If we *do* have the click, I think we should check with rfeeley that this copy is OK - ie, that he doesn't also want some visual clue that clicking the notification will open devices. IOW, I'm wondering if we want something like "click to manage your devices" appended.

> However, given the fact that rfkelly isn't comfortable with exposing that yet, that might all be moot. To help us have this conversation, I've made a new github milestone and added the current known issues with the devices view: https://waffle.io/mozilla/fxa?milestone=FxA-89:%20devices%20view The biggest issues in my mind are: * We've currently disabled tracking the last-connected timestamp, because it produced huge db write load * Android and iOS devices will not show up in the list yet * It's apparently pretty easy to produce duplicate device entries in that list, for reasons we don't fully understand

It doesn't sound ready for even Nightly yet. Any chance it can be behind an about:config pref?

Demoting to P3 until device manager work is closer to shipping (since we decided this work is blocked on that work).

https://blog.mozilla.org/services/2016/10/06/device-management-coming-to-your-firefox-account/ We should reconsider this soon :)

From triage: definitely valuable; something we'll consider landing this quarter.

In FxA train 71 [1], the guys landed push notification to notify the user's browsers that a new device was added to their account. The notifications are purely functional to eliminate lag when a login is confirmed but speaking to rfk, we believe that 80-90% of the work is done for this bug and we would just need to work on the UI/UX portion of the notification. Things to think about: - Do we want to highlight the device that was added in FxA device manager when a user clicks on the notification? - What happens if you're on the device that was added? Do we want to also send the user to the device manager? Is that the desired experience? [1] https://mail.mozilla.org/pipermail/dev-fxacct/2016-October/002094.html

> - Do we want to highlight the device that was added in FxA device manager > when a user clicks on the notification? We know that the vast majority of multi-device users have only two devices, so I'm not sure it would warrant the work (and would require a change to the URL in the notification). > - What happens if you're on the device that was added? Do we want to also > send the user to the device manager? Is that the desired experience? When the user signs in to Sync on their second device, we will do our best to onboard them into learning about sending/receiving tabs. I don't think we'd tell them about device management in onboarding, but perhaps via email if there are security events.

When do we expect the device manager to hit 100%?

It should be at 100% as of earlier today :-)

Is this actionable again?

Yes, I think we now have all the pieces necessary to do a good job of this.

Looking forward to this!

Updated so the connected message is more consistent with the disconnect message.

Updated with: - Rebased - Browser chrome tests - New wording from rfeeley

Comment on attachment 8755063 [details] Bug 1201335 - Display notification when a new device is added to Sync account. https://reviewboard.mozilla.org/r/54380/#review102770 Looks great, and thanks for the tests :) But please add a PD header to the new test files: <!-- Any copyright is dedicated to the Public Domain. http://creativecommons.org/publicdomain/zero/1.0/ -->

Comment on attachment 8755063 [details] Bug 1201335 - Display notification when a new device is added to Sync account. https://reviewboard.mozilla.org/r/54380/#review102770 (oops - obviously don't use HTML comment blocks in the JS test files ;)

Pushed by eoger@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c557bb902365 Display notification when a new device is added to Sync account. r=markh

Note for the future: when using parameters in a string, add a comment explaining what's the replacement, especially if it's not intuitive. https://hg.mozilla.org/mozilla-central/diff/c557bb902365/browser/locales/en-US/chrome/browser/accounts.properties Localizers will only see "deviceConnectedBody = This computer is now syncing with %S.", not the bug, and will have no idea what %S will become. I was going to object the use of hard-coded Firefox Sync in strings, but apparently it's not the first case.