|
|
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. |
DescriptionAdd 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 #
Messages
Total messages: 44 (39 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) 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...
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...
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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: 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Add AwVariationsSeedFetcher and refactory VariationsSeedFetcher BUG=733857 ========== to ========== Add AwVariationsSeedFetcher and refactory VariationsSeedFetcher BUG=733857 ==========
yiyuny@google.com changed reviewers: + asvitkine@chromium.org, paulmiller@chromium.org
On 2017/07/06 18:36:27, yiyuny wrote: > mailto:yiyuny@google.com changed reviewers: > + mailto:asvitkine@chromium.org, mailto:paulmiller@chromium.org ptal
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:27: public static final String VARIATIONS_PLATFORM = "android_webview"; I think this is fine to put in the VariationsSeedFetcher as VARIATIONS_PLATFORM_ANDROID or just PLATFORM_ANDROID. https://codereview.chromium.org/2970993002/diff/100001/components/variations/... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java (right): https://codereview.chromium.org/2970993002/diff/100001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java:35: public static final String VARIATIONS_PLATFORM = "android"; Nit: VARIATIONS_PLATFORM_ANDROID Also, add a blank line below it. https://codereview.chromium.org/2970993002/diff/100001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java:177: headFields.put(IM_HEADERFIELD, getHeaderFieldOrEmpty(connection, IM_HEADERFIELD)); Suggest defining a simple class with these fields + the seed bytes instead of using a map out-parameter. Then, it could still be a simple return value without any map params and you don't need the constants for the header names - keep them inline here in one place. https://codereview.chromium.org/2970993002/diff/100001/components/variations/... File components/variations/android/junit/src/org/chromium/components/variations/firstrun/VariationsSeedFetcherTest.java (right): https://codereview.chromium.org/2970993002/diff/100001/components/variations/... components/variations/android/junit/src/org/chromium/components/variations/firstrun/VariationsSeedFetcherTest.java:59: + VariationsSeedFetcher.VARIATIONS_PLATFORM, It's not great to leak the details of how to construct the URL outside of the class. How about having the function just take the platform name as a parameter and construct the full URL itself?
I don't see any non-static fields in either VariationsSeedFetcher or AwVariationsSeedFetcher. What's the point of instantiating them? Also, rather than making various private bits of VariationsSeedFetcher public, what about making them protected, and making AwVariationsSeedFetcher a subclass of VariationsSeedFetcher? (Not that you have to do it that way. Just a suggestion.)
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. I don't think this is true in WebView's case. https://codereview.chromium.org/2970993002/diff/100001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetcher.java:31: // Synchronization lock Superfluous comment (not yours, but still). https://codereview.chromium.org/2970993002/diff/100001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetcher.java:74: Log.e(TAG, "FileNotFoundException storing seed: ", e); Mention Finch in these error messages, so people have some idea where they're coming from. Remember, logcat is noisy, and these will be mixed in with logs from all over the place. Also, I think printing the stack (which is what happens when you pass in e) is unnecessarily verbose, since there's only one codepath leading to this function. logcat already has too many non-actionable stack traces in it. https://codereview.chromium.org/2970993002/diff/100001/components/variations/... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java (right): https://codereview.chromium.org/2970993002/diff/100001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java:193: return rawSeed; It looks like this can be null, but AwVariationsSeedFetcher assumes it's not null. Since it's not obvious from the method name that it might return null, a safer design might be to propagate the exception rather than returning null. Then change AwVariationsSeedFetcher to handle the exception.
sgurun@chromium.org changed reviewers: + sgurun@chromium.org
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
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.
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 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 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...) |