Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23140005:
Added of new UMA signals in order to be able to discover early if the "save password" feature gets … (Closed)
|
Created:
7 years, 4 months ago by jdomingos Modified:
7 years, 3 months ago CC:
chromium-reviews, Raman Kakilate, jar (doing other things), benquan, ahutter, asvitkine+watch_chromium.org, Dane Wallinga, dyu1, estade+watch_chromium.org, Albert Bodenhamer, rouslan+autofillwatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdded of new UMA signals in order to be able to discover early if the "save password" feature gets broken again.
According the http://crbug.com/262215: Detect, fix and monitor
usability problems of password management
New Boolean : PasswordManager.InforbarDisplayed
PasswordManager.InforbarDisplayedTooShortly
PasswordManager.PreviousPasswordSavedRecently
BUG=262215, 268980
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=223597
Patch Set 1 #
Total comments: 31
Patch Set 2 : Second review #Patch Set 3 : Minor changes #Patch Set 4 : Additional minor changes #
Total comments: 11
Patch Set 5 : Review 3 #
Total comments: 7
Patch Set 6 : Review 4 #Patch Set 7 : Review 4 bis (Base files missing error in the previous one) #
Total comments: 49
Patch Set 8 : Review 5 #
Total comments: 35
Patch Set 9 : Review 5 #
Total comments: 4
Patch Set 10 : Signal renamed and buckets added #Patch Set 11 : Minor changes #
Total comments: 20
Patch Set 12 : Review 6 #
Total comments: 56
Patch Set 13 : Review 7 #
Total comments: 41
Patch Set 14 : Review 8 #Patch Set 15 : Minor changes #
Total comments: 68
Patch Set 16 : Review 9 #
Total comments: 22
Patch Set 17 : Review 10 #Patch Set 18 : Minor changes #
Total comments: 26
Patch Set 19 : Review 11 #Patch Set 20 : Minor changes #Patch Set 21 : Minor changes #
Total comments: 15
Patch Set 22 : Review 12 #Patch Set 23 : Minor changes #
Total comments: 8
Patch Set 24 : Review 13 #Patch Set 25 : Minor changes #Patch Set 26 : Minor changes #Patch Set 27 : Minor changes #
Total comments: 4
Patch Set 28 : Review 14 #
Total comments: 1
Patch Set 29 : Test name changed #Messages
Total messages: 60 (0 generated)
Hi everybody, I upload my first cl for the UMA signal, Could you please take a look to my code source ? Thanks.
Hi Jordy, Please find the comments below. Cheers, Vaclav https://codereview.chromium.org/23140005/diff/1/chrome/browser/password_manag... File chrome/browser/password_manager/password_manager.cc (right): https://codereview.chromium.org/23140005/diff/1/chrome/browser/password_manag... chrome/browser/password_manager/password_manager.cc:71: domain_name.insert(0, "_"); We may not append any domain name, only one of a carefully selected popular ones (say top 5 or top 10 on the list from Patrick Nepper). For other domains, we should not report at all. https://codereview.chromium.org/23140005/diff/1/chrome/browser/password_manag... chrome/browser/password_manager/password_manager.cc:304: // All functions are tested on by one in order to send the appropriate on by one -> one by one https://codereview.chromium.org/23140005/diff/1/chrome/browser/password_manag... File chrome/browser/password_manager/password_manager.h (right): https://codereview.chromium.org/23140005/diff/1/chrome/browser/password_manag... chrome/browser/password_manager/password_manager.h:40: SAVE_PASSWORD_DISABLE, nit: DISABLE->DISABLED ? https://codereview.chromium.org/23140005/diff/1/chrome/browser/password_manag... chrome/browser/password_manager/password_manager.h:47: CANNOT_GET_THE_PROVIOSIONAL_PASSWORD, typo: PROVIOSIONAL https://codereview.chromium.org/23140005/diff/1/chrome/browser/password_manag... chrome/browser/password_manager/password_manager.h:49: LOGIN_ALREADY_KNEW, nit: KNEW->KNOWN ? https://codereview.chromium.org/23140005/diff/1/chrome/browser/password_manag... chrome/browser/password_manager/password_manager.h:50: PASSWORD_GENERATED, How does this differ from PASSWORD_GENERATED_OR_AUTOCOMPLETED? https://codereview.chromium.org/23140005/diff/1/chrome/browser/password_manag... chrome/browser/password_manager/password_manager.h:51: ORIGIN_MATCHING_OF_PUBLIC_SUFFIX, nit: leave out "OF" ? https://codereview.chromium.org/23140005/diff/1/chrome/browser/password_manag... chrome/browser/password_manager/password_manager.h:52: NUM_ERROR_TYPES Here I am not sure about the C++ standard -- are we guaranteed that the enum values are numbered from 0 up by +1 increments? If yes, then this is fine. Otherwise we need to assign values explicitly. https://codereview.chromium.org/23140005/diff/1/chrome/browser/password_manag... File chrome/browser/password_manager/password_manager_delegate_impl.cc (right): https://codereview.chromium.org/23140005/diff/1/chrome/browser/password_manag... chrome/browser/password_manager/password_manager_delegate_impl.cc:115: domain_name_ = gurl.host(); Here again, only for a handful of hosts we want to log the UMA signal. https://codereview.chromium.org/23140005/diff/1/chrome/browser/password_manag... chrome/browser/password_manager/password_manager_delegate_impl.cc:123: // Time in milliseconds. Time of what? Please comment (in the code, not in the code-review tool) on what his constant means and why it was chosen. https://codereview.chromium.org/23140005/diff/1/chrome/browser/password_manag... chrome/browser/password_manager/password_manager_delegate_impl.cc:125: UMA_HISTOGRAM_ENUMERATION("PasswordManager.InfoBarResponse", This comments is on behalf of Jordy, as a reminder: "Add the enum definition of InfoBarResponse from http://chromegw/viewvc/chrome-internal/trunk/tools/histograms/histograms.xml?... Note that I'm not sure about that actually. It looks like a couple of enums are defined in the above internal histograms.xml, but used also in the public one. Garrett, do you know what is the policy about using enums from the deprecated internal histograms.xml? https://codereview.chromium.org/23140005/diff/1/chrome/browser/password_manag... chrome/browser/password_manager/password_manager_delegate_impl.cc:129: timer_.Elapsed().InMilliseconds() > kTime ? true : false); 1) Omit the ternary operator. boolean ? true : false can be written simply just as boolean 2) Did you mean to negate the result? In other words is "too shortly" equivalent to "timer_.Elapsed().InMilliseconds() < kTime"? https://codereview.chromium.org/23140005/diff/1/chrome/renderer/autofill/form... File chrome/renderer/autofill/form_autofill_browsertest.cc (right): https://codereview.chromium.org/23140005/diff/1/chrome/renderer/autofill/form... chrome/renderer/autofill/form_autofill_browsertest.cc:3041: TEST_F(FormAutofillTest, ExtractHostNameFromUrlTest) { The tested function has been removed, right? If so, then also the test needs to go. https://codereview.chromium.org/23140005/diff/1/tools/metrics/histograms/hist... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/23140005/diff/1/tools/metrics/histograms/hist... tools/metrics/histograms/histograms.xml:9476: +<histogram name="PasswordManager.InfobarDislayedTooShortly"> I saw a couple of Boolean* enum types in http://chromegw/viewvc/chrome-internal/trunk/tools/histograms/histograms.xml?.... Maybe we could use them or create a similar one in that fashion? I'm not sure what is the guidance here, maybe Garrett knows? https://codereview.chromium.org/23140005/diff/1/tools/metrics/histograms/hist... tools/metrics/histograms/histograms.xml:9479: + term of time. nit: "in term of time" could be probably left out; or at least please substitute term->terms. https://codereview.chromium.org/23140005/diff/1/tools/metrics/histograms/hist... tools/metrics/histograms/histograms.xml:9500: + List of the reasons to not display the save password info bar. This is not a list, right? Is it rather "an enumeration of possible reasons..."? https://codereview.chromium.org/23140005/diff/1/tools/metrics/histograms/hist... tools/metrics/histograms/histograms.xml:9506: + A boolean that indicates if the current user saved his password for in the "for in" -> for "his password" -> "a password for the same login" https://codereview.chromium.org/23140005/diff/1/tools/metrics/histograms/hist... tools/metrics/histograms/histograms.xml:20393: +<enum name="PotentialInfoBarErrorReasons" type="int"> Don't forget to update this definition once you change the corresponding enum in the C++ code. https://codereview.chromium.org/23140005/diff/1/tools/metrics/histograms/hist... tools/metrics/histograms/histograms.xml:24018: + <group name="twitter" label="twitter"/> We should make up our mind about which hosts to include in the monitoring. Can you take the first 10 of Patrick Nepper's list?
Also, I swapped Garrett for Ilya in the reviewers field, since Ilya is OOO. Vaclav
https://codereview.chromium.org/23140005/diff/1/chrome/browser/password_manag... File chrome/browser/password_manager/password_manager.cc (right): https://codereview.chromium.org/23140005/diff/1/chrome/browser/password_manag... chrome/browser/password_manager/password_manager.cc:71: domain_name.insert(0, "_"); On 2013/08/14 14:39:58, vabr (Chromium) wrote: > We may not append any domain name, only one of a carefully selected popular ones > (say top 5 or top 10 on the list from Patrick Nepper). > For other domains, we should not report at all. I didn't even realize you could send any identifiable information at all via UMA stats, though I trust your judgement. What is this list of domains BTW? Do we think that it's going to be worth trying to report domain stats for just a few domains? If we do this, will we have to update the privacy policy in any way? https://codereview.chromium.org/23140005/diff/1/chrome/browser/password_manag... chrome/browser/password_manager/password_manager.cc:73: UMA_HISTOGRAM_BOOLEAN("PasswordManager.InfobarDisplayed" + domain_name, I'm not sure if this is necessary as if this is negative, we are logging the failure reasons, and if it's positive then we have a different UMA stat that tracks that "PasswordManager.InfoBarResponse" https://codereview.chromium.org/23140005/diff/1/chrome/browser/password_manag... chrome/browser/password_manager/password_manager.cc:155: if (!IsSavingEnabled()) { I actually just submitted a CL (https://codereview.chromium.org/22960002/) which records provisional save failure cases, so you should probably sync and pick up that CL. It's basically a subset of what you are trying to do, as it only checks provisional save password failures and doesn't try to record any domain name information. https://codereview.chromium.org/23140005/diff/1/chrome/browser/password_manag... chrome/browser/password_manager/password_manager.cc:302: bool PasswordManager::ShouldShowSavePasswordInfoBar() const { I don't think that this function does what you think. If it returns true it means that we need to prompt the user to save the password, otherwise the user has already given their consent and we can save automatically. Having stats on these cases is interesting, but I'm not sure if we want to group them together with cases where we actually don't save the password. https://codereview.chromium.org/23140005/diff/1/chrome/browser/password_manag... chrome/browser/password_manager/password_manager.cc:328: visible_forms[0].origin); Not sure if any of the reporting in this function is necessary. This will get triggered only if provisional saving fails, which we already have monitoring for. https://codereview.chromium.org/23140005/diff/1/chrome/browser/password_manag... chrome/browser/password_manager/password_manager.cc:342: FORM_REAPPEARED, GURL(provisional_save_manager_->realm())); We already keep track of this stat in the provisional_save_manager. See PasswordFormManager::SubmitFailed() https://codereview.chromium.org/23140005/diff/1/chrome/browser/password_manag... File chrome/browser/password_manager/password_manager.h (right): https://codereview.chromium.org/23140005/diff/1/chrome/browser/password_manag... chrome/browser/password_manager/password_manager.h:52: NUM_ERROR_TYPES On 2013/08/14 14:39:58, vabr (Chromium) wrote: > Here I am not sure about the C++ standard -- are we guaranteed that the enum > values are numbered from 0 up by +1 increments? > If yes, then this is fine. > Otherwise we need to assign values explicitly. Yep. https://codereview.chromium.org/23140005/diff/1/chrome/browser/password_manag... File chrome/browser/password_manager/password_manager_delegate_impl.cc (right): https://codereview.chromium.org/23140005/diff/1/chrome/browser/password_manag... chrome/browser/password_manager/password_manager_delegate_impl.cc:125: UMA_HISTOGRAM_ENUMERATION("PasswordManager.InfoBarResponse", On 2013/08/14 14:39:58, vabr (Chromium) wrote: > This comments is on behalf of Jordy, as a reminder: > > "Add the enum definition of InfoBarResponse from > http://chromegw/viewvc/chrome-internal/trunk/tools/histograms/histograms.xml?... > > Note that I'm not sure about that actually. It looks like a couple of enums are > defined in the above internal histograms.xml, but used also in the public one. > Garrett, do you know what is the policy about using enums from the deprecated > internal histograms.xml? It sounds like they are just migrating from the internal to the external one, and you can use data from either, but this is not something I know too much about. asvitkine@ knows better. https://codereview.chromium.org/23140005/diff/1/tools/metrics/histograms/hist... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/23140005/diff/1/tools/metrics/histograms/hist... tools/metrics/histograms/histograms.xml:9476: +<histogram name="PasswordManager.InfobarDislayedTooShortly"> On 2013/08/14 14:39:58, vabr (Chromium) wrote: > I saw a couple of Boolean* enum types in > http://chromegw/viewvc/chrome-internal/trunk/tools/histograms/histograms.xml?.... > Maybe we could use them or create a similar one in that fashion? > > I'm not sure what is the guidance here, maybe Garrett knows? It looks like people seem to just use enum="BooleanSuccess", but not my area of expertise.
https://codereview.chromium.org/23140005/diff/1/chrome/browser/password_manag... File chrome/browser/password_manager/password_manager.cc (right): https://codereview.chromium.org/23140005/diff/1/chrome/browser/password_manag... chrome/browser/password_manager/password_manager.cc:71: domain_name.insert(0, "_"); On 2013/08/14 23:02:47, Garrett Casto wrote: > On 2013/08/14 14:39:58, vabr (Chromium) wrote: > > We may not append any domain name, only one of a carefully selected popular > ones > > (say top 5 or top 10 on the list from Patrick Nepper). > > For other domains, we should not report at all. > > I didn't even realize you could send any identifiable information at all via UMA > stats, though I trust your judgement. Yeah, we have to be careful. Logging the domain for a very popular site might not be considered identifying, but this will still need a privacy review (by someone else than me). There is already a review ticket for M31 waiting for this: http://crbug.com/268980 (I Cc-ed you for access). > What is this list of domains BTW? Do we > think that it's going to be worth trying to report domain stats for just a few > domains? If we do this, will we have to update the privacy policy in any way? https://codereview.chromium.org/23140005/diff/1/chrome/browser/password_manag... chrome/browser/password_manager/password_manager.cc:73: UMA_HISTOGRAM_BOOLEAN("PasswordManager.InfobarDisplayed" + domain_name, On 2013/08/14 23:02:47, Garrett Casto wrote: > I'm not sure if this is necessary as if this is negative, we are logging the > failure reasons, and if it's positive then we have a different UMA stat that > tracks that "PasswordManager.InfoBarResponse" That's a good point. Jordy, when you are back, let's talk about how we would actually display the signals you are adding, and see if having this one helps anything or not.
@vabr @gcasto @markusheintz @isherman Hi everyone, I made the changes asked, could you please review my code ? @isherman I saw a TODO about making a boolean histogram to report the state of the password manager (enable, disable). Because I was working on this file I allowed myself to implement it. If you think that I did something wrong, or if you want to do it personally I can undo my changes. @gcasto @vabr Regarding the PasswordManager.InfobarDisplayed signal, I think that it is relevant to keep it because, even if we can know if the info bar is displayed thanks to the info bar response, a change of the total number of info bar displayed can have a real meaning in order to know if the password manager is broken. https://codereview.chromium.org/23140005/diff/1/chrome/renderer/autofill/form... File chrome/renderer/autofill/form_autofill_browsertest.cc (right): https://codereview.chromium.org/23140005/diff/1/chrome/renderer/autofill/form... chrome/renderer/autofill/form_autofill_browsertest.cc:3041: TEST_F(FormAutofillTest, ExtractHostNameFromUrlTest) { On 2013/08/14 14:39:58, vabr (Chromium) wrote: > The tested function has been removed, right? If so, then also the test needs to > go. Done.
https://codereview.chromium.org/23140005/diff/18001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager.cc (right): https://codereview.chromium.org/23140005/diff/18001/chrome/browser/password_m... chrome/browser/password_manager/password_manager.cc:62: UMA_HISTOGRAM_BOOLEAN("PasswordManager.PasswordManager_Enabled", false); I would think that the name should be "PasswordManager.Enabled". Also, you don't need an if clause, just use |password_manager_enabled| in UMA_HISTOGRAM_BOOLEAN(...); https://codereview.chromium.org/23140005/diff/18001/chrome/browser/password_m... chrome/browser/password_manager/password_manager.cc:234: UMA_HISTOGRAM_ENUMERATION( Unfortunately this doesn't work. If you check out the expansion of UMA_HISTOGRAM_ENUMERATION, it believes the name of the histogram is constant and will store it in a static variable. Read the comments at the top of histogram.h for more information. I'm actually not sure if there is a reasonable way to dynamically generate histogram names, though Ilya might know. https://codereview.chromium.org/23140005/diff/18001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_delegate_impl.cc (right): https://codereview.chromium.org/23140005/diff/18001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:58: PerfTimer timer_; These should be grouped with the other password member variables below. https://codereview.chromium.org/23140005/diff/18001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:130: "PasswordManager.InfobarDislayedTooShortly" + domain_name_, This seems like it would be better as a histogram that displayed the distribution of infobar times (UMA_HISTOGRAM_TIMES) instead of a boolean stat. If you keep it as a boolean, I don't think that "too shortly" is grammatically correct. I would probably say "InfobarDisappearedQuickly". https://codereview.chromium.org/23140005/diff/18001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_util.h (right): https://codereview.chromium.org/23140005/diff/18001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_util.h:12: namespace PasswordManagerUtil { Namespace are lowercase (http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespace_Names) https://codereview.chromium.org/23140005/diff/18001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_util.h:14: const std::string kMonitoredDomainName[] = { Use char* instead of std::string (http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Static_and_Glo...). This should probably also be in the .cc file as I'm assuming you always want callers to access this through IsDomainNameMonitored. https://codereview.chromium.org/23140005/diff/18001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_util.h:27: // Check wether the |url_host| is monitored or not. If yes, we put the "whether" https://codereview.chromium.org/23140005/diff/18001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_util.h:29: bool IsDomainNameMonitored(const std::string& url_host, Should probably have some basic unit testing for this. https://codereview.chromium.org/23140005/diff/18001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_util.h:30: std::string& domain_name); It is very rare for functions in Chrome to use a reference for a mutable parameter (http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Reference_Argu...). Use a pointer instead.
@gcasto @vabr Hi, I made the changes asked, could you please review my code ? @gcasto I added a comment regarding the UMA_HISTOGRAM_NUMERATION. Could you please take a look to it and tell me if I am wrong or not ? Thanks Thanks. https://codereview.chromium.org/23140005/diff/18001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager.cc (right): https://codereview.chromium.org/23140005/diff/18001/chrome/browser/password_m... chrome/browser/password_manager/password_manager.cc:234: UMA_HISTOGRAM_ENUMERATION( On 2013/08/28 01:09:17, Garrett Casto wrote: > Unfortunately this doesn't work. If you check out the expansion of > UMA_HISTOGRAM_ENUMERATION, it believes the name of the histogram is constant and > will store it in a static variable. Read the comments at the top of histogram.h > for more information. > > I'm actually not sure if there is a reasonable way to dynamically generate > histogram names, though Ilya might know. Normally I think it should work because I added a fieldtrial which permit me to append "group name" to each histogram which are listed as "affected-histogram name". https://codereview.chromium.org/23140005/diff/25001/tools/metrics/histograms/.... Am I wrong ?
https://codereview.chromium.org/23140005/diff/25001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_delegate_impl.cc (right): https://codereview.chromium.org/23140005/diff/25001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:57: PerfTimer timer_; As Garrett pointed out, these variables should be declared with the others in the "private:" section, in particular after method definitions. https://codereview.chromium.org/23140005/diff/25001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:128: "PasswordManager.InfobarDisappearedQuickly" + domain_name_, I really liked Garrett's proposal to make this a UMA_HISTOGRAM_TIMES instead of a boolean. That way we don't need to guess kTime in advance. Could you please change the type of this signal to UMA_HISTOGRAM_TIMES, and just pass the elapsed time to it? https://codereview.chromium.org/23140005/diff/25001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_unittest.cc (right): https://codereview.chromium.org/23140005/diff/25001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_unittest.cc:552: ASSERT_TRUE(domain_name == ""); Please also add tests for bare domains ("yahoo.com"), and non-matching suffix equivalent domains ("thisisnotbaidu.com"). https://codereview.chromium.org/23140005/diff/25001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_util.cc (right): https://codereview.chromium.org/23140005/diff/25001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_util.cc:29: if (EndsWith(url_host, kMonitoredDomainName[i], true)) { Currently this would match "notreallygoogle.com" as "google.com". Please make sure that |url_host| either matches kMonitoredDomainName[i] exactly, or it has a dot immediately before the suffix match (".google.com"). https://codereview.chromium.org/23140005/diff/25001/tools/metrics/histograms/... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/23140005/diff/25001/tools/metrics/histograms/... tools/metrics/histograms/histograms.xml:10239: + <summary>Indicates if the password manager is enable.</summary> enable->enabled https://codereview.chromium.org/23140005/diff/25001/tools/metrics/histograms/... tools/metrics/histograms/histograms.xml:10248: + A boolean that indicates whether the save password info bar is displayed. nit: As we are changing from info bar to a bubble soon, maybe change "info bar" to "prompt"? https://codereview.chromium.org/23140005/diff/25001/tools/metrics/histograms/... tools/metrics/histograms/histograms.xml:10260: +<histogram name="PasswordManager.PreviousPasswordSavedRecently"> Is this not used in the code?
https://codereview.chromium.org/23140005/diff/18001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager.cc (right): https://codereview.chromium.org/23140005/diff/18001/chrome/browser/password_m... chrome/browser/password_manager/password_manager.cc:234: UMA_HISTOGRAM_ENUMERATION( On 2013/08/28 15:23:20, jdomingos wrote: > On 2013/08/28 01:09:17, Garrett Casto wrote: > > Unfortunately this doesn't work. If you check out the expansion of > > UMA_HISTOGRAM_ENUMERATION, it believes the name of the histogram is constant > and > > will store it in a static variable. Read the comments at the top of > histogram.h > > for more information. > > > > I'm actually not sure if there is a reasonable way to dynamically generate > > histogram names, though Ilya might know. > > Normally I think it should work because I added a fieldtrial which permit me to > append "group name" to each histogram which are listed as "affected-histogram > name". > https://codereview.chromium.org/23140005/diff/25001/tools/metrics/histograms/.... > > Am I wrong ? I think you pasted the wrong link. Do you mean you did something like UMA_HISTOGRAM_COUNTS(FieldTrial::MakeName("PasswordManager.", "Experiment"), count); This still works because field trials are static across a given run, but in the cases that you have the domain name can change during a run. You can test it out to verify (I think that it will DCHECK()) but it looks like it will fail.
@vabr @gcasto Hi, I changes asked are made, could you please review my code ? @vabr The histogram PasswordManager.PreviousPasswordSavedRecently is not used yet because I don't know how to get this information. Could you please help me to figure it out ? Thanks
https://codereview.chromium.org/23140005/diff/35001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager.cc (right): https://codereview.chromium.org/23140005/diff/35001/chrome/browser/password_m... chrome/browser/password_manager/password_manager.cc:230: domain_name.insert(0, "_"); nit: Why not just include the underscore in the string constant on line 232? https://codereview.chromium.org/23140005/diff/35001/chrome/browser/password_m... chrome/browser/password_manager/password_manager.cc:233: MAX_FAILURE_VALUE); As in password_manager_delegate_impl.cc, you'll need to inline the macro, because the histogram name is not a run-time constant. https://codereview.chromium.org/23140005/diff/35001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_delegate_impl.cc (right): https://codereview.chromium.org/23140005/diff/35001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:83: // signals. This comment doesn't fully reflect the use of the variable, neither in terms of mentioning that it is empty for most domains, nor in mentioning that when it is non-empty, it is prefixed with an underscore. https://codereview.chromium.org/23140005/diff/35001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:123: true); This will not work, for the reasons that the other reviewers mentioned: The UMA_HISTOGRAM macros require the histogram name to be a run-time static constant. Instead, you'll want to inline the macro, similarly to this: https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... https://codereview.chromium.org/23140005/diff/35001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:123: true); This is a little strange, because it causes the meaning of the PasswordManager.InfobarDisplayed histogram to be "whether an infobar was displayed on any website *other* than the ones we are measuring explicitly". IMO you should either always log the PasswordManager.InfobarDisplayed metric, or add a suffix like "_other" for non-tracked domains. Better yet, rather than logging this as N separate histograms, I'd recommend logging this a single enumerated histogram, where each bucket corresponds to a specific tracked domain, plus an additional bucket for "other". https://codereview.chromium.org/23140005/diff/35001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:128: // displayed during enough time for the user to make a decision. I don't understand what this comment means. Could you please try re-wording it to be clearer? It also looks like it should be moved down, so that it is clearly describing the UMA_HISTOGRAM_TIMES macro rather than the _ENUMERATION macro. https://codereview.chromium.org/23140005/diff/35001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:132: "PasswordManager.InfobarLifetime" + domain_name_, As above, you'll need to inline the macro, because the histogram name is not a run-time constant. https://codereview.chromium.org/23140005/diff/35001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:132: "PasswordManager.InfobarLifetime" + domain_name_, As above, it's strange that the un-suffixed PasswordManager.InfobarLifetime histogram does not measure *all* sites, but only those that we are not tracking explicitly via other histograms. https://codereview.chromium.org/23140005/diff/35001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_unittest.cc (right): https://codereview.chromium.org/23140005/diff/35001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_unittest.cc:546: ASSERT_TRUE(domain_name == "linkedin.com"); nit: EXPECT_EQ https://codereview.chromium.org/23140005/diff/35001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_unittest.cc:559: } nit: This test should be placed in a file named password_manager_util_unittest. https://codereview.chromium.org/23140005/diff/35001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_util.cc (right): https://codereview.chromium.org/23140005/diff/35001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_util.cc:13: const char *kMonitoredDomainName[] = { nit: "char*" (swap the position of the asterisk and the space) https://codereview.chromium.org/23140005/diff/35001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_util.cc:22: }; Oof, this seems really questionable from a privacy perspective. *Please* make sure to check with the privacy team whether it's ok to record user visits to specific websites like this. https://codereview.chromium.org/23140005/diff/35001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_util.cc:27: for (std::string::size_type i = 0; i < kMonitoredDomainNameLength; ++i) { nit: You can just use size_t rather than std::string::size_type https://codereview.chromium.org/23140005/diff/35001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_util.cc:33: url_host[point_position] == '.')) { Please use the GURL and url_parse::Parsed classes for parsing URLs. https://codereview.chromium.org/23140005/diff/35001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_util.h (right): https://codereview.chromium.org/23140005/diff/35001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_util.h:6: #define CHROME_BROWSER_PASSWORD_MANAGER_PASSWORD_MANAGER_UTIL_H_ How about password_manager_metrics_util.h? General "util" files tend to devolve into unfocused dumping grounds. https://codereview.chromium.org/23140005/diff/35001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_util.h:14: // Check whether the |url_host| is monitored or not. If yes, we put the nit: "monitored or not" -> "monitored for metrics" https://codereview.chromium.org/23140005/diff/35001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_util.h:15: // domain name into |domain_name| nit: End the sentence with a period. https://codereview.chromium.org/23140005/diff/35001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_util.h:17: std::string* domain_name); Seems like you might want to annotate this with WARN_UNUSED_RESULT. If not, you might as well just drop the boolean return value, and simply return the domain_name, or an empty string if the domain name isn't one we're explicitly monitoring.
Plus some additional comments on histograms.xml: https://codereview.chromium.org/23140005/diff/35001/tools/metrics/histograms/... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/23140005/diff/35001/tools/metrics/histograms/... tools/metrics/histograms/histograms.xml:10239: + <summary>Indicates if the password manager is enabled.</summary> Please specify when this is logged. https://codereview.chromium.org/23140005/diff/35001/tools/metrics/histograms/... tools/metrics/histograms/histograms.xml:10244: + A boolean that indicates whether the save password prompt is displayed. This is a confusing description, as the histogram can never emit any "false" values, only "true" ones. https://codereview.chromium.org/23140005/diff/35001/tools/metrics/histograms/... tools/metrics/histograms/histograms.xml:10248: +<histogram name="PasswordManager.InfobarLifetime"> nit: Please specify the units. https://codereview.chromium.org/23140005/diff/35001/tools/metrics/histograms/... tools/metrics/histograms/histograms.xml:10249: + <summary>Indicates the lifetime of the save password prompt.</summary> nit: Please reword this to be a little bit clearer; for example, something along the lines of: "The time elapsed between when a "Save password?" infobar was shown and when it was hidden. The infobar can be hidden either as a result of user interaction with the infobar, or automatically when the page is navigated or the tab is closed." https://codereview.chromium.org/23140005/diff/35001/tools/metrics/histograms/... tools/metrics/histograms/histograms.xml:10265: +</histogram> Hmm, I don't see this histogram being logged in the C++ code... https://codereview.chromium.org/23140005/diff/35001/tools/metrics/histograms/... tools/metrics/histograms/histograms.xml:25349: + <group name="amazon.com" label="amazon"/> Looking at the code, it seems like you should also have a group with an empty name.
https://codereview.chromium.org/23140005/diff/35001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_util.cc (right): https://codereview.chromium.org/23140005/diff/35001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_util.cc:22: }; On 2013/08/29 06:42:21, Ilya Sherman wrote: > Oof, this seems really questionable from a privacy perspective. *Please* make > sure to check with the privacy team whether it's ok to record user visits to > specific websites like this. I can confirm that somebody other than me will be reviewing this on the Privacy team. It is being tracked for M31 reviews under http://crbug.com/268980 (I Cc-ed Ilya for access).
https://codereview.chromium.org/23140005/diff/35001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_util.cc (right): https://codereview.chromium.org/23140005/diff/35001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_util.cc:22: }; On 2013/08/29 08:15:13, vabr (Chromium) wrote: > On 2013/08/29 06:42:21, Ilya Sherman wrote: > > Oof, this seems really questionable from a privacy perspective. *Please* make > > sure to check with the privacy team whether it's ok to record user visits to > > specific websites like this. > > I can confirm that somebody other than me will be reviewing this on the Privacy > team. > It is being tracked for M31 reviews under http://crbug.com/268980 (I Cc-ed Ilya > for access). I'm not comfortable approving this change prior to it being reviewed by the privacy team.
@vabr @isherman Hi, I made the changes asked, could you please review my code ? Thanks https://codereview.chromium.org/23140005/diff/35001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager.cc (right): https://codereview.chromium.org/23140005/diff/35001/chrome/browser/password_m... chrome/browser/password_manager/password_manager.cc:230: domain_name.insert(0, "_"); On 2013/08/29 06:42:21, Ilya Sherman wrote: > nit: Why not just include the underscore in the string constant on line 232? Done. https://codereview.chromium.org/23140005/diff/35001/chrome/browser/password_m... chrome/browser/password_manager/password_manager.cc:233: MAX_FAILURE_VALUE); On 2013/08/29 06:42:21, Ilya Sherman wrote: > As in password_manager_delegate_impl.cc, you'll need to inline the macro, > because the histogram name is not a run-time constant. Done. https://codereview.chromium.org/23140005/diff/35001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_delegate_impl.cc (right): https://codereview.chromium.org/23140005/diff/35001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:83: // signals. On 2013/08/29 06:42:21, Ilya Sherman wrote: > This comment doesn't fully reflect the use of the variable, neither in terms of > mentioning that it is empty for most domains, nor in mentioning that when it is > non-empty, it is prefixed with an underscore. Done. https://codereview.chromium.org/23140005/diff/35001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:123: true); On 2013/08/29 06:42:21, Ilya Sherman wrote: > This is a little strange, because it causes the meaning of the > PasswordManager.InfobarDisplayed histogram to be "whether an infobar was > displayed on any website *other* than the ones we are measuring explicitly". > IMO you should either always log the PasswordManager.InfobarDisplayed metric, or > add a suffix like "_other" for non-tracked domains. > > Better yet, rather than logging this as N separate histograms, I'd recommend > logging this a single enumerated histogram, where each bucket corresponds to a > specific tracked domain, plus an additional bucket for "other". Hi Ilya, Indeed regarding the PasswordManager.InfobarDisplayed, we just want this information for monitored site. I moved the UMA_HISTOGRAM_BOOLEAN in the if block. Regarding the N separate histograms, this will work for this signal but not for the other ones present in the fieldtrial because they are already enumeration. https://codereview.chromium.org/23140005/diff/35001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:123: true); On 2013/08/29 06:42:21, Ilya Sherman wrote: > This will not work, for the reasons that the other reviewers mentioned: The > UMA_HISTOGRAM macros require the histogram name to be a run-time static > constant. Instead, you'll want to inline the macro, similarly to this: > https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... Done. https://codereview.chromium.org/23140005/diff/35001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:128: // displayed during enough time for the user to make a decision. On 2013/08/29 06:42:21, Ilya Sherman wrote: > I don't understand what this comment means. Could you please try re-wording it > to be clearer? It also looks like it should be moved down, so that it is > clearly describing the UMA_HISTOGRAM_TIMES macro rather than the _ENUMERATION > macro. The comment here was for a previous variable which I deleted. Sorry for it. https://codereview.chromium.org/23140005/diff/35001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:132: "PasswordManager.InfobarLifetime" + domain_name_, On 2013/08/29 06:42:21, Ilya Sherman wrote: > As above, it's strange that the un-suffixed PasswordManager.InfobarLifetime > histogram does not measure *all* sites, but only those that we are not tracking > explicitly via other histograms. You are right, as above this is not the expected behavior. https://codereview.chromium.org/23140005/diff/35001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:132: "PasswordManager.InfobarLifetime" + domain_name_, On 2013/08/29 06:42:21, Ilya Sherman wrote: > As above, you'll need to inline the macro, because the histogram name is not a > run-time constant. Done. https://codereview.chromium.org/23140005/diff/35001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_unittest.cc (right): https://codereview.chromium.org/23140005/diff/35001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_unittest.cc:546: ASSERT_TRUE(domain_name == "linkedin.com"); On 2013/08/29 06:42:21, Ilya Sherman wrote: > nit: EXPECT_EQ Done. https://codereview.chromium.org/23140005/diff/35001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_unittest.cc:559: } On 2013/08/29 06:42:21, Ilya Sherman wrote: > nit: This test should be placed in a file named password_manager_util_unittest. Done. https://codereview.chromium.org/23140005/diff/35001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_util.cc (right): https://codereview.chromium.org/23140005/diff/35001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_util.cc:13: const char *kMonitoredDomainName[] = { On 2013/08/29 06:42:21, Ilya Sherman wrote: > nit: "char*" (swap the position of the asterisk and the space) Done. https://codereview.chromium.org/23140005/diff/35001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_util.cc:27: for (std::string::size_type i = 0; i < kMonitoredDomainNameLength; ++i) { On 2013/08/29 06:42:21, Ilya Sherman wrote: > nit: You can just use size_t rather than std::string::size_type Done. https://codereview.chromium.org/23140005/diff/35001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_util.cc:33: url_host[point_position] == '.')) { On 2013/08/29 06:42:21, Ilya Sherman wrote: > Please use the GURL and url_parse::Parsed classes for parsing URLs. I don't understand what you want me to do here. Could you please add additional details please ? https://codereview.chromium.org/23140005/diff/35001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_util.h (right): https://codereview.chromium.org/23140005/diff/35001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_util.h:6: #define CHROME_BROWSER_PASSWORD_MANAGER_PASSWORD_MANAGER_UTIL_H_ On 2013/08/29 06:42:21, Ilya Sherman wrote: > How about password_manager_metrics_util.h? General "util" files tend to devolve > into unfocused dumping grounds. Done. https://codereview.chromium.org/23140005/diff/35001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_util.h:14: // Check whether the |url_host| is monitored or not. If yes, we put the On 2013/08/29 06:42:21, Ilya Sherman wrote: > nit: "monitored or not" -> "monitored for metrics" Done. https://codereview.chromium.org/23140005/diff/35001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_util.h:15: // domain name into |domain_name| On 2013/08/29 06:42:21, Ilya Sherman wrote: > nit: End the sentence with a period. Done. https://codereview.chromium.org/23140005/diff/35001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_util.h:17: std::string* domain_name); On 2013/08/29 06:42:21, Ilya Sherman wrote: > Seems like you might want to annotate this with WARN_UNUSED_RESULT. If not, you > might as well just drop the boolean return value, and simply return the > domain_name, or an empty string if the domain name isn't one we're explicitly > monitoring. Done. https://codereview.chromium.org/23140005/diff/35001/tools/metrics/histograms/... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/23140005/diff/35001/tools/metrics/histograms/... tools/metrics/histograms/histograms.xml:10239: + <summary>Indicates if the password manager is enabled.</summary> On 2013/08/29 06:46:44, Ilya Sherman wrote: > Please specify when this is logged. Done. https://codereview.chromium.org/23140005/diff/35001/tools/metrics/histograms/... tools/metrics/histograms/histograms.xml:10244: + A boolean that indicates whether the save password prompt is displayed. On 2013/08/29 06:46:44, Ilya Sherman wrote: > This is a confusing description, as the histogram can never emit any "false" > values, only "true" ones. Done. https://codereview.chromium.org/23140005/diff/35001/tools/metrics/histograms/... tools/metrics/histograms/histograms.xml:10249: + <summary>Indicates the lifetime of the save password prompt.</summary> On 2013/08/29 06:46:44, Ilya Sherman wrote: > nit: Please reword this to be a little bit clearer; for example, something along > the lines of: "The time elapsed between when a "Save password?" infobar was > shown and when it was hidden. The infobar can be hidden either as a result of > user interaction with the infobar, or automatically when the page is navigated > or the tab is closed." Done. https://codereview.chromium.org/23140005/diff/35001/tools/metrics/histograms/... tools/metrics/histograms/histograms.xml:10265: +</histogram> On 2013/08/29 06:46:44, Ilya Sherman wrote: > Hmm, I don't see this histogram being logged in the C++ code... Done. https://codereview.chromium.org/23140005/diff/35001/tools/metrics/histograms/... tools/metrics/histograms/histograms.xml:25349: + <group name="amazon.com" label="amazon"/> On 2013/08/29 06:46:44, Ilya Sherman wrote: > Looking at the code, it seems like you should also have a group with an empty > name. Only the PasswordManager.ProvisionalSaveFailure needs an empty group name. Are you sure that I should add it ?
https://codereview.chromium.org/23140005/diff/35001/tools/metrics/histograms/... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/23140005/diff/35001/tools/metrics/histograms/... tools/metrics/histograms/histograms.xml:25349: + <group name="amazon.com" label="amazon"/> On 2013/08/30 21:09:20, jdomingos wrote: > On 2013/08/29 06:46:44, Ilya Sherman wrote: > > Looking at the code, it seems like you should also have a group with an empty > > name. > > Only the PasswordManager.ProvisionalSaveFailure needs an empty group name. Are > you sure that I should add it ? If you don't include the empty group name, then I'm pretty sure the histogram will not appear on the dashboard with just the base name, i.e. you won't see the data for the PasswordManager.ProvisionalSaveFailure not broken down by domain name. https://codereview.chromium.org/23140005/diff/45001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager.cc (right): https://codereview.chromium.org/23140005/diff/45001/chrome/browser/password_m... chrome/browser/password_manager/password_manager.cc:230: if (domain_name.size()) { nit: if (!domain_name.empty()) https://codereview.chromium.org/23140005/diff/45001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_delegate_impl.cc (right): https://codereview.chromium.org/23140005/diff/45001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:85: // it is empty. nit: I find this comment rather hard to parse. One possible rephrasing: "The domain name corresponding to |form_to_save_| if the form is on a monitored domain. Otherwise, an empty string." (See also my comment below about dropping the underscore prefix.) https://codereview.chromium.org/23140005/diff/45001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:86: std::string domain_name_; nit: Perhaps rename this to |monitored_domain_name_|? https://codereview.chromium.org/23140005/diff/45001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:122: if (domain_name_.size()) { nit: if (!domain_name_.empty()) https://codereview.chromium.org/23140005/diff/45001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:123: domain_name_.insert(0, "_"); Since all of the histogram logging code in this file now only logs data for non-empty domain names, IMO it would be cleaner to include the underscore in the histogram names rather than prepending it to the domain_name_. https://codereview.chromium.org/23140005/diff/45001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:133: if (domain_name_.size()) { nit: if (!domain_name_.empty()) https://codereview.chromium.org/23140005/diff/45001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_metrics_util.cc (right): https://codereview.chromium.org/23140005/diff/45001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:12: const char* kMonitoredDomainName[] = { nit: "const char* const" https://codereview.chromium.org/23140005/diff/45001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:35: return domain_name; Please write this as: GURL url(url_host); for (size_t i = 0; i < kMonitoredDomainNameLength; ++i) { std::string domain = kMonitoredDomainName[i]; if (url_host.DomainIs(domain.c_str())) return domain; } return std::string(); https://codereview.chromium.org/23140005/diff/45001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_metrics_util.h (right): https://codereview.chromium.org/23140005/diff/45001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:11: #include "base/metrics/histogram.h" nit: Please move this into the .cc file. https://codereview.chromium.org/23140005/diff/45001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:16: // domain name into |domain_name| Please update this comment. https://codereview.chromium.org/23140005/diff/45001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:28: const base::TimeDelta& duration); nit: You'll need to forward-declare base::TimeDelta, and #include time.h in the .cc file. https://codereview.chromium.org/23140005/diff/45001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_metrics_util_unittest.cc (right): https://codereview.chromium.org/23140005/diff/45001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util_unittest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. nit: 2013. Also, please omit the "(c)". https://codereview.chromium.org/23140005/diff/45001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util_unittest.cc:5: #include "chrome/browser/password_manager/password_manager_metrics_util.h" nit: Please include a blank line after this one. https://codereview.chromium.org/23140005/diff/45001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util_unittest.cc:6: #include "testing/gmock/include/gmock/gmock.h" nit: This is not needed. https://codereview.chromium.org/23140005/diff/45001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util_unittest.cc:9: TEST_F(PasswordManagerTest, IsDomainNameMonitoredTest) { nit: TEST, not TEST_F. https://codereview.chromium.org/23140005/diff/45001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util_unittest.cc:26: EXPECT_EQ(domain_name == ""); Please update this test code so that it compiles with the latest API.
https://codereview.chromium.org/23140005/diff/45001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_metrics_util_unittest.cc (right): https://codereview.chromium.org/23140005/diff/45001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util_unittest.cc:26: EXPECT_EQ(domain_name == ""); On 2013/08/30 22:34:42, Ilya Sherman wrote: > Please update this test code so that it compiles with the latest API. Hi Ilya, What should I change in order to make it compiles with the latest API ? Thanks.
https://codereview.chromium.org/23140005/diff/45001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_metrics_util_unittest.cc (right): https://codereview.chromium.org/23140005/diff/45001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util_unittest.cc:26: EXPECT_EQ(domain_name == ""); On 2013/08/30 22:57:17, jdomingos wrote: > On 2013/08/30 22:34:42, Ilya Sherman wrote: > > Please update this test code so that it compiles with the latest API. > > Hi Ilya, > > What should I change in order to make it compiles with the latest API ? IsDomainNameMonitored() no longer returns a boolean, and you're using incorrect syntax for EXPECT_EQ. Try compiling the code, the compiler will tell you what fails to compile ;)
https://codereview.chromium.org/23140005/diff/45001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_metrics_util_unittest.cc (right): https://codereview.chromium.org/23140005/diff/45001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util_unittest.cc:26: EXPECT_EQ(domain_name == ""); On 2013/08/30 23:00:47, Ilya Sherman wrote: > On 2013/08/30 22:57:17, jdomingos wrote: > > On 2013/08/30 22:34:42, Ilya Sherman wrote: > > > Please update this test code so that it compiles with the latest API. > > > > Hi Ilya, > > > > What should I change in order to make it compiles with the latest API ? > > IsDomainNameMonitored() no longer returns a boolean, and you're using incorrect > syntax for EXPECT_EQ. Try compiling the code, the compiler will tell you what > fails to compile ;) Indeed it is obvious that it doesn't work now. Thank you :) .
@vabr @isherman Hi, I made the changes asked could you please review my code please ? https://codereview.chromium.org/23140005/diff/45001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:12: const char* kMonitoredDomainName[] = { On 2013/08/30 22:34:42, Ilya Sherman wrote: > nit: "const char* const" Hi Ilya, I am wondering if the const you want me to add is necessary. Indeed this array can't be modified because it's stored in a read-only location. Thanks https://codereview.chromium.org/23140005/diff/45001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager.cc (right): https://codereview.chromium.org/23140005/diff/45001/chrome/browser/password_m... chrome/browser/password_manager/password_manager.cc:230: if (domain_name.size()) { On 2013/08/30 22:34:42, Ilya Sherman wrote: > nit: if (!domain_name.empty()) Done. https://codereview.chromium.org/23140005/diff/45001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_delegate_impl.cc (right): https://codereview.chromium.org/23140005/diff/45001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:85: // it is empty. On 2013/08/30 22:34:42, Ilya Sherman wrote: > nit: I find this comment rather hard to parse. One possible rephrasing: "The > domain name corresponding to |form_to_save_| if the form is on a monitored > domain. Otherwise, an empty string." (See also my comment below about dropping > the underscore prefix.) Done. https://codereview.chromium.org/23140005/diff/45001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:86: std::string domain_name_; On 2013/08/30 22:34:42, Ilya Sherman wrote: > nit: Perhaps rename this to |monitored_domain_name_|? Done. https://codereview.chromium.org/23140005/diff/45001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:122: if (domain_name_.size()) { On 2013/08/30 22:34:42, Ilya Sherman wrote: > nit: if (!domain_name_.empty()) Done. https://codereview.chromium.org/23140005/diff/45001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:123: domain_name_.insert(0, "_"); On 2013/08/30 22:34:42, Ilya Sherman wrote: > Since all of the histogram logging code in this file now only logs data for > non-empty domain names, IMO it would be cleaner to include the underscore in the > histogram names rather than prepending it to the domain_name_. Done. https://codereview.chromium.org/23140005/diff/45001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:133: if (domain_name_.size()) { On 2013/08/30 22:34:42, Ilya Sherman wrote: > nit: if (!domain_name_.empty()) Done. https://codereview.chromium.org/23140005/diff/45001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_metrics_util.cc (right): https://codereview.chromium.org/23140005/diff/45001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:12: const char* kMonitoredDomainName[] = { On 2013/08/30 22:34:42, Ilya Sherman wrote: > nit: "const char* const" Hi Ilya, I am wondering if the const you want me to add is necessary. Indeed this array can't be modified because it's stored in a read-only location. Thanks. https://codereview.chromium.org/23140005/diff/45001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:35: return domain_name; On 2013/08/30 22:34:42, Ilya Sherman wrote: > Please write this as: > > GURL url(url_host); > for (size_t i = 0; i < kMonitoredDomainNameLength; ++i) { > std::string domain = kMonitoredDomainName[i]; > if (url_host.DomainIs(domain.c_str())) > return domain; > } > return std::string(); Done. https://codereview.chromium.org/23140005/diff/45001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_metrics_util.h (right): https://codereview.chromium.org/23140005/diff/45001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:11: #include "base/metrics/histogram.h" On 2013/08/30 22:34:42, Ilya Sherman wrote: > nit: Please move this into the .cc file. Done. https://codereview.chromium.org/23140005/diff/45001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:16: // domain name into |domain_name| On 2013/08/30 22:34:42, Ilya Sherman wrote: > Please update this comment. Done. https://codereview.chromium.org/23140005/diff/45001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:28: const base::TimeDelta& duration); On 2013/08/30 22:34:42, Ilya Sherman wrote: > nit: You'll need to forward-declare base::TimeDelta, and #include time.h in the > .cc file. Done. https://codereview.chromium.org/23140005/diff/45001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_metrics_util_unittest.cc (right): https://codereview.chromium.org/23140005/diff/45001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util_unittest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/08/30 22:34:42, Ilya Sherman wrote: > nit: 2013. Also, please omit the "(c)". Done. https://codereview.chromium.org/23140005/diff/45001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util_unittest.cc:5: #include "chrome/browser/password_manager/password_manager_metrics_util.h" On 2013/08/30 22:34:42, Ilya Sherman wrote: > nit: Please include a blank line after this one. Done. https://codereview.chromium.org/23140005/diff/45001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util_unittest.cc:6: #include "testing/gmock/include/gmock/gmock.h" On 2013/08/30 22:34:42, Ilya Sherman wrote: > nit: This is not needed. Done. https://codereview.chromium.org/23140005/diff/45001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util_unittest.cc:9: TEST_F(PasswordManagerTest, IsDomainNameMonitoredTest) { On 2013/08/30 22:34:42, Ilya Sherman wrote: > nit: TEST, not TEST_F. Done.
Thanks. LG other than the remaining question about the privacy aspects of this change, which are being discussed in the related bug. https://codereview.chromium.org/23140005/diff/45001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_metrics_util.cc (right): https://codereview.chromium.org/23140005/diff/45001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:12: const char* kMonitoredDomainName[] = { On 2013/09/02 09:58:45, jdomingos wrote: > On 2013/08/30 22:34:42, Ilya Sherman wrote: > > nit: "const char* const" > > Hi Ilya, > > I am wondering if the const you want me to add is necessary. > Indeed this array can't be modified because it's stored in a read-only location. > > Thanks. The second const prevents you from writing code like kMonitoredDomainName[2] = "hello world"; https://codereview.chromium.org/23140005/diff/55001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_metrics_util.cc (right): https://codereview.chromium.org/23140005/diff/55001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:28: DLOG(INFO) << url_host; Please remove this logging code prior to committing the CL. https://codereview.chromium.org/23140005/diff/55001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_metrics_util.h (right): https://codereview.chromium.org/23140005/diff/55001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:17: // the domain name otherwise we return an empty. nit: "we return" -> "returns" https://codereview.chromium.org/23140005/diff/55001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:17: // the domain name otherwise we return an empty. nit: "an empty" -> "an empty string". https://codereview.chromium.org/23140005/diff/55001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_metrics_util_unittest.cc (right): https://codereview.chromium.org/23140005/diff/55001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util_unittest.cc:14: EXPECT_EQ(domain_name, "linkedin.com"); nit: Pretty sure that the order of the arguments should be swapped, i.e. this should be written as EXPECT_EQ("linkedin.com", domain_name), for failure messages to print correctly. You can double-check me on that by compiling locally with an incorrect expectation, and seeing what's output by the test suite. This comment applies to all of the EXPECT_EQ's in this file.
@vabr @isherman Hi, I made the changes asked regarding the comments and the privacy review. Now each website is contained into multiple buckets. Could you please review my code ? Thanks.
@vabr @isherman Hi, I made the changes asked regarding the comments and the privacy review. Now each website is contained into multiple buckets. Could you please review my code ? Thanks.
On 2013/09/09 18:04:07, jdomingos wrote: > @vabr @isherman > > Hi, > > I made the changes asked regarding the comments and the privacy review. Now each > website is contained into multiple buckets. > Could you please review my code ? > > Thanks. I don't see any updates in the privacy review bug indicating the privacy team's explicit approval for any concrete approach. I'd really like to get that nailed down prior to reviewing the specific code changes, as it will have considerable impact on what the code ought to look like.
Hi Jordy, A couple of comments below. As far as I understood the most recent discussion on the privacy review, we should be good to go with the groups of hosts. Cheers, Vaclav https://codereview.chromium.org/23140005/diff/67001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_metrics_util.cc (right): https://codereview.chromium.org/23140005/diff/67001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:20: const struct MonitoredWebsiteInfos kInfos[] = { This should be also "static", otherwise I believe the whole table is created on the stack during each call to IsDomainNameMonitored. https://codereview.chromium.org/23140005/diff/67001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:41: kInfos[i].bucketsID[base::GetCurrentProcId() % kInfosLength]); Please do not use |kInfosLength| here! The fact that both kInfosLength and the size of the bucketsID arrays is always the same (10) is a coincidence. If anyone changed those parameters in the future, we might overflow the array's limits. Instead, please introduce also something like // The common size of the bucket_ids part of each element in // |kInfosLength|. static const size_t kBucketsPerDomain = 10u; right below (or above) kInfosLength. https://codereview.chromium.org/23140005/diff/67001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:41: kInfos[i].bucketsID[base::GetCurrentProcId() % kInfosLength]); Using GetCurrentProcId() should also have a comment explaining why we are using this and what would an alternative need to satisfy. I suggest: // Use GetCurrentProcId % kBucketsPerDomain to get a number which: // 1. Is stable during the lifetime of the browser. // 2. Is sufficiently random, in both the sense that // 2a. it's not easy to predict for a specific user, and // 2b. all kBucketsPerDomain possible values are roughly uniformly // used across all users. https://codereview.chromium.org/23140005/diff/67001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:47: void LogUMAHistogramEnumeration(const std::string& name, (Dear OWNERS, any guidance on the following?) Looking at all the LogUMAHistogram* functions, I'm thinking how to avoid code duplication (against the originals in components/autofill/core/browser/autofill_metrics.cc). You use these functions in a code from chrome/chrome_browser.gypi:browser, which already lists a couple of dependencies from ../components/components.gyp:... The originals of LogUMAHistogram* are in ../components/components.gyp:autofill. You can either add that to the dependencies of chrome/chrome_browser.gypi:browser, or separate those LogUMAHistogram* functions in their own directory and target and only add that (see the startup_metric_utils for an example). https://codereview.chromium.org/23140005/diff/67001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_metrics_util.h (right): https://codereview.chromium.org/23140005/diff/67001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:16: struct MonitoredWebsiteInfos { nit: Could we have a more descriptive name? "Infos" does not tell much. What about DomainBucketsPair or something similar? https://codereview.chromium.org/23140005/diff/67001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:18: int const bucketsID[10]; hacker_style for member variables. Also (nit) -- the plural should be with IDs not buckets. So suggesting: bucket_ids https://codereview.chromium.org/23140005/diff/67001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:43: void LogUMAHistogramBoolean(const std::string& name, bool sample); Looks like this function is not used. If so, then we should better remove it. https://codereview.chromium.org/23140005/diff/67001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_metrics_util_unittest.cc (right): https://codereview.chromium.org/23140005/diff/67001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util_unittest.cc:20: EXPECT_EQ("", domain_name); nit: EXPECT_TRUE(domain_name.empty()) that would probably be more efficient, and consistent with the EXPECT_FALSE statements here. (also below) https://codereview.chromium.org/23140005/diff/67001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/23140005/diff/67001/chrome/chrome_browser.gyp... chrome/chrome_browser.gypi:1300: 'browser/password_manager/password_manager.h', Please remove the added whitespace. https://codereview.chromium.org/23140005/diff/67001/tools/metrics/histograms/... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/23140005/diff/67001/tools/metrics/histograms/... tools/metrics/histograms/histograms.xml:27093: + <group name="google.com" label="google"/> We also need to update those to use "bucketXY" suffixes, right?
@vabr @gcasto @isherman Hi ! I made the changes asked could you please review my code ? Thanks. https://codereview.chromium.org/23140005/diff/67001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_metrics_util.cc (right): https://codereview.chromium.org/23140005/diff/67001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:20: const struct MonitoredWebsiteInfos kInfos[] = { On 2013/09/10 08:46:53, vabr (Chromium) wrote: > This should be also "static", otherwise I believe the whole table is created on > the stack during each call to IsDomainNameMonitored. Done. https://codereview.chromium.org/23140005/diff/67001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:41: kInfos[i].bucketsID[base::GetCurrentProcId() % kInfosLength]); On 2013/09/10 08:46:53, vabr (Chromium) wrote: > Please do not use |kInfosLength| here! > The fact that both kInfosLength and the size of the bucketsID arrays is always > the same (10) is a coincidence. If anyone changed those parameters in the > future, we might overflow the array's limits. > > Instead, please introduce also something like > > // The common size of the bucket_ids part of each element in > // |kInfosLength|. > static const size_t kBucketsPerDomain = 10u; > > right below (or above) kInfosLength. Done. https://codereview.chromium.org/23140005/diff/67001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:41: kInfos[i].bucketsID[base::GetCurrentProcId() % kInfosLength]); On 2013/09/10 08:46:53, vabr (Chromium) wrote: > Using GetCurrentProcId() should also have a comment explaining why we are using > this and what would an alternative need to satisfy. I suggest: > > // Use GetCurrentProcId % kBucketsPerDomain to get a number which: > // 1. Is stable during the lifetime of the browser. > // 2. Is sufficiently random, in both the sense that > // 2a. it's not easy to predict for a specific user, and > // 2b. all kBucketsPerDomain possible values are roughly uniformly > // used across all users. Done. https://codereview.chromium.org/23140005/diff/67001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_metrics_util.h (right): https://codereview.chromium.org/23140005/diff/67001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:16: struct MonitoredWebsiteInfos { On 2013/09/10 08:46:53, vabr (Chromium) wrote: > nit: Could we have a more descriptive name? "Infos" does not tell much. What > about DomainBucketsPair or something similar? Done. https://codereview.chromium.org/23140005/diff/67001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:18: int const bucketsID[10]; On 2013/09/10 08:46:53, vabr (Chromium) wrote: > hacker_style for member variables. Also (nit) -- the plural should be with IDs > not buckets. So suggesting: > bucket_ids Done. https://codereview.chromium.org/23140005/diff/67001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:43: void LogUMAHistogramBoolean(const std::string& name, bool sample); On 2013/09/10 08:46:53, vabr (Chromium) wrote: > Looks like this function is not used. If so, then we should better remove it. Done. https://codereview.chromium.org/23140005/diff/67001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_metrics_util_unittest.cc (right): https://codereview.chromium.org/23140005/diff/67001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util_unittest.cc:20: EXPECT_EQ("", domain_name); On 2013/09/10 08:46:53, vabr (Chromium) wrote: > nit: > EXPECT_TRUE(domain_name.empty()) > > that would probably be more efficient, and consistent with the EXPECT_FALSE > statements here. > > (also below) Done. https://codereview.chromium.org/23140005/diff/67001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/23140005/diff/67001/chrome/chrome_browser.gyp... chrome/chrome_browser.gypi:1300: 'browser/password_manager/password_manager.h', On 2013/09/10 08:46:53, vabr (Chromium) wrote: > Please remove the added whitespace. Done. https://codereview.chromium.org/23140005/diff/67001/tools/metrics/histograms/... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/23140005/diff/67001/tools/metrics/histograms/... tools/metrics/histograms/histograms.xml:27093: + <group name="google.com" label="google"/> On 2013/09/10 08:46:53, vabr (Chromium) wrote: > We also need to update those to use "bucketXY" suffixes, right? Done.
https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager.cc (right): https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager.cc:224: const std::string& url_host) { Optional nit: Perhaps name this variable something more like |form_origin| or |form_domain|? https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager.cc:229: bucket_name = password_manager_metrics_util::IsDomainNameMonitored(url_host); nit: Why is the assignment on a separate line from the variable declaration? https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager.h (right): https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager.h:116: // Log failure for UMA Please document the new parameter. https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_delegate_impl.cc (right): https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:85: std::string bucket_name_; nit: Perhaps name this |uma_histogram_suffix_| or |domain_type_for_metrics_| or something a little more descriptive like these two suggestions. https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:124: 1); It's wasteful to allocate 50 buckets and only use one of them as you are doing here. Please either create a single enumerated histogram with one bucket per category, or use fewer buckets per histogram. https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:129: nit: Spurious newline. https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:138: timer_.Elapsed()); Why is this histogram tracker per-category, rather than having just a single histogram for all possible infobars? Are you really trying to measure a derived signal that can be inferred from the timer value -- e.g. did the infobar disappear before the user could act on it -- that could be measured as a boolean histogram on the client rather than needing to fill out the complete time distribution? https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_metrics_util.cc (right): https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:18: static const size_t kBucketsPerDomain = 10u; This terminology is a little confusing, as "buckets" already has a meaning within the context of histograms -- a single histogram has multiple buckets. I'd recommend using a different term to describe the association of domains with ids -- perhaps "groups" or "sets" or something like that. https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:21: static const struct DomainBucketsPair kInfos[] = { nit: Extraneous "struct" https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:21: static const struct DomainBucketsPair kInfos[] = { nit: Perhaps |kDomainMapping|? https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:31: {"tumblr.com", {4, 8, 10, 12, 13, 15, 16, 18, 19, 20}}, This bucket distribution doesn't really seem well balanced -- I'd expect each bucket to appear an equal number of times, but '1' appears 5 times, whereas '3' only appears 4 times. https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:33: const size_t kInfosLength = arraysize(kInfos); This needs a lot more documentation. Please explain the algorithm: what the bucket ids mean, how they were chosen, etc. https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:33: const size_t kInfosLength = arraysize(kInfos); Suppose we wanted to add tracking for an additional website -- say, facebook (incidentally, why isn't that already included?). Would there be a way to add the new website while keeping the bucket ids stable? If not, that seems like a pretty serious flaw with this design, as the histogram meanings will need to change every time we change which set of websites we're interested in tracking. https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:45: // used across all users. Why not just compute a truly random number once per run, by caching the number in a static variable? https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:49: .bucket_ids[base::GetCurrentProcId() % kBucketsPerDomain]); Rather than splitting this statement over four lines, please introduce a temporary named variable and write it as two statements. https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_metrics_util.h (right): https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:17: char const* domain_name; char const* char https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:18: int const bucket_ids[10]; Why does each domain have 10 bucket ids? https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:19: }; This doesn't seem to be used anywhere else in the header file, so please move it into the implementation file. https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:19: }; Please document this struct. https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:24: std::string IsDomainNameMonitored(const std::string& url_host); This method name suggests that the method returns a boolean, which is no longer true. Please update the name to match the current semantics. https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:37: // A version of the UMA_HISTOGRAM_COUNT macro that allows the |name| nit: I think you mean UMA_HISTOGRAM_COUNTS. Please fix that here and below. https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_metrics_util_unittest.cc (right): https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util_unittest.cc:27: } If you keep the bucketization strategy you've implemented here, please add some test coverage to verify that it is working correctly, if possible. This might mean you need to mock out the source of randomness somehow for testing, since it's fairly important to test that (a) a single website can land in multiple buckets and (b) a single website can't land in too many different buckets and (c) buckets are "equally weighted" in some sense. https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_unittest.cc (right): https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_unittest.cc:13: #include "chrome/browser/password_manager/password_manager_metrics_util.h" nit: Not needed.
https://codereview.chromium.org/23140005/diff/67001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_metrics_util.cc (right): https://codereview.chromium.org/23140005/diff/67001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:47: void LogUMAHistogramEnumeration(const std::string& name, On 2013/09/10 08:46:53, vabr (Chromium) wrote: > (Dear OWNERS, any guidance on the following?) > > Looking at all the LogUMAHistogram* functions, I'm thinking how to avoid code > duplication (against the originals in > components/autofill/core/browser/autofill_metrics.cc). > > You use these functions in a code from chrome/chrome_browser.gypi:browser, which > already lists a couple of dependencies from ../components/components.gyp:... > The originals of LogUMAHistogram* are in ../components/components.gyp:autofill. > You can either add that to the dependencies of > chrome/chrome_browser.gypi:browser, or separate those LogUMAHistogram* functions > in their own directory and target and only add that (see the > startup_metric_utils for an example). The most appropriate place for these would be in base/metrics, if we want to reduce the amount of code duplication.
Hi Jordy, I mostly went through Ilya's comments, and will make one more pass to review your code later today, ideally after you incorporate most of Ilya's comments. I'll need to rush out soon, but will be back in the afternoon in case you want to talk about anything (new tests, what to add to the design doc, etc.). Cheers, Vaclav https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_metrics_util.cc (right): https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:31: {"tumblr.com", {4, 8, 10, 12, 13, 15, 16, 18, 19, 20}}, On 2013/09/10 22:30:54, Ilya Sherman wrote: > This bucket distribution doesn't really seem well balanced -- I'd expect each > bucket to appear an equal number of times, but '1' appears 5 times, whereas '3' > only appears 4 times. Good catch, Ilya! ebay.com should have started with 3, not 13. Jordy, can you please double-check for other typos? The way we generated this, each bucket should have appeared 5 times (in other words, each bucket contains exactly 5 hosts). https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:33: const size_t kInfosLength = arraysize(kInfos); On 2013/09/10 22:30:54, Ilya Sherman wrote: > This needs a lot more documentation. Please explain the algorithm: what the > bucket ids mean, how they were chosen, etc. Jordy is putting all of it in the design doc (currently https://docs.google.com/a/google.com/document/d/1AEeaQGxKy0sKhZENTH1WWf8K7sfk..., but the algorithm is not yet there), and we should put a short link here before landing it. Jordy, one technical thing to do: you need to start a new document in your chromium.org account and add view rights for everyone with that link. Then we should copy the current design doc over and mark the old one as obsolete. This is because you cannot share a google.com-owned document outside google.com, which would be a pita for non-Google Chrome contributors. Also the URL shortener needs to be public, e.g., goo.gl. https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:33: const size_t kInfosLength = arraysize(kInfos); On 2013/09/10 22:30:54, Ilya Sherman wrote: > Suppose we wanted to add tracking for an additional website -- say, facebook > (incidentally, why isn't that already included?). Would there be a way to add > the new website while keeping the bucket ids stable? If not, that seems like a > pretty serious flaw with this design, as the histogram meanings will need to > change every time we change which set of websites we're interested in tracking. Unfortunately, there is no way to add websites while keeping bucket ids stable, unless we just make a new independent group of those websites. How often do we expect to add sites, though? Is the site's popularity changing that often that we won't be interested in how the heuristics work on the above soon? If we are about to change that once a year or less frequently, which is what I expect, then we should be fine, because a couple of weeks of history on those histograms should be OK to detect drastic changes due to breakage. https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:45: // used across all users. On 2013/09/10 22:30:54, Ilya Sherman wrote: > Why not just compute a truly random number once per run, by caching the number > in a static variable? Seems like a good idea! https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_metrics_util.h (right): https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:17: char const* domain_name; On 2013/09/10 22:30:54, Ilya Sherman wrote: > char const* char I suppose Ilya meant const char* ? https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:18: int const bucket_ids[10]; int const -> const int https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:18: int const bucket_ids[10]; On 2013/09/10 22:30:54, Ilya Sherman wrote: > Why does each domain have 10 bucket ids? It has to be a half of the total number of buckets. For that we chose 20 rather randomly (choosing less means harder time interpreting the results, choosing more means more histograms and more values to monitor). This parameter should be documented in the design doc as well, before we land this (it's not yet). https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:18: int const bucket_ids[10]; Jordy -- instead of just writing "10", you should introduce a named constant. You can then 1. Give a short comment about why 10, pointing to the design doc. 2. Use the constant for for loops in the cc file.
@vabr @isherman Hi, I made the changes asked, could you please review my code ? Thanks https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager.cc (right): https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager.cc:224: const std::string& url_host) { On 2013/09/10 22:30:54, Ilya Sherman wrote: > Optional nit: Perhaps name this variable something more like |form_origin| or > |form_domain|? Done. https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager.cc:229: bucket_name = password_manager_metrics_util::IsDomainNameMonitored(url_host); On 2013/09/10 22:30:54, Ilya Sherman wrote: > nit: Why is the assignment on a separate line from the variable declaration? Done. https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager.h (right): https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager.h:116: // Log failure for UMA On 2013/09/10 22:30:54, Ilya Sherman wrote: > Please document the new parameter. Done. https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_delegate_impl.cc (right): https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:85: std::string bucket_name_; On 2013/09/10 22:30:54, Ilya Sherman wrote: > nit: Perhaps name this |uma_histogram_suffix_| or |domain_type_for_metrics_| or > something a little more descriptive like these two suggestions. Done. https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:124: 1); On 2013/09/10 22:30:54, Ilya Sherman wrote: > It's wasteful to allocate 50 buckets and only use one of them as you are doing > here. Please either create a single enumerated histogram with one bucket per > category, or use fewer buckets per histogram. Done. https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:129: On 2013/09/10 22:30:54, Ilya Sherman wrote: > nit: Spurious newline. Done. https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_metrics_util.cc (right): https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:18: static const size_t kBucketsPerDomain = 10u; On 2013/09/10 22:30:54, Ilya Sherman wrote: > This terminology is a little confusing, as "buckets" already has a meaning > within the context of histograms -- a single histogram has multiple buckets. > I'd recommend using a different term to describe the association of domains with > ids -- perhaps "groups" or "sets" or something like that. Done. https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:21: static const struct DomainBucketsPair kInfos[] = { On 2013/09/10 22:30:54, Ilya Sherman wrote: > nit: Extraneous "struct" Done. https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:21: static const struct DomainBucketsPair kInfos[] = { On 2013/09/10 22:30:54, Ilya Sherman wrote: > nit: Perhaps |kDomainMapping|? Done. https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:31: {"tumblr.com", {4, 8, 10, 12, 13, 15, 16, 18, 19, 20}}, On 2013/09/11 07:25:43, vabr (Chromium) wrote: > On 2013/09/10 22:30:54, Ilya Sherman wrote: > > This bucket distribution doesn't really seem well balanced -- I'd expect each > > bucket to appear an equal number of times, but '1' appears 5 times, whereas > '3' > > only appears 4 times. > > Good catch, Ilya! > http://ebay.com should have started with 3, not 13. > Jordy, can you please double-check for other typos? The way we generated this, > each bucket should have appeared 5 times (in other words, each bucket contains > exactly 5 hosts). Thanks Ilya and Vaclav. I am going to double-check for other typos. https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:33: const size_t kInfosLength = arraysize(kInfos); On 2013/09/11 07:25:43, vabr (Chromium) wrote: > On 2013/09/10 22:30:54, Ilya Sherman wrote: > > This needs a lot more documentation. Please explain the algorithm: what the > > bucket ids mean, how they were chosen, etc. > > Jordy is putting all of it in the design doc (currently > https://docs.google.com/a/google.com/document/d/1AEeaQGxKy0sKhZENTH1WWf8K7sfk..., > but the algorithm is not yet there), and we should put a short link here before > landing it. > > Jordy, one technical thing to do: you need to start a new document in your > http://chromium.org account and add view rights for everyone with that link. Then we > should copy the current design doc over and mark the old one as obsolete. This > is because you cannot share a google.com-owned document outside http://google.com, > which would be a pita for non-Google Chrome contributors. Also the URL shortener > needs to be public, e.g., goo.gl. Done. https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:33: const size_t kInfosLength = arraysize(kInfos); On 2013/09/10 22:30:54, Ilya Sherman wrote: > Suppose we wanted to add tracking for an additional website -- say, facebook > (incidentally, why isn't that already included?). Would there be a way to add > the new website while keeping the bucket ids stable? If not, that seems like a > pretty serious flaw with this design, as the histogram meanings will need to > change every time we change which set of websites we're interested in tracking. Facebook is not already included because the save password prompt is not working properly on it. I will try to see what is the problem and fix it if possible. https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:45: // used across all users. On 2013/09/11 07:25:43, vabr (Chromium) wrote: > On 2013/09/10 22:30:54, Ilya Sherman wrote: > > Why not just compute a truly random number once per run, by caching the number > > in a static variable? > > Seems like a good idea! Done. https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:49: .bucket_ids[base::GetCurrentProcId() % kBucketsPerDomain]); On 2013/09/10 22:30:54, Ilya Sherman wrote: > Rather than splitting this statement over four lines, please introduce a > temporary named variable and write it as two statements. Done. https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:49: .bucket_ids[base::GetCurrentProcId() % kBucketsPerDomain]); On 2013/09/10 22:30:54, Ilya Sherman wrote: > Rather than splitting this statement over four lines, please introduce a > temporary named variable and write it as two statements. Done. https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_metrics_util.h (right): https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:18: int const bucket_ids[10]; On 2013/09/11 07:25:43, vabr (Chromium) wrote: > int const -> const int Done. https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:19: }; On 2013/09/10 22:30:54, Ilya Sherman wrote: > This doesn't seem to be used anywhere else in the header file, so please move it > into the implementation file. Done. https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:19: }; On 2013/09/10 22:30:54, Ilya Sherman wrote: > This doesn't seem to be used anywhere else in the header file, so please move it > into the implementation file. Done. https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:24: std::string IsDomainNameMonitored(const std::string& url_host); On 2013/09/10 22:30:54, Ilya Sherman wrote: > This method name suggests that the method returns a boolean, which is no longer > true. Please update the name to match the current semantics. Done. https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:24: std::string IsDomainNameMonitored(const std::string& url_host); On 2013/09/10 22:30:54, Ilya Sherman wrote: > This method name suggests that the method returns a boolean, which is no longer > true. Please update the name to match the current semantics. Done. https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:37: // A version of the UMA_HISTOGRAM_COUNT macro that allows the |name| On 2013/09/10 22:30:54, Ilya Sherman wrote: > nit: I think you mean UMA_HISTOGRAM_COUNTS. Please fix that here and below. Done. https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:37: // A version of the UMA_HISTOGRAM_COUNT macro that allows the |name| On 2013/09/10 22:30:54, Ilya Sherman wrote: > nit: I think you mean UMA_HISTOGRAM_COUNTS. Please fix that here and below. Done. https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_metrics_util_unittest.cc (right): https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util_unittest.cc:27: } On 2013/09/10 22:30:54, Ilya Sherman wrote: > If you keep the bucketization strategy you've implemented here, please add some > test coverage to verify that it is working correctly, if possible. This might > mean you need to mock out the source of randomness somehow for testing, since > it's fairly important to test that (a) a single website can land in multiple > buckets and (b) a single website can't land in too many different buckets and > (c) buckets are "equally weighted" in some sense. Hi Ilya, The points (a) and (c), depend on the random value generation therefore I am wondering if we really need these tests. https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_unittest.cc (right): https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_unittest.cc:13: #include "chrome/browser/password_manager/password_manager_metrics_util.h" On 2013/09/10 22:30:54, Ilya Sherman wrote: > nit: Not needed. Done.
Hi Jordy, Please see the comments. Vaclav https://codereview.chromium.org/23140005/diff/85001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager.h (right): https://codereview.chromium.org/23140005/diff/85001/chrome/browser/password_m... chrome/browser/password_manager/password_manager.h:117: // |form_origin| is used in order to know if the save password prompt behavior nit: the previous line is wrapped too soon. Just add a full-stop (".") and continue the sentence until you have to wrap. https://codereview.chromium.org/23140005/diff/85001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_delegate_impl.cc (right): https://codereview.chromium.org/23140005/diff/85001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:120: uma_histogram_suffix_ = password_manager_metrics_util::MonitoredDomainGroup( nit: this could be done in the initialiser list: uma_histogram_suffix_(password_manager_metrics_util::MonitoredDomainGroup(...)) Then you can make uma_histogram_suffix_ a const member variable, improving the readability and efficiency. https://codereview.chromium.org/23140005/diff/85001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:127: if (!base::StringToInt(string_group_number, &group_number)) You have just written an int to a string in MonitoredDomainGroup, and here you parse it back again. Could you get it directly? For example, you can split MonitoredDomainGroup into the part which gets the number, and the wrapper which creates the suffix, and call here only the inner part. https://codereview.chromium.org/23140005/diff/85001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:139: // displayed during enough time for the user to make a decision. This is not grammatically correct, and could me more informative. Suggested rephrasing: "The shortest period (in ms) for which the prompt needs to live, so that we don't consider it killed prematurely, as might happen, e.g., if a pre-rendered page gets swapped in (and the current WebContent is destroyed)." https://codereview.chromium.org/23140005/diff/85001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_metrics_util.cc (right): https://codereview.chromium.org/23140005/diff/85001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:21: const char* domain_name; nit: const char* const (Maybe that's what Ilya meant, but swapped char and const.) https://codereview.chromium.org/23140005/diff/85001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:25: static unsigned int random_group_id = kGroupsPerDomain; Please prepend "g_" to the variable's name, as the style guide suggests. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Variab... Also, please put it into an anonymous namespace. I think you can then drop "static", it will not be accessible from outside. https://codereview.chromium.org/23140005/diff/85001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:36: // Only used in the IsDomainNameMonitoredTest. Please remove this comment. It's duplicated, and should be replaced by the function's name suffix (ForTesting) anyway. https://codereview.chromium.org/23140005/diff/85001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:42: // This array contains each monitored websites together with all id of nit: id -> ids https://codereview.chromium.org/23140005/diff/85001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:44: // times and every |group_ids| are different. For more information about the nit: "every |group_ids| are different" -> "every ID group is different" https://codereview.chromium.org/23140005/diff/85001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:89: void LogUMAHistogramTimes(const std::string& name, Do you still need this function? If not, please remove it. https://codereview.chromium.org/23140005/diff/85001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_metrics_util.h (right): https://codereview.chromium.org/23140005/diff/85001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:16: // The number of time that each monitored website appears. Please change "time that" -> "groups in which" https://codereview.chromium.org/23140005/diff/85001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:17: // It is a half of the total number of buckets. First: "groups", not "buckets". Second: If you mean that this is always the half of kGroupNumber, then don't say this here, and instead define const size_t kGroupNumber = 2 * kGroupsPerDomain; below. https://codereview.chromium.org/23140005/diff/85001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:18: // For more information see goo.gl/vUuFd5. Please add http://, so that this becomes a clickable link in CodeSearch. https://codereview.chromium.org/23140005/diff/85001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:18: // For more information see goo.gl/vUuFd5. Also, maybe write a very short paragraph about what are domains and groups in this context, put it just after the namespace definition (with a trailing blank line), and then move the link to the design doc at the end of that paragraph. Suggestion for the paragraph text (but feel free to rephrase): "We monitor the performance of the save password heuristic for a handful of domains. For privacy reasons we are not reporting UMA signals by domain, but by a domain group. A domain group can contain mutliple domains, and a domain can be contained in multiple groups." Also, having the preamble will make comments to individual definitions below shorter and clearer. https://codereview.chromium.org/23140005/diff/85001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:19: const size_t kGroupsPerDomain = 10u; Also, are these constants used outside of this header file's *.cc file? If not, they should go to the .cc file and into an anonymous namespace. https://codereview.chromium.org/23140005/diff/85001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:21: // Number of groups which can store monitored websites. "store monitored website" sounds confusing (like saving the HTML). Maybe remove this comment, and instead add an initial comment for this section, as described above. https://codereview.chromium.org/23140005/diff/85001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:51: void generateRandomId(); Please capitalise (GenerateRandomId), also below for SetRandomId. https://codereview.chromium.org/23140005/diff/85001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:54: void setRandomId(unsigned int value); Please append "ForTesting" to the function name. This marks is clearly as a testing-only code. You can then drop the comment. https://codereview.chromium.org/23140005/diff/85001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_metrics_util_unittest.cc (right): https://codereview.chromium.org/23140005/diff/85001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util_unittest.cc:47: std::map<std::string, unsigned int> groups; nit: It could be better if the rest of this test is made to a separate test in this file. But that's optional, feel free to ignore. https://codereview.chromium.org/23140005/diff/85001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util_unittest.cc:61: unsigned int number_of_assignemt = groups["group_1"]; typo (assignemt) https://codereview.chromium.org/23140005/diff/85001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util_unittest.cc:64: EXPECT_EQ(it->second, number_of_assignemt); nit: Adding << " group name = " << it->first might produce more useful error messages in case of failure.
@vabr @isherman Hi ! Could you please review my code ? Thanks. https://codereview.chromium.org/23140005/diff/85001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager.h (right): https://codereview.chromium.org/23140005/diff/85001/chrome/browser/password_m... chrome/browser/password_manager/password_manager.h:117: // |form_origin| is used in order to know if the save password prompt behavior On 2013/09/11 17:31:24, vabr (Chromium) wrote: > nit: the previous line is wrapped too soon. Just add a full-stop (".") and > continue the sentence until you have to wrap. Done. https://codereview.chromium.org/23140005/diff/85001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_delegate_impl.cc (right): https://codereview.chromium.org/23140005/diff/85001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:120: uma_histogram_suffix_ = password_manager_metrics_util::MonitoredDomainGroup( On 2013/09/11 17:31:24, vabr (Chromium) wrote: > nit: this could be done in the initialiser list: > > uma_histogram_suffix_(password_manager_metrics_util::MonitoredDomainGroup(...)) > > Then you can make uma_histogram_suffix_ a const member variable, improving the > readability and efficiency. Done. https://codereview.chromium.org/23140005/diff/85001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:127: if (!base::StringToInt(string_group_number, &group_number)) On 2013/09/11 17:31:24, vabr (Chromium) wrote: > You have just written an int to a string in MonitoredDomainGroup, and here you > parse it back again. Could you get it directly? For example, you can split > MonitoredDomainGroup into the part which gets the number, and the wrapper which > creates the suffix, and call here only the inner part. Done. https://codereview.chromium.org/23140005/diff/85001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:139: // displayed during enough time for the user to make a decision. On 2013/09/11 17:31:24, vabr (Chromium) wrote: > This is not grammatically correct, and could me more informative. > Suggested rephrasing: > > "The shortest period (in ms) for which the prompt needs to > live, so that we don't consider it killed prematurely, as might > happen, e.g., if a pre-rendered page gets swapped in (and the > current WebContent is destroyed)." Done. https://codereview.chromium.org/23140005/diff/85001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_metrics_util.cc (right): https://codereview.chromium.org/23140005/diff/85001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:21: const char* domain_name; On 2013/09/11 17:31:24, vabr (Chromium) wrote: > nit: const char* const > (Maybe that's what Ilya meant, but swapped char and const.) Done. https://codereview.chromium.org/23140005/diff/85001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:25: static unsigned int random_group_id = kGroupsPerDomain; On 2013/09/11 17:31:24, vabr (Chromium) wrote: > Please prepend "g_" to the variable's name, as the style guide suggests. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Variab... > > Also, please put it into an anonymous namespace. I think you can then drop > "static", it will not be accessible from outside. Done. https://codereview.chromium.org/23140005/diff/85001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:36: // Only used in the IsDomainNameMonitoredTest. On 2013/09/11 17:31:24, vabr (Chromium) wrote: > Please remove this comment. It's duplicated, and should be replaced by the > function's name suffix (ForTesting) anyway. Done. https://codereview.chromium.org/23140005/diff/85001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:42: // This array contains each monitored websites together with all id of On 2013/09/11 17:31:24, vabr (Chromium) wrote: > nit: id -> ids Done. https://codereview.chromium.org/23140005/diff/85001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:44: // times and every |group_ids| are different. For more information about the On 2013/09/11 17:31:24, vabr (Chromium) wrote: > nit: "every |group_ids| are different" -> "every ID group is different" Done. https://codereview.chromium.org/23140005/diff/85001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:89: void LogUMAHistogramTimes(const std::string& name, On 2013/09/11 17:31:24, vabr (Chromium) wrote: > Do you still need this function? If not, please remove it. Done. https://codereview.chromium.org/23140005/diff/85001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_metrics_util.h (right): https://codereview.chromium.org/23140005/diff/85001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:16: // The number of time that each monitored website appears. On 2013/09/11 17:31:24, vabr (Chromium) wrote: > Please change > "time that" -> "groups in which" Done. https://codereview.chromium.org/23140005/diff/85001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:17: // It is a half of the total number of buckets. On 2013/09/11 17:31:24, vabr (Chromium) wrote: > First: "groups", not "buckets". > Second: If you mean that this is always the half of kGroupNumber, > then don't say this here, and instead define > const size_t kGroupNumber = 2 * kGroupsPerDomain; > below. Done. https://codereview.chromium.org/23140005/diff/85001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:18: // For more information see goo.gl/vUuFd5. On 2013/09/11 17:31:24, vabr (Chromium) wrote: > Please add http://, so that this becomes a clickable link in CodeSearch. Done. https://codereview.chromium.org/23140005/diff/85001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:18: // For more information see goo.gl/vUuFd5. On 2013/09/11 17:31:24, vabr (Chromium) wrote: > Also, maybe write a very short paragraph about what are domains and groups in > this context, put it just after the namespace definition (with a trailing blank > line), and then move the link to the design doc at the end of that paragraph. > > Suggestion for the paragraph text (but feel free to rephrase): > "We monitor the performance of the save password heuristic for a handful of > domains. For privacy reasons we are not reporting UMA signals by domain, but by > a domain group. A domain group can contain mutliple domains, and a domain can be > contained in multiple groups." > > > Also, having the preamble will make comments to individual definitions below > shorter and clearer. Done. https://codereview.chromium.org/23140005/diff/85001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:21: // Number of groups which can store monitored websites. On 2013/09/11 17:31:24, vabr (Chromium) wrote: > "store monitored website" sounds confusing (like saving the HTML). > Maybe remove this comment, and instead add an initial comment for this section, > as described above. Done. https://codereview.chromium.org/23140005/diff/85001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:51: void generateRandomId(); On 2013/09/11 17:31:24, vabr (Chromium) wrote: > Please capitalise (GenerateRandomId), also below for SetRandomId. Done. https://codereview.chromium.org/23140005/diff/85001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:54: void setRandomId(unsigned int value); On 2013/09/11 17:31:24, vabr (Chromium) wrote: > Please append "ForTesting" to the function name. This marks is clearly as a > testing-only code. You can then drop the comment. Done. https://codereview.chromium.org/23140005/diff/85001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_metrics_util_unittest.cc (right): https://codereview.chromium.org/23140005/diff/85001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util_unittest.cc:47: std::map<std::string, unsigned int> groups; On 2013/09/11 17:31:24, vabr (Chromium) wrote: > nit: It could be better if the rest of this test is made to a separate test in > this file. But that's optional, feel free to ignore. Done. https://codereview.chromium.org/23140005/diff/85001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util_unittest.cc:61: unsigned int number_of_assignemt = groups["group_1"]; On 2013/09/11 17:31:24, vabr (Chromium) wrote: > typo (assignemt) Done. https://codereview.chromium.org/23140005/diff/85001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util_unittest.cc:64: EXPECT_EQ(it->second, number_of_assignemt); On 2013/09/11 17:31:24, vabr (Chromium) wrote: > nit: Adding > << " group name = " << it->first > might produce more useful error messages in case of failure. Done.
https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_metrics_util.h (right): https://codereview.chromium.org/23140005/diff/77001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:17: char const* domain_name; On 2013/09/11 07:25:43, vabr (Chromium) wrote: > On 2013/09/10 22:30:54, Ilya Sherman wrote: > > char const* char > > I suppose Ilya meant > const char* > ? Whoops, good catch. I meant "const char* const".
https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager.h (right): https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager.h:117: // password prompt behavior is monitored for the current host. I find the second sentence in this comment somewhat hard to follow. One possible rephrasing that I think would be clearer: "Logs additional metrics if the |form_origin| corresponds to one of the top, explicitly monitored websites." https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_delegate_impl.cc (right): https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:11: #include "base/test/perftimer.h" I just noticed that the timer you're relying on is intended for test code. Please pick a different sort of timer, that is intended for production code. https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:125: return; What is the purpose of this early return, i.e. why do you need an additional check beyond "if (!uma_histogram_suffix_.empty())"? This check currently looks wrong to me -- it will cause the constructor to return early unless the monitored domain group id is 0... https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:130: "PasswordManager.SavePasswordPromptDisplayed", group_id); Please emit this as a linear (i.e. enumerated) histogram with exactly the number of buckets that you're interested in. Currently, this will allocate too many buckets, and also place multiple groups into a single bucket. (You can see what I mean by looking at the values emitted to this histogram when viewed via about:histograms.) https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:136: infobar_response_, NUM_RESPONSE_TYPES); nit: Please leave a blank line after this one. https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:140: // current WebContent is destroyed) nit: Please end the sentence with a period. https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:140: // current WebContent is destroyed) nit: WebContent -> WebContents https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:141: const int kTime = 1000; Please give this constant an even more descriptive name. A couple of suggestions: kMinimumPromptDisplayTimeMs, kQuickDismissalThresholdMs. Note the "ms" suffix to indicate the units -- though even better would be to declare this as a TimeDelta rather than as a raw integer. https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_metrics_util.cc (right): https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:9: #include "base/process/process_handle.h" nit: No longer needed? https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:13: #include "base/time/time.h" nit: No longer needed? https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:20: // The number of domain group. nit: "group" -> "groups" https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:21: const size_t kGroupNumber = 20u; nit: "kNumGroups" https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:32: unsigned int g_random_group_id = kGroupsPerDomain; Please always prefer size_t or one of the uint* types to unsigned int. https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:32: unsigned int g_random_group_id = kGroupsPerDomain; This should be declared in the anonymous namespace. https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:48: return "group_" + base::IntToString(group_id); Seems like this ought to return an empty string if the group_id corresponds to an unmonitored domain. https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:49: } Please make sure that the order of the functions is consistent between the implementation file and the header file. https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:52: // This array contains each monitored websites together with all ids of nit: "websites" -> "website" https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:54: // times and every |group_ids| is different. For more information about the nit: Possible rephrasing of "Each website appears |kGroupsPerDomain| times and every |group_ids| is different.": "Each website appears in |kGroupsPerDomain| groups, and each group includes an equal number of websites, so that no two websites have the same set of groups that they belong to." https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:74: std::string domain = kDomainMapping[i].domain_name; nit: Not a big deal, but this seems like an unnecessary string allocation + copy, especially since you go right back and pull out the c-string below. https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_metrics_util.h (right): https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:12: } nit: No longer needed? https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:18: // by a domain group. A domain group can contain mutliple domains, and a domain nit: "mutliple" -> "multiple" https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:28: // returns -1. What does it mean for a function that returns an unsigned integer to return -1? https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:47: // the value of |random_id| is not changed. |random_group_id| and |random_id| are not variable names, so they shouldn't be surrounded by pipes, nor written in hacker_case. Perhaps you meant |g_random_group_id| in both cases? https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:47: // the value of |random_id| is not changed. This sentence seems like it describes the opposite of what actually happens in the code. https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:48: void GenerateRandomId(); This seems like it should be an internal method, tucked into the anonymous namespace in the implementation file. https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:53: std::string AppendGroup(unsigned int group_id); nit: IMO this function would be better named something like "GroupIdToString()". https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_metrics_util_unittest.cc (right): https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util_unittest.cc:14: const char* const kMonitoredWebsite[] = { nit: kMonitoredWebsites (plural) https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util_unittest.cc:30: std::map<unsigned int, unsigned int> groups; Please always use "size_t" rather than "unsigned int". https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util_unittest.cc:36: j++) { nit: ++j https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util_unittest.cc:43: // Check if all groups get assigned the same number of time. nit: "time" -> "times" https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util_unittest.cc:44: unsigned int number_of_assigment = groups[1]; Why groups[1] rather than, say, groups[0] or groups.begin()? https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util_unittest.cc:56: EXPECT_TRUE(group_id > 0); nit: For readability's sake, it might be helpful to decompose an "IsMonitored()" helper function that you can call here.
Hi Jordy, I put in a couple of comments, but then I decided I will make a pass after you address Ilya's recent comments. If you have any questions during addressing any of the comments (from me or Ilya), please feel free to let me know. Also please be aware that I'm off by 4pm today, and will be back on Monday. If you finish the CL up to Ilya's satisfaction and approval until then, there is no reason to wait for me with landing. Thanks! Vaclav https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_delegate_impl.cc (right): https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:11: #include "base/test/perftimer.h" On 2013/09/12 04:45:45, Ilya Sherman wrote: > I just noticed that the timer you're relying on is intended for test code. > Please pick a different sort of timer, that is intended for production code. That's a good point. Probably something from base/timer/timer.h would be appropriate. As a side point, the PerfTimer seems to be used in a couple of places in production code, so I guess I will ask at chromium-dev why is that timer in test, and whether we should correct the current usage. https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:125: return; On 2013/09/12 04:45:45, Ilya Sherman wrote: > What is the purpose of this early return, i.e. why do you need an additional > check beyond "if (!uma_histogram_suffix_.empty())"? This check currently looks > wrong to me -- it will cause the constructor to return early unless the > monitored domain group id is 0... I agree, this code should be removed: if (password_manager_metrics_util::MonitoredDomainGroupId(form_to_save->realm())) return; https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_metrics_util_unittest.cc (right): https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util_unittest.cc:56: EXPECT_TRUE(group_id > 0); Also, this is better written as EXPECT_GT(group_id, 0) The difference is that in the latter case the value of group_id will be part of the log in case of failure, whereas EXPECT_TRUE only says if the comparison was true or false (which is already clear from there being a failure).
https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_delegate_impl.cc (right): https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:11: #include "base/test/perftimer.h" On 2013/09/12 08:51:04, vabr (Chromium) wrote: > Probably something from base/timer/timer.h would be appropriate. Using timer/timer.h is actually a bad idea (thanks Jordy for pointing that out). That timer is for scheduling, not measuring. I'm not sure whether there is a production code timer for measuring. It looks like PerfTimer is used for that purpose. I already e-mailed chromium-dev asking about this, let's see whether we have any response.
On 2013/09/12 10:12:22, vabr (Chromium) wrote: > https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... > File chrome/browser/password_manager/password_manager_delegate_impl.cc (right): > > https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... > chrome/browser/password_manager/password_manager_delegate_impl.cc:11: #include > "base/test/perftimer.h" > On 2013/09/12 08:51:04, vabr (Chromium) wrote: > > Probably something from base/timer/timer.h would be appropriate. > Using timer/timer.h is actually a bad idea (thanks Jordy for pointing that out). > That timer is for scheduling, not measuring. > > I'm not sure whether there is a production code timer for measuring. It looks > like PerfTimer is used for that purpose. I already e-mailed chromium-dev asking > about this, let's see whether we have any response. I think that we can always use Time::Now() (base/time/time.h) on start and end, and get the result as a delta. That's production code. But it is also exactly what the PerfTimer does. So I'm wondering if using PerfTimer is a bad idea after all.
@vabr @isherman Hi ! Could you please review my code ? Thanks https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager.h (right): https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager.h:117: // password prompt behavior is monitored for the current host. On 2013/09/12 04:45:45, Ilya Sherman wrote: > I find the second sentence in this comment somewhat hard to follow. One > possible rephrasing that I think would be clearer: "Logs additional metrics if > the |form_origin| corresponds to one of the top, explicitly monitored websites." Done. https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_delegate_impl.cc (right): https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:125: return; On 2013/09/12 04:45:45, Ilya Sherman wrote: > What is the purpose of this early return, i.e. why do you need an additional > check beyond "if (!uma_histogram_suffix_.empty())"? This check currently looks > wrong to me -- it will cause the constructor to return early unless the > monitored domain group id is 0... Done. https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:130: "PasswordManager.SavePasswordPromptDisplayed", group_id); On 2013/09/12 04:45:45, Ilya Sherman wrote: > Please emit this as a linear (i.e. enumerated) histogram with exactly the number > of buckets that you're interested in. Currently, this will allocate too many > buckets, and also place multiple groups into a single bucket. (You can see what > I mean by looking at the values emitted to this histogram when viewed via > about:histograms.) Done. https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:136: infobar_response_, NUM_RESPONSE_TYPES); On 2013/09/12 04:45:45, Ilya Sherman wrote: > nit: Please leave a blank line after this one. Done. https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:140: // current WebContent is destroyed) On 2013/09/12 04:45:45, Ilya Sherman wrote: > nit: Please end the sentence with a period. Done. https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:140: // current WebContent is destroyed) On 2013/09/12 04:45:45, Ilya Sherman wrote: > nit: WebContent -> WebContents Done. https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:141: const int kTime = 1000; On 2013/09/12 04:45:45, Ilya Sherman wrote: > Please give this constant an even more descriptive name. A couple of > suggestions: kMinimumPromptDisplayTimeMs, kQuickDismissalThresholdMs. Note the > "ms" suffix to indicate the units -- though even better would be to declare this > as a TimeDelta rather than as a raw integer. Done. https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_metrics_util.cc (right): https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:9: #include "base/process/process_handle.h" On 2013/09/12 04:45:45, Ilya Sherman wrote: > nit: No longer needed? Done. https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:20: // The number of domain group. On 2013/09/12 04:45:45, Ilya Sherman wrote: > nit: "group" -> "groups" Done. https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:21: const size_t kGroupNumber = 20u; On 2013/09/12 04:45:45, Ilya Sherman wrote: > nit: "kNumGroups" Done. https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:32: unsigned int g_random_group_id = kGroupsPerDomain; On 2013/09/12 04:45:45, Ilya Sherman wrote: > This should be declared in the anonymous namespace. Done. https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:32: unsigned int g_random_group_id = kGroupsPerDomain; On 2013/09/12 04:45:45, Ilya Sherman wrote: > Please always prefer size_t or one of the uint* types to unsigned int. Done. https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:48: return "group_" + base::IntToString(group_id); On 2013/09/12 04:45:45, Ilya Sherman wrote: > Seems like this ought to return an empty string if the group_id corresponds to > an unmonitored domain. Done. https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:49: } On 2013/09/12 04:45:45, Ilya Sherman wrote: > Please make sure that the order of the functions is consistent between the > implementation file and the header file. Done. https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:52: // This array contains each monitored websites together with all ids of On 2013/09/12 04:45:45, Ilya Sherman wrote: > nit: "websites" -> "website" Done. https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:54: // times and every |group_ids| is different. For more information about the On 2013/09/12 04:45:45, Ilya Sherman wrote: > nit: Possible rephrasing of "Each website appears |kGroupsPerDomain| times and > every |group_ids| is different.": "Each website appears in |kGroupsPerDomain| > groups, and each group includes an equal number of websites, so that no two > websites have the same set of groups that they belong to." Done. https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:74: std::string domain = kDomainMapping[i].domain_name; On 2013/09/12 04:45:45, Ilya Sherman wrote: > nit: Not a big deal, but this seems like an unnecessary string allocation + > copy, especially since you go right back and pull out the c-string below. Done. https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_metrics_util.h (right): https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:12: } On 2013/09/12 04:45:45, Ilya Sherman wrote: > nit: No longer needed? Done. https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:18: // by a domain group. A domain group can contain mutliple domains, and a domain On 2013/09/12 04:45:45, Ilya Sherman wrote: > nit: "mutliple" -> "multiple" Done. https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:28: // returns -1. On 2013/09/12 04:45:45, Ilya Sherman wrote: > What does it mean for a function that returns an unsigned integer to return -1? Sorry the previous version used an integer. Now the function returns 0 when the host is not found because the ids begin at 1. https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:47: // the value of |random_id| is not changed. On 2013/09/12 04:45:45, Ilya Sherman wrote: > This sentence seems like it describes the opposite of what actually happens in > the code. Done. https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:47: // the value of |random_id| is not changed. On 2013/09/12 04:45:45, Ilya Sherman wrote: > |random_group_id| and |random_id| are not variable names, so they shouldn't be > surrounded by pipes, nor written in hacker_case. Perhaps you meant > |g_random_group_id| in both cases? Done. https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:48: void GenerateRandomId(); On 2013/09/12 04:45:45, Ilya Sherman wrote: > This seems like it should be an internal method, tucked into the anonymous > namespace in the implementation file. Done. https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:53: std::string AppendGroup(unsigned int group_id); On 2013/09/12 04:45:45, Ilya Sherman wrote: > nit: IMO this function would be better named something like "GroupIdToString()". Done. https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_metrics_util_unittest.cc (right): https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util_unittest.cc:14: const char* const kMonitoredWebsite[] = { On 2013/09/12 04:45:45, Ilya Sherman wrote: > nit: kMonitoredWebsites (plural) Done. https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util_unittest.cc:30: std::map<unsigned int, unsigned int> groups; On 2013/09/12 04:45:45, Ilya Sherman wrote: > Please always use "size_t" rather than "unsigned int". Done. https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util_unittest.cc:36: j++) { On 2013/09/12 04:45:45, Ilya Sherman wrote: > nit: ++j Done. https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util_unittest.cc:43: // Check if all groups get assigned the same number of time. On 2013/09/12 04:45:45, Ilya Sherman wrote: > nit: "time" -> "times" Done. https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util_unittest.cc:44: unsigned int number_of_assigment = groups[1]; On 2013/09/12 04:45:45, Ilya Sherman wrote: > Why groups[1] rather than, say, groups[0] or groups.begin()? Done. https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util_unittest.cc:56: EXPECT_TRUE(group_id > 0); On 2013/09/12 04:45:45, Ilya Sherman wrote: > nit: For readability's sake, it might be helpful to decompose an "IsMonitored()" > helper function that you can call here. Done. https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util_unittest.cc:56: EXPECT_TRUE(group_id > 0); On 2013/09/12 08:51:04, vabr (Chromium) wrote: > Also, this is better written as > EXPECT_GT(group_id, 0) > > The difference is that in the latter case the value of group_id will be part of > the log in case of failure, whereas EXPECT_TRUE only says if the comparison was > true or false (which is already clear from there being a failure). Done.
Hi Jordy, That's all from me until Monday. As I said, if you work with Ilya to get his approval earlier than that, don't feel obliged to wait for me. Thanks, Vaclav https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_delegate_impl.cc (right): https://codereview.chromium.org/23140005/diff/94001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:11: #include "base/test/perftimer.h" On 2013/09/12 10:12:22, vabr (Chromium) wrote: > On 2013/09/12 08:51:04, vabr (Chromium) wrote: > > Probably something from base/timer/timer.h would be appropriate. > Using timer/timer.h is actually a bad idea (thanks Jordy for pointing that out). > That timer is for scheduling, not measuring. > > I'm not sure whether there is a production code timer for measuring. It looks > like PerfTimer is used for that purpose. I already e-mailed chromium-dev asking > about this, let's see whether we have any response. So the result from chromium-dev is that Thiago is working on moving PerfTimer out of test: https://codereview.chromium.org/23985006/. So it should be OK to keep using PerfTimer here. https://codereview.chromium.org/23140005/diff/79001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_delegate_impl.cc (right): https://codereview.chromium.org/23140005/diff/79001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:9: #include "base/strings/string_number_conversions.h" No longer needed? https://codereview.chromium.org/23140005/diff/79001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:61: BOUNDARY_VALUE, I wonder what are the naming conventions. Should this rather be NUM_SAVE_PASSWORD_PROMPT_DISPLAYED_ENUM, similarly to ResponseType? (Also, do we need the "Enum" suffix?) Saying just BOUNDARY_VALUE does not identify the enum's name, which is confusing given there are multiple enums in this context. https://codereview.chromium.org/23140005/diff/79001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:143: const base::TimeDelta kMinimumPromptDisplayTimeMs = Now that this is a TimeDelta, you don't need the "Ms" suffix. :) That's because TimeDelta represents an abstract time interval, and can be only converted to numbers using methods which contain the time units as a part of their name. https://codereview.chromium.org/23140005/diff/79001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_metrics_util.cc (right): https://codereview.chromium.org/23140005/diff/79001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:20: const size_t kNumGroups = 20u; Why not kNumGroups = 2 * kGroupsPerDomain? That would spare you the comment at kGroupsPerDomain about being a half, and also make sure that this relation always holds. https://codereview.chromium.org/23140005/diff/79001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_metrics_util.h (right): https://codereview.chromium.org/23140005/diff/79001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:18: // The number of group in which each monitored website appears. group -> groups https://codereview.chromium.org/23140005/diff/79001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:25: unsigned int MonitoredDomainGroupId(const std::string& url_host); As Ilya pointed out, please use size_t, or some unsigned type with a fixed width, so that we cannot run into problems with platforms with different bit width of "unsigned int". See also http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Intege... https://codereview.chromium.org/23140005/diff/79001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:35: void LogUMAHistogramCounts(const std::string& name, int sample); You don't use this any more. Please remove. https://codereview.chromium.org/23140005/diff/79001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:46: void SetRandomIdForTesting(unsigned int value); Also please change to a fixed-width unsigned type. (As above.) https://codereview.chromium.org/23140005/diff/79001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:50: std::string GroupIdToString(unsigned int group_id); Ditto. https://codereview.chromium.org/23140005/diff/79001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_metrics_util_unittest.cc (right): https://codereview.chromium.org/23140005/diff/79001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util_unittest.cc:61: size_t zero = 0; Can you delete zero and replace it with 0u? https://codereview.chromium.org/23140005/diff/79001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util_unittest.cc:68: EXPECT_EQ(group_id, zero); If you replace zero with 0u, the please also swap the order to: EXPECT_EQ(0u, group_id) because it is the convention to put the expected constant as the first number for equality checks (not necessarily for GT, but you might also swap that and replace GT with LT). Also at the EXPECT_EQ.
https://codereview.chromium.org/23140005/diff/79001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_metrics_util_unittest.cc (right): https://codereview.chromium.org/23140005/diff/79001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util_unittest.cc:68: EXPECT_EQ(group_id, zero); > Also at the EXPECT_EQ. I meant "Also at the EXPECT_EQ below." :)
@isherman Hi Ilya. I did the changes asked. Could you please review my code ? Thanks https://codereview.chromium.org/23140005/diff/79001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_delegate_impl.cc (right): https://codereview.chromium.org/23140005/diff/79001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:9: #include "base/strings/string_number_conversions.h" On 2013/09/12 13:40:02, vabr (Chromium) wrote: > No longer needed? Done. https://codereview.chromium.org/23140005/diff/79001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:143: const base::TimeDelta kMinimumPromptDisplayTimeMs = On 2013/09/12 13:40:02, vabr (Chromium) wrote: > Now that this is a TimeDelta, you don't need the "Ms" suffix. :) > That's because TimeDelta represents an abstract time interval, and can be only > converted to numbers using methods which contain the time units as a part of > their name. Done. https://codereview.chromium.org/23140005/diff/79001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_metrics_util.cc (right): https://codereview.chromium.org/23140005/diff/79001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.cc:20: const size_t kNumGroups = 20u; On 2013/09/12 13:40:02, vabr (Chromium) wrote: > Why not kNumGroups = 2 * kGroupsPerDomain? > That would spare you the comment at kGroupsPerDomain about being a half, and > also make sure that this relation always holds. Done. https://codereview.chromium.org/23140005/diff/79001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_metrics_util.h (right): https://codereview.chromium.org/23140005/diff/79001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:25: unsigned int MonitoredDomainGroupId(const std::string& url_host); On 2013/09/12 13:40:02, vabr (Chromium) wrote: > As Ilya pointed out, please use size_t, or some unsigned type with a fixed > width, so that we cannot run into problems with platforms with different bit > width of "unsigned int". > > See also > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Intege... Done. https://codereview.chromium.org/23140005/diff/79001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:35: void LogUMAHistogramCounts(const std::string& name, int sample); On 2013/09/12 13:40:02, vabr (Chromium) wrote: > You don't use this any more. Please remove. Done. https://codereview.chromium.org/23140005/diff/79001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:46: void SetRandomIdForTesting(unsigned int value); On 2013/09/12 13:40:02, vabr (Chromium) wrote: > Also please change to a fixed-width unsigned type. (As above.) Done. https://codereview.chromium.org/23140005/diff/79001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util.h:50: std::string GroupIdToString(unsigned int group_id); On 2013/09/12 13:40:02, vabr (Chromium) wrote: > Ditto. Done. https://codereview.chromium.org/23140005/diff/79001/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_metrics_util_unittest.cc (right): https://codereview.chromium.org/23140005/diff/79001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util_unittest.cc:61: size_t zero = 0; On 2013/09/12 13:40:02, vabr (Chromium) wrote: > Can you delete zero and replace it with 0u? Done. https://codereview.chromium.org/23140005/diff/79001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util_unittest.cc:68: EXPECT_EQ(group_id, zero); On 2013/09/12 13:40:02, vabr (Chromium) wrote: > If you replace zero with 0u, the please also swap the order to: > > EXPECT_EQ(0u, group_id) > > because it is the convention to put the expected constant as the first number > for equality checks (not necessarily for GT, but you might also swap that and > replace GT with LT). > > Also at the EXPECT_EQ. Done. https://codereview.chromium.org/23140005/diff/79001/chrome/browser/password_m... chrome/browser/password_manager/password_manager_metrics_util_unittest.cc:68: EXPECT_EQ(group_id, zero); On 2013/09/12 13:40:02, vabr (Chromium) wrote: > If you replace zero with 0u, the please also swap the order to: > > EXPECT_EQ(0u, group_id) > > because it is the convention to put the expected constant as the first number > for equality checks (not necessarily for GT, but you might also swap that and > replace GT with LT). > > Also at the EXPECT_EQ. Done.
Thanks, this is getting quite close to being ready to land :) https://codereview.chromium.org/23140005/diff/108001/chrome/browser/password_... File chrome/browser/password_manager/password_manager_delegate_impl.cc (right): https://codereview.chromium.org/23140005/diff/108001/chrome/browser/password_... chrome/browser/password_manager/password_manager_delegate_impl.cc:61: }; For a histogram with just one possible value, you might as well just emit to a boolean histogram and omit the enum. https://codereview.chromium.org/23140005/diff/108001/chrome/browser/password_... chrome/browser/password_manager/password_manager_delegate_impl.cc:143: base::TimeDelta::FromInternalValue(1000); Please don't use FromInternalValue. Instead, use one of the From* methods that indicates the units of the value that you're constructing the TimeDelta from. https://codereview.chromium.org/23140005/diff/108001/chrome/browser/password_... File chrome/browser/password_manager/password_manager_metrics_util.cc (right): https://codereview.chromium.org/23140005/diff/108001/chrome/browser/password_... chrome/browser/password_manager/password_manager_metrics_util.cc:22: size_t g_random_group_id = kGroupsPerDomain; Please add documentation for this variable. https://codereview.chromium.org/23140005/diff/108001/chrome/browser/password_... chrome/browser/password_manager/password_manager_metrics_util.cc:103: if (group_id > 0 && group_id <= kNumGroups) nit: DCHECK_LE that group_id <= kNumGroups, since that should be an invariant. https://codereview.chromium.org/23140005/diff/108001/chrome/browser/password_... chrome/browser/password_manager/password_manager_metrics_util.cc:105: return ""; nit: return std::string(); https://codereview.chromium.org/23140005/diff/108001/chrome/browser/password_... File chrome/browser/password_manager/password_manager_metrics_util.h (right): https://codereview.chromium.org/23140005/diff/108001/chrome/browser/password_... chrome/browser/password_manager/password_manager_metrics_util.h:40: void GenerateRandomId(); Please move this declaration into the anonymous namespace in the .cc file. https://codereview.chromium.org/23140005/diff/108001/chrome/browser/password_... chrome/browser/password_manager/password_manager_metrics_util.h:42: void SetRandomIdForTesting(size_t value); Please add documentation for this function. https://codereview.chromium.org/23140005/diff/108001/chrome/browser/password_... File chrome/browser/password_manager/password_manager_metrics_util_unittest.cc (right): https://codereview.chromium.org/23140005/diff/108001/chrome/browser/password_... chrome/browser/password_manager/password_manager_metrics_util_unittest.cc:15: size_t IsMonitored(const char* url_host) { A method whose name starts with "Is" should return a boolean rather than a size_t. My suggestion was to make this method be implemented as return password_manager_metrics_util::MonitoredDomainGroupId(url_host) > 0; https://codereview.chromium.org/23140005/diff/108001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/23140005/diff/108001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:10444: + Indicates if the password manager is enable when PasswordManagers are nit: "Indicates if" -> "Indicates whether" https://codereview.chromium.org/23140005/diff/108001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:10444: + Indicates if the password manager is enable when PasswordManagers are nit: "is enable" -> "is enabled" https://codereview.chromium.org/23140005/diff/108001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:10444: + Indicates if the password manager is enable when PasswordManagers are "when PasswordManagers are constructed" is not very helpful to someone unfamiliar with the codebase. You should determine what event triggers the construction of a PasswordManager -- a tab being opened? a WebContents being created? something else? -- and state that here in the histogram description. https://codereview.chromium.org/23140005/diff/108001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:10468: + Indicates if the save password prompt disappeared without the user was able nit: "Indicates if" -> "Indicates whether" https://codereview.chromium.org/23140005/diff/108001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:10469: + to do a choice. nit: "without the user was able to do a choice" -> "in less than one second. This most likely indicates that the prompt was dismissed automatically, e.g. due to a page navigation, before the user was able to respond to the infobar."
@isherman Hi ! I made the changes asked could you please review my code ? I am staying at the office tonight in order to finish that cl =). Thanks https://codereview.chromium.org/23140005/diff/108001/chrome/browser/password_... File chrome/browser/password_manager/password_manager_delegate_impl.cc (right): https://codereview.chromium.org/23140005/diff/108001/chrome/browser/password_... chrome/browser/password_manager/password_manager_delegate_impl.cc:61: }; On 2013/09/12 23:43:40, Ilya Sherman wrote: > For a histogram with just one possible value, you might as well just emit to a > boolean histogram and omit the enum. Done. https://codereview.chromium.org/23140005/diff/108001/chrome/browser/password_... chrome/browser/password_manager/password_manager_delegate_impl.cc:143: base::TimeDelta::FromInternalValue(1000); On 2013/09/12 23:43:40, Ilya Sherman wrote: > Please don't use FromInternalValue. Instead, use one of the From* methods that > indicates the units of the value that you're constructing the TimeDelta from. Done. https://codereview.chromium.org/23140005/diff/108001/chrome/browser/password_... File chrome/browser/password_manager/password_manager_metrics_util.cc (right): https://codereview.chromium.org/23140005/diff/108001/chrome/browser/password_... chrome/browser/password_manager/password_manager_metrics_util.cc:22: size_t g_random_group_id = kGroupsPerDomain; On 2013/09/12 23:43:40, Ilya Sherman wrote: > Please add documentation for this variable. Done. https://codereview.chromium.org/23140005/diff/108001/chrome/browser/password_... chrome/browser/password_manager/password_manager_metrics_util.cc:103: if (group_id > 0 && group_id <= kNumGroups) On 2013/09/12 23:43:40, Ilya Sherman wrote: > nit: DCHECK_LE that group_id <= kNumGroups, since that should be an invariant. Done. https://codereview.chromium.org/23140005/diff/108001/chrome/browser/password_... chrome/browser/password_manager/password_manager_metrics_util.cc:105: return ""; On 2013/09/12 23:43:40, Ilya Sherman wrote: > nit: return std::string(); Done. https://codereview.chromium.org/23140005/diff/108001/chrome/browser/password_... File chrome/browser/password_manager/password_manager_metrics_util.h (right): https://codereview.chromium.org/23140005/diff/108001/chrome/browser/password_... chrome/browser/password_manager/password_manager_metrics_util.h:40: void GenerateRandomId(); On 2013/09/12 23:43:40, Ilya Sherman wrote: > Please move this declaration into the anonymous namespace in the .cc file. Done. https://codereview.chromium.org/23140005/diff/108001/chrome/browser/password_... chrome/browser/password_manager/password_manager_metrics_util.h:42: void SetRandomIdForTesting(size_t value); On 2013/09/12 23:43:40, Ilya Sherman wrote: > Please add documentation for this function. Done. https://codereview.chromium.org/23140005/diff/108001/chrome/browser/password_... File chrome/browser/password_manager/password_manager_metrics_util_unittest.cc (right): https://codereview.chromium.org/23140005/diff/108001/chrome/browser/password_... chrome/browser/password_manager/password_manager_metrics_util_unittest.cc:15: size_t IsMonitored(const char* url_host) { On 2013/09/12 23:43:40, Ilya Sherman wrote: > A method whose name starts with "Is" should return a boolean rather than a > size_t. My suggestion was to make this method be implemented as > > return password_manager_metrics_util::MonitoredDomainGroupId(url_host) > 0; Done. https://codereview.chromium.org/23140005/diff/108001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/23140005/diff/108001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:10444: + Indicates if the password manager is enable when PasswordManagers are On 2013/09/12 23:43:40, Ilya Sherman wrote: > nit: "Indicates if" -> "Indicates whether" Done. https://codereview.chromium.org/23140005/diff/108001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:10444: + Indicates if the password manager is enable when PasswordManagers are On 2013/09/12 23:43:40, Ilya Sherman wrote: > nit: "is enable" -> "is enabled" Done. https://codereview.chromium.org/23140005/diff/108001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:10444: + Indicates if the password manager is enable when PasswordManagers are On 2013/09/12 23:43:40, Ilya Sherman wrote: > "when PasswordManagers are constructed" is not very helpful to someone > unfamiliar with the codebase. You should determine what event triggers the > construction of a PasswordManager -- a tab being opened? a WebContents being > created? something else? -- and state that here in the histogram description. Done. https://codereview.chromium.org/23140005/diff/108001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:10468: + Indicates if the save password prompt disappeared without the user was able On 2013/09/12 23:43:40, Ilya Sherman wrote: > nit: "Indicates if" -> "Indicates whether" Done. https://codereview.chromium.org/23140005/diff/108001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:10469: + to do a choice. On 2013/09/12 23:43:40, Ilya Sherman wrote: > nit: "without the user was able to do a choice" -> "in less than one second. > This most likely indicates that the prompt was dismissed automatically, e.g. due > to a page navigation, before the user was able to respond to the infobar." Done.
Thanks! Just a few more little things: https://codereview.chromium.org/23140005/diff/123001/chrome/browser/password_... File chrome/browser/password_manager/password_manager_delegate_impl.cc (right): https://codereview.chromium.org/23140005/diff/123001/chrome/browser/password_... chrome/browser/password_manager/password_manager_delegate_impl.cc:79: // Save password prompt lifetime needed for a UMA signal. Optional nit: This comment might be slightly clearer as "Measures the "Save password?" prompt lifetime. Used to report an UMA signal." https://codereview.chromium.org/23140005/diff/123001/chrome/browser/password_... chrome/browser/password_manager/password_manager_delegate_impl.cc:123: "PasswordManager.SavePasswordPromptDisplayed" + uma_histogram_suffix_, Seems like there ought to be an underscore after the histogram name, i.e. the name should be "PasswordManager.SavePasswordPromptDisplayed_" https://codereview.chromium.org/23140005/diff/123001/chrome/browser/password_... chrome/browser/password_manager/password_manager_delegate_impl.cc:132: // The shortest period (in ms) for which the prompt needs to nit: No longer a need for "(in ms)" https://codereview.chromium.org/23140005/diff/123001/chrome/browser/password_... chrome/browser/password_manager/password_manager_delegate_impl.cc:137: base::TimeDelta::FromMilliseconds(1000); nit: Perhaps FromSeconds(1)? https://codereview.chromium.org/23140005/diff/123001/chrome/browser/password_... chrome/browser/password_manager/password_manager_delegate_impl.cc:144: "PasswordManager.SavePasswordPromptDisappearedQuickly" + Seems like there ought to be an underscore after the histogram name, i.e. the name should be "PasswordManager.SavePasswordPromptDisappearedQuickly_" https://codereview.chromium.org/23140005/diff/123001/chrome/browser/password_... File chrome/browser/password_manager/password_manager_metrics_util.cc (right): https://codereview.chromium.org/23140005/diff/123001/chrome/browser/password_... chrome/browser/password_manager/password_manager_metrics_util.cc:26: // each session. This isn't really a group id, but rather an index into group ids, right? Please update the comment accordingly, and possibly update the variable name to be "g_random_group_index". https://codereview.chromium.org/23140005/diff/123001/chrome/browser/password_... chrome/browser/password_manager/password_manager_metrics_util.cc:30: // If the |random_group_id| is equal to its initial value random_group_id -> g_random_group_id https://codereview.chromium.org/23140005/diff/123001/chrome/browser/password_... chrome/browser/password_manager/password_manager_metrics_util.cc:45: }; nit: Please move this into the anonymous namespace. https://codereview.chromium.org/23140005/diff/123001/chrome/browser/password_... File chrome/browser/password_manager/password_manager_metrics_util.h (right): https://codereview.chromium.org/23140005/diff/123001/chrome/browser/password_... chrome/browser/password_manager/password_manager_metrics_util.h:37: // Sets the |kGroupsPerDomain| to |value|. Needed in order to test the |kGroupsPerDomain| is a constant, not a variable, so you can't set it to some other value. I think you meant |g_random_group_id|... but since that's an implementation detail that's not exposed in the header file, it would be better to say something like "Sets the random seed used to generate group ids for monitored domains". https://codereview.chromium.org/23140005/diff/123001/chrome/browser/password_... File chrome/browser/password_manager/password_manager_metrics_util_unittest.cc (right): https://codereview.chromium.org/23140005/diff/123001/chrome/browser/password_... chrome/browser/password_manager/password_manager_metrics_util_unittest.cc:60: size_t group_id; This type is no longer correct -- it should be a bool. Better yet, though, would be to simply remove the variable, and write expectations like EXPECT_TRUE(IsMonitored("https://www.linkedin.com")); https://codereview.chromium.org/23140005/diff/123001/chrome/browser/password_... chrome/browser/password_manager/password_manager_metrics_util_unittest.cc:63: EXPECT_LT(0u, group_id); Note that EXPECT_LT doesn't really make sense anymore with a boolean return value from IsMonitored. https://codereview.chromium.org/23140005/diff/123001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/23140005/diff/123001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:10451: + Indicates whether the password manager is enabled when a tab being opened. nit: "when a tab being opened" -> "when a tab is opened". https://codereview.chromium.org/23140005/diff/123001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:10451: + Indicates whether the password manager is enabled when a tab being opened. Please mention that this includes prerendered tabs. https://codereview.chromium.org/23140005/diff/123001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:10482: + <summary>Indicates whether the save password prompt is displayed.</summary> nit: "is" -> "was" https://codereview.chromium.org/23140005/diff/123001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:10487: + <summary>Breakdown of responses offered by the save password prompt.</summary> Perhaps "Breakdown of which response the user selected from the save password prompt."
@isherman Hi Ilya, Thanks for the previous review. I made the changes asked, could you please review my code ? Thank you.
https://codereview.chromium.org/23140005/diff/105001/chrome/browser/password_... File chrome/browser/password_manager/password_manager_delegate_impl.cc (right): https://codereview.chromium.org/23140005/diff/105001/chrome/browser/password_... chrome/browser/password_manager/password_manager_delegate_impl.cc:136: // current WebContents is destroyed). nit: Please re-wrap this comment back to 80-col wrapping. https://codereview.chromium.org/23140005/diff/105001/chrome/browser/password_... File chrome/browser/password_manager/password_manager_metrics_util.cc (right): https://codereview.chromium.org/23140005/diff/105001/chrome/browser/password_... chrome/browser/password_manager/password_manager_metrics_util.cc:22: // The group id attributed to the user in order to report the behaviour of Again, this is not the group id; rather, it's used to determine the group id. https://codereview.chromium.org/23140005/diff/105001/chrome/browser/password_... chrome/browser/password_manager/password_manager_metrics_util.cc:37: // If the |g_random_group_index| is equal to its initial value "is equal" -> "is not equal"
https://codereview.chromium.org/23140005/diff/105001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/23140005/diff/105001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:27486: + <group name="bucket_10" label="bucket 10"/> "bucket" -> "group" throughout
Thank you Ilya for that fast review ! I made the change asked. Could you please review my code ? Thanks https://codereview.chromium.org/23140005/diff/105001/chrome/browser/password_... File chrome/browser/password_manager/password_manager_delegate_impl.cc (right): https://codereview.chromium.org/23140005/diff/105001/chrome/browser/password_... chrome/browser/password_manager/password_manager_delegate_impl.cc:136: // current WebContents is destroyed). On 2013/09/14 04:45:54, Ilya Sherman wrote: > nit: Please re-wrap this comment back to 80-col wrapping. Done. https://codereview.chromium.org/23140005/diff/105001/chrome/browser/password_... File chrome/browser/password_manager/password_manager_metrics_util.cc (right): https://codereview.chromium.org/23140005/diff/105001/chrome/browser/password_... chrome/browser/password_manager/password_manager_metrics_util.cc:22: // The group id attributed to the user in order to report the behaviour of On 2013/09/14 04:45:54, Ilya Sherman wrote: > Again, this is not the group id; rather, it's used to determine the group id. Sorry for it. https://codereview.chromium.org/23140005/diff/105001/chrome/browser/password_... chrome/browser/password_manager/password_manager_metrics_util.cc:37: // If the |g_random_group_index| is equal to its initial value On 2013/09/14 04:45:54, Ilya Sherman wrote: > "is equal" -> "is not equal" Done. https://codereview.chromium.org/23140005/diff/105001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/23140005/diff/105001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:27486: + <group name="bucket_10" label="bucket 10"/> On 2013/09/14 04:46:49, Ilya Sherman wrote: > "bucket" -> "group" throughout Done.
Thanks. LGTM except for a final two comments: https://codereview.chromium.org/23140005/diff/50010/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_delegate_impl.cc (right): https://codereview.chromium.org/23140005/diff/50010/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:134: // consider it killed prematurely, as might happen, e.g., if a pre-rendered nit: Please remove the doubled space after "might". https://codereview.chromium.org/23140005/diff/50010/tools/metrics/histograms/... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/23140005/diff/50010/tools/metrics/histograms/... tools/metrics/histograms/histograms.xml:27486: + <group name="group_10" label="group 10"/> There are 20 groups, right?
Hi Ilya, Thank you for all your reviews. I learned a lot during that cl thanks to you. I made the last changes asked. Thanks. https://codereview.chromium.org/23140005/diff/50010/chrome/browser/password_m... File chrome/browser/password_manager/password_manager_delegate_impl.cc (right): https://codereview.chromium.org/23140005/diff/50010/chrome/browser/password_m... chrome/browser/password_manager/password_manager_delegate_impl.cc:134: // consider it killed prematurely, as might happen, e.g., if a pre-rendered On 2013/09/14 05:35:54, Ilya Sherman wrote: > nit: Please remove the doubled space after "might". Done. https://codereview.chromium.org/23140005/diff/50010/tools/metrics/histograms/... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/23140005/diff/50010/tools/metrics/histograms/... tools/metrics/histograms/histograms.xml:27486: + <group name="group_10" label="group 10"/> On 2013/09/14 05:35:54, Ilya Sherman wrote: > There are 20 groups, right? You are right.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jdomingos@chromium.org/23140005/130002
Retried try job too often on linux_aura for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jdomingos@chromium.org/23140005/130002
Retried try job too often on linux_rel for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
Hi Jordy, A suggestion for how to fix the error on trybots below. Vaclav https://chromiumcodereview.appspot.com/23140005/diff/130002/chrome/browser/pa... File chrome/browser/password_manager/password_manager_metrics_util_unittest.cc (right): https://chromiumcodereview.appspot.com/23140005/diff/130002/chrome/browser/pa... chrome/browser/password_manager/password_manager_metrics_util_unittest.cc:21: TEST(PasswordManagerTest, MonitoredDomainGroupAssigmentTest) { Please change the test name to PasswordManagerMetricsUtilTest. Having the existing test class (PasswordManagerTest) makes the unittests fail, because it is used elsewhere with the slightly different TEST_F (fixture) macro.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jdomingos@chromium.org/23140005/161001
Sorry for I got bad news for ya. Compile failed with a clobber build on win_x64_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re... 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/jdomingos@chromium.org/23140005/161001
Message was sent while issue was closed.
Change committed as 223597 |
