Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
20777006:
Omnibox: Create Bundled Field Trial; Convert SearchHistory trial to it (Closed)
|
Created:
7 years, 5 months ago by Mark P Modified:
7 years, 4 months ago CC:
chromium-reviews, James Su, Alexei Svitkine (slow) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionOmnibox: Create Bundled Field Trial; Convert SearchHistory trial to it
Create one field trial that's intended to rule over all omnibox field
trials. They allows our trials to run correlated. For example, suppose
we have two trials A (with 50-50 groups a1 and a2) and B (with 50-50
groups b1 b2). Currently this yields four conditions users can be in
25% a1-b1, 25% a1-b2, 25% a2-b1, 25% a2-b2.
This would be great, but what if we want to compare a1-b1 and a1-b2 and
a2-b1 but think a2-b2 makes no sense. There's no way to do it with the
current independent field trial framework.
This change creates one field trial that supposed to include all omnibox
field trials that we want to run.
This change converts the SearchHistory field trial to this bundled
field trial.
Obviously, this trials-can-be-dependent property isn't useful until we
add more experiments to this bundled field trial. I'll do that in
another changelist.
This change also gives this new bundled trial the ability to condition
certain experiment on page context. In particular, depending on what
the user is viewing when using the omnibox, the different experiments
can be active. For example, an omnibox on the new tab page can have
different experiments than the omnibox on an arbitrary web page.
Tested using my own variations server with an appropriate config
with various experiment params.
BUG=264062, 264065, 263823
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=215457
Patch Set 1 #Patch Set 2 : now with tests! #Patch Set 3 : refactor search history rule into omnibox_field_trial.cc #Patch Set 4 : rebase #Patch Set 5 : make GetConsequences private for now #
Total comments: 1
Patch Set 6 : make a friend #Patch Set 7 : slightly revise comment #
Total comments: 21
Patch Set 8 : Peter's comments #
Total comments: 2
Patch Set 9 : Alexei's spacing suggestion #Patch Set 10 : Alexei's spacing suggestion #
Total comments: 26
Patch Set 11 : Peter's comments #
Total comments: 2
Patch Set 12 : make context stuff clearer, better comment unit test #
Total comments: 2
Patch Set 13 : revise comment #Patch Set 14 : rebase #
Messages
Total messages: 20 (0 generated)
Peter - Can you take a look at this please? I think the change description is longer than the change itself. :-) asvitkine@ - CCed to you as an FYI, both to see my comment about empty strings and also to see how I plan to use variation params for omnibox experiments. --mark https://codereview.chromium.org/20777006/diff/10001/chrome/browser/omnibox/om... File chrome/browser/omnibox/omnibox_field_trial.cc (right): https://codereview.chromium.org/20777006/diff/10001/chrome/browser/omnibox/om... chrome/browser/omnibox/omnibox_field_trial.cc:255: std::string OmniboxFieldTrial::GetConsequencesOfRuleInPageClassificationContext( asvitkine: This function would be a little shorter if I could simply call GetVariationParamValue() twice, but it can't tell the difference between an empty value and a missing key.
LGTM https://codereview.chromium.org/20777006/diff/16001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/20777006/diff/16001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:528: input_.current_page_classification()); Nit: It seems like both of these members are only checked in a single place. Perhaps it makes sense now to eliminate the members, and just call the field trial methods directly at these two conditional sites. https://codereview.chromium.org/20777006/diff/16001/chrome/browser/omnibox/om... File chrome/browser/omnibox/omnibox_field_trial.cc (right): https://codereview.chromium.org/20777006/diff/16001/chrome/browser/omnibox/om... chrome/browser/omnibox/omnibox_field_trial.cc:247: // key=6:SearchHistory Nit: If your variations syntax is not already set in stone, "<rule>:<classification>" makes more sense than the reverse. https://codereview.chromium.org/20777006/diff/16001/chrome/browser/omnibox/om... chrome/browser/omnibox/omnibox_field_trial.cc:258: std::map<std::string, std::string> params; Nit: Perhaps where GetVariationParams() is declared we should have a typedef for this type. https://codereview.chromium.org/20777006/diff/16001/chrome/browser/omnibox/om... chrome/browser/omnibox/omnibox_field_trial.cc:261: return ""; Nit: std::string() (2 places) https://codereview.chromium.org/20777006/diff/16001/chrome/browser/omnibox/om... chrome/browser/omnibox/omnibox_field_trial.cc:263: // Lookup rule in this exact context. Nit: Lookup -> Look up (first is a noun, second is a verb) (2 places) https://codereview.chromium.org/20777006/diff/16001/chrome/browser/omnibox/om... chrome/browser/omnibox/omnibox_field_trial.cc:271: if (it != params.end()) Nit: Shorter: return (it == params.end()) ? std::string() : it->second; https://codereview.chromium.org/20777006/diff/16001/chrome/browser/omnibox/om... File chrome/browser/omnibox/omnibox_field_trial.h (right): https://codereview.chromium.org/20777006/diff/16001/chrome/browser/omnibox/om... chrome/browser/omnibox/omnibox_field_trial.h:109: // Returns true if the user is in the experiment group that, in the Nit: last "in" -> "given"? (2 places) https://codereview.chromium.org/20777006/diff/16001/chrome/browser/omnibox/om... chrome/browser/omnibox/omnibox_field_trial.h:125: // Returns the "consequences" for the |rule| that applies in the context Nit: Putting "consequences" in quotes and never defining what it means leaves the reader still unsure how this function is actually used. I might hoist most of the comments from the .cc file to here, since that background is all useful in the .h file, rather than being low-level implementation detail that's only of interest in deciphering the implementation. I'd also probably use "GetRuleValue" or "GetValueOfRule" or "GetValueForRule" instead of using the word "consequences" anywhere. https://codereview.chromium.org/20777006/diff/16001/chrome/browser/omnibox/om... File chrome/browser/omnibox/omnibox_field_trial_unittest.cc (right): https://codereview.chromium.org/20777006/diff/16001/chrome/browser/omnibox/om... chrome/browser/omnibox/omnibox_field_trial_unittest.cc:137: // defined in omnibox_field_trial.cc. Nit: Should we declare this in a .h somewhere, then?
https://codereview.chromium.org/20777006/diff/16001/chrome/browser/omnibox/om... File chrome/browser/omnibox/omnibox_field_trial_unittest.cc (right): https://codereview.chromium.org/20777006/diff/16001/chrome/browser/omnibox/om... chrome/browser/omnibox/omnibox_field_trial_unittest.cc:19: ResetFieldTrialList(); Hmm, I think this keeps the field trial list persistent between all the cases in your test, whereas if you do it in either a "virtual void SetUp() OVERRIDE" method or in the ctor, you can have a field trial list per test case, so different tests don't contaminate each other. In fact, you can simplify this by just having a base::FieldTrialList field_trial_list_; instance variable and initialize it in the ctor via field_trial_list_(new metrics::SHA1EntropyProvider("foo")) instead of having the more complicated ResetFieldTrialList() logic.
please take another look, just to make sure you're comfortable with my responses. --mark https://codereview.chromium.org/20777006/diff/16001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/20777006/diff/16001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:528: input_.current_page_classification()); On 2013/07/31 20:14:09, Peter Kasting wrote: > Nit: It seems like both of these members are only checked in a single place. > Perhaps it makes sense now to eliminate the members, and just call the field > trial methods directly at these two conditional sites. Good idea. Done. https://codereview.chromium.org/20777006/diff/16001/chrome/browser/omnibox/om... File chrome/browser/omnibox/omnibox_field_trial.cc (right): https://codereview.chromium.org/20777006/diff/16001/chrome/browser/omnibox/om... chrome/browser/omnibox/omnibox_field_trial.cc:247: // key=6:SearchHistory On 2013/07/31 20:14:09, Peter Kasting wrote: > Nit: If your variations syntax is not already set in stone, > "<rule>:<classification>" makes more sense than the reverse. Okay. Changed. Also reversed order of GetConsequencesOfRuleInPageClassificationContext https://codereview.chromium.org/20777006/diff/16001/chrome/browser/omnibox/om... chrome/browser/omnibox/omnibox_field_trial.cc:258: std::map<std::string, std::string> params; On 2013/07/31 20:14:09, Peter Kasting wrote: > Nit: Perhaps where GetVariationParams() is declared we should have a typedef for > this type. I tried to get this to work but (oddly) couldn't get it to work (compile errors in variations_seed_processor that I couldn't grok). It's not worth me spending more tha 30 minutes on so I stopped. https://codereview.chromium.org/20777006/diff/16001/chrome/browser/omnibox/om... chrome/browser/omnibox/omnibox_field_trial.cc:261: return ""; On 2013/07/31 20:14:09, Peter Kasting wrote: > Nit: std::string() (2 places) Done. https://codereview.chromium.org/20777006/diff/16001/chrome/browser/omnibox/om... chrome/browser/omnibox/omnibox_field_trial.cc:263: // Lookup rule in this exact context. On 2013/07/31 20:14:09, Peter Kasting wrote: > Nit: Lookup -> Look up (first is a noun, second is a verb) (2 places) Done. https://codereview.chromium.org/20777006/diff/16001/chrome/browser/omnibox/om... chrome/browser/omnibox/omnibox_field_trial.cc:271: if (it != params.end()) On 2013/07/31 20:14:09, Peter Kasting wrote: > Nit: Shorter: > > return (it == params.end()) ? std::string() : it->second; Done. https://codereview.chromium.org/20777006/diff/16001/chrome/browser/omnibox/om... File chrome/browser/omnibox/omnibox_field_trial.h (right): https://codereview.chromium.org/20777006/diff/16001/chrome/browser/omnibox/om... chrome/browser/omnibox/omnibox_field_trial.h:109: // Returns true if the user is in the experiment group that, in the On 2013/07/31 20:14:09, Peter Kasting wrote: > Nit: last "in" -> "given"? (2 places) Done. https://codereview.chromium.org/20777006/diff/16001/chrome/browser/omnibox/om... chrome/browser/omnibox/omnibox_field_trial.h:125: // Returns the "consequences" for the |rule| that applies in the context On 2013/07/31 20:14:09, Peter Kasting wrote: > Nit: Putting "consequences" in quotes and never defining what it means leaves > the reader still unsure how this function is actually used. I might hoist most > of the comments from the .cc file to here, since that background is all useful > in the .h file, rather than being low-level implementation detail that's only of > interest in deciphering the implementation. I'd also probably use > "GetRuleValue" or "GetValueOfRule" or "GetValueForRule" instead of using the > word "consequences" anywhere. I think with the newly rewritten comment and removing the reference to consequences things read pretty well. I don't feel the need to drag the long formatting of rules comment into the header. https://codereview.chromium.org/20777006/diff/16001/chrome/browser/omnibox/om... File chrome/browser/omnibox/omnibox_field_trial_unittest.cc (right): https://codereview.chromium.org/20777006/diff/16001/chrome/browser/omnibox/om... chrome/browser/omnibox/omnibox_field_trial_unittest.cc:19: ResetFieldTrialList(); On 2013/07/31 20:27:46, Alexei Svitkine wrote: > Hmm, I think this keeps the field trial list persistent between all the cases in > your test, whereas if you do it in either a "virtual void SetUp() OVERRIDE" > method or in the ctor, you can have a field trial list per test case, so > different tests don't contaminate each other. > > In fact, you can simplify this by just having a base::FieldTrialList > field_trial_list_; instance variable and initialize it in the ctor via > field_trial_list_(new metrics::SHA1EntropyProvider("foo")) instead of having the > more complicated ResetFieldTrialList() logic. These tests pass as-is. (This isn't the changelist I discuss with you over e-mail.) In the change that adds another test, I needed to (and did) move the field_trial_list_ to the constructor as you suggest. I'd like to leave this as-is and fix the test structure in the same change that requires the correct test structure to function correctly. https://codereview.chromium.org/20777006/diff/16001/chrome/browser/omnibox/om... chrome/browser/omnibox/omnibox_field_trial_unittest.cc:137: // defined in omnibox_field_trial.cc. On 2013/07/31 20:14:09, Peter Kasting wrote: > Nit: Should we declare this in a .h somewhere, then? I'd rather not have someone reading the .h have to think about this implementation detail, even if it's in a private section and listed as a friend.
lgtm with a nit https://codereview.chromium.org/20777006/diff/16001/chrome/browser/omnibox/om... File chrome/browser/omnibox/omnibox_field_trial_unittest.cc (right): https://codereview.chromium.org/20777006/diff/16001/chrome/browser/omnibox/om... chrome/browser/omnibox/omnibox_field_trial_unittest.cc:137: // defined in omnibox_field_trial.cc. On 2013/07/31 21:59:28, Mark P wrote: > On 2013/07/31 20:14:09, Peter Kasting wrote: > > Nit: Should we declare this in a .h somewhere, then? > > I'd rather not have someone reading the .h have to think about this > implementation detail, even if it's in a private section and listed as a friend. Fair enough, SGTM. https://codereview.chromium.org/20777006/diff/24001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/20777006/diff/24001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:630: input_.current_page_classification())) { Nit: Indent 4 more.
https://codereview.chromium.org/20777006/diff/24001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/20777006/diff/24001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:630: input_.current_page_classification())) { On 2013/08/01 15:25:35, Alexei Svitkine wrote: > Nit: Indent 4 more. Done.
Peter, Do you want to re-glance at this change or not? (You asked for some simple, straightforward things, though these things were substantial.) thanks, mark
LGTM with a bunch of more minor suggestions. https://codereview.chromium.org/20777006/diff/34001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/20777006/diff/34001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:630: input_.current_page_classification())) { Nit: Indent 4, not 8. I would remove {} too since the autocomplete code generally avoids these even on multiline conditions as long as the body is one line. https://codereview.chromium.org/20777006/diff/34001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/20777006/diff/34001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.h:441: // the relevance curve slightly downward. Nit: You might want a clearer explanation of these last two params (and maybe even different names for them). Basically, you'd want to explain that we have two scoring curves, and why; then if |prevent_inline_autocomplete| is true, note that we force all searches to use the second curve, and the last arg is ignored, while if |prevent_inline_autocomplete| is false but |prevent_search_history_inlining| is true, we allow two scoring curves to be used, but lower the top value of the first curve. Writing all that out makes me think that maybe this whole "2-curve, 2-param" combination is more complex than it really should be. After all, if it's hard to explain... https://codereview.chromium.org/20777006/diff/34001/chrome/browser/omnibox/om... File chrome/browser/omnibox/omnibox_field_trial.cc (right): https://codereview.chromium.org/20777006/diff/34001/chrome/browser/omnibox/om... chrome/browser/omnibox/omnibox_field_trial.cc:236: // Background and implementation details: Honestly, I still think this comment, as-is, is clearer than the comment you currently have in the .h file. I'd still probably just replace that comment with this one. https://codereview.chromium.org/20777006/diff/34001/chrome/browser/omnibox/om... chrome/browser/omnibox/omnibox_field_trial.cc:245: // this rule applies in all contexts. Nit: "in all contexts" -> "for all classifications" (The theme of my suggested function rename and the comments here and below is that I'm trying to move away from using the word "context", as I think it adds verbosity but not clarity.) https://codereview.chromium.org/20777006/diff/34001/chrome/browser/omnibox/om... chrome/browser/omnibox/omnibox_field_trial.cc:263: // Look up rule in this exact context. Nit: "in this exact context" -> "for the provided classification" https://codereview.chromium.org/20777006/diff/34001/chrome/browser/omnibox/om... chrome/browser/omnibox/omnibox_field_trial.cc:269: // Look up rule in the global context. Nit: Maybe "See if we have a fallback rule for all classifications." https://codereview.chromium.org/20777006/diff/34001/chrome/browser/omnibox/om... File chrome/browser/omnibox/omnibox_field_trial.h (right): https://codereview.chromium.org/20777006/diff/34001/chrome/browser/omnibox/om... chrome/browser/omnibox/omnibox_field_trial.h:131: static std::string GetValueOfRuleInPageClassificationContext( Nit: Slightly shorter might be GetValueForRuleWithPageClassification() or GetValueOfRuleForPageClassification() https://codereview.chromium.org/20777006/diff/34001/chrome/browser/omnibox/om... chrome/browser/omnibox/omnibox_field_trial.h:133: AutocompleteInput::PageClassification current_page_classification); Nit: "current" probably not necessary (this function doesn't really care if the provided classification is in any sense "current") https://codereview.chromium.org/20777006/diff/34001/chrome/browser/omnibox/om... File chrome/browser/omnibox/omnibox_field_trial_unittest.cc (right): https://codereview.chromium.org/20777006/diff/34001/chrome/browser/omnibox/om... chrome/browser/omnibox/omnibox_field_trial_unittest.cc:140: params["rule1:1"] = "rule1-1-value"; Nit: Might want EOL comments like "// NEW_TAB_PAGE" to clarify what "1"/"3"/"4" correspond to https://codereview.chromium.org/20777006/diff/34001/chrome/browser/omnibox/om... chrome/browser/omnibox/omnibox_field_trial_unittest.cc:151: EXPECT_EQ("rule1-1-value", Tiny nit: All lines of args at the same nesting level should be aligned (so, you'd probably need to put this alone on a subsequent, indented-4 line). https://codereview.chromium.org/20777006/diff/34001/chrome/browser/omnibox/om... chrome/browser/omnibox/omnibox_field_trial_unittest.cc:153: "rule1", AutocompleteInput::NEW_TAB_PAGE)); // exact match Nit: Rather than these EOL comments, how about block comments like: // Test exact matches for particular rule/classification pairs. EXPECT... ... // Test mismatches for rules with global "wildcard" fallbacks. ... // Test mismatches for rules without global fallbacks. ... // Test nonexistent rules. ...
Some agreement, some pushback. Please take another look. --mark https://codereview.chromium.org/20777006/diff/34001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/20777006/diff/34001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:630: input_.current_page_classification())) { On 2013/08/01 22:44:48, Peter Kasting wrote: > Nit: Indent 4, not 8. I would remove {} too since the autocomplete code > generally avoids these even on multiline conditions as long as the body is one > line. Hey, I previously indented 4 and Alexei told me to change it. Put back. :-) Also dropped the {} https://codereview.chromium.org/20777006/diff/34001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/20777006/diff/34001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.h:441: // the relevance curve slightly downward. On 2013/08/01 22:44:48, Peter Kasting wrote: > Nit: You might want a clearer explanation of these last two params (and maybe > even different names for them). Basically, you'd want to explain that we have > two scoring curves, and why; then if |prevent_inline_autocomplete| is true, note > that we force all searches to use the second curve, and the last arg is ignored, > while if |prevent_inline_autocomplete| is false but > |prevent_search_history_inlining| is true, we allow two scoring curves to be > used, but lower the top value of the first curve. > > Writing all that out makes me think that maybe this whole "2-curve, 2-param" > combination is more complex than it really should be. After all, if it's hard > to explain... With judicious renaming, I think I managed to rewrite the comment in a much clearer way. I agree the old way with two "prevent" variables was ugly. https://codereview.chromium.org/20777006/diff/34001/chrome/browser/omnibox/om... File chrome/browser/omnibox/omnibox_field_trial.cc (right): https://codereview.chromium.org/20777006/diff/34001/chrome/browser/omnibox/om... chrome/browser/omnibox/omnibox_field_trial.cc:236: // Background and implementation details: On 2013/08/01 22:44:48, Peter Kasting wrote: > Honestly, I still think this comment, as-is, is clearer than the comment you > currently have in the .h file. I'd still probably just replace that comment > with this one. Added a new opening paragraph to the .h that imports some of the explanation found here but not the implementation details (format of key-value pairs). https://codereview.chromium.org/20777006/diff/34001/chrome/browser/omnibox/om... chrome/browser/omnibox/omnibox_field_trial.cc:245: // this rule applies in all contexts. On 2013/08/01 22:44:48, Peter Kasting wrote: > Nit: "in all contexts" -> "for all classifications" > > (The theme of my suggested function rename and the comments here and below is > that I'm trying to move away from using the word "context", as I think it adds > verbosity but not clarity.) see other comment about context. https://codereview.chromium.org/20777006/diff/34001/chrome/browser/omnibox/om... chrome/browser/omnibox/omnibox_field_trial.cc:263: // Look up rule in this exact context. On 2013/08/01 22:44:48, Peter Kasting wrote: > Nit: "in this exact context" -> "for the provided classification" see other comment about context. https://codereview.chromium.org/20777006/diff/34001/chrome/browser/omnibox/om... chrome/browser/omnibox/omnibox_field_trial.cc:269: // Look up rule in the global context. On 2013/08/01 22:44:48, Peter Kasting wrote: > Nit: Maybe "See if we have a fallback rule for all classifications." see other comment about context. https://codereview.chromium.org/20777006/diff/34001/chrome/browser/omnibox/om... File chrome/browser/omnibox/omnibox_field_trial.h (right): https://codereview.chromium.org/20777006/diff/34001/chrome/browser/omnibox/om... chrome/browser/omnibox/omnibox_field_trial.h:131: static std::string GetValueOfRuleInPageClassificationContext( On 2013/08/01 22:44:48, Peter Kasting wrote: > Nit: Slightly shorter might be > > GetValueForRuleWithPageClassification() > > or > > GetValueOfRuleForPageClassification() They are shorter. I think the word context definitely adds something here and I'd rather not drop it. Left as is. Addendum after reading your other context comments: I think context is an important notion. For instance, I can easily see adding an input_type context as well. I can easily imagine contexts overlapping and having priorities between them. I think it really helps to think of the context as a bigger notion. Indeed, I'd rather drop PageClassification from this function name rather than Context. (the former can be inferred from the parameter types). https://codereview.chromium.org/20777006/diff/34001/chrome/browser/omnibox/om... chrome/browser/omnibox/omnibox_field_trial.h:133: AutocompleteInput::PageClassification current_page_classification); On 2013/08/01 22:44:48, Peter Kasting wrote: > Nit: "current" probably not necessary (this function doesn't really care if the > provided classification is in any sense "current") Good point. Revised. https://codereview.chromium.org/20777006/diff/34001/chrome/browser/omnibox/om... File chrome/browser/omnibox/omnibox_field_trial_unittest.cc (right): https://codereview.chromium.org/20777006/diff/34001/chrome/browser/omnibox/om... chrome/browser/omnibox/omnibox_field_trial_unittest.cc:140: params["rule1:1"] = "rule1-1-value"; On 2013/08/01 22:44:48, Peter Kasting wrote: > Nit: Might want EOL comments like "// NEW_TAB_PAGE" to clarify what "1"/"3"/"4" > correspond to Done. https://codereview.chromium.org/20777006/diff/34001/chrome/browser/omnibox/om... chrome/browser/omnibox/omnibox_field_trial_unittest.cc:151: EXPECT_EQ("rule1-1-value", On 2013/08/01 22:44:48, Peter Kasting wrote: > Tiny nit: All lines of args at the same nesting level should be aligned (so, > you'd probably need to put this alone on a subsequent, indented-4 line). Done. https://codereview.chromium.org/20777006/diff/34001/chrome/browser/omnibox/om... chrome/browser/omnibox/omnibox_field_trial_unittest.cc:153: "rule1", AutocompleteInput::NEW_TAB_PAGE)); // exact match On 2013/08/01 22:44:48, Peter Kasting wrote: > Nit: Rather than these EOL comments, how about block comments like: > > // Test exact matches for particular rule/classification pairs. > EXPECT... > ... > > // Test mismatches for rules with global "wildcard" fallbacks. > ... > > // Test mismatches for rules without global fallbacks. > ... > > // Test nonexistent rules. > ... I think this test is more easily understood by looking at each set of cases about a particular rule. That's the current order. Otherwise, for instance, to examine all tests in each of your particular categories, one would have to keep all rules in one's head. This way one only has to keep one set of rules in one's head at once.
(Haven't re-reviewed) https://codereview.chromium.org/20777006/diff/34001/chrome/browser/omnibox/om... File chrome/browser/omnibox/omnibox_field_trial.h (right): https://codereview.chromium.org/20777006/diff/34001/chrome/browser/omnibox/om... chrome/browser/omnibox/omnibox_field_trial.h:131: static std::string GetValueOfRuleInPageClassificationContext( On 2013/08/02 00:44:36, Mark P wrote: > Addendum after reading your other context comments: I think context is an > important notion. For instance, I can easily see adding an input_type context > as well. I can easily imagine contexts overlapping and having priorities > between them. I think it really helps to think of the context as a bigger > notion. Indeed, I'd rather drop PageClassification from this function name > rather than Context. (the former can be inferred from the parameter types). I partly react to this with "then update the name and comments to reflect that larger context at the time you actually add such a larger context; but for now you don't have it". But if you're really committed to this, then I think you should go ahead and drop "PageClassification" right now, and explicitly note in your comments that while you only have one piece of context at the moment, you're planning to add more. But if you're not planning to add more and that comment is purely hypothetical, then as I said, I would drop the notion of context for now, because as a reader it's really confusing. https://codereview.chromium.org/20777006/diff/34001/chrome/browser/omnibox/om... File chrome/browser/omnibox/omnibox_field_trial_unittest.cc (right): https://codereview.chromium.org/20777006/diff/34001/chrome/browser/omnibox/om... chrome/browser/omnibox/omnibox_field_trial_unittest.cc:153: "rule1", AutocompleteInput::NEW_TAB_PAGE)); // exact match On 2013/08/02 00:44:36, Mark P wrote: > On 2013/08/01 22:44:48, Peter Kasting wrote: > > Nit: Rather than these EOL comments, how about block comments like: > > > > // Test exact matches for particular rule/classification pairs. > > EXPECT... > > ... > > > > // Test mismatches for rules with global "wildcard" fallbacks. > > ... > > > > // Test mismatches for rules without global fallbacks. > > ... > > > > // Test nonexistent rules. > > ... > > I think this test is more easily understood by looking at each set of cases > about a particular rule. OK, then break the test into blocks that have to deal with each rule, and put comments like "// Tests for rule 1." or something atop each block. I'm not as sold as you on this ordering, but at least then it's not an intimidating wall of verbiage that all looks the same, which is how the function reads today.
https://codereview.chromium.org/20777006/diff/43001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/20777006/diff/43001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:630: input_.current_page_classification())) Peter, can you explain why this should be indented differently then e.g. line 1205? Or even e.g. line 287. I thought the rule was to indent 4 more whenever you wrap, unless aligning with the same type of thing on the previous line.
https://codereview.chromium.org/20777006/diff/43001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/20777006/diff/43001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:630: input_.current_page_classification())) On 2013/08/02 15:25:34, Alexei Svitkine wrote: > Peter, can you explain why this should be indented differently then e.g. line > 1205? Or even e.g. line 287. > > I thought the rule was to indent 4 more whenever you wrap, unless aligning with > the same type of thing on the previous line. It's not indented differently. You indent 4 from the beginning of the previous line, not from the beginning of the subexpression. Also, you don't necessarily indent an additional 4 each time you wrap, you indent only when the start of the previous line was at a different subexpression level. For example, "a + b + c", with long a/b/c, should be: a + b + c; Not a + b + c;
https://codereview.chromium.org/20777006/diff/34001/chrome/browser/omnibox/om... File chrome/browser/omnibox/omnibox_field_trial.h (right): https://codereview.chromium.org/20777006/diff/34001/chrome/browser/omnibox/om... chrome/browser/omnibox/omnibox_field_trial.h:131: static std::string GetValueOfRuleInPageClassificationContext( On 2013/08/02 00:49:33, Peter Kasting wrote: > On 2013/08/02 00:44:36, Mark P wrote: > > Addendum after reading your other context comments: I think context is an > > important notion. For instance, I can easily see adding an input_type context > > as well. I can easily imagine contexts overlapping and having priorities > > between them. I think it really helps to think of the context as a bigger > > notion. Indeed, I'd rather drop PageClassification from this function name > > rather than Context. (the former can be inferred from the parameter types). > > I partly react to this with "then update the name and comments to reflect that > larger context at the time you actually add such a larger context; but for now > you don't have it". But if you're really committed to this, then I think you > should go ahead and drop "PageClassification" right now, and explicitly note in > your comments that while you only have one piece of context at the moment, > you're planning to add more. > > But if you're not planning to add more and that comment is purely hypothetical, > then as I said, I would drop the notion of context for now, because as a reader > it's really confusing. Next week I'm going to add another piece of context (whether Instant Extended is enabled), so I revised the comment and function name to put less emphasis on page classification. https://codereview.chromium.org/20777006/diff/34001/chrome/browser/omnibox/om... File chrome/browser/omnibox/omnibox_field_trial_unittest.cc (right): https://codereview.chromium.org/20777006/diff/34001/chrome/browser/omnibox/om... chrome/browser/omnibox/omnibox_field_trial_unittest.cc:153: "rule1", AutocompleteInput::NEW_TAB_PAGE)); // exact match On 2013/08/02 00:49:33, Peter Kasting wrote: > On 2013/08/02 00:44:36, Mark P wrote: > > On 2013/08/01 22:44:48, Peter Kasting wrote: > > > Nit: Rather than these EOL comments, how about block comments like: > > > > > > // Test exact matches for particular rule/classification pairs. > > > EXPECT... > > > ... > > > > > > // Test mismatches for rules with global "wildcard" fallbacks. > > > ... > > > > > > // Test mismatches for rules without global fallbacks. > > > ... > > > > > > // Test nonexistent rules. > > > ... > > > > I think this test is more easily understood by looking at each set of cases > > about a particular rule. > > OK, then break the test into blocks that have to deal with each rule, and put > comments like "// Tests for rule 1." or something atop each block. > > I'm not as sold as you on this ordering, but at least then it's not an > intimidating wall of verbiage that all looks the same, which is how the function > reads today. Done.
LGTM https://codereview.chromium.org/20777006/diff/49001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/20777006/diff/49001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.h:442: // allowed to exceed 1300. Nit: This last sentence can be read to mean the opposite of its intent; how about "When using the aggressive method, scores may exceed 1300 unless |prevent_search_history_inlining| is set."
https://codereview.chromium.org/20777006/diff/49001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/20777006/diff/49001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.h:442: // allowed to exceed 1300. On 2013/08/02 19:57:41, Peter Kasting wrote: > Nit: This last sentence can be read to mean the opposite of its intent; how > about "When using the aggressive method, scores may exceed 1300 unless > |prevent_search_history_inlining| is set." Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mpearson@chromium.org/20777006/46002
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mpearson@chromium.org/20777006/70001
Message was sent while issue was closed.
Change committed as 215457 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
