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

Issue 2975693002: Add AwVariationsSeedFetchService and refactory VariationsSeedFetcher (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months, 2 weeks ago by yiyuny
Modified:
2 months 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 months, 2 weeks 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 ...
2 months, 1 week 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 ...
2 months, 1 week 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: > ...
2 months, 1 week 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, ...
2 months, 1 week 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()) ...
2 months, 1 week 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: > ...
2 months, 1 week 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 ...
2 months, 1 week 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> ...
2 months, 1 week ago (2017-07-12 15:52:50 UTC) #29
sgurun-gerrit only
Gustav, since you already wrote one service and became an expert, can you take review ...
2 months, 1 week ago (2017-07-12 16:12:26 UTC) #32
sgurun-gerrit only
and thanks Alexei for doing a through review!
2 months, 1 week 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 ...
2 months, 1 week 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 ...
2 months, 1 week 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 ...
2 months, 1 week 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 ...
2 months, 1 week 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 ...
2 months, 1 week 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: ...
2 months, 1 week 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: > ...
2 months, 1 week 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 ...
2 months, 1 week 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 ...
2 months, 1 week 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: ...
2 months, 1 week ago (2017-07-14 17:41:06 UTC) #55
sgurun-gerrit only
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): > > ...
2 months, 1 week 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 ...
2 months, 1 week 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 ...
2 months, 1 week 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 ...
2 months, 1 week 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 ...
2 months ago (2017-07-17 14:32:11 UTC) #72
sgurun-gerrit only
On 2017/07/17 14:32:11, gsennton wrote: > Sorry about the massive amounts of comments, I might ...
2 months 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 ...
2 months ago (2017-07-17 16:37:06 UTC) #74
gsennton
lgtm, thanks!
2 months 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 ...
2 months 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 ...
2 months 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 ...
2 months 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. ...
2 months 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 ...
2 months ago (2017-07-18 21:44:58 UTC) #88
yiyuny
ptal
2 months ago (2017-07-20 00:05:29 UTC) #91
gsennton
Still lgtm, thanks!
2 months 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 { ...
2 months 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 ...
2 months 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 > ...
2 months 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 ...
2 months 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
2 months 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)
2 months ago (2017-07-20 18:35:23 UTC) #103
sgurun-gerrit only
Thanks for the code and for all the review comments. I am stamping lgtm note: ...
2 months ago (2017-07-21 02:31:31 UTC) #104
sgurun-gerrit only
Thanks for the code and for all the review comments. I am stamping lgtm note: ...
2 months 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 months 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 months ago (2017-07-21 18:43:01 UTC) #113
yiyuny
2 months 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 b40b6558b