Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1417503010:
Variations seed is pulled from the Java application on the first launch of Chrome (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. |
DescriptionVariations seed is pulled from the Java application on the first launch of Chrome
BUG=551029
Committed: https://crrev.com/25d2111a2857676f524fc4f686ddf14ed08d3fd9
Cr-Commit-Position: refs/heads/master@{#358006}
Patch Set 1 #Patch Set 2 : Minor fix: removed debug output #
Total comments: 27
Patch Set 3 : Fixes according to code review and lint comments #
Total comments: 34
Patch Set 4 : Minor fixes according to code review comments #
Total comments: 14
Patch Set 5 : Fixes according to latest code review. #
Total comments: 7
Patch Set 6 : Minor fixes #Patch Set 7 : Minor fixes #
Total comments: 13
Patch Set 8 : Minor fixes according to latest codereview #Patch Set 9 : Implemented decoding Base64 encoded seed on Java side and then passing it to Chromium core as byte[… #Patch Set 10 : Moved GetVariationsFirstRunSeedCallback() to a separate .cc file due to clang build errors #
Total comments: 2
Patch Set 11 : Added gyp/gn rules and fixed minor comments according to code review #Patch Set 12 : Newline added due to clang build error #Messages
Total messages: 66 (21 generated)
Description was changed from ========== Variations seed is pulled from the Java application on the first launch of Chrome BUG= ========== to ========== Variations seed is pulled from the Java application on the first launch of Chrome BUG= ==========
agulenko@google.com changed reviewers: + asvitkine@chromium.org, holte@chromium.org, newt@chromium.org
https://codereview.chromium.org/1417503010/diff/20001/chrome/browser/android/... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/1417503010/diff/20001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:979: /* std::string seed; Looks like unfinished code? https://codereview.chromium.org/1417503010/diff/20001/components/variations/v... File components/variations/variations_seed_store.cc (right): https://codereview.chromium.org/1417503010/diff/20001/components/variations/v... components/variations/variations_seed_store.cc:352: LOG(WARNING) << "variationsTracker: Trying to read seed from Java side"; Move this block of code into a function, e.g. MigrateJavaSeed. https://codereview.chromium.org/1417503010/diff/20001/components/variations/v... components/variations/variations_seed_store.cc:369: // Trying to translated the seed that we pulled from the Java side. translate -> migrate, but moving to a function should make this comment unnecessary. https://codereview.chromium.org/1417503010/diff/20001/components/variations/v... components/variations/variations_seed_store.cc:370: // ToDo(agulenko): replace "true" with real value Nit: All caps TODO https://codereview.chromium.org/1417503010/diff/20001/components/variations/v... components/variations/variations_seed_store.cc:370: // ToDo(agulenko): replace "true" with real value Probably TODO: support gzipped seed? https://codereview.chromium.org/1417503010/diff/20001/components/variations/v... components/variations/variations_seed_store.cc:373: LOG(WARNING) << "Seed translation failed!"; Change to "First run variations seed is invalid" https://codereview.chromium.org/1417503010/diff/20001/components/variations/v... components/variations/variations_seed_store.cc:375: } Add a TODO to clear the Java prefs (regardless if StoreSeedData fails) https://codereview.chromium.org/1417503010/diff/20001/components/variations/v... components/variations/variations_seed_store.cc:377: base64_seed_data = local_state_->GetString(prefs::kVariationsCompressedSeed); To avoid this duplication it's probably better to do the migration before the first attempt to load the seed, e.g. change bool VariationsSeedStore::LoadSeed(variations::VariationsSeed* seed) { invalid_base64_signature_.clear(); std::string seed_data; if (!ReadSeedData(&seed_data)) return false; to bool VariationsSeedStore::LoadSeed(variations::VariationsSeed* seed) { invalid_base64_signature_.clear(); if (!local_state_->HasPrefPath(prefs::kVariationsSeedSignature)) MigrateJavaSeed(); std::string seed_data; if (!ReadSeedData(&seed_data)) return false;
https://codereview.chromium.org/1417503010/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/1417503010/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:223: // prefs::kVariationsSeedSignature and prefs::kVariationsCountry respectively Can we avoid the C++ code needing to know these internal pref names? I have a suggestion for how to do that in a later comment. https://codereview.chromium.org/1417503010/diff/20001/chrome/browser/android/... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/1417503010/diff/20001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:956: std::string PrefServiceBridge::PullVariationsSeedPref( Instead of being able to pass arbitrary pref names, I suggest making this function have out-params for the specific fields it needs. Specifically, it will look like: bool PrefServiceBridge::GetFirstRunVariationSeedValues( std::string* seed_content, std::string* seed_signature, std::string* seed_country) { ... } Then, the calling code from VariationsSeedStore just needs to do a single call to get all the values and doesn't need to know the internal pref names used on the Java side to get this info. https://codereview.chromium.org/1417503010/diff/20001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:958: if (prefName == variations::prefs::kVariationsSeed) { Nit: No {}'s https://codereview.chromium.org/1417503010/diff/20001/components/variations/v... File components/variations/variations_seed_store.cc (right): https://codereview.chromium.org/1417503010/diff/20001/components/variations/v... components/variations/variations_seed_store.cc:352: LOG(WARNING) << "variationsTracker: Trying to read seed from Java side"; On 2015/10/28 01:00:18, Steven Holte wrote: > Move this block of code into a function, e.g. MigrateJavaSeed. Agree with Steve. How about naming it ImportJavaSeed()? I think that's a little bit clearer than Migrate, which can mean different things. https://codereview.chromium.org/1417503010/diff/20001/components/variations/v... components/variations/variations_seed_store.cc:365: // ToDo (agulenko): pull actual time from the response. Nit: Usual format for these is: TODO(agulenko): Pull actual time from the response. (Also, I'm ok with this being a TODO for this CL and can be implemented in a follow-up CL). https://codereview.chromium.org/1417503010/diff/20001/components/variations/v... components/variations/variations_seed_store.cc:367: scoped_ptr<variations::VariationsSeed> seed(new variations::VariationsSeed); It doesn't seem like you're using |seed| other than the param below. Since the param can be nullptr, just pass that in instead.
By the way, I didn't see you mail this CL before Steve started commenting on it. When you upload a new CL/patchset to a CL, you should use the "Publish+Mail Comments" function to send the email to reviewers to take a look at this again.
agulenko@google.com changed required reviewers: + newt@chromium.org
Please review the updated code. https://codereview.chromium.org/1417503010/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/1417503010/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:223: // prefs::kVariationsSeedSignature and prefs::kVariationsCountry respectively On 2015/10/28 15:16:38, Alexei Svitkine (slow) wrote: > Can we avoid the C++ code needing to know these internal pref names? I have a > suggestion for how to do that in a later comment. Done. https://codereview.chromium.org/1417503010/diff/20001/chrome/browser/android/... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/1417503010/diff/20001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:956: std::string PrefServiceBridge::PullVariationsSeedPref( On 2015/10/28 15:16:38, Alexei Svitkine (slow) wrote: > Instead of being able to pass arbitrary pref names, I suggest making this > function have out-params for the specific fields it needs. > > Specifically, it will look like: > > bool PrefServiceBridge::GetFirstRunVariationSeedValues( > std::string* seed_content, > std::string* seed_signature, > std::string* seed_country) { > ... > } > > Then, the calling code from VariationsSeedStore just needs to do a single call > to get all the values and doesn't need to know the internal pref names used on > the Java side to get this info. Done. https://codereview.chromium.org/1417503010/diff/20001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:958: if (prefName == variations::prefs::kVariationsSeed) { On 2015/10/28 15:16:38, Alexei Svitkine (slow) wrote: > Nit: No {}'s Done. https://codereview.chromium.org/1417503010/diff/20001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:979: /* std::string seed; On 2015/10/28 01:00:18, Steven Holte wrote: > Looks like unfinished code? Yes, this piece of code has been changed in Patch 2. https://codereview.chromium.org/1417503010/diff/20001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:979: /* std::string seed; On 2015/10/28 01:00:18, Steven Holte wrote: > Looks like unfinished code? Done. https://codereview.chromium.org/1417503010/diff/20001/components/variations/v... File components/variations/variations_seed_store.cc (right): https://codereview.chromium.org/1417503010/diff/20001/components/variations/v... components/variations/variations_seed_store.cc:352: LOG(WARNING) << "variationsTracker: Trying to read seed from Java side"; On 2015/10/28 15:16:38, Alexei Svitkine (slow) wrote: > On 2015/10/28 01:00:18, Steven Holte wrote: > > Move this block of code into a function, e.g. MigrateJavaSeed. > > Agree with Steve. How about naming it ImportJavaSeed()? I think that's a little > bit clearer than Migrate, which can mean different things. Done. https://codereview.chromium.org/1417503010/diff/20001/components/variations/v... components/variations/variations_seed_store.cc:365: // ToDo (agulenko): pull actual time from the response. On 2015/10/28 15:16:38, Alexei Svitkine (slow) wrote: > Nit: Usual format for these is: > > TODO(agulenko): Pull actual time from the response. > > (Also, I'm ok with this being a TODO for this CL and can be implemented in a > follow-up CL). Done. https://codereview.chromium.org/1417503010/diff/20001/components/variations/v... components/variations/variations_seed_store.cc:367: scoped_ptr<variations::VariationsSeed> seed(new variations::VariationsSeed); On 2015/10/28 15:16:38, Alexei Svitkine (slow) wrote: > It doesn't seem like you're using |seed| other than the param below. Since the > param can be nullptr, just pass that in instead. Done. https://codereview.chromium.org/1417503010/diff/20001/components/variations/v... components/variations/variations_seed_store.cc:369: // Trying to translated the seed that we pulled from the Java side. On 2015/10/28 01:00:18, Steven Holte wrote: > translate -> migrate, but moving to a function should make this comment > unnecessary. Done. https://codereview.chromium.org/1417503010/diff/20001/components/variations/v... components/variations/variations_seed_store.cc:370: // ToDo(agulenko): replace "true" with real value On 2015/10/28 01:00:18, Steven Holte wrote: > Probably TODO: support gzipped seed? Done. https://codereview.chromium.org/1417503010/diff/20001/components/variations/v... components/variations/variations_seed_store.cc:373: LOG(WARNING) << "Seed translation failed!"; On 2015/10/28 01:00:18, Steven Holte wrote: > Change to "First run variations seed is invalid" Done. https://codereview.chromium.org/1417503010/diff/20001/components/variations/v... components/variations/variations_seed_store.cc:375: } On 2015/10/28 01:00:18, Steven Holte wrote: > Add a TODO to clear the Java prefs (regardless if StoreSeedData fails) Done. https://codereview.chromium.org/1417503010/diff/20001/components/variations/v... components/variations/variations_seed_store.cc:377: base64_seed_data = local_state_->GetString(prefs::kVariationsCompressedSeed); On 2015/10/28 01:00:18, Steven Holte wrote: > To avoid this duplication it's probably better to do the migration before the > first attempt to load the seed, e.g. change > > bool VariationsSeedStore::LoadSeed(variations::VariationsSeed* seed) { > invalid_base64_signature_.clear(); > > std::string seed_data; > if (!ReadSeedData(&seed_data)) > return false; > > to > > > bool VariationsSeedStore::LoadSeed(variations::VariationsSeed* seed) { > invalid_base64_signature_.clear(); > > if (!local_state_->HasPrefPath(prefs::kVariationsSeedSignature)) > MigrateJavaSeed(); > > std::string seed_data; > if (!ReadSeedData(&seed_data)) > return false; Done.
Couple nits, otherwise lgtm. https://codereview.chromium.org/1417503010/diff/40001/components/variations/v... File components/variations/variations_seed_store.cc (right): https://codereview.chromium.org/1417503010/diff/40001/components/variations/v... components/variations/variations_seed_store.cc:343: LOG(WARNING) << "variationsTracker: Trying to read seed from Java side"; Remove or change this to DVLOG(1) << "Importing first run seed from Java preferences." https://codereview.chromium.org/1417503010/diff/40001/components/variations/v... components/variations/variations_seed_store.cc:349: RecordVariationSeedEmptyHistogram(VARIATIONS_SEED_EMPTY); This should already get recorded by ReadSeedData (since we won't have stored a seed). We should probably add a new UMA histogram recording the state of the first run seed specifically, but can leave that as a TODO.
asvitkine@chromium.org changed reviewers: + bauerb@chromium.org
+bauerb, who commented on the other CL. Bernhard: Per some offline discussion, the CL is being split into two parts as Alex is still investigating an issue with that Java-side Service start up. Alex: See Bernhard's comments on the previous CL - I suspect a lot of them still apply to this one.
https://codereview.chromium.org/1417503010/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/1417503010/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:222: public static final String VARIATIONS_FIRST_RUN_SEED_SIGNATURE = "variations_seed_signature"; See Bernhard's suggestion about having a new class for this functionality. I'm also OK with a TODO() here about that and doing that in a separate CL, though. (But ultimately up to owners of this code.) https://codereview.chromium.org/1417503010/diff/40001/chrome/browser/android/... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/1417503010/diff/40001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:975: reinterpret_cast<jbyte*>(&(seed[0]))); Can you make a helper similar to https://code.google.com/p/chromium/codesearch#chromium/src/base/android/jni_a... for this? https://codereview.chromium.org/1417503010/diff/40001/chrome/browser/android/... File chrome/browser/android/preferences/pref_service_bridge.h (right): https://codereview.chromium.org/1417503010/diff/40001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.h:31: // (seed signature and country code) Please use a complete sentence. https://codereview.chromium.org/1417503010/diff/40001/components/variations/v... File components/variations/variations_seed_store.cc (right): https://codereview.chromium.org/1417503010/diff/40001/components/variations/v... components/variations/variations_seed_store.cc:344: std::string seed_data, seed_signature, seed_country; Nit: 1 param per line. https://codereview.chromium.org/1417503010/diff/40001/components/variations/v... components/variations/variations_seed_store.cc:368: Nit: Remove extra diff here, same on line 370.
https://codereview.chromium.org/1417503010/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/1417503010/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:240: sVariationsFirstRunSeedData = rawSeed; Actually, I'm a bit confused here about why we are storing this in a static variable while storing the other values in prefs. It seems like we should be doing all one way or the other. I guess using the static variable assumes Chrome hasn't been evicted from memory by the time it is started. We probably need to check what conditions that will be true in.
Is there another CL that actually uses the methods added in PrefServiceBridge? It would be easier to provide advice here if you could link to that other CL. General comments: Please add more context to the commit message. Unless it's a very small almost trivial change, it's generally good to explain WHY and HOW: WHY - why the change is being made: is it part of a larger feature, a bug fix, an optimization, cleanup, etc? HOW - explain anything technically tricky or interesting about the CL: are you adding a new abstraction or class or framework, refactoring things, working around an OS bug in a non-obvious way, etc Also add a BUG number to the commit message so reviewers and people looking at this CL later on can get more background. https://codereview.chromium.org/1417503010/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/1417503010/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:222: public static final String VARIATIONS_FIRST_RUN_SEED_SIGNATURE = "variations_seed_signature"; On 2015/10/29 15:04:12, Alexei Svitkine (slow) wrote: > See Bernhard's suggestion about having a new class for this functionality. > > I'm also OK with a TODO() here about that and doing that in a separate CL, > though. (But ultimately up to owners of this code.) Yes, please pull this out into a separate class. PrefServiceBridge is intended for one-off methods to allow getting and setting native prefs from Java, and is mainly used by code inside of Settings. This chunk is large enough and logically cohesive that it should be in a separate file. To add a new Java/C++ bridge, just follow the pattern used by all the classes listed in chrome_jni_registrar.cc. Also, this API is a bit wonky. Without yet seeing how this API is actually used, I would recommend this design with a single public method: public static void setVariationsSeed(String signature, String country); If you also need public getters, then you could add methods like getVariationsSeedSignature(). Any method or constant that doesn't need to be public should be kept private. https://codereview.chromium.org/1417503010/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:233: prefs.edit().remove(prefName).apply(); There's no need to call remove(). The next line will overwrite the existing value anyway. https://codereview.chromium.org/1417503010/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:240: sVariationsFirstRunSeedData = rawSeed; On 2015/10/29 21:26:56, Steven Holte wrote: > Actually, I'm a bit confused here about why we are storing this in a static > variable while storing the other values in prefs. It seems like we should be > doing all one way or the other. I guess using the static variable assumes > Chrome hasn't been evicted from memory by the time it is started. We probably > need to check what conditions that will be true in. statics should rarely be used in Java. They're global and they don't ever get garbage collected. It's possible that a static is appropriate here, but some justification is warranted. Does this seed need to live for the entire lifetime of the process? (How large is the seed, anyway?) https://codereview.chromium.org/1417503010/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:244: public static byte[] getVariationsFirstRunSeedData(Context context) { These methods can be private since they're only called via JNI and they're an implementation detail of this class
https://codereview.chromium.org/1417503010/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/1417503010/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:240: sVariationsFirstRunSeedData = rawSeed; On 2015/10/30 00:46:36, newt wrote: > On 2015/10/29 21:26:56, Steven Holte wrote: > > Actually, I'm a bit confused here about why we are storing this in a static > > variable while storing the other values in prefs. It seems like we should be > > doing all one way or the other. I guess using the static variable assumes > > Chrome hasn't been evicted from memory by the time it is started. We probably > > need to check what conditions that will be true in. > > statics should rarely be used in Java. They're global and they don't ever get > garbage collected. It's possible that a static is appropriate here, but some > justification is warranted. Does this seed need to live for the entire lifetime > of the process? (How large is the seed, anyway?) In addition, what if Chrome is killed _before_ this is read? https://codereview.chromium.org/1417503010/diff/40001/chrome/browser/android/... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/1417503010/diff/40001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:955: //static Space after // https://codereview.chromium.org/1417503010/diff/40001/components/variations/v... File components/variations/variations_seed_store.h (right): https://codereview.chromium.org/1417503010/diff/40001/components/variations/v... components/variations/variations_seed_store.h:81: const base::Callback<void(std::string*, std::string*, std::string*)>& Hm... instead of a single callback that returns three values (where in the actual implementation the three values seems to be pretty much independent), it might be cleaner to just make an interface with three methods out of this.
https://codereview.chromium.org/1417503010/diff/40001/components/variations/v... File components/variations/variations_seed_store.h (right): https://codereview.chromium.org/1417503010/diff/40001/components/variations/v... components/variations/variations_seed_store.h:81: const base::Callback<void(std::string*, std::string*, std::string*)>& On 2015/10/30 09:56:08, Bernhard Bauer wrote: > Hm... instead of a single callback that returns three values (where in the > actual implementation the three values seems to be pretty much independent), it > might be cleaner to just make an interface with three methods out of this. They are not really independent, since they all come from a single variations seed fetch. A previous version of the CL had them independent as calls of 1 function, and I think this version is a bit cleaner.
https://codereview.chromium.org/1417503010/diff/40001/components/variations/v... File components/variations/variations_seed_store.h (right): https://codereview.chromium.org/1417503010/diff/40001/components/variations/v... components/variations/variations_seed_store.h:81: const base::Callback<void(std::string*, std::string*, std::string*)>& On 2015/11/02 19:05:40, Steven Holte wrote: > On 2015/10/30 09:56:08, Bernhard Bauer wrote: > > Hm... instead of a single callback that returns three values (where in the > > actual implementation the three values seems to be pretty much independent), > it > > might be cleaner to just make an interface with three methods out of this. > > They are not really independent, since they all come from a single variations > seed fetch. A previous version of the CL had them independent as calls of 1 > function, and I think this version is a bit cleaner. What I meant was that it looks like doing this with independent methods would not require much duplication of code – it's just three different calls to read preferences. Alternatively, if these three values are really meant to be used together, would it make sense to put them into a struct?
https://codereview.chromium.org/1417503010/diff/40001/components/variations/v... File components/variations/variations_seed_store.h (right): https://codereview.chromium.org/1417503010/diff/40001/components/variations/v... components/variations/variations_seed_store.h:81: const base::Callback<void(std::string*, std::string*, std::string*)>& On 2015/11/02 19:10:07, Bernhard Bauer wrote: > On 2015/11/02 19:05:40, Steven Holte wrote: > > On 2015/10/30 09:56:08, Bernhard Bauer wrote: > > > Hm... instead of a single callback that returns three values (where in the > > > actual implementation the three values seems to be pretty much independent), > > it > > > might be cleaner to just make an interface with three methods out of this. > > > > They are not really independent, since they all come from a single variations > > seed fetch. A previous version of the CL had them independent as calls of 1 > > function, and I think this version is a bit cleaner. > > What I meant was that it looks like doing this with independent methods would > not require much duplication of code – it's just three different calls to read > preferences. Alternatively, if these three values are really meant to be used > together, would it make sense to put them into a struct? They are meant to be used together. I think a struct sounds good, but we'd need to find a place for it given right now the prefs bridge code doesn't depend on the variations code. I suggest adding a TODO for this but leaving it as-is for this CL. And then address in a follow-up CL. (Bernhard, for context we're trying hard to get this in before the M48 branch since the data compression folks are planning to use this for a rollout they have targeted for M48. So I'd rather this code start landing sooner and being tested by the waterfall, etc. Some minor parts like this can be cleaned post the initial commit. More important things I think it still makes sense to get right in this first CL, but in my opinion this can wait for a follow-up CL).
Fixed most objections. Still working on moving methods from PrefServiceBridge to a separate class. https://codereview.chromium.org/1417503010/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/1417503010/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:222: public static final String VARIATIONS_FIRST_RUN_SEED_SIGNATURE = "variations_seed_signature"; On 2015/10/30 00:46:36, newt wrote: > On 2015/10/29 15:04:12, Alexei Svitkine (slow) wrote: > > See Bernhard's suggestion about having a new class for this functionality. > > > > I'm also OK with a TODO() here about that and doing that in a separate CL, > > though. (But ultimately up to owners of this code.) > > Yes, please pull this out into a separate class. PrefServiceBridge is intended > for one-off methods to allow getting and setting native prefs from Java, and is > mainly used by code inside of Settings. This chunk is large enough and logically > cohesive that it should be in a separate file. > > To add a new Java/C++ bridge, just follow the pattern used by all the classes > listed in chrome_jni_registrar.cc. > > Also, this API is a bit wonky. Without yet seeing how this API is actually used, > I would recommend this design with a single public method: > > public static void setVariationsSeed(String signature, String country); > > If you also need public getters, then you could add methods like > getVariationsSeedSignature(). > > Any method or constant that doesn't need to be public should be kept private. Acknowledged. https://codereview.chromium.org/1417503010/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:233: prefs.edit().remove(prefName).apply(); On 2015/10/30 00:46:36, newt wrote: > There's no need to call remove(). The next line will overwrite the existing > value anyway. Done. https://codereview.chromium.org/1417503010/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:244: public static byte[] getVariationsFirstRunSeedData(Context context) { On 2015/10/30 00:46:36, newt wrote: > These methods can be private since they're only called via JNI and they're an > implementation detail of this class Done. https://codereview.chromium.org/1417503010/diff/40001/chrome/browser/android/... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/1417503010/diff/40001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:955: //static On 2015/10/30 09:56:08, Bernhard Bauer wrote: > Space after // Done. https://codereview.chromium.org/1417503010/diff/40001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:975: reinterpret_cast<jbyte*>(&(seed[0]))); On 2015/10/29 15:04:12, Alexei Svitkine (slow) wrote: > Can you make a helper similar to > https://code.google.com/p/chromium/codesearch#chromium/src/base/android/jni_a... > for this? Done. https://codereview.chromium.org/1417503010/diff/40001/chrome/browser/android/... File chrome/browser/android/preferences/pref_service_bridge.h (right): https://codereview.chromium.org/1417503010/diff/40001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.h:31: // (seed signature and country code) On 2015/10/29 15:04:12, Alexei Svitkine (slow) wrote: > Please use a complete sentence. Done. https://codereview.chromium.org/1417503010/diff/40001/components/variations/v... File components/variations/variations_seed_store.cc (right): https://codereview.chromium.org/1417503010/diff/40001/components/variations/v... components/variations/variations_seed_store.cc:343: LOG(WARNING) << "variationsTracker: Trying to read seed from Java side"; On 2015/10/29 01:37:12, Steven Holte wrote: > Remove or change this to > DVLOG(1) << "Importing first run seed from Java preferences." Done. https://codereview.chromium.org/1417503010/diff/40001/components/variations/v... components/variations/variations_seed_store.cc:344: std::string seed_data, seed_signature, seed_country; On 2015/10/29 15:04:12, Alexei Svitkine (slow) wrote: > Nit: 1 param per line. Done. https://codereview.chromium.org/1417503010/diff/40001/components/variations/v... components/variations/variations_seed_store.cc:349: RecordVariationSeedEmptyHistogram(VARIATIONS_SEED_EMPTY); On 2015/10/29 01:37:12, Steven Holte wrote: > This should already get recorded by ReadSeedData (since we won't have stored a > seed). > > We should probably add a new UMA histogram recording the state of the first run > seed specifically, but can leave that as a TODO. Done. https://codereview.chromium.org/1417503010/diff/40001/components/variations/v... components/variations/variations_seed_store.cc:368: On 2015/10/29 15:04:12, Alexei Svitkine (slow) wrote: > Nit: Remove extra diff here, same on line 370. Done.
https://codereview.chromium.org/1417503010/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/1417503010/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:222: public static final String VARIATIONS_FIRST_RUN_SEED_SIGNATURE = "variations_seed_signature"; On 2015/10/30 00:46:36, newt wrote: > On 2015/10/29 15:04:12, Alexei Svitkine (slow) wrote: > > See Bernhard's suggestion about having a new class for this functionality. > > > > I'm also OK with a TODO() here about that and doing that in a separate CL, > > though. (But ultimately up to owners of this code.) > > Yes, please pull this out into a separate class. PrefServiceBridge is intended > for one-off methods to allow getting and setting native prefs from Java, and is > mainly used by code inside of Settings. This chunk is large enough and logically > cohesive that it should be in a separate file. > > To add a new Java/C++ bridge, just follow the pattern used by all the classes > listed in chrome_jni_registrar.cc. ^^^ https://codereview.chromium.org/1417503010/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:240: sVariationsFirstRunSeedData = rawSeed; On 2015/10/30 09:56:08, Bernhard Bauer wrote: > On 2015/10/30 00:46:36, newt wrote: > > On 2015/10/29 21:26:56, Steven Holte wrote: > > > Actually, I'm a bit confused here about why we are storing this in a static > > > variable while storing the other values in prefs. It seems like we should > be > > > doing all one way or the other. I guess using the static variable assumes > > > Chrome hasn't been evicted from memory by the time it is started. We > probably > > > need to check what conditions that will be true in. > > > > statics should rarely be used in Java. They're global and they don't ever get > > garbage collected. It's possible that a static is appropriate here, but some > > justification is warranted. Does this seed need to live for the entire > lifetime > > of the process? (How large is the seed, anyway?) > > In addition, what if Chrome is killed _before_ this is read? ^^^ https://codereview.chromium.org/1417503010/diff/60001/chrome/browser/metrics/... File chrome/browser/metrics/variations/chrome_variations_service_client.h (right): https://codereview.chromium.org/1417503010/diff/60001/chrome/browser/metrics/... chrome/browser/metrics/variations/chrome_variations_service_client.h:13: #if defined(OS_ANDROID) Conditional includes go after regular includes (ordered by their condition, i.e. OS_ANDROID before OS_WINDOW). https://codereview.chromium.org/1417503010/diff/60001/components/variations/s... File components/variations/service/variations_service_client.h (right): https://codereview.chromium.org/1417503010/diff/60001/components/variations/s... components/variations/service/variations_service_client.h:65: GetVariationsFirstRunSeedCallback() = 0; Indent two more spaces. Really, `git cl format` is your friend here. https://codereview.chromium.org/1417503010/diff/60001/components/variations/v... File components/variations/variations_seed_store.cc (right): https://codereview.chromium.org/1417503010/diff/60001/components/variations/v... components/variations/variations_seed_store.cc:157: ImportFirstRunJavaSeed(); Indent. https://codereview.chromium.org/1417503010/diff/60001/components/variations/v... File components/variations/variations_seed_store.h (right): https://codereview.chromium.org/1417503010/diff/60001/components/variations/v... components/variations/variations_seed_store.h:82: callback) { `git cl format`. Also, the fact that you have to break this signature twice should tell you it might be time to make a type for this callback 😃 https://codereview.chromium.org/1417503010/diff/60001/components/variations/v... components/variations/variations_seed_store.h:154: get_variations_first_run_seed_; Indent.
https://codereview.chromium.org/1417503010/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/1417503010/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:222: public static final String VARIATIONS_FIRST_RUN_SEED_SIGNATURE = "variations_seed_signature"; On 2015/11/03 09:44:49, Bernhard Bauer wrote: > On 2015/10/30 00:46:36, newt wrote: > > On 2015/10/29 15:04:12, Alexei Svitkine (slow) wrote: > > > See Bernhard's suggestion about having a new class for this functionality. > > > > > > I'm also OK with a TODO() here about that and doing that in a separate CL, > > > though. (But ultimately up to owners of this code.) > > > > Yes, please pull this out into a separate class. PrefServiceBridge is intended > > for one-off methods to allow getting and setting native prefs from Java, and > is > > mainly used by code inside of Settings. This chunk is large enough and > logically > > cohesive that it should be in a separate file. > > > > To add a new Java/C++ bridge, just follow the pattern used by all the classes > > listed in chrome_jni_registrar.cc. > > ^^^ (Or at the very least add a TODO.)
https://codereview.chromium.org/1417503010/diff/60001/chrome/browser/android/... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/1417503010/diff/60001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:121: std::string* out) { I suggest just making this return the std::string() Then, your code actually can become simpler. One of the returns can change to "return std::string();" and another to "return std::string(array_data.begin(), array_data.end());" It will simplify the code and likely won't introduce an extra copy since compilers can optimize returned C++ objects (either via move ctors or in other ways)/ https://codereview.chromium.org/1417503010/diff/60001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:978: Java_PrefServiceBridge_getVariationsFirstRunSeedData( Nit: Run "git cl format", since you have the wrong indent here.
I filed crbug.com/551029 for this, please set BUG=551029 on the CL.
Description was changed from ========== Variations seed is pulled from the Java application on the first launch of Chrome BUG= ========== to ========== Variations seed is pulled from the Java application on the first launch of Chrome BUG=551029 ==========
On 2015/11/03 18:40:04, Alexei Svitkine (slow) wrote: > I filed crbug.com/551029 for this, please set BUG=551029 on the CL. Done.
https://codereview.chromium.org/1417503010/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/1417503010/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:240: sVariationsFirstRunSeedData = rawSeed; On 2015/11/03 09:44:49, Bernhard Bauer wrote: > On 2015/10/30 09:56:08, Bernhard Bauer wrote: > > On 2015/10/30 00:46:36, newt wrote: > > > On 2015/10/29 21:26:56, Steven Holte wrote: > > > > Actually, I'm a bit confused here about why we are storing this in a > static > > > > variable while storing the other values in prefs. It seems like we should > > be > > > > doing all one way or the other. I guess using the static variable assumes > > > > Chrome hasn't been evicted from memory by the time it is started. We > > probably > > > > need to check what conditions that will be true in. > > > > > > statics should rarely be used in Java. They're global and they don't ever > get > > > garbage collected. It's possible that a static is appropriate here, but some > > > justification is warranted. Does this seed need to live for the entire > > lifetime > > > of the process? (How large is the seed, anyway?) > > > > In addition, what if Chrome is killed _before_ this is read? > > ^^^ I think we should store the three things in the same way, else there are weird edge cases with different lifetimes between them. It sounds like using prefs is better because of the process possibly being killed and all that stuff. In that case, you'll likely need to serialize the pref before storing, such as via base64 - and de-serialize when querying it.
https://codereview.chromium.org/1417503010/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/1417503010/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:222: public static final String VARIATIONS_FIRST_RUN_SEED_SIGNATURE = "variations_seed_signature"; On 2015/11/03 09:45:22, Bernhard Bauer wrote: > On 2015/11/03 09:44:49, Bernhard Bauer wrote: > > On 2015/10/30 00:46:36, newt wrote: > > > On 2015/10/29 15:04:12, Alexei Svitkine (slow) wrote: > > > > See Bernhard's suggestion about having a new class for this functionality. > > > > > > > > I'm also OK with a TODO() here about that and doing that in a separate CL, > > > > though. (But ultimately up to owners of this code.) > > > > > > Yes, please pull this out into a separate class. PrefServiceBridge is > intended > > > for one-off methods to allow getting and setting native prefs from Java, and > > is > > > mainly used by code inside of Settings. This chunk is large enough and > > logically > > > cohesive that it should be in a separate file. > > > > > > To add a new Java/C++ bridge, just follow the pattern used by all the > classes > > > listed in chrome_jni_registrar.cc. > > > > ^^^ > > (Or at the very least add a TODO.) Acknowledged. https://codereview.chromium.org/1417503010/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:240: sVariationsFirstRunSeedData = rawSeed; On 2015/11/03 18:44:01, Alexei Svitkine (slow) wrote: > On 2015/11/03 09:44:49, Bernhard Bauer wrote: > > On 2015/10/30 09:56:08, Bernhard Bauer wrote: > > > On 2015/10/30 00:46:36, newt wrote: > > > > On 2015/10/29 21:26:56, Steven Holte wrote: > > > > > Actually, I'm a bit confused here about why we are storing this in a > > static > > > > > variable while storing the other values in prefs. It seems like we > should > > > be > > > > > doing all one way or the other. I guess using the static variable > assumes > > > > > Chrome hasn't been evicted from memory by the time it is started. We > > > probably > > > > > need to check what conditions that will be true in. > > > > > > > > statics should rarely be used in Java. They're global and they don't ever > > get > > > > garbage collected. It's possible that a static is appropriate here, but > some > > > > justification is warranted. Does this seed need to live for the entire > > > lifetime > > > > of the process? (How large is the seed, anyway?) > > > > > > In addition, what if Chrome is killed _before_ this is read? > > > > ^^^ > > I think we should store the three things in the same way, else there are weird > edge cases with different lifetimes between them. It sounds like using prefs is > better because of the process possibly being killed and all that stuff. In that > case, you'll likely need to serialize the pref before storing, such as via > base64 - and de-serialize when querying it. Done. https://codereview.chromium.org/1417503010/diff/60001/chrome/browser/android/... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/1417503010/diff/60001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:121: std::string* out) { On 2015/11/03 18:34:03, Alexei Svitkine (slow) wrote: > I suggest just making this return the std::string() > > Then, your code actually can become simpler. One of the returns can change to > "return std::string();" and another to "return std::string(array_data.begin(), > array_data.end());" > > It will simplify the code and likely won't introduce an extra copy since > compilers can optimize returned C++ objects (either via move ctors or in other > ways)/ Removed this function at all. Done. https://codereview.chromium.org/1417503010/diff/60001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:978: Java_PrefServiceBridge_getVariationsFirstRunSeedData( On 2015/11/03 18:34:03, Alexei Svitkine (slow) wrote: > Nit: Run "git cl format", since you have the wrong indent here. Done. https://codereview.chromium.org/1417503010/diff/60001/chrome/browser/metrics/... File chrome/browser/metrics/variations/chrome_variations_service_client.h (right): https://codereview.chromium.org/1417503010/diff/60001/chrome/browser/metrics/... chrome/browser/metrics/variations/chrome_variations_service_client.h:13: #if defined(OS_ANDROID) On 2015/11/03 09:44:49, Bernhard Bauer wrote: > Conditional includes go after regular includes (ordered by their condition, i.e. > OS_ANDROID before OS_WINDOW). Done. https://codereview.chromium.org/1417503010/diff/60001/components/variations/s... File components/variations/service/variations_service_client.h (right): https://codereview.chromium.org/1417503010/diff/60001/components/variations/s... components/variations/service/variations_service_client.h:65: GetVariationsFirstRunSeedCallback() = 0; On 2015/11/03 09:44:49, Bernhard Bauer wrote: > Indent two more spaces. Really, `git cl format` is your friend here. Done. https://codereview.chromium.org/1417503010/diff/60001/components/variations/v... File components/variations/variations_seed_store.cc (right): https://codereview.chromium.org/1417503010/diff/60001/components/variations/v... components/variations/variations_seed_store.cc:157: ImportFirstRunJavaSeed(); On 2015/11/03 09:44:49, Bernhard Bauer wrote: > Indent. Done. https://codereview.chromium.org/1417503010/diff/60001/components/variations/v... File components/variations/variations_seed_store.h (right): https://codereview.chromium.org/1417503010/diff/60001/components/variations/v... components/variations/variations_seed_store.h:82: callback) { On 2015/11/03 09:44:49, Bernhard Bauer wrote: > `git cl format`. Also, the fact that you have to break this signature twice > should tell you it might be time to make a type for this callback 😃 Done. https://codereview.chromium.org/1417503010/diff/60001/components/variations/v... components/variations/variations_seed_store.h:154: get_variations_first_run_seed_; On 2015/11/03 09:44:49, Bernhard Bauer wrote: > Indent. Done.
https://codereview.chromium.org/1417503010/diff/80001/chrome/browser/android/... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/1417503010/diff/80001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:966: env, GetApplicationContext()); Indent. Please just run `git cl format` over your CL before you upload it; it's going to save you a bunch of unnecessary review roundtrips for style errors. https://codereview.chromium.org/1417503010/diff/80001/chrome/browser/metrics/... File chrome/browser/metrics/variations/chrome_variations_service_client.h (right): https://codereview.chromium.org/1417503010/diff/80001/chrome/browser/metrics/... chrome/browser/metrics/variations/chrome_variations_service_client.h:12: No empty line. https://codereview.chromium.org/1417503010/diff/80001/components/variations/v... File components/variations/variations_seed_store.cc (right): https://codereview.chromium.org/1417503010/diff/80001/components/variations/v... components/variations/variations_seed_store.cc:346: LOG(WARNING) << "Importing first run seed from Java preferences."; Change back to DVLOG(1).
https://codereview.chromium.org/1417503010/diff/80001/chrome/browser/android/... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/1417503010/diff/80001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:966: env, GetApplicationContext()); On 2015/11/04 09:08:57, Bernhard Bauer wrote: > Indent. Please just run `git cl format` over your CL before you upload it; it's > going to save you a bunch of unnecessary review roundtrips for style errors. I have already run this command but git ignored whitespace-only changes. Done. https://codereview.chromium.org/1417503010/diff/80001/chrome/browser/metrics/... File chrome/browser/metrics/variations/chrome_variations_service_client.h (right): https://codereview.chromium.org/1417503010/diff/80001/chrome/browser/metrics/... chrome/browser/metrics/variations/chrome_variations_service_client.h:12: On 2015/11/04 09:08:57, Bernhard Bauer wrote: > No empty line. Done. https://codereview.chromium.org/1417503010/diff/80001/components/variations/v... File components/variations/variations_seed_store.cc (right): https://codereview.chromium.org/1417503010/diff/80001/components/variations/v... components/variations/variations_seed_store.cc:346: LOG(WARNING) << "Importing first run seed from Java preferences."; On 2015/11/04 09:08:57, Bernhard Bauer wrote: > Change back to DVLOG(1). Done.
lgtm https://codereview.chromium.org/1417503010/diff/80001/chrome/browser/android/... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/1417503010/diff/80001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:966: env, GetApplicationContext()); On 2015/11/04 09:42:29, Alexander Agulenko wrote: > On 2015/11/04 09:08:57, Bernhard Bauer wrote: > > Indent. Please just run `git cl format` over your CL before you upload it; > it's > > going to save you a bunch of unnecessary review roundtrips for style errors. > > I have already run this command but git ignored whitespace-only changes. > > Done. Ok, then you probably should configure Git not to do that 😃
LGTM % my remaining comments below I'm hoping we can land this today and to start reviewing the follow-up CLs that address the TODOs. https://codereview.chromium.org/1417503010/diff/120001/chrome/browser/android... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/1417503010/diff/120001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:8: #include <vector> Is this needed anymore? https://codereview.chromium.org/1417503010/diff/120001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:12: #include "base/android/jni_array.h" Is this needed anymore? https://codereview.chromium.org/1417503010/diff/120001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:964: Java_PrefServiceBridge_getVariationsFirstRunSeedData( Nit: Now that you're returning base64, please name things accordingly. getVariationsFirstRunSeedDataBase64(), j_seed_data_base64, etc. https://codereview.chromium.org/1417503010/diff/120001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:972: // deserialize raw seed data. Nit: Capitalize https://codereview.chromium.org/1417503010/diff/120001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:973: base::Base64Decode(ConvertJavaStringToUTF8(j_seed_data), seed_data); Nit: This API could potentially fail. I think it should be something like: if (!base::Base64Decode(ConvertJavaStringToUTF8(j_seed_data), seed_data)) seed_data->clear();
agulenko@google.com changed required reviewers: - newt@chromium.org
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/1417503010/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1417503010/120001
I've sent of a cq try run of this, to see if there's any issues on the bots.
https://codereview.chromium.org/1417503010/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/1417503010/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:223: // TODO(agulenko): Move this piece of code to a separate class. Please remember to do this soon (put it on your personal TODO list). It's all too common that promised follow-up work is quickly forgotten and someone else is left to deal with the mess. I'm not saying that *you* would do this... but many people do. Thank you :) https://codereview.chromium.org/1417503010/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:231: return prefs.getString(prefName, new String()); s/new String()/""/ (no need to allocate a new String, when "" is available) https://codereview.chromium.org/1417503010/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:242: setVariationsFirstRunSeedPref( write this as: SharedPreferences prefs = PreferenceManager.getDefaultSharedPreferences(context); prefs.edit() .putString(VARIATIONS_FIRST_RUN_SEED, Base64.encodeToString(...)) .putString(VARIATIONS_FIRST_RUN_SEED_SIGNATURE, signature) .putString(VARIATIONS_FIRST_RUN_SEED_COUNTRY, country) .apply(); That's more efficient that getting the default shared preferences object three times and making three separate updates to it. It's also less code: you can delete setVariationsFirstRunSeedPref(). https://codereview.chromium.org/1417503010/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:249: private static String getVariationsFirstRunSeedData(Context context) { Do the base64 decoding here (in Java), and change the return value to byte[]. That minimizes the amount of code that cares how the rawSeed was stored in shared prefs. (and it's probably faster because it eliminates the JNI string conversion using ConvertJavaStringToUTF8()) This will also avoid issues between different base64 encoding/decoding settings. E.g. some encoders append ='s to the end of the string to make the string length a multiple of four; some decoders will fail if these equal signs aren't present (some could fail if ='s are present).
https://codereview.chromium.org/1417503010/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/1417503010/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:223: // TODO(agulenko): Move this piece of code to a separate class. On 2015/11/04 18:10:43, newt wrote: > Please remember to do this soon (put it on your personal TODO list). It's all > too common that promised follow-up work is quickly forgotten and someone else is > left to deal with the mess. I'm not saying that *you* would do this... but many > people do. Thank you :) Acknowledged. https://codereview.chromium.org/1417503010/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:231: return prefs.getString(prefName, new String()); On 2015/11/04 18:10:43, newt wrote: > s/new String()/""/ > > (no need to allocate a new String, when "" is available) Done. https://codereview.chromium.org/1417503010/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:242: setVariationsFirstRunSeedPref( On 2015/11/04 18:10:43, newt wrote: > write this as: > > SharedPreferences prefs = > PreferenceManager.getDefaultSharedPreferences(context); > prefs.edit() > .putString(VARIATIONS_FIRST_RUN_SEED, Base64.encodeToString(...)) > .putString(VARIATIONS_FIRST_RUN_SEED_SIGNATURE, signature) > .putString(VARIATIONS_FIRST_RUN_SEED_COUNTRY, country) > .apply(); > > That's more efficient that getting the default shared preferences object three > times and making three separate updates to it. It's also less code: you can > delete setVariationsFirstRunSeedPref(). Done. https://codereview.chromium.org/1417503010/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:249: private static String getVariationsFirstRunSeedData(Context context) { On 2015/11/04 18:10:43, newt wrote: > Do the base64 decoding here (in Java), and change the return value to byte[]. > That minimizes the amount of code that cares how the rawSeed was stored in > shared prefs. (and it's probably faster because it eliminates the JNI string > conversion using ConvertJavaStringToUTF8()) > > This will also avoid issues between different base64 encoding/decoding settings. > E.g. some encoders append ='s to the end of the string to make the string length > a multiple of four; some decoders will fail if these equal signs aren't present > (some could fail if ='s are present). 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/1417503010/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1417503010/160001
chrome/android and chrome/browser/android lgtm. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
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/1417503010/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1417503010/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
LGTM % two small nits https://codereview.chromium.org/1417503010/diff/180001/chrome/browser/android... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/1417503010/diff/180001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:120: if (!byte_array) { Nit: No {}'s https://codereview.chromium.org/1417503010/diff/180001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:121: return ""; Nit: return std::string();
The CQ bit was checked by agulenko@google.com
The CQ bit was unchecked by agulenko@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from holte@chromium.org, bauerb@chromium.org, newt@chromium.org Link to the patchset: https://codereview.chromium.org/1417503010/#ps180001 (title: "Moved GetVariationsFirstRunSeedCallback() to a separate .cc file due to clang build errors")
The CQ bit was unchecked by agulenko@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1417503010/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1417503010/180001
(maybe you forgot to upload a new patch set with my nits addressed and the gyp/gn additions?)
On 2015/11/05 03:49:59, Alexei Svitkine (slow) wrote: > (maybe you forgot to upload a new patch set with my nits addressed and the > gyp/gn additions?) Sure. I am not sure what I was thinking about at that moment, maybe I just was too hungry. Will upload the patch in a couple of minutes.
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/1417503010/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1417503010/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
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/1417503010/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1417503010/220001
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 agulenko@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, asvitkine@chromium.org, holte@chromium.org, newt@chromium.org Link to the patchset: https://codereview.chromium.org/1417503010/#ps220001 (title: "Newline added due to clang build error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1417503010/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1417503010/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/25d2111a2857676f524fc4f686ddf14ed08d3fd9 Cr-Commit-Position: refs/heads/master@{#358006} |
