Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(71)

Issue 2970993002: Add AwVariationsSeedFetcher and refactory VariationsSeedFetcher (Closed)

Created:
3 years, 5 months ago by yiyuny
Modified:
3 years, 4 months ago
CC:
chromium-reviews, android-webview-reviews_chromium.org, asvitkine+watch_chromium.org, agrieve+watch_chromium.org, sgurun-gerrit only, Tima Vaisburd, kmilka
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Add AwVariationsSeedFetcher and refactory VariationsSeedFetcher BUG=733857

Patch Set 1 #

Patch Set 2 : Update unittest for refactorying VariationsSeedFetcher #

Patch Set 3 : Update unittest #

Patch Set 4 : Remove unrelated imports #

Patch Set 5 : Add variations in DEPS of android_webview #

Patch Set 6 : Update unittest #

Total comments: 16

Patch Set 7 : Update desgin of AwVariationsSeedFetcher #

Patch Set 8 : Update comments #

Patch Set 9 : Update logic in handling Exceptions #

Unified diffs Side-by-side diffs Delta from patch set Stats (+163 lines, -18 lines) Patch
M android_webview/BUILD.gn View 2 chunks +2 lines, -0 lines 0 comments Download
M android_webview/DEPS View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
A android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetcher.java View 1 2 3 4 5 6 7 8 1 chunk +104 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 chunks +53 lines, -17 lines 0 comments Download
M components/variations/android/junit/src/org/chromium/components/variations/firstrun/VariationsSeedFetcherTest.java View 1 2 3 4 5 6 1 chunk +3 lines, -1 line 0 comments Download

Messages

Total messages: 44 (39 generated)
yiyuny
On 2017/07/06 18:36:27, yiyuny wrote: > mailto:yiyuny@google.com changed reviewers: > + mailto:asvitkine@chromium.org, mailto:paulmiller@chromium.org ptal
3 years, 5 months ago (2017-07-06 18:38:09 UTC) #27
Alexei Svitkine (slow)
https://codereview.chromium.org/2970993002/diff/100001/android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetcher.java File android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetcher.java (right): https://codereview.chromium.org/2970993002/diff/100001/android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetcher.java#newcode27 android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetcher.java:27: public static final String VARIATIONS_PLATFORM = "android_webview"; I think ...
3 years, 5 months ago (2017-07-06 22:01:10 UTC) #28
paulmiller
I don't see any non-static fields in either VariationsSeedFetcher or AwVariationsSeedFetcher. What's the point of ...
3 years, 5 months ago (2017-07-06 23:07:04 UTC) #29
paulmiller
https://codereview.chromium.org/2970993002/diff/100001/android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetcher.java File android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetcher.java (right): https://codereview.chromium.org/2970993002/diff/100001/android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetcher.java#newcode22 android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetcher.java:22: * Fetches the variations seed before the actual first ...
3 years, 5 months ago (2017-07-06 23:09:05 UTC) #30
sgurun-gerrit only
3 years, 5 months ago (2017-07-06 23:38:11 UTC) #32
https://codereview.chromium.org/2970993002/diff/100001/android_webview/java/s...
File
android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetcher.java
(right):

https://codereview.chromium.org/2970993002/diff/100001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetcher.java:22:
* Fetches the variations seed before the actual first run of Android WebView.
On 2017/07/06 23:09:05, paulmiller wrote:
> I don't think this is true in WebView's case.

yep not true. Please update class doc to explain in more detail.

https://codereview.chromium.org/2970993002/diff/100001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetcher.java:36:
AwVariationsSeedFetcher() {}
is this class supposed to be publicly instantiatable? Otherwise private.

Himm, actually something is weird in this class. It has no state, so no need to
instantiate it at all. the get method is not necessary.

https://codereview.chromium.org/2970993002/diff/100001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetcher.java:39:
// TODO(aberent) Check not running on UI thread. Doing so however makes
Robolectric testing
why are we expecting aberent to fix that? is this a copy paste error?

https://codereview.chromium.org/2970993002/diff/100001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetcher.java:43:
Log.d(TAG, "using android webview fetcher");
does not look necessary, remove.

https://codereview.chromium.org/2970993002/diff/100001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetcher.java:50:
public void fetchSeed(String restrictMode) {
static

https://codereview.chromium.org/2970993002/diff/100001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetcher.java:50:
public void fetchSeed(String restrictMode) {
and who is going to call that? write tests maybe?

https://codereview.chromium.org/2970993002/diff/100001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetcher.java:52:
// Prevent multiple simultaneous fetches
document how multiple simultanous fetches are possible.

https://codereview.chromium.org/2970993002/diff/100001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetcher.java:62:
private void storeSeed(byte[] rawSeed, Map<String, String> headerFields) {
static

Powered by Google App Engine
This is Rietveld 408576698