Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11612017:
[Autofill] Add protobuf for Google Wallet risk parameters and a utility function for filling it out. (Closed)
|
Created:
8 years ago by Ilya Sherman Modified:
7 years, 10 months ago CC:
chromium-reviews, Raman Kakilate, benquan, dhollowa+watch_chromium.org, ahutter, browser-components-watch_chromium.org, dbeam+watch-autofill_chromium.org, Dane Wallinga, dyu1, estade+watch_chromium.org, Albert Bodenhamer, Ilya Sherman Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Autofill] Add protobuf for Google Wallet risk parameters and a utility function for filling it out.
This data is used for fraud prevention when using Google Wallet.
BUG=166596
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=179568
Patch Set 1 #
Total comments: 21
Patch Set 2 : Clean it up! #
Total comments: 12
Patch Set 3 : Parallelized, with an ear toward profile destruction #Patch Set 4 : Van Gogh #Patch Set 5 : Clean up! #
Total comments: 8
Patch Set 6 : Callback + test #
Total comments: 19
Patch Set 7 : TimeDeltas #Patch Set 8 : Rebase #Patch Set 9 : Rebase #
Total comments: 4
Patch Set 10 : Make sure the code is NOTREACHED() for now #Patch Set 11 : No explicit NOTREACHED() for tests #Patch Set 12 : Fix test compile #
Messages
Total messages: 29 (0 generated)
This CL is not quite ready for review, but I'm uploading it now in case y'all would like to take a look. Sample output: http://pastebin.com/8ZntSX8D
Just a few high level comments. https://chromiumcodereview.appspot.com/11612017/diff/1/build/protoc.gypi File build/protoc.gypi (right): https://chromiumcodereview.appspot.com/11612017/diff/1/build/protoc.gypi#newc... build/protoc.gypi:104: '<(DEPTH)/third_party/protobuf/protobuf.gyp:protobuf_full_do_not_use', Test code? https://chromiumcodereview.appspot.com/11612017/diff/1/chrome/browser/autofil... File chrome/browser/autofill/risk/machine_fingerprint.cc (right): https://chromiumcodereview.appspot.com/11612017/diff/1/chrome/browser/autofil... chrome/browser/autofill/risk/machine_fingerprint.cc:38: int64 GetMillisecondsSinceEpoch(const base::Time& timestamp) { Should this live in a util module somewhere? https://chromiumcodereview.appspot.com/11612017/diff/1/chrome/browser/autofil... chrome/browser/autofill/risk/machine_fingerprint.cc:196: // TODO(isherman): REMOVEME Remove https://chromiumcodereview.appspot.com/11612017/diff/1/chrome/browser/autofil... chrome/browser/autofill/risk/machine_fingerprint.cc:218: scoped_ptr<BrowserNativeFingerprinting> data) { |fingerprint| ? https://chromiumcodereview.appspot.com/11612017/diff/1/chrome/browser/autofil... File chrome/browser/autofill/risk/machine_fingerprint.h (right): https://chromiumcodereview.appspot.com/11612017/diff/1/chrome/browser/autofil... chrome/browser/autofill/risk/machine_fingerprint.h:16: namespace risk { Is it worth doing a nested namespace here? https://chromiumcodereview.appspot.com/11612017/diff/1/chrome/browser/autofil... File chrome/browser/autofill/risk/proto/machine_fingerprint.proto (right): https://chromiumcodereview.appspot.com/11612017/diff/1/chrome/browser/autofil... chrome/browser/autofill/risk/proto/machine_fingerprint.proto:58: // TODO(isherman): This seems like TMI. Include pamartin@ in privacy Yeah, that wasn't included in the list of things we'd reviewed and they should be able to extract it in other ways. File a bug to resolve the TMI questions?
https://chromiumcodereview.appspot.com/11612017/diff/1/chrome/browser/autofil... File chrome/browser/autofill/risk/machine_fingerprint.cc (right): https://chromiumcodereview.appspot.com/11612017/diff/1/chrome/browser/autofil... chrome/browser/autofill/risk/machine_fingerprint.cc:38: int64 GetMillisecondsSinceEpoch(const base::Time& timestamp) { On 2012/12/18 17:42:22, Albert Bodenhamer wrote: > Should this live in a util module somewhere? yea, this seems similar to ToJsTime https://chromiumcodereview.appspot.com/11612017/diff/1/chrome/browser/autofil... chrome/browser/autofill/risk/machine_fingerprint.cc:89: for (base::ListValue::const_iterator it = fonts->begin(); dunno if you're looking for this level of review yet, but it would be nice to break each thing out into utility functions
This is now pretty much ready for a full review, though there are still two outstanding issues: (1) privacy questions, and (2) Profile* lifetime management. For (1), for now I simply haven't implemented the privacy-sensitive fields. For (2), I'm a little stumped: Some of the data that we want to upload is stored in an individual Profile's preferences file. I'm currently using the ProfileManager to figure out which profile was last used, and pulling the preferences from that. However, because the data is loaded asynchronously, the last used profile by the time we get to the function that actually writes out the data might be the wrong profile. OTOH, if I try to get a pointer to the correct profile earlier, I don't think there's any guarantee that this profile will still be available by the time the asynchronous data is loaded. Any ideas? Please ignore for the moment the change to the .gypi file and the DLOG line marked with REMOVEME -- those are handy for testing, but I'll remove them prior to committing, once question (2) is either resolved or punted. https://chromiumcodereview.appspot.com/11612017/diff/1/build/protoc.gypi File build/protoc.gypi (right): https://chromiumcodereview.appspot.com/11612017/diff/1/build/protoc.gypi#newc... build/protoc.gypi:104: '<(DEPTH)/third_party/protobuf/protobuf.gyp:protobuf_full_do_not_use', On 2012/12/18 17:42:22, Albert Bodenhamer wrote: > Test code? Yep, this enables the DebugString() method that allows me to dump a human-readable version of the filled out protocol buffer for testing. I'm not planning to commit this :) https://chromiumcodereview.appspot.com/11612017/diff/1/chrome/browser/autofil... File chrome/browser/autofill/risk/machine_fingerprint.cc (right): https://chromiumcodereview.appspot.com/11612017/diff/1/chrome/browser/autofil... chrome/browser/autofill/risk/machine_fingerprint.cc:38: int64 GetMillisecondsSinceEpoch(const base::Time& timestamp) { On 2012/12/18 22:14:34, Evan Stade wrote: > On 2012/12/18 17:42:22, Albert Bodenhamer wrote: > > Should this live in a util module somewhere? > > yea, this seems similar to ToJsTime I'll check with Brett whether he's ok with moving this into base/time.h... https://chromiumcodereview.appspot.com/11612017/diff/1/chrome/browser/autofil... chrome/browser/autofill/risk/machine_fingerprint.cc:89: for (base::ListValue::const_iterator it = fonts->begin(); On 2012/12/18 22:14:34, Evan Stade wrote: > dunno if you're looking for this level of review yet, but it would be nice to > break each thing out into utility functions Done. https://chromiumcodereview.appspot.com/11612017/diff/1/chrome/browser/autofil... chrome/browser/autofill/risk/machine_fingerprint.cc:196: // TODO(isherman): REMOVEME On 2012/12/18 17:42:22, Albert Bodenhamer wrote: > Remove This is useful for debugging; I'll remove once the code is properly ready. https://chromiumcodereview.appspot.com/11612017/diff/1/chrome/browser/autofil... chrome/browser/autofill/risk/machine_fingerprint.cc:218: scoped_ptr<BrowserNativeFingerprinting> data) { On 2012/12/18 17:42:22, Albert Bodenhamer wrote: > |fingerprint| ? Unfortunately, fingerprint is one of the fields within this data structure, so it doesn't work very well as the name for the structure itself... https://chromiumcodereview.appspot.com/11612017/diff/1/chrome/browser/autofil... File chrome/browser/autofill/risk/machine_fingerprint.h (right): https://chromiumcodereview.appspot.com/11612017/diff/1/chrome/browser/autofil... chrome/browser/autofill/risk/machine_fingerprint.h:16: namespace risk { On 2012/12/18 17:42:22, Albert Bodenhamer wrote: > Is it worth doing a nested namespace here? I think this is tidier than including "Risk" in the function name. Since Autofill code should already be in the autofill namespace (note: not all of it currently is, but it *should* be), invocations of the method will look like "risk::GetMachineFingerprint(...)", which IMO is clear and easier to read than "GetMachineFingerprintForRisk(...)" or something like that. https://chromiumcodereview.appspot.com/11612017/diff/1/chrome/browser/autofil... File chrome/browser/autofill/risk/proto/machine_fingerprint.proto (right): https://chromiumcodereview.appspot.com/11612017/diff/1/chrome/browser/autofil... chrome/browser/autofill/risk/proto/machine_fingerprint.proto:58: // TODO(isherman): This seems like TMI. Include pamartin@ in privacy On 2012/12/18 17:42:22, Albert Bodenhamer wrote: > Yeah, that wasn't included in the list of things we'd reviewed and they should > be able to extract it in other ways. > > File a bug to resolve the TMI questions? Sent an email to the privacy team; let's see what they say.
https://chromiumcodereview.appspot.com/11612017/diff/1/chrome/browser/autofil... File chrome/browser/autofill/risk/machine_fingerprint.cc (right): https://chromiumcodereview.appspot.com/11612017/diff/1/chrome/browser/autofil... chrome/browser/autofill/risk/machine_fingerprint.cc:218: scoped_ptr<BrowserNativeFingerprinting> data) { On 2012/12/19 01:50:08, Ilya Sherman wrote: > On 2012/12/18 17:42:22, Albert Bodenhamer wrote: > > |fingerprint| ? > > Unfortunately, fingerprint is one of the fields within this data structure, so > it doesn't work very well as the name for the structure itself... imo give the field within the data structure a better name (I suggested a couple) and call this fingerprint. https://chromiumcodereview.appspot.com/11612017/diff/8001/chrome/browser/auto... File chrome/browser/autofill/risk/machine_fingerprint.cc (right): https://chromiumcodereview.appspot.com/11612017/diff/8001/chrome/browser/auto... chrome/browser/autofill/risk/machine_fingerprint.cc:184: // be dead by the time I look at it. I think you'd need to listen for PROFILE_DESTROYED notifications. https://chromiumcodereview.appspot.com/11612017/diff/8001/chrome/browser/auto... chrome/browser/autofill/risk/machine_fingerprint.cc:242: content::PluginService::GetInstance()->GetPlugins( it seems like you could create one class that does all these async load steps in parallel rather than making them occur in sequence. Then when all have completed, call GetMachineFingerprintImpl with all the collected data. https://chromiumcodereview.appspot.com/11612017/diff/8001/chrome/browser/auto... chrome/browser/autofill/risk/machine_fingerprint.cc:271: // Data that will be passed on to the next loading phase. it would make more sense to me if you passed this data in via a callback. i.e. instead of calling GetFontListAsync on a bunch of member vars, GpuDataLoader would just do next_step_.Run(); but that's a moot point if you accept the suggestion in the above comment. https://chromiumcodereview.appspot.com/11612017/diff/8001/chrome/browser/auto... File chrome/browser/autofill/risk/proto/machine_fingerprint.proto (right): https://chromiumcodereview.appspot.com/11612017/diff/8001/chrome/browser/auto... chrome/browser/autofill/risk/proto/machine_fingerprint.proto:23: message Fingerprint { maybe this should be called something more specific, like MachineCharacteristics or DurableFingerprint? https://chromiumcodereview.appspot.com/11612017/diff/8001/chrome/browser/auto... chrome/browser/autofill/risk/proto/machine_fingerprint.proto:155: message UserBehavior { docs call it UserCharacteristics?
https://chromiumcodereview.appspot.com/11612017/diff/1/chrome/browser/autofil... File chrome/browser/autofill/risk/machine_fingerprint.cc (right): https://chromiumcodereview.appspot.com/11612017/diff/1/chrome/browser/autofil... chrome/browser/autofill/risk/machine_fingerprint.cc:38: int64 GetMillisecondsSinceEpoch(const base::Time& timestamp) { I think the code you have here is short and quite easy to understand, so I don't think we need a global function for this.
On Tue, Dec 18, 2012 at 7:13 PM, <estade@chromium.org> wrote: > > https://chromiumcodereview.**appspot.com/11612017/diff/1/** > chrome/browser/autofill/risk/**machine_fingerprint.cc<https://chromiumcodereview.appspot.com/11612017/diff/1/chrome/browser/autofill/risk/machine_fingerprint.cc> > File chrome/browser/autofill/risk/**machine_fingerprint.cc (right): > > https://chromiumcodereview.**appspot.com/11612017/diff/1/** > chrome/browser/autofill/risk/**machine_fingerprint.cc#**newcode218<https://chromiumcodereview.appspot.com/11612017/diff/1/chrome/browser/autofill/risk/machine_fingerprint.cc#newcode218> > chrome/browser/autofill/risk/**machine_fingerprint.cc:218: > scoped_ptr<**BrowserNativeFingerprinting> data) { > On 2012/12/19 01:50:08, Ilya Sherman wrote: > >> On 2012/12/18 17:42:22, Albert Bodenhamer wrote: >> > |fingerprint| ? >> > > Unfortunately, fingerprint is one of the fields within this data >> > structure, so > >> it doesn't work very well as the name for the structure itself... >> > > imo give the field within the data structure a better name (I suggested > a couple) and call this fingerprint. > +1 It seems weird to have a GetMachineFingerprint function that returns an item called "data" that includes a bunch of data info PLUS a finger print. Either the top level is wrong (it's something more than a fingerprint?) or the lower level is wrong. My guess is the lower level. > > https://chromiumcodereview.**appspot.com/11612017/diff/** > 8001/chrome/browser/autofill/**risk/machine_fingerprint.cc<https://chromiumcodereview.appspot.com/11612017/diff/8001/chrome/browser/autofill/risk/machine_fingerprint.cc> > File chrome/browser/autofill/risk/**machine_fingerprint.cc (right): > > https://chromiumcodereview.**appspot.com/11612017/diff/** > 8001/chrome/browser/autofill/**risk/machine_fingerprint.cc#**newcode184<https://chromiumcodereview.appspot.com/11612017/diff/8001/chrome/browser/autofill/risk/machine_fingerprint.cc#newcode184> > chrome/browser/autofill/risk/**machine_fingerprint.cc:184: // be dead by > the time I look at it. > I think you'd need to listen for PROFILE_DESTROYED notifications. > > https://chromiumcodereview.**appspot.com/11612017/diff/** > 8001/chrome/browser/autofill/**risk/machine_fingerprint.cc#**newcode242<https://chromiumcodereview.appspot.com/11612017/diff/8001/chrome/browser/autofill/risk/machine_fingerprint.cc#newcode242> > chrome/browser/autofill/risk/**machine_fingerprint.cc:242: > content::PluginService::**GetInstance()->GetPlugins( > it seems like you could create one class that does all these async load > steps in parallel rather than making them occur in sequence. Then when > all have completed, call GetMachineFingerprintImpl with all the > collected data. > This might be worth considering, but I worry about premature optimization. Any idea what the latency is likely to be? > > https://chromiumcodereview.**appspot.com/11612017/diff/** > 8001/chrome/browser/autofill/**risk/machine_fingerprint.cc#**newcode271<https://chromiumcodereview.appspot.com/11612017/diff/8001/chrome/browser/autofill/risk/machine_fingerprint.cc#newcode271> > chrome/browser/autofill/risk/**machine_fingerprint.cc:271: // Data that > will be passed on to the next loading phase. > it would make more sense to me if you passed this data in via a > callback. i.e. instead of calling GetFontListAsync on a bunch of member > vars, GpuDataLoader would just do next_step_.Run(); > > but that's a moot point if you accept the suggestion in the above > comment. > > https://chromiumcodereview.**appspot.com/11612017/diff/** > 8001/chrome/browser/autofill/**risk/proto/machine_**fingerprint.proto<https://chromiumcodereview.appspot.com/11612017/diff/8001/chrome/browser/autofill/risk/proto/machine_fingerprint.proto> > File chrome/browser/autofill/risk/**proto/machine_fingerprint.**proto > (right): > > https://chromiumcodereview.**appspot.com/11612017/diff/** > 8001/chrome/browser/autofill/**risk/proto/machine_** > fingerprint.proto#newcode23<https://chromiumcodereview.appspot.com/11612017/diff/8001/chrome/browser/autofill/risk/proto/machine_fingerprint.proto#newcode23> > chrome/browser/autofill/risk/**proto/machine_fingerprint.**proto:23: > message > Fingerprint { > maybe this should be called something more specific, like > MachineCharacteristics or DurableFingerprint? > > https://chromiumcodereview.**appspot.com/11612017/diff/** > 8001/chrome/browser/autofill/**risk/proto/machine_** > fingerprint.proto#newcode155<https://chromiumcodereview.appspot.com/11612017/diff/8001/chrome/browser/autofill/risk/proto/machine_fingerprint.proto#newcode155> > chrome/browser/autofill/risk/**proto/machine_fingerprint.**proto:155: > message UserBehavior { > docs > > call it UserCharacteristics? > > https://chromiumcodereview.**appspot.com/11612017/<https://chromiumcodereview... > -- Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> org
https://chromiumcodereview.appspot.com/11612017/diff/1/chrome/browser/autofil... File chrome/browser/autofill/risk/machine_fingerprint.h (right): https://chromiumcodereview.appspot.com/11612017/diff/1/chrome/browser/autofil... chrome/browser/autofill/risk/machine_fingerprint.h:16: namespace risk { On 2012/12/19 01:50:08, Ilya Sherman wrote: > On 2012/12/18 17:42:22, Albert Bodenhamer wrote: > > Is it worth doing a nested namespace here? > > I think this is tidier than including "Risk" in the function name. Since > Autofill code should already be in the autofill namespace (note: not all of it > currently is, but it *should* be), invocations of the method will look like > "risk::GetMachineFingerprint(...)", which IMO is clear and easier to read than > "GetMachineFingerprintForRisk(...)" or something like that. Ok. https://chromiumcodereview.appspot.com/11612017/diff/1/chrome/browser/autofil... chrome/browser/autofill/risk/machine_fingerprint.h:22: // for Google's authentication system. Since this is specific to Risk, it's probably worth mentioning in a comment that this data is intended for use in fraud reduction.
https://chromiumcodereview.appspot.com/11612017/diff/8001/chrome/browser/auto... File chrome/browser/autofill/risk/machine_fingerprint.h (right): https://chromiumcodereview.appspot.com/11612017/diff/8001/chrome/browser/auto... chrome/browser/autofill/risk/machine_fingerprint.h:23: void GetMachineFingerprint(int64 gaia_id, Should we consider adding some sort of protection to require the TOS to be accepted before this can be called? It could act as a safety net to prevent a caller from accidentally sending this data incorrectly.
On 2012/12/19 22:02:38, Albert Bodenhamer wrote: > > chrome/browser/autofill/risk/**machine_fingerprint.cc:242: > > content::PluginService::**GetInstance()->GetPlugins( > > it seems like you could create one class that does all these async load > > steps in parallel rather than making them occur in sequence. Then when > > all have completed, call GetMachineFingerprintImpl with all the > > collected data. > > > > This might be worth considering, but I worry about premature optimization. > Any idea what the latency is likely to be? The latency is low. I haven't measured it precisely, but it feels more or less instantaneous. That said, fetching everything in parallel is simple enough, and simplifies the code in some ways, so I've gone ahead and made it so. https://chromiumcodereview.appspot.com/11612017/diff/1/chrome/browser/autofil... File chrome/browser/autofill/risk/machine_fingerprint.cc (right): https://chromiumcodereview.appspot.com/11612017/diff/1/chrome/browser/autofil... chrome/browser/autofill/risk/machine_fingerprint.cc:218: scoped_ptr<BrowserNativeFingerprinting> data) { On 2012/12/19 03:13:27, Evan Stade wrote: > On 2012/12/19 01:50:08, Ilya Sherman wrote: > > On 2012/12/18 17:42:22, Albert Bodenhamer wrote: > > > |fingerprint| ? > > > > Unfortunately, fingerprint is one of the fields within this data structure, so > > it doesn't work very well as the name for the structure itself... > > imo give the field within the data structure a better name (I suggested a > couple) and call this fingerprint. Done. https://chromiumcodereview.appspot.com/11612017/diff/1/chrome/browser/autofil... File chrome/browser/autofill/risk/machine_fingerprint.h (right): https://chromiumcodereview.appspot.com/11612017/diff/1/chrome/browser/autofil... chrome/browser/autofill/risk/machine_fingerprint.h:22: // for Google's authentication system. On 2012/12/19 22:10:17, Albert Bodenhamer wrote: > Since this is specific to Risk, it's probably worth mentioning in a comment that > this data is intended for use in fraud reduction. Done. https://chromiumcodereview.appspot.com/11612017/diff/8001/chrome/browser/auto... File chrome/browser/autofill/risk/machine_fingerprint.cc (right): https://chromiumcodereview.appspot.com/11612017/diff/8001/chrome/browser/auto... chrome/browser/autofill/risk/machine_fingerprint.cc:184: // be dead by the time I look at it. On 2012/12/19 03:13:27, Evan Stade wrote: > I think you'd need to listen for PROFILE_DESTROYED notifications. Done. https://chromiumcodereview.appspot.com/11612017/diff/8001/chrome/browser/auto... chrome/browser/autofill/risk/machine_fingerprint.cc:242: content::PluginService::GetInstance()->GetPlugins( On 2012/12/19 03:13:27, Evan Stade wrote: > it seems like you could create one class that does all these async load steps in > parallel rather than making them occur in sequence. Then when all have > completed, call GetMachineFingerprintImpl with all the collected data. Done. https://chromiumcodereview.appspot.com/11612017/diff/8001/chrome/browser/auto... chrome/browser/autofill/risk/machine_fingerprint.cc:271: // Data that will be passed on to the next loading phase. On 2012/12/19 03:13:27, Evan Stade wrote: > it would make more sense to me if you passed this data in via a callback. i.e. > instead of calling GetFontListAsync on a bunch of member vars, GpuDataLoader > would just do next_step_.Run(); > > but that's a moot point if you accept the suggestion in the above comment. moot. https://chromiumcodereview.appspot.com/11612017/diff/8001/chrome/browser/auto... File chrome/browser/autofill/risk/machine_fingerprint.h (right): https://chromiumcodereview.appspot.com/11612017/diff/8001/chrome/browser/auto... chrome/browser/autofill/risk/machine_fingerprint.h:23: void GetMachineFingerprint(int64 gaia_id, On 2012/12/19 22:14:18, Albert Bodenhamer wrote: > Should we consider adding some sort of protection to require the TOS to be > accepted before this can be called? It could act as a safety net to prevent a > caller from accidentally sending this data incorrectly. I'm not aware of any way to check that currently; but I've added a TODO. https://chromiumcodereview.appspot.com/11612017/diff/8001/chrome/browser/auto... File chrome/browser/autofill/risk/proto/machine_fingerprint.proto (right): https://chromiumcodereview.appspot.com/11612017/diff/8001/chrome/browser/auto... chrome/browser/autofill/risk/proto/machine_fingerprint.proto:23: message Fingerprint { On 2012/12/19 03:13:27, Evan Stade wrote: > maybe this should be called something more specific, like MachineCharacteristics > or DurableFingerprint? Done. https://chromiumcodereview.appspot.com/11612017/diff/8001/chrome/browser/auto... chrome/browser/autofill/risk/proto/machine_fingerprint.proto:155: message UserBehavior { On 2012/12/19 03:13:27, Evan Stade wrote: > docs > > call it UserCharacteristics? Done.
lgtm
https://chromiumcodereview.appspot.com/11612017/diff/18001/chrome/browser/aut... File chrome/browser/autofill/risk/proto/fingerprint.proto (right): https://chromiumcodereview.appspot.com/11612017/diff/18001/chrome/browser/aut... chrome/browser/autofill/risk/proto/fingerprint.proto:19: required int32 width = 1; Is this proto going to also live in google3? If so, you shouldn't use required anywhere.
looking good. https://codereview.chromium.org/11612017/diff/18001/chrome/browser/autofill/r... File chrome/browser/autofill/risk/fingerprint.cc (right): https://codereview.chromium.org/11612017/diff/18001/chrome/browser/autofill/r... chrome/browser/autofill/risk/fingerprint.cc:193: void MaybeFillFingerprint(); nit: \n https://codereview.chromium.org/11612017/diff/18001/chrome/browser/autofill/r... chrome/browser/autofill/risk/fingerprint.cc:333: nit: one less newline https://codereview.chromium.org/11612017/diff/18001/chrome/browser/autofill/r... File chrome/browser/autofill/risk/fingerprint.h (right): https://codereview.chromium.org/11612017/diff/18001/chrome/browser/autofill/r... chrome/browser/autofill/risk/fingerprint.h:23: // fingerprint for this (machine, user) pair, used for fraud prevention. This makes it sound like |fingerprint| is an out param, but outparams should not be owned by the function. Also since it seems that ownership is transferred, I'm confused how the caller ever gets the data back.
Added a basic test. https://chromiumcodereview.appspot.com/11612017/diff/18001/chrome/browser/aut... File chrome/browser/autofill/risk/fingerprint.cc (right): https://chromiumcodereview.appspot.com/11612017/diff/18001/chrome/browser/aut... chrome/browser/autofill/risk/fingerprint.cc:193: void MaybeFillFingerprint(); On 2012/12/20 21:33:27, Evan Stade wrote: > nit: \n Done. https://chromiumcodereview.appspot.com/11612017/diff/18001/chrome/browser/aut... chrome/browser/autofill/risk/fingerprint.cc:333: On 2012/12/20 21:33:27, Evan Stade wrote: > nit: one less newline Done. https://chromiumcodereview.appspot.com/11612017/diff/18001/chrome/browser/aut... File chrome/browser/autofill/risk/fingerprint.h (right): https://chromiumcodereview.appspot.com/11612017/diff/18001/chrome/browser/aut... chrome/browser/autofill/risk/fingerprint.h:23: // fingerprint for this (machine, user) pair, used for fraud prevention. On 2012/12/20 21:33:27, Evan Stade wrote: > This makes it sound like |fingerprint| is an out param, but outparams should not > be owned by the function. Also since it seems that ownership is transferred, I'm > confused how the caller ever gets the data back. Yeah, I was kind of punting on the callback issue... implemented now. https://chromiumcodereview.appspot.com/11612017/diff/18001/chrome/browser/aut... File chrome/browser/autofill/risk/proto/fingerprint.proto (right): https://chromiumcodereview.appspot.com/11612017/diff/18001/chrome/browser/aut... chrome/browser/autofill/risk/proto/fingerprint.proto:19: required int32 width = 1; On 2012/12/20 18:35:12, ahutter wrote: > Is this proto going to also live in google3? If so, you shouldn't use required > anywhere. Done.
https://chromiumcodereview.appspot.com/11612017/diff/21001/chrome/browser/aut... File chrome/browser/autofill/risk/fingerprint.cc (right): https://chromiumcodereview.appspot.com/11612017/diff/21001/chrome/browser/aut... chrome/browser/autofill/risk/fingerprint.cc:40: int64 GetMillisecondsSinceEpoch(const base::Time& timestamp) { I think it would be preferable to have these time functions return TimeDelta, and do the conversion to milliseconds at the call site. Also, I question how useful this particular function is, it seems you could just inline the implementation wherever it's called. But I won't force you to remove it. https://chromiumcodereview.appspot.com/11612017/diff/21001/chrome/browser/aut... chrome/browser/autofill/risk/fingerprint.cc:68: base::Time::FromUTCExploded(local) - base::Time::FromUTCExploded(utc); can't you just use |now| for base::Time::FromUTCExploded(utc) https://chromiumcodereview.appspot.com/11612017/diff/21001/chrome/browser/aut... chrome/browser/autofill/risk/fingerprint.cc:75: return nit: looks slightly weird to me, consider: return [...] + [...]; https://chromiumcodereview.appspot.com/11612017/diff/21001/chrome/browser/aut... chrome/browser/autofill/risk/fingerprint.cc:217: base::Callback<void(scoped_ptr<Fingerprint>)> callback_; I think it's worth creating a typedef for this callback type. https://chromiumcodereview.appspot.com/11612017/diff/21001/chrome/browser/aut... chrome/browser/autofill/risk/fingerprint.cc:304: AddFontsToFingerprint(*fonts_.get(), machine); you don't need the .get() part of *fonts_.get() https://chromiumcodereview.appspot.com/11612017/diff/21001/chrome/browser/aut... File chrome/browser/autofill/risk/fingerprint.h (right): https://chromiumcodereview.appspot.com/11612017/diff/21001/chrome/browser/aut... chrome/browser/autofill/risk/fingerprint.h:39: one less \n https://chromiumcodereview.appspot.com/11612017/diff/21001/chrome/browser/aut... File chrome/browser/autofill/risk/fingerprint_browsertest.cc (right): https://chromiumcodereview.appspot.com/11612017/diff/21001/chrome/browser/aut... chrome/browser/autofill/risk/fingerprint_browsertest.cc:48: ASSERT_TRUE(machine.has_screen_count()); does this mean the test will fail on a headless machine? Is it normal that a browser test would fail if no screen is attached? https://chromiumcodereview.appspot.com/11612017/diff/21001/chrome/browser/aut... chrome/browser/autofill/risk/fingerprint_browsertest.cc:53: ASSERT_TRUE(machine.has_cpu()); do all of these need to be ASSERT instead of EXPECT?
https://chromiumcodereview.appspot.com/11612017/diff/21001/chrome/browser/aut... File chrome/browser/autofill/risk/fingerprint.cc (right): https://chromiumcodereview.appspot.com/11612017/diff/21001/chrome/browser/aut... chrome/browser/autofill/risk/fingerprint.cc:40: int64 GetMillisecondsSinceEpoch(const base::Time& timestamp) { On 2012/12/21 22:37:08, Evan Stade wrote: > I think it would be preferable to have these time functions return TimeDelta, > and do the conversion to milliseconds at the call site. > > Also, I question how useful this particular function is, it seems you could just > inline the implementation wherever it's called. But I won't force you to remove > it. I think the code was slightly cleaner with this helper function in place, but done. https://chromiumcodereview.appspot.com/11612017/diff/21001/chrome/browser/aut... chrome/browser/autofill/risk/fingerprint.cc:68: base::Time::FromUTCExploded(local) - base::Time::FromUTCExploded(utc); On 2012/12/21 22:37:08, Evan Stade wrote: > can't you just use |now| for base::Time::FromUTCExploded(utc) Done. https://chromiumcodereview.appspot.com/11612017/diff/21001/chrome/browser/aut... chrome/browser/autofill/risk/fingerprint.cc:75: return On 2012/12/21 22:37:08, Evan Stade wrote: > nit: looks slightly weird to me, consider: > > return [...] + > [...]; Done. https://chromiumcodereview.appspot.com/11612017/diff/21001/chrome/browser/aut... chrome/browser/autofill/risk/fingerprint.cc:217: base::Callback<void(scoped_ptr<Fingerprint>)> callback_; On 2012/12/21 22:37:08, Evan Stade wrote: > I think it's worth creating a typedef for this callback type. Why? I'm not fond of typedefs, and I think this type is not really that hard to read vs. other templated types. If we add a typedef, what specifically do you suggest? Should I add it to the header file, or just locally to the implementation file? What should I name it? https://chromiumcodereview.appspot.com/11612017/diff/21001/chrome/browser/aut... chrome/browser/autofill/risk/fingerprint.cc:304: AddFontsToFingerprint(*fonts_.get(), machine); On 2012/12/21 22:37:08, Evan Stade wrote: > you don't need the .get() part of *fonts_.get() Done. https://chromiumcodereview.appspot.com/11612017/diff/21001/chrome/browser/aut... File chrome/browser/autofill/risk/fingerprint.h (right): https://chromiumcodereview.appspot.com/11612017/diff/21001/chrome/browser/aut... chrome/browser/autofill/risk/fingerprint.h:39: On 2012/12/21 22:37:08, Evan Stade wrote: > one less \n Done. https://chromiumcodereview.appspot.com/11612017/diff/21001/chrome/browser/aut... File chrome/browser/autofill/risk/fingerprint_browsertest.cc (right): https://chromiumcodereview.appspot.com/11612017/diff/21001/chrome/browser/aut... chrome/browser/autofill/risk/fingerprint_browsertest.cc:48: ASSERT_TRUE(machine.has_screen_count()); On 2012/12/21 22:37:08, Evan Stade wrote: > does this mean the test will fail on a headless machine? Is it normal that a > browser test would fail if no screen is attached? The test would still pass on a headless machine. There's a difference between explicitly setting the default value (which is 0) for an optional field vs. leaving the field entirely unset. This checks that the value has been set, but not what it has been set to. https://chromiumcodereview.appspot.com/11612017/diff/21001/chrome/browser/aut... chrome/browser/autofill/risk/fingerprint_browsertest.cc:53: ASSERT_TRUE(machine.has_cpu()); On 2012/12/21 22:37:08, Evan Stade wrote: > do all of these need to be ASSERT instead of EXPECT? Some of them need to be ASSERTs for the sake of subsequent checks, e.g. EXPECT_EQ(kCharset, machine.charset()); Given that some have to be ASSERTs for that reason, I'd rather have them be uniform than having to reason about each of them whether it could be an EXPECT -- especially since that would then potentially need to be updated when the test is updated. As a side note, I've never identified a good reason for using EXPECTs when ASSERTs will also compile. Tests either pass or fail; I don't see much value in running extra checks within a single test if it's already failed. But since the common Chrome coding practice seems to be to prefer EXPECT checks, I try to use them where they're obviously sufficient.
lgtm https://chromiumcodereview.appspot.com/11612017/diff/21001/chrome/browser/aut... File chrome/browser/autofill/risk/fingerprint.cc (right): https://chromiumcodereview.appspot.com/11612017/diff/21001/chrome/browser/aut... chrome/browser/autofill/risk/fingerprint.cc:217: base::Callback<void(scoped_ptr<Fingerprint>)> callback_; On 2012/12/22 01:22:00, Ilya Sherman wrote: > On 2012/12/21 22:37:08, Evan Stade wrote: > > I think it's worth creating a typedef for this callback type. > > Why? I'm not fond of typedefs, and I think this type is not really that hard to > read vs. other templated types. > > If we add a typedef, what specifically do you suggest? Should I add it to the > header file, or just locally to the implementation file? What should I name it? I would put this in the header file: typedef base::Callback<void(scoped_ptr<Fingerprint>)> GotFingerprintCallback; the advantages: 1) it implies something about the purpose of the callback (makes it easier to understand) 2) it's shorter and contains fewer crazy characters (makes it easier to read) 3) easier to grep for but if you hate the idea you don't need to do it. https://chromiumcodereview.appspot.com/11612017/diff/21001/chrome/browser/aut... File chrome/browser/autofill/risk/fingerprint_browsertest.cc (right): https://chromiumcodereview.appspot.com/11612017/diff/21001/chrome/browser/aut... chrome/browser/autofill/risk/fingerprint_browsertest.cc:53: ASSERT_TRUE(machine.has_cpu()); > As a side note, I've never identified a good reason for using EXPECTs when > ASSERTs will also compile. Tests either pass or fail; I don't see much value in > running extra checks within a single test if it's already failed. But since the > common Chrome coding practice seems to be to prefer EXPECT checks, I try to use > them where they're obviously sufficient. EXPECT cuts down on the number of fix-compile-run cycles you need to do when you make a change that breaks a test. This is especially important when the compile-run cycle takes 2 hours because it's a trybot on a configuration you don't have local access to. As you've noted, it's not the end of the world if you have an ASSERT instead of an EXPECT, but the reverse is also true: it's not that bad to EXPECT instead of ASSERT, because a crashing test is also a failing test. You already separated some of the checks into their own blocks based on check relatedness. You could do this in more places (e.g. the cpu checks, graphics card checks, etc.) and each block will start with an ASSERT followed by many EXPECTs. I think this improves readability.
https://chromiumcodereview.appspot.com/11612017/diff/21001/chrome/browser/aut... File chrome/browser/autofill/risk/fingerprint_browsertest.cc (right): https://chromiumcodereview.appspot.com/11612017/diff/21001/chrome/browser/aut... chrome/browser/autofill/risk/fingerprint_browsertest.cc:53: ASSERT_TRUE(machine.has_cpu()); On 2012/12/22 02:17:34, Evan Stade wrote: > > As a side note, I've never identified a good reason for using EXPECTs when > > ASSERTs will also compile. Tests either pass or fail; I don't see much value > in > > running extra checks within a single test if it's already failed. But since > the > > common Chrome coding practice seems to be to prefer EXPECT checks, I try to > use > > them where they're obviously sufficient. > > EXPECT cuts down on the number of fix-compile-run cycles you need to do when you > make a change that breaks a test. This is especially important when the > compile-run cycle takes 2 hours because it's a trybot on a configuration you > don't have local access to. Fair enough, though for this particular test's ASSERTs, there shouldn't be any behavioral differences that depend on the build configuration. (There are also git cl try command flags that you can pass to reduce the cycle time if you're debugging a specific test, but that's somewhat tangential.) > As you've noted, it's not the end of the world if > you have an ASSERT instead of an EXPECT, but the reverse is also true: it's not > that bad to EXPECT instead of ASSERT, because a crashing test is also a failing > test. > > You already separated some of the checks into their own blocks based on check > relatedness. You could do this in more places (e.g. the cpu checks, graphics > card checks, etc.) and each block will start with an ASSERT followed by many > EXPECTs. I think this improves readability. The test is currently split into two parts: (1) checking that the expected fields are set in the protocol buffer, and (2) checking that some mocked values were written correctly. I tried interleaving these, which is what you're suggesting, and found it both less readable -- because the test was bouncing back and forth between different types of checks -- and less maintainable -- because you'd have to remember to change specific EXPECTs to ASSERTs as more values were mocked out.
+joi, scottmg: https://chromiumcodereview.appspot.com/11612017/diff/36002/chrome/browser/aut... File chrome/browser/autofill/DEPS (right): https://chromiumcodereview.appspot.com/11612017/diff/36002/chrome/browser/aut... chrome/browser/autofill/DEPS:50: ], Joi, is there currently a way to access the LocalStore prefs without going through g_browser_process? https://chromiumcodereview.appspot.com/11612017/diff/36002/chrome/browser/aut... File chrome/browser/autofill/risk/fingerprint.cc (right): https://chromiumcodereview.appspot.com/11612017/diff/36002/chrome/browser/aut... chrome/browser/autofill/risk/fingerprint.cc:122: // TODO(scottmg): NativeScreen maybe wrong. http://crbug.com/133312 Scott, I copied this TODO from metrics_log.cc. Is it still relevant? I tried reading through the linked bug, but couldn't tell how it applied to this code.
https://chromiumcodereview.appspot.com/11612017/diff/36002/chrome/browser/aut... File chrome/browser/autofill/DEPS (right): https://chromiumcodereview.appspot.com/11612017/diff/36002/chrome/browser/aut... chrome/browser/autofill/DEPS:50: ], On 2012/12/27 00:18:12, Ilya Sherman wrote: > Joi, is there currently a way to access the LocalStore prefs without going > through g_browser_process? You could add "GetLocalState" as a method to AutofillManagerDelegate and have the embedder (Chrome) pass a pointer to it via that method. OTOH, since we are not currently actively working on Autofill componentization*, feel free to add the concrete dependency if you need. * Our priorities shifted a bit and we're focusing more on componentizing "leaf nodes" in the dependency graph, so e.g. before finishing the componentization of Autofill we would componentize Prefs (in progress), WebData, and a few others.
https://chromiumcodereview.appspot.com/11612017/diff/36002/chrome/browser/aut... File chrome/browser/autofill/DEPS (right): https://chromiumcodereview.appspot.com/11612017/diff/36002/chrome/browser/aut... chrome/browser/autofill/DEPS:50: ], On 2012/12/27 09:54:28, Jói wrote: > On 2012/12/27 00:18:12, Ilya Sherman wrote: > > Joi, is there currently a way to access the LocalStore prefs without going > > through g_browser_process? > > You could add "GetLocalState" as a method to AutofillManagerDelegate and have > the embedder (Chrome) pass a pointer to it via that method. > > OTOH, since we are not currently actively working on Autofill componentization*, > feel free to add the concrete dependency if you need. Ok, if you don't mind, I'll just leave this TODO in place for now. Thanks :)
Nico, could you stamp for chrome/ OWNERS approval?
Nico, could you stamp for chrome/ OWNERS approval?
gypi lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/isherman@chromium.org/11612017/43001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/isherman@chromium.org/11612017/59001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_aura. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/isherman@chromium.org/11612017/67002
Message was sent while issue was closed.
Change committed as 179568 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
