Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1417733008:
Java code for fetching variations first run seed after receiving chrome.TOS_ACKED broadcast. (Closed)
|
Created:
5 years, 1 month ago by Alexander Agulenko Modified:
5 years, 1 month ago CC:
chromium-reviews, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionJava code for fetching variations first run seed after receiving chrome.TOS_ACKED broadcast.
I implemented BroadcastReceiver that starts a service upon receiving a chrome.TOS_ACKED broadcast. The service makes a HTTP request to Google servers to fetch the seed and stores it in SharedPreferences.
After Chrome start Chromium core uses a bridge to get this seed from Java side (see https://codereview.chromium.org/1417503010/).
BUG=551029
Committed: https://crrev.com/2d59816a69420e25c1ff39fb55d2d6f245c424b3
Cr-Commit-Position: refs/heads/master@{#359036}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Moved code for fetching variations first run seed to components/variations #Patch Set 3 : Minor fixes: changed comments and removed unused includes and usings #
Total comments: 43
Patch Set 4 : Code review comments fixed #
Total comments: 4
Patch Set 5 : IOException is caught instead of generic; added HTTP response code validation #
Total comments: 2
Patch Set 6 : Minor codestyle fix #
Total comments: 6
Patch Set 7 : Moved exception catch closer to place of possible raise #
Total comments: 11
Patch Set 8 : Splitted fetching code into two functions #
Total comments: 2
Patch Set 9 : Minor change in logging message #
Total comments: 2
Patch Set 10 : Removed unnecessary exception throw declaration #Messages
Total messages: 56 (16 generated)
Description was changed from ========== Java code for fetching variations first run seed after receiving chrome.TOS_ACKED broadcast. BUG=551029 ========== to ========== Java code for fetching variations first run seed after receiving chrome.TOS_ACKED broadcast. I implemented BroadcastReceiver that starts a service upon receiving a chrome.TOS_ACKED broadcast. The service makes a HTTP request to Google servers to fetch the seed and stores it in SharedPreferences. After Chrome start Chromium core uses a bridge to get this seed from Java side (see https://codereview.chromium.org/1417503010/). BUG=551029 ==========
The CQ bit was checked by agulenko@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1417733008/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1417733008/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
The CQ bit was checked by asvitkine@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1417733008/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1417733008/1
https://codereview.chromium.org/1417733008/diff/1/chrome/browser/android/chro... File chrome/browser/android/chrome_jni_registrar.cc (right): https://codereview.chromium.org/1417733008/diff/1/chrome/browser/android/chro... chrome/browser/android/chrome_jni_registrar.cc:347: VariationsSeedBridge::RegisterVariationsSeedBridge}, Seems like we already have "Variations" above? Can the new code live there? i.e. in components/variations/android ? https://codereview.chromium.org/1417733008/diff/1/chrome/browser/android/firs... File chrome/browser/android/firstrun/variations/variations_seed_bridge.cc (right): https://codereview.chromium.org/1417733008/diff/1/chrome/browser/android/firs... chrome/browser/android/firstrun/variations/variations_seed_bridge.cc:18: using base::android::CheckException; I don't see this being used. Please update the list of using decls and includes to only what's needed. https://codereview.chromium.org/1417733008/diff/1/chrome/browser/android/firs... File chrome/browser/android/firstrun/variations/variations_seed_bridge.h (right): https://codereview.chromium.org/1417733008/diff/1/chrome/browser/android/firs... chrome/browser/android/firstrun/variations/variations_seed_bridge.h:16: static void GetVariationsFirstRunSeed(std::string* seed_data, Can this be a standalone method instead of a static on class? e.g. similar to: https://code.google.com/p/chromium/codesearch#chromium/src/components/variati...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
agulenko@google.com changed reviewers: + holte@chromium.org, newt@chromium.org
agulenko@google.com changed required reviewers: + asvitkine@chromium.org, newt@chromium.org
Fixed codereview comments and tested the code. Alexei, please run Dry Run for this CL to figure out any possible errors ASAP. Thanks in advance! https://codereview.chromium.org/1417733008/diff/1/chrome/browser/android/chro... File chrome/browser/android/chrome_jni_registrar.cc (right): https://codereview.chromium.org/1417733008/diff/1/chrome/browser/android/chro... chrome/browser/android/chrome_jni_registrar.cc:347: VariationsSeedBridge::RegisterVariationsSeedBridge}, On 2015/11/05 16:38:26, Alexei Svitkine (slow) wrote: > Seems like we already have "Variations" above? > > Can the new code live there? i.e. in components/variations/android ? > Done. https://codereview.chromium.org/1417733008/diff/1/chrome/browser/android/firs... File chrome/browser/android/firstrun/variations/variations_seed_bridge.cc (right): https://codereview.chromium.org/1417733008/diff/1/chrome/browser/android/firs... chrome/browser/android/firstrun/variations/variations_seed_bridge.cc:18: using base::android::CheckException; On 2015/11/05 16:38:26, Alexei Svitkine (slow) wrote: > I don't see this being used. Please update the list of using decls and includes > to only what's needed. Done. https://codereview.chromium.org/1417733008/diff/1/chrome/browser/android/firs... File chrome/browser/android/firstrun/variations/variations_seed_bridge.h (right): https://codereview.chromium.org/1417733008/diff/1/chrome/browser/android/firs... chrome/browser/android/firstrun/variations/variations_seed_bridge.h:16: static void GetVariationsFirstRunSeed(std::string* seed_data, On 2015/11/05 16:38:27, Alexei Svitkine (slow) wrote: > Can this be a standalone method instead of a static on class? > > e.g. similar to: > > https://code.google.com/p/chromium/codesearch#chromium/src/components/variati... Done.
The CQ bit was checked by asvitkine@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1417733008/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1417733008/40001
https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedBridge.java (right): https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedBridge.java:16: * before the Nit: Wrapping is off. https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedBridge.java:23: Nit: Remove empty line. https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java (right): https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:51: private class DownloadTask extends AsyncTask<URL, Void, Boolean> { Nit: Make this a separate top-level class with downloadContent a private method of it. You can put the various public statics inside it. Call it VariationsSeedDownloadTask.java This way, when you add the second service, it can just re-use this code. https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:62: private Boolean downloadContent(URL variationsServerUrl) Boolean -> boolean The uppercase version is an object instead of a primitive which doesn't seem useful here. https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:69: connection.setRequestMethod("GET"); Nit: Per the docs, "The default method is GET." So no need for this line. https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:76: Log.d(TAG, "variationsTracker: Response code = " + Integer.toString(responseCode)); Nit: What's variationsTracker? Also, I don't think Integer.toString() is needed. Java will do this implicitly when you use string + int. https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:93: public byte[] convertInputStreamToByteArray(InputStream inputStream) Nit: Should be private. Document params. https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... File components/variations/android/variations_seed_bridge.h (right): https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... components/variations/android/variations_seed_bridge.h:12: Nit: Can remove this empty line. Same on line 23 and in the other file.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
General question: what happens if Chrome is offline when tos acked is broadcast? Seems like we'll just fail to download the seed. Is that acceptable, or should we try again later when the device does have connectivity? https://codereview.chromium.org/1417733008/diff/40001/chrome/android/java/And... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/1417733008/diff/40001/chrome/android/java/And... chrome/android/java/AndroidManifest.xml:468: <receiver android:name="org.chromium.components.variations.firstrun.VariationsSeedServiceLauncher" Could we just use the ToSAckedReceiver instead of creating a new receiver? https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedBridge.java (right): https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedBridge.java:5: package org.chromium.components.variations.firstrun; firstrun could be a confusing package name to other developers since "first run" has a very specific meaning on Chrome Android: i.e. the code under org.chromium.chrome.browser.firstrun / the flow that we send users through the first time they launch Chrome. This code is really "pre first run", as you mention in the comment below. Not sure what the best name is though. https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedBridge.java:17: * actual Chrome first run to Chromium core. Class provides implementation of methods to store the nit: remove "implementation of" https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedBridge.java:34: public static void setVariationsFirstRunSeed( javadoc for public methods https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java (right): https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:11: import android.util.Log; Use org.chromium.base.Log in stead of android.util.Log. org.chromium.base.Log has the same API but Debug and lower log messages are filtered out in release builds. (Incidentally, this may address my comment below about minimizing logging on production builds.) https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:38: Log.d(TAG, "Variations service started!"); Do we need to log this on production devices, or just test devices? i.e. will its helpfulness in tracking down bugs outweigh its cost to every user? The log buffer has a limited, small size and extraneous logs push important logs out of the buffer and make debugging harder. Logging should be very limited for this reason. https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:40: new DownloadTask().execute(new URL(VARIATIONS_SERVER_URL)); You don't need to use an AsyncTask here because this method is already running on a background thread (that's the point of IntentService.onHandleIntent()). This could just be downloadContent(new URL(VARIATIONS_SERVER_URL)); https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:47: public IBinder onBind(Intent intent) { don't need this. IntentService provides this implementation already. https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:74: int responseCode = connection.getResponseCode(); don't you need to check if the responseCode is 200 (HttpURLConnection.HTTP_OK)? https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:76: Log.d(TAG, "variationsTracker: Response code = " + Integer.toString(responseCode)); Use format strings instead of string concatenation in log messages. E.g. Log.d(TAG, "Response code = %d", responseCode); This allows the string concatenation to be avoided in cases where the log statement isn't going to be printed, and allows the Log statement to b effectively removed from the code in release builds. https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:86: } finally { Don't you also need to call connection.disconnect() here? https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:98: int totalCharactersCount = 0; You could just use byteBuffer.size() instead of explicitly tracking totalCharactersCount. https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:103: Log.w(TAG, "variationsTracker: seed length = " + totalCharactersCount); Again, is this logging important? https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedServiceLauncher.java (right): https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedServiceLauncher.java:25: Intent serviceIntent = new Intent(context, VariationsSeedService.class); We should check whether we've already downloaded the seed (or started the service), and if so, do nothing here. That will be especially important if we're using a connectivity change receiver or something else that gets called often.
>> General question: what happens if Chrome is offline when tos acked is broadcast? To newt: as I mentioned in TODO for VariationsSeedServiceLauncher.java file, we have some problems with receiving chrome.TOS_ACKED broadcast in Android N, that's why I plan to move to connectivity change broadcast. This is one of the reason why we do not use ToSAckedReceiver (though it was our initial approach). https://codereview.chromium.org/1417733008/diff/40001/chrome/android/java/And... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/1417733008/diff/40001/chrome/android/java/And... chrome/android/java/AndroidManifest.xml:468: <receiver android:name="org.chromium.components.variations.firstrun.VariationsSeedServiceLauncher" On 2015/11/06 18:27:14, newt wrote: > Could we just use the ToSAckedReceiver instead of creating a new receiver? It was our initial approach. See my general comment about moving to another broadcast. https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java (right): https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:103: Log.w(TAG, "variationsTracker: seed length = " + totalCharactersCount); On 2015/11/06 18:27:15, newt wrote: > Again, is this logging important? No, this logging was added for debug purposes only. I will remove it in the updated patch.
https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedBridge.java (right): https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedBridge.java:16: * before the On 2015/11/06 16:17:55, Alexei Svitkine (slow) wrote: > Nit: Wrapping is off. Done. https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedBridge.java:17: * actual Chrome first run to Chromium core. Class provides implementation of methods to store the On 2015/11/06 18:27:14, newt wrote: > nit: remove "implementation of" Done. https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedBridge.java:23: On 2015/11/06 16:17:55, Alexei Svitkine (slow) wrote: > Nit: Remove empty line. Done. https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedBridge.java:34: public static void setVariationsFirstRunSeed( On 2015/11/06 18:27:14, newt wrote: > javadoc for public methods Done. https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java (right): https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:11: import android.util.Log; On 2015/11/06 18:27:14, newt wrote: > Use org.chromium.base.Log in stead of android.util.Log. org.chromium.base.Log > has the same API but Debug and lower log messages are filtered out in release > builds. (Incidentally, this may address my comment below about minimizing > logging on production builds.) Done. https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:38: Log.d(TAG, "Variations service started!"); On 2015/11/06 18:27:14, newt wrote: > Do we need to log this on production devices, or just test devices? i.e. will > its helpfulness in tracking down bugs outweigh its cost to every user? The log > buffer has a limited, small size and extraneous logs push important logs out of > the buffer and make debugging harder. Logging should be very limited for this > reason. Done. https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:40: new DownloadTask().execute(new URL(VARIATIONS_SERVER_URL)); On 2015/11/06 18:27:15, newt wrote: > You don't need to use an AsyncTask here because this method is already running > on a background thread (that's the point of IntentService.onHandleIntent()). > This could just be > > downloadContent(new URL(VARIATIONS_SERVER_URL)); Done. https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:47: public IBinder onBind(Intent intent) { On 2015/11/06 18:27:14, newt wrote: > don't need this. IntentService provides this implementation already. Done. https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:51: private class DownloadTask extends AsyncTask<URL, Void, Boolean> { On 2015/11/06 16:17:56, Alexei Svitkine (slow) wrote: > Nit: Make this a separate top-level class with downloadContent a private method > of it. You can put the various public statics inside it. > > Call it VariationsSeedDownloadTask.java > > This way, when you add the second service, it can just re-use this code. Removed this class according to comments by newt@. https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:62: private Boolean downloadContent(URL variationsServerUrl) On 2015/11/06 16:17:56, Alexei Svitkine (slow) wrote: > Boolean -> boolean > > The uppercase version is an object instead of a primitive which doesn't seem > useful here. Done. https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:69: connection.setRequestMethod("GET"); On 2015/11/06 16:17:56, Alexei Svitkine (slow) wrote: > Nit: Per the docs, "The default method is GET." > > So no need for this line. Done. https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:76: Log.d(TAG, "variationsTracker: Response code = " + Integer.toString(responseCode)); On 2015/11/06 16:17:56, Alexei Svitkine (slow) wrote: > Nit: What's variationsTracker? > > Also, I don't think Integer.toString() is needed. Java will do this implicitly > when you use string + int. To Alexei: Integer.toString() was added according to bengr@ comments in previous issue (https://codereview.chromium.org/1394133006/), but I totally agree with you. https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:76: Log.d(TAG, "variationsTracker: Response code = " + Integer.toString(responseCode)); On 2015/11/06 18:27:14, newt wrote: > Use format strings instead of string concatenation in log messages. E.g. > > Log.d(TAG, "Response code = %d", responseCode); > > This allows the string concatenation to be avoided in cases where the log > statement isn't going to be printed, and allows the Log statement to b > effectively removed from the code in release builds. Done. https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:86: } finally { On 2015/11/06 18:27:15, newt wrote: > Don't you also need to call connection.disconnect() here? Done. https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:93: public byte[] convertInputStreamToByteArray(InputStream inputStream) On 2015/11/06 16:17:56, Alexei Svitkine (slow) wrote: > Nit: Should be private. Document params. Done. https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:98: int totalCharactersCount = 0; On 2015/11/06 18:27:14, newt wrote: > You could just use byteBuffer.size() instead of explicitly tracking > totalCharactersCount. Removed it. Done. https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedServiceLauncher.java (right): https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedServiceLauncher.java:25: Intent serviceIntent = new Intent(context, VariationsSeedService.class); On 2015/11/06 18:27:15, newt wrote: > We should check whether we've already downloaded the seed (or started the > service), and if so, do nothing here. That will be especially important if we're > using a connectivity change receiver or something else that gets called often. Acknowledged. Since we currently use chrome.TOS_ACKED broadcast, it is very unlikely to have more than one broadcasted message, that's why I do not have any additional verifications. I will definitely add it in the next CL when we move to another broadcast. https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... File components/variations/android/variations_seed_bridge.h (right): https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... components/variations/android/variations_seed_bridge.h:12: On 2015/11/06 16:17:56, Alexei Svitkine (slow) wrote: > Nit: Can remove this empty line. Same on line 23 and in the other file. Done.
https://codereview.chromium.org/1417733008/diff/60001/components/variations/a... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java (right): https://codereview.chromium.org/1417733008/diff/60001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:60: Log.d(TAG, "Response code = %d", responseCode); As mentioned earlier, please check that the response code is 200.
https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java (right): https://codereview.chromium.org/1417733008/diff/40001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:76: Log.d(TAG, "variationsTracker: Response code = " + Integer.toString(responseCode)); On 2015/11/06 19:54:37, Alexander Agulenko wrote: > On 2015/11/06 16:17:56, Alexei Svitkine (slow) wrote: > > Nit: What's variationsTracker? > > > > Also, I don't think Integer.toString() is needed. Java will do this implicitly > > when you use string + int. > > To Alexei: Integer.toString() was added according to bengr@ comments in previous > issue (https://codereview.chromium.org/1394133006/), but I totally agree with > you. Yeah. In Java, it's perfectly acceptable to count on the language to do the string conversion for you. https://codereview.chromium.org/1417733008/diff/60001/components/variations/a... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java (right): https://codereview.chromium.org/1417733008/diff/60001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:41: } catch (Exception e) { Do NOT catch Exception. This will silence all sorts of serious programming errors, such NullPointerExceptions, ArrayIndexOutOfBoundsExceptions, etc. What Exceptions are you expecting here, anyway?
https://codereview.chromium.org/1417733008/diff/60001/components/variations/a... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java (right): https://codereview.chromium.org/1417733008/diff/60001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:41: } catch (Exception e) { On 2015/11/06 20:57:23, newt wrote: > Do NOT catch Exception. This will silence all sorts of serious programming > errors, such NullPointerExceptions, ArrayIndexOutOfBoundsExceptions, etc. > > What Exceptions are you expecting here, anyway? Done. https://codereview.chromium.org/1417733008/diff/60001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:60: Log.d(TAG, "Response code = %d", responseCode); On 2015/11/06 20:11:12, Alexei Svitkine (slow) wrote: > As mentioned earlier, please check that the response code is 200. Done.
LGTM % comment. Please also wait for newt@ to review. https://codereview.chromium.org/1417733008/diff/80001/components/variations/a... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java (right): https://codereview.chromium.org/1417733008/diff/80001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:71: return false; Nit: Make this an early return instead. if (responseCode != HttpURLConnection.HTTP_OK) { return false; } // rest of code
https://codereview.chromium.org/1417733008/diff/80001/components/variations/a... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java (right): https://codereview.chromium.org/1417733008/diff/80001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:71: return false; On 2015/11/09 16:02:33, Alexei Svitkine (slow) wrote: > Nit: Make this an early return instead. > > if (responseCode != HttpURLConnection.HTTP_OK) { > return false; > } > // rest of code Done.
The CQ bit was checked by agulenko@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1417733008/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1417733008/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: All required reviewers (with asterisk prefixes) have not yet approved this CL. No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
https://codereview.chromium.org/1417733008/diff/100001/components/variations/... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java (right): https://codereview.chromium.org/1417733008/diff/100001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:41: } catch (IOException e) { Why are you catching IOException? Please catch exceptions as close as possible to the code that could throw them, and isolate the exceptions from other code; that'll ensure that readers of your code will understand why you are catching various exceptions. https://codereview.chromium.org/1417733008/diff/100001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:59: Log.d(TAG, "Response code = %d", responseCode); remove log statement, though you might want a log statement in the unexpected case where the server returns something other than 200.
https://codereview.chromium.org/1417733008/diff/100001/components/variations/... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java (right): https://codereview.chromium.org/1417733008/diff/100001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:41: } catch (IOException e) { On 2015/11/09 22:47:12, newt wrote: > Why are you catching IOException? Please catch exceptions as close as possible > to the code that could throw them, and isolate the exceptions from other code; > that'll ensure that readers of your code will understand why you are catching > various exceptions. Otherwise it gives me compile error because I do not catch IOException from downloadContent() and do not throw this exception futher.
https://codereview.chromium.org/1417733008/diff/100001/components/variations/... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java (right): https://codereview.chromium.org/1417733008/diff/100001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:41: } catch (IOException e) { On 2015/11/09 22:48:43, Alexander Agulenko wrote: > On 2015/11/09 22:47:12, newt wrote: > > Why are you catching IOException? Please catch exceptions as close as possible > > to the code that could throw them, and isolate the exceptions from other code; > > that'll ensure that readers of your code will understand why you are catching > > various exceptions. > > Otherwise it gives me compile error because I do not catch IOException from > downloadContent() and do not throw this exception futher. To reiterate: Please catch exceptions as close as possible to the code that could throw them, and isolate the exceptions from other code; that'll ensure that readers of your code will understand why you are catching various exceptions. Yes, I understand that IOException is thrown somewhere inside downloadContent(). So, you should catch IOException in downloadContent()... close to where the exception is thrown.
https://codereview.chromium.org/1417733008/diff/100001/components/variations/... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java (right): https://codereview.chromium.org/1417733008/diff/100001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:41: } catch (IOException e) { On 2015/11/09 22:51:20, newt wrote: > On 2015/11/09 22:48:43, Alexander Agulenko wrote: > > On 2015/11/09 22:47:12, newt wrote: > > > Why are you catching IOException? Please catch exceptions as close as > possible > > > to the code that could throw them, and isolate the exceptions from other > code; > > > that'll ensure that readers of your code will understand why you are > catching > > > various exceptions. > > > > Otherwise it gives me compile error because I do not catch IOException from > > downloadContent() and do not throw this exception futher. > > To reiterate: Please catch exceptions as close as possible to the code that > could throw them, and isolate the exceptions from other code; that'll ensure > that readers of your code will understand why you are catching various > exceptions. > > Yes, I understand that IOException is thrown somewhere inside downloadContent(). > So, you should catch IOException in downloadContent()... close to where the > exception is thrown. Done. https://codereview.chromium.org/1417733008/diff/100001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:59: Log.d(TAG, "Response code = %d", responseCode); On 2015/11/09 22:47:12, newt wrote: > remove log statement, though you might want a log statement in the unexpected > case where the server returns something other than 200. Done.
https://codereview.chromium.org/1417733008/diff/120001/components/variations/... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java (right): https://codereview.chromium.org/1417733008/diff/120001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:53: throws IOException, MalformedURLException { Remove the "throws" clause; this shouldn't throw IOException or MalformedURLException. If a MalformedURLException is thrown inside the method (I don't see where that would happen; but perhaps it does), then you should catch it inside this method. https://codereview.chromium.org/1417733008/diff/120001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:78: } finally { just add the catch statement here (and delete the new downloadContent() method that you added in this patch set)
https://codereview.chromium.org/1417733008/diff/120001/components/variations/... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java (right): https://codereview.chromium.org/1417733008/diff/120001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:53: throws IOException, MalformedURLException { On 2015/11/10 06:20:15, newt wrote: > Remove the "throws" clause; this shouldn't throw IOException or > MalformedURLException. If a MalformedURLException is thrown inside the method (I > don't see where that would happen; but perhaps it does), then you should catch > it inside this method. See my comment below. https://codereview.chromium.org/1417733008/diff/120001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:78: } finally { On 2015/11/10 06:20:15, newt wrote: > just add the catch statement here (and delete the new downloadContent() method > that you added in this patch set) I tried to make exactly as you suggest, but inputStream.close() also can throw IOException -- that is the reason why I added additional layer with try/catch blocks.
https://codereview.chromium.org/1417733008/diff/120001/components/variations/... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java (right): https://codereview.chromium.org/1417733008/diff/120001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:78: } finally { On 2015/11/10 06:40:12, Alexander Agulenko wrote: > On 2015/11/10 06:20:15, newt wrote: > > just add the catch statement here (and delete the new downloadContent() method > > that you added in this patch set) > > I tried to make exactly as you suggest, but inputStream.close() also can throw > IOException -- that is the reason why I added additional layer with try/catch > blocks. Of course, I can resolve it by making nested try/catch blocks, but it does not seem to be the best idea.
still lgtm https://codereview.chromium.org/1417733008/diff/120001/components/variations/... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java (right): https://codereview.chromium.org/1417733008/diff/120001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:66: Log.d(TAG, "Response code = %d", responseCode); Nit: Move this above outside the if, or changing this message to "Non-200 response code = " or something.
By the way, we're hoping to land this very soon as Alex has a couple follow up CLs we'd like to land before the M48 branch. So would appreciate a quick review/iteration time on this from reviewers. Thanks!
https://codereview.chromium.org/1417733008/diff/120001/components/variations/... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java (right): https://codereview.chromium.org/1417733008/diff/120001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:78: } finally { On 2015/11/10 06:40:12, Alexander Agulenko wrote: > On 2015/11/10 06:20:15, newt wrote: > > just add the catch statement here (and delete the new downloadContent() method > > that you added in this patch set) > > I tried to make exactly as you suggest, but inputStream.close() also can throw > IOException -- that is the reason why I added additional layer with try/catch > blocks. Actually, you should wrap inputStream.close() in a nested try/catch block, so that if inputStream.close() fails, we'll still try to call connection.disconnect().
https://codereview.chromium.org/1417733008/diff/120001/components/variations/... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java (right): https://codereview.chromium.org/1417733008/diff/120001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:78: } finally { On 2015/11/10 17:17:51, newt wrote: > On 2015/11/10 06:40:12, Alexander Agulenko wrote: > > On 2015/11/10 06:20:15, newt wrote: > > > just add the catch statement here (and delete the new downloadContent() > method > > > that you added in this patch set) > > > > I tried to make exactly as you suggest, but inputStream.close() also can throw > > IOException -- that is the reason why I added additional layer with try/catch > > blocks. > > Actually, you should wrap inputStream.close() in a nested try/catch block, so > that if inputStream.close() fails, we'll still try to call > connection.disconnect(). For tidiness, split into two funcions: private byte[] getRawSeed(HttpURLConnection connection) { InputStream inputStream = null; try { inputStream = connection.getInputStream(); return convertInputStreamToByteArray(inputStream); } finally { if (inputStream != null) { inputStream.close(); } } }
https://codereview.chromium.org/1417733008/diff/120001/components/variations/... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java (right): https://codereview.chromium.org/1417733008/diff/120001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:66: Log.d(TAG, "Response code = %d", responseCode); On 2015/11/10 16:23:24, Alexei Svitkine (slow) wrote: > Nit: Move this above outside the if, or changing this message to "Non-200 > response code = " or something. Done. https://codereview.chromium.org/1417733008/diff/120001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:78: } finally { On 2015/11/10 20:51:10, Steven Holte wrote: > On 2015/11/10 17:17:51, newt wrote: > > On 2015/11/10 06:40:12, Alexander Agulenko wrote: > > > On 2015/11/10 06:20:15, newt wrote: > > > > just add the catch statement here (and delete the new downloadContent() > > method > > > > that you added in this patch set) > > > > > > I tried to make exactly as you suggest, but inputStream.close() also can > throw > > > IOException -- that is the reason why I added additional layer with > try/catch > > > blocks. > > > > Actually, you should wrap inputStream.close() in a nested try/catch block, so > > that if inputStream.close() fails, we'll still try to call > > connection.disconnect(). > > For tidiness, split into two funcions: > > private byte[] getRawSeed(HttpURLConnection connection) { > InputStream inputStream = null; > try { > inputStream = connection.getInputStream(); > return convertInputStreamToByteArray(inputStream); > } finally { > if (inputStream != null) { > inputStream.close(); > } > } > } Done. https://codereview.chromium.org/1417733008/diff/120001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:78: } finally { On 2015/11/10 17:17:51, newt wrote: > On 2015/11/10 06:40:12, Alexander Agulenko wrote: > > On 2015/11/10 06:20:15, newt wrote: > > > just add the catch statement here (and delete the new downloadContent() > method > > > that you added in this patch set) > > > > I tried to make exactly as you suggest, but inputStream.close() also can throw > > IOException -- that is the reason why I added additional layer with try/catch > > blocks. > > Actually, you should wrap inputStream.close() in a nested try/catch block, so > that if inputStream.close() fails, we'll still try to call > connection.disconnect(). Check the comment below by Steven, it seems to be less messy than nested try/catches.
lgtm https://codereview.chromium.org/1417733008/diff/140001/components/variations/... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java (right): https://codereview.chromium.org/1417733008/diff/140001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:67: Log.w(TAG, "Variations service has thrown IO Exception.", e); Nit: "Variations service" -> "VariationsSeedService" or just simplify: Log.w(TAG, "IOException fetching seed:", e);
https://codereview.chromium.org/1417733008/diff/140001/components/variations/... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java (right): https://codereview.chromium.org/1417733008/diff/140001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:67: Log.w(TAG, "Variations service has thrown IO Exception.", e); On 2015/11/10 22:49:45, Alexei Svitkine (slow) wrote: > Nit: "Variations service" -> "VariationsSeedService" > > or just simplify: > > Log.w(TAG, "IOException fetching seed:", e); We do not need to write "VariationsSeedService", because this information will be logged as a TAG. Done.
The CQ bit was checked by asvitkine@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/1417733008/#ps160001 (title: "Minor change in logging message")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1417733008/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1417733008/160001
The CQ bit was unchecked by commit-bot@chromium.org
All required reviewers (with asterisk prefixes) have not yet approved this CL. No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
lgtm after nit https://codereview.chromium.org/1417733008/diff/160001/components/variations/... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java (right): https://codereview.chromium.org/1417733008/diff/160001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:43: private boolean downloadContent(URL variationsServerUrl) throws MalformedURLException { nit: Does this really throw MalformedURLException? If not, remove this throws clause.
https://codereview.chromium.org/1417733008/diff/160001/components/variations/... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java (right): https://codereview.chromium.org/1417733008/diff/160001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:43: private boolean downloadContent(URL variationsServerUrl) throws MalformedURLException { On 2015/11/10 23:26:21, newt wrote: > nit: Does this really throw MalformedURLException? If not, remove this throws > clause. Done.
The CQ bit was checked by agulenko@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, newt@chromium.org Link to the patchset: https://codereview.chromium.org/1417733008/#ps180001 (title: "Removed unnecessary exception throw declaration")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1417733008/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1417733008/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/2d59816a69420e25c1ff39fb55d2d6f245c424b3 Cr-Commit-Position: refs/heads/master@{#359036} |
