1239042 - Add minimum display time to Syncing… and spinner

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P2)

Description

[Ryan Feeley [:rfeeley]]

Attachment: Simulation

Very often the user will click Sync Now and will only see Syncing… appear for a split second.

We should add a minimum display time of 1600ms and four frames of animation to the ellipsis character.

Syncing
Syncing.
Syncing..
Syncing… (or if that looks odd, Syncing...)

The animation should run twice. Each step should taking 200ms for a total of 1600ms.

Comment 1

[Chris Karlof [:ckarlof]]

Hijacking this bug for minimum display time and spinning out the animation to a separate bug.

Comment 2

[Ryan Feeley [:rfeeley]]

Similar treatment done by Mac App Store on web pages: https://search.itunes.apple.com/WebObjects/MZContentLink.woa/wa/link?path=mac%2fPDFScanner

Comment 3

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1239042 - Show sync status for a minimum of 1.6s. r?markh

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

Comment 4

[Lina Butler [:lina]]

Comment on attachment 8712459 [details]
MozReview Request: Bug 1239042 - Show sync status for a minimum of 1.6s. r?markh

This is a quick way to do it. (Hard-coded constants FTW!) It'll affect the "Sync Now" button in the hamburger menu, since it also observes the `sync-status` element.

Another approach is to decouple the observer in "Synced Tabs". That seems better, but we need to be extra careful that we keep the state consistent with the "real" sync status. WDYT?

Comment 5

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

https://reviewboard.mozilla.org/r/32549/#review29525

I've a niggling concern about the delays cascading here - part of me wants to say it's not going to be a problem in practice, but another part thinks it might be - eg, we sync on each bookmark change, so someone managing their bookmarks might implicitly trigger many bookmark syncs, which if they complete in less than 1.6 seconds might have the delay stack up.

I could be wrong, but if I'm not, it might be worth replacing that \_numActiveSyncs complexity with some complexity to kill an existing timer when a new one is started. What do you think?

For diagnostics, it might also be worth writing .debug entries to the log as we start a new timer and as each timer is resolved - these entries will appear in sync-logs.

