Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1437833004:
Implemented clearing Java prefs after pulling variations first run seed from Java to C++ side (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. |
DescriptionImplemented clearing Java prefs after pulling variations first run seed from Java to C++ side
BUG=551029
Committed: https://crrev.com/683e65ca552aad24fd7794ca362a198103519d7f
Cr-Commit-Position: refs/heads/master@{#359649}
Patch Set 1 #
Total comments: 11
Patch Set 2 : Minor fixes according to codereview comments. #
Total comments: 4
Patch Set 3 : Merged with CL 1437833002 + fixed some code review comments #
Total comments: 14
Patch Set 4 : Minor code review fixes #
Total comments: 4
Patch Set 5 : Minor code review changes #Patch Set 6 : Minor fix #Patch Set 7 : Clang build error avoided #
Total comments: 4
Patch Set 8 : Fixed unit tests failure #
Messages
Total messages: 41 (13 generated)
Description was changed from ========== Implemented clearing Java prefs after pulling variations first run seed from Java to C++ side BUG=551029 ========== to ========== Implemented clearing Java prefs after pulling variations first run seed from Java to C++ side BUG=551029 ==========
agulenko@google.com changed reviewers: + asvitkine@chromium.org, holte@chromium.org
https://codereview.chromium.org/1437833004/diff/1/components/variations/andro... File components/variations/android/variations_seed_bridge.cc (right): https://codereview.chromium.org/1437833004/diff/1/components/variations/andro... components/variations/android/variations_seed_bridge.cc:58: void ClearJavaPrefs() { Nit: ClearJavaFirstRunPrefs(). https://codereview.chromium.org/1437833004/diff/1/components/variations/andro... components/variations/android/variations_seed_bridge.cc:60: Java_VariationsSeedBridge_clearVariationsFirstRunSeedPrefs( In this case, since it's on VariationsSeedBridge, you probably can make the method name shorter. e.g. clearFirstRunPrefs() https://codereview.chromium.org/1437833004/diff/1/components/variations/andro... File components/variations/android/variations_seed_bridge.h (right): https://codereview.chromium.org/1437833004/diff/1/components/variations/andro... components/variations/android/variations_seed_bridge.h:21: void ClearJavaPrefs(); Add a comment. https://codereview.chromium.org/1437833004/diff/1/components/variations/varia... File components/variations/variations_seed_store.cc (right): https://codereview.chromium.org/1437833004/diff/1/components/variations/varia... components/variations/variations_seed_store.cc:372: &seed_country); Per offline discussion, I suggested the below would be done when storing the seed if the current seed is empty. https://codereview.chromium.org/1437833004/diff/1/components/variations/varia... components/variations/variations_seed_store.cc:373: android::ClearJavaPrefs(); This needs an ifdef android. Also probably the include.
https://codereview.chromium.org/1437833004/diff/1/components/variations/varia... File components/variations/variations_seed_store.cc (right): https://codereview.chromium.org/1437833004/diff/1/components/variations/varia... components/variations/variations_seed_store.cc:373: android::ClearJavaPrefs(); On 2015/11/12 16:30:11, Alexei Svitkine (slow) wrote: > This needs an ifdef android. Also probably the include. I am not sure that it is really necessary: the whole function is wrapped by ifdef android, so there is no sense in wrapping a piece of code inside it.
https://codereview.chromium.org/1437833004/diff/1/components/variations/varia... File components/variations/variations_seed_store.cc (right): https://codereview.chromium.org/1437833004/diff/1/components/variations/varia... components/variations/variations_seed_store.cc:373: android::ClearJavaPrefs(); On 2015/11/12 17:25:25, Alexander Agulenko wrote: > On 2015/11/12 16:30:11, Alexei Svitkine (slow) wrote: > > This needs an ifdef android. Also probably the include. > > I am not sure that it is really necessary: the whole function is wrapped by > ifdef android, so there is no sense in wrapping a piece of code inside it. Sorry, you're right. However, see my other comment about moving this to a different location, where it likely will need to be ifdefd.
https://codereview.chromium.org/1437833004/diff/1/components/variations/andro... File components/variations/android/variations_seed_bridge.cc (right): https://codereview.chromium.org/1437833004/diff/1/components/variations/andro... components/variations/android/variations_seed_bridge.cc:58: void ClearJavaPrefs() { On 2015/11/12 16:30:11, Alexei Svitkine (slow) wrote: > Nit: ClearJavaFirstRunPrefs(). Done. https://codereview.chromium.org/1437833004/diff/1/components/variations/andro... components/variations/android/variations_seed_bridge.cc:60: Java_VariationsSeedBridge_clearVariationsFirstRunSeedPrefs( On 2015/11/12 16:30:11, Alexei Svitkine (slow) wrote: > In this case, since it's on VariationsSeedBridge, you probably can make the > method name shorter. > > e.g. clearFirstRunPrefs() Done. https://codereview.chromium.org/1437833004/diff/1/components/variations/andro... File components/variations/android/variations_seed_bridge.h (right): https://codereview.chromium.org/1437833004/diff/1/components/variations/andro... components/variations/android/variations_seed_bridge.h:21: void ClearJavaPrefs(); On 2015/11/12 16:30:11, Alexei Svitkine (slow) wrote: > Add a comment. Done. https://codereview.chromium.org/1437833004/diff/1/components/variations/varia... File components/variations/variations_seed_store.cc (right): https://codereview.chromium.org/1437833004/diff/1/components/variations/varia... components/variations/variations_seed_store.cc:372: &seed_country); On 2015/11/12 16:30:11, Alexei Svitkine (slow) wrote: > Per offline discussion, I suggested the below would be done when storing the > seed if the current seed is empty. Done.
https://codereview.chromium.org/1437833004/diff/20001/components/variations/v... File components/variations/variations_seed_store.cc (right): https://codereview.chromium.org/1437833004/diff/20001/components/variations/v... components/variations/variations_seed_store.cc:468: // If currently we do not have any stored pref then we clear prefs on the Add an ifdef around this block https://codereview.chromium.org/1437833004/diff/20001/components/variations/v... components/variations/variations_seed_store.cc:470: if (local_state_->GetString(prefs::kVariationsCompressedSeed).empty()) { Nit: No {}'s
Let's combine this with: https://codereview.chromium.org/1437833002/ Given the clear prefs markVariationsSeedAsStored() logic should happen at the same time, it should just be one call that does it.
https://codereview.chromium.org/1437833004/diff/20001/components/variations/v... File components/variations/variations_seed_store.cc (right): https://codereview.chromium.org/1437833004/diff/20001/components/variations/v... components/variations/variations_seed_store.cc:468: // If currently we do not have any stored pref then we clear prefs on the On 2015/11/12 17:42:17, Alexei Svitkine (slow) wrote: > Add an ifdef around this block Done. https://codereview.chromium.org/1437833004/diff/20001/components/variations/v... components/variations/variations_seed_store.cc:470: if (local_state_->GetString(prefs::kVariationsCompressedSeed).empty()) { On 2015/11/12 17:42:17, Alexei Svitkine (slow) wrote: > Nit: No {}'s Added another function call to this block, so {}'s are needed now.
https://codereview.chromium.org/1437833004/diff/40001/components/variations/a... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedBridge.java (right): https://codereview.chromium.org/1437833004/diff/40001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedBridge.java:24: private static final String VARIATIONS_FIRST_RUN_SEED_NATIVE_STORED = Add a comment above this. https://codereview.chromium.org/1437833004/diff/40001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedBridge.java:48: SharedPreferences prefs = PreferenceManager.getDefaultSharedPreferences(context); Nit: Inline this into the line below, given you only use |prefs| once. https://codereview.chromium.org/1437833004/diff/40001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedBridge.java:61: return !prefs.getString(VARIATIONS_FIRST_RUN_SEED_BASE64, "").equals(""); equals("") -> isEmpty() https://codereview.chromium.org/1437833004/diff/40001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedBridge.java:65: * Returns the status of the variations first run fetch: was it successful or not. Fix comments. Right now the two functions have the same comment. https://codereview.chromium.org/1437833004/diff/40001/components/variations/a... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java (right): https://codereview.chromium.org/1437833004/diff/40001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:29: private static boolean sFetchInProgress = false; Add a comment. https://codereview.chromium.org/1437833004/diff/40001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:37: if (sFetchInProgress || VariationsSeedBridge.hasJavaPref(getApplicationContext()) Add a comment. https://codereview.chromium.org/1437833004/diff/40001/components/variations/a... File components/variations/android/variations_seed_bridge.cc (right): https://codereview.chromium.org/1437833004/diff/40001/components/variations/a... components/variations/android/variations_seed_bridge.cc:63: void markVariationsSeedAsStored() { Cap the first letter.
https://codereview.chromium.org/1437833004/diff/40001/components/variations/a... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedBridge.java (right): https://codereview.chromium.org/1437833004/diff/40001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedBridge.java:24: private static final String VARIATIONS_FIRST_RUN_SEED_NATIVE_STORED = On 2015/11/12 19:03:22, Alexei Svitkine (slow) wrote: > Add a comment above this. Done. https://codereview.chromium.org/1437833004/diff/40001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedBridge.java:48: SharedPreferences prefs = PreferenceManager.getDefaultSharedPreferences(context); On 2015/11/12 19:03:22, Alexei Svitkine (slow) wrote: > Nit: Inline this into the line below, given you only use |prefs| once. Done. https://codereview.chromium.org/1437833004/diff/40001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedBridge.java:61: return !prefs.getString(VARIATIONS_FIRST_RUN_SEED_BASE64, "").equals(""); On 2015/11/12 19:03:22, Alexei Svitkine (slow) wrote: > equals("") -> isEmpty() Done. https://codereview.chromium.org/1437833004/diff/40001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedBridge.java:65: * Returns the status of the variations first run fetch: was it successful or not. On 2015/11/12 19:03:22, Alexei Svitkine (slow) wrote: > Fix comments. Right now the two functions have the same comment. Done. https://codereview.chromium.org/1437833004/diff/40001/components/variations/a... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java (right): https://codereview.chromium.org/1437833004/diff/40001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:29: private static boolean sFetchInProgress = false; On 2015/11/12 19:03:22, Alexei Svitkine (slow) wrote: > Add a comment. Done. https://codereview.chromium.org/1437833004/diff/40001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:37: if (sFetchInProgress || VariationsSeedBridge.hasJavaPref(getApplicationContext()) On 2015/11/12 19:03:22, Alexei Svitkine (slow) wrote: > Add a comment. Done. https://codereview.chromium.org/1437833004/diff/40001/components/variations/a... File components/variations/android/variations_seed_bridge.cc (right): https://codereview.chromium.org/1437833004/diff/40001/components/variations/a... components/variations/android/variations_seed_bridge.cc:63: void markVariationsSeedAsStored() { On 2015/11/12 19:03:22, Alexei Svitkine (slow) wrote: > Cap the first letter. Done.
lgtm % comments https://codereview.chromium.org/1437833004/diff/60001/components/variations/a... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedBridge.java (right): https://codereview.chromium.org/1437833004/diff/60001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedBridge.java:24: // This pref is used to store information about successful seed storing on the C++ side not to Nit: "on the C++ side, in order to not fetch the seed again." https://codereview.chromium.org/1437833004/diff/60001/components/variations/v... File components/variations/variations_seed_store.cc (right): https://codereview.chromium.org/1437833004/diff/60001/components/variations/v... components/variations/variations_seed_store.cc:15: #include "components/variations/android/variations_seed_bridge.h" Put the header in an ifdef below since it won't build on other platforms otherwise.
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/1437833004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1437833004/60001
lgtm % alexei's comments.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
https://codereview.chromium.org/1437833004/diff/60001/components/variations/a... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedBridge.java (right): https://codereview.chromium.org/1437833004/diff/60001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedBridge.java:24: // This pref is used to store information about successful seed storing on the C++ side not to On 2015/11/12 22:27:31, Alexei Svitkine (slow) wrote: > Nit: "on the C++ side, in order to not fetch the seed again." Done. https://codereview.chromium.org/1437833004/diff/60001/components/variations/v... File components/variations/variations_seed_store.cc (right): https://codereview.chromium.org/1437833004/diff/60001/components/variations/v... components/variations/variations_seed_store.cc:15: #include "components/variations/android/variations_seed_bridge.h" On 2015/11/12 22:27:31, Alexei Svitkine (slow) wrote: > Put the header in an ifdef below since it won't build on other platforms > otherwise. 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/1437833004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1437833004/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
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/1437833004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1437833004/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
still lgtm % comments https://codereview.chromium.org/1437833004/diff/120001/components/variations/... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java (right): https://codereview.chromium.org/1437833004/diff/120001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:46: ssetFetchInProgressFlagValue(true); Nit: typo (sset -> set) https://codereview.chromium.org/1437833004/diff/120001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:56: // Separate function is needed to avoid clang build error (assigning value to static variable I don't think it's a clang error. Clang doesn't compile Java code. It's a FINDBUGS error.
https://codereview.chromium.org/1437833004/diff/120001/components/variations/... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java (right): https://codereview.chromium.org/1437833004/diff/120001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:46: ssetFetchInProgressFlagValue(true); On 2015/11/13 16:04:05, Alexei Svitkine (slow) wrote: > Nit: typo (sset -> set) Done. https://codereview.chromium.org/1437833004/diff/120001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:56: // Separate function is needed to avoid clang build error (assigning value to static variable On 2015/11/13 16:04:05, Alexei Svitkine (slow) wrote: > I don't think it's a clang error. Clang doesn't compile Java code. > > It's a FINDBUGS error. Done.
The CQ bit was checked by agulenko@google.com to run a CQ dry run
lgtm
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1437833004/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1437833004/140001
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 asvitkine@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from holte@chromium.org Link to the patchset: https://codereview.chromium.org/1437833004/#ps140001 (title: "Fixed unit tests failure")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1437833004/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1437833004/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/683e65ca552aad24fd7794ca362a198103519d7f Cr-Commit-Position: refs/heads/master@{#359649}
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:140001) has been created in https://codereview.chromium.org/1447823003/ by loyso@chromium.org. The reason for reverting is: Crashes android gn build (component unittests) .
Message was sent while issue was closed.
Description was changed from ========== Implemented clearing Java prefs after pulling variations first run seed from Java to C++ side BUG=551029 Committed: https://crrev.com/683e65ca552aad24fd7794ca362a198103519d7f Cr-Commit-Position: refs/heads/master@{#359649} ========== to ========== Implemented clearing Java prefs after pulling variations first run seed from Java to C++ side BUG=551029 Committed: https://crrev.com/683e65ca552aad24fd7794ca362a198103519d7f Cr-Commit-Position: refs/heads/master@{#359649} ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
