1275927 - Replace custom Sync success doorhanger with platform notification

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Ryan Feeley [:rfeeley]]

Attachment: sync-moment.png

Now that notifications work well, let's get all the custom UI out, including the code that goes with it.

Comment 1

[Ryan Feeley [:rfeeley]]

If possible, when clicked it should link to about:preferences#sync

Comment 2

[Edouard Oger [:eoger]]

Attachment: Bug 1275927 - Replace custom Sync success doorhanger with platform notification.

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

Comment 3

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8758455 [details]
Bug 1275927 - Replace custom Sync success doorhanger with platform notification.

https://reviewboard.mozilla.org/r/56664/#review53468

I think we should also be able to undo many of the changes added in bug 966098 - that bug explains why the 5 second delay and the check for the window being active - I'm not sure that any of those considerations apply using notifications (ie, we should be able to show it even if there is no Firefox window, or if it happens to be minimized.) We could then simply add the listener to bgrowserglue and remove the pref, delay, activate listener etc. I guess you should check with rfeeley that he's OK with that (but I assume he is - the entire point of notifications is that they aren't closely tied to a specific window)

::: browser/locales/en-US/chrome/browser/accounts.properties:28
(Diff revision 1)
>  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.
> +
> +syncStartNotification.title = Sync enabled
> +syncStartNotification.body = Firefox Sync will begin syncing momentarily.

Do we really want "Sync" here? The old strings didn't use it.

Comment 4

[Edouard Oger [:eoger]]

Comment on attachment 8758455 [details]
Bug 1275927 - Replace custom Sync success doorhanger with platform notification.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56664/diff/1-2/

Comment 5

[Edouard Oger [:eoger]]

More code removal :) The notification should be 1/2 sec less accurate since it triggers only on onverified instead of onverified -> syncstart, but it doesn't really matter since the state change is very fast.

Comment 6

[Ryan Feeley [:rfeeley]]

Mark's right. "Firefox will begin" not "Firefox Sync will begin"

Comment 7

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8758455 [details]
Bug 1275927 - Replace custom Sync success doorhanger with platform notification.

https://reviewboard.mozilla.org/r/56664/#review53800

This looks great, thanks - I don't think I need to look again just for the _openPreferences() addition.

::: browser/components/nsBrowserGlue.js:1844
(Diff revision 2)
>  
> +  _showSyncStartedDoorhanger: function () {
> +    let bundle = Services.strings.createBundle("chrome://browser/locale/accounts.properties");
> +    let title = bundle.GetStringFromName("syncStartNotification.title");
> +    let body = bundle.GetStringFromName("syncStartNotification.body");
> +    AlertsService.showAlertNotification(null, title, body);

Can we call this._openPreferences("sync") when the user clicks it?

::: browser/locales/en-US/chrome/browser/accounts.properties:27
(Diff revision 2)
>  # LOCALIZATION NOTE (verificationSentFull) - %S = Email address of user's Firefox Account
>  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.
> +
> +syncStartNotification.title = Sync enabled

Add a comment here indicating the strings are used in a notification shown after Sync is connected.

Comment 8

[Edouard Oger [:eoger]]

Comment on attachment 8758455 [details]
Bug 1275927 - Replace custom Sync success doorhanger with platform notification.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56664/diff/2-3/

Comment 9

[Carsten Book [:Tomcat]]

has problems to apply:

patching file browser/locales/en-US/chrome/browser/accounts.properties
Hunk #1 FAILED at 17
1 out of 1 hunks FAILED -- saving rejects to file browser/locales/en-US/chrome/browser/accounts.properties.rej
patch failed to apply

Comment 10

[Edouard Oger [:eoger]]

Comment on attachment 8758455 [details]
Bug 1275927 - Replace custom Sync success doorhanger with platform notification.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56664/diff/3-4/

Comment 11

[Edouard Oger [:eoger]]

Rebased against fx-team

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/8d3252104e46
Replace custom Sync success doorhanger with platform notification. r=markh

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8d3252104e46

Comment 14

[Azmina]

I have reproduced this bug with Firefox Nightly 49.0a1 (2016-05-26) on windows 8.1 , 64 bit

The bug's fix is verified on Aurora 49.0a2

Build ID : 20160709004037

User Agent : Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:49.0) Gecko/20100101 Firefox/49.0

[testday-20160708]