Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(fix): Listener on task #321

Merged
merged 5 commits into from
Mar 11, 2020
Merged

(fix): Listener on task #321

merged 5 commits into from
Mar 11, 2020

Conversation

thomaszurkan-optimizely
Copy link
Contributor

Summary

  • Load the cached datafile in the datafile loader if the datafile is null from cdn. This avoids loading the cached datafile in the onPostExecute of the datafile loader which is on the main thread.

Issues

Copy link
Contributor

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

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

A fix for async init thread changes datafile loading for sync init.

Comment on lines 288 to 293
// if (isDatafileCached(context)) {
// String datafile = datafileHandler.loadSavedDatafile(context, datafileConfig);
// if (datafile != null) {
// return datafile;
// }
// }
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this function is shared for sync and async init. I see you move this to async download thread. It looks great.
This will change sync init behavior (it'll skip cache load) if we remove it. We probably need a separate path for sync and async for getDatafile().

Copy link
Contributor

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

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

LGTM

@thomaszurkan-optimizely thomaszurkan-optimizely merged commit c67fa20 into master Mar 11, 2020
@thomaszurkan-optimizely thomaszurkan-optimizely deleted the ListenerOnTask branch March 11, 2020 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants