Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(7)

Issue 2975693002: Add AwVariationsSeedFetchService and refactory VariationsSeedFetcher (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 weeks, 1 day ago by yiyuny
Modified:
1 day, 23 hours ago
CC:
chromium-reviews, android-webview-reviews_chromium.org, asvitkine+watch_chromium.org, agrieve+watch_chromium.org, Tima Vaisburd, kmilka
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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/+/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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+395 lines, -33 lines) Patch
M android_webview/BUILD.gn View 2 chunks +2 lines, -0 lines 0 comments Download
M android_webview/apk/java/AndroidManifest.xml View 1 2 3 4 1 chunk +4 lines, -0 lines 0 comments Download
M android_webview/java/DEPS View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +1 line, -0 lines 0 comments Download
A android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 1 chunk +237 lines, -0 lines 0 comments Download
M android_webview/javatests/DEPS View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +1 line, -0 lines 0 comments Download
A android_webview/javatests/src/org/chromium/android_webview/test/AwVariationsSeedFetchServiceTest.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +58 lines, -0 lines 0 comments Download
M android_webview/test/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +2 lines, -0 lines 0 comments Download
M components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 11 chunks +87 lines, -32 lines 0 comments Download
M components/variations/android/junit/src/org/chromium/components/variations/firstrun/VariationsSeedFetcherTest.java View 1 chunk +3 lines, -1 line 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 114 (67 generated)
yiyuny
On 2017/07/08 00:08:05, yiyuny wrote: > The CQ bit was checked by mailto:yiyuny@google.com to run ...
2 weeks, 1 day ago (2017-07-08 00:08:26 UTC) #6
paulmiller
In your commit message: s/refactory/refactor/. Also say a little more in the body about what ...
1 week, 6 days ago (2017-07-10 18:23:13 UTC) #12
paulmiller
https://codereview.chromium.org/2975693002/diff/60001/android_webview/apk/java/AndroidManifest.xml File android_webview/apk/java/AndroidManifest.xml (right): https://codereview.chromium.org/2975693002/diff/60001/android_webview/apk/java/AndroidManifest.xml#newcode41 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 ...
1 week, 5 days ago (2017-07-10 23:37:40 UTC) #15
yiyuny
ptal https://codereview.chromium.org/2975693002/diff/60001/android_webview/apk/java/AndroidManifest.xml File android_webview/apk/java/AndroidManifest.xml (right): https://codereview.chromium.org/2975693002/diff/60001/android_webview/apk/java/AndroidManifest.xml#newcode41 android_webview/apk/java/AndroidManifest.xml:41: <service android:name="org.chromium.android_webview.AwVariationsSeedFetchService" On 2017/07/10 18:23:12, paulmiller wrote: > ...
1 week, 4 days ago (2017-07-11 19:15:19 UTC) #18
Alexei Svitkine (slow)
https://codereview.chromium.org/2975693002/diff/80001/android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java File android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java (right): https://codereview.chromium.org/2975693002/diff/80001/android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java#newcode51 android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:51: // When job parameter is not satisfied any more, ...
1 week, 4 days ago (2017-07-11 19:37:24 UTC) #19
paulmiller
Don't forget to fix the commit message. https://codereview.chromium.org/2975693002/diff/80001/android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java File android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java (right): https://codereview.chromium.org/2975693002/diff/80001/android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java#newcode77 android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:77: if (sLock.tryLock()) ...
1 week, 4 days ago (2017-07-11 20:51:36 UTC) #22
Alexei Svitkine (slow)
https://codereview.chromium.org/2975693002/diff/80001/android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java File android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java (right): https://codereview.chromium.org/2975693002/diff/80001/android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java#newcode77 android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:77: if (sLock.tryLock()) { On 2017/07/11 20:51:36, paulmiller wrote: > ...
1 week, 4 days ago (2017-07-11 20:53:48 UTC) #23
yiyuny
ptal https://codereview.chromium.org/2975693002/diff/80001/android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java File android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java (right): https://codereview.chromium.org/2975693002/diff/80001/android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java#newcode51 android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:51: // When job parameter is not satisfied any ...
1 week, 4 days ago (2017-07-11 22:43:37 UTC) #26
Alexei Svitkine (slow)
Almost there. https://codereview.chromium.org/2975693002/diff/100001/android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java File android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java (right): https://codereview.chromium.org/2975693002/diff/100001/android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java#newcode55 android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:55: private class FetchFinchSeedDataTask extends AsyncTask<Void, Void, Void> ...
1 week, 4 days ago (2017-07-12 15:52:50 UTC) #29
sgurun
Gustav, since you already wrote one service and became an expert, can you take review ...
1 week, 4 days ago (2017-07-12 16:12:26 UTC) #32
sgurun
and thanks Alexei for doing a through review!
1 week, 4 days ago (2017-07-12 16:12:47 UTC) #33
yiyuny
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#newcode13 android_webview/DEPS:13: "+components/variations", On 2017/07/12 16:12:25, sgurun wrote: > can ...
1 week, 4 days ago (2017-07-12 18:31:31 UTC) #36
Alexei Svitkine (slow)
https://codereview.chromium.org/2975693002/diff/100001/android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java File android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java (right): https://codereview.chromium.org/2975693002/diff/100001/android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java#newcode55 android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:55: private class FetchFinchSeedDataTask extends AsyncTask<Void, Void, Void> { On ...
1 week, 3 days ago (2017-07-12 19:01:22 UTC) #37
paulmiller
https://codereview.chromium.org/2975693002/diff/120001/android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java File android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java (right): https://codereview.chromium.org/2975693002/diff/120001/android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java#newcode143 android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:143: "FileNotFoundException failed to open file to write seed data ...
1 week, 3 days ago (2017-07-12 20:21:59 UTC) #38
Alexei Svitkine (slow)
https://codereview.chromium.org/2975693002/diff/120001/android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java File android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java (right): https://codereview.chromium.org/2975693002/diff/120001/android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java#newcode143 android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:143: "FileNotFoundException failed to open file to write seed data ...
1 week, 3 days ago (2017-07-12 20:32:31 UTC) #39
yiyuny
ptal https://codereview.chromium.org/2975693002/diff/120001/android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java File android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java (right): https://codereview.chromium.org/2975693002/diff/120001/android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java#newcode97 android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:97: * SeedPreference serialize related fields of seed data ...
1 week, 3 days ago (2017-07-12 22:22:06 UTC) #42
gsennton
Alright, modulo the inline comments and one question I think this (android_webview/) looks fine. Question: ...
1 week, 3 days ago (2017-07-13 13:31:59 UTC) #47
Alexei Svitkine (slow)
https://codereview.chromium.org/2975693002/diff/120001/android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java File android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java (right): https://codereview.chromium.org/2975693002/diff/120001/android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java#newcode135 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: > ...
1 week, 2 days ago (2017-07-13 20:36:25 UTC) #48
yiyuny
The service is a just to fetch the seed data from the finch server and ...
1 week, 2 days ago (2017-07-14 01:33:57 UTC) #51
gsennton
Thanks for the fixes Yiyun. Sorry for not looking at the design doc earlier, it ...
1 week, 2 days ago (2017-07-14 12:45:39 UTC) #54
paulmiller
https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java File android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java (right): https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java#newcode82 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: ...
1 week, 2 days ago (2017-07-14 17:41:06 UTC) #55
sgurun
On 2017/07/14 17:41:06, paulmiller wrote: > https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java > File > android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java > (right): > > ...
1 week, 2 days ago (2017-07-14 17:52:06 UTC) #56
paulmiller
https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java File android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java (right): https://codereview.chromium.org/2975693002/diff/140001/android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java#newcode146 android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:146: "FileNotFoundException failed to open file to write seed data ...
1 week, 1 day ago (2017-07-14 23:06:24 UTC) #63
yiyuny
Sorry for the delay and I have updated the current design in it. As Selim ...
1 week, 1 day ago (2017-07-15 00:19:00 UTC) #68
yiyuny
On 2017/07/15 00:19:00, yiyuny wrote: > Sorry for the delay and I have updated the ...
1 week, 1 day ago (2017-07-15 00:31:34 UTC) #69
gsennton
Sorry about the massive amounts of comments, I might have let you go easier if ...
6 days, 4 hours ago (2017-07-17 14:32:11 UTC) #72
sgurun
On 2017/07/17 14:32:11, gsennton wrote: > Sorry about the massive amounts of comments, I might ...
6 days, 3 hours ago (2017-07-17 15:24:36 UTC) #73
yiyuny
I have added a line before the AwVariationsSeedFetchService class commenting it is a prototype of ...
6 days, 2 hours ago (2017-07-17 16:37:06 UTC) #74
gsennton
lgtm, thanks!
6 days, 2 hours ago (2017-07-17 16:42:04 UTC) #75
Alexei Svitkine (slow)
Still waiting for code to read the new pref and unit test that verifies that ...
6 days, 1 hour ago (2017-07-17 17:06:45 UTC) #78
yiyuny
Add unit tests for reading and writing seed data and preference. Sorry for Gustav, you ...
5 days, 19 hours ago (2017-07-17 23:00:09 UTC) #83
gsennton
still lgtm for android_webview/ modulo one comment about cleaning up your test environment. https://codereview.chromium.org/2975693002/diff/280001/android_webview/javatests/src/org/chromium/android_webview/test/AwVariationsSeedFetchServiceTest.java File ...
5 days, 11 hours ago (2017-07-18 07:38:57 UTC) #86
Alexei Svitkine (slow)
https://codereview.chromium.org/2975693002/diff/300001/android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java File android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java (right): https://codereview.chromium.org/2975693002/diff/300001/android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java#newcode175 android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java:175: public static SeedPreference getVariationsSeedPreference() Public functions should have javadoc. ...
4 days, 23 hours ago (2017-07-18 19:09:41 UTC) #87
yiyuny
I am figuring out how to make new tests work correctly on trybots. The trybots ...
4 days, 20 hours ago (2017-07-18 21:44:58 UTC) #88
yiyuny
ptal
3 days, 18 hours ago (2017-07-20 00:05:29 UTC) #91
gsennton
Still lgtm, thanks!
3 days, 7 hours ago (2017-07-20 11:04:32 UTC) #94
Alexei Svitkine (slow)
lgtm https://codereview.chromium.org/2975693002/diff/420001/components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java (right): https://codereview.chromium.org/2975693002/diff/420001/components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java#newcode245 components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java:245: public static byte[] convertInputStreamToByteArray(InputStream inputStream) throws IOException { ...
3 days, 4 hours ago (2017-07-20 14:36:36 UTC) #95
paulmiller
lgtm https://codereview.chromium.org/2975693002/diff/440001/android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java File android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java (right): https://codereview.chromium.org/2975693002/diff/440001/android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java#newcode221 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 ...
3 days ago (2017-07-20 18:06:21 UTC) #96
paulmiller
On 2017/07/20 18:06:21, paulmiller wrote: > lgtm > > https://codereview.chromium.org/2975693002/diff/440001/android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java > File > android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java > ...
3 days ago (2017-07-20 18:07:00 UTC) #97
yiyuny
https://codereview.chromium.org/2975693002/diff/440001/android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java File android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java (right): https://codereview.chromium.org/2975693002/diff/440001/android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java#newcode221 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 ...
3 days ago (2017-07-20 18:20:45 UTC) #98
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2975693002/460001
3 days ago (2017-07-20 18:21:23 UTC) #101
commit-bot: I haz the power
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_presubmit/builds/495456)
3 days ago (2017-07-20 18:35:23 UTC) #103
sgurun
Thanks for the code and for all the review comments. I am stamping lgtm note: ...
2 days, 16 hours ago (2017-07-21 02:31:31 UTC) #104
sgurun
Thanks for the code and for all the review comments. I am stamping lgtm note: ...
2 days, 16 hours ago (2017-07-21 02:31:37 UTC) #105
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2975693002/480001
2 days, 1 hour ago (2017-07-21 17:30:41 UTC) #109
commit-bot: I haz the power
Committed patchset #25 (id:480001) as https://chromium.googlesource.com/chromium/src/+/f7857086902db2e23fa374fce4b8ee8378f48e9d
2 days ago (2017-07-21 18:43:01 UTC) #113
yiyuny
1 day, 23 hours ago (2017-07-21 18:53:25 UTC) #114
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 25c286973