fix (systemaddon): #3058 refresh TopSitesFeed when TippyTopProvider init is complete #3088
Conversation
…vider init is complete
| switch (action.type) { | ||
| case at.INIT: |
There was a problem hiding this comment.
This could still fail due to a race condition, because NEW_TAB_LOAD could be fired before this._tippyTopProvider is done initializing. I think it might be better to do something like this inside refresh:
if (!this._tippyTopProvider.initialized) await this._tippyTopProvider.init()
// continue refreshingThere was a problem hiding this comment.
yeah, I was discussing with @Mardak on IRC: "so adding this refresh means it could refresh earlier than necessary or result in a second refresh after a first NEW_TAB_LOAD"
But we figured that might be better than holding up the refresh when a NEW_TAB_LOAD event comes in. Don't you think?
There was a problem hiding this comment.
@rlr and I did chat about this and decided that if we do have a NEW_TAB_LOAD it seemed undesirable for it to be blocked on a potentially unbounded amount of time for tippyTopProvider.init
There was a problem hiding this comment.
And on the flip side, if tippyTop does resolve sooner than NEW_TAB_LOAD, the state will already be updated and not refreshed due to the 15-minute check.
There was a problem hiding this comment.
Also, if we block NEW_TAB_LOAD, the section would be collapsed until there were sites. (Although that early, other things aren't quite loaded yet either including strings !)
Check out this 10x slowed-down video of a debug build with this patch (and my load-correctly-on-startup patch):
https://ed.agadak.net/as/activity-stream.startup.10x.webm
There was a problem hiding this comment.
👀 Cool video. How do you do that?
There was a problem hiding this comment.
I used Screenflow, but any screen recording software with editing should be good. (Oh it exports by default to mp4 and I used Miro video converter to make it webm)
There was a problem hiding this comment.
I suppose one downside of the "go ahead without waiting" is that it'll trigger fetching screenshots.
rlr
changed the title
There was a problem hiding this comment.
I'll file a separate issue about startup behavior, but I think this approach is good enough for the common case of people opening up a new tab not immediately on startup as the state will already be populated with an already-refreshed sites with tippy top data. In the less common case where toppyTopProvider.init() resolves after NEW_TAB_LOAD has already responded without waiting, the user would at least be able to see the sites and letter/hostname.
| switch (action.type) { | ||
| case at.INIT: |
There was a problem hiding this comment.
Also, if we block NEW_TAB_LOAD, the section would be collapsed until there were sites. (Although that early, other things aren't quite loaded yet either including strings !)
Check out this 10x slowed-down video of a debug build with this patch (and my load-correctly-on-startup patch):
https://ed.agadak.net/as/activity-stream.startup.10x.webm
Fixes #3058
r? @Mardak