Skip to content
This repository was archived by the owner on Feb 29, 2020. It is now read-only.

Comments

fix (systemaddon): #3377 dont skip refresh if already refreshing#3386

Merged
Mardak merged 1 commit intomozilla:masterfrom
rlr:gh3377/topsites-race
Sep 8, 2017
Merged

fix (systemaddon): #3377 dont skip refresh if already refreshing#3386
Mardak merged 1 commit intomozilla:masterfrom
rlr:gh3377/topsites-race

Conversation

@rlr
Copy link
Contributor

@rlr rlr commented Sep 8, 2017

This is one (simple) way to fix #3377 the problem.

The downside is that TippyTop provider can be initialized twice in some cases, but that does no harm other than wasting a few cycles.

Another option is to not call refresh() on INIT and wait until NEW_TAB_LOAD. Downside of that is the first tab load might take more time.

There are more complex solutions. Like we could have some sort of list of targets that need to be refreshed when the current refresh is complete.

@AdamHillier @Mardak thoughts?

@AdamHillier
Copy link
Contributor

This seems reasonable as long as there isn't a problem with initialising twice. I mean the actual fix in my patch was ensuring initialisation happens before refresh, which this doesn't affect.

@Mardak
Copy link
Member

Mardak commented Sep 8, 2017

Well, the fix here in addition to having TippyTop initialize twice, it also causes refresh to happen twice (or each additional refresh+init). We can relatively easily fix the multi-init by having init know it's initializing and return a promise for when the initial init finishes, but this still has all refresh callers fully refresh by calling getLinksWithDefaults.

I'll think about this a bit more…

Copy link
Member

@Mardak Mardak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eh. let's take it for the export. should be a corner case………

@Mardak Mardak merged commit 5aa91af into mozilla:master Sep 8, 2017
@as-pine-proxy
Copy link
Collaborator

@rlr rlr deleted the gh3377/topsites-race branch September 21, 2017 14:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Early new tab pages might not have top sites

4 participants