Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
789613004:
Decision whenever "Allow to collect URL?" bubble should be shown or not (Closed)
DescriptionDecision whenever "Allow to collect URL?" bubble should be shown or not is made based on Finch experiment.
If user belongs to group for which bubble should be shown, then
we calculate a time span when bubble should appear, if it's current span, when we show bubble and saving in settings information that bubble was shown, so we'll never show bubble again. If it's not current time span when we do not show bubble this time.
BUG=435080
R=vabr@chromium.org
Committed: https://crrev.com/c489fa833c62edf608a6ecbf553036fe9d7a3292
Cr-Commit-Position: refs/heads/master@{#308170}
Patch Set 1 : This code belongs to issue 777423004. #Patch Set 2 : Changes which belong to this CL. #
Total comments: 22
Patch Set 3 : On top of master #
Total comments: 28
Patch Set 4 : #
Total comments: 3
Patch Set 5 : Do not review #Patch Set 6 : Please, review #
Total comments: 59
Patch Set 7 : Please review #
Total comments: 7
Patch Set 8 : Do not review #Patch Set 9 : Please review. #
Total comments: 14
Patch Set 10 : #Patch Set 11 : #
Total comments: 4
Patch Set 12 : Don't review #Patch Set 13 : Don't review #Patch Set 14 : #
Total comments: 9
Patch Set 15 : #Patch Set 16 : Comment changed #
Total comments: 13
Patch Set 17 : kWasAllowToCollectURLBubbleShown->kAllowToCollectURLBubbleWasShown #Patch Set 18 : #Patch Set 19 : #Messages
Total messages: 50 (9 generated)
Patchset #2 (id:20001) has been deleted
Hi Vaclav, I can't land https://codereview.chromium.org/777423004/ because I'm missing review for DEPS file. But I still want to proceed with this CL, so I organized it like this: first patch consists of changes from 777423004, but changes in second patch belong to this CL. So please review only difference between Patch1 and Patch2. Thanks, Tanja
Hi Tanja, Thanks, I reviewed patch set 2 vs. patch set 1. Btw., if you base your git branch for this patch on your git branch for the older CL, you don't have to start with putting the other CL as patch set 1. The downside is that you cannot run trybots until the predecessor lands, but that's a fair tradeoff for the clear reviewing structure. I left some initial comments. Overall the change looks good, but I want to clarify the UUID stuff. Cheers, Vaclav https://codereview.chromium.org/789613004/diff/40001/chrome/browser/password_... File chrome/browser/password_manager/chrome_password_manager_client.cc (right): https://codereview.chromium.org/789613004/diff/40001/chrome/browser/password_... chrome/browser/password_manager/chrome_password_manager_client.cc:159: std::stringstream profile_uuid_stream; The style guide forbids streams (except for logging). Please use std::string::append() and others. https://codereview.chromium.org/789613004/diff/40001/components/password_mana... File components/password_manager/core/browser/password_manager_url_collection_experiment.cc (right): https://codereview.chromium.org/789613004/diff/40001/components/password_mana... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:30: if (!retrieved) nit: You could also insert the GetVariationParams call in the if. It seems like |retrieved| is not used below. https://codereview.chromium.org/789613004/diff/40001/components/password_mana... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:41: bool ShowBubbleExperiment(PrefService* prefs, This should be called "ShouldShowBubbleIfExperimentActive", because it answers that question. It does not show "bubble experiment". https://codereview.chromium.org/789613004/diff/40001/components/password_mana... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:53: base::Time experiment_active_to_user_start = I might be hitting the issue of parsing long names again, but should this be "active for user" rather than "active to user"? https://codereview.chromium.org/789613004/diff/40001/components/password_mana... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:55: bool greater_than_now = experiment_active_to_user_start >= now; You can drop both these bools, because you seem to recompute them in the return anyway. :) And yes, I think the recomputing in the return is fine and clear, no need to cache in variables. https://codereview.chromium.org/789613004/diff/40001/components/password_mana... File components/password_manager/core/browser/password_manager_url_collection_experiment.h (right): https://codereview.chromium.org/789613004/diff/40001/components/password_mana... components/password_manager/core/browser/password_manager_url_collection_experiment.h:36: bool ShouldShowBubble(PrefService* prefs, const std::string& profile_uuid); The |profile_uuid| parameter needs a solid explanation. First, it needs to be noted that it is not stored anywhere (and please correct me, if I'm wrong). Then, it needs to be said what is it used for, and what uniqueness we assume (when is it OK if for two users these values collide). I'd like to check if the UUID is generated correctly, or if it could be generated in other ways, but I'm not sure what is actually expected. https://codereview.chromium.org/789613004/diff/40001/components/password_mana... components/password_manager/core/browser/password_manager_url_collection_experiment.h:51: // Determines how long experiment is active. grammar: the experiment https://codereview.chromium.org/789613004/diff/40001/components/password_mana... components/password_manager/core/browser/password_manager_url_collection_experiment.h:53: // Determines length of time span when experiment is active for user. The word "active" is used in two contexts (active and active for user), without much hints for the difference in meaning. Please replace "active for user" with a description of what it really means. Also, "length of time span" seems to be equivalent to the plain "how long" you use above. Please try to simplify the language in the comments. https://codereview.chromium.org/789613004/diff/40001/components/password_mana... File components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc (right): https://codereview.chromium.org/789613004/diff/40001/components/password_mana... components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc:109: SetupNeverShowBubbleExperimentGroup(); Should we also call PretendThatThisIsTimeSpanWhenBubbleAppears to make sure that the expected false on the next line does not come because of the clock only? (Also in the next test.)
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
melandory@chromium.org changed reviewers: + asvitkine@chromium.org
https://codereview.chromium.org/789613004/diff/40001/chrome/browser/password_... File chrome/browser/password_manager/chrome_password_manager_client.cc (right): https://codereview.chromium.org/789613004/diff/40001/chrome/browser/password_... chrome/browser/password_manager/chrome_password_manager_client.cc:159: std::stringstream profile_uuid_stream; On 2014/12/10 18:19:54, vabr (Chromium) wrote: > The style guide forbids streams (except for logging). Please use > std::string::append() and others. Done. https://codereview.chromium.org/789613004/diff/40001/components/password_mana... File components/password_manager/core/browser/password_manager_url_collection_experiment.cc (right): https://codereview.chromium.org/789613004/diff/40001/components/password_mana... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:30: if (!retrieved) On 2014/12/10 18:19:54, vabr (Chromium) wrote: > nit: You could also insert the GetVariationParams call in the if. It seems like > |retrieved| is not used below. Done. https://codereview.chromium.org/789613004/diff/40001/components/password_mana... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:41: bool ShowBubbleExperiment(PrefService* prefs, On 2014/12/10 18:19:54, vabr (Chromium) wrote: > This should be called "ShouldShowBubbleIfExperimentActive", because it answers > that question. It does not show "bubble experiment". Done. https://codereview.chromium.org/789613004/diff/40001/components/password_mana... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:53: base::Time experiment_active_to_user_start = On 2014/12/10 18:19:55, vabr (Chromium) wrote: > I might be hitting the issue of parsing long names again, but should this be > "active for user" rather than "active to user"? Done. You are right. https://codereview.chromium.org/789613004/diff/40001/components/password_mana... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:55: bool greater_than_now = experiment_active_to_user_start >= now; On 2014/12/10 18:19:54, vabr (Chromium) wrote: > You can drop both these bools, because you seem to recompute them in the return > anyway. :) > And yes, I think the recomputing in the return is fine and clear, no need to > cache in variables. Done. Sorry was using them for debug purposes and forgot to remove then. https://codereview.chromium.org/789613004/diff/40001/components/password_mana... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:96: if (base::FieldTrialList::TrialExists(kExperimentName)) { asvitkine@: "get the group for "AskToSubmitURLBubble" field trial and get the value of the "ShowBubble" param for that group" - giving you the correct result without you needing to use the raw FieldTrialList API directly." https://codereview.chromium.org/789613004/diff/40001/components/password_mana... File components/password_manager/core/browser/password_manager_url_collection_experiment.h (right): https://codereview.chromium.org/789613004/diff/40001/components/password_mana... components/password_manager/core/browser/password_manager_url_collection_experiment.h:51: // Determines how long experiment is active. On 2014/12/10 18:19:55, vabr (Chromium) wrote: > grammar: the experiment Done. https://codereview.chromium.org/789613004/diff/40001/components/password_mana... components/password_manager/core/browser/password_manager_url_collection_experiment.h:53: // Determines length of time span when experiment is active for user. On 2014/12/10 18:19:55, vabr (Chromium) wrote: > The word "active" is used in two contexts (active and active for user), without > much hints for the difference in meaning. Please replace "active for user" with > a description of what it really means. > Also, "length of time span" seems to be equivalent to the plain "how long" you > use above. Please try to simplify the language in the comments. Done. https://codereview.chromium.org/789613004/diff/40001/components/password_mana... File components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc (right): https://codereview.chromium.org/789613004/diff/40001/components/password_mana... components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc:41: password_manager::urls_collection_experiment::kExperimentName, asvitkine@: "Nit: It's best practice to put the test in the same namespace as the code its testing, so you don't need to use the namespace prefixes in the test."
Hi Tanja, I left a couple more of comments + the one I pinged from last time. You improved the explanations regarding the UUID used for the experiments entropy generator, but it's not all clear to me yet. Intuitively, it looks like you need a random seed for the generator, so that the choice of the active week is uniformly spread over the year of this experiment. Is that correct? For that, you need not the property of uniqueness, do you? In fact, in one of the code comments you even call that "pseudo unique" (but elsewhere just unique). We should clarify this. And if you do require uniqueness, you should make clear among which group. Unique in the world? Unique among the profiles on a particular device? Etc. In any case, the currently generated ID does not sound that much unique, while the comments claim it is, so that also needs to be clarified. Also, as you already speak to Alexei, could you please confirm, whether the Finch infrastructure does not already have something to provide this kind of uniform distribution? They do have the same problem with assigning profiles to groups, so we should make sure not to reinvent the wheel here. The other crucial property is stability. The alternative to having a stable ID would be generating it only once, and saving the computed active week in the preferences. Would that work? If it would, you would not need to worry about stability. Note that saving the date, unlike the ID, is OK from the privacy perspective (it clearly does not identify the user, as many users will need to share the same week). Cheers, Vaclav https://codereview.chromium.org/789613004/diff/40001/components/password_mana... File components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc (right): https://codereview.chromium.org/789613004/diff/40001/components/password_mana... components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc:109: SetupNeverShowBubbleExperimentGroup(); On 2014/12/10 18:19:55, vabr (Chromium) wrote: > Should we also call PretendThatThisIsTimeSpanWhenBubbleAppears to make sure that > the expected false on the next line does not come because of the clock only? > (Also in the next test.) I'd like to ping this comment. :) https://codereview.chromium.org/789613004/diff/120001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment.h (right): https://codereview.chromium.org/789613004/diff/120001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.h:53: // Bubble is shown only once within |kParamTimePeriodInDays| from starting day nit: To clarify: "only once within" -> "only once and only within" https://codereview.chromium.org/789613004/diff/120001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.h:53: // Bubble is shown only once within |kParamTimePeriodInDays| from starting day nit: starting day -> the starting day Also, please add: ", as determined by DetermineStartOfActivityPeriod." to clarify what starting date you mean. https://codereview.chromium.org/789613004/diff/120001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.h:56: // Indicate whenever the experiment should be shown or not. nit: whenever -> whether https://codereview.chromium.org/789613004/diff/120001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.h:56: // Indicate whenever the experiment should be shown or not. Please reconsider this sentence. I fail to understand it on two places: (1) How does the content of this string constant indicate that something is shown (is it rather a name of some setting that indicates that?) (2) An experiment cannot be shown. A bubble can, perhaps. https://codereview.chromium.org/789613004/diff/120001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc (right): https://codereview.chromium.org/789613004/diff/120001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc:113: }; // namespace password_manager Please swap this line and the following one. Also please do not include ; at the end of the namespace.
On 2014/12/11 09:37:55, vabr (Chromium) wrote: > Hi Tanja, > > I left a couple more of comments + the one I pinged from last time. > > You improved the explanations regarding the UUID used for the experiments > entropy generator, but it's not all clear to me yet. > > Intuitively, it looks like you need a random seed for the generator, so that the > choice of the active week is uniformly spread over the year of this experiment. > Is that correct? Yes. > For that, you need not the property of uniqueness, do you? Exactly. That is why I called it pseudo unique as you've noticed. Basically, important property is stability. >And if you do require uniqueness, you should > make clear among which group. Unique in the world? Unique among the profiles on > a particular device? Nice property is uniqueness for profile on device, so there is no chance for user to get bubble two time on a same device but under different profiles. > > Also, as you already speak to Alexei, could you please confirm, whether the > Finch infrastructure does not already have something to provide this kind of > uniform distribution? They do have the same problem with assigning profiles to > groups, so we should make sure not to reinvent the wheel here. Will send him an email. > The other crucial property is stability. The alternative to having a stable ID > would be generating it only once, and saving the computed active week in the > preferences. Would that work? Yes, it will work and it was considered by me and vasilii@, but at the end we decided to go with this approach. If I remember correctly reasoning was: not to spoil preferences more because already a lot of thing are stored there. > Cheers, > Vaclav > > https://codereview.chromium.org/789613004/diff/40001/components/password_mana... > File > components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc > (right): > > https://codereview.chromium.org/789613004/diff/40001/components/password_mana... > components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc:109: > SetupNeverShowBubbleExperimentGroup(); > On 2014/12/10 18:19:55, vabr (Chromium) wrote: > > Should we also call PretendThatThisIsTimeSpanWhenBubbleAppears to make sure > that > > the expected false on the next line does not come because of the clock only? > > (Also in the next test.) > > I'd like to ping this comment. :) > > https://codereview.chromium.org/789613004/diff/120001/components/password_man... > File > components/password_manager/core/browser/password_manager_url_collection_experiment.h > (right): > > https://codereview.chromium.org/789613004/diff/120001/components/password_man... > components/password_manager/core/browser/password_manager_url_collection_experiment.h:53: > // Bubble is shown only once within |kParamTimePeriodInDays| from starting day > nit: To clarify: "only once within" -> "only once and only within" > > https://codereview.chromium.org/789613004/diff/120001/components/password_man... > components/password_manager/core/browser/password_manager_url_collection_experiment.h:53: > // Bubble is shown only once within |kParamTimePeriodInDays| from starting day > nit: starting day -> the starting day > Also, please add: > ", as determined by DetermineStartOfActivityPeriod." > to clarify what starting date you mean. > > https://codereview.chromium.org/789613004/diff/120001/components/password_man... > components/password_manager/core/browser/password_manager_url_collection_experiment.h:56: > // Indicate whenever the experiment should be shown or not. > nit: whenever -> whether > > https://codereview.chromium.org/789613004/diff/120001/components/password_man... > components/password_manager/core/browser/password_manager_url_collection_experiment.h:56: > // Indicate whenever the experiment should be shown or not. > Please reconsider this sentence. I fail to understand it on two places: > (1) How does the content of this string constant indicate that something is > shown (is it rather a name of some setting that indicates that?) > (2) An experiment cannot be shown. A bubble can, perhaps. > > https://codereview.chromium.org/789613004/diff/120001/components/password_man... > File > components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc > (right): > > https://codereview.chromium.org/789613004/diff/120001/components/password_man... > components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc:113: > }; // namespace password_manager > Please swap this line and the following one. Also please do not include ; at the > end of the namespace.
On 2014/12/11 10:59:58, melandory wrote: > On 2014/12/11 09:37:55, vabr (Chromium) wrote: > > Hi Tanja, > > > > I left a couple more of comments + the one I pinged from last time. > > > > You improved the explanations regarding the UUID used for the experiments > > entropy generator, but it's not all clear to me yet. > > > > Intuitively, it looks like you need a random seed for the generator, so that > the > > choice of the active week is uniformly spread over the year of this > experiment. > > Is that correct? > Yes. > > For that, you need not the property of uniqueness, do you? > Exactly. That is why I called it pseudo unique as you've noticed. Basically, > important property is stability. > >And if you do require uniqueness, you should > > make clear among which group. Unique in the world? Unique among the profiles > on > > a particular device? > Nice property is uniqueness for profile on device, so there is no chance for > user to get > bubble two time on a same device but under different profiles. Ah, that clarifies a lot. So instead of saying "(pseudo) unique", the comments should explain exactly the scope (local device) and purpose (not to show the bubble more times to a single human behind multiple profiles) of the uniqueness. > > > > Also, as you already speak to Alexei, could you please confirm, whether the > > Finch infrastructure does not already have something to provide this kind of > > uniform distribution? They do have the same problem with assigning profiles to > > groups, so we should make sure not to reinvent the wheel here. > Will send him an email. > > The other crucial property is stability. The alternative to having a stable ID > > would be generating it only once, and saving the computed active week in the > > preferences. Would that work? > Yes, it will work and it was considered by me and vasilii@, but at the end we > decided to go > with this approach. If I remember correctly reasoning was: not to spoil > preferences more because already > a lot of thing are stored there. I can see the argument with polluting preferences. On the other hand, here we pollute the code -- generating the ID looks more complicated than the actual core of this CL (which is computing the dates). Personally, I would like much more to use device-specific preferences (those saved in user data dir's Local\ State file) to store that we showed the bubble in order to achieve showing it only once on a device. We can still duplicate that information in the profile preferences as well, if we want to make sure syncing users are only asked on one of their devices. We can also put TODOs in the code to clear these up once the experiment is over. I'm happy to discuss this with you and vasilii@. Cheers, Vaclav
On 2014/12/11 11:59:57, vabr (Chromium) wrote: > On 2014/12/11 10:59:58, melandory wrote: > > On 2014/12/11 09:37:55, vabr (Chromium) wrote: > > > Hi Tanja, > > > > > > I left a couple more of comments + the one I pinged from last time. > > > > > > You improved the explanations regarding the UUID used for the experiments > > > entropy generator, but it's not all clear to me yet. > > > > > > Intuitively, it looks like you need a random seed for the generator, so that > > the > > > choice of the active week is uniformly spread over the year of this > > experiment. > > > Is that correct? > > Yes. > > > For that, you need not the property of uniqueness, do you? > > Exactly. That is why I called it pseudo unique as you've noticed. Basically, > > important property is stability. > > >And if you do require uniqueness, you should > > > make clear among which group. Unique in the world? Unique among the profiles > > on > > > a particular device? > > Nice property is uniqueness for profile on device, so there is no chance for > > user to get > > bubble two time on a same device but under different profiles. > > Ah, that clarifies a lot. So instead of saying "(pseudo) unique", the comments > should explain exactly the scope (local device) and purpose (not to show the > bubble > more times to a single human behind multiple profiles) of the uniqueness. > > > > > > > Also, as you already speak to Alexei, could you please confirm, whether the > > > Finch infrastructure does not already have something to provide this kind of > > > uniform distribution? They do have the same problem with assigning profiles > to > > > groups, so we should make sure not to reinvent the wheel here. > > Will send him an email. > > > The other crucial property is stability. The alternative to having a stable > ID > > > would be generating it only once, and saving the computed active week in the > > > preferences. Would that work? > > Yes, it will work and it was considered by me and vasilii@, but at the end we > > decided to go > > with this approach. If I remember correctly reasoning was: not to spoil > > preferences more because already > > a lot of thing are stored there. > > I can see the argument with polluting preferences. On the other hand, here we > pollute the code -- generating the ID looks more complicated than the actual > core of this CL (which is computing the dates). Personally, I would like much > more to use device-specific preferences (those saved in user data dir's > Local\ State file) to store that we showed the bubble in order to achieve > showing it only once on a device. We can still duplicate that information > in the profile preferences as well, if we want to make sure syncing users > are only asked on one of their devices. We can also put TODOs in the > code to clear these up once the experiment is over. > > I'm happy to discuss this with you and vasilii@. > > Cheers, > Vaclav Hi Vasilii, can you participate in our discussion?
Patchset #4 (id:140001) has been deleted
https://codereview.chromium.org/789613004/diff/40001/components/password_mana... File components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc (right): https://codereview.chromium.org/789613004/diff/40001/components/password_mana... components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc:109: SetupNeverShowBubbleExperimentGroup(); On 2014/12/11 09:37:55, vabr (Chromium) wrote: > On 2014/12/10 18:19:55, vabr (Chromium) wrote: > > Should we also call PretendThatThisIsTimeSpanWhenBubbleAppears to make sure > that > > the expected false on the next line does not come because of the clock only? > > (Also in the next test.) > > I'd like to ping this comment. :) We should call only SetupNeverShowBubbleExperiment, because decision will be made on group of a user, not clocks. In the next test only setting should be set, not time period. https://codereview.chromium.org/789613004/diff/120001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment.h (right): https://codereview.chromium.org/789613004/diff/120001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.h:53: // Bubble is shown only once within |kParamTimePeriodInDays| from starting day On 2014/12/11 09:37:55, vabr (Chromium) wrote: > nit: To clarify: "only once within" -> "only once and only within" Done. https://codereview.chromium.org/789613004/diff/120001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.h:53: // Bubble is shown only once within |kParamTimePeriodInDays| from starting day On 2014/12/11 09:37:55, vabr (Chromium) wrote: > nit: starting day -> the starting day > Also, please add: > ", as determined by DetermineStartOfActivityPeriod." > to clarify what starting date you mean. Done. https://codereview.chromium.org/789613004/diff/120001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.h:56: // Indicate whenever the experiment should be shown or not. On 2014/12/11 09:37:55, vabr (Chromium) wrote: > Please reconsider this sentence. I fail to understand it on two places: > (1) How does the content of this string constant indicate that something is > shown (is it rather a name of some setting that indicates that?) > (2) An experiment cannot be shown. A bubble can, perhaps. Done. https://codereview.chromium.org/789613004/diff/120001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.h:56: // Indicate whenever the experiment should be shown or not. On 2014/12/11 09:37:55, vabr (Chromium) wrote: > nit: whenever -> whether Done. https://codereview.chromium.org/789613004/diff/120001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc (right): https://codereview.chromium.org/789613004/diff/120001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc:113: }; // namespace password_manager On 2014/12/11 09:37:55, vabr (Chromium) wrote: > Please swap this line and the following one. Also please do not include ; at the > end of the namespace. Done.
vasilii@chromium.org changed reviewers: + vasilii@chromium.org
I'm for simplicity. So storing a float from [0, 1) in the preferences sounds good to me. Note that we can't store the beginning of the week because that depends on the experiment runtime parameters. https://codereview.chromium.org/789613004/diff/120001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment.cc (right): https://codereview.chromium.org/789613004/diff/120001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:26: base::TimeDelta* time_delta) { Here and below |time_delta| isn't descriptive enough. It should be something like activity_period or activity_length. https://codereview.chromium.org/789613004/diff/120001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:39: bool ShouldShowBubbleExperiment(PrefService* prefs, The experiment can't be shown. Probably the name should be ShouldShowSmthBubble. https://codereview.chromium.org/789613004/diff/120001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:44: int experiment_length_in_days; = 0; https://codereview.chromium.org/789613004/diff/120001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:54: experiment_active_for_user_start < now + time_delta; That's strange. In the current implementation |experiment_active_for_user_start| is exactly the end of the activity period. https://codereview.chromium.org/789613004/diff/120001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:61: password_manager::prefs::kWasAllowToCollectURLBubbleShown, The name sounds broken. It should be like kSomeBubbleWasShown. https://codereview.chromium.org/789613004/diff/120001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment.h (right): https://codereview.chromium.org/789613004/diff/120001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.h:38: // user in different runs of Chrome. |profile_uuid| doesn't store anywhere. |profile_uuid| isn't stored anywhere. https://codereview.chromium.org/789613004/diff/120001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.h:51: // Determines how long the experiment is active. The comments here and below are wrong. They are names of the parameters of the Finch experiment which determine something. https://codereview.chromium.org/789613004/diff/120001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc (right): https://codereview.chromium.org/789613004/diff/120001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc:55: void PretendThatThisIsTimeSpanWhenBubbleAppears( Here and below: the names are too long. Get rid of the repetitive "ThatThisIsTimeSpanWhen" part. https://codereview.chromium.org/789613004/diff/120001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc:109: PretendThatBubbleWasAlreadyShown(); The test isn't strong enough because it will pass even without pretending. You should simulate the activity period so that kWasAllowToCollectURLBubbleShown is the only reason for suppressing the bubble.
https://codereview.chromium.org/789613004/diff/160001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment.h (right): https://codereview.chromium.org/789613004/diff/160001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.h:46: void RecordBubbleClosed(PrefService* prefs); While I understand where this function came from, I don't think it's a good approach here. Recording that the bubble was shown when it disappears seems flaky. Instead it should be recorded immediately once the bubble was shown. It's not just speculation. If the user closes the tab with the bubble open then the state isn't recorded in the current implementation. It's to be fixed in the separate CL.
Hi Tanja, Some more comments. Following up on Vasilii's sentences: > I'm for simplicity. So storing a float from [0, 1) in the preferences sounds > good to me. Note that we can't store the beginning of the week because that > depends on the experiment runtime parameters. +1 to storing the number. That solves the stability constraint. Now about the uniqueness: I'm actually still a bit puzzled. Currently, your code generates different IDs for different profiles. That means that each profile on the device will potentially show a bubble, but very probably in different times. Is that the intention? What I understood previously is that at max one profile on a device should show the bubble. Cheers, Vaclav https://codereview.chromium.org/789613004/diff/40001/components/password_mana... File components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc (right): https://codereview.chromium.org/789613004/diff/40001/components/password_mana... components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc:109: SetupNeverShowBubbleExperimentGroup(); On 2014/12/11 13:16:10, melandory wrote: > On 2014/12/11 09:37:55, vabr (Chromium) wrote: > > On 2014/12/10 18:19:55, vabr (Chromium) wrote: > > > Should we also call PretendThatThisIsTimeSpanWhenBubbleAppears to make sure > > that > > > the expected false on the next line does not come because of the clock only? > > > (Also in the next test.) > > > > I'd like to ping this comment. :) > > We should call only SetupNeverShowBubbleExperiment, because decision will be > made on group of a user, not clocks. > In the next test only setting should be set, not time period. Well that's how it works now. But the purpose of the test is to check that if somebody changes (inadvertently) the code, the "never show" group will still never show the bubble (or that the bubble won't be shown more than twice, respectively). And while it is impossible to emulate all the potential states, we should try to create the one, which is most likely to be affected by a possible mistake. In this case, calling PretendThatThisIsTimeSpanWhenBubbleAppears removes the possibility that the bubble is not shown because of the time setting. If it does not influence the tested code -- great! If yes -- that's a bug and we're more likely to discover it. https://codereview.chromium.org/789613004/diff/160001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment.h (right): https://codereview.chromium.org/789613004/diff/160001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.h:56: // Status value defines should bubble appear or not. grammar: The status value defines whether the bubble should appear or not.
> Now about the uniqueness: I'm actually still a bit puzzled. > Currently, your code generates different IDs for different profiles. > That means that each profile on the device will potentially show a bubble, but > very probably in different times. > Is that the intention? What I understood previously is that at max one profile > on a device should show the bubble. Nope, it's ok that every profile on device will get a bubble. But it's better if they won't get it on a same week.
On 2014/12/11 14:14:25, melandory wrote: > > Now about the uniqueness: I'm actually still a bit puzzled. > > Currently, your code generates different IDs for different profiles. > > That means that each profile on the device will potentially show a bubble, but > > very probably in different times. > > Is that the intention? What I understood previously is that at max one profile > > on a device should show the bubble. > Nope, it's ok that every profile on device will get a bubble. But it's better if > they won't get it on a same week. Good, now I understand. So the last question is to find a good way to have a uniform distribution of the active week on the device. I can see how using the profile name is a good start for that. The "installation date" part of the random seed, that's for having a uniform distribution among all users in the world?
On 2014/12/11 14:40:10, vabr (Chromium) wrote: > On 2014/12/11 14:14:25, melandory wrote: > > > Now about the uniqueness: I'm actually still a bit puzzled. > > > Currently, your code generates different IDs for different profiles. > > > That means that each profile on the device will potentially show a bubble, > but > > > very probably in different times. > > > Is that the intention? What I understood previously is that at max one > profile > > > on a device should show the bubble. > > Nope, it's ok that every profile on device will get a bubble. But it's better > if > > they won't get it on a same week. > > Good, now I understand. > > So the last question is to find a good way to have a uniform distribution of the > active week on the device. I can see how using the profile name is a good start > for that. The "installation date" part of the random seed, that's for having a > uniform distribution among all users in the world? It's just something Dominic suggested to use as part id for user. I'm not sure how it affects distribution, but it's insterestion to think an=bout it.
Patchset #5 (id:180001) has been deleted
https://codereview.chromium.org/789613004/diff/120001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc (right): https://codereview.chromium.org/789613004/diff/120001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc:55: void PretendThatThisIsTimeSpanWhenBubbleAppears( On 2014/12/11 13:20:11, vasilii wrote: > Here and below: the names are too long. Get rid of the repetitive > "ThatThisIsTimeSpanWhen" part. So actually, the name should still speak about setting time. What about: SetTimeToBubbleActivePeriod SetTimePastBubbleActivePeriod
https://codereview.chromium.org/789613004/diff/40001/components/password_mana... File components/password_manager/core/browser/password_manager_url_collection_experiment.cc (right): https://codereview.chromium.org/789613004/diff/40001/components/password_mana... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:96: if (base::FieldTrialList::TrialExists(kExperimentName)) { On 2014/12/11 08:37:38, melandory wrote: > asvitkine@: "get the group for "AskToSubmitURLBubble" field trial and get the > value of the "ShowBubble" param for that group" - giving you the correct result > without you needing to use the raw FieldTrialList API directly." Done. https://codereview.chromium.org/789613004/diff/120001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment.cc (right): https://codereview.chromium.org/789613004/diff/120001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:26: base::TimeDelta* time_delta) { On 2014/12/11 13:20:11, vasilii wrote: > Here and below |time_delta| isn't descriptive enough. It should be something > like activity_period or activity_length. Done. https://codereview.chromium.org/789613004/diff/120001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:39: bool ShouldShowBubbleExperiment(PrefService* prefs, On 2014/12/11 13:20:11, vasilii wrote: > The experiment can't be shown. Probably the name should be ShouldShowSmthBubble. Done. https://codereview.chromium.org/789613004/diff/120001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:44: int experiment_length_in_days; On 2014/12/11 13:20:11, vasilii wrote: > = 0; Done. https://codereview.chromium.org/789613004/diff/120001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:54: experiment_active_for_user_start < now + time_delta; On 2014/12/11 13:20:11, vasilii wrote: > That's strange. In the current implementation |experiment_active_for_user_start| > is exactly the end of the activity period. Done. https://codereview.chromium.org/789613004/diff/120001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment.h (right): https://codereview.chromium.org/789613004/diff/120001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.h:38: // user in different runs of Chrome. |profile_uuid| doesn't store anywhere. On 2014/12/11 13:20:11, vasilii wrote: > |profile_uuid| isn't stored anywhere. Acknowledged. https://codereview.chromium.org/789613004/diff/120001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.h:51: // Determines how long the experiment is active. On 2014/12/11 13:20:11, vasilii wrote: > The comments here and below are wrong. They are names of the parameters of the > Finch experiment which determine something. Done. https://codereview.chromium.org/789613004/diff/120001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc (right): https://codereview.chromium.org/789613004/diff/120001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc:55: void PretendThatThisIsTimeSpanWhenBubbleAppears( On 2014/12/11 13:20:11, vasilii wrote: > Here and below: the names are too long. Get rid of the repetitive > "ThatThisIsTimeSpanWhen" part. Done. https://codereview.chromium.org/789613004/diff/120001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc:109: PretendThatBubbleWasAlreadyShown(); On 2014/12/11 13:20:11, vasilii wrote: > The test isn't strong enough because it will pass even without pretending. You > should simulate the activity period so that kWasAllowToCollectURLBubbleShown is > the only reason for suppressing the bubble. Done. https://codereview.chromium.org/789613004/diff/160001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment.h (right): https://codereview.chromium.org/789613004/diff/160001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.h:46: void RecordBubbleClosed(PrefService* prefs); On 2014/12/11 13:45:18, vasilii wrote: > While I understand where this function came from, I don't think it's a good > approach here. Recording that the bubble was shown when it disappears seems > flaky. Instead it should be recorded immediately once the bubble was shown. > It's not just speculation. If the user closes the tab with the bubble open then > the state isn't recorded in the current implementation. > It's to be fixed in the separate CL. Acknowledged.
https://codereview.chromium.org/789613004/diff/220001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment.cc (right): https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:7: #include "base/hash.h" Clean up unused headers like this one. https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:26: int defaulValueForStartingDayId = -1; const? Here and below it's not an Id. It's rather a factor. https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:45: return false; I don't see where we check if the Password Manager is enabled. https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:52: base::Time::FromString(kExperimentStartDate, &beginning); unused https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:56: return now >= experiment_active_for_user_start && This is a complex return statement so in should be in (). Then you can align the second line with the first one (like it's now). However, the 3rd line should be 4 spaces indented relative to the second one. https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:90: return beginning + base::TimeDelta::FromDays(experiment_length_in_days * Every activity period starts at 12PM. That's not right. It's covered in the doc. https://codereview.chromium.org/789613004/diff/220001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment.h (right): https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.h:8: #include <string> Is it really used? https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.h:12: class Time; For this class we need "#include" here. https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.h:37: bool ShouldShowBubble(PrefService* prefs, base::Clock* clock); If it's just for testing then you can declare it in the test file. https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.h:47: extern const char kParamLengthInDays[]; kParamExperimentLengthInDays https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.h:51: extern const char kParamTimePeriodInDays[]; kParamActivePeriodInDays https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.h:52: // Name of experiment parameter which value defines should bubble appear or not. if bubble should... https://codereview.chromium.org/789613004/diff/220001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc (right): https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc:7: #include "base/files/scoped_temp_dir.h" Clean up the headers. https://codereview.chromium.org/789613004/diff/220001/components/password_man... File components/password_manager/core/common/password_manager_pref_names.cc (right): https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/common/password_manager_pref_names.cc:51: // period when "Allow to collect URL?" bubble can be shown. All the comments should be in the header instead. https://codereview.chromium.org/789613004/diff/220001/components/password_man... File components/password_manager/core/common/password_manager_pref_names.h (right): https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/common/password_manager_pref_names.h:28: extern const char kWasAllowToCollectURLBubbleShown[]; While you're here fix this name too.
Thanks, Tanja. This already LGTM, with some comments below. Cheers, Vaclav https://codereview.chromium.org/789613004/diff/220001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment.cc (right): https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:7: #include "base/hash.h" looks unused https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:16: #include "components/variations/entropy_provider.h" looks unused https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:25: // Bubble won't be shown because starting day will be in a past. Grammar: The bubble won't be shown, because the start day is in the past. https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:25: // Bubble won't be shown because starting day will be in a past. It is not clear, why the bubble should not be shown. Please improve that, and explain the relation to preferences. Also please separate by a blank line from above. https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:26: int defaulValueForStartingDayId = -1; style: Use either kConstName for constants or non_const_name for non-const variables. https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:26: int defaulValueForStartingDayId = -1; typo: defaul -> default https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:51: base::Time beginning; beginning is unused, please delete it. https://codereview.chromium.org/789613004/diff/220001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment.h (right): https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.h:28: // Implements algorithm which determines starting time of a period when we grammar + simplification: Implements an algorithm determining when the period starts, in which "Allow to collect URL?" bubble can be shown. https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.h:45: // Name of experiment parameter which value determines how long the experiment Grammar: The name of the experiment parameter, value of which determines... https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.h:48: // Bubble is shown only once and only within value of experiment parameter grammar + an easier to read structure: The bubble is shown only once and only within a certain period. The length of the period is the value of the experiment parameter |kParamTimePeriodInDays|, and DetermineStartOfActivityPeriod computes the start of the period. https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.h:52: // Name of experiment parameter which value defines should bubble appear or not. Grammar: The name of the experiment parameter, value of which defines whether the bubble should appear or not. https://codereview.chromium.org/789613004/diff/220001/components/password_man... File components/password_manager/core/common/password_manager_pref_names.cc (right): https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/common/password_manager_pref_names.cc:50: // Value of this parameter is used for calculation of starting day of Grammar: The value of this parameter is used to calculate the start day of the period, in which the "Allow to collect URL?" bubble can be shown.
https://codereview.chromium.org/789613004/diff/220001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment.cc (right): https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:7: #include "base/hash.h" On 2014/12/11 18:46:46, vasilii wrote: > Clean up unused headers like this one. Done. https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:7: #include "base/hash.h" On 2014/12/11 18:55:10, vabr (Chromium) wrote: > looks unused Done. https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:16: #include "components/variations/entropy_provider.h" On 2014/12/11 18:55:10, vabr (Chromium) wrote: > looks unused Done. https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:25: // Bubble won't be shown because starting day will be in a past. On 2014/12/11 18:55:10, vabr (Chromium) wrote: > Grammar: The bubble won't be shown, because the start day is in the past. Done. https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:25: // Bubble won't be shown because starting day will be in a past. On 2014/12/11 18:55:10, vabr (Chromium) wrote: > Grammar: The bubble won't be shown, because the start day is in the past. Done. https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:26: int defaulValueForStartingDayId = -1; On 2014/12/11 18:55:10, vabr (Chromium) wrote: > typo: defaul -> default Done. https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:26: int defaulValueForStartingDayId = -1; On 2014/12/11 18:55:10, vabr (Chromium) wrote: > style: Use either > kConstName for constants > or > non_const_name for non-const variables. Done. https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:26: int defaulValueForStartingDayId = -1; On 2014/12/11 18:46:45, vasilii wrote: > const? > Here and below it's not an Id. It's rather a factor. Done. https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:45: return false; On 2014/12/11 18:46:46, vasilii wrote: > I don't see where we check if the Password Manager is enabled. We can't reach this code and have password manager disabled: Here https://code.google.com/p/chromium/codesearch#chromium/src/components/passwor... it's checked that PM is enabled if not SAVING_DISABLED error logged. Error of our interest is logged after: https://code.google.com/p/chromium/codesearch#chromium/src/components/passwor... But I see point in your comment, so I think it make sense to add additional check before SHouldAskUserToSubmitURL is called, so we do not depend on order in which errors are logged. https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:51: base::Time beginning; On 2014/12/11 18:55:10, vabr (Chromium) wrote: > beginning is unused, please delete it. Done. https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:52: base::Time::FromString(kExperimentStartDate, &beginning); On 2014/12/11 18:46:46, vasilii wrote: > unused Done. https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:56: return now >= experiment_active_for_user_start && On 2014/12/11 18:46:45, vasilii wrote: > This is a complex return statement so in should be in (). Then you can align the > second line with the first one (like it's now). However, the 3rd line should be > 4 spaces indented relative to the second one. git cl format has other opinion =). Done. https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:90: return beginning + base::TimeDelta::FromDays(experiment_length_in_days * On 2014/12/11 18:46:45, vasilii wrote: > Every activity period starts at 12PM. That's not right. It's covered in the doc. Done. https://codereview.chromium.org/789613004/diff/220001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment.h (right): https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.h:8: #include <string> On 2014/12/11 18:46:46, vasilii wrote: > Is it really used? Forgot to remove. Was used for profileuuid. https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.h:12: class Time; On 2014/12/11 18:46:46, vasilii wrote: > For this class we need "#include" here. Why? https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.h:28: // Implements algorithm which determines starting time of a period when we On 2014/12/11 18:55:10, vabr (Chromium) wrote: > grammar + simplification: > Implements an algorithm determining when the period starts, in which "Allow to > collect URL?" bubble can be shown. Done. https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.h:37: bool ShouldShowBubble(PrefService* prefs, base::Clock* clock); On 2014/12/11 18:46:46, vasilii wrote: > If it's just for testing then you can declare it in the test file. Done. https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.h:45: // Name of experiment parameter which value determines how long the experiment On 2014/12/11 18:55:11, vabr (Chromium) wrote: > Grammar: The name of the experiment parameter, value of which determines... Done. https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.h:47: extern const char kParamLengthInDays[]; On 2014/12/11 18:46:46, vasilii wrote: > kParamExperimentLengthInDays Done. https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.h:48: // Bubble is shown only once and only within value of experiment parameter On 2014/12/11 18:55:10, vabr (Chromium) wrote: > grammar + an easier to read structure: > The bubble is shown only once and only within a certain period. The length of > the period is the value of the experiment parameter |kParamTimePeriodInDays|, > and DetermineStartOfActivityPeriod computes the start of the period. Done. https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.h:51: extern const char kParamTimePeriodInDays[]; On 2014/12/11 18:46:46, vasilii wrote: > kParamActivePeriodInDays Done. https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.h:52: // Name of experiment parameter which value defines should bubble appear or not. On 2014/12/11 18:46:46, vasilii wrote: > if bubble should... Done. https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.h:52: // Name of experiment parameter which value defines should bubble appear or not. On 2014/12/11 18:55:10, vabr (Chromium) wrote: > Grammar: The name of the experiment parameter, value of which defines whether > the bubble should appear or not. Done. https://codereview.chromium.org/789613004/diff/220001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc (right): https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc:7: #include "base/files/scoped_temp_dir.h" On 2014/12/11 18:46:46, vasilii wrote: > Clean up the headers. Do you know by chance, is there an util which can do it? I've heard that there is one in google3. https://codereview.chromium.org/789613004/diff/220001/components/password_man... File components/password_manager/core/common/password_manager_pref_names.cc (right): https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/common/password_manager_pref_names.cc:50: // Value of this parameter is used for calculation of starting day of On 2014/12/11 18:55:11, vabr (Chromium) wrote: > Grammar: The value of this parameter is used to calculate the start day of the > period, in which the "Allow to collect URL?" bubble can be shown. Done. https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/common/password_manager_pref_names.cc:51: // period when "Allow to collect URL?" bubble can be shown. On 2014/12/11 18:46:46, vasilii wrote: > All the comments should be in the header instead. It states in .h that documentation should be in .cc: "Keep alphabetized, and document each in the .cc file." https://codereview.chromium.org/789613004/diff/220001/components/password_man... File components/password_manager/core/common/password_manager_pref_names.h (right): https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/common/password_manager_pref_names.h:28: extern const char kWasAllowToCollectURLBubbleShown[]; On 2014/12/11 18:46:46, vasilii wrote: > While you're here fix this name too. Sorry, don't follow you. How to fix it?
Still LGTM, Thanks also for the off-line explanations! Vaclav https://codereview.chromium.org/789613004/diff/220001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment.cc (right): https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:56: return now >= experiment_active_for_user_start && On 2014/12/11 23:09:58, melandory wrote: > On 2014/12/11 18:46:45, vasilii wrote: > > This is a complex return statement so in should be in (). Then you can align > the > > second line with the first one (like it's now). However, the 3rd line should > be > > 4 spaces indented relative to the second one. > > git cl format has other opinion =). Done. FWIW, I agree with git cl format and Tanja's original formatting. The +4 spaces originally started from the location of experiment_active_for_user_start, which makes sense -- experiment_active_for_user_period is added to that, not to the comparison between now and experiment_active_for_user_start. Also, in general: unless git cl format goes explicitly against the style, or creates something really hard to read (not the case here), I strongly support using git cl format. If somebody in the future will do a change of variable name or similar refactoring, and runs git cl format on the whole patch to get formatting right, sections not conforming to git cl format will increase the size of that future diff. https://codereview.chromium.org/789613004/diff/220001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc (right): https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc:7: #include "base/files/scoped_temp_dir.h" On 2014/12/11 23:09:59, melandory wrote: > On 2014/12/11 18:46:46, vasilii wrote: > > Clean up the headers. > > Do you know by chance, is there an util which can do it? I've heard that there > is one in google3. I think there was a failed attempt to use IWYU. https://code.google.com/p/chromium/wiki/IncludeWhatYouUse https://codereview.chromium.org/789613004/diff/240001/components/password_man... File components/password_manager/core/browser/password_manager.cc (right): https://codereview.chromium.org/789613004/diff/240001/components/password_man... components/password_manager/core/browser/password_manager.cc:329: if (failure == NO_MATCHING_FORM && IsSavingEnabledForCurrentPage() && Good catch! Maybe you could even move the IsSavingEnabledForCurrentPage to inside the ShouldAskUserToSubmitURL method. That would make more sense, because that method should be the ultimate source of truth for whether to show the bubble or not. In other words, if somebody calls ShouldAskUserToSubmitURL somewhere else, they should not need to worry about also copying the if-condition around it. https://codereview.chromium.org/789613004/diff/240001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment.cc (right): https://codereview.chromium.org/789613004/diff/240001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:24: // The bubble won't be shown, because active week for user gets multiplied There is a misunderstanding here: It's clear why setting defaultValueForStartingDayFactor to -1 causes the bubble never to be shown. What the comment should explain is: why the default value should cause the bubble not to be shown. Maybe it is as simple as: // A safe default value. Using this will not show the bubble. My question is whether the "safe default" is the reason, or if there is another one. https://codereview.chromium.org/789613004/diff/240001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:103: bool ShouldShowBubble(PrefService* prefs) { optional nit: Maybe add a comment saying this is for testing only and declared in the unittest file. I'll talk to Vasilii to learn more about this technique. :) https://codereview.chromium.org/789613004/diff/240001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment.h (right): https://codereview.chromium.org/789613004/diff/240001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.h:28: // collect URL?" bubble can be shown. nit: Please join with the previous line.
https://codereview.chromium.org/789613004/diff/240001/components/password_man... File components/password_manager/core/browser/password_manager.cc (right): https://codereview.chromium.org/789613004/diff/240001/components/password_man... components/password_manager/core/browser/password_manager.cc:329: if (failure == NO_MATCHING_FORM && IsSavingEnabledForCurrentPage() && On 2014/12/12 08:47:31, vabr (Chromium) wrote: > Good catch! > Maybe you could even move the IsSavingEnabledForCurrentPage to inside the > ShouldAskUserToSubmitURL method. That would make more sense, because that method > should be the ultimate source of truth for whether to show the bubble or not. In > other words, if somebody calls ShouldAskUserToSubmitURL somewhere else, they > should not need to worry about also copying the if-condition around it. Sure, I just didn't noticed that password manager is available in client. Done. https://codereview.chromium.org/789613004/diff/240001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment.cc (right): https://codereview.chromium.org/789613004/diff/240001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:24: // The bubble won't be shown, because active week for user gets multiplied On 2014/12/12 08:47:31, vabr (Chromium) wrote: > There is a misunderstanding here: > It's clear why setting defaultValueForStartingDayFactor to -1 causes the bubble > never to be shown. > What the comment should explain is: why the default value should cause the > bubble not to be shown. Maybe it is as simple as: > // A safe default value. Using this will not show the bubble. > > My question is whether the "safe default" is the reason, or if there is another > one. Yes, safe default is the reason https://codereview.chromium.org/789613004/diff/240001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment.h (right): https://codereview.chromium.org/789613004/diff/240001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.h:28: // collect URL?" bubble can be shown. On 2014/12/12 08:47:31, vabr (Chromium) wrote: > nit: Please join with the previous line. Done.
https://codereview.chromium.org/789613004/diff/220001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment.h (right): https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.h:12: class Time; On 2014/12/11 23:09:59, melandory wrote: > On 2014/12/11 18:46:46, vasilii wrote: > > For this class we need "#include" here. > > Why? This is a part of the interface. You return base::Time by value. Forward declaring everything frantically isn't good for code health. If I include your header and call DetermineStartOfActivityPeriod(nullptr, 0); it won't compile. https://codereview.chromium.org/789613004/diff/220001/components/password_man... File components/password_manager/core/common/password_manager_pref_names.cc (right): https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/common/password_manager_pref_names.cc:51: // period when "Allow to collect URL?" bubble can be shown. On 2014/12/11 23:09:59, melandory wrote: > On 2014/12/11 18:46:46, vasilii wrote: > > All the comments should be in the header instead. > > It states in .h that documentation should be in .cc: "Keep alphabetized, and > document each in the .cc file." That's not right but it's irrelevant to your CL. https://codereview.chromium.org/789613004/diff/220001/components/password_man... File components/password_manager/core/common/password_manager_pref_names.h (right): https://codereview.chromium.org/789613004/diff/220001/components/password_man... components/password_manager/core/common/password_manager_pref_names.h:28: extern const char kWasAllowToCollectURLBubbleShown[]; On 2014/12/11 23:10:00, melandory wrote: > On 2014/12/11 18:46:46, vasilii wrote: > > While you're here fix this name too. > > Sorry, don't follow you. How to fix it? The name sounds broken. It should be kSomeBubbleWasShown or kSomeBubbleShown. https://codereview.chromium.org/789613004/diff/280001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment.cc (right): https://codereview.chromium.org/789613004/diff/280001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:25: const int defaultValueForStartingDayFactor = -1; The name should start with 'k'. https://codereview.chromium.org/789613004/diff/280001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:42: bool ShouldShowBubbleExperiment(PrefService* prefs, base::Clock* clock) { The name sounds broken. One can't show the experiment but bubble. https://codereview.chromium.org/789613004/diff/280001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:71: const char kExperimentName[] = "AskToSubmitURLBubble"; Constants should go before RegisterPrefs. https://codereview.chromium.org/789613004/diff/280001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:78: double active_period_start_day_id = It's a factor, not id. https://codereview.chromium.org/789613004/diff/280001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment.h (right): https://codereview.chromium.org/789613004/diff/280001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.h:47: // and DetermineStartOfActivityPeriod computes the start of the period. I'd remove the line about DetermineStartOfActivityPeriod here.
https://codereview.chromium.org/789613004/diff/280001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment.cc (right): https://codereview.chromium.org/789613004/diff/280001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:25: const int defaultValueForStartingDayFactor = -1; On 2014/12/12 12:10:54, vasilii wrote: > The name should start with 'k'. Done. https://codereview.chromium.org/789613004/diff/280001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:42: bool ShouldShowBubbleExperiment(PrefService* prefs, base::Clock* clock) { On 2014/12/12 12:10:54, vasilii wrote: > The name sounds broken. One can't show the experiment but bubble. It should be parsed like [ShouldShowBubble]Experiment. Another name I can come up with is: ShoulBubbleBeShownExperiment. What do you think? https://codereview.chromium.org/789613004/diff/280001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:71: const char kExperimentName[] = "AskToSubmitURLBubble"; On 2014/12/12 12:10:54, vasilii wrote: > Constants should go before RegisterPrefs. Done. https://codereview.chromium.org/789613004/diff/280001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:78: double active_period_start_day_id = On 2014/12/12 12:10:54, vasilii wrote: > It's a factor, not id. Done. https://codereview.chromium.org/789613004/diff/280001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment.h (right): https://codereview.chromium.org/789613004/diff/280001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.h:47: // and DetermineStartOfActivityPeriod computes the start of the period. On 2014/12/12 12:10:54, vasilii wrote: > I'd remove the line about DetermineStartOfActivityPeriod here. It was suggested by vabr@ to add it, so I have to ask his opinion before removing. https://codereview.chromium.org/789613004/diff/320001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment.cc (right): https://codereview.chromium.org/789613004/diff/320001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:55: experiment_active_for_user_period); git cl format keeps saying that this is way to format this line. Fir me it's simpler to believe him, so I do not need manually change it back
My comments about the preference name and an include are not addressed. https://codereview.chromium.org/789613004/diff/280001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment.cc (right): https://codereview.chromium.org/789613004/diff/280001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:42: bool ShouldShowBubbleExperiment(PrefService* prefs, base::Clock* clock) { On 2014/12/12 13:30:42, melandory wrote: > On 2014/12/12 12:10:54, vasilii wrote: > > The name sounds broken. One can't show the experiment but bubble. > > It should be parsed like [ShouldShowBubble]Experiment. > Another name I can come up with is: ShoulBubbleBeShownExperiment. > What do you think? I think the function is identical in its meaning to ShouldShowBubble below. I'd call it ShouldShowBubbleInternal. https://codereview.chromium.org/789613004/diff/280001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment.h (right): https://codereview.chromium.org/789613004/diff/280001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.h:47: // and DetermineStartOfActivityPeriod computes the start of the period. On 2014/12/12 13:30:42, melandory wrote: > On 2014/12/12 12:10:54, vasilii wrote: > > I'd remove the line about DetermineStartOfActivityPeriod here. > > It was suggested by vabr@ to add it, so I have to ask his opinion before > removing. Vaclav, I think the comment above is sufficient. kParamActivePeriodInDays isn't related to the beginning of the period. If we need additional clarification on the algorithm it probably should go above ShouldShowBubble() https://codereview.chromium.org/789613004/diff/320001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment.cc (right): https://codereview.chromium.org/789613004/diff/320001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:25: const int kdefaultValueForStartingDayFactor = -1; kDefault... https://codereview.chromium.org/789613004/diff/320001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:55: experiment_active_for_user_period); On 2014/12/12 13:30:42, melandory wrote: > git cl format keeps saying that this is way to format this line. Fir me it's > simpler to believe him, so I do not need manually change it back Acknowledged.
On 2014/12/12 13:46:59, vasilii wrote: > My comments about the preference name and an include are not addressed. Sorry, I haven't noticed that you replied me. > > https://codereview.chromium.org/789613004/diff/280001/components/password_man... > File > components/password_manager/core/browser/password_manager_url_collection_experiment.cc > (right): > > https://codereview.chromium.org/789613004/diff/280001/components/password_man... > components/password_manager/core/browser/password_manager_url_collection_experiment.cc:42: > bool ShouldShowBubbleExperiment(PrefService* prefs, base::Clock* clock) { > On 2014/12/12 13:30:42, melandory wrote: > > On 2014/12/12 12:10:54, vasilii wrote: > > > The name sounds broken. One can't show the experiment but bubble. > > > > It should be parsed like [ShouldShowBubble]Experiment. > > Another name I can come up with is: ShoulBubbleBeShownExperiment. > > What do you think? > > I think the function is identical in its meaning to ShouldShowBubble below. I'd > call it ShouldShowBubbleInternal. > > https://codereview.chromium.org/789613004/diff/280001/components/password_man... > File > components/password_manager/core/browser/password_manager_url_collection_experiment.h > (right): > > https://codereview.chromium.org/789613004/diff/280001/components/password_man... > components/password_manager/core/browser/password_manager_url_collection_experiment.h:47: > // and DetermineStartOfActivityPeriod computes the start of the period. > On 2014/12/12 13:30:42, melandory wrote: > > On 2014/12/12 12:10:54, vasilii wrote: > > > I'd remove the line about DetermineStartOfActivityPeriod here. > > > > It was suggested by vabr@ to add it, so I have to ask his opinion before > > removing. > > Vaclav, I think the comment above is sufficient. kParamActivePeriodInDays isn't > related to the beginning of the period. If we need additional clarification on > the algorithm it probably should go above ShouldShowBubble() > > https://codereview.chromium.org/789613004/diff/320001/components/password_man... > File > components/password_manager/core/browser/password_manager_url_collection_experiment.cc > (right): > > https://codereview.chromium.org/789613004/diff/320001/components/password_man... > components/password_manager/core/browser/password_manager_url_collection_experiment.cc:25: > const int kdefaultValueForStartingDayFactor = -1; > kDefault... > > https://codereview.chromium.org/789613004/diff/320001/components/password_man... > components/password_manager/core/browser/password_manager_url_collection_experiment.cc:55: > experiment_active_for_user_period); > On 2014/12/12 13:30:42, melandory wrote: > > git cl format keeps saying that this is way to format this line. Fir me it's > > simpler to believe him, so I do not need manually change it back > > Acknowledged.
Sorry if missed something. https://codereview.chromium.org/789613004/diff/280001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment.cc (right): https://codereview.chromium.org/789613004/diff/280001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:42: bool ShouldShowBubbleExperiment(PrefService* prefs, base::Clock* clock) { On 2014/12/12 13:46:59, vasilii wrote: > On 2014/12/12 13:30:42, melandory wrote: > > On 2014/12/12 12:10:54, vasilii wrote: > > > The name sounds broken. One can't show the experiment but bubble. > > > > It should be parsed like [ShouldShowBubble]Experiment. > > Another name I can come up with is: ShoulBubbleBeShownExperiment. > > What do you think? > > I think the function is identical in its meaning to ShouldShowBubble below. I'd > call it ShouldShowBubbleInternal. Done. https://codereview.chromium.org/789613004/diff/320001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment.cc (right): https://codereview.chromium.org/789613004/diff/320001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:25: const int kdefaultValueForStartingDayFactor = -1; On 2014/12/12 13:46:59, vasilii wrote: > kDefault... Done.
https://codereview.chromium.org/789613004/diff/280001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment.h (right): https://codereview.chromium.org/789613004/diff/280001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.h:47: // and DetermineStartOfActivityPeriod computes the start of the period. On 2014/12/12 13:46:59, vasilii wrote: > On 2014/12/12 13:30:42, melandory wrote: > > On 2014/12/12 12:10:54, vasilii wrote: > > > I'd remove the line about DetermineStartOfActivityPeriod here. > > > > It was suggested by vabr@ to add it, so I have to ask his opinion before > > removing. > > Vaclav, I think the comment above is sufficient. kParamActivePeriodInDays isn't > related to the beginning of the period. If we need additional clarification on > the algorithm it probably should go above ShouldShowBubble() Sure, leaving out the line with DetermineStartOfActivityPeriod is fine here. (The original suggestion was not targeted at adding that particular information, but about explaining what kind of period was meant.)
LGTM with two light comments. :) https://codereview.chromium.org/789613004/diff/380001/components/password_man... File components/password_manager/core/common/password_manager_pref_names.h (right): https://codereview.chromium.org/789613004/diff/380001/components/password_man... components/password_manager/core/common/password_manager_pref_names.h:15: // component. Keep alphabetized, and document each in the .cc file. By moving the comments for the last two variables here, this comment became invalid. While I agree with Vasilii that descriptions belong to the header file, the ideal sequence of steps here is: 1. Add your new comments in the .cc file in this CL, to preserve consistency. 2. You or somebody else should file a separate clean-up moving all the comments from the .cc to the .h file. https://codereview.chromium.org/789613004/diff/380001/components/password_man... components/password_manager/core/common/password_manager_pref_names.h:28: // Boolean that indicates whether "Allow to collect URL?" bubble was shown or I hate to nitpick :), but the comment says "Boolean" while the type is char[]. Feel free to reuse the "The value of..." sentence below and adjust it to fit the description of the Boolean parameter here.
It's 4th time I'm asking to change the preference name. I'm not going to give in. https://codereview.chromium.org/789613004/diff/380001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment.h (right): https://codereview.chromium.org/789613004/diff/380001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.h:11: class Clock; unused https://codereview.chromium.org/789613004/diff/380001/components/password_man... File components/password_manager/core/common/password_manager_pref_names.h (right): https://codereview.chromium.org/789613004/diff/380001/components/password_man... components/password_manager/core/common/password_manager_pref_names.h:30: extern const char kWasAllowToCollectURLBubbleShown[]; The name sounds broken. It should be kSmthBubbleWasShown or kSmthBubbleShown.
On 2014/12/12 15:31:06, vasilii wrote: > It's 4th time I'm asking to change the preference name. I'm not going to give > in. It's not that I deliberately ignoring you. I'm just completely lost and have no clue how to follow comments without ability to mark them resolved and without ability to show list of unresolved comments. Plus last time you reminded I thought that you're talking about comments location in password_manager_pref_names.h I'm really sorry. > https://codereview.chromium.org/789613004/diff/380001/components/password_man... > File > components/password_manager/core/browser/password_manager_url_collection_experiment.h > (right): > > https://codereview.chromium.org/789613004/diff/380001/components/password_man... > components/password_manager/core/browser/password_manager_url_collection_experiment.h:11: > class Clock; > unused > > https://codereview.chromium.org/789613004/diff/380001/components/password_man... > File components/password_manager/core/common/password_manager_pref_names.h > (right): > > https://codereview.chromium.org/789613004/diff/380001/components/password_man... > components/password_manager/core/common/password_manager_pref_names.h:30: extern > const char kWasAllowToCollectURLBubbleShown[]; > The name sounds broken. It should be kSmthBubbleWasShown or kSmthBubbleShown.
https://codereview.chromium.org/789613004/diff/380001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment.h (right): https://codereview.chromium.org/789613004/diff/380001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.h:11: class Clock; On 2014/12/12 15:31:06, vasilii wrote: > unused Done. https://codereview.chromium.org/789613004/diff/380001/components/password_man... File components/password_manager/core/common/password_manager_pref_names.h (right): https://codereview.chromium.org/789613004/diff/380001/components/password_man... components/password_manager/core/common/password_manager_pref_names.h:28: // Boolean that indicates whether "Allow to collect URL?" bubble was shown or On 2014/12/12 15:04:13, vabr (Chromium) wrote: > I hate to nitpick :), but the comment says "Boolean" while the type is char[]. > Feel free to reuse the "The value of..." sentence below and adjust it to fit the > description of the Boolean parameter here. Done. https://codereview.chromium.org/789613004/diff/380001/components/password_man... components/password_manager/core/common/password_manager_pref_names.h:30: extern const char kWasAllowToCollectURLBubbleShown[]; On 2014/12/12 15:31:06, vasilii wrote: > The name sounds broken. It should be kSmthBubbleWasShown or kSmthBubbleShown. Name follows convention where "AllowToCollectURL" is name of bubble. Does kAllowToCollectURLBubbleWasShown sounds good to you?
https://codereview.chromium.org/789613004/diff/380001/components/password_man... File components/password_manager/core/common/password_manager_pref_names.h (right): https://codereview.chromium.org/789613004/diff/380001/components/password_man... components/password_manager/core/common/password_manager_pref_names.h:30: extern const char kWasAllowToCollectURLBubbleShown[]; On 2014/12/12 15:58:12, melandory wrote: > On 2014/12/12 15:31:06, vasilii wrote: > > The name sounds broken. It should be kSmthBubbleWasShown or kSmthBubbleShown. > > Name follows convention where "AllowToCollectURL" is name of bubble. > Does kAllowToCollectURLBubbleWasShown sounds good to you? Yes.
https://codereview.chromium.org/789613004/diff/380001/components/password_man... File components/password_manager/core/common/password_manager_pref_names.h (right): https://codereview.chromium.org/789613004/diff/380001/components/password_man... components/password_manager/core/common/password_manager_pref_names.h:30: extern const char kWasAllowToCollectURLBubbleShown[]; On 2014/12/12 16:00:05, vasilii wrote: > On 2014/12/12 15:58:12, melandory wrote: > > On 2014/12/12 15:31:06, vasilii wrote: > > > The name sounds broken. It should be kSmthBubbleWasShown or > kSmthBubbleShown. > > > > Name follows convention where "AllowToCollectURL" is name of bubble. > > Does kAllowToCollectURLBubbleWasShown sounds good to you? > > Yes. Done.
syntactically, LGTM % comments below However, I'm still not convinced of your 1-year scaling approach. I suggest continuing the discussion in the offline thread about with finch-team@ (since I'll be away soon) to see if we can find a better solution. But I won't block this CL or canary/dev experimentation because of it. https://codereview.chromium.org/789613004/diff/390007/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment.cc (right): https://codereview.chromium.org/789613004/diff/390007/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:35: !base::StringToInt(params[kParamActivePeriodInDays], &days)) Nit: I think {}'s are preferred for multi-line ifs. https://codereview.chromium.org/789613004/diff/390007/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:47: &experiment_active_for_user_period)) Nit: Ditto. https://codereview.chromium.org/789613004/diff/390007/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:92: bool ShouldShowBubble(PrefService* prefs, base::Clock* clock) { Nit: Overloading is discouraged. How about naming this ShouldShowBubbleWithClock()? https://codereview.chromium.org/789613004/diff/390007/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc (right): https://codereview.chromium.org/789613004/diff/390007/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc:37: ASSERT_TRUE(variations::AssociateVariationParams(kExperimentName, You need to call variations::testing::ClearAllVariationParams() in the TearDown() or destructor of the test - else this test will create global state that will affect other tests and cause flakyness - depending on the ordering of the tests. https://codereview.chromium.org/789613004/diff/390007/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc:50: bool ShouldShowBubble(PrefService* prefs, base::Clock* clock); This should be in the header file. Declaring external methods in a .cc file is discouraged. https://codereview.chromium.org/789613004/diff/390007/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc:61: password_manager::prefs:: Since this code is in password_manager:: namespace, you don't need to use these prefixes throughout this file. https://codereview.chromium.org/789613004/diff/390007/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc:93: }; Nit: DISALLOW_COPY_AND_ASSIGN
lgtm
The CQ bit was checked by melandory@chromium.org
https://codereview.chromium.org/789613004/diff/390007/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment.cc (right): https://codereview.chromium.org/789613004/diff/390007/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:35: !base::StringToInt(params[kParamActivePeriodInDays], &days)) On 2014/12/12 16:38:50, asvitkine (OOO until Jan4) wrote: > Nit: I think {}'s are preferred for multi-line ifs. Done. https://codereview.chromium.org/789613004/diff/390007/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:47: &experiment_active_for_user_period)) On 2014/12/12 16:38:50, asvitkine (OOO until Jan4) wrote: > Nit: Ditto. Done. https://codereview.chromium.org/789613004/diff/390007/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:92: bool ShouldShowBubble(PrefService* prefs, base::Clock* clock) { On 2014/12/12 16:38:50, asvitkine (OOO until Jan4) wrote: > Nit: Overloading is discouraged. How about naming this > ShouldShowBubbleWithClock()? Done. https://codereview.chromium.org/789613004/diff/390007/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc (right): https://codereview.chromium.org/789613004/diff/390007/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc:37: ASSERT_TRUE(variations::AssociateVariationParams(kExperimentName, On 2014/12/12 16:38:50, asvitkine (OOO until Jan4) wrote: > You need to call variations::testing::ClearAllVariationParams() in the > TearDown() or destructor of the test - else this test will create global state > that will affect other tests and cause flakyness - depending on the ordering of > the tests. Done. https://codereview.chromium.org/789613004/diff/390007/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc:50: bool ShouldShowBubble(PrefService* prefs, base::Clock* clock); On 2014/12/12 16:38:51, asvitkine (OOO until Jan4) wrote: > This should be in the header file. Declaring external methods in a .cc file is > discouraged. Won't fix as it was in .h file but then moved to .cc as vasilii@ recommended. https://codereview.chromium.org/789613004/diff/390007/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc:61: password_manager::prefs:: On 2014/12/12 16:38:51, asvitkine (OOO until Jan4) wrote: > Since this code is in password_manager:: namespace, you don't need to use these > prefixes throughout this file. Done.
https://codereview.chromium.org/789613004/diff/390007/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment.cc (right): https://codereview.chromium.org/789613004/diff/390007/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:35: !base::StringToInt(params[kParamActivePeriodInDays], &days)) On 2014/12/12 16:38:50, asvitkine (OOO until Jan4) wrote: > Nit: I think {}'s are preferred for multi-line ifs. Done. https://codereview.chromium.org/789613004/diff/390007/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:47: &experiment_active_for_user_period)) On 2014/12/12 16:38:50, asvitkine (OOO until Jan4) wrote: > Nit: Ditto. Done. https://codereview.chromium.org/789613004/diff/390007/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:92: bool ShouldShowBubble(PrefService* prefs, base::Clock* clock) { On 2014/12/12 16:38:50, asvitkine (OOO until Jan4) wrote: > Nit: Overloading is discouraged. How about naming this > ShouldShowBubbleWithClock()? Done. https://codereview.chromium.org/789613004/diff/390007/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc (right): https://codereview.chromium.org/789613004/diff/390007/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc:37: ASSERT_TRUE(variations::AssociateVariationParams(kExperimentName, On 2014/12/12 16:38:50, asvitkine (OOO until Jan4) wrote: > You need to call variations::testing::ClearAllVariationParams() in the > TearDown() or destructor of the test - else this test will create global state > that will affect other tests and cause flakyness - depending on the ordering of > the tests. Done. https://codereview.chromium.org/789613004/diff/390007/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc:50: bool ShouldShowBubble(PrefService* prefs, base::Clock* clock); On 2014/12/12 16:38:51, asvitkine (OOO until Jan4) wrote: > This should be in the header file. Declaring external methods in a .cc file is > discouraged. Won't fix as it was in .h file but then moved to .cc as vasilii@ recommended. https://codereview.chromium.org/789613004/diff/390007/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc:61: password_manager::prefs:: On 2014/12/12 16:38:51, asvitkine (OOO until Jan4) wrote: > Since this code is in password_manager:: namespace, you don't need to use these > prefixes throughout this file. Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/789613004/470001
Message was sent while issue was closed.
Committed patchset #19 (id:470001)
Message was sent while issue was closed.
Patchset 19 (id:??) landed as https://crrev.com/c489fa833c62edf608a6ecbf553036fe9d7a3292 Cr-Commit-Position: refs/heads/master@{#308170} |
