Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
22031002:
Omnibox: Create DemoteByType Experiment (Closed)
|
Created:
7 years, 4 months ago by Mark P Modified:
7 years, 4 months ago CC:
chromium-reviews, James Su, Ilya Sherman, jar (doing other things), asvitkine+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionOmnibox: Create DemoteByType Experiment
This experiment, which runs as part of the bundled omnibox field trial,
uses the field trial parameters to demote results of particular types
(e.g., HISTORY_TITLE results), possibly depending on context (e.g.,
search results page doing search term replacement).
It also improves the testing framework for field trial parameters,
allowing them to be cleared between consecutive TEST_F()s.
Tested via unit tests and by setting up my local variations server with
some parameters and making sure results can be demoted or
omitted.
BUG=264066
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=216633
Patch Set 1 #Patch Set 2 : come cleanup, getting proper function names, etc. #
Total comments: 30
Patch Set 3 : rebase #Patch Set 4 : snapshot for Alexei #Patch Set 5 : Peter's comments and Alexei's improvements. #
Total comments: 9
Patch Set 6 : resolve #
Total comments: 4
Patch Set 7 : string_split.cc #Patch Set 8 : Peter + Alexei comments #
Total comments: 4
Patch Set 9 : Darin's suggested renames #Patch Set 10 : finish strings typedef rename #Patch Set 11 : fix include that caused android build failure #Messages
Total messages: 28 (0 generated)
Hi guys, Peter - can you review the autocomplete/... and omnibox/... files? Alexei - can you review the variations_util files and the field trial testing structure in omnibox_field_trial_unittest.cc? thanks, mark
https://codereview.chromium.org/22031002/diff/3001/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete_result_unittest.cc (right): https://codereview.chromium.org/22031002/diff/3001/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_result_unittest.cc:39: if (field_trial_list_) Use a scoped_ptr here too (same as my comment for the other test). https://codereview.chromium.org/22031002/diff/3001/chrome/browser/omnibox/omn... File chrome/browser/omnibox/omnibox_field_trial_unittest.cc (right): https://codereview.chromium.org/22031002/diff/3001/chrome/browser/omnibox/omn... chrome/browser/omnibox/omnibox_field_trial_unittest.cc:21: delete field_trial_list_; If you make field_trial_list_ a non-static scoped_ptr, you can delete this method and also simplify ResetFieldTrialList below by just calling .reset() on it with the new instance. (Also remove static from ResetFieldTrialList in that case.) https://codereview.chromium.org/22031002/diff/3001/chrome/common/metrics/vari... File chrome/common/metrics/variations/variations_util.cc (right): https://codereview.chromium.org/22031002/diff/3001/chrome/common/metrics/vari... chrome/common/metrics/variations/variations_util.cc:148: void ClearAllParamsForTesting() { You will need to rebase this on top of http://crrev.com/r215644 which moved this code to a different file.
https://codereview.chromium.org/22031002/diff/3001/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete_result_unittest.cc (right): https://codereview.chromium.org/22031002/diff/3001/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_result_unittest.cc:39: if (field_trial_list_) On 2013/08/05 18:33:11, Alexei Svitkine wrote: > Use a scoped_ptr here too (same as my comment for the other test). Didn't try; see other comment. https://codereview.chromium.org/22031002/diff/3001/chrome/browser/omnibox/omn... File chrome/browser/omnibox/omnibox_field_trial_unittest.cc (right): https://codereview.chromium.org/22031002/diff/3001/chrome/browser/omnibox/omn... chrome/browser/omnibox/omnibox_field_trial_unittest.cc:21: delete field_trial_list_; On 2013/08/05 18:33:11, Alexei Svitkine wrote: > If you make field_trial_list_ a non-static scoped_ptr, you can delete this > method and also simplify ResetFieldTrialList below by just calling .reset() on > it with the new instance. (Also remove static from ResetFieldTrialList in that > case.) If I make the changes you suggest, I get a failure: [30420:30420:0805/144359:958398008167:FATAL:field_trial.cc(213)] Check failed: !global_. https://codereview.chromium.org/22031002/diff/3001/chrome/common/metrics/vari... File chrome/common/metrics/variations/variations_util.cc (right): https://codereview.chromium.org/22031002/diff/3001/chrome/common/metrics/vari... chrome/common/metrics/variations/variations_util.cc:148: void ClearAllParamsForTesting() { On 2013/08/05 18:33:11, Alexei Svitkine wrote: > You will need to rebase this on top of http://crrev.com/r215644 which moved this > code to a different file. Done.
https://codereview.chromium.org/22031002/diff/3001/chrome/browser/omnibox/omn... File chrome/browser/omnibox/omnibox_field_trial_unittest.cc (right): https://codereview.chromium.org/22031002/diff/3001/chrome/browser/omnibox/omn... chrome/browser/omnibox/omnibox_field_trial_unittest.cc:21: delete field_trial_list_; On 2013/08/05 22:11:41, Mark P wrote: > On 2013/08/05 18:33:11, Alexei Svitkine wrote: > > If you make field_trial_list_ a non-static scoped_ptr, you can delete this > > method and also simplify ResetFieldTrialList below by just calling .reset() on > > it with the new instance. (Also remove static from ResetFieldTrialList in that > > case.) > > If I make the changes you suggest, I get a failure: > [30420:30420:0805/144359:958398008167:FATAL:field_trial.cc(213)] Check failed: > !global_. Hmm, that should not be happening if you're actually creating the FieldTrialList instance. i.e. if you still call ResetFieldTrialList() from the constructor and in that function, you do: field_trial_list_.reset( new base::FieldTrialList(new metrics::SHA1EntropyProvider("foo"))); If you're doing that and that's still happening, does it give you stack trace?
LGTM https://codereview.chromium.org/22031002/diff/3001/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete_result.cc (right): https://codereview.chromium.org/22031002/diff/3001/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_result.cc:18: // that allows match of particular types to be demoted in AutocompleteResult. Nit: match -> matches? AutocompleteResult -> AutocompleteResult::SortAndCull()? https://codereview.chromium.org/22031002/diff/3001/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_result.cc:45: match.relevance * demotions_by_type_[match.type] : match.relevance; Nit: How about: OmniboxFieldTrial::DemotionMultiplierByType::const_iterator demotion = demotions_by_type_.find(match.type); return (demotion == demotions_by_type_.end()) ? match.relevance : (match.relevance * *demotion)); This is longer, but it avoids the double-lookup (probably no meaningful perf impact, but if we've got the iterator anyway...) https://codereview.chromium.org/22031002/diff/3001/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_result.cc:158: for (num_matches = 0u; (num_matches < max_num_matches) && Nit: Simpler and (to me) clearer: matches_.resize(std::count_if(matches_.begin(), matches_.begin() + max_num_matches, comparing_object.GetDemotedRelevance)); (You might have to fiddle with some adapters if the compiler complains) https://codereview.chromium.org/22031002/diff/3001/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete_result.h (right): https://codereview.chromium.org/22031002/diff/3001/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_result.h:139: void AddMatch(const AutocompleteInput& input, I think you should just pass the actual PageClassification here instead of the whole Input (which you don't need). (2 places) https://codereview.chromium.org/22031002/diff/3001/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete_result_unittest.cc (right): https://codereview.chromium.org/22031002/diff/3001/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_result_unittest.cc:39: if (field_trial_list_) On 2013/08/05 22:11:41, Mark P wrote: > On 2013/08/05 18:33:11, Alexei Svitkine wrote: > > Use a scoped_ptr here too (same as my comment for the other test). > > Didn't try; see other comment. It seems worth fixing. This manual deletion stuff kinda sucks. https://codereview.chromium.org/22031002/diff/3001/chrome/browser/omnibox/omn... File chrome/browser/omnibox/omnibox_field_trial.cc (right): https://codereview.chromium.org/22031002/diff/3001/chrome/browser/omnibox/omn... chrome/browser/omnibox/omnibox_field_trial.cc:251: std::vector<std::pair<std::string, std::string> > kv_pairs; Nit: This suggests that string_split.h should have a typedef for this https://codereview.chromium.org/22031002/diff/3001/chrome/browser/omnibox/omn... File chrome/browser/omnibox/omnibox_field_trial.h (right): https://codereview.chromium.org/22031002/diff/3001/chrome/browser/omnibox/omn... chrome/browser/omnibox/omnibox_field_trial.h:23: typedef std::map<AutocompleteMatchType::Type, float> DemotionMultiplierByType; Nit: "DemotionMultipliers" seems pretty clear too, and shorter. https://codereview.chromium.org/22031002/diff/3001/chrome/browser/omnibox/omn... chrome/browser/omnibox/omnibox_field_trial.h:134: // of certain types of matches, populate the |demotions_by_type| map Nit: populate -> populates https://codereview.chromium.org/22031002/diff/3001/chrome/browser/omnibox/omn... chrome/browser/omnibox/omnibox_field_trial.h:135: // appropriately. Otherwise, clear |demotions_by_type|. Nit: clear -> clears https://codereview.chromium.org/22031002/diff/3001/chrome/browser/omnibox/omn... File chrome/browser/omnibox/omnibox_field_trial_unittest.cc (right): https://codereview.chromium.org/22031002/diff/3001/chrome/browser/omnibox/omn... chrome/browser/omnibox/omnibox_field_trial_unittest.cc:154: demotions_by_type[AutocompleteMatchType::HISTORY_TITLE]); I think this expectation will still pass even if the code is broken, because operator[] will add a value if it doesn't already exist in the map (and 0 would be the default value). You might want to use find() instead of [] for these tests (perhaps via a small helper function to reduce verbosity?). Nit: All lines of args must be aligned.
This comment is for Alexei. I'll reply to Peter's comments shortly. --mark https://codereview.chromium.org/22031002/diff/3001/chrome/browser/omnibox/omn... File chrome/browser/omnibox/omnibox_field_trial_unittest.cc (right): https://codereview.chromium.org/22031002/diff/3001/chrome/browser/omnibox/omn... chrome/browser/omnibox/omnibox_field_trial_unittest.cc:21: delete field_trial_list_; On 2013/08/06 18:20:41, Alexei Svitkine wrote: > On 2013/08/05 22:11:41, Mark P wrote: > > On 2013/08/05 18:33:11, Alexei Svitkine wrote: > > > If you make field_trial_list_ a non-static scoped_ptr, you can delete this > > > method and also simplify ResetFieldTrialList below by just calling .reset() > on > > > it with the new instance. (Also remove static from ResetFieldTrialList in > that > > > case.) > > > > If I make the changes you suggest, I get a failure: > > [30420:30420:0805/144359:958398008167:FATAL:field_trial.cc(213)] Check failed: > > !global_. > > Hmm, that should not be happening if you're actually creating the FieldTrialList > instance. > > i.e. if you still call ResetFieldTrialList() from the constructor and in that > function, you do: > > field_trial_list_.reset( > new base::FieldTrialList(new metrics::SHA1EntropyProvider("foo"))); > > If you're doing that and that's still happening, does it give you stack trace? Stack trace below. I put the code I used to generate this stack trace on the patch set labeled "snapshot for Alexei". ./out/Debug/unit_tests --gtest_filter=OmniboxF*.* Note: Google Test filter = OmniboxF*.* [==========] Running 4 tests from 1 test case. [----------] Global test environment set-up. [----------] 4 tests from OmniboxFieldTrialTest [ RUN ] OmniboxFieldTrialTest.GetDisabledProviderTypes [5938:5938:0806/125041:1038000432976:WARNING:omnibox_field_trial.cc(138)] Malformed DisabledProviders string: DisabledProviders_ [5938:5938:0806/125041:1038000433107:FATAL:field_trial.cc(213)] Check failed: !global_. [0x7f2ec09b58ac] base::debug::StackTrace::StackTrace() [0x7f2ec09f8553] logging::LogMessage::~LogMessage() [0x7f2ec0996fe3] base::FieldTrialList::FieldTrialList() [0x00000116ed65] OmniboxFieldTrialTest::ResetFieldTrialList() [0x00000116ba28] OmniboxFieldTrialTest_GetDisabledProviderTypes_Test::TestBody() [0x00000337e2d6] testing::internal::HandleSehExceptionsInMethodIfSupported<>() [0x00000337bb3f] testing::internal::HandleExceptionsInMethodIfSupported<>() [0x000003371ef9] testing::Test::Run() [0x0000033724d6] testing::TestInfo::Run() [0x0000033729e4] testing::TestCase::Run() [0x000003376fe4] testing::internal::UnitTestImpl::RunAllTests() [0x00000337f0a7] testing::internal::HandleSehExceptionsInMethodIfSupported<>() [0x00000337c2d3] testing::internal::HandleExceptionsInMethodIfSupported<>() [0x00000337612f] testing::UnitTest::Run() [0x000003f24138] base::TestSuite::Run() [0x000003dfb4b6] content::UnitTestTestSuite::Run() [0x000003c454ed] main [0x7f2eb1fa476d] __libc_start_main [0x0000007e81a9] <unknown> [5938:5938:0806/125041:1038000433107:FATAL:field_trial.cc(213)] Check failed: !global_. [0x7f2ec09b58ac] base::debug::StackTrace::StackTrace() [0x7f2ec09f8553] logging::LogMessage::~LogMessage() [0x7f2ec0996fe3] base::FieldTrialList::FieldTrialList() [0x00000116ed65] OmniboxFieldTrialTest::ResetFieldTrialList() [0x00000116ba28] OmniboxFieldTrialTest_GetDisabledProviderTypes_Test::TestBody() [0x00000337e2d6] testing::internal::HandleSehExceptionsInMethodIfSupported<>() [0x00000337bb3f] testing::internal::HandleExceptionsInMethodIfSupported<>() [0x000003371ef9] testing::Test::Run() [0x0000033724d6] testing::TestInfo::Run() [0x0000033729e4] testing::TestCase::Run() [0x000003376fe4] testing::internal::UnitTestImpl::RunAllTests() [0x00000337f0a7] testing::internal::HandleSehExceptionsInMethodIfSupported<>() [0x00000337c2d3] testing::internal::HandleExceptionsInMethodIfSupported<>() [0x00000337612f] testing::UnitTest::Run() [0x000003f24138] base::TestSuite::Run() [0x000003dfb4b6] content::UnitTestTestSuite::Run() [0x000003c454ed] main [0x7f2eb1fa476d] __libc_start_main [0x0000007e81a9] <unknown>
Ah, I see what's going on. The new FieldTrialList is being constructed
while the old one hasn't been free'd yet, which is being caught by the
DCHECK.
So you can add an empty .reset() call before the one that sets the new list:
// Destroy the existing FieldTrialList before creating a new one to avoid a
DCHECK.
field_trial_list_.reset();
field_trial_list_.reset( new base::FieldTrialList(
new metrics::SHA1EntropyProvider("foo")));
On Tue, Aug 6, 2013 at 3:52 PM, <mpearson@chromium.org> wrote:
>
> This comment is for Alexei.
>
> I'll reply to Peter's comments shortly.
>
> --mark
>
>
>
>
> https://codereview.chromium.**org/22031002/diff/3001/chrome/**
>
browser/omnibox/omnibox_field_**trial_unittest.cc<https://codereview.chromium.org/22031002/diff/3001/chrome/browser/omnibox/omnibox_field_trial_unittest.cc>
> File chrome/browser/omnibox/**omnibox_field_trial_unittest.**cc (right):
>
> https://codereview.chromium.**org/22031002/diff/3001/chrome/**
>
browser/omnibox/omnibox_field_**trial_unittest.cc#newcode21<https://codereview.chromium.org/22031002/diff/3001/chrome/browser/omnibox/omnibox_field_trial_unittest.cc#newcode21>
> chrome/browser/omnibox/**omnibox_field_trial_unittest.**cc:21: delete
> field_trial_list_;
> On 2013/08/06 18:20:41, Alexei Svitkine wrote:
>
>> On 2013/08/05 22:11:41, Mark P wrote:
>> > On 2013/08/05 18:33:11, Alexei Svitkine wrote:
>> > > If you make field_trial_list_ a non-static scoped_ptr, you can
>>
> delete this
>
>> > > method and also simplify ResetFieldTrialList below by just calling
>>
> .reset()
>
>> on
>> > > it with the new instance. (Also remove static from
>>
> ResetFieldTrialList in
>
>> that
>> > > case.)
>> >
>> > If I make the changes you suggest, I get a failure:
>> > [30420:30420:0805/144359:**958398008167:FATAL:field_**trial.cc(213)]
>>
> Check failed:
>
>> > !global_.
>>
>
> Hmm, that should not be happening if you're actually creating the
>>
> FieldTrialList
>
>> instance.
>>
>
> i.e. if you still call ResetFieldTrialList() from the constructor and
>>
> in that
>
>> function, you do:
>>
>
> field_trial_list_.reset(
>> new base::FieldTrialList(new
>>
> metrics::SHA1EntropyProvider("**foo")));
>
> If you're doing that and that's still happening, does it give you
>>
> stack trace?
>
> Stack trace below. I put the code I used to generate this stack trace
> on the patch set labeled "snapshot for Alexei".
>
> ./out/Debug/unit_tests --gtest_filter=OmniboxF*.*
> Note: Google Test filter = OmniboxF*.*
> [==========] Running 4 tests from 1 test case.
> [----------] Global test environment set-up.
> [----------] 4 tests from OmniboxFieldTrialTest
> [ RUN ] OmniboxFieldTrialTest.**GetDisabledProviderTypes
> [5938:5938:0806/125041:**1038000432976:WARNING:omnibox_**
> field_trial.cc(138)]
> Malformed DisabledProviders string: DisabledProviders_
> [5938:5938:0806/125041:**1038000433107:FATAL:field_**trial.cc(213)] Check
> failed: !global_.
> [0x7f2ec09b58ac] base::debug::StackTrace::**StackTrace()
> [0x7f2ec09f8553] logging::LogMessage::~**LogMessage()
> [0x7f2ec0996fe3] base::FieldTrialList::**FieldTrialList()
> [0x00000116ed65] OmniboxFieldTrialTest::**ResetFieldTrialList()
> [0x00000116ba28]
> OmniboxFieldTrialTest_**GetDisabledProviderTypes_Test:**:TestBody()
> [0x00000337e2d6]
> testing::internal::**HandleSehExceptionsInMethodIfS**upported<>()
> [0x00000337bb3f]
> testing::internal::**HandleExceptionsInMethodIfSupp**orted<>()
> [0x000003371ef9] testing::Test::Run()
> [0x0000033724d6] testing::TestInfo::Run()
> [0x0000033729e4] testing::TestCase::Run()
> [0x000003376fe4] testing::internal::**UnitTestImpl::RunAllTests()
> [0x00000337f0a7]
> testing::internal::**HandleSehExceptionsInMethodIfS**upported<>()
> [0x00000337c2d3]
> testing::internal::**HandleExceptionsInMethodIfSupp**orted<>()
> [0x00000337612f] testing::UnitTest::Run()
> [0x000003f24138] base::TestSuite::Run()
> [0x000003dfb4b6] content::UnitTestTestSuite::**Run()
> [0x000003c454ed] main
> [0x7f2eb1fa476d] __libc_start_main
> [0x0000007e81a9] <unknown>
>
> [5938:5938:0806/125041:**1038000433107:FATAL:field_**trial.cc(213)] Check
> failed: !global_.
> [0x7f2ec09b58ac] base::debug::StackTrace::**StackTrace()
> [0x7f2ec09f8553] logging::LogMessage::~**LogMessage()
> [0x7f2ec0996fe3] base::FieldTrialList::**FieldTrialList()
> [0x00000116ed65] OmniboxFieldTrialTest::**ResetFieldTrialList()
> [0x00000116ba28]
> OmniboxFieldTrialTest_**GetDisabledProviderTypes_Test:**:TestBody()
> [0x00000337e2d6]
> testing::internal::**HandleSehExceptionsInMethodIfS**upported<>()
> [0x00000337bb3f]
> testing::internal::**HandleExceptionsInMethodIfSupp**orted<>()
> [0x000003371ef9] testing::Test::Run()
> [0x0000033724d6] testing::TestInfo::Run()
> [0x0000033729e4] testing::TestCase::Run()
> [0x000003376fe4] testing::internal::**UnitTestImpl::RunAllTests()
> [0x00000337f0a7]
> testing::internal::**HandleSehExceptionsInMethodIfS**upported<>()
> [0x00000337c2d3]
> testing::internal::**HandleExceptionsInMethodIfSupp**orted<>()
> [0x00000337612f] testing::UnitTest::Run()
> [0x000003f24138] base::TestSuite::Run()
> [0x000003dfb4b6] content::UnitTestTestSuite::**Run()
> [0x000003c454ed] main
> [0x7f2eb1fa476d] __libc_start_main
> [0x0000007e81a9] <unknown>
>
>
https://codereview.chromium.**org/22031002/<https://codereview.chromium.org/2...
>
On 2013/08/06 20:05:38, Alexei Svitkine wrote:
> Ah, I see what's going on. The new FieldTrialList is being constructed
> while the old one hasn't been free'd yet, which is being caught by the
> DCHECK.
>
> So you can add an empty .reset() call before the one that sets the new list:
>
> // Destroy the existing FieldTrialList before creating a new one to avoid a
> DCHECK.
> field_trial_list_.reset();
> field_trial_list_.reset( new base::FieldTrialList(
> new metrics::SHA1EntropyProvider("foo")));
Is there some way we can make the code more flexible about this kind of use? It
would be nice not to do this sort of workaround.
At the very least, such code would need to be documented as to why we need it.
On 2013/08/06 20:07:59, Peter Kasting wrote:
> On 2013/08/06 20:05:38, Alexei Svitkine wrote:
> > Ah, I see what's going on. The new FieldTrialList is being constructed
> > while the old one hasn't been free'd yet, which is being caught by the
> > DCHECK.
> >
> > So you can add an empty .reset() call before the one that sets the new list:
> >
> > // Destroy the existing FieldTrialList before creating a new one to avoid a
> > DCHECK.
> > field_trial_list_.reset();
> > field_trial_list_.reset( new base::FieldTrialList(
> > new metrics::SHA1EntropyProvider("foo")));
>
> Is there some way we can make the code more flexible about this kind of use?
It
> would be nice not to do this sort of workaround.
>
> At the very least, such code would need to be documented as to why we need it.
I think it's been that way since Jim's original implementation way back.
It is kind of weird because it's meant to work mostly like a singleton except in
cases like this where you need to reset it (or each test to instantiate their
own).
Generally, I usually write tests where a single FieldTrialList is used
throughout the test, in which case this kind of thing is not necessary (since
the FieldTrialList can be a non-ptr member of the test harness or instantiated
at the start of the specific test case that needs it). So perhaps the right
answer is to discourage tests that try to reset a field trial list within a
single test case.
All comments fixed, plus revised both field trial to use Alexei's suggestion of a scoped_ptr. Please take another look thanks, mark https://codereview.chromium.org/22031002/diff/3001/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete_result.cc (right): https://codereview.chromium.org/22031002/diff/3001/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_result.cc:18: // that allows match of particular types to be demoted in AutocompleteResult. On 2013/08/06 19:33:50, Peter Kasting wrote: > Nit: match -> matches? Done. > AutocompleteResult -> AutocompleteResult::SortAndCull()? Not done because this comparison operator is used in two places in this file, only one of which is SortAndCull. The key is that this MoreRelevant should be used as the comparison function everywhere here, that's why I don't want to name function(s). https://codereview.chromium.org/22031002/diff/3001/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_result.cc:45: match.relevance * demotions_by_type_[match.type] : match.relevance; On 2013/08/06 19:33:50, Peter Kasting wrote: > Nit: How about: > > OmniboxFieldTrial::DemotionMultiplierByType::const_iterator demotion = > demotions_by_type_.find(match.type); > return (demotion == demotions_by_type_.end()) ? > match.relevance : (match.relevance * *demotion)); > > This is longer, but it avoids the double-lookup (probably no meaningful perf > impact, but if we've got the iterator anyway...) Done, with a bit of cleanup. https://codereview.chromium.org/22031002/diff/3001/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_result.cc:158: for (num_matches = 0u; (num_matches < max_num_matches) && On 2013/08/06 19:33:50, Peter Kasting wrote: > Nit: Simpler and (to me) clearer: > > matches_.resize(std::count_if(matches_.begin(), > matches_.begin() + max_num_matches, > comparing_object.GetDemotedRelevance)); > > (You might have to fiddle with some adapters if the compiler complains) I took your suggestion in the simplest way I could get it to work. I'm not sure if you'll like it. Rather than the operator() which is in this patchset, I could instead pass in something like std::bind(&CompareWithDemoteByType::GetDemotedRelevance, &comparing_object, std::placeholders::_1) as the third parameter to the count_if. I'm not sure if either of these are better than what I already had. https://codereview.chromium.org/22031002/diff/3001/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete_result.h (right): https://codereview.chromium.org/22031002/diff/3001/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_result.h:139: void AddMatch(const AutocompleteInput& input, On 2013/08/06 19:33:50, Peter Kasting wrote: > I think you should just pass the actual PageClassification here instead of the > whole Input (which you don't need). (2 places) Done (both places). https://codereview.chromium.org/22031002/diff/3001/chrome/browser/omnibox/omn... File chrome/browser/omnibox/omnibox_field_trial.cc (right): https://codereview.chromium.org/22031002/diff/3001/chrome/browser/omnibox/omn... chrome/browser/omnibox/omnibox_field_trial.cc:251: std::vector<std::pair<std::string, std::string> > kv_pairs; On 2013/08/06 19:33:50, Peter Kasting wrote: > Nit: This suggests that string_split.h should have a typedef for this Done. https://codereview.chromium.org/22031002/diff/3001/chrome/browser/omnibox/omn... File chrome/browser/omnibox/omnibox_field_trial.h (right): https://codereview.chromium.org/22031002/diff/3001/chrome/browser/omnibox/omn... chrome/browser/omnibox/omnibox_field_trial.h:23: typedef std::map<AutocompleteMatchType::Type, float> DemotionMultiplierByType; On 2013/08/06 19:33:50, Peter Kasting wrote: > Nit: "DemotionMultipliers" seems pretty clear too, and shorter. Done. https://codereview.chromium.org/22031002/diff/3001/chrome/browser/omnibox/omn... chrome/browser/omnibox/omnibox_field_trial.h:23: typedef std::map<AutocompleteMatchType::Type, float> DemotionMultiplierByType; On 2013/08/06 19:33:50, Peter Kasting wrote: > Nit: "DemotionMultipliers" seems pretty clear too, and shorter. Done. Changes a variable name or two as appropriate too. https://codereview.chromium.org/22031002/diff/3001/chrome/browser/omnibox/omn... chrome/browser/omnibox/omnibox_field_trial.h:134: // of certain types of matches, populate the |demotions_by_type| map On 2013/08/06 19:33:50, Peter Kasting wrote: > Nit: populate -> populates Done. https://codereview.chromium.org/22031002/diff/3001/chrome/browser/omnibox/omn... chrome/browser/omnibox/omnibox_field_trial.h:135: // appropriately. Otherwise, clear |demotions_by_type|. On 2013/08/06 19:33:50, Peter Kasting wrote: > Nit: clear -> clears Done. https://codereview.chromium.org/22031002/diff/3001/chrome/browser/omnibox/omn... File chrome/browser/omnibox/omnibox_field_trial_unittest.cc (right): https://codereview.chromium.org/22031002/diff/3001/chrome/browser/omnibox/omn... chrome/browser/omnibox/omnibox_field_trial_unittest.cc:154: demotions_by_type[AutocompleteMatchType::HISTORY_TITLE]); On 2013/08/06 19:33:50, Peter Kasting wrote: > I think this expectation will still pass even if the code is broken, because > operator[] will add a value if it doesn't already exist in the map (and 0 would > be the default value). You might want to use find() instead of [] for these > tests (perhaps via a small helper function to reduce verbosity?). Good point. Added helper function. > Nit: All lines of args must be aligned. now obsolete.
LGTM https://codereview.chromium.org/22031002/diff/3001/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete_result.cc (right): https://codereview.chromium.org/22031002/diff/3001/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_result.cc:158: for (num_matches = 0u; (num_matches < max_num_matches) && On 2013/08/06 22:00:15, Mark P wrote: > On 2013/08/06 19:33:50, Peter Kasting wrote: > > Nit: Simpler and (to me) clearer: > > > > matches_.resize(std::count_if(matches_.begin(), > > matches_.begin() + max_num_matches, > > comparing_object.GetDemotedRelevance)); > > > > (You might have to fiddle with some adapters if the compiler complains) > > I took your suggestion in the simplest way I could get it to work. I'm not sure > if you'll like it. It's clever, but I think semantically confusing. Reading "count if (comparing object)" doesn't really make any sense. > Rather than the operator() which is in this patchset, I could instead pass in > something like > std::bind(&CompareWithDemoteByType::GetDemotedRelevance, &comparing_object, > std::placeholders::_1) > as the third parameter to the count_if. I think std::bind() is C++11, no? We don't support that (yet) in Chromium. The only way I can think of to do this is to write another functor that takes a CompareWithDemoteByType object and provides an operator(). This is probably uglier than just doing what you had originally, so I'd probably give up. https://codereview.chromium.org/22031002/diff/25001/base/strings/string_split.h File base/strings/string_split.h (right): https://codereview.chromium.org/22031002/diff/25001/base/strings/string_split... base/strings/string_split.h:39: typedef std::vector<std::pair<std::string, std::string> > VectorOfKeyValuePairs; Nit: "KeyValuePairs" is shorter, just as clear, and avoids encoding "vector" into the typename which would suck if for some reason we e.g. made this return a map or something someday. Can you maybe file a cleanup bug that all code that calls this function can begin using this typedef? https://codereview.chromium.org/22031002/diff/25001/chrome/browser/omnibox/om... File chrome/browser/omnibox/omnibox_field_trial.cc (right): https://codereview.chromium.org/22031002/diff/25001/chrome/browser/omnibox/om... chrome/browser/omnibox/omnibox_field_trial.cc:251: // std::vector<std::pair<std::string, std::string> > Nit: Nuke commented-out line https://codereview.chromium.org/22031002/diff/25001/chrome/browser/omnibox/om... chrome/browser/omnibox/omnibox_field_trial.cc:254: for (size_t i = 0; i < kv_pairs.size(); ++i) { Nit: With typedef now available (especially if you shorten the name per my other comment), consider using an iterator here. https://codereview.chromium.org/22031002/diff/25001/chrome/browser/omnibox/om... File chrome/browser/omnibox/omnibox_field_trial_unittest.cc (right): https://codereview.chromium.org/22031002/diff/25001/chrome/browser/omnibox/om... chrome/browser/omnibox/omnibox_field_trial_unittest.cc:59: EXPECT_TRUE(demotion_it != demotions.end()); Nit: ASSERT_TRUE, since you deref this
lgtm with nits Thanks! https://codereview.chromium.org/22031002/diff/35001/chrome/browser/omnibox/om... File chrome/browser/omnibox/omnibox_field_trial.cc (right): https://codereview.chromium.org/22031002/diff/35001/chrome/browser/omnibox/om... chrome/browser/omnibox/omnibox_field_trial.cc:251: // std::vector<std::pair<std::string, std::string> > Nit: Remove this line https://codereview.chromium.org/22031002/diff/35001/chrome/common/metrics/var... File chrome/common/metrics/variations/variations_associated_data.h (right): https://codereview.chromium.org/22031002/diff/35001/chrome/common/metrics/var... chrome/common/metrics/variations/variations_associated_data.h:141: // that is implemented above. Nit: I realise that this comment is wrong here. Could you delete the second sentence? (it's correct in the context of the .cc file, but not the .h file) Thanks!
https://codereview.chromium.org/22031002/diff/3001/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete_result.cc (right): https://codereview.chromium.org/22031002/diff/3001/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_result.cc:158: for (num_matches = 0u; (num_matches < max_num_matches) && On 2013/08/06 22:19:40, Peter Kasting wrote: > On 2013/08/06 22:00:15, Mark P wrote: > > On 2013/08/06 19:33:50, Peter Kasting wrote: > > > Nit: Simpler and (to me) clearer: > > > > > > matches_.resize(std::count_if(matches_.begin(), > > > matches_.begin() + max_num_matches, > > > comparing_object.GetDemotedRelevance)); > > > > > > (You might have to fiddle with some adapters if the compiler complains) > > > > I took your suggestion in the simplest way I could get it to work. I'm not > sure > > if you'll like it. > > It's clever, but I think semantically confusing. Reading "count if (comparing > object)" doesn't really make any sense. > > > Rather than the operator() which is in this patchset, I could instead pass in > > something like > > std::bind(&CompareWithDemoteByType::GetDemotedRelevance, &comparing_object, > > std::placeholders::_1) > > as the third parameter to the count_if. > > I think std::bind() is C++11, no? We don't support that (yet) in Chromium. > > The only way I can think of to do this is to write another functor that takes a > CompareWithDemoteByType object and provides an operator(). This is probably > uglier than just doing what you had originally, so I'd probably give up. Okay, reverted back to original code. https://codereview.chromium.org/22031002/diff/25001/base/strings/string_split.h File base/strings/string_split.h (right): https://codereview.chromium.org/22031002/diff/25001/base/strings/string_split... base/strings/string_split.h:39: typedef std::vector<std::pair<std::string, std::string> > VectorOfKeyValuePairs; On 2013/08/06 22:19:40, Peter Kasting wrote: > Nit: "KeyValuePairs" is shorter, just as clear, and avoids encoding "vector" > into the typename which would suck if for some reason we e.g. made this return a > map or something someday. Okay. > Can you maybe file a cleanup bug that all code that calls this function can > begin using this typedef? Will do. Can you suggest a reviewer for the current strings change? It requires a global owner (there's no base/strings/ owner). I'll mention this followup bug once I get that reviewer. https://codereview.chromium.org/22031002/diff/25001/chrome/browser/omnibox/om... File chrome/browser/omnibox/omnibox_field_trial.cc (right): https://codereview.chromium.org/22031002/diff/25001/chrome/browser/omnibox/om... chrome/browser/omnibox/omnibox_field_trial.cc:251: // std::vector<std::pair<std::string, std::string> > On 2013/08/06 22:19:40, Peter Kasting wrote: > Nit: Nuke commented-out line Done. https://codereview.chromium.org/22031002/diff/25001/chrome/browser/omnibox/om... chrome/browser/omnibox/omnibox_field_trial.cc:254: for (size_t i = 0; i < kv_pairs.size(); ++i) { On 2013/08/06 22:19:40, Peter Kasting wrote: > Nit: With typedef now available (especially if you shorten the name per my other > comment), consider using an iterator here. Done. https://codereview.chromium.org/22031002/diff/25001/chrome/browser/omnibox/om... File chrome/browser/omnibox/omnibox_field_trial_unittest.cc (right): https://codereview.chromium.org/22031002/diff/25001/chrome/browser/omnibox/om... chrome/browser/omnibox/omnibox_field_trial_unittest.cc:59: EXPECT_TRUE(demotion_it != demotions.end()); On 2013/08/06 22:19:40, Peter Kasting wrote: > Nit: ASSERT_TRUE, since you deref this Done. https://codereview.chromium.org/22031002/diff/35001/chrome/browser/omnibox/om... File chrome/browser/omnibox/omnibox_field_trial.cc (right): https://codereview.chromium.org/22031002/diff/35001/chrome/browser/omnibox/om... chrome/browser/omnibox/omnibox_field_trial.cc:251: // std::vector<std::pair<std::string, std::string> > On 2013/08/06 22:26:33, Alexei Svitkine wrote: > Nit: Remove this line Done. https://codereview.chromium.org/22031002/diff/35001/chrome/common/metrics/var... File chrome/common/metrics/variations/variations_associated_data.h (right): https://codereview.chromium.org/22031002/diff/35001/chrome/common/metrics/var... chrome/common/metrics/variations/variations_associated_data.h:141: // that is implemented above. On 2013/08/06 22:26:33, Alexei Svitkine wrote: > Nit: I realise that this comment is wrong here. Could you delete the second > sentence? (it's correct in the context of the .cc file, but not the .h file) > Thanks! Done.
https://codereview.chromium.org/22031002/diff/25001/base/strings/string_split.h File base/strings/string_split.h (right): https://codereview.chromium.org/22031002/diff/25001/base/strings/string_split... base/strings/string_split.h:39: typedef std::vector<std::pair<std::string, std::string> > VectorOfKeyValuePairs; On 2013/08/06 22:44:49, Mark P wrote: > Can you suggest a reviewer for the current strings change? It > requires a global owner (there's no base/strings/ owner). Anyone in base/OWNERS. For example, brettw.
Brett, could you please review the base/strings/... change? thanks, mark
Brett, could you please review the base/strings/... change? I've got two other changes backed up behind this that I'd like to submit this week. thanks, mark
Darin, Could you please review and approve the base/strings/... change? It should be a really fast review. thanks, mark
https://codereview.chromium.org/22031002/diff/27002/base/strings/string_split.h File base/strings/string_split.h (right): https://codereview.chromium.org/22031002/diff/27002/base/strings/string_split... base/strings/string_split.h:39: typedef std::vector<std::pair<std::string, std::string> > KeyValuePairs; If I saw base::KeyValuePairs in code, it wouldn't be very obvious that it was really just a vector of string pairs. It seems like base::StringPairs would be a much more descriptive name for this type. It just so happens that the SplitStringIntoKeyValuePairs function returns data of that type. Seems like an improvement, right? https://codereview.chromium.org/22031002/diff/27002/base/strings/string_split... base/strings/string_split.h:45: KeyValuePairs* kv_pairs); nit: while you are here, perhaps kv_pairs can be named key_value_pairs for consistency with other params?
https://codereview.chromium.org/22031002/diff/27002/base/strings/string_split.h File base/strings/string_split.h (right): https://codereview.chromium.org/22031002/diff/27002/base/strings/string_split... base/strings/string_split.h:39: typedef std::vector<std::pair<std::string, std::string> > KeyValuePairs; On 2013/08/08 04:40:55, darin wrote: > If I saw base::KeyValuePairs in code, it wouldn't be very obvious that it was > really just a vector of string pairs. It seems like base::StringPairs would be > a much more descriptive name for this type. It just so happens that the > SplitStringIntoKeyValuePairs function returns data of that type. Seems like an > improvement, right? Yes, that's an improvement. Done. https://codereview.chromium.org/22031002/diff/27002/base/strings/string_split... base/strings/string_split.h:45: KeyValuePairs* kv_pairs); On 2013/08/08 04:40:55, darin wrote: > nit: while you are here, perhaps kv_pairs can be named key_value_pairs for > consistency with other params? Okay. Done.
LGTM for base/ On Aug 8, 2013 6:16 AM, <mpearson@chromium.org> wrote: > > https://codereview.chromium.**org/22031002/diff/27002/base/** > strings/string_split.h<https://codereview.chromium.org/22031002/diff/27002/base/strings/string_split.h> > File base/strings/string_split.h (right): > > https://codereview.chromium.**org/22031002/diff/27002/base/** > strings/string_split.h#**newcode39<https://codereview.chromium.org/22031002/diff/27002/base/strings/string_split.h#newcode39> > base/strings/string_split.h:**39: typedef > std::vector<std::pair<std::**string, std::string> > KeyValuePairs; > On 2013/08/08 04:40:55, darin wrote: > >> If I saw base::KeyValuePairs in code, it wouldn't be very obvious that >> > it was > >> really just a vector of string pairs. It seems like base::StringPairs >> > would be > >> a much more descriptive name for this type. It just so happens that >> > the > >> SplitStringIntoKeyValuePairs function returns data of that type. >> > Seems like an > >> improvement, right? >> > > Yes, that's an improvement. Done. > > https://codereview.chromium.**org/22031002/diff/27002/base/** > strings/string_split.h#**newcode45<https://codereview.chromium.org/22031002/diff/27002/base/strings/string_split.h#newcode45> > base/strings/string_split.h:**45: KeyValuePairs* kv_pairs); > On 2013/08/08 04:40:55, darin wrote: > >> nit: while you are here, perhaps kv_pairs can be named key_value_pairs >> > for > >> consistency with other params? >> > > Okay. Done. > > https://codereview.chromium.**org/22031002/<https://codereview.chromium.org/2... >
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mpearson@chromium.org/22031002/49001
Failed to trigger a try job on win_x64_rel HTTP Error 400: Bad Request
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mpearson@chromium.org/22031002/69001
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=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mpearson@chromium.org/22031002/69001
Sorry for I got bad news for ya. Compile failed with a clobber build on android_dbg. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mpearson@chromium.org/22031002/88001
Message was sent while issue was closed.
Change committed as 216633 |
