Replace custom Sync success doorhanger with platform notification

RESOLVED FIXED in Firefox 49

Status

()

Firefox
Sync
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: rfeeley, Assigned: eoger)

Tracking

unspecified
Firefox 49
Points:
---
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox49 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
Created attachment 8756880 [details]
sync-moment.png

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

Updated

2 years ago
Flags: firefox-backlog+
(Reporter)

Comment 1

2 years ago
If possible, when clicked it should link to about:preferences#sync
(Assignee)

Updated

2 years ago
Assignee: nobody → edouard.oger
Status: NEW → ASSIGNED
(Assignee)

Comment 2

2 years ago
Created attachment 8758455 [details]
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/
Attachment #8758455 - Flags: review?(markh)

Updated

2 years ago
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.
(Assignee)

Comment 4

2 years ago
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)
(Assignee)

Comment 5

2 years ago
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.
(Reporter)

Comment 6

2 years ago
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+
(Assignee)

Comment 8

2 years ago
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/
(Assignee)

Updated

2 years ago
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
(Assignee)

Comment 10

2 years ago
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.
(Assignee)

Comment 11

2 years ago
Rebased against fx-team
Flags: needinfo?(edouard.oger)
Keywords: checkin-needed

Comment 12

2 years ago
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

Comment 13

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8d3252104e46
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49

Updated

2 years ago
See Also: → bug 1285751

Comment 14

2 years ago
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.