(also not sure how to flag f+ in reviewboard - "ship it" doesn't sound correct for feedback.)

::: browser/base/content/browser-syncui.js:183
(Diff revision 1)
>      if (++this._numActiveSyncTasks == 1) {

I'm inclined to think we should kill the \_numActiveSyncTasks variable here too - it was used to support the now-defunct readinglist.

::: browser/base/content/browser-syncui.js:185
(Diff revision 1)
> +        new Promise(resolve => setTimeout(resolve, 1600))

If I'm reading this correctly, a user mashing the sync button many times will cause the delay to get quite long - I'm not sure if I think that's a problem yet though :)

Comment 6

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

(In reply to Kit Cambridge [:kitcambridge] from comment #4)
> Another approach is to decouple the observer in "Synced Tabs". That seems
> better, but we need to be extra careful that we keep the state consistent
> with the "real" sync status. WDYT?

I think your approach is good and that the hamburger menu *should* see this increased time before the spinner stops (ie, all sync indicators should remain consistent)

Comment 7

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

I think we can make this simpler still - the request is more to say "the spinner should spin for a minimum time" rather than "the spinner should spin for some time after sync completes".

IOW, if the spinner is still spinning (due to this delay) when a new sync starts, there's no reason to *extend* the time further. I *think* that makes things easier.

Comment 8

[Lina Butler [:lina]]

Comment on attachment 8712459 [details]
MozReview Request: Bug 1239042 - Show sync status for a minimum of 1.6s. r?markh

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

Comment 9

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

Comment on attachment 8712459 [details]
MozReview Request: Bug 1239042 - Show sync status for a minimum of 1.6s. r?markh

https://reviewboard.mozilla.org/r/32549/#review32789

LGTM, thanks!

Comment 10

[Lina Butler [:lina]]

It would help if I updated `browser/base/content/test/general/browser_syncui.js`. ;-)

Comment 11

[Lina Butler [:lina]]

Comment on attachment 8712459 [details]
MozReview Request: Bug 1239042 - Show sync status for a minimum of 1.6s. r?markh

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

Comment 12

[Lina Butler [:lina]]

There we go: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5b42ff33cb1

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/e8a80985911b

Comment 14

[Ryan Feeley [:rfeeley]]

(In reply to Kit Cambridge [:kitcambridge] from comment #12)
> There we go:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5b42ff33cb1

I've been using it all day! Great work!

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Backed out for frequent Windows intermittent browser_syncui.js failures. Seemed to hit mostly on PGO builds, but did occasionally strike on regular opt as well. WinXP was the most regular offender, but Win7 also got in the act.
https://hg.mozilla.org/integration/fx-team/rev/ccfb45a887bf

https://treeherder.mozilla.org/logviewer.html#?job_id=7538012&repo=fx-team

Comment 16

[Lina Butler [:lina]]

Comment on attachment 8712459 [details]
MozReview Request: Bug 1239042 - Show sync status for a minimum of 1.6s. r?markh

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

Comment 17

[Lina Butler [:lina]]

Comment on attachment 8712459 [details]
MozReview Request: Bug 1239042 - Show sync status for a minimum of 1.6s. r?markh

As we discussed, I reverted the test changes. That should squelch the FxA and Sync UI failures for PGO builds: https://treeherder.mozilla.org/#/jobs?repo=try&revision=499e422a6c58

Mark, could you take another look, please?

Comment 18

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

Comment on attachment 8712459 [details]
MozReview Request: Bug 1239042 - Show sync status for a minimum of 1.6s. r?markh

https://reviewboard.mozilla.org/r/32549/#review45063

I thought that the earlier patch struck issues with the browser_syncui test? I know we agree to skip specific testing of this animation, but I thought there may still be test changes needed for existing tests?

::: browser/base/content/browser-syncui.js:160
(Diff revision 4)
>      this._promiseUpdateUI().catch(err => {
>        this.log.error("updateUI failed", err);
>      })
>    },
>  
> +  _hideBroadcaster(id, hidden) {

I'm that keen on this change TBH - it means a typo in an element ID will not be picked up, and I see no reason a valid ID will ever be null here (apart from the "browser is tearing down" case, which is why the gBrowser checks exist). Did you strike an actual problem that requires this?

::: browser/base/content/browser-syncui.js:206
(Diff revision 4)
> -    if (++this._numActiveSyncTasks == 1) {
> +
> +    clearTimeout(this._syncAnimationTimer);
> +    this._syncStartTime = Date.now();
> +
> -      let broadcaster = document.getElementById("sync-status");
> +    let broadcaster = document.getElementById("sync-status");
> +    if (broadcaster) {

ditto here and below - we've checked gBrowser, so I'm interested to know when broadcaster may be null? And if it *is* null, I doubt that falling through to updateUI is the right thing to do.

Comment 19

[Lina Butler [:lina]]

Comment on attachment 8712459 [details]
MozReview Request: Bug 1239042 - Show sync status for a minimum of 1.6s. r?markh

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

Comment 20

[Lina Butler [:lina]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=72a1212cda0b

FWIW, the `TypeError: document.getElementById(...) is null` issue is still there, if you search in the log for a run that failed for unrelated reasons: https://treeherder.mozilla.org/logviewer.html#?job_id=19894786&repo=try I don't know if it fails on the green runs, too, since Try silences logs when the test suite passes.

I wish this were easier to investigate. :-( I can't reproduce it locally on my Windows box, even with PGO. I've yet to set up or try http://rr-project.org/ on my Linux VM; maybe it could help. But that seems like a yak for another day.

Comment 21

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

(In reply to Kit Cambridge [:kitcambridge] from comment #20)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=72a1212cda0b
> 
> FWIW, the `TypeError: document.getElementById(...) is null` issue is still
> there, if you search in the log for a run that failed for unrelated reasons:
> https://treeherder.mozilla.org/logviewer.html#?job_id=19894786&repo=try I
> don't know if it fails on the green runs, too, since Try silences logs when
> the test suite passes.

The implication of that though is that these messages aren't actually causing any problems, just log noise in tests. If that's correct then I think we should just leave it alone and have those messages keep appearing - hopefully that will still cause someone to prioritize working out what's going on here. If that's *not* correct (ie, that your patch is somehow making these existing messages cause failures) then I do think we need to dig a little deeper into this.

Comment 22

[Lina Butler [:lina]]

It has shown up on other runs before; the test that I added to the original patch just exposed it: https://treeherder.mozilla.org/logviewer.html#?job_id=26644359&repo=mozilla-inbound, https://treeherder.mozilla.org/logviewer.html#?job_id=26635532&repo=mozilla-inbound, https://treeherder.mozilla.org/logviewer.html#?job_id=26619189&repo=mozilla-inbound

Now that I've removed the test, of course, there's no visible problem. :-) But I'm stumped why it sometimes fails to find the broadcasters, even when there's a gBrowser.

Comment 23

[Lina Butler [:lina]]

Comment on attachment 8712459 [details]
MozReview Request: Bug 1239042 - Show sync status for a minimum of 1.6s. r?markh

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

Comment 24

[Lina Butler [:lina]]

Try push without test: https://treeherder.mozilla.org/#/jobs?repo=try&revision=61a3dcd82a3b
With test: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7f62880fde2

Looks like that wasn't the root cause. :-(

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/d14d6f68201e

Comment 26

[Lina Butler [:lina]]

Comment on attachment 8712459 [details]
MozReview Request: Bug 1239042 - Show sync status for a minimum of 1.6s. r?markh

Mark rubber-stamped this over IRC. :-)

Comment 27

[Carsten Book [:Tomcat]]

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