Add minimum display time to Syncing… and spinner

RESOLVED FIXED in Firefox 49

Status

()

Firefox
Sync
P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: rfeeley, Assigned: kitcambridge)

Tracking

(Blocks: 2 bugs)

unspecified
Firefox 49
Points:
---
Dependency tree / graph
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 8707028 [details]
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.
Flags: firefox-backlog+
Priority: -- → P2
Blocks: 1239084
Hijacking this bug for minimum display time and spinning out the animation to a separate bug.
Summary: Add minimum display time and animation to Syncing… → Add minimum display time to Syncing… and spinner
Blocks: 1239535
(Reporter)

Comment 2

2 years ago
Similar treatment done by Mac App Store on web pages: https://search.itunes.apple.com/WebObjects/MZContentLink.woa/wa/link?path=mac%2fPDFScanner
Assignee: nobody → kcambridge
Status: NEW → ASSIGNED
Created attachment 8712459 [details]
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 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?
Attachment #8712459 - Flags: feedback?(markh)
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 :)
(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)

Updated

2 years ago
Attachment #8712459 - Flags: feedback?(markh) → feedback+
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.
(Assignee)

Updated

2 years ago
Attachment #8712459 - Attachment description: MozReview Request: Bug 1239042 - Show sync status for a minimum of 1.6s. → MozReview Request: Bug 1239042 - Show sync status for a minimum of 1.6s. r?markh
Attachment #8712459 - Flags: feedback+ → review?(markh)
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 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!
Attachment #8712459 - Flags: review?(markh) → review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
It would help if I updated `browser/base/content/test/general/browser_syncui.js`. ;-)
Keywords: checkin-needed
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/
Attachment #8712459 - Attachment description: MozReview Request: Bug 1239042 - Show sync status for a minimum of 1.6s. r?markh → MozReview Request: Bug 1239042 - Show sync status for a minimum of 1.6s. r=markh
There we go: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5b42ff33cb1
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 13

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/e8a80985911b
Keywords: checkin-needed
(Reporter)

Comment 14

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

Updated

2 years ago
Blocks: 1257587

Updated

2 years ago
Blocks: 1257995

Updated

2 years ago
No longer blocks: 1257587
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/
Attachment #8712459 - Attachment description: MozReview Request: Bug 1239042 - Show sync status for a minimum of 1.6s. r=markh → MozReview Request: Bug 1239042 - Show sync status for a minimum of 1.6s. r?markh
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?
Attachment #8712459 - Flags: review+ → review?(markh)
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.
Attachment #8712459 - Flags: review?(markh)
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/
Attachment #8712459 - Flags: review?(markh)
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.
(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.
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 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/
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

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/d14d6f68201e
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. :-)
Attachment #8712459 - Flags: review?(markh) → review+

Comment 27

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d14d6f68201e
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
You need to log in before you can comment on or make changes to this bug.