Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
884843002:
Recording in UMA when a user interacted with an address form or a credit card form. (Closed)
|
Created:
5 years, 10 months ago by Walter Cacau Modified:
5 years, 10 months ago CC:
chromium-reviews, estade+watch_chromium.org, browser-components-watch_chromium.org, asvitkine+watch_chromium.org, rouslan+autofillwatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRecording in UMA when a user interacted with an address form or a credit card form.
BUG=454140
Committed: https://crrev.com/62ae3a5d1154960a8f01c0b7e2a3c6f609c4767f
Cr-Commit-Position: refs/heads/master@{#314899}
Patch Set 1 #Patch Set 2 : Recording in UMA when a user interacted with an address form or a credit card form. #
Total comments: 13
Patch Set 3 : addressing isherman comments #
Total comments: 28
Patch Set 4 : addressing estade@ comments #Patch Set 5 : fixing setter #
Total comments: 2
Patch Set 6 : Adding tests. #Patch Set 7 : addressing latest comments #Patch Set 8 : fixing duplicate emission of CreditCard histograms #
Total comments: 4
Patch Set 9 : addressing isherman@ comments #
Total comments: 22
Patch Set 10 : addressing review comments #
Total comments: 8
Patch Set 11 : addressing comments #Patch Set 12 : rebasing #Patch Set 13 : Fixing test_personal_data_manager. #
Total comments: 3
Patch Set 14 : fixing tests by making personal_data_manager consult web_profiles() instead of accessing web_profil… #
Total comments: 2
Patch Set 15 : inlining web_profiles() call #
Messages
Total messages: 41 (6 generated)
waltercacau@chromium.org changed reviewers: + estade@chromium.org
waltercacau@chromium.org changed reviewers: + isherman@chromium.org
On 2015/01/29 01:03:13, waltercacau wrote: Hi, could I get some early feedback on this CL before I dive in too deep on the tests? Thanks
Patchset #2 (id:20001) has been deleted
I mostly just looked at histograms.xml; I'll defer to Evan for the Autofill details. https://codereview.chromium.org/884843002/diff/40001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/884843002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:1811: +<histogram name="Autofill.FormEvents.Address.WithBothServerAndLocalData" I'd recommend using a <histogram_suffixes> element to reduce the amount of repetition across these histogram descriptions. https://codereview.chromium.org/884843002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:1834: +<histogram name="Autofill.FormEvents.Address.WithOnlyServerData" As I mentioned on the doc, I think the local/server/local+server distinction is probably overkill. Do you really expect users to interact with the UI differently depending on the data sources available to them? At the least, I request that you log a TODO and file a bug to re-evaluate whether this breakdown is still needed after a few milestones have passed. https://codereview.chromium.org/884843002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:42843: + <int value="1" label="Interected once"/> nit: "Interected" -> "Interacted" https://codereview.chromium.org/884843002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:42843: + <int value="1" label="Interected once"/> nit: Any reason not to start numbering at 0? https://codereview.chromium.org/884843002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:42843: + <int value="1" label="Interected once"/> Are you planning to add more events? Any reason not to add them as part of this CL?
https://codereview.chromium.org/884843002/diff/40001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/884843002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:1811: +<histogram name="Autofill.FormEvents.Address.WithBothServerAndLocalData" On 2015/01/30 05:35:48, Ilya Sherman wrote: > I'd recommend using a <histogram_suffixes> element to reduce the amount of > repetition across these histogram descriptions. Done. https://codereview.chromium.org/884843002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:1834: +<histogram name="Autofill.FormEvents.Address.WithOnlyServerData" On 2015/01/30 05:35:48, Ilya Sherman wrote: > As I mentioned on the doc, I think the local/server/local+server distinction is > probably overkill. Do you really expect users to interact with the UI > differently depending on the data sources available to them? > > At the least, I request that you log a TODO and file a bug to re-evaluate > whether this breakdown is still needed after a few milestones have passed. That is a fair point. We (me and my team) believe we need it, but as you said we can always remove them altogether after a few milestones. So, where should this bug be filed? Any place in particular where I should also add a TODO? In the code or in the xml? https://codereview.chromium.org/884843002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:42843: + <int value="1" label="Interected once"/> On 2015/01/30 05:35:48, Ilya Sherman wrote: > Are you planning to add more events? Any reason not to add them as part of this > CL? Yes I am. I may do it in this CL but I wanted a to that the approach I was taking seemed reasonable. If it is I can simply add more to this CL. https://codereview.chromium.org/884843002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:42843: + <int value="1" label="Interected once"/> On 2015/01/30 05:35:48, Ilya Sherman wrote: > nit: "Interected" -> "Interacted" Done. https://codereview.chromium.org/884843002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:42843: + <int value="1" label="Interected once"/> On 2015/01/30 05:35:48, Ilya Sherman wrote: > nit: Any reason not to start numbering at 0? Funny story, I have a feeling we have been doing something wrong here because if I make it start from 0 the histogram code crashes in debug mode (an assertion fails). For enumerations it seems that the code expect enums to have minimum value of 1.
https://codereview.chromium.org/884843002/diff/60001/components/autofill/core... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/884843002/diff/60001/components/autofill/core... components/autofill/core/browser/autofill_manager.cc:477: GetCachedFormAndField(form, field, &form_structure, &autofill_field) && where is the code where the user happiness histogram decides a form is fillable/unfillable? https://codereview.chromium.org/884843002/diff/60001/components/autofill/core... components/autofill/core/browser/autofill_manager.cc:875: has_logged_interacted_with_credit_card_form_ = false; can we put these in FormEventLogger instead https://codereview.chromium.org/884843002/diff/60001/components/autofill/core... components/autofill/core/browser/autofill_manager.cc:939: for (std::vector<CreditCard*>::const_iterator it = credit_cards.begin(); range based for loop for (auto it : credit_cards) { } https://codereview.chromium.org/884843002/diff/60001/components/autofill/core... components/autofill/core/browser/autofill_manager.cc:948: } no curlies also, less verbose: if (credit_card->record_type() == CreditCard::LOCAL_CARD) is_local_data_available = true; else is_server_data_available = true; https://codereview.chromium.org/884843002/diff/60001/components/autofill/core... components/autofill/core/browser/autofill_manager.cc:961: AutofillProfile *profile = *it; AutofillProfile* profile = *it; https://codereview.chromium.org/884843002/diff/60001/components/autofill/core... components/autofill/core/browser/autofill_manager.cc:964: } no curlies; use else https://codereview.chromium.org/884843002/diff/60001/components/autofill/core... File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/884843002/diff/60001/components/autofill/core... components/autofill/core/browser/autofill_metrics.cc:478: std::string custom_name("Autofill.FormEvents."); ? https://codereview.chromium.org/884843002/diff/60001/components/autofill/core... components/autofill/core/browser/autofill_metrics.cc:491: "Autofill.FormEvents.CreditCard.WithNoData", this code could be a bit less verbose -- const char* kCreditCard = "CreditCard"; const char* fill_type = is_credit_card_ ? kCreditCard : kAddress; if (... UMA_HISTOGRAM_ENUMERATION("Foobar.baz." + fill_type + ".WithNoData"); else if (... UMA_HISTOGRAM_ENUMERATION("Foobar.bag." + fill_type + ".WithOnlyServerData"); else if (... etc. you might have to use a #define, or something https://codereview.chromium.org/884843002/diff/60001/components/autofill/core... File components/autofill/core/browser/autofill_metrics.h (right): https://codereview.chromium.org/884843002/diff/60001/components/autofill/core... components/autofill/core/browser/autofill_metrics.h:248: enum AutofillFormEvent { s/Event/Metric to match the other enums here (Event sounds too much like a struct to me) https://codereview.chromium.org/884843002/diff/60001/components/autofill/core... components/autofill/core/browser/autofill_metrics.h:254: // one enum value. Ilya^ https://codereview.chromium.org/884843002/diff/60001/components/autofill/core... components/autofill/core/browser/autofill_metrics.h:433: struct FormEventLogger { this should be a class not a struct https://codereview.chromium.org/884843002/diff/60001/components/autofill/core... components/autofill/core/browser/autofill_metrics.h:439: void Log(AutofillFormEvent event); missing newline and doc https://codereview.chromium.org/884843002/diff/60001/components/autofill/core... components/autofill/core/browser/autofill_metrics.h:444: }; missing newline
Addressed the comments. Still missing tests for segmentation (trying to figure out how to stub the profiles on personal manager). https://codereview.chromium.org/884843002/diff/60001/components/autofill/core... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/884843002/diff/60001/components/autofill/core... components/autofill/core/browser/autofill_manager.cc:477: GetCachedFormAndField(form, field, &form_structure, &autofill_field) && On 2015/01/30 19:39:49, Evan Stade wrote: > where is the code where the user happiness histogram decides a form is > fillable/unfillable? https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... It actually does custom logic as it verifies what it believes the fields were based on the user input data and then applies the minimum 3 fields criteria. https://codereview.chromium.org/884843002/diff/60001/components/autofill/core... components/autofill/core/browser/autofill_manager.cc:875: has_logged_interacted_with_credit_card_form_ = false; On 2015/01/30 19:39:49, Evan Stade wrote: > can we put these in FormEventLogger instead Done. https://codereview.chromium.org/884843002/diff/60001/components/autofill/core... components/autofill/core/browser/autofill_manager.cc:939: for (std::vector<CreditCard*>::const_iterator it = credit_cards.begin(); On 2015/01/30 19:39:49, Evan Stade wrote: > range based for loop > > for (auto it : credit_cards) { > > } Done. https://codereview.chromium.org/884843002/diff/60001/components/autofill/core... components/autofill/core/browser/autofill_manager.cc:948: } On 2015/01/30 19:39:49, Evan Stade wrote: > no curlies > > also, less verbose: > > if (credit_card->record_type() == CreditCard::LOCAL_CARD) > is_local_data_available = true; > else > is_server_data_available = true; Done. https://codereview.chromium.org/884843002/diff/60001/components/autofill/core... components/autofill/core/browser/autofill_manager.cc:961: AutofillProfile *profile = *it; On 2015/01/30 19:39:49, Evan Stade wrote: > AutofillProfile* profile = *it; Done. https://codereview.chromium.org/884843002/diff/60001/components/autofill/core... components/autofill/core/browser/autofill_manager.cc:964: } On 2015/01/30 19:39:49, Evan Stade wrote: > no curlies; use else Done. https://codereview.chromium.org/884843002/diff/60001/components/autofill/core... File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/884843002/diff/60001/components/autofill/core... components/autofill/core/browser/autofill_metrics.cc:478: std::string custom_name("Autofill.FormEvents."); On 2015/01/30 19:39:49, Evan Stade wrote: > ? ops old code from when I tried dynamically generating the histogram name. https://codereview.chromium.org/884843002/diff/60001/components/autofill/core... components/autofill/core/browser/autofill_metrics.cc:491: "Autofill.FormEvents.CreditCard.WithNoData", On 2015/01/30 19:39:50, Evan Stade wrote: > this code could be a bit less verbose -- > > const char* kCreditCard = "CreditCard"; > const char* fill_type = is_credit_card_ ? kCreditCard : kAddress; > > if (... > UMA_HISTOGRAM_ENUMERATION("Foobar.baz." + fill_type + ".WithNoData"); > else if (... > UMA_HISTOGRAM_ENUMERATION("Foobar.bag." + fill_type + ".WithOnlyServerData"); > else if (... > > etc. > > you might have to use a #define, or something So, I actually had code like that but the UMA macro defines a static variable that contains the histogram and it is supposed to be used for exactly one. The internal UMA code actually stores the first histogram name one it sees from a call and if any subsequent call tries to use something different an assertion fails. There may be another better API to UMA to replace this, but I don't know which. https://codereview.chromium.org/884843002/diff/60001/components/autofill/core... File components/autofill/core/browser/autofill_metrics.h (right): https://codereview.chromium.org/884843002/diff/60001/components/autofill/core... components/autofill/core/browser/autofill_metrics.h:248: enum AutofillFormEvent { On 2015/01/30 19:39:50, Evan Stade wrote: > s/Event/Metric to match the other enums here (Event sounds too much like a > struct to me) Keeping event for now as there seems to be a few other instances in the file. https://codereview.chromium.org/884843002/diff/60001/components/autofill/core... components/autofill/core/browser/autofill_metrics.h:433: struct FormEventLogger { On 2015/01/30 19:39:50, Evan Stade wrote: > this should be a class not a struct Done. https://codereview.chromium.org/884843002/diff/60001/components/autofill/core... components/autofill/core/browser/autofill_metrics.h:439: void Log(AutofillFormEvent event); On 2015/01/30 19:39:50, Evan Stade wrote: > missing newline and doc Done. https://codereview.chromium.org/884843002/diff/60001/components/autofill/core... components/autofill/core/browser/autofill_metrics.h:444: }; On 2015/01/30 19:39:50, Evan Stade wrote: > missing newline Done.
Thanks, histograms.xml is looking much better. https://codereview.chromium.org/884843002/diff/40001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/884843002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:1834: +<histogram name="Autofill.FormEvents.Address.WithOnlyServerData" On 2015/01/30 05:55:15, waltercacau wrote: > On 2015/01/30 05:35:48, Ilya Sherman wrote: > > As I mentioned on the doc, I think the local/server/local+server distinction > is > > probably overkill. Do you really expect users to interact with the UI > > differently depending on the data sources available to them? > > > > At the least, I request that you log a TODO and file a bug to re-evaluate > > whether this breakdown is still needed after a few milestones have passed. > > That is a fair point. We (me and my team) believe we need it, but as you said we > can always remove them altogether after a few milestones. > > So, where should this bug be filed? Any place in particular where I should also > add a TODO? In the code or in the xml? Please file a bug in the Chromium bug tracker, with appropriate labels for Autofill and the target milestone. For the TODO, I think it's easiest to include it in autofill_metrics.cc. https://codereview.chromium.org/884843002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:42843: + <int value="1" label="Interected once"/> On 2015/01/30 05:55:15, waltercacau wrote: > On 2015/01/30 05:35:48, Ilya Sherman wrote: > > nit: Any reason not to start numbering at 0? > > Funny story, I have a feeling we have been doing something wrong here because if > I make it start from 0 the histogram code crashes in debug mode (an assertion > fails). For enumerations it seems that the code expect enums to have minimum > value of 1. The assertion is only tripped if you try to define an enumerated histogram with only a single bucket. The reason for this is that bucket 0 is typically reserved for underflow in histograms. However, enumerated histograms don't have underflow; so most enumerated histograms actually write real data into the underflow bucket. The result is that when you have an enumerated histogram with just a single bucket, it looks like you're defining a histogram that has only and underflow and an overflow bucket, and nothing else. This trips an assertion failure, because it looks like you're defining a histogram with no real buckets. Once you add more events, the assertion will no longer be tripped. https://codereview.chromium.org/884843002/diff/100001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/884843002/diff/100001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:1808: + <summary>Autofill form events for address forms.</summary> Please document when events are recorded -- I think it's at form submission time, right? (Ditto below.)
https://codereview.chromium.org/884843002/diff/60001/components/autofill/core... File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/884843002/diff/60001/components/autofill/core... components/autofill/core/browser/autofill_metrics.cc:491: "Autofill.FormEvents.CreditCard.WithNoData", On 2015/01/30 22:05:56, waltercacau wrote: > On 2015/01/30 19:39:50, Evan Stade wrote: > > this code could be a bit less verbose -- > > > > const char* kCreditCard = "CreditCard"; > > const char* fill_type = is_credit_card_ ? kCreditCard : kAddress; > > > > if (... > > UMA_HISTOGRAM_ENUMERATION("Foobar.baz." + fill_type + ".WithNoData"); > > else if (... > > UMA_HISTOGRAM_ENUMERATION("Foobar.bag." + fill_type + > ".WithOnlyServerData"); > > else if (... > > > > etc. > > > > you might have to use a #define, or something > > So, I actually had code like that but the UMA macro defines a static variable > that contains the histogram and it is supposed to be used for exactly one. The > internal UMA code actually stores the first histogram name one it sees from a > call and if any subsequent call tries to use something different an assertion > fails. > > There may be another better API to UMA to replace this, but I don't know which. pretty sure you could still do this somehow with macros #define TYPE_SPECIFIC_HISTOGRAM(a, b, c, d, e) \ if (a)\ UMA_HISTOGRAM_ENUMERATION(b "CreditCard" c, d, e);\ else\ UMA_HISTOGRAM_ENUMERATION(b "Address" c, d, e)
https://codereview.chromium.org/884843002/diff/60001/components/autofill/core... File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/884843002/diff/60001/components/autofill/core... components/autofill/core/browser/autofill_metrics.cc:491: "Autofill.FormEvents.CreditCard.WithNoData", On 2015/01/30 22:35:15, Evan Stade wrote: > On 2015/01/30 22:05:56, waltercacau wrote: > > On 2015/01/30 19:39:50, Evan Stade wrote: > > > this code could be a bit less verbose -- > > > > > > const char* kCreditCard = "CreditCard"; > > > const char* fill_type = is_credit_card_ ? kCreditCard : kAddress; > > > > > > if (... > > > UMA_HISTOGRAM_ENUMERATION("Foobar.baz." + fill_type + ".WithNoData"); > > > else if (... > > > UMA_HISTOGRAM_ENUMERATION("Foobar.bag." + fill_type + > > ".WithOnlyServerData"); > > > else if (... > > > > > > etc. > > > > > > you might have to use a #define, or something > > > > So, I actually had code like that but the UMA macro defines a static variable > > that contains the histogram and it is supposed to be used for exactly one. The > > internal UMA code actually stores the first histogram name one it sees from a > > call and if any subsequent call tries to use something different an assertion > > fails. > > > > There may be another better API to UMA to replace this, but I don't know > which. > > pretty sure you could still do this somehow with macros > > #define TYPE_SPECIFIC_HISTOGRAM(a, b, c, d, e) \ > if (a)\ > UMA_HISTOGRAM_ENUMERATION(b "CreditCard" c, d, e);\ > else\ > UMA_HISTOGRAM_ENUMERATION(b "Address" c, d, e) Alternately, you can inline the macro definition, as is done in other parts of this file (mostly toward the top, IIRC).
Added all the tests. Ready for review :)
I actually missed addressing your latest comments. Sorry about that, will address them shortly. On Jan 30, 2015 3:51 PM, <waltercacau@chromium.org> wrote: > Added all the tests. Ready for review :) > > https://codereview.chromium.org/884843002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
now it is ready for review https://codereview.chromium.org/884843002/diff/40001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/884843002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:1834: +<histogram name="Autofill.FormEvents.Address.WithOnlyServerData" On 2015/01/30 22:23:48, Ilya Sherman wrote: > On 2015/01/30 05:55:15, waltercacau wrote: > > On 2015/01/30 05:35:48, Ilya Sherman wrote: > > > As I mentioned on the doc, I think the local/server/local+server distinction > > is > > > probably overkill. Do you really expect users to interact with the UI > > > differently depending on the data sources available to them? > > > > > > At the least, I request that you log a TODO and file a bug to re-evaluate > > > whether this breakdown is still needed after a few milestones have passed. > > > > That is a fair point. We (me and my team) believe we need it, but as you said > we > > can always remove them altogether after a few milestones. > > > > So, where should this bug be filed? Any place in particular where I should > also > > add a TODO? In the code or in the xml? > > Please file a bug in the Chromium bug tracker, with appropriate labels for > Autofill and the target milestone. For the TODO, I think it's easiest to > include it in autofill_metrics.cc. Done. https://code.google.com/p/chromium/issues/detail?id=454017 https://codereview.chromium.org/884843002/diff/60001/components/autofill/core... File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/884843002/diff/60001/components/autofill/core... components/autofill/core/browser/autofill_metrics.cc:491: "Autofill.FormEvents.CreditCard.WithNoData", On 2015/01/30 22:38:05, Ilya Sherman wrote: > On 2015/01/30 22:35:15, Evan Stade wrote: > > On 2015/01/30 22:05:56, waltercacau wrote: > > > On 2015/01/30 19:39:50, Evan Stade wrote: > > > > this code could be a bit less verbose -- > > > > > > > > const char* kCreditCard = "CreditCard"; > > > > const char* fill_type = is_credit_card_ ? kCreditCard : kAddress; > > > > > > > > if (... > > > > UMA_HISTOGRAM_ENUMERATION("Foobar.baz." + fill_type + ".WithNoData"); > > > > else if (... > > > > UMA_HISTOGRAM_ENUMERATION("Foobar.bag." + fill_type + > > > ".WithOnlyServerData"); > > > > else if (... > > > > > > > > etc. > > > > > > > > you might have to use a #define, or something > > > > > > So, I actually had code like that but the UMA macro defines a static > variable > > > that contains the histogram and it is supposed to be used for exactly one. > The > > > internal UMA code actually stores the first histogram name one it sees from > a > > > call and if any subsequent call tries to use something different an > assertion > > > fails. > > > > > > There may be another better API to UMA to replace this, but I don't know > > which. > > > > pretty sure you could still do this somehow with macros > > > > #define TYPE_SPECIFIC_HISTOGRAM(a, b, c, d, e) \ > > if (a)\ > > UMA_HISTOGRAM_ENUMERATION(b "CreditCard" c, d, e);\ > > else\ > > UMA_HISTOGRAM_ENUMERATION(b "Address" c, d, e) > > Alternately, you can inline the macro definition, as is done in other parts of > this file (mostly toward the top, IIRC). Done. https://codereview.chromium.org/884843002/diff/100001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/884843002/diff/100001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:1808: + <summary>Autofill form events for address forms.</summary> On 2015/01/30 22:23:48, Ilya Sherman wrote: > Please document when events are recorded -- I think it's at form submission > time, right? (Ditto below.) not quite. it is when the user interacts (which includes click, autofill suggestion, autofill selection and submit).
Thanks. The histograms are now looking good, though I'd still encourage you to go ahead and track more than just the one event. https://codereview.chromium.org/884843002/diff/160001/components/autofill/cor... File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/884843002/diff/160001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.cc:503: // segmentation. nit: Please link to the bug that you filed, as "http://crbug.com/NNNNNN". https://codereview.chromium.org/884843002/diff/160001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/884843002/diff/160001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:1810: + interacts with a form containing an address. Pedantic nit: I think this metric is logged when the form requests an address, rather than when the form contains one -- right?
Also, please file a bug for this CL, and update the BUG= line. In the bug, please link to the doc where these metrics were discussed.
created http://crbug.com/454140 https://codereview.chromium.org/884843002/diff/160001/components/autofill/cor... File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/884843002/diff/160001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.cc:503: // segmentation. On 2015/01/31 01:54:05, Ilya Sherman wrote: > nit: Please link to the bug that you filed, as "http://crbug.com/NNNNNN". Done. https://codereview.chromium.org/884843002/diff/160001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/884843002/diff/160001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:1810: + interacts with a form containing an address. On 2015/01/31 01:54:05, Ilya Sherman wrote: > Pedantic nit: I think this metric is logged when the form requests an address, > rather than when the form contains one -- right? Done.
histograms.xml LGTM. I defer to Evan for review of the remaining files. https://codereview.chromium.org/884843002/diff/180001/components/autofill/cor... File components/autofill/core/browser/autofill_metrics.h (right): https://codereview.chromium.org/884843002/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.h:248: enum AutofillFormEvent { Since this enum is already in a class that has "Autofill" in its name, which furthermore lives in the "autofill" namespace, I'd drop the "Autofill" from this name. https://codereview.chromium.org/884843002/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.h:251: INTERACTED_ONCE_FORM_EVENT = 0, nit: By convention, FORM_EVENT should be the prefix rather than the suffix. https://codereview.chromium.org/884843002/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.h:253: // This circunvents an assertion that gets triggered when you have nit: "circunvents" -> "circumvents"
https://codereview.chromium.org/884843002/diff/180001/components/autofill/cor... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/884843002/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:476: bool is_filling_credit_card = (type.group() == CREDIT_CARD); nit: if (autofill_field->Type().group() == CREDIT_CARD) https://codereview.chromium.org/884843002/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:912: credit_card->record_type() == CreditCard::FULL_SERVER_CARD) curlies required but not if you make it shorter by just checking against LOCAL_CARD type https://codereview.chromium.org/884843002/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:937: if (profiles.empty() && credit_cards.empty()) { nit: no curlies https://codereview.chromium.org/884843002/diff/180001/components/autofill/cor... File components/autofill/core/browser/autofill_metrics.h (right): https://codereview.chromium.org/884843002/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.h:437: void set_is_server_data_available(bool is_server_data_available); inline this function https://codereview.chromium.org/884843002/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.h:440: void set_is_local_data_available(bool is_local_data_available); inline this function https://codereview.chromium.org/884843002/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.h:443: void OnDidInteractWithAutofillableForm(); move above the setters https://codereview.chromium.org/884843002/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.h:449: bool is_credit_card_; nit: variable name doesn't sound quite right. It's more like whether this is a FormEventLogger for a credit card fill event. So for_credit_card_ would make more sense? https://codereview.chromium.org/884843002/diff/180001/components/autofill/cor... File components/autofill/core/browser/personal_data_manager.h (right): https://codereview.chromium.org/884843002/diff/180001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.h:334: bool test_force_wallet_card_sync_enabled_; this is not appropriate. You can adjust the command line flags in tests.
https://codereview.chromium.org/884843002/diff/180001/components/autofill/cor... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/884843002/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:476: bool is_filling_credit_card = (type.group() == CREDIT_CARD); On 2015/02/02 23:13:12, Evan Stade wrote: > nit: > > if (autofill_field->Type().group() == CREDIT_CARD) Done. https://codereview.chromium.org/884843002/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:912: credit_card->record_type() == CreditCard::FULL_SERVER_CARD) On 2015/02/02 23:13:12, Evan Stade wrote: > curlies required > > but not if you make it shorter by just checking against LOCAL_CARD type Done. https://codereview.chromium.org/884843002/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:937: if (profiles.empty() && credit_cards.empty()) { On 2015/02/02 23:13:12, Evan Stade wrote: > nit: no curlies Done. https://codereview.chromium.org/884843002/diff/180001/components/autofill/cor... File components/autofill/core/browser/autofill_metrics.h (right): https://codereview.chromium.org/884843002/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.h:248: enum AutofillFormEvent { On 2015/02/02 22:32:16, Ilya Sherman wrote: > Since this enum is already in a class that has "Autofill" in its name, which > furthermore lives in the "autofill" namespace, I'd drop the "Autofill" from this > name. Done. https://codereview.chromium.org/884843002/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.h:251: INTERACTED_ONCE_FORM_EVENT = 0, On 2015/02/02 22:32:16, Ilya Sherman wrote: > nit: By convention, FORM_EVENT should be the prefix rather than the suffix. Done. https://codereview.chromium.org/884843002/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.h:253: // This circunvents an assertion that gets triggered when you have On 2015/02/02 22:32:16, Ilya Sherman wrote: > nit: "circunvents" -> "circumvents" Done. https://codereview.chromium.org/884843002/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.h:437: void set_is_server_data_available(bool is_server_data_available); On 2015/02/02 23:13:12, Evan Stade wrote: > inline this function Done. https://codereview.chromium.org/884843002/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.h:440: void set_is_local_data_available(bool is_local_data_available); On 2015/02/02 23:13:12, Evan Stade wrote: > inline this function Done. https://codereview.chromium.org/884843002/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.h:443: void OnDidInteractWithAutofillableForm(); On 2015/02/02 23:13:12, Evan Stade wrote: > move above the setters Done. https://codereview.chromium.org/884843002/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.h:449: bool is_credit_card_; On 2015/02/02 23:13:12, Evan Stade wrote: > nit: variable name doesn't sound quite right. It's more like whether this is a > FormEventLogger for a credit card fill event. So for_credit_card_ would make > more sense? You are right. Doing is_for_credit_card_ just because we keep the is_ which makes it more explicit that it is a boolean. https://codereview.chromium.org/884843002/diff/180001/components/autofill/cor... File components/autofill/core/browser/personal_data_manager.h (right): https://codereview.chromium.org/884843002/diff/180001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.h:334: bool test_force_wallet_card_sync_enabled_; On 2015/02/02 23:13:12, Evan Stade wrote: > this is not appropriate. You can adjust the command line flags in tests. Done.
lgtm https://codereview.chromium.org/884843002/diff/200001/components/autofill/cor... File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/884843002/diff/200001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.cc:484: if (is_for_credit_card_) { no curlies https://codereview.chromium.org/884843002/diff/200001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.cc:494: if (!is_server_data_available_ && !is_local_data_available_) { no curlies for any of this block https://codereview.chromium.org/884843002/diff/200001/components/autofill/cor... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/884843002/diff/200001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:1199: bool use_auxiliary_profiles = this kind of ifdef splicing confuses a lot of syntax highlighters --- please just copy this one line twice. https://codereview.chromium.org/884843002/diff/200001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:1209: if (use_auxiliary_profiles) { no curlies
https://codereview.chromium.org/884843002/diff/200001/components/autofill/cor... File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/884843002/diff/200001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.cc:484: if (is_for_credit_card_) { On 2015/02/03 01:51:36, Evan Stade wrote: > no curlies Done. https://codereview.chromium.org/884843002/diff/200001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.cc:494: if (!is_server_data_available_ && !is_local_data_available_) { On 2015/02/03 01:51:36, Evan Stade wrote: > no curlies for any of this block Done. https://codereview.chromium.org/884843002/diff/200001/components/autofill/cor... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/884843002/diff/200001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:1199: bool use_auxiliary_profiles = On 2015/02/03 01:51:36, Evan Stade wrote: > this kind of ifdef splicing confuses a lot of syntax highlighters --- please > just copy this one line twice. Done. https://codereview.chromium.org/884843002/diff/200001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:1209: if (use_auxiliary_profiles) { On 2015/02/03 01:51:36, Evan Stade wrote: > no curlies Done.
The CQ bit was checked by waltercacau@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/884843002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2015/02/03 04:31:47, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > linux_chromium_chromeos_rel_ng on tryserver.chromium.linux > (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) Had to fix test_personal_data_manager.cc. Before, it only had overridden the GetProfiles() public method, but the internal GetProfiles(bool) was being triggered in several tests. After my change to GetProfiles(bool), these tests started failing. The fix was to also override GetProfiles(bool) to return the desired stubbed profiles.
https://codereview.chromium.org/884843002/diff/220002/components/autofill/cor... File components/autofill/core/browser/personal_data_manager.cc (left): https://codereview.chromium.org/884843002/diff/220002/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:1212: profiles_.insert(profiles_.end(), web_profiles_.begin(), web_profiles_.end()); couldn't you fix this by changing these web_profiles_ to web_profiles()?
https://codereview.chromium.org/884843002/diff/220002/components/autofill/cor... File components/autofill/core/browser/personal_data_manager.cc (left): https://codereview.chromium.org/884843002/diff/220002/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:1212: profiles_.insert(profiles_.end(), web_profiles_.begin(), web_profiles_.end()); On 2015/02/03 20:02:11, Evan Stade wrote: > couldn't you fix this by changing these web_profiles_ to web_profiles()? Unfortunately this does not work because test_personal_manager uses profiles_ as its storage mechanism and we clear it here. So far I can only see the following alternative options: 1) Change test_personal_manager to have its own _test_profiles variable instead of it relying on _profiles. 2) Keep the current solution but don't override GetProfiles, just override GetProfiles(bool). Reasoning: GetProfiles implementation is just GetProfiles(false).
On 2015/02/03 20:25:00, waltercacau wrote: > https://codereview.chromium.org/884843002/diff/220002/components/autofill/cor... > File components/autofill/core/browser/personal_data_manager.cc (left): > > https://codereview.chromium.org/884843002/diff/220002/components/autofill/cor... > components/autofill/core/browser/personal_data_manager.cc:1212: > profiles_.insert(profiles_.end(), web_profiles_.begin(), web_profiles_.end()); > On 2015/02/03 20:02:11, Evan Stade wrote: > > couldn't you fix this by changing these web_profiles_ to web_profiles()? > > Unfortunately this does not work because test_personal_manager uses profiles_ as > its storage mechanism and we clear it here. > > So far I can only see the following alternative options: > 1) Change test_personal_manager to have its own _test_profiles variable instead > of it relying on _profiles. this > 2) Keep the current solution but don't override GetProfiles, just override > GetProfiles(bool). Reasoning: GetProfiles implementation is just > GetProfiles(false).
On 2015/02/05 03:22:04, Evan Stade wrote: > On 2015/02/03 20:25:00, waltercacau wrote: > > > https://codereview.chromium.org/884843002/diff/220002/components/autofill/cor... > > File components/autofill/core/browser/personal_data_manager.cc (left): > > > > > https://codereview.chromium.org/884843002/diff/220002/components/autofill/cor... > > components/autofill/core/browser/personal_data_manager.cc:1212: > > profiles_.insert(profiles_.end(), web_profiles_.begin(), web_profiles_.end()); > > On 2015/02/03 20:02:11, Evan Stade wrote: > > > couldn't you fix this by changing these web_profiles_ to web_profiles()? > > > > Unfortunately this does not work because test_personal_manager uses profiles_ > as > > its storage mechanism and we clear it here. > > > > So far I can only see the following alternative options: > > 1) Change test_personal_manager to have its own _test_profiles variable > instead > > of it relying on _profiles. > > this > > > 2) Keep the current solution but don't override GetProfiles, just override > > GetProfiles(bool). Reasoning: GetProfiles implementation is just > > GetProfiles(false). (option 1, if that was unclear)
https://codereview.chromium.org/884843002/diff/220002/components/autofill/cor... File components/autofill/core/browser/personal_data_manager.cc (left): https://codereview.chromium.org/884843002/diff/220002/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:1212: profiles_.insert(profiles_.end(), web_profiles_.begin(), web_profiles_.end()); On 2015/02/03 20:25:00, waltercacau wrote: > On 2015/02/03 20:02:11, Evan Stade wrote: > > couldn't you fix this by changing these web_profiles_ to web_profiles()? > > Unfortunately this does not work because test_personal_manager uses profiles_ as > its storage mechanism and we clear it here. > > So far I can only see the following alternative options: > 1) Change test_personal_manager to have its own _test_profiles variable instead > of it relying on _profiles. > 2) Keep the current solution but don't override GetProfiles, just override > GetProfiles(bool). Reasoning: GetProfiles implementation is just > GetProfiles(false). I was wrong. There is no conflict in the profiles_ because test_personal_manager also declares a private profile_ member. Did in the original way you suggested.
https://codereview.chromium.org/884843002/diff/270001/components/autofill/cor... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/884843002/diff/270001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:1214: profiles_.insert(profiles_.end(), web_profiles.begin(), web_profiles.end()); why can't you just use web_profiles() inline?
https://codereview.chromium.org/884843002/diff/270001/components/autofill/cor... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/884843002/diff/270001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:1214: profiles_.insert(profiles_.end(), web_profiles.begin(), web_profiles.end()); On 2015/02/05 04:07:25, Evan Stade wrote: > why can't you just use web_profiles() inline? inlined now I initially thought that web_profiles() did more then just return the contents of web_profiles_.
On 2015/02/05 04:12:03, waltercacau wrote: > https://codereview.chromium.org/884843002/diff/270001/components/autofill/cor... > File components/autofill/core/browser/personal_data_manager.cc (right): > > https://codereview.chromium.org/884843002/diff/270001/components/autofill/cor... > components/autofill/core/browser/personal_data_manager.cc:1214: > profiles_.insert(profiles_.end(), web_profiles.begin(), web_profiles.end()); > On 2015/02/05 04:07:25, Evan Stade wrote: > > why can't you just use web_profiles() inline? > > inlined now > > I initially thought that web_profiles() did more then just return the contents > of web_profiles_. generally speaking, if it did, it would/should be called GetWebProfiles()
lgtm
The CQ bit was checked by waltercacau@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/884843002/290001
Message was sent while issue was closed.
Committed patchset #15 (id:290001)
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/62ae3a5d1154960a8f01c0b7e2a3c6f609c4767f Cr-Commit-Position: refs/heads/master@{#314899} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
