Closed Bug 1275927 Opened 4 years ago Closed 3 years ago

Replace custom Sync success doorhanger with platform notification

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 49
Tracking Status
firefox49 --- fixed

People

(Reporter: rfeeley, Assigned: eoger)

References

Details

Attachments

(2 files)

Attached image sync-moment.png
Now that notifications work well, let's get all the custom UI out, including the code that goes with it.
Flags: firefox-backlog+
If possible, when clicked it should link to about:preferences#sync
Assignee: nobody → edouard.oger
Status: NEW → ASSIGNED
Attachment #8758455 - Flags: review?(markh)
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 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/
Attachment #8758455 - Flags: review?(markh)
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.
Mark's right. "Firefox will begin" not "Firefox Sync will begin"
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.
Attachment #8758455 - Flags: review?(markh) → review+
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/
Keywords: checkin-needed
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
Flags: needinfo?(edouard.oger)
Keywords: checkin-needed
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/
Attachment #8758455 - Attachment description: MozReview Request: Bug 1275927 - Replace custom Sync success doorhanger with platform notification. r?markh → Bug 1275927 - Replace custom Sync success doorhanger with platform notification.
Rebased against fx-team
Flags: needinfo?(edouard.oger)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/8d3252104e46
Replace custom Sync success doorhanger with platform notification. r=markh
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8d3252104e46
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
See Also: → 1285751
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]
You need to log in before you can comment on or make changes to this bug.