Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
794133002:
[Password Manager] Add UMA signals to understand how often the password form submit navigate… (Closed)
|
Created:
6 years ago by Pritam Nikam Modified:
6 years ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
Description[Password Manager] Add UMA signals to understand how often the password form submit navigates to different domain or host.
BUG=435152
Committed: https://crrev.com/8b52071f8eafc83cb827334f8a281399fda51679
Cr-Commit-Position: refs/heads/master@{#308978}
Patch Set 1 : #
Total comments: 6
Patch Set 2 : Incorporated review comments. #
Total comments: 6
Patch Set 3 : Incorporated review inputs. #
Total comments: 8
Patch Set 4 : Addressed nits. #
Total comments: 6
Patch Set 5 : Incorporated review comments from Ilya. #
Total comments: 4
Patch Set 6 : Restored the c++ changes. #Patch Set 7 : Rebase. #
Messages
Total messages: 28 (8 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
pritam.nikam@samsung.com changed reviewers: + vabr@chromium.org
Hi Vaclav, I've added UMA matrics for password form navigating to different site on submission, PTAL. Thanks!
Hi Pritam, First -- did you remove the Cc line? If yes, please do not remove it, and put it back here! It contains the all-reviews alias, as well as watch e-mails of interested developers, and we should not skip those. Second -- I do not think you are comparing the right URLs here. Think from the perspective of the user: the important URLs are those of the main frame (seen in the Omnibox), before and after submitting the form. Note that: (1) Forms inside iframes can have different origin than the containing main frame. (2) Action URL is the next, not the ultimate step in the redirect chain after submission. The action URL is not necessarily what the user sees in Omnibox after the submission. Cheers, Vaclav https://codereview.chromium.org/794133002/diff/40001/chrome/browser/password_... File chrome/browser/password_manager/chrome_password_manager_client.cc (right): https://codereview.chromium.org/794133002/diff/40001/chrome/browser/password_... chrome/browser/password_manager/chrome_password_manager_client.cc:68: void LogFormSubmitNavigateToDifferentDomain( nit: Log->Report (logging is usually used in the debug sense of the word, while UMA metrics are reported, recorded, or just sent). Further, the name suggests that the navigation was to a different domain (while it should name the question about that). So what about RecordWhetherTargetDomainDiffers https://codereview.chromium.org/794133002/diff/40001/chrome/browser/password_... chrome/browser/password_manager/chrome_password_manager_client.cc:227: // Capture the UMA stats to get to know the volume and frequency for I don't think this comment is necessary. We always use UMA to "get to know the volume and frequency" :), and the rest is covered by the method name. https://codereview.chromium.org/794133002/diff/40001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/794133002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:22958: + different domain/host or not. This is slightly different from the check you make. Please say: "Indicates whether submitting a password login form changes the registry controlled domain of the main frame."
Patchset #2 (id:60001) has been deleted
Thanks Vaclav for review. I've attempted addressing your review inputs along patch set #2, PTAL, Thanks! https://codereview.chromium.org/794133002/diff/40001/chrome/browser/password_... File chrome/browser/password_manager/chrome_password_manager_client.cc (right): https://codereview.chromium.org/794133002/diff/40001/chrome/browser/password_... chrome/browser/password_manager/chrome_password_manager_client.cc:68: void LogFormSubmitNavigateToDifferentDomain( On 2014/12/11 12:31:33, vabr (Chromium) wrote: > nit: Log->Report (logging is usually used in the debug sense of the word, while > UMA metrics are reported, recorded, or just sent). > Further, the name suggests that the navigation was to a different domain (while > it should name the question about that). So what about > > RecordWhetherTargetDomainDiffers Done. https://codereview.chromium.org/794133002/diff/40001/chrome/browser/password_... chrome/browser/password_manager/chrome_password_manager_client.cc:227: // Capture the UMA stats to get to know the volume and frequency for On 2014/12/11 12:31:33, vabr (Chromium) wrote: > I don't think this comment is necessary. We always use UMA to "get to know the > volume and frequency" :), and the rest is covered by the method name. Done. https://codereview.chromium.org/794133002/diff/40001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/794133002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:22958: + different domain/host or not. On 2014/12/11 12:31:33, vabr (Chromium) wrote: > This is slightly different from the check you make. Please say: > "Indicates whether submitting a password login form changes the registry > controlled domain of the main frame." Done.
Hi Pritam Nikam, Thanks, this looks better than the the previous patch. I still think it could be improved, though. First -- are you sure that this would ever report different URLs? If I read it correctly, it records the main frame URL in OnPasswordFormsParsed, which happens just shortly before OnPasswordFormsRendered during the same load. The comparison of the current and stored URL is done in the latter event. Also, PromptUserToSavePassword could be called from ContentCredentialManagerDispatcher. I suggest the following changes: (1) In the client, only add a method to get the main frame URL. The client does not need to bother about the purpose of getting the URL. (2) The PasswordManager should then snapshot that URL when provisionally saving a password, and then compare+report it just before it would issue the save-password prompt. Cheers, Vaclav https://codereview.chromium.org/794133002/diff/80001/chrome/browser/password_... File chrome/browser/password_manager/chrome_password_manager_client.cc (right): https://codereview.chromium.org/794133002/diff/80001/chrome/browser/password_... chrome/browser/password_manager/chrome_password_manager_client.cc:65: // Helper function to capture the UMA matriculations for instances where What does "matriculations" in this context mean? http://dictionary.cambridge.org/dictionary/british/matriculate?q=matriculation seems like not what one expects. https://codereview.chromium.org/794133002/diff/80001/chrome/browser/password_... chrome/browser/password_manager/chrome_password_manager_client.cc:68: void RecordWhetherTargetDomainDiffers(const GURL& url1, const GURL& url2) { nit: to put in context with the method name, maybe rename url1 and url2 to src and target, respectively. https://codereview.chromium.org/794133002/diff/80001/chrome/browser/password_... File chrome/browser/password_manager/chrome_password_manager_client.h (right): https://codereview.chromium.org/794133002/diff/80001/chrome/browser/password_... chrome/browser/password_manager/chrome_password_manager_client.h:174: // Visible URL for password form being parsed, i.e. of the main frame (seen in Please just call the variable: main_frame_url_ and replace the comment by adding the information about usage: // The user-visible URL for UMA reports.
Thanks Vaclav for your inputs. I've addressed these along newest patch (#3). PTAL. Thanks! https://codereview.chromium.org/794133002/diff/80001/chrome/browser/password_... File chrome/browser/password_manager/chrome_password_manager_client.cc (right): https://codereview.chromium.org/794133002/diff/80001/chrome/browser/password_... chrome/browser/password_manager/chrome_password_manager_client.cc:65: // Helper function to capture the UMA matriculations for instances where On 2014/12/12 16:24:00, vabr (Chromium) wrote: > What does "matriculations" in this context mean? > http://dictionary.cambridge.org/dictionary/british/matriculate?q=matriculation > seems like not what one expects. Done. https://codereview.chromium.org/794133002/diff/80001/chrome/browser/password_... chrome/browser/password_manager/chrome_password_manager_client.cc:68: void RecordWhetherTargetDomainDiffers(const GURL& url1, const GURL& url2) { On 2014/12/12 16:24:00, vabr (Chromium) wrote: > nit: to put in context with the method name, maybe rename url1 and url2 to src > and target, respectively. Done. https://codereview.chromium.org/794133002/diff/80001/chrome/browser/password_... File chrome/browser/password_manager/chrome_password_manager_client.h (right): https://codereview.chromium.org/794133002/diff/80001/chrome/browser/password_... chrome/browser/password_manager/chrome_password_manager_client.h:174: // Visible URL for password form being parsed, i.e. of the main frame (seen in On 2014/12/12 16:24:00, vabr (Chromium) wrote: > Please just call the variable: > main_frame_url_ > and replace the comment by adding the information about usage: > // The user-visible URL for UMA reports. Done.
Thanks, Pritam. This LGTM with a couple of nits below. On important thing, though -- before landing please do test this out with both a website where the registry controlled domain changes through the login navigation, and with a website where it does not change. I'm not completely sure about when the main frame URL is changed. Also, for the histograms.xml, you'll need either isherman@ or asvitkine@ to approve your change. Cheers, Vaclav https://codereview.chromium.org/794133002/diff/100001/components/password_man... File components/password_manager/core/browser/password_manager.cc (right): https://codereview.chromium.org/794133002/diff/100001/components/password_man... components/password_manager/core/browser/password_manager.cc:101: // Helper function to capture the UMA matrics for instances where submitting a typo: matrics -> metrics https://codereview.chromium.org/794133002/diff/100001/components/password_man... components/password_manager/core/browser/password_manager.cc:101: // Helper function to capture the UMA matrics for instances where submitting a Currently this comment is spending a lot of words on not saying much: Helper function ... where <elaborate condition> or not. Note that the part following "where" can be left out as it says "where <A> or <not A>". You can either leave the comment out completely (the function name is rather clear, and the UMA macro is just a couple of lines below), or shorten it to something like: // Helper UMA reporting function for differences in URLs during form submission. https://codereview.chromium.org/794133002/diff/100001/components/password_man... components/password_manager/core/browser/password_manager.cc:325: // Cache the user-visible URL (i.e. one seen in the omnibox). Comparing it nit: "i.e." should be followed by a comma. Also: one -> the one. https://codereview.chromium.org/794133002/diff/100001/components/password_man... components/password_manager/core/browser/password_manager.cc:325: // Cache the user-visible URL (i.e. one seen in the omnibox). Comparing it I suggest simplifying the whole comment as follows: // Cache the user-visible URL (i.e., the one seen in the omnibox). Once the post-submit navigation concludes, we compare the landing URL against the cached and report the difference through UMA. https://codereview.chromium.org/794133002/diff/100001/components/password_man... File components/password_manager/core/browser/password_manager.h (right): https://codereview.chromium.org/794133002/diff/100001/components/password_man... components/password_manager/core/browser/password_manager.h:202: // The user-visible URL for UMA reports. Rather than saying "for UMA reports", please say: " from the last time a password was provisionally saved."
pritam.nikam@samsung.com changed reviewers: + isherman@chromium.org
+ Ilya, Thanks Vaclav for review! Hi Ilya, PTAL @ tools/metrics/histograms/histograms.xml Feel free to glance through rest of the changes as well. Thanks! https://codereview.chromium.org/794133002/diff/100001/components/password_man... File components/password_manager/core/browser/password_manager.cc (right): https://codereview.chromium.org/794133002/diff/100001/components/password_man... components/password_manager/core/browser/password_manager.cc:101: // Helper function to capture the UMA matrics for instances where submitting a On 2014/12/15 09:53:51, vabr (OOO soon) wrote: > Currently this comment is spending a lot of words on not saying much: > Helper function ... where <elaborate condition> or not. Note that the part > following "where" can be left out as it says "where <A> or <not A>". > > You can either leave the comment out completely (the function name is rather > clear, and the UMA macro is just a couple of lines below), or shorten it to > something like: > // Helper UMA reporting function for differences in URLs during form submission. Done. https://codereview.chromium.org/794133002/diff/100001/components/password_man... components/password_manager/core/browser/password_manager.cc:325: // Cache the user-visible URL (i.e. one seen in the omnibox). Comparing it On 2014/12/15 09:53:51, vabr (OOO soon) wrote: > nit: "i.e." should be followed by a comma. Also: one -> the one. Done. https://codereview.chromium.org/794133002/diff/100001/components/password_man... File components/password_manager/core/browser/password_manager.h (right): https://codereview.chromium.org/794133002/diff/100001/components/password_man... components/password_manager/core/browser/password_manager.h:202: // The user-visible URL for UMA reports. On 2014/12/15 09:53:51, vabr (OOO soon) wrote: > Rather than saying "for UMA reports", please say: " from the last time a > password was provisionally saved." Done.
Hi Pritam, Please confirm that you have tested your patch on both a page where the login and landing domain differ, and where they are the same. Also please replace the word "matriculations" in the CL description and title with "signals". Otherwise, patch set 4 LGTM. Cheers! Vaclav
On 2014/12/15 16:15:51, vabr (OOO soon) wrote: > Hi Pritam, > > Please confirm that you have tested your patch on both a page where the login > and landing domain differ, and where they are the same. > > Also please replace the word "matriculations" in the CL description and title > with "signals". > > Otherwise, patch set 4 LGTM. > > Cheers! > Vaclav Hi Vaclav, I've tested this cl for both where the registry controlled domain changes through the login navigation, and where it does not change, through my locally hosted test pages/server (& with iframes etc..). However, I didn't find any real website where navigation redirects to different domain. Thanks!
Pritam, just to double-check, are you aware that UMA metrics are only visible to Googlers? I'm a little curious how you chose to work on this particular bug. In any case, thanks for adding the metrics. Some minor comments below: https://codereview.chromium.org/794133002/diff/120001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/794133002/diff/120001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:22999: +<histogram name="PasswordManager.SubmitNavigatesToDifferentDomain" I think "origin" is a better term than "domain", unless you are measuring something different from the origin. https://codereview.chromium.org/794133002/diff/120001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:23000: + enum="Boolean"> Please define and use a custom enumeration, with the labels "Same origin" and "Different origin".
Thanks Ilya for review. I've addressed your inputs along newest patch set. PTAL! Thanks! https://codereview.chromium.org/794133002/diff/120001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/794133002/diff/120001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:22999: +<histogram name="PasswordManager.SubmitNavigatesToDifferentDomain" On 2014/12/15 19:31:14, Ilya Sherman wrote: > I think "origin" is a better term than "domain", unless you are measuring > something different from the origin. Done. https://codereview.chromium.org/794133002/diff/120001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:23000: + enum="Boolean"> On 2014/12/15 19:31:14, Ilya Sherman wrote: > Please define and use a custom enumeration, with the labels "Same origin" and > "Different origin". Done.
Thanks, Pritam! LGTM. The testing on your test domains is OK, thanks for confirming. I had to push back to Ilya's suggestion about renaming domains to origins. Because I'm leaving for holidays really soon now, please go with whatever decision Ilya makes, don't wait for further approval from me. Ilya -- to your point about Pritam not being able to see the signals: that's unfortunately true, but we will have a look, and decide the next steps based on the data. The next steps can be done by Pritam again, if he will be still interested. Thanks! Vaclav https://codereview.chromium.org/794133002/diff/120001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/794133002/diff/120001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:22999: +<histogram name="PasswordManager.SubmitNavigatesToDifferentDomain" On 2014/12/15 19:31:14, Ilya Sherman wrote: > I think "origin" is a better term than "domain", unless you are measuring > something different from the origin. Actually, this signal does measure something different than an origin: it checks if the registry controlled domains are the same or not. This is as in fuzzy password matching -- m.facebook.com and www.facebook.com would be reported as the same "domain", but they are not the same origin. https://codereview.chromium.org/794133002/diff/140001/components/password_man... File components/password_manager/core/browser/password_manager.cc (right): https://codereview.chromium.org/794133002/diff/140001/components/password_man... components/password_manager/core/browser/password_manager.cc:101: const int registry_controlled_domains_same = 0; Please do not use const int, use an enum, with names corresponding to those defined in histograms.xml.
https://codereview.chromium.org/794133002/diff/120001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/794133002/diff/120001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:22999: +<histogram name="PasswordManager.SubmitNavigatesToDifferentDomain" On 2014/12/16 10:53:37, vabr (back in January) wrote: > On 2014/12/15 19:31:14, Ilya Sherman wrote: > > I think "origin" is a better term than "domain", unless you are measuring > > something different from the origin. > > Actually, this signal does measure something different than an origin: it checks > if the registry controlled domains are the same or not. This is as in fuzzy > password matching -- http://m.facebook.com and http://www.facebook.com would be reported as > the same "domain", but they are not the same origin. Ah, thanks for correcting me. In that case, "domain" is the more correct term. Apologies, Pritam, for the bad advice. https://codereview.chromium.org/794133002/diff/140001/components/password_man... File components/password_manager/core/browser/password_manager.cc (right): https://codereview.chromium.org/794133002/diff/140001/components/password_man... components/password_manager/core/browser/password_manager.cc:122: GetAllNavigationTypes()); Please revert your changes to the C++ code in this latest patch set. You should continue to use an UMA_HISTOGRAM_BOOLEAN macro, as you were before. My request was just that you associate labels for the histogram in histograms.xml.
Thanks Ilya & Vaclav. PTAL at new patch set. Thanks! https://codereview.chromium.org/794133002/diff/140001/components/password_man... File components/password_manager/core/browser/password_manager.cc (right): https://codereview.chromium.org/794133002/diff/140001/components/password_man... components/password_manager/core/browser/password_manager.cc:101: const int registry_controlled_domains_same = 0; On 2014/12/16 10:53:37, vabr (back in January) wrote: > Please do not use const int, use an enum, with names corresponding to those > defined in histograms.xml. Done. https://codereview.chromium.org/794133002/diff/140001/components/password_man... components/password_manager/core/browser/password_manager.cc:122: GetAllNavigationTypes()); On 2014/12/16 19:56:26, Ilya Sherman wrote: > Please revert your changes to the C++ code in this latest patch set. You should > continue to use an UMA_HISTOGRAM_BOOLEAN macro, as you were before. My request > was just that you associate labels for the histogram in histograms.xml. Done.
LGTM, thanks.
The CQ bit was checked by isherman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/794133002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/10...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by pritam.nikam@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/794133002/180001
Message was sent while issue was closed.
Committed patchset #7 (id:180001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/8b52071f8eafc83cb827334f8a281399fda51679 Cr-Commit-Position: refs/heads/master@{#308978} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
