Closed Bug 1397744 Opened 7 years ago Closed 7 years ago

Manual sync animation delay on startup

Categories

(Firefox :: Sync, defect, P2)

Product:
Firefox
Component:
Sync
Version:
55 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: rfeeley, Assigned: eoger)

Details

Attachments

(2 files)

manual-sync-startup-delay.mov
1.45 MB, video/quicktime
Details
Bug 1397744 - Show the Sync animation immediately after clicking the manual sync button.
59 bytes, text/x-review-board-request
markh
: review+
Details 
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
Does this reproduce on 55 or 56?
Flags: needinfo?(rfeeley)
(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.
Can the animation kick in earlier even if it's fake?
Flags: needinfo?(rfeeley)
That would we would show the syncing animation even if we are in an error state later however.
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.
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...
Priority: -- → P2
Assignee: nobody → eoger
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
> 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 on attachment 8910375 [details]
Bug 1397744 - Show the Sync animation immediately after clicking the manual sync button.

https://reviewboard.mozilla.org/r/181826/#review188274
Attachment #8910375 - Flags: review?(markh) → review+
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/11d5f873f409
Show the Sync animation immediately after clicking the manual sync button. r=markh
Backout by kwierso@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/aa97adfab999
Backed out changeset 11d5f873f409 for failures in browser_synced_tabs_menu.js a=backout
There was a race-condition in a test :)
Flags: needinfo?(eoger)
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8855ba3f65ba
Show the Sync animation immediately after clicking the manual sync button. r=markh
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
Flags: needinfo?(eoger)
Ok I have made changes to my patch, pushed to try multiples times, this should totally work now https://imgur.com/a/YevX3
Flags: needinfo?(eoger)
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/97fdb9d392e5
Show the Sync animation immediately after clicking the manual sync button. r=markh
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9ab9bde4021f
Show the Sync animation immediately after clicking the manual sync button. r=markh
Backout by archaeopteryx@coole-files.de:
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
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 -
Flags: needinfo?(eoger)
https://imgur.com/a/rmH95
Flags: needinfo?(eoger)
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fe8db1b3e361
Show the Sync animation immediately after clicking the manual sync button. r=markh
https://hg.mozilla.org/mozilla-central/rev/fe8db1b3e361
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: