Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10787023:
Adding UMA stats. (Closed)
|
Created:
8 years, 5 months ago by zysxqn Modified:
8 years, 5 months ago CC:
chromium-reviews, dhollowa+watch_chromium.org, brettw-cc_chromium.org, dyu1, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFirst version to add some UMA stats to the chrome histograms. This change covers most part of the main flow chart along the password generation function, including:
- detected account creation form
- password generation icon shown
- password generation bubble shown (Linux)
- regeneration password button clicked (Linux)
- password entry edited by users (Linux)
- learn more link clicked (Linux)
- generated password accepted (Linux)
- generated password submitted
All this are simple counters for now and Windows UI will come next as a separate CL.
BUG=130330
TEST=unit tests, browser tests
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=147757
Patch Set 1 #
Total comments: 6
Patch Set 2 : Change to enumeration histogram. #Patch Set 3 : A nit. #Patch Set 4 : #Patch Set 5 : Another nit. #Patch Set 6 : Fix the test. #
Total comments: 10
Patch Set 7 : Don't update histograms in the renderer. #
Total comments: 18
Patch Set 8 : Some nits. #
Total comments: 2
Patch Set 9 : More comments. #
Total comments: 6
Patch Set 10 : Separate renderer histogram and browser histogram. #
Total comments: 5
Patch Set 11 : Add one more stat. #Patch Set 12 : #Patch Set 13 : Fix a compiler error. #Patch Set 14 : Avoid using anonymous enum. #Patch Set 15 : Rename IGNORE. #Patch Set 16 : A typo. #
Messages
Total messages: 39 (0 generated)
It would be nice if we could consolidate these more into some common location so that we didn't have to write these out for every OS. Some of the stats (bubble creation, password accepted) can be pushed into the renderer, but I'm not sure what to do about the others. Adding a base class for the bubbles seems like more trouble that it's worth given the amount of duplication. http://codereview.chromium.org/10787023/diff/1/chrome/browser/password_manage... File chrome/browser/password_manager/password_manager.cc (left): http://codereview.chromium.org/10787023/diff/1/chrome/browser/password_manage... chrome/browser/password_manager/password_manager.cc:100: // TODO(gcasto): Add UMA stats to track this. This comment was meant for this particular case, specifically if we were creating a PasswordFormManager here instead on when the page is parsed. Basically I don't think that it should happen at the moment, so I'd like to know if it is. http://codereview.chromium.org/10787023/diff/1/chrome/browser/ui/gtk/password... File chrome/browser/ui/gtk/password_generation_bubble_gtk.cc (right): http://codereview.chromium.org/10787023/diff/1/chrome/browser/ui/gtk/password... chrome/browser/ui/gtk/password_generation_bubble_gtk.cc:99: "PasswordGeneration.PasswordGenerationBubbleShown_Linux", 1); You can break down UMA stats by OS so you don't need to encode that information in the stat name. http://codereview.chromium.org/10787023/diff/1/chrome/browser/ui/gtk/password... chrome/browser/ui/gtk/password_generation_bubble_gtk.cc:141: UMA_HISTOGRAM_COUNTS("PasswordGeneration.PasswordEditedByUsers", 1); I would think that we would want to know if the password had been edited when it's submitted, not how many times it's been edited. So have this function simply set a boolean and read it during OnAcceptClicked and possibly increment the count.
I strongly recommend using an enumerated histogram (or several, as appropriate), rather than lots of individual "counts" histograms for this. Enumerated histograms naturally provide context for any numbers, so that you can easily make connections like "70% of the time, users generate a password after seeing the bubble". Otherwise, you just get lots of numbers like "20,000" users generated a password today, which is harder to make sense of. In general, for any metrics that you add, I strongly encourage you to think of how you'll be interpreting the metric and deciding whether what you're seeing is "good" or "as desired" vs. "bad" or "surprising". For metrics that essentially track user actions, this typically involves creating an enumerated histogram (or several) with a baseline to compare to as well as the metric that you directly wanted to measure.
Hmm, I'm not sure enumerated histogram would give us much advantage in this case, since most of our interested metrics are binary in nature. Following your example, we are interested in seeing something like "70% of the time, users generate a password after seeing the bubble". But I think it's easier to just record the number of times users see the bubble and the number of times users generate the password, and calculate the ratio ourselves. If we use enum, the only other value is that users "don't generate a password after seeing the bubble (i.e. ignore the bubble)", which is very hard to track. Am I missing anything? On Mon, Jul 16, 2012 at 3:33 PM, <isherman@chromium.org> wrote: > I strongly recommend using an enumerated histogram (or several, as > appropriate), > rather than lots of individual "counts" histograms for this. Enumerated > histograms naturally provide context for any numbers, so that you can > easily > make connections like "70% of the time, users generate a password after > seeing > the bubble". Otherwise, you just get lots of numbers like "20,000" users > generated a password today, which is harder to make sense of. > > In general, for any metrics that you add, I strongly encourage you to > think of > how you'll be interpreting the metric and deciding whether what you're > seeing is > "good" or "as desired" vs. "bad" or "surprising". For metrics that > essentially > track user actions, this typically involves creating an enumerated > histogram (or > several) with a baseline to compare to as well as the metric that you > directly > wanted to measure. > > http://codereview.chromium.**org/10787023/<http://codereview.chromium.org/107... >
On 2012/07/17 17:02:04, zysxqn wrote: > Hmm, I'm not sure enumerated histogram would give us much advantage in this > case, since most of our interested metrics are binary in nature. Following > your example, we are interested in seeing something like "70% of the time, > users generate a password after seeing the bubble". But I think it's easier > to just record the number of times users see the bubble and the number of > times users generate the password, and calculate the ratio ourselves. If we > use enum, the only other value is that users "don't generate a password > after seeing the bubble (i.e. ignore the bubble)", which is very hard to > track. Am I missing anything? The enumerated histogram for the case you describe would simply be a single histogram that records the number of times users see the bubble as one enumerated value, and the number of times users generate the password as another enumerated value. These are the same events you are already planning to measure -- the difference is just that they are combined into a single histogram, so that it's easy to visually group and compare them. This is much more convenient for evaluating the data than manually crunching the numbers is. > On Mon, Jul 16, 2012 at 3:33 PM, <mailto:isherman@chromium.org> wrote: > > > I strongly recommend using an enumerated histogram (or several, as > > appropriate), > > rather than lots of individual "counts" histograms for this. Enumerated > > histograms naturally provide context for any numbers, so that you can > > easily > > make connections like "70% of the time, users generate a password after > > seeing > > the bubble". Otherwise, you just get lots of numbers like "20,000" users > > generated a password today, which is harder to make sense of. > > > > In general, for any metrics that you add, I strongly encourage you to > > think of > > how you'll be interpreting the metric and deciding whether what you're > > seeing is > > "good" or "as desired" vs. "bad" or "surprising". For metrics that > > essentially > > track user actions, this typically involves creating an enumerated > > histogram (or > > several) with a baseline to compare to as well as the metric that you > > directly > > wanted to measure. > > > > > http://codereview.chromium.**org/10787023/%3Chttp://codereview.chromium.org/1...> > >
This makes sense to me. I will update the code shortly. Thanks! On Tue, Jul 17, 2012 at 3:56 PM, <isherman@chromium.org> wrote: > On 2012/07/17 17:02:04, zysxqn wrote: > >> Hmm, I'm not sure enumerated histogram would give us much advantage in >> this >> case, since most of our interested metrics are binary in nature. Following >> your example, we are interested in seeing something like "70% of the time, >> users generate a password after seeing the bubble". But I think it's >> easier >> to just record the number of times users see the bubble and the number of >> times users generate the password, and calculate the ratio ourselves. If >> we >> use enum, the only other value is that users "don't generate a password >> after seeing the bubble (i.e. ignore the bubble)", which is very hard to >> track. Am I missing anything? >> > > The enumerated histogram for the case you describe would simply be a single > histogram that records the number of times users see the bubble as one > enumerated value, and the number of times users generate the password as > another > enumerated value. These are the same events you are already planning to > measure > -- the difference is just that they are combined into a single histogram, > so > that it's easy to visually group and compare them. This is much more > convenient > for evaluating the data than manually crunching the numbers is. > > > On Mon, Jul 16, 2012 at 3:33 PM, <mailto:isherman@chromium.org> wrote: >> > > > I strongly recommend using an enumerated histogram (or several, as >> > appropriate), >> > rather than lots of individual "counts" histograms for this. Enumerated >> > histograms naturally provide context for any numbers, so that you can >> > easily >> > make connections like "70% of the time, users generate a password after >> > seeing >> > the bubble". Otherwise, you just get lots of numbers like "20,000" >> users >> > generated a password today, which is harder to make sense of. >> > >> > In general, for any metrics that you add, I strongly encourage you to >> > think of >> > how you'll be interpreting the metric and deciding whether what you're >> > seeing is >> > "good" or "as desired" vs. "bad" or "surprising". For metrics that >> > essentially >> > track user actions, this typically involves creating an enumerated >> > histogram (or >> > several) with a baseline to compare to as well as the metric that you >> > directly >> > wanted to measure. >> > >> > >> > > http://codereview.chromium.****org/10787023/%3Chttp://coderev** > iew.chromium.org/10787023/ <http://codereview.chromium.org/10787023/>> > >> > >> > > > > http://codereview.chromium.**org/10787023/<http://codereview.chromium.org/107... >
This update has two major changes:
(1) User enumeration histogram and we have two histograms now:
PasswordGeneration.Events:
- Sign up form detected.
- Icon shown.
- Bubble Shown.
- Generated password submitted
PasswordGeneration.UserActions: actions after user see the bubble
- Ignore.
- Click on the learn more link.
- Accept original password
- Accept password after manually editing
(2) Refactor the UI such that it only collects relevant status of the user
actions and send all of them to the renderer, and all actual processing happen
in the renderer now.
Please have another look.
http://codereview.chromium.org/10787023/diff/1/chrome/browser/password_manage...
File chrome/browser/password_manager/password_manager.cc (left):
http://codereview.chromium.org/10787023/diff/1/chrome/browser/password_manage...
chrome/browser/password_manager/password_manager.cc:100: // TODO(gcasto): Add
UMA stats to track this.
On 2012/07/16 22:32:39, Garrett Casto wrote:
> This comment was meant for this particular case, specifically if we were
> creating a PasswordFormManager here instead on when the page is parsed.
Oops, misunderstood your meaning here. Put it back.
> Basically I don't think that it should happen at the moment, so I'd like to
know
> if it is.
http://codereview.chromium.org/10787023/diff/1/chrome/browser/ui/gtk/password...
File chrome/browser/ui/gtk/password_generation_bubble_gtk.cc (right):
http://codereview.chromium.org/10787023/diff/1/chrome/browser/ui/gtk/password...
chrome/browser/ui/gtk/password_generation_bubble_gtk.cc:99:
"PasswordGeneration.PasswordGenerationBubbleShown_Linux", 1);
On 2012/07/16 22:32:39, Garrett Casto wrote:
> You can break down UMA stats by OS so you don't need to encode that
information
> in the stat name.
Done.
http://codereview.chromium.org/10787023/diff/1/chrome/browser/ui/gtk/password...
chrome/browser/ui/gtk/password_generation_bubble_gtk.cc:141:
UMA_HISTOGRAM_COUNTS("PasswordGeneration.PasswordEditedByUsers", 1);
On 2012/07/16 22:32:39, Garrett Casto wrote:
> I would think that we would want to know if the password had been edited when
> it's submitted, not how many times it's been edited. So have this function
> simply set a boolean and read it during OnAcceptClicked and possibly increment
> the count.
Done.
http://codereview.chromium.org/10787023/diff/10012/chrome/browser/password_ma... File chrome/browser/password_manager/password_manager.cc (right): http://codereview.chromium.org/10787023/diff/10012/chrome/browser/password_ma... chrome/browser/password_manager/password_manager.cc:245: UMA_HISTOGRAM_ENUMERATION("PasswordGeneration.EVENTS", This histogram name does not match the name in other places, where you use "PasswordGeneration.Events". In general, the best way to avoid such errors is to only have one UMA_HISTOGRAM_ENUMERATION call site for each histogram name -- often in a helper function -- and to call that helper function with the enum value that you want to log from the various places where you want to log the event. That way, you get compile-time checking that the histogram is uniformly named. http://codereview.chromium.org/10787023/diff/10012/chrome/browser/ui/gtk/pass... File chrome/browser/ui/gtk/password_generation_bubble_gtk.cc (right): http://codereview.chromium.org/10787023/diff/10012/chrome/browser/ui/gtk/pass... chrome/browser/ui/gtk/password_generation_bubble_gtk.cc:118: password_generation_status_)); What's the advantage of sending the status to the renderer, rather than logging the histograms from the browser process? In general, we prefer to (a) do more in the browser, and less in the renderer; and (b) send less data from one process to the other. In this case, I'm not seeing the gain from pushing this work to the renderer. http://codereview.chromium.org/10787023/diff/10012/chrome/renderer/autofill/p... File chrome/renderer/autofill/password_generation_manager.cc (right): http://codereview.chromium.org/10787023/diff/10012/chrome/renderer/autofill/p... chrome/renderer/autofill/password_generation_manager.cc:25: using namespace password_generation; "using namespace" is disallowed by the Chromium style guide
http://codereview.chromium.org/10787023/diff/10012/chrome/browser/ui/gtk/pass... File chrome/browser/ui/gtk/password_generation_bubble_gtk.cc (right): http://codereview.chromium.org/10787023/diff/10012/chrome/browser/ui/gtk/pass... chrome/browser/ui/gtk/password_generation_bubble_gtk.cc:118: password_generation_status_)); On 2012/07/18 05:36:15, Ilya Sherman wrote: > What's the advantage of sending the status to the renderer, rather than logging > the histograms from the browser process? In general, we prefer to (a) do more > in the browser, and less in the renderer; and (b) send less data from one > process to the other. In this case, I'm not seeing the gain from pushing this > work to the renderer. Reading back through the comments on this CL, I think you might be doing this based on Garrett's comment that "Some of the stats (bubble creation, password accepted) can be pushed into the renderer". I don't think this is what he meant, though I'll let him clarify in case I'm misunderstanding.
On Tue, Jul 17, 2012 at 10:38 PM, <isherman@chromium.org> wrote: > > http://codereview.chromium.**org/10787023/diff/10012/** > chrome/browser/ui/gtk/**password_generation_bubble_**gtk.cc<http://codereview.chromium.org/10787023/diff/10012/chrome/browser/ui/gtk/password_generation_bubble_gtk.cc> > File chrome/browser/ui/gtk/**password_generation_bubble_**gtk.cc (right): > > http://codereview.chromium.**org/10787023/diff/10012/** > chrome/browser/ui/gtk/**password_generation_bubble_**gtk.cc#newcode118<http://codereview.chromium.org/10787023/diff/10012/chrome/browser/ui/gtk/password_generation_bubble_gtk.cc#newcode118> > chrome/browser/ui/gtk/**password_generation_bubble_**gtk.cc:118: > password_generation_status_)); > On 2012/07/18 05:36:15, Ilya Sherman wrote: > >> What's the advantage of sending the status to the renderer, rather >> > than logging > >> the histograms from the browser process? In general, we prefer to (a) >> > do more > >> in the browser, and less in the renderer; and (b) send less data from >> > one > >> process to the other. In this case, I'm not seeing the gain from >> > pushing this > >> work to the renderer. >> > > Reading back through the comments on this CL, I think you might be doing > this based on Garrett's comment that "Some of the stats (bubble > creation, password accepted) can be pushed into the renderer". I don't > think this is what he meant, though I'll let him clarify in case I'm > misunderstanding. > I think his point is to minimize the code in the platform-dependent UI class. Right now if we keep the histogram code in the browser process we have to put it in each UI class. Given your comment, I think the best way is adding a platform-independent PasswordGenerationBubble parent class (e.g. in browser/ui) and put all common code (including updating the histograms) there. Thoughts? > > http://codereview.chromium.**org/10787023/<http://codereview.chromium.org/107... >
http://codereview.chromium.org/10787023/diff/10012/chrome/browser/ui/gtk/pass... File chrome/browser/ui/gtk/password_generation_bubble_gtk.cc (right): http://codereview.chromium.org/10787023/diff/10012/chrome/browser/ui/gtk/pass... chrome/browser/ui/gtk/password_generation_bubble_gtk.cc:118: password_generation_status_)); On 2012/07/18 05:38:14, Ilya Sherman wrote: > On 2012/07/18 05:36:15, Ilya Sherman wrote: > > What's the advantage of sending the status to the renderer, rather than > logging > > the histograms from the browser process? In general, we prefer to (a) do more > > in the browser, and less in the renderer; and (b) send less data from one > > process to the other. In this case, I'm not seeing the gain from pushing this > > work to the renderer. > > Reading back through the comments on this CL, I think you might be doing this > based on Garrett's comment that "Some of the stats (bubble creation, password > accepted) can be pushed into the renderer". I don't think this is what he > meant, though I'll let him clarify in case I'm misunderstanding. Yeah, I just meant that for those particular stats you can already tell in the renderer that they are happening so you wouldn't need to edit each individual UI file. Since we want to keep track of other stats as well I don't know if it actually makes sense to log them there, I was more musing about possible ways to cut down on code duplication. Given the setup now, I think that just having a helper function that logs a PasswordGenerationStatus would be better. http://codereview.chromium.org/10787023/diff/10012/chrome/browser/ui/password... File chrome/browser/ui/password_generation_status.h (right): http://codereview.chromium.org/10787023/diff/10012/chrome/browser/ui/password... chrome/browser/ui/password_generation_status.h:53: struct PasswordGenerationStatus { Given the other suggestions, can you drop the generated_password here and rename this to PasswordGenerationActions.
http://codereview.chromium.org/10787023/diff/10012/chrome/browser/password_ma... File chrome/browser/password_manager/password_manager.cc (right): http://codereview.chromium.org/10787023/diff/10012/chrome/browser/password_ma... chrome/browser/password_manager/password_manager.cc:245: UMA_HISTOGRAM_ENUMERATION("PasswordGeneration.EVENTS", On 2012/07/18 05:36:15, Ilya Sherman wrote: > This histogram name does not match the name in other places, where you use > "PasswordGeneration.Events". In general, the best way to avoid such errors is > to only have one UMA_HISTOGRAM_ENUMERATION call site for each histogram name -- > often in a helper function -- and to call that helper function with the enum > value that you want to log from the various places where you want to log the > event. That way, you get compile-time checking that the histogram is uniformly > named. Done. http://codereview.chromium.org/10787023/diff/10012/chrome/browser/ui/gtk/pass... File chrome/browser/ui/gtk/password_generation_bubble_gtk.cc (right): http://codereview.chromium.org/10787023/diff/10012/chrome/browser/ui/gtk/pass... chrome/browser/ui/gtk/password_generation_bubble_gtk.cc:118: password_generation_status_)); On 2012/07/18 17:20:03, Garrett Casto wrote: > On 2012/07/18 05:38:14, Ilya Sherman wrote: > > On 2012/07/18 05:36:15, Ilya Sherman wrote: > > > What's the advantage of sending the status to the renderer, rather than > > logging > > > the histograms from the browser process? In general, we prefer to (a) do > more > > > in the browser, and less in the renderer; and (b) send less data from one > > > process to the other. In this case, I'm not seeing the gain from pushing > this > > > work to the renderer. > > > > Reading back through the comments on this CL, I think you might be doing this > > based on Garrett's comment that "Some of the stats (bubble creation, password > > accepted) can be pushed into the renderer". I don't think this is what he > > meant, though I'll let him clarify in case I'm misunderstanding. > > Yeah, I just meant that for those particular stats you can already tell in the > renderer that they are happening so you wouldn't need to edit each individual UI > file. Since we want to keep track of other stats as well I don't know if it > actually makes sense to log them there, I was more musing about possible ways to > cut down on code duplication. Given the setup now, I think that just having a > helper function that logs a PasswordGenerationStatus would be better. Done. http://codereview.chromium.org/10787023/diff/10012/chrome/browser/ui/password... File chrome/browser/ui/password_generation_status.h (right): http://codereview.chromium.org/10787023/diff/10012/chrome/browser/ui/password... chrome/browser/ui/password_generation_status.h:53: struct PasswordGenerationStatus { On 2012/07/18 17:20:03, Garrett Casto wrote: > Given the other suggestions, can you drop the generated_password here and rename > this to PasswordGenerationActions. Done. http://codereview.chromium.org/10787023/diff/10012/chrome/renderer/autofill/p... File chrome/renderer/autofill/password_generation_manager.cc (right): http://codereview.chromium.org/10787023/diff/10012/chrome/renderer/autofill/p... chrome/renderer/autofill/password_generation_manager.cc:25: using namespace password_generation; On 2012/07/18 05:36:15, Ilya Sherman wrote: > "using namespace" is disallowed by the Chromium style guide Done.
This is looking much better, thanks :) +cc Jim for comment on whether it's ok to log to the same histogram both from the browser and the renderer process. (My vague worry is that we can, in rare cases, lose histogram data from renderers. This should be ok if all of the values are being logged in the renderer, as it just decreases the volume of the data sent to the server. However, if some of the values are only logged in the renderer, and some are only logged in the browser, this might erroneously inflate the numbers reported for the values logged in the browser.) http://codereview.chromium.org/10787023/diff/8003/chrome/browser/ui/password_... File chrome/browser/ui/password_generation_status.cc (right): http://codereview.chromium.org/10787023/diff/8003/chrome/browser/ui/password_... chrome/browser/ui/password_generation_status.cc:25: ACCEPT_AFTER_EDITING, ACTION_ENUM_COUNT); nit: The "UMA_HISTOGRAM_ENUMERATION" block is repeated four times in this method. I would recommend having the if/else logic simply select which metric to log, and moving the rest of this block out of the branched logic. http://codereview.chromium.org/10787023/diff/8003/chrome/browser/ui/password_... File chrome/browser/ui/password_generation_status.h (right): http://codereview.chromium.org/10787023/diff/8003/chrome/browser/ui/password_... chrome/browser/ui/password_generation_status.h:35: // implementation this will result to closing the bubble so this action nit: "will result to" -> "will result in" http://codereview.chromium.org/10787023/diff/8003/chrome/browser/ui/password_... chrome/browser/ui/password_generation_status.h:48: }; nit: Can this enum be tucked into an anonymous namespace in the implementation file? http://codereview.chromium.org/10787023/diff/8003/chrome/browser/ui/password_... chrome/browser/ui/password_generation_status.h:50: // Data structure to store various password generation status . Optional nit: Perhaps something more like "Wrapper to store the state of user interactions with the password generation bubble"? (My suggestion is admittedly still a little clumsy...) http://codereview.chromium.org/10787023/diff/8003/chrome/browser/ui/password_... chrome/browser/ui/password_generation_status.h:61: // Whether the user has clicked on the regereated button. nit: "regereated" -> "regenerate" http://codereview.chromium.org/10787023/diff/8003/chrome/browser/ui/password_... chrome/browser/ui/password_generation_status.h:68: void UpdateUserActionsHistogram(PasswordGenerationActions actions); Optional nit: Perhaps "LogPasswordGenerationUserActions" (or just "LogUserActions", if you think the "PasswordGeneration" part is redundant)? http://codereview.chromium.org/10787023/diff/8003/chrome/browser/ui/password_... chrome/browser/ui/password_generation_status.h:70: void UpdateEventsHistogram(PasswordGenerationEvent event); Optional nit: Perhaps "LogPasswordGenerationEvent"? http://codereview.chromium.org/10787023/diff/8003/chrome/renderer/autofill/pa... File chrome/renderer/autofill/password_generation_manager.cc (right): http://codereview.chromium.org/10787023/diff/8003/chrome/renderer/autofill/pa... chrome/renderer/autofill/password_generation_manager.cc:8: #include "chrome/browser/ui/password_generation_status.h" chrome/renderer should not include any headers from chrome/browser. Instead, shared headers should live in chrome/common.
gtk code lgtm
http://codereview.chromium.org/10787023/diff/8003/chrome/browser/ui/password_... File chrome/browser/ui/password_generation_status.cc (right): http://codereview.chromium.org/10787023/diff/8003/chrome/browser/ui/password_... chrome/browser/ui/password_generation_status.cc:25: ACCEPT_AFTER_EDITING, ACTION_ENUM_COUNT); On 2012/07/18 20:48:56, Ilya Sherman wrote: > nit: The "UMA_HISTOGRAM_ENUMERATION" block is repeated four times in this > method. I would recommend having the if/else logic simply select which metric > to log, and moving the rest of this block out of the branched logic. Done. http://codereview.chromium.org/10787023/diff/8003/chrome/browser/ui/password_... File chrome/browser/ui/password_generation_status.h (right): http://codereview.chromium.org/10787023/diff/8003/chrome/browser/ui/password_... chrome/browser/ui/password_generation_status.h:35: // implementation this will result to closing the bubble so this action On 2012/07/18 20:48:56, Ilya Sherman wrote: > nit: "will result to" -> "will result in" Done. http://codereview.chromium.org/10787023/diff/8003/chrome/browser/ui/password_... chrome/browser/ui/password_generation_status.h:48: }; On 2012/07/18 20:48:56, Ilya Sherman wrote: > nit: Can this enum be tucked into an anonymous namespace in the implementation > file? Feel that putting it in the header file will make the programmers easy to find what are the possible values of this enum and what's their meanings (we usually expect to look up such info in the header file I believe). But I'm fine with either way. http://codereview.chromium.org/10787023/diff/8003/chrome/browser/ui/password_... chrome/browser/ui/password_generation_status.h:50: // Data structure to store various password generation status . On 2012/07/18 20:48:57, Ilya Sherman wrote: > Optional nit: Perhaps something more like "Wrapper to store the state of user > interactions with the password generation bubble"? (My suggestion is admittedly > still a little clumsy...) hmm, changed to "Wrapper to store user interactions with the password generation bubble" http://codereview.chromium.org/10787023/diff/8003/chrome/browser/ui/password_... chrome/browser/ui/password_generation_status.h:61: // Whether the user has clicked on the regereated button. On 2012/07/18 20:48:57, Ilya Sherman wrote: > nit: "regereated" -> "regenerate" Done. http://codereview.chromium.org/10787023/diff/8003/chrome/browser/ui/password_... chrome/browser/ui/password_generation_status.h:68: void UpdateUserActionsHistogram(PasswordGenerationActions actions); On 2012/07/18 20:48:57, Ilya Sherman wrote: > Optional nit: Perhaps "LogPasswordGenerationUserActions" (or just > "LogUserActions", if you think the "PasswordGeneration" part is redundant)? Done. http://codereview.chromium.org/10787023/diff/8003/chrome/browser/ui/password_... chrome/browser/ui/password_generation_status.h:70: void UpdateEventsHistogram(PasswordGenerationEvent event); On 2012/07/18 20:48:57, Ilya Sherman wrote: > Optional nit: Perhaps "LogPasswordGenerationEvent"? Done. http://codereview.chromium.org/10787023/diff/8003/chrome/renderer/autofill/pa... File chrome/renderer/autofill/password_generation_manager.cc (right): http://codereview.chromium.org/10787023/diff/8003/chrome/renderer/autofill/pa... chrome/renderer/autofill/password_generation_manager.cc:8: #include "chrome/browser/ui/password_generation_status.h" On 2012/07/18 20:48:57, Ilya Sherman wrote: > chrome/renderer should not include any headers from chrome/browser. Instead, > shared headers should live in chrome/common. Moved to chrome/common and renamed it to password_generation_util.h since we now have helper functions in it.
LGTM, but please also wait for Jim's feedback regarding logging to a single histogram from multiple processes. http://codereview.chromium.org/10787023/diff/8003/chrome/browser/ui/password_... File chrome/browser/ui/password_generation_status.h (right): http://codereview.chromium.org/10787023/diff/8003/chrome/browser/ui/password_... chrome/browser/ui/password_generation_status.h:48: }; On 2012/07/18 22:46:17, zysxqn wrote: > On 2012/07/18 20:48:56, Ilya Sherman wrote: > > nit: Can this enum be tucked into an anonymous namespace in the implementation > > file? > > Feel that putting it in the header file will make the programmers easy to find > what are the possible values of this enum and what's their meanings (we usually > expect to look up such info in the header file I believe). But I'm fine with > either way. The primary description for the histogram actually lives in an XML file in the internal repository. If discoverability is the only reason for keeping this in the header file, I'd prefer you move it to the implementation file -- I'd expect it to be found by a code search in either case. http://codereview.chromium.org/10787023/diff/14004/chrome/common/password_gen... File chrome/common/password_generation_util.h (right): http://codereview.chromium.org/10787023/diff/14004/chrome/common/password_gen... chrome/common/password_generation_util.h:70: void LogPasswordGenerationEvents(PasswordGenerationEvent event); nit: "Events" -> "Event"
http://codereview.chromium.org/10787023/diff/8003/chrome/browser/ui/password_... File chrome/browser/ui/password_generation_status.h (right): http://codereview.chromium.org/10787023/diff/8003/chrome/browser/ui/password_... chrome/browser/ui/password_generation_status.h:48: }; On 2012/07/18 23:13:23, Ilya Sherman wrote: > On 2012/07/18 22:46:17, zysxqn wrote: > > On 2012/07/18 20:48:56, Ilya Sherman wrote: > > > nit: Can this enum be tucked into an anonymous namespace in the > implementation > > > file? > > > > Feel that putting it in the header file will make the programmers easy to find > > what are the possible values of this enum and what's their meanings (we > usually > > expect to look up such info in the header file I believe). But I'm fine with > > either way. > > The primary description for the histogram actually lives in an XML file in the > internal repository. If discoverability is the only reason for keeping this in > the header file, I'd prefer you move it to the implementation file -- I'd expect > it to be found by a code search in either case. Done. http://codereview.chromium.org/10787023/diff/14004/chrome/common/password_gen... File chrome/common/password_generation_util.h (right): http://codereview.chromium.org/10787023/diff/14004/chrome/common/password_gen... chrome/common/password_generation_util.h:70: void LogPasswordGenerationEvents(PasswordGenerationEvent event); On 2012/07/18 23:13:23, Ilya Sherman wrote: > nit: "Events" -> "Event" Done.
As a general rule, if you can get the info in the browser (easily), then you should push it into a histogram there. Ilya is correct, that when you record data in a renderer (or any child process), there is a greater risk that the process will go away, and the data will be lost (tossed before getting a chance to be pushed to the browser process). In the case of renederers, this is more common, as navigation often induces the creation of a new renderer process, and the orphaning or discarding of another. As a related caveat, it is probably better to only compare browser gathered data to browser gathered data, or compare renderer data to renderer data, as it is difficult to guess what the loss rate will be :-/. The renderer histograms were put in place to make it easy to add histograms pretty much anywhere (they are now supported even in plugin and GPU processes). It will "work" to added to a single named histogram in both the renderer and browser process (they will indeed aggregate in the browser)... but you won't be able to glean much, due to the unknown bias in loss rate :-(. http://codereview.chromium.org/10787023/diff/19004/chrome/browser/ui/gtk/pass... File chrome/browser/ui/gtk/password_generation_bubble_gtk.cc (right): http://codereview.chromium.org/10787023/diff/19004/chrome/browser/ui/gtk/pass... chrome/browser/ui/gtk/password_generation_bubble_gtk.cc:113: BubbleGtk* bubble, bool closed_by_escape) { nit: all parameters should be aligned, once you decided you couldn't fit them on one line. http://codereview.chromium.org/10787023/diff/19004/chrome/browser/ui/gtk/pass... File chrome/browser/ui/gtk/password_generation_bubble_gtk.h (right): http://codereview.chromium.org/10787023/diff/19004/chrome/browser/ui/gtk/pass... chrome/browser/ui/gtk/password_generation_bubble_gtk.h:27: class PasswordGenerationBubbleGtk : public BubbleDelegateGtk{ nit: space before curly http://codereview.chromium.org/10787023/diff/19004/chrome/common/password_gen... File chrome/common/password_generation_util.cc (right): http://codereview.chromium.org/10787023/diff/19004/chrome/common/password_gen... chrome/common/password_generation_util.cc:48: UserAction action; personal nit: I'd rather see this initialized to IGNORE, than have to read the code below to see there was always a default initialization. You can ignore this request... but IMO, it helps with readability. Compiler will generate the same code either way.
Changed to use a separate histogram for the submitted event -- this one is relevantly independent with other events as we just want to see how many times user actually use password generation by submitting the generated password. And now no shared histogram for browser and renderer code. On 2012/07/19 00:05:41, jar wrote: > As a general rule, if you can get the info in the browser (easily), then you > should push it into a histogram there. Ilya is correct, that when you record > data in a renderer (or any child process), there is a greater risk that the > process will go away, and the data will be lost (tossed before getting a chance > to be pushed to the browser process). In the case of renederers, this is more > common, as navigation often induces the creation of a new renderer process, and > the orphaning or discarding of another. > > As a related caveat, it is probably better to only compare browser gathered data > to browser gathered data, or compare renderer data to renderer data, as it is > difficult to guess what the loss rate will be :-/. > > The renderer histograms were put in place to make it easy to add histograms > pretty much anywhere (they are now supported even in plugin and GPU processes). > It will "work" to added to a single named histogram in both the renderer and > browser process (they will indeed aggregate in the browser)... but you won't be > able to glean much, due to the unknown bias in loss rate :-(. > > http://codereview.chromium.org/10787023/diff/19004/chrome/browser/ui/gtk/pass... > File chrome/browser/ui/gtk/password_generation_bubble_gtk.cc (right): > > http://codereview.chromium.org/10787023/diff/19004/chrome/browser/ui/gtk/pass... > chrome/browser/ui/gtk/password_generation_bubble_gtk.cc:113: BubbleGtk* bubble, > bool closed_by_escape) { > nit: all parameters should be aligned, once you decided you couldn't fit them on > one line. > > http://codereview.chromium.org/10787023/diff/19004/chrome/browser/ui/gtk/pass... > File chrome/browser/ui/gtk/password_generation_bubble_gtk.h (right): > > http://codereview.chromium.org/10787023/diff/19004/chrome/browser/ui/gtk/pass... > chrome/browser/ui/gtk/password_generation_bubble_gtk.h:27: class > PasswordGenerationBubbleGtk : public BubbleDelegateGtk{ > nit: space before curly > > http://codereview.chromium.org/10787023/diff/19004/chrome/common/password_gen... > File chrome/common/password_generation_util.cc (right): > > http://codereview.chromium.org/10787023/diff/19004/chrome/common/password_gen... > chrome/common/password_generation_util.cc:48: UserAction action; > personal nit: I'd rather see this initialized to IGNORE, than have to read the > code below to see there was always a default initialization. > > You can ignore this request... but IMO, it helps with readability. Compiler > will generate the same code either way.
http://codereview.chromium.org/10787023/diff/19004/chrome/browser/ui/gtk/pass... File chrome/browser/ui/gtk/password_generation_bubble_gtk.cc (right): http://codereview.chromium.org/10787023/diff/19004/chrome/browser/ui/gtk/pass... chrome/browser/ui/gtk/password_generation_bubble_gtk.cc:113: BubbleGtk* bubble, bool closed_by_escape) { On 2012/07/19 00:05:41, jar wrote: > nit: all parameters should be aligned, once you decided you couldn't fit them on > one line. Done. http://codereview.chromium.org/10787023/diff/19004/chrome/browser/ui/gtk/pass... File chrome/browser/ui/gtk/password_generation_bubble_gtk.h (right): http://codereview.chromium.org/10787023/diff/19004/chrome/browser/ui/gtk/pass... chrome/browser/ui/gtk/password_generation_bubble_gtk.h:27: class PasswordGenerationBubbleGtk : public BubbleDelegateGtk{ On 2012/07/19 00:05:42, jar wrote: > nit: space before curly Done. http://codereview.chromium.org/10787023/diff/19004/chrome/common/password_gen... File chrome/common/password_generation_util.cc (right): http://codereview.chromium.org/10787023/diff/19004/chrome/common/password_gen... chrome/common/password_generation_util.cc:48: UserAction action; On 2012/07/19 00:05:42, jar wrote: > personal nit: I'd rather see this initialized to IGNORE, than have to read the > code below to see there was always a default initialization. > > You can ignore this request... but IMO, it helps with readability. Compiler > will generate the same code either way. Done.
http://codereview.chromium.org/10787023/diff/17012/chrome/browser/password_ma... File chrome/browser/password_manager/password_manager.cc (right): http://codereview.chromium.org/10787023/diff/17012/chrome/browser/password_ma... chrome/browser/password_manager/password_manager.cc:244: UMA_HISTOGRAM_COUNTS("PasswordGeneration.Submitted", 1); Keep in mind that this will have the same bias as if you were logging to a single histogram, so you'll still need to be aware of this bias when analyzing the uploaded data. If you want to be able to compare the stats gathered in the renderer to this one, you will probably want to send messages from the renderer process to the browser process as the events occur, and log to a single histogram in the browser process. If you're not too worried about the bias, you can leave the code as it is, but please add a note about this potential for bias to the histogram description (which you'll create in a separate CL, as part of adding the histogram to the master XML file).
(also, still lgtm)
Yes, I think we won't compare it directly with other renderer stats for now. If needed later I will have renderer send a message with the stats and log all in the browser. Also will add a note in the XML file. Submitting. On Thu, Jul 19, 2012 at 4:29 PM, <isherman@chromium.org> wrote: > > http://codereview.chromium.**org/10787023/diff/17012/** > chrome/browser/password_**manager/password_manager.cc<http://codereview.chromium.org/10787023/diff/17012/chrome/browser/password_manager/password_manager.cc> > File chrome/browser/password_**manager/password_manager.cc (right): > > http://codereview.chromium.**org/10787023/diff/17012/** > chrome/browser/password_**manager/password_manager.cc#**newcode244<http://codereview.chromium.org/10787023/diff/17012/chrome/browser/password_manager/password_manager.cc#newcode244> > chrome/browser/password_**manager/password_manager.cc:**244: > UMA_HISTOGRAM_COUNTS("**PasswordGeneration.Submitted", 1); > Keep in mind that this will have the same bias as if you were logging to > a single histogram, so you'll still need to be aware of this bias when > analyzing the uploaded data. > > If you want to be able to compare the stats gathered in the renderer to > this one, you will probably want to send messages from the renderer > process to the browser process as the events occur, and log to a single > histogram in the browser process. If you're not too worried about the > bias, you can leave the code as it is, but please add a note about this > potential for bias to the histogram description (which you'll create in > a separate CL, as part of adding the histogram to the master XML file). > > http://codereview.chromium.**org/10787023/<http://codereview.chromium.org/107... >
One small thing. Otherwise lgtm http://codereview.chromium.org/10787023/diff/17012/chrome/renderer/autofill/p... File chrome/renderer/autofill/password_generation_manager.cc (right): http://codereview.chromium.org/10787023/diff/17012/chrome/renderer/autofill/p... chrome/renderer/autofill/password_generation_manager.cc:127: } Can we have a negative case here? Something like NO_SIGNUP_DETECTED. Partially I want this data so that we can get an idea of how often this feature triggers in normal browsing, but I also want to get a reasonable user count of people that have this feature enabled. Most users aren't going to hit an account creation page every day, so we won't get reasonable user counts otherwise.
http://codereview.chromium.org/10787023/diff/17012/chrome/renderer/autofill/p... File chrome/renderer/autofill/password_generation_manager.cc (right): http://codereview.chromium.org/10787023/diff/17012/chrome/renderer/autofill/p... chrome/renderer/autofill/password_generation_manager.cc:127: } On 2012/07/20 18:00:40, Garrett Casto wrote: > Can we have a negative case here? Something like NO_SIGNUP_DETECTED. Partially I > want this data so that we can get an idea of how often this feature triggers in > normal browsing, but I also want to get a reasonable user count of people that > have this feature enabled. Most users aren't going to hit an account creation > page every day, so we won't get reasonable user counts otherwise. For getting a sense of how often this feature triggers in normal browsing, what's your preference of using a total count like "PASSWORD_FORM_DETECTED" v.s. the negative count as you suggested? Also, I'm not sure how to get the "user" count? I think we can only count how many "pages" are examined by our heuristic.
http://codereview.chromium.org/10787023/diff/17012/chrome/renderer/autofill/p... File chrome/renderer/autofill/password_generation_manager.cc (right): http://codereview.chromium.org/10787023/diff/17012/chrome/renderer/autofill/p... chrome/renderer/autofill/password_generation_manager.cc:127: } On 2012/07/20 18:20:32, zysxqn wrote: > On 2012/07/20 18:00:40, Garrett Casto wrote: > > Can we have a negative case here? Something like NO_SIGNUP_DETECTED. Partially > I > > want this data so that we can get an idea of how often this feature triggers > in > > normal browsing, but I also want to get a reasonable user count of people that > > have this feature enabled. Most users aren't going to hit an account creation > > page every day, so we won't get reasonable user counts otherwise. > > For getting a sense of how often this feature triggers in normal browsing, > what's your preference of using a total count like "PASSWORD_FORM_DETECTED" v.s. > the negative count as you suggested? > I think that I'd like to have all pages for sure. I can imagine that seeing how often we show the icon per password form page might be interesting, but I haven't thought about that. I would probably leave it off for now, but it's up to you. > Also, I'm not sure how to get the "user" count? I think we can only count how > many "pages" are examined by our heuristic. So it doesn't look like this is displayed on the histogram page, but UMA pings have a GUID associated with them so we should be able to figure out the user numbers from this. It sounded like Ian was trying to get someone to work on having UMA report all of the preferences that each user has selected automatically which would give us this data as well, but it wasn't clear when this was going to happen.
http://codereview.chromium.org/10787023/diff/17012/chrome/renderer/autofill/p... File chrome/renderer/autofill/password_generation_manager.cc (right): http://codereview.chromium.org/10787023/diff/17012/chrome/renderer/autofill/p... chrome/renderer/autofill/password_generation_manager.cc:127: } On 2012/07/20 18:43:58, Garrett Casto wrote: > On 2012/07/20 18:20:32, zysxqn wrote: > > On 2012/07/20 18:00:40, Garrett Casto wrote: > > > Can we have a negative case here? Something like NO_SIGNUP_DETECTED. > Partially > > I > > > want this data so that we can get an idea of how often this feature triggers > > in > > > normal browsing, but I also want to get a reasonable user count of people > that > > > have this feature enabled. Most users aren't going to hit an account > creation > > > page every day, so we won't get reasonable user counts otherwise. > > > > For getting a sense of how often this feature triggers in normal browsing, > > what's your preference of using a total count like "PASSWORD_FORM_DETECTED" > v.s. > > the negative count as you suggested? > > > > I think that I'd like to have all pages for sure. I can imagine that seeing how > often we show the icon per password form page might be interesting, but I > haven't thought about that. I would probably leave it off for now, but it's up > to you. > > > Also, I'm not sure how to get the "user" count? I think we can only count how > > many "pages" are examined by our heuristic. > > So it doesn't look like this is displayed on the histogram page, but UMA pings > have a GUID associated with them so we should be able to figure out the user > numbers from this. > > It sounded like Ian was trying to get someone to work on having UMA report all > of the preferences that each user has selected automatically which would give us > this data as well, but it wasn't clear when this was going to happen. Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zysxqn@google.com/10787023/15007
Try job failure for 10787023-15007 (retry) on win_rel for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zysxqn@google.com/10787023/7032
Try job failure for 10787023-7032 (retry) on win_rel for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
hmm.. It is really weird. Only the windows compiler fails with defining the enum in the anonymous namespace. I really couldn't see any problem there... Ilya, since you originally suggest to put the enum in .cpp under the anonymous namespace. Do you want to take a look at? On Fri, Jul 20, 2012 at 2:00 PM, <commit-bot@chromium.org> wrote: > Try job failure for 10787023-7032 (retry) on win_rel for step "compile" > (clobber > > build). > It's a second try, previously, step "compile" failed. > http://build.chromium.org/p/**tryserver.chromium/** > buildstatus?builder=win_rel&**number=45902<http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&number=45902> > > > https://chromiumcodereview.**appspot.com/10787023/<https://chromiumcodereview... >
It looks like vs2010 doesn't allow enum defined in anonymous namespace ( http://msdn.microsoft.com/en-us/library/a6cskb49(v=vs.110).aspx). So I'm going to change it back to the .h file under password_generation namespace. On Fri, Jul 20, 2012 at 2:10 PM, Yue Zhang <zysxqn@google.com> wrote: > hmm.. It is really weird. Only the windows compiler fails with defining > the enum in the anonymous namespace. I really couldn't see any problem > there... Ilya, since you originally suggest to put the enum in .cpp under > the anonymous namespace. Do you want to take a look at? > > > On Fri, Jul 20, 2012 at 2:00 PM, <commit-bot@chromium.org> wrote: > >> Try job failure for 10787023-7032 (retry) on win_rel for step "compile" >> (clobber >> >> build). >> It's a second try, previously, step "compile" failed. >> http://build.chromium.org/p/**tryserver.chromium/** >> buildstatus?builder=win_rel&**number=45902<http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&number=45902> >> >> >> https://chromiumcodereview.**appspot.com/10787023/<https://chromiumcodereview... >> > >
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zysxqn@google.com/10787023/7034
On 2012/07/20 23:48:10, zysxqn wrote: > It looks like vs2010 doesn't allow enum defined in anonymous namespace ( > http://msdn.microsoft.com/en-us/library/a6cskb49%28v=vs.110%29.aspx). So I'm > going to change it back to the .h file under password_generation namespace. enums are definitely allowed in anonymous namespaces; for example, the autofill_metrics.cc code [1] includes one. [1] https://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/autofill/auto... I suspect that |IGNORE| is a constant that's defined in one of the Windows header files. If you rename it, the code should compile.
yeah, autofill_metrics.cc has an example. So I will try renaming IGNORE. Thanks! On Fri, Jul 20, 2012 at 5:08 PM, <isherman@chromium.org> wrote: > On 2012/07/20 23:48:10, zysxqn wrote: > >> It looks like vs2010 doesn't allow enum defined in anonymous namespace ( >> http://msdn.microsoft.com/en-**us/library/a6cskb49%28v=vs.**110%29.aspx<http:...). >> So I'm >> >> going to change it back to the .h file under password_generation >> namespace. >> > > enums are definitely allowed in anonymous namespaces; for example, the > autofill_metrics.cc code [1] includes one. > [1] > https://src.chromium.org/**viewvc/chrome/trunk/src/** > chrome/browser/autofill/**autofill_metrics.cc?view=**markup<https://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/autofill/autofill_metrics.cc?view=markup> > > I suspect that |IGNORE| is a constant that's defined in one of the Windows > header files. If you rename it, the code should compile. > > https://chromiumcodereview.**appspot.com/10787023/<https://chromiumcodereview... >
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zysxqn@google.com/10787023/14017
Change committed as 147757 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
