Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2975693002:
Add AwVariationsSeedFetchService and refactory VariationsSeedFetcher (Closed)
DescriptionAdd AwVariationsSeedFetchService and refactory VariationsSeedFetcher
The CL add the AwVariationsSeedFetchService which is a JobService to fetch Finch
seed data from the Finch Server. It reuse some of the code from the
VariationsSeedFetcher, so I refactory the class to expose some reusable
function. The download is triggered by AwVariationsConfigurationService which is
a bound Service to managing the Finch seed data system-wide. The fetched seed
data is going to store in the local directory belongs to the Android WebView.
This is just a prototype of the Variations Seed Fetch Service which is one part
of the work of adding Finch to Android WebView, so there will be another CL
related to the AwVariationsConfigurationService.
BUG=733857
Review-Url: https://codereview.chromium.org/2975693002
Cr-Commit-Position: refs/heads/master@{#488719}
Committed: https://chromium.googlesource.com/chromium/src/+/f7857086902db2e23fa374fce4b8ee8378f48e9d
Patch Set 1 #Patch Set 2 : Add service to Manifest #Patch Set 3 : Update comments and store seed class #Patch Set 4 : Add Seed Preference to store seed independently #
Total comments: 30
Patch Set 5 : Update for comments of Patch 4 #
Total comments: 17
Patch Set 6 : Update for comments of Patch 5 and fix test failure #
Total comments: 29
Patch Set 7 : Update for comments of Patch 6 #
Total comments: 19
Patch Set 8 : Update for comments of Patch 7 #
Total comments: 34
Patch Set 9 : Update for comments of Patch 8 #Patch Set 10 : Update a bug in deleting file #
Total comments: 2
Patch Set 11 : Update for comments of Patch 10 #Patch Set 12 : Add annotation to ignore file.delete() warning #Patch Set 13 : Add imports for SuppressFBWarnings #
Total comments: 2
Patch Set 14 : Update for comments of Patch 13 #
Total comments: 1
Patch Set 15 : Update unit tests for read and write seed data #
Total comments: 2
Patch Set 16 : Update code to delete test file. #
Total comments: 4
Patch Set 17 : Update to see if the proguard fails the test #Patch Set 18 : Update for comments of Patch 16 #Patch Set 19 : Try MainDex to solve test problem #Patch Set 20 : Try if bots don't build service #Patch Set 21 : Try verbose proguard output #Patch Set 22 : Add MinAndroidSdkLevel to ignore tests below L #
Total comments: 1
Patch Set 23 : Update for comments for Patch 22 #
Total comments: 4
Patch Set 24 : Update for comments of Patch 23 #Patch Set 25 : Fix typo in comments #Messages
Total messages: 114 (67 generated)
The CQ bit was checked by yiyuny@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add AwVariationsSeedFetchService and refactory VariationsSeedFetcher BUG=733857 ========== to ========== Add AwVariationsSeedFetchService and refactory VariationsSeedFetcher BUG=733857 ==========
yiyuny@google.com changed reviewers: + asvitkine@chromium.org, paulmiller@chromium.org
The CQ bit was checked by yiyuny@google.com to run a CQ dry run
On 2017/07/08 00:08:05, yiyuny wrote: > The CQ bit was checked by mailto:yiyuny@google.com to run a CQ dry run ptal
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yiyuny@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
In your commit message: s/refactory/refactor/. Also say a little more in the body about what you're doing. Say how WebView's seed fetching design is different than Chrome's, and that we need to refactor VariationsSeedFetcher to support the diverging seed storage code. https://codereview.chromium.org/2975693002/diff/60001/android_webview/apk/jav... File android_webview/apk/java/AndroidManifest.xml (right): https://codereview.chromium.org/2975693002/diff/60001/android_webview/apk/jav... android_webview/apk/java/AndroidManifest.xml:41: <service android:name="org.chromium.android_webview.AwVariationsSeedFetchService" unindent https://codereview.chromium.org/2975693002/diff/60001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java (right): https://codereview.chromium.org/2975693002/diff/60001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:27: * Job Service to fetch seed in the background which guarantees to get the job done. This comment is confusing. Explain what "seed" means, and remove "guarantees to get the job done". (I don't think anything about this is guaranteed.) https://codereview.chromium.org/2975693002/diff/60001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:34: public static final String SEED_DATA_PREF_FILENAME = "variations_seed_pref"; These files should go in the "app_webview" subdirectory, where WebView puts all its other files like cookies and the UMA GUID. (E.g. `adb shell ls /data/data/com.google.android.gm/app_webview`.) https://codereview.chromium.org/2975693002/diff/60001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:47: return false; Shouldn't this stop the FetchFinchSeedDataTask? https://codereview.chromium.org/2975693002/diff/60001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:54: FetchFinchSeedDataTask(JobParameters params, JobService svc) { It seems strange to declare the subclass static, but then keep a "this" reference anyway. Why not just make the class non-static? https://codereview.chromium.org/2975693002/diff/60001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:71: private static void fetchSeed(String restrictMode) { Remove the argument, since it's only ever called with "". https://codereview.chromium.org/2975693002/diff/60001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:74: synchronized (sLock) { If multiple jobs are scheduled at the same time, this will run all of them, serially. But we only need one of them to run. What about using tryLock(), and just not doing the fetch if we don't get the lock? https://codereview.chromium.org/2975693002/diff/60001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:83: } catch (IOException e) { Group these exceptions together using "|" syntax: https://docs.oracle.com/javase/tutorial/essential/exceptions/catch.html Also, empty catch blocks are suspicious. Whenever you write an empty catch block, it's best to put in a comment justifying the lack of error-handling. Otherwise your reader feels nervous for the rest of the day :) https://codereview.chromium.org/2975693002/diff/60001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:112: // store separately to prevent an expensive checking expiration This comment is confusing. What is "checking expiration"? https://codereview.chromium.org/2975693002/diff/60001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:126: Log.e(TAG, "IOException file close"); Keep in mind that when people read your error messages, they'll read them in logcat, mixed in with lots of other messages from all over the place, and they won't be familiar with your code which produced the error. Be nice to these people by making your error messages short but informative :) E.g. "Failed to close variations seed file". https://codereview.chromium.org/2975693002/diff/60001/components/variations/a... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java (right): https://codereview.chromium.org/2975693002/diff/60001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java:93: break; assert false? https://codereview.chromium.org/2975693002/diff/60001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java:140: } catch (IOException e) { Same as my comment in AwVariationsSeedFetchService.java - group the exceptions, and justify the lack of extra error-handling code. https://codereview.chromium.org/2975693002/diff/60001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java:181: return null; Continuing the previous discussion from here: https://codereview.chromium.org/2970993002/diff/100001/components/variations/... Last time, I suggested throwing exceptions instead of returning null, since the calling code wasn't handling the null, and because null results can be surprising. But now you're doing both; this case returns null, but the other cases throw exceptions.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2975693002/diff/60001/android_webview/apk/jav... File android_webview/apk/java/AndroidManifest.xml (right): https://codereview.chromium.org/2975693002/diff/60001/android_webview/apk/jav... android_webview/apk/java/AndroidManifest.xml:41: <service android:name="org.chromium.android_webview.AwVariationsSeedFetchService" On 2017/07/10 18:23:12, paulmiller wrote: > unindent Never mind; per the "WebView Service design" email thread, it sounds like this Service should actually go inside the "donor_package" condition, below. https://codereview.chromium.org/2975693002/diff/60001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java (right): https://codereview.chromium.org/2975693002/diff/60001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:34: public static final String SEED_DATA_PREF_FILENAME = "variations_seed_pref"; On 2017/07/10 18:23:12, paulmiller wrote: > These files should go in the "app_webview" subdirectory, where WebView puts all > its other files like cookies and the UMA GUID. (E.g. `adb shell ls > /data/data/com.google.android.gm/app_webview`.) Tony pointed out this is for the WebView/Monochrome directory, not the app's directory, so there is no app_webview subdirectory. But we should still put them in some subdirectory to avoid cluttering Chrome's data directory. Maybe "webview_variations"? (Crash uploading seems to use "WebView_Crashes".) https://codereview.chromium.org/2975693002/diff/60001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:47: return false; On 2017/07/10 18:23:12, paulmiller wrote: > Shouldn't this stop the FetchFinchSeedDataTask? Tony pointed out that this is only called when the JobParameters are no longer satisfied, so we don't need to do anything right now.
The CQ bit was checked by yiyuny@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ptal https://codereview.chromium.org/2975693002/diff/60001/android_webview/apk/jav... File android_webview/apk/java/AndroidManifest.xml (right): https://codereview.chromium.org/2975693002/diff/60001/android_webview/apk/jav... android_webview/apk/java/AndroidManifest.xml:41: <service android:name="org.chromium.android_webview.AwVariationsSeedFetchService" On 2017/07/10 18:23:12, paulmiller wrote: > unindent Done. https://codereview.chromium.org/2975693002/diff/60001/android_webview/apk/jav... android_webview/apk/java/AndroidManifest.xml:41: <service android:name="org.chromium.android_webview.AwVariationsSeedFetchService" On 2017/07/10 23:37:40, paulmiller wrote: > On 2017/07/10 18:23:12, paulmiller wrote: > > unindent > > Never mind; per the "WebView Service design" email thread, it sounds like this > Service should actually go inside the "donor_package" condition, below. Acknowledged. https://codereview.chromium.org/2975693002/diff/60001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java (right): https://codereview.chromium.org/2975693002/diff/60001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:27: * Job Service to fetch seed in the background which guarantees to get the job done. On 2017/07/10 18:23:12, paulmiller wrote: > This comment is confusing. Explain what "seed" means, and remove "guarantees to > get the job done". (I don't think anything about this is guaranteed.) Done. https://codereview.chromium.org/2975693002/diff/60001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:34: public static final String SEED_DATA_PREF_FILENAME = "variations_seed_pref"; On 2017/07/10 18:23:12, paulmiller wrote: > These files should go in the "app_webview" subdirectory, where WebView puts all > its other files like cookies and the UMA GUID. (E.g. `adb shell ls > /data/data/com.google.android.gm/app_webview`.) Done. https://codereview.chromium.org/2975693002/diff/60001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:47: return false; On 2017/07/10 23:37:40, paulmiller wrote: > On 2017/07/10 18:23:12, paulmiller wrote: > > Shouldn't this stop the FetchFinchSeedDataTask? > > Tony pointed out that this is only called when the JobParameters are no longer > satisfied, so we don't need to do anything right now. Done. https://codereview.chromium.org/2975693002/diff/60001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:54: FetchFinchSeedDataTask(JobParameters params, JobService svc) { On 2017/07/10 18:23:12, paulmiller wrote: > It seems strange to declare the subclass static, but then keep a "this" > reference anyway. Why not just make the class non-static? Done. https://codereview.chromium.org/2975693002/diff/60001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:71: private static void fetchSeed(String restrictMode) { On 2017/07/10 18:23:12, paulmiller wrote: > Remove the argument, since it's only ever called with "". Done. https://codereview.chromium.org/2975693002/diff/60001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:74: synchronized (sLock) { On 2017/07/10 18:23:12, paulmiller wrote: > If multiple jobs are scheduled at the same time, this will run all of them, > serially. But we only need one of them to run. What about using tryLock(), and > just not doing the fetch if we don't get the lock? Done. https://codereview.chromium.org/2975693002/diff/60001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:83: } catch (IOException e) { On 2017/07/10 18:23:12, paulmiller wrote: > Group these exceptions together using "|" syntax: > https://docs.oracle.com/javase/tutorial/essential/exceptions/catch.html > > Also, empty catch blocks are suspicious. Whenever you write an empty catch > block, it's best to put in a comment justifying the lack of error-handling. > Otherwise your reader feels nervous for the rest of the day :) Done. https://codereview.chromium.org/2975693002/diff/60001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:112: // store separately to prevent an expensive checking expiration On 2017/07/10 18:23:12, paulmiller wrote: > This comment is confusing. What is "checking expiration"? Done. https://codereview.chromium.org/2975693002/diff/60001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:126: Log.e(TAG, "IOException file close"); On 2017/07/10 18:23:12, paulmiller wrote: > Keep in mind that when people read your error messages, they'll read them in > logcat, mixed in with lots of other messages from all over the place, and they > won't be familiar with your code which produced the error. Be nice to these > people by making your error messages short but informative :) E.g. "Failed to > close variations seed file". Done. https://codereview.chromium.org/2975693002/diff/60001/components/variations/a... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java (right): https://codereview.chromium.org/2975693002/diff/60001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java:93: break; On 2017/07/10 18:23:13, paulmiller wrote: > assert false? Done. https://codereview.chromium.org/2975693002/diff/60001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java:140: } catch (IOException e) { On 2017/07/10 18:23:13, paulmiller wrote: > Same as my comment in AwVariationsSeedFetchService.java - group the exceptions, > and justify the lack of extra error-handling code. Done. https://codereview.chromium.org/2975693002/diff/60001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java:181: return null; On 2017/07/10 18:23:13, paulmiller wrote: > Continuing the previous discussion from here: > https://codereview.chromium.org/2970993002/diff/100001/components/variations/... > > Last time, I suggested throwing exceptions instead of returning null, since the > calling code wasn't handling the null, and because null results can be > surprising. But now you're doing both; this case returns null, but the other > cases throw exceptions. Done.
https://codereview.chromium.org/2975693002/diff/80001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java (right): https://codereview.chromium.org/2975693002/diff/80001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:51: // When job parameter is not satisfied any more, the job should be rescheduled Comments should be full sentences. i.e. terminate with a . Also do the same elsewhere. https://codereview.chromium.org/2975693002/diff/80001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:77: if (sLock.tryLock()) { I don't think this does what you want it do. Unless I'm missing something subtle - in which case you need to add more comments. From the docs: "tryLock() Acquires the lock only if it is not held by another thread at the time of invocation." This means if another thread is holding the lock, we just ignore it and do nothing here. Which doesn't seem correct to me. I think you either you want: a) This operation cannot be done at the same time by two threads - in which case you can use just the synchronized keyword. b) This operation should only be done once or - or have some rate-limiting on how often it's done. If so, you probably want a variable that keeps track of whether this has been started already or how long ago it's been done - or perhaps a Future object indicating it was started but yet not complete, probably in conjunction with synchronized keyword. Of course, maybe I'm missing something and tryLock() is really desired. If so, please provide the reasoning and it should also be made clear in the code comments. https://codereview.chromium.org/2975693002/diff/80001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:96: public static class SeedPreference implements Serializable { Can SeedInfo from the fetcher be re-used? https://codereview.chromium.org/2975693002/diff/80001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:110: public static File getOrCreateWebViewVariationsDir() { Does this need to be public? https://codereview.chromium.org/2975693002/diff/80001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:148: } This is pretty verbose. Perhaps add a helper function for this construct since you're using it twice? Or perhaps there's a helper somewhere already for this sort of thing? https://codereview.chromium.org/2975693002/diff/80001/components/variations/a... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java (right): https://codereview.chromium.org/2975693002/diff/80001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java:165: public SeedInfo downloadContent(final VariationsPlatform platform, final String restrictMode) Please add javadoc documenting params, return value and the exception cases. Also, doesn't seem like the params need to be final. https://codereview.chromium.org/2975693002/diff/80001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java:168: SeedInfo info = new SeedInfo(); Can this be constructed right above line 187? Ideally, you could move the return info line directly after line 192 too, but not sure if compiler will complain. If not, you could initialize it to null here and create it inside the try. https://codereview.chromium.org/2975693002/diff/80001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java:182: "HTTP error code when fetching variations seed data: " + responseCode); Instead of crafting a whole new string, how about re-using the same string as Log.w().
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Don't forget to fix the commit message. https://codereview.chromium.org/2975693002/diff/80001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java (right): https://codereview.chromium.org/2975693002/diff/80001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:77: if (sLock.tryLock()) { On 2017/07/11 19:37:23, Alexei Svitkine (slow) wrote: > I don't think this does what you want it do. Unless I'm missing something subtle > - in which case you need to add more comments. > > From the docs: > > "tryLock() > Acquires the lock only if it is not held by another thread at the time of > invocation." > > This means if another thread is holding the lock, we just ignore it and do > nothing here. Which doesn't seem correct to me. > > I think you either you want: > a) This operation cannot be done at the same time by two threads - in which > case you can use just the synchronized keyword. > b) This operation should only be done once or - or have some rate-limiting on > how often it's done. If so, you probably want a variable that keeps track of > whether this has been started already or how long ago it's been done - or > perhaps a Future object indicating it was started but yet not complete, probably > in conjunction with synchronized keyword. > > Of course, maybe I'm missing something and tryLock() is really desired. If so, > please provide the reasoning and it should also be made clear in the code > comments. > It was synchronized before. I don't think we want synchronized, because it's pointless for threads to make the same request right after one another - we only want this to happen once in a while. I recommended tryLock() for that reason. Explicit rate limiting is was in the design doc, though I'm not sure where we were planning to implement it. Here would be a good place if Tony isn't already planning to put it somewhere else.
https://codereview.chromium.org/2975693002/diff/80001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java (right): https://codereview.chromium.org/2975693002/diff/80001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:77: if (sLock.tryLock()) { On 2017/07/11 20:51:36, paulmiller wrote: > On 2017/07/11 19:37:23, Alexei Svitkine (slow) wrote: > > I don't think this does what you want it do. Unless I'm missing something > subtle > > - in which case you need to add more comments. > > > > From the docs: > > > > "tryLock() > > Acquires the lock only if it is not held by another thread at the time of > > invocation." > > > > This means if another thread is holding the lock, we just ignore it and do > > nothing here. Which doesn't seem correct to me. > > > > I think you either you want: > > a) This operation cannot be done at the same time by two threads - in which > > case you can use just the synchronized keyword. > > b) This operation should only be done once or - or have some rate-limiting > on > > how often it's done. If so, you probably want a variable that keeps track of > > whether this has been started already or how long ago it's been done - or > > perhaps a Future object indicating it was started but yet not complete, > probably > > in conjunction with synchronized keyword. > > > > Of course, maybe I'm missing something and tryLock() is really desired. If so, > > please provide the reasoning and it should also be made clear in the code > > comments. > > > > It was synchronized before. I don't think we want synchronized, because it's > pointless for threads to make the same request right after one another - we only > want this to happen once in a while. I recommended tryLock() for that reason. > Explicit rate limiting is was in the design doc, though I'm not sure where we > were planning to implement it. Here would be a good place if Tony isn't already > planning to put it somewhere else. Gotcha. It seems OK as a temporary solution with an explanation and TODO. But otherwise I'd recommend something more robust if we expect multiple threads to call this at once (i.e. so we don't make requests one after another if the first one completes already.)
The CQ bit was checked by yiyuny@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ptal https://codereview.chromium.org/2975693002/diff/80001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java (right): https://codereview.chromium.org/2975693002/diff/80001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:51: // When job parameter is not satisfied any more, the job should be rescheduled On 2017/07/11 19:37:23, Alexei Svitkine (slow) wrote: > Comments should be full sentences. i.e. terminate with a . > > Also do the same elsewhere. Done. https://codereview.chromium.org/2975693002/diff/80001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:77: if (sLock.tryLock()) { On 2017/07/11 20:53:47, Alexei Svitkine (slow) wrote: > On 2017/07/11 20:51:36, paulmiller wrote: > > On 2017/07/11 19:37:23, Alexei Svitkine (slow) wrote: > > > I don't think this does what you want it do. Unless I'm missing something > > subtle > > > - in which case you need to add more comments. > > > > > > From the docs: > > > > > > "tryLock() > > > Acquires the lock only if it is not held by another thread at the time of > > > invocation." > > > > > > This means if another thread is holding the lock, we just ignore it and do > > > nothing here. Which doesn't seem correct to me. > > > > > > I think you either you want: > > > a) This operation cannot be done at the same time by two threads - in > which > > > case you can use just the synchronized keyword. > > > b) This operation should only be done once or - or have some rate-limiting > > on > > > how often it's done. If so, you probably want a variable that keeps track of > > > whether this has been started already or how long ago it's been done - or > > > perhaps a Future object indicating it was started but yet not complete, > > probably > > > in conjunction with synchronized keyword. > > > > > > Of course, maybe I'm missing something and tryLock() is really desired. If > so, > > > please provide the reasoning and it should also be made clear in the code > > > comments. > > > > > > > It was synchronized before. I don't think we want synchronized, because it's > > pointless for threads to make the same request right after one another - we > only > > want this to happen once in a while. I recommended tryLock() for that reason. > > Explicit rate limiting is was in the design doc, though I'm not sure where we > > were planning to implement it. Here would be a good place if Tony isn't > already > > planning to put it somewhere else. > > Gotcha. It seems OK as a temporary solution with an explanation and TODO. But > otherwise I'd recommend something more robust if we expect multiple threads to > call this at once (i.e. so we don't make requests one after another if the first > one completes already.) Done. https://codereview.chromium.org/2975693002/diff/80001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:96: public static class SeedPreference implements Serializable { On 2017/07/11 19:37:23, Alexei Svitkine (slow) wrote: > Can SeedInfo from the fetcher be re-used? I didn't re-use the SeedInfo class is because 1. SeedPreference is to store seed preference and needs to implement Serializable, but it is weird to implement it for SeedInfo which is a simple class to get the return value(which is useless for clank). 2. There will be read operation on the seed preference, and I think store and read operation had better not depend on the SeedInfo which is outside the component. https://codereview.chromium.org/2975693002/diff/80001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:110: public static File getOrCreateWebViewVariationsDir() { On 2017/07/11 19:37:23, Alexei Svitkine (slow) wrote: > Does this need to be public? The function will be used when reading the seed preference. I will make it private in this CL. https://codereview.chromium.org/2975693002/diff/80001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:148: } On 2017/07/11 19:37:23, Alexei Svitkine (slow) wrote: > This is pretty verbose. Perhaps add a helper function for this construct since > you're using it twice? Or perhaps there's a helper somewhere already for this > sort of thing? Done. https://codereview.chromium.org/2975693002/diff/80001/components/variations/a... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java (right): https://codereview.chromium.org/2975693002/diff/80001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java:165: public SeedInfo downloadContent(final VariationsPlatform platform, final String restrictMode) On 2017/07/11 19:37:24, Alexei Svitkine (slow) wrote: > Please add javadoc documenting params, return value and the exception cases. > > Also, doesn't seem like the params need to be final. Done. https://codereview.chromium.org/2975693002/diff/80001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java:182: "HTTP error code when fetching variations seed data: " + responseCode); On 2017/07/11 19:37:24, Alexei Svitkine (slow) wrote: > Instead of crafting a whole new string, how about re-using the same string as > Log.w(). Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Almost there. https://codereview.chromium.org/2975693002/diff/100001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java (right): https://codereview.chromium.org/2975693002/diff/100001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:55: private class FetchFinchSeedDataTask extends AsyncTask<Void, Void, Void> { Can this class be static? https://codereview.chromium.org/2975693002/diff/100001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:78: // seed in a while before the seed is expired. Nit: "before the seed is expired" isn't too meaningful. I think you want to say "to ensure there's only one threading fetching at a time and that the seed doesn't get fetched too frequently". https://codereview.chromium.org/2975693002/diff/100001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:81: VariationsSeedFetcher.SeedInfo si = VariationsSeedFetcher.get().downloadContent( Nit: seedInfo https://codereview.chromium.org/2975693002/diff/100001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:96: * Store seed preference independently from Seed Info. This comment reads like a method comment, since it starts with a verb. Can you describe the class instead and *then* mention how it's used. You say it's stored independently - stored where? Mention that it's serializable and what it means for future changes to this class. (i.e. what would someone have to be careful of when changing the class.) https://codereview.chromium.org/2975693002/diff/100001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:102: public boolean isGzipCompressed; Can these be final - or is that something that's not allowed with Serializable? https://codereview.chromium.org/2975693002/diff/100001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:104: public SeedPreference(final VariationsSeedFetcher.SeedInfo si) { Nit: seedInfo. Acronyms are discouraged, especially for such short things. No need for final qualifier here. https://codereview.chromium.org/2975693002/diff/100001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:114: ContextUtils.getApplicationContext().getFilesDir(), WEBVIEW_VARIATIONS_DIR); Nit: If this is wrapping already, suggest extracting the result of getFilesDir() to a separate variable above, so it's still 2 lines but a bit nicer. https://codereview.chromium.org/2975693002/diff/100001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:121: private static void storeVariationsSeed(final VariationsSeedFetcher.SeedInfo si) { seedInfo https://codereview.chromium.org/2975693002/diff/100001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:123: ObjectOutputStream oosSeedPref = null; Nit: Move these to right above the try. https://codereview.chromium.org/2975693002/diff/100001/components/variations/... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java (right): https://codereview.chromium.org/2975693002/diff/100001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java:103: * Store the seed and its related header fields. Nit: Don't use the word "Store" because that's ambiguous. It may imply that it's actually being stored somewhere, whereas it's really used as a return value. How about: "Object holding the seed data and related fields retrieved from HTTP headers." https://codereview.chromium.org/2975693002/diff/100001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java:110: public byte[] rawSeed; Nit: Maybe seedData? https://codereview.chromium.org/2975693002/diff/100001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java:166: * Actually download the variations seed data with platform and retrictMode Nit: "Actually" doesn't add much, so just start with "Download". Also add a . at the end. https://codereview.chromium.org/2975693002/diff/100001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java:168: * runned on the platform. "can be run on that platform"
sgurun@chromium.org changed reviewers: + gsennton@chromium.org
sgurun@chromium.org changed reviewers: + sgurun@chromium.org
Gustav, since you already wrote one service and became an expert, can you take review the code too, Thanks:) https://codereview.chromium.org/2975693002/diff/100001/android_webview/DEPS File android_webview/DEPS (right): https://codereview.chromium.org/2975693002/diff/100001/android_webview/DEPS#n... android_webview/DEPS:13: "+components/variations", can this be more fine grained - such as android_webview/java/DEPS
and thanks Alexei for doing a through review!
The CQ bit was checked by yiyuny@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ptal https://codereview.chromium.org/2975693002/diff/100001/android_webview/DEPS File android_webview/DEPS (right): https://codereview.chromium.org/2975693002/diff/100001/android_webview/DEPS#n... android_webview/DEPS:13: "+components/variations", On 2017/07/12 16:12:25, sgurun wrote: > can this be more fine grained - such as android_webview/java/DEPS Done. https://codereview.chromium.org/2975693002/diff/100001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java (right): https://codereview.chromium.org/2975693002/diff/100001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:55: private class FetchFinchSeedDataTask extends AsyncTask<Void, Void, Void> { On 2017/07/12 15:52:50, Alexei Svitkine (slow) wrote: > Can this class be static? I prefer to make it non-static because function jobFinished needs to be called after fetching is done and it is a function of JobService, which means in the task we need to maintain a pointer to the JobService. https://codereview.chromium.org/2975693002/diff/100001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:78: // seed in a while before the seed is expired. On 2017/07/12 15:52:50, Alexei Svitkine (slow) wrote: > Nit: "before the seed is expired" isn't too meaningful. I think you want to say > "to ensure there's only one threading fetching at a time and that the seed > doesn't get fetched too frequently". Done. https://codereview.chromium.org/2975693002/diff/100001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:81: VariationsSeedFetcher.SeedInfo si = VariationsSeedFetcher.get().downloadContent( On 2017/07/12 15:52:50, Alexei Svitkine (slow) wrote: > Nit: seedInfo Done. https://codereview.chromium.org/2975693002/diff/100001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:96: * Store seed preference independently from Seed Info. On 2017/07/12 15:52:49, Alexei Svitkine (slow) wrote: > This comment reads like a method comment, since it starts with a verb. Can you > describe the class instead and *then* mention how it's used. You say it's stored > independently - stored where? Mention that it's serializable and what it means > for future changes to this class. (i.e. what would someone have to be careful of > when changing the class.) Done. https://codereview.chromium.org/2975693002/diff/100001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:102: public boolean isGzipCompressed; On 2017/07/12 15:52:49, Alexei Svitkine (slow) wrote: > Can these be final - or is that something that's not allowed with Serializable? Done. https://codereview.chromium.org/2975693002/diff/100001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:104: public SeedPreference(final VariationsSeedFetcher.SeedInfo si) { On 2017/07/12 15:52:49, Alexei Svitkine (slow) wrote: > Nit: seedInfo. Acronyms are discouraged, especially for such short things. No > need for final qualifier here. Done. https://codereview.chromium.org/2975693002/diff/100001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:114: ContextUtils.getApplicationContext().getFilesDir(), WEBVIEW_VARIATIONS_DIR); On 2017/07/12 15:52:50, Alexei Svitkine (slow) wrote: > Nit: If this is wrapping already, suggest extracting the result of getFilesDir() > to a separate variable above, so it's still 2 lines but a bit nicer. Done. https://codereview.chromium.org/2975693002/diff/100001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:121: private static void storeVariationsSeed(final VariationsSeedFetcher.SeedInfo si) { On 2017/07/12 15:52:49, Alexei Svitkine (slow) wrote: > seedInfo Done. https://codereview.chromium.org/2975693002/diff/100001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:123: ObjectOutputStream oosSeedPref = null; On 2017/07/12 15:52:49, Alexei Svitkine (slow) wrote: > Nit: Move these to right above the try. Done. https://codereview.chromium.org/2975693002/diff/100001/components/variations/... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java (right): https://codereview.chromium.org/2975693002/diff/100001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java:103: * Store the seed and its related header fields. On 2017/07/12 15:52:50, Alexei Svitkine (slow) wrote: > Nit: Don't use the word "Store" because that's ambiguous. It may imply that it's > actually being stored somewhere, whereas it's really used as a return value. > > How about: > > "Object holding the seed data and related fields retrieved from HTTP headers." Done. https://codereview.chromium.org/2975693002/diff/100001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java:110: public byte[] rawSeed; On 2017/07/12 15:52:50, Alexei Svitkine (slow) wrote: > Nit: Maybe seedData? Done. https://codereview.chromium.org/2975693002/diff/100001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java:166: * Actually download the variations seed data with platform and retrictMode On 2017/07/12 15:52:50, Alexei Svitkine (slow) wrote: > Nit: "Actually" doesn't add much, so just start with "Download". Also add a . at > the end. Done. https://codereview.chromium.org/2975693002/diff/100001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java:168: * runned on the platform. On 2017/07/12 15:52:50, Alexei Svitkine (slow) wrote: > "can be run on that platform" Done.
https://codereview.chromium.org/2975693002/diff/100001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java (right): https://codereview.chromium.org/2975693002/diff/100001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:55: private class FetchFinchSeedDataTask extends AsyncTask<Void, Void, Void> { On 2017/07/12 18:31:30, yiyuny wrote: > On 2017/07/12 15:52:50, Alexei Svitkine (slow) wrote: > > Can this class be static? > > I prefer to make it non-static because function jobFinished needs to be called > after fetching is done and it is a function of JobService, which means in the > task we need to maintain a pointer to the JobService. That makes sense. https://codereview.chromium.org/2975693002/diff/120001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java (right): https://codereview.chromium.org/2975693002/diff/120001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:97: * SeedPreference serialize related fields of seed data and is used when reading or writing Nit: "serializes" or "is used to serialize" https://codereview.chromium.org/2975693002/diff/120001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:99: * when the fields are changed. Nit: Move the last sentence to be about serialVersionUID. Mention that InvalidClassException is thrown when it doesn't match. https://codereview.chromium.org/2975693002/diff/120001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:102: private static final long serialVersionUID = 0L; Nit: Add an empty line after this. https://codereview.chromium.org/2975693002/diff/120001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:135: fosSeed.write(seedInfo.seedData, 0, seedInfo.seedData.length); How will you read this? I think you need to also write the length as a field, so you know how much data to read. For example, the length can be in the SeedPreference struct. Or, I think you can just have the byte[] be part of that struct and the ObjectOutputStream can handle both? Please add a function to read the data and add a unit test that verifies that the data you've written can be read successfully. https://codereview.chromium.org/2975693002/diff/120001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:143: "FileNotFoundException failed to open file to write seed data or preference."); Nit: If you pass e as a last param, you don't need to have the name of the exception in the message. Just started with "Failed to". Same below. https://codereview.chromium.org/2975693002/diff/120001/components/variations/... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java (right): https://codereview.chromium.org/2975693002/diff/120001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java:52: // Synchronization lock to make singleton thread-safe Nit: . https://codereview.chromium.org/2975693002/diff/120001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java:142: prefs.edit().putBoolean(VARIATIONS_INITIALIZED_PREF, true).apply(); Should this still be set if there was an exception? If it should, should have a comment. Oterhwise, put in the try? https://codereview.chromium.org/2975693002/diff/120001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java:216: "IOException I/O errors while opening the connection to fetch variations seed.", Nit: Does this really need to be so verbose? I think the previous message was good enough.
https://codereview.chromium.org/2975693002/diff/120001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java (right): https://codereview.chromium.org/2975693002/diff/120001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:143: "FileNotFoundException failed to open file to write seed data or preference."); On 2017/07/12 19:01:21, Alexei Svitkine (slow) wrote: > Nit: If you pass e as a last param, you don't need to have the name of the > exception in the message. Just started with "Failed to". Same below. I recommended not passing e, because I thought a stack trace would be unnecessarily verbose. It wouldn't be very informative, because there's only 1 code path to this function, and logcat is already pretty noisy with stack traces.
https://codereview.chromium.org/2975693002/diff/120001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java (right): https://codereview.chromium.org/2975693002/diff/120001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:143: "FileNotFoundException failed to open file to write seed data or preference."); On 2017/07/12 20:21:59, paulmiller wrote: > On 2017/07/12 19:01:21, Alexei Svitkine (slow) wrote: > > Nit: If you pass e as a last param, you don't need to have the name of the > > exception in the message. Just started with "Failed to". Same below. > > I recommended not passing e, because I thought a stack trace would be > unnecessarily verbose. It wouldn't be very informative, because there's only 1 > code path to this function, and logcat is already pretty noisy with stack > traces. Ah, fair enough. Maybe log e.getMessage() then, at least?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ptal https://codereview.chromium.org/2975693002/diff/120001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java (right): https://codereview.chromium.org/2975693002/diff/120001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:97: * SeedPreference serialize related fields of seed data and is used when reading or writing On 2017/07/12 19:01:21, Alexei Svitkine (slow) wrote: > Nit: "serializes" or "is used to serialize" Done. https://codereview.chromium.org/2975693002/diff/120001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:99: * when the fields are changed. On 2017/07/12 19:01:21, Alexei Svitkine (slow) wrote: > Nit: Move the last sentence to be about serialVersionUID. Mention that > InvalidClassException is thrown when it doesn't match. Done. https://codereview.chromium.org/2975693002/diff/120001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:102: private static final long serialVersionUID = 0L; On 2017/07/12 19:01:21, Alexei Svitkine (slow) wrote: > Nit: Add an empty line after this. Done. https://codereview.chromium.org/2975693002/diff/120001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:135: fosSeed.write(seedInfo.seedData, 0, seedInfo.seedData.length); On 2017/07/12 19:01:21, Alexei Svitkine (slow) wrote: > How will you read this? > > I think you need to also write the length as a field, so you know how much data > to read. For example, the length can be in the SeedPreference struct. > > Or, I think you can just have the byte[] be part of that struct and the > ObjectOutputStream can handle both? > > Please add a function to read the data and add a unit test that verifies that > the data you've written can be read successfully. I choose to store the seed data and preference separately because I don't want to read the whole seed when I need to check if it is expired. So based on this, if there is only seed data in one file, I don't need to store the length because I can read it until EOF. And there is a function convertInputStreamToByteArray in VariationsSeedFetcher will help. https://codereview.chromium.org/2975693002/diff/120001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:143: "FileNotFoundException failed to open file to write seed data or preference."); On 2017/07/12 20:32:31, Alexei Svitkine (slow) wrote: > On 2017/07/12 20:21:59, paulmiller wrote: > > On 2017/07/12 19:01:21, Alexei Svitkine (slow) wrote: > > > Nit: If you pass e as a last param, you don't need to have the name of the > > > exception in the message. Just started with "Failed to". Same below. > > > > I recommended not passing e, because I thought a stack trace would be > > unnecessarily verbose. It wouldn't be very informative, because there's only 1 > > code path to this function, and logcat is already pretty noisy with stack > > traces. > > Ah, fair enough. Maybe log e.getMessage() then, at least? I don't know the default value of the Exception's message, so I don't know what it will print out. But I think it is fine to use the current one for it just explains the situation. Do you mean that I should use getMessage() to replace the whole string here? https://codereview.chromium.org/2975693002/diff/120001/components/variations/... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java (right): https://codereview.chromium.org/2975693002/diff/120001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java:52: // Synchronization lock to make singleton thread-safe On 2017/07/12 19:01:21, Alexei Svitkine (slow) wrote: > Nit: . Done. https://codereview.chromium.org/2975693002/diff/120001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java:142: prefs.edit().putBoolean(VARIATIONS_INITIALIZED_PREF, true).apply(); On 2017/07/12 19:01:21, Alexei Svitkine (slow) wrote: > Should this still be set if there was an exception? If it should, should have a > comment. Oterhwise, put in the try? Done. https://codereview.chromium.org/2975693002/diff/120001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java:216: "IOException I/O errors while opening the connection to fetch variations seed.", On 2017/07/12 19:01:21, Alexei Svitkine (slow) wrote: > Nit: Does this really need to be so verbose? I think the previous message was > good enough. Done.
The CQ bit was checked by yiyuny@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Alright, modulo the inline comments and one question I think this (android_webview/) looks fine. Question: Is there a design for how often this service will run, and how it will be started? Given that it will be running in its own process we will want to run it very rarely - more specifically we should NOT be starting it up every time WebView starts (since there are so many apps on Android using WebView that we would essentially be keeping it alive all the time). You might want to consider adding tests to ensure some of your assumptions actually hold (e.g. ensuring two different threads can't download a seed at the same time) - it is often easier adding tests earlier rather than later. How have you manually tested starting the Service? (I'm interested in how the chromium-code has been intialized - in AwMinidumpUploadJobService we had to call ContextUtils.initApplicationContext() explicitly to avoid crashes in some cases). I would ensure that 1. we don't cause a crash by killing the ':variations_service' process before AwVariationsSeedFetchService starts. 2. Your code works both for Monochrome and standalone WebView. Apologies if this is a bit involved - using Services in WebView is still a new thing. Thanks! https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java (right): https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:15: import org.chromium.components.variations.firstrun.VariationsSeedFetcher; What's the initialization process for VariationsSeedFetcher, does it require any native code to be loaded? Since this Service will be run in its own separate process we have to be very careful to ensure we run all the initialization code we need to make sure everything works in the service (and whether that code is run before our service is started might depend on whether we are using the Standalone WebView package, or Monochrome, as WebView implementation). https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:33: @SuppressLint("NewApi") // JobService requires API level 21. WebView doesn't support pre-L anyway: please replace the suppresslint statement above with @TargetApi(Build.VERSION_CODES.LOLLIPOP) https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:52: // When job parameter is not satisfied any more, the job should be rescheduled. I have no idea what this comment means :) https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:71: jobFinished(mJobParams, false); Please add an inline comment describing what false means here (so that the next person looking at this code doesn't need to double-check what that boolean means) e.g. jobFinished(mJobParams, false /* needsReschedule */); , or jobFinished(mJobParams, false /* false -> don't reschedule */); https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:78: // TODO(yiyuny): Add explicitly control to to ensure there's only one threading fetching at Do you have a plan on how to solve this TODO? Are you planning to add asserts, or tests as well? https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:82: SeedInfo seedInfo = VariationsSeedFetcher.get().downloadContent( What's the size of the data we are downloading here? Do we care about the kind of network we are using for the download? (WiFi vs. data plan). https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:86: // Exceptions are handled in the downloadContent function and rethrowing the Maybe note that exceptions are also logged within downloadContent(). If they weren't we would want logging here. so "Exceptions are handled in the downloadContent" -> "Exceptions are handled and logged in the downloadContent" https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:120: File webViewFileDir = ContextUtils.getApplicationContext().getFilesDir(); Are we guaranteed that ContextUtils is initialized at this point? (AFAICT this code is running in a process separate to the normal WebView-loading code). https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:130: if (webViewVariationsDir == null) { Please add a log-statement before the return-statement here - I assume we should almost never fail to create webviewVariationsDir? (so we should log an error/warning here). https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:137: fosSeed = new FileOutputStream(new File(webViewVariationsDir, SEED_DATA_FILENAME)); One very common pattern when performing file writes (especially when the data written is large) is to first write the data to a temporary file, and then rename that file to the final file-name. If you don't follow that pattern you might end up crashing in the middle of the data-write (and thus end up with a file with broken content). How will these two files be read? If we are overwriting these files every time we download a new seed we should definitely make sure we aren't overwriting them partially / messing the files up :) https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:139: // Store separately so that reading large seed data (which is expensive) can be Just to clarify (to make sure I understand): we are storing the fetch time in a separate time to avoid having to open the large seed-data file just to read the fetch-time? https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:146: "FileNotFoundException failed to open file to write seed data or preference."); Include the actual exception in the logs please (this helps a lot when something goes wrong :)) Log.e(TAG, "FileNotFoundException failed to open file to write seed data or preference.", e); And for the IOException as well. Actually, given that the FileNotFoundException is a type of IOException, I would just merge these two catch-statements since we are just printing a generic message anyway.
https://codereview.chromium.org/2975693002/diff/120001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java (right): https://codereview.chromium.org/2975693002/diff/120001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:135: fosSeed.write(seedInfo.seedData, 0, seedInfo.seedData.length); On 2017/07/12 22:22:06, yiyuny wrote: > On 2017/07/12 19:01:21, Alexei Svitkine (slow) wrote: > > How will you read this? > > > > I think you need to also write the length as a field, so you know how much > data > > to read. For example, the length can be in the SeedPreference struct. > > > > Or, I think you can just have the byte[] be part of that struct and the > > ObjectOutputStream can handle both? > > > > Please add a function to read the data and add a unit test that verifies that > > the data you've written can be read successfully. > > I choose to store the seed data and preference separately because I don't want > to read the whole seed when I need to check if it is expired. So based on this, > if there is only seed data in one file, I don't need to store the length because > I can read it until EOF. And there is a function convertInputStreamToByteArray > in VariationsSeedFetcher will help. Ah, sorry I had missed that you're writing to different files. OK. Still, please add a unit test that reads the data to ensure both parts work correctly with each other. https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java (right): https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:15: import org.chromium.components.variations.firstrun.VariationsSeedFetcher; On 2017/07/13 13:31:59, gsennton wrote: > What's the initialization process for VariationsSeedFetcher, does it require any > native code to be loaded? > Since this Service will be run in its own separate process we have to be very > careful to ensure we run all the initialization code we need to make sure > everything works in the service (and whether that code is run before our service > is started might depend on whether we are using the Standalone WebView package, > or Monochrome, as WebView implementation). VariationsSeedFetcher does not depend on any native code and this is by design. (The way it's used in Chrome is specifically to fetch variations before native has been initialized.) https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:82: SeedInfo seedInfo = VariationsSeedFetcher.get().downloadContent( On 2017/07/13 13:31:58, gsennton wrote: > What's the size of the data we are downloading here? > Do we care about the kind of network we are using for the download? (WiFi vs. > data plan). The chrome one is around 50k. It will be smaller for webview because only specific experiments will target it.
The CQ bit was checked by yiyuny@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The service is a just to fetch the seed data from the finch server and extending the existing code. More design details are in the next CL. ptal https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java (right): https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:15: import org.chromium.components.variations.firstrun.VariationsSeedFetcher; On 2017/07/13 20:36:25, Alexei Svitkine (slow) wrote: > On 2017/07/13 13:31:59, gsennton wrote: > > What's the initialization process for VariationsSeedFetcher, does it require > any > > native code to be loaded? > > Since this Service will be run in its own separate process we have to be very > > careful to ensure we run all the initialization code we need to make sure > > everything works in the service (and whether that code is run before our > service > > is started might depend on whether we are using the Standalone WebView > package, > > or Monochrome, as WebView implementation). > > VariationsSeedFetcher does not depend on any native code and this is by design. > (The way it's used in Chrome is specifically to fetch variations before native > has been initialized.) Thanks for the explanation from Alexie. Paul and I search for the place which probably contains the code to call the native code and find it safe to use it. https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:33: @SuppressLint("NewApi") // JobService requires API level 21. On 2017/07/13 13:31:58, gsennton wrote: > WebView doesn't support pre-L anyway: > please replace the suppresslint statement above with > @TargetApi(Build.VERSION_CODES.LOLLIPOP) Done. https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:52: // When job parameter is not satisfied any more, the job should be rescheduled. On 2017/07/13 13:31:59, gsennton wrote: > I have no idea what this comment means :) Oh, what I want to say is that the function will only be called when the job's parameter is no longer satisfied before the jobFinished function is called. So at this time, the function return true so that the job will be rescheduled. https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:71: jobFinished(mJobParams, false); On 2017/07/13 13:31:58, gsennton wrote: > Please add an inline comment describing what false means here (so that the next > person looking at this code doesn't need to double-check what that boolean > means) > e.g. > jobFinished(mJobParams, false /* needsReschedule */); > , or > jobFinished(mJobParams, false /* false -> don't reschedule */); Done. https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:78: // TODO(yiyuny): Add explicitly control to to ensure there's only one threading fetching at On 2017/07/13 13:31:59, gsennton wrote: > Do you have a plan on how to solve this TODO? > Are you planning to add asserts, or tests as well? Yes, I will the do the explicit control in the FinchConfigurationService. For now, I have manually tested it. https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:82: SeedInfo seedInfo = VariationsSeedFetcher.get().downloadContent( On 2017/07/13 20:36:25, Alexei Svitkine (slow) wrote: > On 2017/07/13 13:31:58, gsennton wrote: > > What's the size of the data we are downloading here? > > Do we care about the kind of network we are using for the download? (WiFi vs. > > data plan). > > The chrome one is around 50k. It will be smaller for webview because only > specific experiments will target it. I'm using NETWORK_TYPE_ANY(data plan/wifi both work) in the next CL. So if is under 50k, I think that will be fine. Thanks to Alexie for providing the data. https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:86: // Exceptions are handled in the downloadContent function and rethrowing the On 2017/07/13 13:31:59, gsennton wrote: > Maybe note that exceptions are also logged within downloadContent(). If they > weren't we would want logging here. > > so > > "Exceptions are handled in the downloadContent" > -> > "Exceptions are handled and logged in the downloadContent" Done. https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:120: File webViewFileDir = ContextUtils.getApplicationContext().getFilesDir(); On 2017/07/13 13:31:59, gsennton wrote: > Are we guaranteed that ContextUtils is initialized at this point? (AFAICT this > code is running in a process separate to the normal WebView-loading code). I added a initApplicationContext in onStartJob. The code will crash in System WebView apk without the initialization. Thanks to Gustav and Paul for helping me find this bug. https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:130: if (webViewVariationsDir == null) { On 2017/07/13 13:31:59, gsennton wrote: > Please add a log-statement before the return-statement here - I assume we should > almost never fail to create webviewVariationsDir? (so we should log an > error/warning here). Done. https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:137: fosSeed = new FileOutputStream(new File(webViewVariationsDir, SEED_DATA_FILENAME)); On 2017/07/13 13:31:58, gsennton wrote: > One very common pattern when performing file writes (especially when the data > written is large) is to first write the data to a temporary file, and then > rename that file to the final file-name. If you don't follow that pattern you > might end up crashing in the middle of the data-write (and thus end up with a > file with broken content). > > How will these two files be read? > If we are overwriting these files every time we download a new seed we should > definitely make sure we aren't overwriting them partially / messing the files up > :) There should be read write lock on the seed pref file. I will do it in the next CL. https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:139: // Store separately so that reading large seed data (which is expensive) can be On 2017/07/13 13:31:59, gsennton wrote: > Just to clarify (to make sure I understand): we are storing the fetch time in a > separate time to avoid having to open the large seed-data file just to read the > fetch-time? Yes https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:146: "FileNotFoundException failed to open file to write seed data or preference."); On 2017/07/13 13:31:59, gsennton wrote: > Include the actual exception in the logs please (this helps a lot when something > goes wrong :)) > Log.e(TAG, "FileNotFoundException failed to open file to write seed data or > preference.", e); > > And for the IOException as well. > Actually, given that the FileNotFoundException is a type of IOException, I would > just merge these two catch-statements since we are just printing a generic > message anyway. I agree with combining two exceptions together. Paul recommends not passing e, because a stack trace would be unnecessarily verbose. It wouldn't be very informative, because there's only 1 code path to this function, and logcat is already pretty noisy with stack traces.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
Thanks for the fixes Yiyun. Sorry for not looking at the design doc earlier, it had escaped me. I've now left some comments on there related to this. I would like to see some of the discussions/questions here in the design doc (https://docs.google.com/document/d/1cBCcLUy-Kgyc_vSYFLHjkR2KiZkLyEK7Wf7QhAZ34...) so we can document them well (and get insights from other people). Especially: 1. Service lifecycle / separate process startup - how often will we bind to the service, is it OK to bind to it on every startup? When will we schedule a seed-download job to be run? 2. Data downloading - is it OK to download this data over metered networks, or should we only use WiFi? (or try WiFi first, and then fall back to data if WiFi isn't available for a long time). 3. Is it possible to create tests for any of this logic? Do we need manual tests for our test team? https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java (right): https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:52: // When job parameter is not satisfied any more, the job should be rescheduled. On 2017/07/14 01:33:57, yiyuny wrote: > On 2017/07/13 13:31:59, gsennton wrote: > > I have no idea what this comment means :) > > Oh, what I want to say is that the function will only be called when the job's > parameter is no longer satisfied before the jobFinished function is called. So > at this time, the function return true so that the job will be rescheduled. Right, I think the comment could be made more clear - "job parameters" is not really the term we want here IMO (job parameters include lots of stuff, not just the scheduling-restrictions of the job). E.g. "This method is called by the JobScheduler to stop a job before it has finished. Return true here to reschedule the job." ? https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:82: SeedInfo seedInfo = VariationsSeedFetcher.get().downloadContent( On 2017/07/14 01:33:57, yiyuny wrote: > On 2017/07/13 20:36:25, Alexei Svitkine (slow) wrote: > > On 2017/07/13 13:31:58, gsennton wrote: > > > What's the size of the data we are downloading here? > > > Do we care about the kind of network we are using for the download? (WiFi > vs. > > > data plan). > > > > The chrome one is around 50k. It will be smaller for webview because only > > specific experiments will target it. > > I'm using NETWORK_TYPE_ANY(data plan/wifi both work) in the next CL. So if is > under 50k, I think that will be fine. Thanks to Alexie for providing the data. What I mean is, we should discuss whether it is OK to use users' data plans for this - WebView is used by 800+ million people, so if all of those users use 50k of data over a data plan that means 50kB * 8e8 = 4e10kB = 40TB. So we would be using 40TB of data plan data (please correct me if my calculations are off). That's quite a lot of data to pay for :P https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:146: "FileNotFoundException failed to open file to write seed data or preference."); On 2017/07/14 01:33:56, yiyuny wrote: > On 2017/07/13 13:31:59, gsennton wrote: > > Include the actual exception in the logs please (this helps a lot when > something > > goes wrong :)) > > Log.e(TAG, "FileNotFoundException failed to open file to write seed data or > > preference.", e); > > > > And for the IOException as well. > > Actually, given that the FileNotFoundException is a type of IOException, I > would > > just merge these two catch-statements since we are just printing a generic > > message anyway. > > I agree with combining two exceptions together. > > Paul recommends not passing e, because a stack trace would be unnecessarily > verbose. It wouldn't be very informative, because there's only 1 code path to > this function, and logcat is already pretty noisy with stack traces. Well, if you're not gonna print the exception then you should probably split this catch-statement in two - so we at least know what kind of exception is thrown. I still think we should print the actual exception to aid debugging - if you don't print the exception you still won't know exactly what went wrong. But whether or not it's worth it depends on how often we expect to receive an exception here - if it is rare enough it should be fine to print the full exception stack trace. https://codereview.chromium.org/2975693002/diff/180001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java (right): https://codereview.chromium.org/2975693002/diff/180001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:89: // Exceptions are handled and logged in the downloadContent function and re-throwing "Exceptions are handled and logged in the downloadContent method, so we don't need any exception handling here. The only reason we need a catch-statement here is because those exceptions are re-thrown from downloadContent to skip the normal logic flow within that method." ? :)
https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java (right): https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:82: SeedInfo seedInfo = VariationsSeedFetcher.get().downloadContent( On 2017/07/14 12:45:39, gsennton wrote: > On 2017/07/14 01:33:57, yiyuny wrote: > > On 2017/07/13 20:36:25, Alexei Svitkine (slow) wrote: > > > On 2017/07/13 13:31:58, gsennton wrote: > > > > What's the size of the data we are downloading here? > > > > Do we care about the kind of network we are using for the download? (WiFi > > vs. > > > > data plan). > > > > > > The chrome one is around 50k. It will be smaller for webview because only > > > specific experiments will target it. > > > > I'm using NETWORK_TYPE_ANY(data plan/wifi both work) in the next CL. So if is > > under 50k, I think that will be fine. Thanks to Alexie for providing the data. > > What I mean is, we should discuss whether it is OK to use users' data plans for > this - WebView is used by 800+ million people, so if all of those users use 50k > of data over a data plan that means 50kB * 8e8 = 4e10kB = 40TB. So we would be > using 40TB of data plan data (please correct me if my calculations are off). > That's quite a lot of data to pay for :P System health will be an important consideration for when we enable this for real, but for this summer project we just want a prototype, which will not be enabled in prod. (I also don't think it makes sense to multiply by number of users, because each user pays individually. The addition of more WebView users doesn't increase data usage for other users.)
On 2017/07/14 17:41:06, paulmiller wrote: > https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/s... > File > android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java > (right): > > https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/s... > android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:82: > SeedInfo seedInfo = VariationsSeedFetcher.get().downloadContent( > On 2017/07/14 12:45:39, gsennton wrote: > > On 2017/07/14 01:33:57, yiyuny wrote: > > > On 2017/07/13 20:36:25, Alexei Svitkine (slow) wrote: > > > > On 2017/07/13 13:31:58, gsennton wrote: > > > > > What's the size of the data we are downloading here? > > > > > Do we care about the kind of network we are using for the download? > (WiFi > > > vs. > > > > > data plan). > > > > > > > > The chrome one is around 50k. It will be smaller for webview because only > > > > specific experiments will target it. > > > > > > I'm using NETWORK_TYPE_ANY(data plan/wifi both work) in the next CL. So if > is > > > under 50k, I think that will be fine. Thanks to Alexie for providing the > data. > > > > What I mean is, we should discuss whether it is OK to use users' data plans > for > > this - WebView is used by 800+ million people, so if all of those users use > 50k > > of data over a data plan that means 50kB * 8e8 = 4e10kB = 40TB. So we would be > > using 40TB of data plan data (please correct me if my calculations are off). > > That's quite a lot of data to pay for :P > > System health will be an important consideration for when we enable this for > real, but for this summer project we just want a prototype, which will not be > enabled in prod. (I also don't think it makes sense to multiply by number of > users, because each user pays individually. The addition of more WebView users > doesn't increase data usage for other users.) Yeah, there are a lot of good questions here. Let's move them to design doc and properly document it, but yes the main goal is not enabling it in the product yet.
The CQ bit was checked by yiyuny@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by yiyuny@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java (right): https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:146: "FileNotFoundException failed to open file to write seed data or preference."); On 2017/07/14 12:45:39, gsennton wrote: > On 2017/07/14 01:33:56, yiyuny wrote: > > On 2017/07/13 13:31:59, gsennton wrote: > > > Include the actual exception in the logs please (this helps a lot when > > something > > > goes wrong :)) > > > Log.e(TAG, "FileNotFoundException failed to open file to write seed data or > > > preference.", e); > > > > > > And for the IOException as well. > > > Actually, given that the FileNotFoundException is a type of IOException, I > > would > > > just merge these two catch-statements since we are just printing a generic > > > message anyway. > > > > I agree with combining two exceptions together. > > > > Paul recommends not passing e, because a stack trace would be unnecessarily > > verbose. It wouldn't be very informative, because there's only 1 code path to > > this function, and logcat is already pretty noisy with stack traces. > > Well, if you're not gonna print the exception then you should probably split > this catch-statement in two - so we at least know what kind of exception is > thrown. > > I still think we should print the actual exception to aid debugging - if you > don't print the exception you still won't know exactly what went wrong. But > whether or not it's worth it depends on how often we expect to receive an > exception here - if it is rare enough it should be fine to print the full > exception stack trace. I think these error messages strings are getting nit-picked back and forth a bit too much; too many cooks, I suppose :) Gustav, if you have a particular message style you'd like to prescribe, we can just go with that. Otherwise let's leave them as-is.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by yiyuny@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Sorry for the delay and I have updated the current design in it. As Selim said, I will move some design questions to the design doc and try to answer them there. ptal https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java (right): https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:52: // When job parameter is not satisfied any more, the job should be rescheduled. On 2017/07/14 12:45:39, gsennton wrote: > On 2017/07/14 01:33:57, yiyuny wrote: > > On 2017/07/13 13:31:59, gsennton wrote: > > > I have no idea what this comment means :) > > > > Oh, what I want to say is that the function will only be called when the job's > > parameter is no longer satisfied before the jobFinished function is called. So > > at this time, the function return true so that the job will be rescheduled. > > Right, I think the comment could be made more clear - "job parameters" is not > really the term we want here IMO (job parameters include lots of stuff, not just > the scheduling-restrictions of the job). E.g. > "This method is called by the JobScheduler to stop a job before it has finished. > Return true here to reschedule the job." > ? Done. https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:146: "FileNotFoundException failed to open file to write seed data or preference."); On 2017/07/14 12:45:39, gsennton wrote: > On 2017/07/14 01:33:56, yiyuny wrote: > > On 2017/07/13 13:31:59, gsennton wrote: > > > Include the actual exception in the logs please (this helps a lot when > > something > > > goes wrong :)) > > > Log.e(TAG, "FileNotFoundException failed to open file to write seed data or > > > preference.", e); > > > > > > And for the IOException as well. > > > Actually, given that the FileNotFoundException is a type of IOException, I > > would > > > just merge these two catch-statements since we are just printing a generic > > > message anyway. > > > > I agree with combining two exceptions together. > > > > Paul recommends not passing e, because a stack trace would be unnecessarily > > verbose. It wouldn't be very informative, because there's only 1 code path to > > this function, and logcat is already pretty noisy with stack traces. > > Well, if you're not gonna print the exception then you should probably split > this catch-statement in two - so we at least know what kind of exception is > thrown. > > I still think we should print the actual exception to aid debugging - if you > don't print the exception you still won't know exactly what went wrong. But > whether or not it's worth it depends on how often we expect to receive an > exception here - if it is rare enough it should be fine to print the full > exception stack trace. I will add 'e' to the end of the string to log what kind of exception is thrown. https://codereview.chromium.org/2975693002/diff/180001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java (right): https://codereview.chromium.org/2975693002/diff/180001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:89: // Exceptions are handled and logged in the downloadContent function and re-throwing On 2017/07/14 12:45:39, gsennton wrote: > "Exceptions are handled and logged in the downloadContent method, so we don't > need any exception handling here. The only reason we need a catch-statement here > is because those exceptions are re-thrown from downloadContent to skip the > normal logic flow within that method." ? > :) Thanks!
On 2017/07/15 00:19:00, yiyuny wrote: > Sorry for the delay and I have updated the current design in it. As Selim said, > I will move some design questions to the design doc and try to answer them > there. > > ptal > > https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/s... > File > android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java > (right): > > https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/s... > android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:52: > // When job parameter is not satisfied any more, the job should be rescheduled. > On 2017/07/14 12:45:39, gsennton wrote: > > On 2017/07/14 01:33:57, yiyuny wrote: > > > On 2017/07/13 13:31:59, gsennton wrote: > > > > I have no idea what this comment means :) > > > > > > Oh, what I want to say is that the function will only be called when the > job's > > > parameter is no longer satisfied before the jobFinished function is called. > So > > > at this time, the function return true so that the job will be rescheduled. > > > > Right, I think the comment could be made more clear - "job parameters" is not > > really the term we want here IMO (job parameters include lots of stuff, not > just > > the scheduling-restrictions of the job). E.g. > > "This method is called by the JobScheduler to stop a job before it has > finished. > > Return true here to reschedule the job." > > ? > > Done. > > https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/s... > android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:146: > "FileNotFoundException failed to open file to write seed data or preference."); > On 2017/07/14 12:45:39, gsennton wrote: > > On 2017/07/14 01:33:56, yiyuny wrote: > > > On 2017/07/13 13:31:59, gsennton wrote: > > > > Include the actual exception in the logs please (this helps a lot when > > > something > > > > goes wrong :)) > > > > Log.e(TAG, "FileNotFoundException failed to open file to write seed data > or > > > > preference.", e); > > > > > > > > And for the IOException as well. > > > > Actually, given that the FileNotFoundException is a type of IOException, I > > > would > > > > just merge these two catch-statements since we are just printing a generic > > > > message anyway. > > > > > > I agree with combining two exceptions together. > > > > > > Paul recommends not passing e, because a stack trace would be unnecessarily > > > verbose. It wouldn't be very informative, because there's only 1 code path > to > > > this function, and logcat is already pretty noisy with stack traces. > > > > Well, if you're not gonna print the exception then you should probably split > > this catch-statement in two - so we at least know what kind of exception is > > thrown. > > > > I still think we should print the actual exception to aid debugging - if you > > don't print the exception you still won't know exactly what went wrong. But > > whether or not it's worth it depends on how often we expect to receive an > > exception here - if it is rare enough it should be fine to print the full > > exception stack trace. > > I will add 'e' to the end of the string to log what kind of exception is thrown. > > https://codereview.chromium.org/2975693002/diff/180001/android_webview/java/s... > File > android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java > (right): > > https://codereview.chromium.org/2975693002/diff/180001/android_webview/java/s... > android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:89: > // Exceptions are handled and logged in the downloadContent function and > re-throwing > On 2017/07/14 12:45:39, gsennton wrote: > > "Exceptions are handled and logged in the downloadContent method, so we don't > > need any exception handling here. The only reason we need a catch-statement > here > > is because those exceptions are re-thrown from downloadContent to skip the > > normal logic flow within that method." ? > > :) > > Thanks! It means design doc. Sorry for the typo.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Sorry about the massive amounts of comments, I might have let you go easier if I knew this was a prototype ;) https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java (right): https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:82: SeedInfo seedInfo = VariationsSeedFetcher.get().downloadContent( On 2017/07/14 17:41:05, paulmiller wrote: > On 2017/07/14 12:45:39, gsennton wrote: > > On 2017/07/14 01:33:57, yiyuny wrote: > > > On 2017/07/13 20:36:25, Alexei Svitkine (slow) wrote: > > > > On 2017/07/13 13:31:58, gsennton wrote: > > > > > What's the size of the data we are downloading here? > > > > > Do we care about the kind of network we are using for the download? > (WiFi > > > vs. > > > > > data plan). > > > > > > > > The chrome one is around 50k. It will be smaller for webview because only > > > > specific experiments will target it. > > > > > > I'm using NETWORK_TYPE_ANY(data plan/wifi both work) in the next CL. So if > is > > > under 50k, I think that will be fine. Thanks to Alexie for providing the > data. > > > > What I mean is, we should discuss whether it is OK to use users' data plans > for > > this - WebView is used by 800+ million people, so if all of those users use > 50k > > of data over a data plan that means 50kB * 8e8 = 4e10kB = 40TB. So we would be > > using 40TB of data plan data (please correct me if my calculations are off). > > That's quite a lot of data to pay for :P > > System health will be an important consideration for when we enable this for > real, but for this summer project we just want a prototype, which will not be > enabled in prod. (I also don't think it makes sense to multiply by number of > users, because each user pays individually. The addition of more WebView users > doesn't increase data usage for other users.) Ah alright, I did not realize that this is a prototype. Do we intent to commit this code, or is this review solely here for us to look at / download? If we're committing it, could we make it clear somewhere that this is a prototype? (e.g. through some comment at the top of this file). https://codereview.chromium.org/2975693002/diff/240001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java (right): https://codereview.chromium.org/2975693002/diff/240001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:165: } catch (SecurityException e) { Why are we catching SecurityException here? (it's a runtime exception)
On 2017/07/17 14:32:11, gsennton wrote: > Sorry about the massive amounts of comments, I might have let you go easier if I > knew this was a prototype ;) Actually questions are great. Let's keep them in the design document as much as possible to not forget. The whole purpose here is to uncover as much as the ground as possible so we know what we need to do when we release it for product. And we will reuse most of the logic here! > > https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/s... > File > android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java > (right): > > https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/s... > android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:82: > SeedInfo seedInfo = VariationsSeedFetcher.get().downloadContent( > On 2017/07/14 17:41:05, paulmiller wrote: > > On 2017/07/14 12:45:39, gsennton wrote: > > > On 2017/07/14 01:33:57, yiyuny wrote: > > > > On 2017/07/13 20:36:25, Alexei Svitkine (slow) wrote: > > > > > On 2017/07/13 13:31:58, gsennton wrote: > > > > > > What's the size of the data we are downloading here? > > > > > > Do we care about the kind of network we are using for the download? > > (WiFi > > > > vs. > > > > > > data plan). > > > > > > > > > > The chrome one is around 50k. It will be smaller for webview because > only > > > > > specific experiments will target it. > > > > > > > > I'm using NETWORK_TYPE_ANY(data plan/wifi both work) in the next CL. So if > > is > > > > under 50k, I think that will be fine. Thanks to Alexie for providing the > > data. > > > > > > What I mean is, we should discuss whether it is OK to use users' data plans > > for > > > this - WebView is used by 800+ million people, so if all of those users use > > 50k > > > of data over a data plan that means 50kB * 8e8 = 4e10kB = 40TB. So we would > be > > > using 40TB of data plan data (please correct me if my calculations are off). > > > That's quite a lot of data to pay for :P > > > > System health will be an important consideration for when we enable this for > > real, but for this summer project we just want a prototype, which will not be > > enabled in prod. (I also don't think it makes sense to multiply by number of > > users, because each user pays individually. The addition of more WebView users > > doesn't increase data usage for other users.) > > Ah alright, I did not realize that this is a prototype. Do we intent to commit > this code, or is this review solely here for us to look at / download? > > If we're committing it, could we make it clear somewhere that this is a > prototype? (e.g. through some comment at the top of this file). > > https://codereview.chromium.org/2975693002/diff/240001/android_webview/java/s... > File > android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java > (right): > > https://codereview.chromium.org/2975693002/diff/240001/android_webview/java/s... > android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:165: > } catch (SecurityException e) { > Why are we catching SecurityException here? (it's a runtime exception)
I have added a line before the AwVariationsSeedFetchService class commenting it is a prototype of the Variations Seed Fetch Service. ptal https://codereview.chromium.org/2975693002/diff/240001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java (right): https://codereview.chromium.org/2975693002/diff/240001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:165: } catch (SecurityException e) { On 2017/07/17 14:32:10, gsennton wrote: > Why are we catching SecurityException here? (it's a runtime exception) Done.
lgtm, thanks!
The CQ bit was checked by yiyuny@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Still waiting for code to read the new pref and unit test that verifies that reading it matches what was written. https://codereview.chromium.org/2975693002/diff/260001/components/variations/... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java (right): https://codereview.chromium.org/2975693002/diff/260001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java:179: SeedInfo info = null; Nit: Since you don't need to return it outside of line 205, combine this into line 198.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yiyuny@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Add unit tests for reading and writing seed data and preference. Sorry for Gustav, you could revert your lgtm. ptal
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
still lgtm for android_webview/ modulo one comment about cleaning up your test environment. https://codereview.chromium.org/2975693002/diff/280001/android_webview/javate... File android_webview/javatests/src/org/chromium/android_webview/test/AwVariationsSeedFetchServiceTest.java (right): https://codereview.chromium.org/2975693002/diff/280001/android_webview/javate... android_webview/javatests/src/org/chromium/android_webview/test/AwVariationsSeedFetchServiceTest.java:35: public void testReadAndWriteSeedData() throws IOException, ClassNotFoundException { You probably want to clean up the files that were created during the test. I would remove the entire directory returned from getOrCreateWebViewVariationsDir() in the tearDown method.
https://codereview.chromium.org/2975693002/diff/300001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java (right): https://codereview.chromium.org/2975693002/diff/300001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:175: public static SeedPreference getVariationsSeedPreference() Public functions should have javadoc. https://codereview.chromium.org/2975693002/diff/300001/android_webview/javate... File android_webview/javatests/src/org/chromium/android_webview/test/AwVariationsSeedFetchServiceTest.java (right): https://codereview.chromium.org/2975693002/diff/300001/android_webview/javate... android_webview/javatests/src/org/chromium/android_webview/test/AwVariationsSeedFetchServiceTest.java:62: assertTrue(mSeedPrefFile.exists()); I don't think the test needs to check this. It's an implementation detail of the class. To clean up data, you should just add a ClearDataForTesting() function to the class and call that - where the class itself can take care about knowing what needs to be cleared. If you do that, you can make getOrCreate*() function private.
I am figuring out how to make new tests work correctly on trybots. The trybots can't find the AwVariationsSeedFetchService class when running the test. And I can pass the test in the local environment. Does anyone have any idea why it doesn't work on the trybots? And also I have updated the unit tests code. ptal https://codereview.chromium.org/2975693002/diff/280001/android_webview/javate... File android_webview/javatests/src/org/chromium/android_webview/test/AwVariationsSeedFetchServiceTest.java (right): https://codereview.chromium.org/2975693002/diff/280001/android_webview/javate... android_webview/javatests/src/org/chromium/android_webview/test/AwVariationsSeedFetchServiceTest.java:35: public void testReadAndWriteSeedData() throws IOException, ClassNotFoundException { On 2017/07/18 07:38:57, gsennton wrote: > You probably want to clean up the files that were created during the test. > I would remove the entire directory returned from > getOrCreateWebViewVariationsDir() in the tearDown method. Done. https://codereview.chromium.org/2975693002/diff/300001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java (right): https://codereview.chromium.org/2975693002/diff/300001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:175: public static SeedPreference getVariationsSeedPreference() On 2017/07/18 19:09:40, Alexei Svitkine (slow) wrote: > Public functions should have javadoc. Done. https://codereview.chromium.org/2975693002/diff/300001/android_webview/javate... File android_webview/javatests/src/org/chromium/android_webview/test/AwVariationsSeedFetchServiceTest.java (right): https://codereview.chromium.org/2975693002/diff/300001/android_webview/javate... android_webview/javatests/src/org/chromium/android_webview/test/AwVariationsSeedFetchServiceTest.java:62: assertTrue(mSeedPrefFile.exists()); On 2017/07/18 19:09:40, Alexei Svitkine (slow) wrote: > I don't think the test needs to check this. It's an implementation detail of the > class. > > To clean up data, you should just add a ClearDataForTesting() function to the > class and call that - where the class itself can take care about knowing what > needs to be cleared. > > If you do that, you can make getOrCreate*() function private. Done.
The CQ bit was checked by yiyuny@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ptal
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Still lgtm, thanks!
lgtm https://codereview.chromium.org/2975693002/diff/420001/components/variations/... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java (right): https://codereview.chromium.org/2975693002/diff/420001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java:245: public static byte[] convertInputStreamToByteArray(InputStream inputStream) throws IOException { Add JavaDoc if you're making it public.
lgtm https://codereview.chromium.org/2975693002/diff/440001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java (right): https://codereview.chromium.org/2975693002/diff/440001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:221: @SuppressFBWarnings("RV_RETURN_VALUE_IGNORED_BAD_PRACTICE") // ignoring File.delete() return renameTo also returns success/fail, yes? https://codereview.chromium.org/2975693002/diff/440001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:232: Log.e(TAG, "Failed to close stream." + e); might want a space after the .
On 2017/07/20 18:06:21, paulmiller wrote: > lgtm > > https://codereview.chromium.org/2975693002/diff/440001/android_webview/java/s... > File > android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java > (right): > > https://codereview.chromium.org/2975693002/diff/440001/android_webview/java/s... > android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:221: > @SuppressFBWarnings("RV_RETURN_VALUE_IGNORED_BAD_PRACTICE") // ignoring > File.delete() return > renameTo also returns success/fail, yes? > > https://codereview.chromium.org/2975693002/diff/440001/android_webview/java/s... > android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:232: > Log.e(TAG, "Failed to close stream." + e); > might want a space after the . oh and also s/refactory/refactor
https://codereview.chromium.org/2975693002/diff/440001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java (right): https://codereview.chromium.org/2975693002/diff/440001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:221: @SuppressFBWarnings("RV_RETURN_VALUE_IGNORED_BAD_PRACTICE") // ignoring File.delete() return On 2017/07/20 18:06:20, paulmiller wrote: > renameTo also returns success/fail, yes? Done. https://codereview.chromium.org/2975693002/diff/440001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:232: Log.e(TAG, "Failed to close stream." + e); On 2017/07/20 18:06:20, paulmiller wrote: > might want a space after the . Done.
The CQ bit was checked by yiyuny@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from gsennton@chromium.org, asvitkine@chromium.org, paulmiller@chromium.org Link to the patchset: https://codereview.chromium.org/2975693002/#ps460001 (title: "Update for comments of Patch 23")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Thanks for the code and for all the review comments. I am stamping lgtm note: before landing please add enough detail to the cl description. It is too brief for such an important change. Even the bug is too sparse in terms of information. 1. what is the service for? 2. what triggers the download, are we planning to enable right away? 3. where do we store the data 4.will there be follow up cls.
Thanks for the code and for all the review comments. I am stamping lgtm note: before landing please add enough detail to the cl description. It is too brief for such an important change. Even the bug is too sparse in terms of information. 1. what is the service for? 2. what triggers the download, are we planning to enable right away? 3. where do we store the data 4.will there be follow up cls.
Description was changed from ========== Add AwVariationsSeedFetchService and refactory VariationsSeedFetcher BUG=733857 ========== to ========== Add AwVariationsSeedFetchService and refactory VariationsSeedFetcher The CL adds the AwVariationsSeedFetchService which is a JobService to fetch the Finch seed data from the Finch Server. It reuses some of the code from the VariationsSeedFetcher, so I refactory the class to expose some reusable function. The seed fetch is triggered by AwVariationsConfigurationService which is a bound Service to manage the Finch seed data system-wide. The service will schedule a seed fetch job to the AwVariationsSeedFetchService when the local seed is expired. The fetched seed data is stored in the local directory belongs to the Android WebView. This is one part of the work of fetching Finch seed and there will be another CL related to the AwVariationsConfigurationService. BUG=733857 ==========
The CQ bit was checked by yiyuny@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from paulmiller@chromium.org, sgurun@chromium.org, gsennton@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2975693002/#ps480001 (title: "Fix typo in comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add AwVariationsSeedFetchService and refactory VariationsSeedFetcher The CL adds the AwVariationsSeedFetchService which is a JobService to fetch the Finch seed data from the Finch Server. It reuses some of the code from the VariationsSeedFetcher, so I refactory the class to expose some reusable function. The seed fetch is triggered by AwVariationsConfigurationService which is a bound Service to manage the Finch seed data system-wide. The service will schedule a seed fetch job to the AwVariationsSeedFetchService when the local seed is expired. The fetched seed data is stored in the local directory belongs to the Android WebView. This is one part of the work of fetching Finch seed and there will be another CL related to the AwVariationsConfigurationService. BUG=733857 ========== to ========== Add AwVariationsSeedFetchService and refactory VariationsSeedFetcher The CL add the AwVariationsSeedFetchService which is a JobService to fetch Finch seed data from the Finch Server. It reuse some of the code from the VariationsSeedFetcher, so I refactory the class to expose some reusable function. The download is triggered by AwVariationsConfigurationService which is a bound Service to managing the Finch seed data system-wide. The fetched seed data is going to store in the local directory belongs to the Android WebView. This is just a prototype of the Variations Seed Fetch Service which is one part of the work of adding Finch to Android WebView, so there will be another CL related to the AwVariationsConfigurationService. BUG=733857 ==========
CQ is committing da patch.
Bot data: {"patchset_id": 480001, "attempt_start_ts": 1500658230588010,
"parent_rev": "79ed387ca6cc60566739c9bba268ef767b499f12", "commit_rev":
"f7857086902db2e23fa374fce4b8ee8378f48e9d"}
Message was sent while issue was closed.
Description was changed from ========== Add AwVariationsSeedFetchService and refactory VariationsSeedFetcher The CL add the AwVariationsSeedFetchService which is a JobService to fetch Finch seed data from the Finch Server. It reuse some of the code from the VariationsSeedFetcher, so I refactory the class to expose some reusable function. The download is triggered by AwVariationsConfigurationService which is a bound Service to managing the Finch seed data system-wide. The fetched seed data is going to store in the local directory belongs to the Android WebView. This is just a prototype of the Variations Seed Fetch Service which is one part of the work of adding Finch to Android WebView, so there will be another CL related to the AwVariationsConfigurationService. BUG=733857 ========== to ========== Add AwVariationsSeedFetchService and refactory VariationsSeedFetcher The CL add the AwVariationsSeedFetchService which is a JobService to fetch Finch seed data from the Finch Server. It reuse some of the code from the VariationsSeedFetcher, so I refactory the class to expose some reusable function. The download is triggered by AwVariationsConfigurationService which is a bound Service to managing the Finch seed data system-wide. The fetched seed data is going to store in the local directory belongs to the Android WebView. This is just a prototype of the Variations Seed Fetch Service which is one part of the work of adding Finch to Android WebView, so there will be another CL related to the AwVariationsConfigurationService. BUG=733857 Review-Url: https://codereview.chromium.org/2975693002 Cr-Commit-Position: refs/heads/master@{#488719} Committed: https://chromium.googlesource.com/chromium/src/+/f7857086902db2e23fa374fce4b8... ==========
Message was sent while issue was closed.
Committed patchset #25 (id:480001) as https://chromium.googlesource.com/chromium/src/+/f7857086902db2e23fa374fce4b8...
Message was sent while issue was closed.
On 2017/07/21 02:31:37, sgurun wrote: > Thanks for the code and for all the review comments. I am stamping > lgtm > > note: before landing please add enough detail to the cl description. It is too > brief for such an important change. Even the bug is too sparse in terms of > information. > > 1. what is the service for? > 2. what triggers the download, are we planning to enable right away? > 3. where do we store the data > 4.will there be follow up cls. Thanks for all the review comments. |
