Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
777423004:
Skeleton code for experiment setup, which will determine should "Allow to collect URL?" bubble be s… (Closed)
DescriptionSkeleton code for experiment setup, which will determine should "Allow to collect URL?" bubble be shown.
This patch doesn't adding functional changes to Chrome.
BUG=435080
R=vabr@chromium.org,battre@chromium.org
Committed: https://crrev.com/b1e332c5692df7d594d718ac26e6f1b6cd41e2ea
Cr-Commit-Position: refs/heads/master@{#307795}
Patch Set 1 #
Total comments: 1
Patch Set 2 : unmoved #Patch Set 3 : Changes on top of master, not url_collection_send_feedback #Patch Set 4 : unit test for pref #
Total comments: 14
Patch Set 5 : #
Total comments: 10
Patch Set 6 : #Patch Set 7 : Rebased on top of master #
Total comments: 5
Patch Set 8 : #
Total comments: 9
Patch Set 9 : #Messages
Total messages: 43 (8 generated)
https://codereview.chromium.org/777423004/diff/1/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/777423004/diff/1/chrome/common/pref_names.cc#... chrome/common/pref_names.cc:2306: const char kWasAllowToCollectURLBubbleShown[] = I believe you can put this constant in components/password_manager/core/common/password_manager_pref_names.* instead. That should solve the dependencies.
Patchset #2 (id:20001) has been deleted
On 2014/12/08 08:52:33, vabr (Chromium) wrote: > https://codereview.chromium.org/777423004/diff/1/chrome/common/pref_names.cc > File chrome/common/pref_names.cc (right): > > https://codereview.chromium.org/777423004/diff/1/chrome/common/pref_names.cc#... > chrome/common/pref_names.cc:2306: const char kWasAllowToCollectURLBubbleShown[] > = > I believe you can put this constant in > components/password_manager/core/common/password_manager_pref_names.* instead. > That should solve the dependencies. Thanks, it worked.
Thanks for the CL. A couple of comments are below. Cheers, Vaclav https://codereview.chromium.org/777423004/diff/80001/chrome/browser/ui/passwo... File chrome/browser/ui/passwords/manage_passwords_bubble_model.cc (right): https://codereview.chromium.org/777423004/diff/80001/chrome/browser/ui/passwo... chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:64: password_manager::urls_collection_experiment::RecordBubbleClosed( It looks like this is called when the bubble is open. Do I misunderstand something? https://codereview.chromium.org/777423004/diff/80001/components/password_mana... File components/password_manager/DEPS (right): https://codereview.chromium.org/777423004/diff/80001/components/password_mana... components/password_manager/DEPS:5: "+components/variations", Please add this to components/password_manager/core/browser/DEPS instead. https://codereview.chromium.org/777423004/diff/80001/components/password_mana... File components/password_manager/core/browser/password_manager_url_collection_experiment.cc (right): https://codereview.chromium.org/777423004/diff/80001/components/password_mana... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:31: const char kExperimentName[] = "AksToSubmitURLBubble"; typo: Aks->Ask https://codereview.chromium.org/777423004/diff/80001/components/password_mana... File components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc (right): https://codereview.chromium.org/777423004/diff/80001/components/password_mana... components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc:41: PrefService* prefs() { return &pref_service_; } nit: Please add a blank line above line 41. https://codereview.chromium.org/777423004/diff/80001/components/password_mana... components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc:45: scoped_ptr<base::FieldTrialList> field_trial_list_; Could this be done without the scoped_ptr, just aggregating base::FieldTrialList directly, and initialising it in the initialiser list of the test constructor? https://codereview.chromium.org/777423004/diff/80001/components/password_mana... components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc:55: EXPECT_FALSE( optional: Maybe add a TODO(melandory) + bug URL about this expectation being just temporary, until the code starts being able to show the bubble to the user. https://codereview.chromium.org/777423004/diff/80001/components/password_mana... File components/password_manager/core/common/password_manager_pref_names.cc (right): https://codereview.chromium.org/777423004/diff/80001/components/password_mana... components/password_manager/core/common/password_manager_pref_names.cc:48: const char kWasAllowToCollectURLBubbleShown[] = The constant name does not match the comment: being allowed to show the bubble is not necessarily the same as having shown the bubble. I suggest dropping "Was" from the name. If you have good reasons to keep the constant's name, then please both amend the comment, and change Allow to Allowed in the name.
Hi Vaclav, thanks for reviewing and in person discussion. Here is patch set with fixes. https://codereview.chromium.org/777423004/diff/80001/chrome/browser/ui/passwo... File chrome/browser/ui/passwords/manage_passwords_bubble_model.cc (right): https://codereview.chromium.org/777423004/diff/80001/chrome/browser/ui/passwo... chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:64: password_manager::urls_collection_experiment::RecordBubbleClosed( On 2014/12/08 11:19:33, vabr (Chromium) wrote: > It looks like this is called when the bubble is open. Do I misunderstand > something? Done. Yep, you are right. There was reason why it was called before creating a bubble, but now it's gone. https://codereview.chromium.org/777423004/diff/80001/components/password_mana... File components/password_manager/DEPS (right): https://codereview.chromium.org/777423004/diff/80001/components/password_mana... components/password_manager/DEPS:5: "+components/variations", On 2014/12/08 11:19:33, vabr (Chromium) wrote: > Please add this to components/password_manager/core/browser/DEPS instead. Done. https://codereview.chromium.org/777423004/diff/80001/components/password_mana... File components/password_manager/core/browser/password_manager_url_collection_experiment.cc (right): https://codereview.chromium.org/777423004/diff/80001/components/password_mana... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:31: const char kExperimentName[] = "AksToSubmitURLBubble"; On 2014/12/08 11:19:33, vabr (Chromium) wrote: > typo: Aks->Ask Done. https://codereview.chromium.org/777423004/diff/80001/components/password_mana... File components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc (right): https://codereview.chromium.org/777423004/diff/80001/components/password_mana... components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc:41: PrefService* prefs() { return &pref_service_; } On 2014/12/08 11:19:33, vabr (Chromium) wrote: > nit: Please add a blank line above line 41. Done. https://codereview.chromium.org/777423004/diff/80001/components/password_mana... components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc:45: scoped_ptr<base::FieldTrialList> field_trial_list_; On 2014/12/08 11:19:33, vabr (Chromium) wrote: > Could this be done without the scoped_ptr, just aggregating base::FieldTrialList > directly, and initialising it in the initialiser list of the test constructor? Done. https://codereview.chromium.org/777423004/diff/80001/components/password_mana... components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc:55: EXPECT_FALSE( On 2014/12/08 11:19:34, vabr (Chromium) wrote: > optional: Maybe add a TODO(melandory) + bug URL about this expectation being > just temporary, until the code starts being able to show the bubble to the user. Done. https://codereview.chromium.org/777423004/diff/80001/components/password_mana... File components/password_manager/core/common/password_manager_pref_names.cc (right): https://codereview.chromium.org/777423004/diff/80001/components/password_mana... components/password_manager/core/common/password_manager_pref_names.cc:48: const char kWasAllowToCollectURLBubbleShown[] = On 2014/12/08 11:19:34, vabr (Chromium) wrote: > The constant name does not match the comment: being allowed to show the bubble > is not necessarily the same as having shown the bubble. I suggest dropping "Was" > from the name. > > If you have good reasons to keep the constant's name, then please both amend the > comment, and change Allow to Allowed in the name. Done.
LGTM with nits. Feel free to land, as soon as you address the comments. Cheers, Vaclav https://codereview.chromium.org/777423004/diff/100001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc (right): https://codereview.chromium.org/777423004/diff/100001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc:36: : field_trial_list_(new metrics::SHA1EntropyProvider("foo")) {} nit: One blank line after line 36, please. :) https://codereview.chromium.org/777423004/diff/100001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc:58: // crbug/435080 nits: (1) Please include a full-stop at the end of the sentence. (2) If you write http://crbug.com/435080, as opposed to just crbug/435080, it will become clickable in the CodeSearch.
On 2014/12/08 13:17:20, vabr (Chromium) wrote: > LGTM with nits. > Feel free to land, as soon as you address the comments. Actually, hold on -- you also need an OWNER review for chrome/browser/prefs/browser_prefs.cc. I suggest asking battre@ for a rubber stamp on that file (make sure to mention the file, to spare him time reviewing the whole CL). Cheers, Vaclav
Hi Dominic, can you look at chrome/browser/prefs/browser_prefs.cc
melandory@chromium.org changed reviewers: + battre@chromium.org
chrome/browser/prefs/browser_prefs.cc LGTM https://codereview.chromium.org/777423004/diff/100001/chrome/browser/ui/passw... File chrome/browser/ui/passwords/manage_passwords_bubble_model.cc (right): https://codereview.chromium.org/777423004/diff/100001/chrome/browser/ui/passw... chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:59: void RecordURLSCollectionExperimentStatistics( nit: RecordURLsCollectionExperimentStatistics https://codereview.chromium.org/777423004/diff/100001/chrome/browser/ui/passw... chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:60: content::WebContents* web_contents) { Does this fit into one line? https://codereview.chromium.org/777423004/diff/100001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment.cc (right): https://codereview.chromium.org/777423004/diff/100001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:17: bool MaybeShowBubbleExperiment(PrefService* prefs) { Can you come up with a name that does not start with Maybe? Is there a precedent for this? I find it difficult to understand what this function would do as "Maybe" is not a verb.
https://codereview.chromium.org/777423004/diff/100001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment.cc (right): https://codereview.chromium.org/777423004/diff/100001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:17: bool MaybeShowBubbleExperiment(PrefService* prefs) { On 2014/12/09 11:02:16, battre wrote: > Can you come up with a name that does not start with Maybe? Is there a precedent > for this? I find it difficult to understand what this function would do as > "Maybe" is not a verb. This is not that uncommon, quick codesearch, e.g., [void\ Maybe], reveals 200+ occurences. A random hand-picked example is MaybeGenerateAuthToken of /net/http/http_auth_controller.h. It is generally used in the form MaybeDoStuff, where DoStuff may not be carried out for all input conditions.
On 2014/12/09 at 11:34:06, vabr wrote: > https://codereview.chromium.org/777423004/diff/100001/components/password_man... > File components/password_manager/core/browser/password_manager_url_collection_experiment.cc (right): > > https://codereview.chromium.org/777423004/diff/100001/components/password_man... > components/password_manager/core/browser/password_manager_url_collection_experiment.cc:17: bool MaybeShowBubbleExperiment(PrefService* prefs) { > On 2014/12/09 11:02:16, battre wrote: > > Can you come up with a name that does not start with Maybe? Is there a precedent > > for this? I find it difficult to understand what this function would do as > > "Maybe" is not a verb. > > This is not that uncommon, quick codesearch, e.g., [void\ Maybe], reveals 200+ occurences. A random hand-picked example is MaybeGenerateAuthToken of /net/http/http_auth_controller.h. > It is generally used in the form MaybeDoStuff, where DoStuff may not be carried out for all input conditions. My understanding of MaybeGenerateAuthToken is different: It either generates an auth token or not. My understanding of MaybeShowBubbleExperiment was that this is really a "ShouldShowBubbleExperiment" whose core purpose is not to do anything but return true or false. Maybe my understanding was wrong.
On 2014/12/09 14:57:33, battre wrote: > On 2014/12/09 at 11:34:06, vabr wrote: > > > https://codereview.chromium.org/777423004/diff/100001/components/password_man... > > File > components/password_manager/core/browser/password_manager_url_collection_experiment.cc > (right): > > > > > https://codereview.chromium.org/777423004/diff/100001/components/password_man... > > > components/password_manager/core/browser/password_manager_url_collection_experiment.cc:17: > bool MaybeShowBubbleExperiment(PrefService* prefs) { > > On 2014/12/09 11:02:16, battre wrote: > > > Can you come up with a name that does not start with Maybe? Is there a > precedent > > > for this? I find it difficult to understand what this function would do as > > > "Maybe" is not a verb. > > > > This is not that uncommon, quick codesearch, e.g., [void\ Maybe], reveals 200+ > occurences. A random hand-picked example is MaybeGenerateAuthToken of > /net/http/http_auth_controller.h. > > It is generally used in the form MaybeDoStuff, where DoStuff may not be > carried out for all input conditions. > > My understanding of MaybeGenerateAuthToken is different: It either generates an > auth token or not. > > My understanding of MaybeShowBubbleExperiment was that this is really a > "ShouldShowBubbleExperiment" whose core purpose is not to do anything but return > true or false. > > Maybe my understanding was wrong. You are right, I read it wrong. I agree that the use of Maybe in the method name and in "kGroupMaybeShowBubble" is slightly confusing. These names should reference the trial group name (ShowBubble), not its effect (maybe showing bubble). But I think I should let Tanja speak for her CL at this moment. :)
On 2014/12/09 15:10:54, vabr (Chromium) wrote: > On 2014/12/09 14:57:33, battre wrote: > > On 2014/12/09 at 11:34:06, vabr wrote: > > > > > > https://codereview.chromium.org/777423004/diff/100001/components/password_man... > > > File > > > components/password_manager/core/browser/password_manager_url_collection_experiment.cc > > (right): > > > > > > > > > https://codereview.chromium.org/777423004/diff/100001/components/password_man... > > > > > > components/password_manager/core/browser/password_manager_url_collection_experiment.cc:17: > > bool MaybeShowBubbleExperiment(PrefService* prefs) { > > > On 2014/12/09 11:02:16, battre wrote: > > > > Can you come up with a name that does not start with Maybe? Is there a > > precedent > > > > for this? I find it difficult to understand what this function would do as > > > > "Maybe" is not a verb. > > > > > > This is not that uncommon, quick codesearch, e.g., [void\ Maybe], reveals > 200+ > > occurences. A random hand-picked example is MaybeGenerateAuthToken of > > /net/http/http_auth_controller.h. > > > It is generally used in the form MaybeDoStuff, where DoStuff may not be > > carried out for all input conditions. > > > > My understanding of MaybeGenerateAuthToken is different: It either generates > an > > auth token or not. > > > > My understanding of MaybeShowBubbleExperiment was that this is really a > > "ShouldShowBubbleExperiment" whose core purpose is not to do anything but > return > > true or false. > > > > Maybe my understanding was wrong. > > You are right, I read it wrong. > I agree that the use of Maybe in the method name and in "kGroupMaybeShowBubble" > is slightly confusing. These names should reference the trial group name > (ShowBubble), not its effect (maybe showing bubble). > > But I think I should let Tanja speak for her CL at this moment. :) My motivation behind "Maybe" in MaybeShowBubbleExperiment is following: fact that user are in ShowBubble group doesn't necessary lead to appearance of a bubble because it can be scheduled for other week. But on the other hand I agree that Maybe in group name kGroupMaybeShowBubble is confusing.
On 2014/12/09 15:20:16, melandory wrote: > On 2014/12/09 15:10:54, vabr (Chromium) wrote: > > On 2014/12/09 14:57:33, battre wrote: > > > On 2014/12/09 at 11:34:06, vabr wrote: > > > > > > > > > > https://codereview.chromium.org/777423004/diff/100001/components/password_man... > > > > File > > > > > > components/password_manager/core/browser/password_manager_url_collection_experiment.cc > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/777423004/diff/100001/components/password_man... > > > > > > > > > > components/password_manager/core/browser/password_manager_url_collection_experiment.cc:17: > > > bool MaybeShowBubbleExperiment(PrefService* prefs) { > > > > On 2014/12/09 11:02:16, battre wrote: > > > > > Can you come up with a name that does not start with Maybe? Is there a > > > precedent > > > > > for this? I find it difficult to understand what this function would do > as > > > > > "Maybe" is not a verb. > > > > > > > > This is not that uncommon, quick codesearch, e.g., [void\ Maybe], reveals > > 200+ > > > occurences. A random hand-picked example is MaybeGenerateAuthToken of > > > /net/http/http_auth_controller.h. > > > > It is generally used in the form MaybeDoStuff, where DoStuff may not be > > > carried out for all input conditions. > > > > > > My understanding of MaybeGenerateAuthToken is different: It either generates > > an > > > auth token or not. > > > > > > My understanding of MaybeShowBubbleExperiment was that this is really a > > > "ShouldShowBubbleExperiment" whose core purpose is not to do anything but > > return > > > true or false. > > > > > > Maybe my understanding was wrong. > > > > You are right, I read it wrong. > > I agree that the use of Maybe in the method name and in > "kGroupMaybeShowBubble" > > is slightly confusing. These names should reference the trial group name > > (ShowBubble), not its effect (maybe showing bubble). > > > > But I think I should let Tanja speak for her CL at this moment. :) > > My motivation behind "Maybe" in MaybeShowBubbleExperiment is following: fact > that user are in ShowBubble group doesn't necessary lead to appearance of a > bubble because it can be scheduled for other week. I believe that the name has a deeper problem: the only verb there is "show", but the method does not show anything. Dominic already suggested "ShouldShowBubbleExperiment", which makes it clear that the method answers the question "should show ...". It also matches the experiment group name (ShowBubble) exactly. > But on the other hand I agree that Maybe in group name kGroupMaybeShowBubble is > confusing. This is a minor point. But the kGroupNeverShowBubble suggests that kGroupMaybeShowBubble should be called kGroupShowBubble to follow the pattern of kGroup<group name>.
The CQ bit was checked by melandory@chromium.org
https://codereview.chromium.org/777423004/diff/100001/chrome/browser/ui/passw... File chrome/browser/ui/passwords/manage_passwords_bubble_model.cc (right): https://codereview.chromium.org/777423004/diff/100001/chrome/browser/ui/passw... chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:60: content::WebContents* web_contents) { On 2014/12/09 11:02:16, battre wrote: > Does this fit into one line? 67 symbols https://codereview.chromium.org/777423004/diff/100001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment.cc (right): https://codereview.chromium.org/777423004/diff/100001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:17: bool MaybeShowBubbleExperiment(PrefService* prefs) { On 2014/12/09 11:34:05, vabr (Chromium) wrote: > On 2014/12/09 11:02:16, battre wrote: > > Can you come up with a name that does not start with Maybe? Is there a > precedent > > for this? I find it difficult to understand what this function would do as > > "Maybe" is not a verb. > > This is not that uncommon, quick codesearch, e.g., [void\ Maybe], reveals 200+ > occurences. A random hand-picked example is MaybeGenerateAuthToken of > /net/http/http_auth_controller.h. > It is generally used in the form MaybeDoStuff, where DoStuff may not be carried > out for all input conditions. Done. https://codereview.chromium.org/777423004/diff/100001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc (right): https://codereview.chromium.org/777423004/diff/100001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc:36: : field_trial_list_(new metrics::SHA1EntropyProvider("foo")) {} On 2014/12/08 13:17:20, vabr (Chromium) wrote: > nit: One blank line after line 36, please. :) Done. https://codereview.chromium.org/777423004/diff/100001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc:58: // crbug/435080 On 2014/12/08 13:17:20, vabr (Chromium) wrote: > nits: > (1) Please include a full-stop at the end of the sentence. > (2) If you write http://crbug.com/435080, as opposed to just crbug/435080, it > will become clickable in the CodeSearch. Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/777423004/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
melandory@chromium.org changed reviewers: + asvitkine@chromium.org
Hi Alexei, can you looks at dependencies added to DEPS. Thanks.
https://codereview.chromium.org/777423004/diff/140001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment.cc (right): https://codereview.chromium.org/777423004/diff/140001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:41: if (group_name == kGroupShowBubble) If you're going to make a decision based on experiment params, then you don't actually need to use the field trial APIs at all! You can just do something like this: std::string show_bubble = variations::GetVariationParamValue("AskToSubmitURLBubble", "ShowBubble"); return show_bubble == "Show"; https://codereview.chromium.org/777423004/diff/140001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc (right): https://codereview.chromium.org/777423004/diff/140001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc:14: #include "components/variations/variations_associated_data.h" Not sure you need this include.
https://codereview.chromium.org/777423004/diff/140001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment.cc (right): https://codereview.chromium.org/777423004/diff/140001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:41: if (group_name == kGroupShowBubble) On 2014/12/10 16:38:03, Alexei Svitkine wrote: > If you're going to make a decision based on experiment params, then you don't > actually need to use the field trial APIs at all! > > You can just do something like this: > > std::string show_bubble = > variations::GetVariationParamValue("AskToSubmitURLBubble", "ShowBubble"); > return show_bubble == "Show"; Sorry, didn't get your point. I see that workflow should be following. I have two groups, for first group I do not show bubble at all and don't have any parameters specified, for second group I take values of parameters in this group and then I decide should I show bubble now or postpone it. So to summarize, first I need to check which group this user belongs and if it's second one then I check params. https://codereview.chromium.org/777423004/diff/140001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc (right): https://codereview.chromium.org/777423004/diff/140001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc:14: #include "components/variations/variations_associated_data.h" On 2014/12/10 16:38:03, Alexei Svitkine wrote: > Not sure you need this include. Done. Yep, I will need it in another CL. I guess I accidentally left it here during separation of changes onto two CL's
https://codereview.chromium.org/777423004/diff/140001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment.cc (right): https://codereview.chromium.org/777423004/diff/140001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:41: if (group_name == kGroupShowBubble) On 2014/12/10 17:22:36, melandory wrote: > On 2014/12/10 16:38:03, Alexei Svitkine wrote: > > If you're going to make a decision based on experiment params, then you don't > > actually need to use the field trial APIs at all! > > > > You can just do something like this: > > > > std::string show_bubble = > > variations::GetVariationParamValue("AskToSubmitURLBubble", > "ShowBubble"); > > return show_bubble == "Show"; > Sorry, didn't get your point. I see that workflow should be following. > I have two groups, for first group I do not show bubble at all and don't have > any parameters specified, for second group I take values of parameters in this > group and then I decide should I show bubble now or postpone it. > So to summarize, first I need to check which group this user belongs and if it's > second one then I check params. The variations params API does the check internally. The idea is the group will define a param called "ShowBubble", which will have a different value depending which group was selected. So, when you use the API I suggest, it does "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.
On 2014/12/10 17:58:45, Alexei Svitkine wrote: > https://codereview.chromium.org/777423004/diff/140001/components/password_man... > File > components/password_manager/core/browser/password_manager_url_collection_experiment.cc > (right): > > https://codereview.chromium.org/777423004/diff/140001/components/password_man... > components/password_manager/core/browser/password_manager_url_collection_experiment.cc:41: > if (group_name == kGroupShowBubble) > On 2014/12/10 17:22:36, melandory wrote: > > On 2014/12/10 16:38:03, Alexei Svitkine wrote: > > > If you're going to make a decision based on experiment params, then you > don't > > > actually need to use the field trial APIs at all! > > > > > > You can just do something like this: > > > > > > std::string show_bubble = > > > variations::GetVariationParamValue("AskToSubmitURLBubble", > > "ShowBubble"); > > > return show_bubble == "Show"; > > Sorry, didn't get your point. I see that workflow should be following. > > I have two groups, for first group I do not show bubble at all and don't have > > any parameters specified, for second group I take values of parameters in this > > group and then I decide should I show bubble now or postpone it. > > So to summarize, first I need to check which group this user belongs and if > it's > > second one then I check params. > > The variations params API does the check internally. > > The idea is the group will define a param called "ShowBubble", which will have a > different value depending which group was selected. > > So, when you use the API I suggest, it does "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. Got it. Will do change. Can you then recheck Finch config file: cl/81779587?
On 2014/12/10 18:06:20, melandory wrote: > On 2014/12/10 17:58:45, Alexei Svitkine wrote: > > > https://codereview.chromium.org/777423004/diff/140001/components/password_man... > > File > > > components/password_manager/core/browser/password_manager_url_collection_experiment.cc > > (right): > > > > > https://codereview.chromium.org/777423004/diff/140001/components/password_man... > > > components/password_manager/core/browser/password_manager_url_collection_experiment.cc:41: > > if (group_name == kGroupShowBubble) > > On 2014/12/10 17:22:36, melandory wrote: > > > On 2014/12/10 16:38:03, Alexei Svitkine wrote: > > > > If you're going to make a decision based on experiment params, then you > > don't > > > > actually need to use the field trial APIs at all! > > > > > > > > You can just do something like this: > > > > > > > > std::string show_bubble = > > > > variations::GetVariationParamValue("AskToSubmitURLBubble", > > > "ShowBubble"); > > > > return show_bubble == "Show"; > > > Sorry, didn't get your point. I see that workflow should be following. > > > I have two groups, for first group I do not show bubble at all and don't > have > > > any parameters specified, for second group I take values of parameters in > this > > > group and then I decide should I show bubble now or postpone it. > > > So to summarize, first I need to check which group this user belongs and if > > it's > > > second one then I check params. > > > > The variations params API does the check internally. > > > > The idea is the group will define a param called "ShowBubble", which will have > a > > different value depending which group was selected. > > > > So, when you use the API I suggest, it does "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. > > Got it. Will do change. Can you then recheck Finch config file: cl/81779587? Or better, may I add this changes https://codereview.chromium.org/789613004/? Since in code there I deal with experiment parameters, so I have all code that supports them in unittests there not here. So it will be much easier to add it there. And then it will be easier to rebase on master when I land this CL. I'll add you as reviewer there also.
Sure, am OK with you doing it in that different CL. Which CL were you planning on landing first? For this CL, if the plan is to use params, I suggest removing the field trial API use for now. On Wed, Dec 10, 2014 at 1:16 PM, <melandory@chromium.org> wrote: > On 2014/12/10 18:06:20, melandory wrote: > >> On 2014/12/10 17:58:45, Alexei Svitkine wrote: >> > >> > > https://codereview.chromium.org/777423004/diff/140001/ > components/password_manager/core/browser/password_manager_ > url_collection_experiment.cc > >> > File >> > >> > > components/password_manager/core/browser/password_manager_ > url_collection_experiment.cc > >> > (right): >> > >> > >> > > https://codereview.chromium.org/777423004/diff/140001/ > components/password_manager/core/browser/password_manager_ > url_collection_experiment.cc#newcode41 > >> > >> > > components/password_manager/core/browser/password_manager_ > url_collection_experiment.cc:41: > >> > if (group_name == kGroupShowBubble) >> > On 2014/12/10 17:22:36, melandory wrote: >> > > On 2014/12/10 16:38:03, Alexei Svitkine wrote: >> > > > If you're going to make a decision based on experiment params, then >> you >> > don't >> > > > actually need to use the field trial APIs at all! >> > > > >> > > > You can just do something like this: >> > > > >> > > > std::string show_bubble = >> > > > variations::GetVariationParamValue("AskToSubmitURLBubble", >> > > "ShowBubble"); >> > > > return show_bubble == "Show"; >> > > Sorry, didn't get your point. I see that workflow should be following. >> > > I have two groups, for first group I do not show bubble at all and >> don't >> have >> > > any parameters specified, for second group I take values of >> parameters in >> this >> > > group and then I decide should I show bubble now or postpone it. >> > > So to summarize, first I need to check which group this user belongs >> and >> > if > >> > it's >> > > second one then I check params. >> > >> > The variations params API does the check internally. >> > >> > The idea is the group will define a param called "ShowBubble", which >> will >> > have > >> a >> > different value depending which group was selected. >> > >> > So, when you use the API I suggest, it does "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. >> > > Got it. Will do change. Can you then recheck Finch config file: >> cl/81779587? >> > > Or better, may I add this changes https://codereview.chromium. > org/789613004/? > Since in code there I deal with experiment parameters, so I have all code > that > supports them in unittests there not here. So it will be much easier to > add it > there. > And then it will be easier to rebase on master when I land this CL. > > I'll add you as reviewer there also. > > https://codereview.chromium.org/777423004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/12/10 18:25:03, Alexei Svitkine wrote: > Sure, am OK with you doing it in that different CL. Which CL were you > planning on landing first? This one. Because second one depends on this. > > For this CL, if the plan is to use params, I suggest removing the field > trial API use for now. Done. > > On Wed, Dec 10, 2014 at 1:16 PM, <mailto:melandory@chromium.org> wrote: > > > On 2014/12/10 18:06:20, melandory wrote: > > > >> On 2014/12/10 17:58:45, Alexei Svitkine wrote: > >> > > >> > > > > https://codereview.chromium.org/777423004/diff/140001/ > > components/password_manager/core/browser/password_manager_ > > url_collection_experiment.cc > > > >> > File > >> > > >> > > > > components/password_manager/core/browser/password_manager_ > > url_collection_experiment.cc > > > >> > (right): > >> > > >> > > >> > > > > https://codereview.chromium.org/777423004/diff/140001/ > > components/password_manager/core/browser/password_manager_ > > url_collection_experiment.cc#newcode41 > > > >> > > >> > > > > components/password_manager/core/browser/password_manager_ > > url_collection_experiment.cc:41: > > > >> > if (group_name == kGroupShowBubble) > >> > On 2014/12/10 17:22:36, melandory wrote: > >> > > On 2014/12/10 16:38:03, Alexei Svitkine wrote: > >> > > > If you're going to make a decision based on experiment params, then > >> you > >> > don't > >> > > > actually need to use the field trial APIs at all! > >> > > > > >> > > > You can just do something like this: > >> > > > > >> > > > std::string show_bubble = > >> > > > variations::GetVariationParamValue("AskToSubmitURLBubble", > >> > > "ShowBubble"); > >> > > > return show_bubble == "Show"; > >> > > Sorry, didn't get your point. I see that workflow should be following. > >> > > I have two groups, for first group I do not show bubble at all and > >> don't > >> have > >> > > any parameters specified, for second group I take values of > >> parameters in > >> this > >> > > group and then I decide should I show bubble now or postpone it. > >> > > So to summarize, first I need to check which group this user belongs > >> and > >> > > if > > > >> > it's > >> > > second one then I check params. > >> > > >> > The variations params API does the check internally. > >> > > >> > The idea is the group will define a param called "ShowBubble", which > >> will > >> > > have > > > >> a > >> > different value depending which group was selected. > >> > > >> > So, when you use the API I suggest, it does "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. > >> > > > > Got it. Will do change. Can you then recheck Finch config file: > >> cl/81779587? > >> > > > > Or better, may I add this changes https://codereview.chromium. > > org/789613004/? > > Since in code there I deal with experiment parameters, so I have all code > > that > > supports them in unittests there not here. So it will be much easier to > > add it > > there. > > And then it will be easier to rebase on master when I land this CL. > > > > I'll add you as reviewer there also. > > > > https://codereview.chromium.org/777423004/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
Patchset #8 (id:160001) has been deleted
Patchset #8 (id:180001) has been deleted
lgtm % comments, looking forward to reviewing the followup cl https://codereview.chromium.org/777423004/diff/200001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment.cc (right): https://codereview.chromium.org/777423004/diff/200001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:30: const char kExperimentName[] = "AskToSubmitURLBubble"; Nit: Put this at the top of the namespace, since you can later use it in ShouldShowBubbleExperiment(). https://codereview.chromium.org/777423004/diff/200001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:31: const char kGroupShowBubble[] = "ShowBubble"; You might not need these if you use params. Maybe omit these constants from this CL entirely. https://codereview.chromium.org/777423004/diff/200001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc (right): https://codereview.chromium.org/777423004/diff/200001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc:38: password_manager::urls_collection_experiment::ShouldShowBubble(prefs())); 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.
The CQ bit was checked by melandory@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/777423004/220001
On 2014/12/10 18:46:52, Alexei Svitkine wrote: > lgtm % comments, looking forward to reviewing the followup cl > > https://codereview.chromium.org/777423004/diff/200001/components/password_man... > File > components/password_manager/core/browser/password_manager_url_collection_experiment.cc > (right): > > https://codereview.chromium.org/777423004/diff/200001/components/password_man... > components/password_manager/core/browser/password_manager_url_collection_experiment.cc:30: > const char kExperimentName[] = "AskToSubmitURLBubble"; > Nit: Put this at the top of the namespace, since you can later use it in > ShouldShowBubbleExperiment(). > > https://codereview.chromium.org/777423004/diff/200001/components/password_man... > components/password_manager/core/browser/password_manager_url_collection_experiment.cc:31: > const char kGroupShowBubble[] = "ShowBubble"; > You might not need these if you use params. Maybe omit these constants from this > CL entirely. > > https://codereview.chromium.org/777423004/diff/200001/components/password_man... > File > components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc > (right): > > https://codereview.chromium.org/777423004/diff/200001/components/password_man... > components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc:38: > password_manager::urls_collection_experiment::ShouldShowBubble(prefs())); > 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. Moved some of comments to second CL.
On 2014/12/10 18:46:52, Alexei Svitkine wrote: > lgtm % comments, looking forward to reviewing the followup cl > > https://codereview.chromium.org/777423004/diff/200001/components/password_man... > File > components/password_manager/core/browser/password_manager_url_collection_experiment.cc > (right): > > https://codereview.chromium.org/777423004/diff/200001/components/password_man... > components/password_manager/core/browser/password_manager_url_collection_experiment.cc:30: > const char kExperimentName[] = "AskToSubmitURLBubble"; > Nit: Put this at the top of the namespace, since you can later use it in > ShouldShowBubbleExperiment(). > > https://codereview.chromium.org/777423004/diff/200001/components/password_man... > components/password_manager/core/browser/password_manager_url_collection_experiment.cc:31: > const char kGroupShowBubble[] = "ShowBubble"; > You might not need these if you use params. Maybe omit these constants from this > CL entirely. > > https://codereview.chromium.org/777423004/diff/200001/components/password_man... > File > components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc > (right): > > https://codereview.chromium.org/777423004/diff/200001/components/password_man... > components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc:38: > password_manager::urls_collection_experiment::ShouldShowBubble(prefs())); > 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. Moved some of comments to second CL.
https://codereview.chromium.org/777423004/diff/200001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment.cc (right): https://codereview.chromium.org/777423004/diff/200001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:30: const char kExperimentName[] = "AskToSubmitURLBubble"; On 2014/12/10 18:46:52, Alexei Svitkine wrote: > Nit: Put this at the top of the namespace, since you can later use it in > ShouldShowBubbleExperiment(). Why? It is declared in .h file, so I can use it nevertheless. No matter where definition is. https://codereview.chromium.org/777423004/diff/200001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:31: const char kGroupShowBubble[] = "ShowBubble"; On 2014/12/10 18:46:52, Alexei Svitkine wrote: > You might not need these if you use params. Maybe omit these constants from this > CL entirely. Done. https://codereview.chromium.org/777423004/diff/200001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc (right): https://codereview.chromium.org/777423004/diff/200001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc:38: password_manager::urls_collection_experiment::ShouldShowBubble(prefs())); On 2014/12/10 18:46:52, Alexei Svitkine wrote: > 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. I will add this change also in second CL.
https://codereview.chromium.org/777423004/diff/200001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment.cc (right): https://codereview.chromium.org/777423004/diff/200001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:30: const char kExperimentName[] = "AskToSubmitURLBubble"; On 2014/12/10 22:08:20, melandory wrote: > On 2014/12/10 18:46:52, Alexei Svitkine wrote: > > Nit: Put this at the top of the namespace, since you can later use it in > > ShouldShowBubbleExperiment(). > > Why? It is declared in .h file, so I can use it nevertheless. No matter where > definition is. That doesn't work. In the .h file, you declare it outside the anon namespace, whereas here it is inside it. It's not referring to the same thing. If you try to use the one from the .h file, you'll get a link error....
https://codereview.chromium.org/777423004/diff/200001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment.cc (right): https://codereview.chromium.org/777423004/diff/200001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:30: const char kExperimentName[] = "AskToSubmitURLBubble"; On 2014/12/10 22:13:51, Alexei Svitkine wrote: > On 2014/12/10 22:08:20, melandory wrote: > > On 2014/12/10 18:46:52, Alexei Svitkine wrote: > > > Nit: Put this at the top of the namespace, since you can later use it in > > > ShouldShowBubbleExperiment(). > > > > Why? It is declared in .h file, so I can use it nevertheless. No matter where > > definition is. > > That doesn't work. In the .h file, you declare it outside the anon namespace, > whereas here it is inside it. It's not referring to the same thing. If you try > to use the one from the .h file, you'll get a link error.... It's not inside anon namespace. Anon namespace ends in line 21
https://codereview.chromium.org/777423004/diff/200001/components/password_man... File components/password_manager/core/browser/password_manager_url_collection_experiment.cc (right): https://codereview.chromium.org/777423004/diff/200001/components/password_man... components/password_manager/core/browser/password_manager_url_collection_experiment.cc:30: const char kExperimentName[] = "AskToSubmitURLBubble"; On 2014/12/10 22:18:09, melandory wrote: > On 2014/12/10 22:13:51, Alexei Svitkine wrote: > > On 2014/12/10 22:08:20, melandory wrote: > > > On 2014/12/10 18:46:52, Alexei Svitkine wrote: > > > > Nit: Put this at the top of the namespace, since you can later use it in > > > > ShouldShowBubbleExperiment(). > > > > > > Why? It is declared in .h file, so I can use it nevertheless. No matter > where > > > definition is. > > > > That doesn't work. In the .h file, you declare it outside the anon namespace, > > whereas here it is inside it. It's not referring to the same thing. If you try > > to use the one from the .h file, you'll get a link error.... > > It's not inside anon namespace. Anon namespace ends in line 21 Oops, you're absolutely right! Sorry, ignore my comment then!
Message was sent while issue was closed.
Committed patchset #9 (id:220001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/b1e332c5692df7d594d718ac26e6f1b6cd41e2ea Cr-Commit-Position: refs/heads/master@{#307795} |
