1397744 - Manual sync animation delay on startup

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P2)

Description

[Ryan Feeley [:rfeeley]]

Attachment: manual-sync-startup-delay.mov

STEPS TO REPRODUCE
1. Restart Nightly
2. Wait under N seconds (N is I think about 10 or 20 seconds, before the first sync begins)
3. Click manual sync in the hamburger menu

EXPECTED RESULTS
- Animates instantly

ACTUAL RESULTS
- There's a noticeable delay

Comment 1

[:Gijs (he/him)]

Does this reproduce on 55 or 56?

Comment 2

[:Gijs (he/him)]

(In reply to :Gijs from comment #1)
> Does this reproduce on 55 or 56?

I see the same thing on 56, at least. I expect this has "always" been the case - my understanding is the sync code delays initialization deliberately to improve startup performance.

Comment 3

[Ryan Feeley [:rfeeley]]

Can the animation kick in earlier even if it's fake?

Comment 4

[Edouard Oger [:eoger]]

That would we would show the syncing animation even if we are in an error state later however.

Comment 5

[Ryan Feeley [:rfeeley]]

I think that would be okay. From a user POV, the spinning icon probably also includes "connecting to server". If it then return an error, that would be okay.

Comment 6

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

This delay is during the initial FxA dance while we get certificates and a token - sync itself doesn't know it is about to run. It has always been there, and TBH I've been waiting for Ryan to notice :) I don't see any reason we can't do something hacky here...

Comment 7

[Edouard Oger [:eoger]]

Attachment: Bug 1397744 - Show the Sync animation immediately after clicking the manual sync button.

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

Comment 8

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

Comment on attachment 8910375 [details]
Bug 1397744 - Show the Sync animation immediately after clicking the manual sync button.

https://reviewboard.mozilla.org/r/181826/#review187802

::: browser/base/content/browser-sync.js:532
(Diff revision 1)
> -      setTimeout(() => Weave.Service.errorHandler.syncAndReportErrors(), 0);
> +      setTimeout(async () => {
> +        try {
> +          this.updateSyncStatus({ syncing: true });
> +          await Weave.Service.errorHandler.syncAndReportErrors();
> +        } finally {
> +          this.updateSyncStatus({ syncing: false });

Do we actually need to do this? I'd have thought we get the normal :finished notification which would stop the animation

Comment 9

[Edouard Oger [:eoger]]

> Do we actually need to do this? I'd have thought we get the normal :finished notification which would stop the animation

If login throws we will spin indefinitely, but login is using Utils.catch, so forget my comment I'll do the change :)

Comment 10

[Edouard Oger [:eoger]]

Comment on attachment 8910375 [details]
Bug 1397744 - Show the Sync animation immediately after clicking the manual sync button.

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

Comment 11

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

Comment on attachment 8910375 [details]
Bug 1397744 - Show the Sync animation immediately after clicking the manual sync button.

https://reviewboard.mozilla.org/r/181826/#review188274

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/11d5f873f409
Show the Sync animation immediately after clicking the manual sync button. r=markh

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=133621740&repo=autoland

Comment 14

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aa97adfab999
Backed out changeset 11d5f873f409 for failures in browser_synced_tabs_menu.js a=backout

Comment 15

[Edouard Oger [:eoger]]

Comment on attachment 8910375 [details]
Bug 1397744 - Show the Sync animation immediately after clicking the manual sync button.

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

Comment 16

[Edouard Oger [:eoger]]

There was a race-condition in a test :)

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8855ba3f65ba
Show the Sync animation immediately after clicking the manual sync button. r=markh

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing browser-chrome's browser/components/customizableui/test/browser_synced_tabs_menu.js:

https://hg.mozilla.org/integration/autoland/rev/a4c7a0be983302308f4485269de5218b66af1745

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=8855ba3f65ba482513c0bfe322b351d4555d8619&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=133869028&repo=autoland

11:08:37     INFO - Entering test bound 
11:08:37     INFO - Buffered messages logged at 11:08:37
11:08:37     INFO - TEST-PASS | browser/components/customizableui/test/browser_synced_tabs_menu.js | Sync Panel is in view - 
11:08:37     INFO - TEST-PASS | browser/components/customizableui/test/browser_synced_tabs_menu.js | main pane is visible - 
11:08:37     INFO - TEST-PASS | browser/components/customizableui/test/browser_synced_tabs_menu.js | first deck entry is visible - 
11:08:37     INFO - Buffered messages finished
11:08:37     INFO - TEST-UNEXPECTED-FAIL | browser/components/customizableui/test/browser_synced_tabs_menu.js | clicking the button called the correct function - 
11:08:37     INFO - Stack trace:
11:08:37     INFO - chrome://mochitests/content/browser/browser/components/customizableui/test/browser_synced_tabs_menu.js:null:250
11:08:37     INFO - Not taking screenshot here: see the one that was previously logged
11:08:37     INFO - TEST-UNEXPECTED-FAIL | browser/components/customizableui/test/browser_synced_tabs_menu.js | no-clients deck entry is visible - Got 2, expected 3
11:08:37     INFO - Stack trace:
11:08:37     INFO - chrome://mochikit/content/browser-test.js:test_is:1011
11:08:37     INFO - chrome://mochitests/content/browser/browser/components/customizableui/test/browser_synced_tabs_menu.js:null:258

Comment 19

[Edouard Oger [:eoger]]

Comment on attachment 8910375 [details]
Bug 1397744 - Show the Sync animation immediately after clicking the manual sync button.

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

Comment 20

[Edouard Oger [:eoger]]

Ok I have made changes to my patch, pushed to try multiples times, this should totally work now https://imgur.com/a/YevX3

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/97fdb9d392e5
Show the Sync animation immediately after clicking the manual sync button. r=markh

Comment 22

[Edouard Oger [:eoger]]

Comment on attachment 8910375 [details]
Bug 1397744 - Show the Sync animation immediately after clicking the manual sync button.

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

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9ab9bde4021f
Show the Sync animation immediately after clicking the manual sync button. r=markh

Comment 24

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e47c500744af
Backed out changeset 97fdb9d392e5 for failing browser-chrome's browser/components/customizableui/test/browser_synced_tabs_menu.js. r=backout

Comment 25

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing browser-chrome's browser/components/customizableui/test/browser_synced_tabs_menu.js:

https://hg.mozilla.org/integration/autoland/rev/698c6c81677c7c4089a34c30a0b6d76042e53d18

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=9ab9bde4021f469b36c2bdd1abd254f696abfc51&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=134988061&repo=autoland

> [task 2017-10-04T19:46:10.811Z] 19:46:10     INFO - TEST-UNEXPECTED-FAIL | browser/components/customizableui/test/browser_synced_tabs_menu.js | test left unexpected property on window: UIState -

Comment 26

[Edouard Oger [:eoger]]

https://imgur.com/a/rmH95

Comment 27

[Edouard Oger [:eoger]]

Comment on attachment 8910375 [details]
Bug 1397744 - Show the Sync animation immediately after clicking the manual sync button.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/181826/diff/5-6/

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fe8db1b3e361
Show the Sync animation immediately after clicking the manual sync button. r=markh

Comment 29

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/fe8db1b3e361