Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10343007:
Retrieving Chrome Variations seed from server and storing in local prefs. Loading seed data from lo… (Closed)
|
Created:
8 years, 7 months ago by jwd Modified:
8 years, 7 months ago CC:
chromium-reviews, finch-team_google.com, jar (doing other things), MAD, Ilya Sherman Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRetrieving Chrome Variations seed from server and storing in local prefs. Loading seed data from local prefs during start up.
BUG=121695
TEST=Clear the Local State file from the chrome data dir. Run chrome. Ensure the variations_seed pref is set.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=135654
Patch Set 1 #
Total comments: 56
Patch Set 2 : Fixing many nits #
Total comments: 39
Patch Set 3 : Fixing more nits from everyone! #
Total comments: 10
Patch Set 4 : Nits and Making VariationsService a singleton again #Patch Set 5 : #
Total comments: 2
Patch Set 6 : MAD's final nits #
Messages
Total messages: 17 (0 generated)
This is the first pass at the variations client. It's not ready to commit yet, but I want to get some comments on it now. Two big things it's missing are periodic server requests and a way of accessing the seed data. There will also be work to use the seed data to control field trials, but that will come in another CL.
Drive-by: Looks good at a high level; lots of nits inline. https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/chrome_... File chrome/browser/chrome_browser_main.h (right): https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/chrome_... chrome/browser/chrome_browser_main.h:216: scoped_ptr<VariationsService> variations_service_; nit: Can this be a member of the MetricsService instead? ChromeBrowserMain is already ginormous -- it would be nice not to add to the problem :) https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... File chrome/browser/metrics/proto/study.proto (right): https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... chrome/browser/metrics/proto/study.proto:13: // study.go. nit: Most of this comment is not relevant on the client-side. https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... File chrome/browser/metrics/variations_service.cc (right): https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:44: source->GetResponseCode() != 200); nit: No need to cache this in a variable name, as it's used in only one place. https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:46: seed_request_pending_.release()); nit: Please add a comment explaining why we cannot immediately free the pending request (assuming that's true -- otherwise just go ahead and reset() it here). This variable name could also be clearer -- perhaps something like |current_request_|? https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:47: if (error) { nit: You can add an early "return;" stmt here, and consequently reduce the indentation of the subsequent block. https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... File chrome/browser/metrics/variations_service.h (right): https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.h:21: virtual ~VariationsService() {} nit: It's a violation of the style guide to inline the constructor and destructor, since the class includes a non-trivial data member (the |variations_seed_|). https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.h:41: static const char* kDefaultVariationsServer; nit: This should probably be "const char kDefaultVariationsServer[]" https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.h:41: static const char* kDefaultVariationsServer; nit: Does this need to be exposed in the header file? https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.h:45: scoped_ptr<content::URLFetcher> seed_request_pending_; nit: The current name sounds like it is describing a boolean. Perhaps "pending_seed_request_"? https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/common/pref_nam... File chrome/common/pref_names.h (right): https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/common/pref_nam... chrome/common/pref_names.h:696: extern const char kVariationsSeed[]; nit: Please group this with the other metrics constants.
Cool! Also pleased with high level. Nits and questions inline. Are you planning on adding the other stuff (scheduled ping, etc) in a follow-up CL? Because I think that would be preferable. Please take a look at other users of URLFetcher and see if there is anything we can unit test here. I want to ensure that the service doesn't set bad seeds or whatever if the server gives us garbage (and does the right persisting/loading if we get correct input). https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/chrome_... File chrome/browser/chrome_browser_main.h (right): https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/chrome_... chrome/browser/chrome_browser_main.h:216: scoped_ptr<VariationsService> variations_service_; I'm not super familiar with BrowserProcess vs BrowserMainParts, so this question can help us figure out if this should go here: Why did you choose to have the VariationService live here and not in the BrowserProcess, where MetricsService live? Is it because we want a lifespan similar to FieldTrialList? https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... File chrome/browser/metrics/variations_service.cc (right): https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:9: #include "base/memory/singleton.h" Is this used here? https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:16: #include "net/base/escape.h" Is this used here? https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:26: base::Base64Decode(base64_seed_data, &seed_data); What if this fails? https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:56: void VariationsService::StoreSeedData( hyper nit: if "std::string seed_data," fits on this line, move it back up, then align "PrefService* local_prefs) {" to it's left hand side. https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:58: // Only store the seed data if it parses correctly ultra nit: correctly -> correctly. https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:62: if (!base::Base64Encode(seed_data, &base64_seed_data)) { nit: braces not needed for a single statement if body https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:67: } Break here. https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... File chrome/browser/metrics/variations_service.h (right): https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.h:18: nit: extra break here not needed https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.h:20: nit: extra break here not needed https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.h:22: Methods below can probably be alphabetized (and in cc). Except for the overriden one. You can just have that at the very top (or bottom) of the public: section. https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.h:26: void FetchVariationsSeed(); nit: Maybe StartFetchingVariationsSeed is a better name. https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.h:26: void FetchVariationsSeed(); Document, please. Remember to point out where the fetch returns the data (ie - in OnURLFetchComplete). https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.h:32: // is assumed to be the raw data stored in a string. It will Base64Encoded for nit: assumed to be the raw *what* data? Raw protobuf seralized data? https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.h:35: void StoreSeedData(std::string seed_data, PrefService* local_prefs); I think you can make seed_data a const ref. https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.h:35: void StoreSeedData(std::string seed_data, PrefService* local_prefs); I think StoreSeedData can probably be private? https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.h:37: // Register Variations related prefs in Lacal State. sp: Lacal -> Local https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.h:41: static const char* kDefaultVariationsServer; Currently this doesn't need to be publically exposed as part of this class, so you can just stick it in an unnamed-namespace in the cc instead. https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.h:44: nit: extra break here not needed https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.h:45: scoped_ptr<content::URLFetcher> seed_request_pending_; Please document.
Fixed most of the comments. One left unfixed, need to look into it more. Still need tests. Here's what I'm thinking so far: - test store load of some test built protobuf - test failure to load a corrupt protobuf from prefs, causing clearing of the pref value - test failure to store corrupt protobuf, leaving the prefs unchanged. SteveT@ wants me to look into to see what other URLFetcher code tests. I think some of his concerns are met by my current load and store strategy of checking if it parses as a valid protobuf and converts properly. https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/chrome_... File chrome/browser/chrome_browser_main.h (right): https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/chrome_... chrome/browser/chrome_browser_main.h:216: scoped_ptr<VariationsService> variations_service_; I put it here because I was modeling it after the TranslateManager. I will look into putting it in MetricsService instead or somewhere else. On 2012/05/03 04:02:52, SteveT wrote: > I'm not super familiar with BrowserProcess vs BrowserMainParts, so this question > can help us figure out if this should go here: Why did you choose to have the > VariationService live here and not in the BrowserProcess, where MetricsService > live? Is it because we want a lifespan similar to FieldTrialList? https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... File chrome/browser/metrics/proto/study.proto (right): https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... chrome/browser/metrics/proto/study.proto:13: // study.go. Good catch. Fixing that and changing it to not mention finch. On 2012/05/02 23:15:08, Ilya Sherman wrote: > nit: Most of this comment is not relevant on the client-side. https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... File chrome/browser/metrics/variations_service.cc (right): https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:9: #include "base/memory/singleton.h" Oops, no, not any more. Fixed On 2012/05/03 04:02:52, SteveT wrote: > Is this used here? https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:16: #include "net/base/escape.h" "Oh god how did this get in here I am not good with computers" http://goo.gl/66qWS Fixed On 2012/05/03 04:02:52, SteveT wrote: > Is this used here? https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:26: base::Base64Decode(base64_seed_data, &seed_data); I've decided to clear the pref if either this or the parse fails. On 2012/05/03 04:02:52, SteveT wrote: > What if this fails? https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:44: source->GetResponseCode() != 200); On 2012/05/02 23:15:08, Ilya Sherman wrote: > nit: No need to cache this in a variable name, as it's used in only one place. Done. https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:46: seed_request_pending_.release()); On 2012/05/02 23:15:08, Ilya Sherman wrote: > nit: Please add a comment explaining why we cannot immediately free the pending > request (assuming that's true -- otherwise just go ahead and reset() it here). > This variable name could also be clearer -- perhaps something like > |current_request_|? Ok, I've made this clearer. I have one question about it, but I'll leave a comment in the next patch set. https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:47: if (error) { On 2012/05/02 23:15:08, Ilya Sherman wrote: > nit: You can add an early "return;" stmt here, and consequently reduce the > indentation of the subsequent block. Done. https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:56: void VariationsService::StoreSeedData( On 2012/05/03 04:02:52, SteveT wrote: > hyper nit: if "std::string seed_data," fits on this line, move it back up, then > align "PrefService* local_prefs) {" to it's left hand side. Done. https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:58: // Only store the seed data if it parses correctly On 2012/05/03 04:02:52, SteveT wrote: > ultra nit: correctly -> correctly. Done. https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:62: if (!base::Base64Encode(seed_data, &base64_seed_data)) { On 2012/05/03 04:02:52, SteveT wrote: > nit: braces not needed for a single statement if body Done. https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:67: } On 2012/05/03 04:02:52, SteveT wrote: > Break here. Done. https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... File chrome/browser/metrics/variations_service.h (right): https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.h:18: fixed On 2012/05/03 04:02:52, SteveT wrote: > nit: extra break here not needed https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.h:20: fixed On 2012/05/03 04:02:52, SteveT wrote: > nit: extra break here not needed https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.h:21: virtual ~VariationsService() {} fixed On 2012/05/02 23:15:08, Ilya Sherman wrote: > nit: It's a violation of the style guide to inline the constructor and > destructor, since the class includes a non-trivial data member (the > |variations_seed_|). https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.h:26: void FetchVariationsSeed(); Agreed, fixed On 2012/05/03 04:02:52, SteveT wrote: > nit: Maybe StartFetchingVariationsSeed is a better name. https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.h:26: void FetchVariationsSeed(); Fixed On 2012/05/03 04:02:52, SteveT wrote: > Document, please. Remember to point out where the fetch returns the data (ie - > in OnURLFetchComplete). https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.h:32: // is assumed to be the raw data stored in a string. It will Base64Encoded for Fixed. On 2012/05/03 04:02:52, SteveT wrote: > nit: assumed to be the raw *what* data? Raw protobuf seralized data? https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.h:35: void StoreSeedData(std::string seed_data, PrefService* local_prefs); Fixed On 2012/05/03 04:02:52, SteveT wrote: > I think you can make seed_data a const ref. https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.h:35: void StoreSeedData(std::string seed_data, PrefService* local_prefs); Fixed On 2012/05/03 04:02:52, SteveT wrote: > I think StoreSeedData can probably be private? https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.h:37: // Register Variations related prefs in Lacal State. Fixed On 2012/05/03 04:02:52, SteveT wrote: > sp: Lacal -> Local https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.h:41: static const char* kDefaultVariationsServer; Fixed On 2012/05/03 04:02:52, SteveT wrote: > Currently this doesn't need to be publically exposed as part of this class, so > you can just stick it in an unnamed-namespace in the cc instead. https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.h:44: Fixed On 2012/05/03 04:02:52, SteveT wrote: > nit: extra break here not needed https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.h:45: scoped_ptr<content::URLFetcher> seed_request_pending_; Agreed, fixed. On 2012/05/02 23:15:08, Ilya Sherman wrote: > nit: The current name sounds like it is describing a boolean. Perhaps > "pending_seed_request_"? https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.h:45: scoped_ptr<content::URLFetcher> seed_request_pending_; Fixed On 2012/05/03 04:02:52, SteveT wrote: > Please document. https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/common/pref_nam... File chrome/common/pref_names.h (right): https://chromiumcodereview.appspot.com/10343007/diff/1/chrome/common/pref_nam... chrome/common/pref_names.h:696: extern const char kVariationsSeed[]; On 2012/05/02 23:15:08, Ilya Sherman wrote: > nit: Please group this with the other metrics constants. Done. https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/chro... File chrome/browser/chrome_browser_main.h (right): https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/chro... chrome/browser/chrome_browser_main.h:216: scoped_ptr<VariationsService> variations_service_; Still need to figure out a better place to put this. https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/metr... File chrome/browser/metrics/variations_service.cc (right): https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/metr... chrome/browser/metrics/variations_service.cc:65: } I'm not sure if this function should be responsible for |source| if the equality wasn't correct. TranslationManager deals with it here http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/translate/t.... But I haven't yet found another example of it (haven't looked VERY thoroughly).
Are you planning on doing more work in this CL (like moving the Service around and whatnot)? https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/metr... File chrome/browser/metrics/variations_service.cc (right): https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/metr... chrome/browser/metrics/variations_service.cc:15: #include "net/base/escape.h" Is it still here? https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/metr... chrome/browser/metrics/variations_service.cc:23: }; No semicolons after namespace closing braces, methinks. https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/metr... chrome/browser/metrics/variations_service.cc:33: // it. With that in mind, it's actually kind of weird that it could get into prefs corrupted in the first place, if your code But it's good to have this check anyway. This makes it harder for malicious doo-dads from changing a user's Local State prefs file on disk and adding some badness here.
Some drive-by: https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/chro... File chrome/browser/chrome_browser_main.cc (right): https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/chro... chrome/browser/chrome_browser_main.cc:566: variations_service_(NULL) { I don't think this line is needed at all - I believe the scoped_ptr default ctor already does this. https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/chro... File chrome/browser/chrome_browser_main.h (right): https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/chro... chrome/browser/chrome_browser_main.h:223: Nit: Don't add a random empty line. ;) https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/metr... File chrome/browser/metrics/variations_service.cc (right): https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/metr... chrome/browser/metrics/variations_service.cc:34: if (!(base::Base64Decode(base64_seed_data, &seed_data) && Shouldn't you check for empty string first, since that's what you store initially? (Avoids trying to base64 decode it, which probably would succeed, and then trying to parse an empty string as a proto, which probably fails). https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/metr... chrome/browser/metrics/variations_service.cc:35: variations_seed_.ParseFromString(seed_data))) { Nit: Change if (!(a && b)) to if (!a || !b). https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/metr... chrome/browser/metrics/variations_service.cc:54: scoped_ptr<const content::URLFetcher> current_request( Nit: rename to |request|. Then you can have less wrapping below! https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/metr... chrome/browser/metrics/variations_service.cc:77: if (seed.ParseFromString(seed_data)){ Nit: space before { https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/metr... chrome/browser/metrics/variations_service.cc:77: if (seed.ParseFromString(seed_data)){ Nit: do an early return here too instead of a nested if, for consistency. Or alternative a single if with those two conditions.
A few more nits and I'll see if I can find a more proper place to own and initialize the service... Thanks! BYE MAD... https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/chro... File chrome/browser/chrome_browser_main.cc (right): https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/chro... chrome/browser/chrome_browser_main.cc:1882: // Request new variations seed information from server. If we keep this here, we need to keep it within the ChromeOS #ifdef because we can't fetch a URL at this point in ChromeOS. https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/metr... File chrome/browser/metrics/variations_service.cc (right): https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/metr... chrome/browser/metrics/variations_service.cc:36: local_prefs->ClearPref(prefs::kVariationsSeed); I would add a DCHECK here or at least a VLOG(1)... https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/metr... chrome/browser/metrics/variations_service.cc:80: return; Please add VLOG(1) where we fail to do save... https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/metr... File chrome/browser/metrics/variations_service.h (right): https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/metr... chrome/browser/metrics/variations_service.h:32: Single empty line please... https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/metr... chrome/browser/metrics/variations_service.h:39: // will Base64Encoded for storage. If the string is invalid or the encoding will BE Base... https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/metr... chrome/browser/metrics/variations_service.h:48: chrome_variations::TrialsSeed variations_seed_; I don't think we will use the variations_seed_ data within the session. I think it's fine to keep it for now since we don't do anything with the data we fetch yet, but I'm guessing we'll remove it once we add the code to use it because I think we will use it as soon as we get it so we won't need to keep it... https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/common/pref_... File chrome/common/pref_names.cc (right): https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/common/pref_... chrome/common/pref_names.cc:1018: // String serialized form of variations seed protobuf '.' at the end of the comment sentence please.
https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/chro... File chrome/browser/chrome_browser_main.cc (right): https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/chro... chrome/browser/chrome_browser_main.cc:566: variations_service_(NULL) { On 2012/05/04 03:09:53, Alexei Svitkine wrote: > I don't think this line is needed at all - I believe the scoped_ptr default ctor > already does this. Done. https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/chro... File chrome/browser/chrome_browser_main.h (right): https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/chro... chrome/browser/chrome_browser_main.h:223: On 2012/05/04 03:09:53, Alexei Svitkine wrote: > Nit: Don't add a random empty line. ;) Done. https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/metr... File chrome/browser/metrics/variations_service.cc (right): https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/metr... chrome/browser/metrics/variations_service.cc:15: #include "net/base/escape.h" On 2012/05/03 23:46:24, SteveT wrote: > Is it still here? I told you, I'm not good with computers. It should be fixed now. https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/metr... chrome/browser/metrics/variations_service.cc:23: }; On 2012/05/03 23:46:24, SteveT wrote: > No semicolons after namespace closing braces, methinks. Done. https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/metr... chrome/browser/metrics/variations_service.cc:34: if (!(base::Base64Decode(base64_seed_data, &seed_data) && On 2012/05/04 03:09:53, Alexei Svitkine wrote: > Shouldn't you check for empty string first, since that's what you store > initially? (Avoids trying to base64 decode it, which probably would succeed, and > then trying to parse an empty string as a proto, which probably fails). The empty string does parse. I figured it was because the only members of TrialsSeed are optional or repeated. I didn't think short circuiting out for an empty string was worth it the check. https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/metr... chrome/browser/metrics/variations_service.cc:35: variations_seed_.ParseFromString(seed_data))) { On 2012/05/04 03:09:53, Alexei Svitkine wrote: > Nit: Change if (!(a && b)) to if (!a || !b). Done. https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/metr... chrome/browser/metrics/variations_service.cc:36: local_prefs->ClearPref(prefs::kVariationsSeed); On 2012/05/04 12:29:57, MAD wrote: > I would add a DCHECK here or at least a VLOG(1)... Done. https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/metr... chrome/browser/metrics/variations_service.cc:54: scoped_ptr<const content::URLFetcher> current_request( On 2012/05/04 03:09:53, Alexei Svitkine wrote: > Nit: rename to |request|. > > Then you can have less wrapping below! Done. https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/metr... chrome/browser/metrics/variations_service.cc:77: if (seed.ParseFromString(seed_data)){ On 2012/05/04 03:09:53, Alexei Svitkine wrote: > Nit: space before { Done. https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/metr... chrome/browser/metrics/variations_service.cc:80: return; On 2012/05/04 12:29:57, MAD wrote: > Please add VLOG(1) where we fail to do save... Done. https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/metr... File chrome/browser/metrics/variations_service.h (right): https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/metr... chrome/browser/metrics/variations_service.h:32: On 2012/05/04 12:29:57, MAD wrote: > Single empty line please... Done. https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/metr... chrome/browser/metrics/variations_service.h:39: // will Base64Encoded for storage. If the string is invalid or the encoding On 2012/05/04 12:29:57, MAD wrote: > will BE Base... Done. https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/metr... chrome/browser/metrics/variations_service.h:48: chrome_variations::TrialsSeed variations_seed_; On 2012/05/04 12:29:57, MAD wrote: > I don't think we will use the variations_seed_ data within the session. I think > it's fine to keep it for now since we don't do anything with the data we fetch > yet, but I'm guessing we'll remove it once we add the code to use it because I > think we will use it as soon as we get it so we won't need to keep it... Based on in person conversation, this won't be needed. I'm keeping it here for now, but adding a TODO to remove it. https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/common/pref_... File chrome/common/pref_names.cc (right): https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/common/pref_... chrome/common/pref_names.cc:1018: // String serialized form of variations seed protobuf On 2012/05/04 12:29:57, MAD wrote: > '.' at the end of the comment sentence please. Done.
LGTM with comments. https://chromiumcodereview.appspot.com/10343007/diff/22001/chrome/browser/met... File chrome/browser/metrics/variations_service.cc (right): https://chromiumcodereview.appspot.com/10343007/diff/22001/chrome/browser/met... chrome/browser/metrics/variations_service.cc:22: } Nit: Change to "} // namespace" https://chromiumcodereview.appspot.com/10343007/diff/22001/chrome/browser/met... chrome/browser/metrics/variations_service.cc:34: !variations_seed_.ParseFromString(seed_data)) { Nit: This should be aligned. https://chromiumcodereview.appspot.com/10343007/diff/22001/chrome/browser/met... chrome/browser/metrics/variations_service.cc:58: net::URLRequestStatus::SUCCESS || This can go on the previous line. https://chromiumcodereview.appspot.com/10343007/diff/22001/chrome/browser/met... File chrome/browser/metrics/variations_service.h (right): https://chromiumcodereview.appspot.com/10343007/diff/22001/chrome/browser/met... chrome/browser/metrics/variations_service.h:16: class VariationsService : public content::URLFetcherDelegate { Need a comment.
A few more... Getting closer... Well done... Thanks! BYE MAD... https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/chro... File chrome/browser/chrome_browser_main.cc (right): https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/chro... chrome/browser/chrome_browser_main.cc:1882: // Request new variations seed information from server. On 2012/05/04 12:29:57, MAD wrote: > If we keep this here, we need to keep it within the ChromeOS #ifdef because we > can't fetch a URL at this point in ChromeOS. Ping? https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/chro... File chrome/browser/chrome_browser_main.h (right): https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/chro... chrome/browser/chrome_browser_main.h:216: scoped_ptr<VariationsService> variations_service_; On 2012/05/03 22:27:49, Jesse Doherty wrote: > Still need to figure out a better place to put this. I thought we discussed offline that a singleton would work here. What made you change your mind? https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/metr... File chrome/browser/metrics/variations_service.cc (right): https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/metr... chrome/browser/metrics/variations_service.cc:65: } On 2012/05/03 22:27:49, Jesse Doherty wrote: > I'm not sure if this function should be responsible for |source| if the equality > wasn't correct. TranslationManager deals with it here > http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/translate/t.... > But I haven't yet found another example of it (haven't looked VERY thoroughly). You could add a DCHECK that you received the one you are expecting. There are other cases where they handle it that way... Others simply don't check and assume it's theirs... https://chromiumcodereview.appspot.com/10343007/diff/22001/chrome/browser/met... File chrome/browser/metrics/variations_service.cc (right): https://chromiumcodereview.appspot.com/10343007/diff/22001/chrome/browser/met... chrome/browser/metrics/variations_service.cc:19: // Default server of Variations seed info. We usually put a newline at the beginning and end of a namespace.
Adding rtenneti@ for OWNERS review. Tests for VariationsService will be put in a different CL based off a CL being put in by asvitkine@. https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/chro... File chrome/browser/chrome_browser_main.cc (right): https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/chro... chrome/browser/chrome_browser_main.cc:1882: // Request new variations seed information from server. On 2012/05/04 20:53:12, MAD wrote: > On 2012/05/04 12:29:57, MAD wrote: > > If we keep this here, we need to keep it within the ChromeOS #ifdef because we > > can't fetch a URL at this point in ChromeOS. > > Ping? Done. https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/chro... File chrome/browser/chrome_browser_main.h (right): https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/chro... chrome/browser/chrome_browser_main.h:216: scoped_ptr<VariationsService> variations_service_; On 2012/05/04 20:53:12, MAD wrote: > On 2012/05/03 22:27:49, Jesse Doherty wrote: > > Still need to figure out a better place to put this. > > I thought we discussed offline that a singleton would work here. What made you > change your mind? Maybe my comment here is from before the offline discussion. I am making it a singleton. https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/metr... File chrome/browser/metrics/variations_service.cc (right): https://chromiumcodereview.appspot.com/10343007/diff/8001/chrome/browser/metr... chrome/browser/metrics/variations_service.cc:65: } On 2012/05/04 20:53:12, MAD wrote: > On 2012/05/03 22:27:49, Jesse Doherty wrote: > > I'm not sure if this function should be responsible for |source| if the > equality > > wasn't correct. TranslationManager deals with it here > > > http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/translate/t.... > > But I haven't yet found another example of it (haven't looked VERY > thoroughly). > > You could add a DCHECK that you received the one you are expecting. There are > other cases where they handle it that way... Others simply don't check and > assume it's theirs... Done. https://chromiumcodereview.appspot.com/10343007/diff/22001/chrome/browser/met... File chrome/browser/metrics/variations_service.cc (right): https://chromiumcodereview.appspot.com/10343007/diff/22001/chrome/browser/met... chrome/browser/metrics/variations_service.cc:19: // Default server of Variations seed info. On 2012/05/04 20:53:12, MAD wrote: > We usually put a newline at the beginning and end of a namespace. Done. https://chromiumcodereview.appspot.com/10343007/diff/22001/chrome/browser/met... chrome/browser/metrics/variations_service.cc:22: } On 2012/05/04 19:39:33, Alexei Svitkine wrote: > Nit: Change to "} // namespace" Done. https://chromiumcodereview.appspot.com/10343007/diff/22001/chrome/browser/met... chrome/browser/metrics/variations_service.cc:34: !variations_seed_.ParseFromString(seed_data)) { On 2012/05/04 19:39:33, Alexei Svitkine wrote: > Nit: This should be aligned. Done. https://chromiumcodereview.appspot.com/10343007/diff/22001/chrome/browser/met... chrome/browser/metrics/variations_service.cc:58: net::URLRequestStatus::SUCCESS || On 2012/05/04 19:39:33, Alexei Svitkine wrote: > This can go on the previous line. Done. https://chromiumcodereview.appspot.com/10343007/diff/22001/chrome/browser/met... File chrome/browser/metrics/variations_service.h (right): https://chromiumcodereview.appspot.com/10343007/diff/22001/chrome/browser/met... chrome/browser/metrics/variations_service.h:16: class VariationsService : public content::URLFetcherDelegate { On 2012/05/04 19:39:33, Alexei Svitkine wrote: > Need a comment.
LGTM with one last request... Thanks! BYE MAD https://chromiumcodereview.appspot.com/10343007/diff/13024/chrome/browser/met... File chrome/browser/metrics/variations_service.h (right): https://chromiumcodereview.appspot.com/10343007/diff/13024/chrome/browser/met... chrome/browser/metrics/variations_service.h:25: Please privatize the destructor or too...
LGTM, given: 1. Please provide manual testing instructions. 2. A follow up CL later on with basic unit tests, if possible.
lgtm LGTM
https://chromiumcodereview.appspot.com/10343007/diff/13024/chrome/browser/met... File chrome/browser/metrics/variations_service.h (right): https://chromiumcodereview.appspot.com/10343007/diff/13024/chrome/browser/met... chrome/browser/metrics/variations_service.h:25: On 2012/05/04 23:09:07, MAD wrote: > Please privatize the destructor or too... Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jwd@chromium.org/10343007/23004
Change committed as 135654 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
