Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13932034:
Omnibox refactor, introduced OmniboxController. (Closed)
|
Created:
7 years, 8 months ago by beaudoin Modified:
7 years, 8 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionOmnibox refactor, introduced OmniboxController.
For the moment, OmniboxController is just a light-weight class between the AutocompleteController and the OmniboxEditModel. The final goal is to remove any reference to the AutocompleteController (and the SearchProvider) from OmniboxEditModel. This is the first step.
BUG=234733
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=196582
Patch Set 1 #Patch Set 2 : With new files this time... #Patch Set 3 : Cleaned some comments. #
Total comments: 2
Patch Set 4 : Answered mathieu, rebased. #
Total comments: 8
Patch Set 5 : Answered Sreeram, added tests. #Patch Set 6 : Corrected code style. #
Total comments: 8
Patch Set 7 : Modified tests, answered Sreeram. #
Total comments: 35
Patch Set 8 : Answered Peter's comments. #
Total comments: 6
Patch Set 9 : Fixed nits. #Patch Set 10 : Rebased #
Messages
Total messages: 26 (0 generated)
Sreeram, mathieu: This is a very basic step in the OmniboxEditModel refactor, but I just wanted to check with you: - Do you think it's going in the right directions? - What do you think of the naming? I'm not sure OmniboxController is quite right... - Should I try to land this and keep going in small installments or should I go for a big-bang approach?
Added missing files...
I think this is a patch that goes in the right direction, looks solid to me! https://chromiumcodereview.appspot.com/13932034/diff/6001/chrome/browser/ui/o... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://chromiumcodereview.appspot.com/13932034/diff/6001/chrome/browser/ui/o... chrome/browser/ui/omnibox/omnibox_edit_model.cc:240: nit: unnecessary new line
With the done this time. :) https://codereview.chromium.org/13932034/diff/6001/chrome/browser/ui/omnibox/... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://codereview.chromium.org/13932034/diff/6001/chrome/browser/ui/omnibox/... chrome/browser/ui/omnibox/omnibox_edit_model.cc:240: On 2013/04/23 15:26:07, Mathieu Perreault wrote: > nit: unnecessary new line Done.
Please create a bug and add it to the BUG= line. This will make it easy to find all CLs related to the refactor. Also, here's a good opportunity for an omnibox_controller_unittest: Test that the set of providers is as expected with and without Instant Extended enabled. https://codereview.chromium.org/13932034/diff/10001/chrome/browser/ui/omnibox... File chrome/browser/ui/omnibox/omnibox_controller.cc (right): https://codereview.chromium.org/13932034/diff/10001/chrome/browser/ui/omnibox... chrome/browser/ui/omnibox/omnibox_controller.cc:14: Profile* profile) This can be formatted like so: OmniboxController::OmniboxController(OmniboxEditModel* omnibox_edit_model, Profile* profile) : omnibox_edit_model_(omnibox_edit_model) { https://codereview.chromium.org/13932034/diff/10001/chrome/browser/ui/omnibox... File chrome/browser/ui/omnibox/omnibox_controller.h (right): https://codereview.chromium.org/13932034/diff/10001/chrome/browser/ui/omnibox... chrome/browser/ui/omnibox/omnibox_controller.h:8: #include "base/compiler_specific.h" Also add "base/basictypes.h" (for the DISALLOW macro) https://codereview.chromium.org/13932034/diff/10001/chrome/browser/ui/omnibox... chrome/browser/ui/omnibox/omnibox_controller.h:18: // longer need to hold any reference to AutocompleteController. Also make need -> needs https://codereview.chromium.org/13932034/diff/10001/chrome/browser/ui/omnibox... chrome/browser/ui/omnibox/omnibox_controller.h:24: Profile* profile); Fits on one line.
Answered your comments, added some tests. https://codereview.chromium.org/13932034/diff/10001/chrome/browser/ui/omnibox... File chrome/browser/ui/omnibox/omnibox_controller.cc (right): https://codereview.chromium.org/13932034/diff/10001/chrome/browser/ui/omnibox... chrome/browser/ui/omnibox/omnibox_controller.cc:14: Profile* profile) On 2013/04/23 18:26:51, sreeram wrote: > This can be formatted like so: > > OmniboxController::OmniboxController(OmniboxEditModel* omnibox_edit_model, > Profile* profile) > : omnibox_edit_model_(omnibox_edit_model) { Done. https://codereview.chromium.org/13932034/diff/10001/chrome/browser/ui/omnibox... File chrome/browser/ui/omnibox/omnibox_controller.h (right): https://codereview.chromium.org/13932034/diff/10001/chrome/browser/ui/omnibox... chrome/browser/ui/omnibox/omnibox_controller.h:8: #include "base/compiler_specific.h" On 2013/04/23 18:26:51, sreeram wrote: > Also add "base/basictypes.h" (for the DISALLOW macro) Done. https://codereview.chromium.org/13932034/diff/10001/chrome/browser/ui/omnibox... chrome/browser/ui/omnibox/omnibox_controller.h:18: // longer need to hold any reference to AutocompleteController. Also make On 2013/04/23 18:26:51, sreeram wrote: > need -> needs Done. https://codereview.chromium.org/13932034/diff/10001/chrome/browser/ui/omnibox... chrome/browser/ui/omnibox/omnibox_controller.h:24: Profile* profile); On 2013/04/23 18:26:51, sreeram wrote: > Fits on one line. Done.
https://codereview.chromium.org/13932034/diff/23001/chrome/browser/ui/omnibox... File chrome/browser/ui/omnibox/omnibox_controller_unittest.cc (right): https://codereview.chromium.org/13932034/diff/23001/chrome/browser/ui/omnibox... chrome/browser/ui/omnibox/omnibox_controller_unittest.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. No "(c)". https://codereview.chromium.org/13932034/diff/23001/chrome/browser/ui/omnibox... chrome/browser/ui/omnibox/omnibox_controller_unittest.cc:69: AutocompleteProvider::TYPE_SHORTCUTS; I don't this test buys us much. In my opinion, the main point of such a test is to ensure that the two lists (instant-extended and non-instant-extended) are kept "in sync". So, for example, if somebody adds or removes a provider from the non-instant-extended list, and they forget to do so similarly for the instant-extended list, the test should alert them to it. With this test, they'll be reminded to update the list in the test here (in CheckDefaultAutocompleteProviders), but they may still not realize that they have to update the other list too. So, I think a better technique is to just have one test, like so: TEST_F(OmniboxControllerTest, DefaultVersusInstantAutocompleteProviders) { int default_providers = AutocompleteClassifier::kDefaultOmniboxProviders; int instant_providers = AutocompleteClassifier::kInstantExtendedOmniboxProviders; int expected_difference = AutocompleteProvider::TYPE_HISTORY_CONTENTS | ...; EXPECT_EQ(expected_difference, default_providers - instant_providers); } This doesn't even need you to enable the Instant extended API, set up a profile, or any such thing. WDYT? https://codereview.chromium.org/13932034/diff/23001/chrome/browser/ui/omnibox... chrome/browser/ui/omnibox/omnibox_controller_unittest.cc:82: switches::kEnableInstantExtendedAPI); You might also instead consider #including "c/b/search/search.h" and calling the EnableInstantExtendedAPIForTesting() method instead. (Of course, if you accept the suggestion in my other comment in this file, this becomes unnecessary.) https://codereview.chromium.org/13932034/diff/23001/chrome/chrome_tests_unit.... File chrome/chrome_tests_unit.gypi (right): https://codereview.chromium.org/13932034/diff/23001/chrome/chrome_tests_unit.... chrome/chrome_tests_unit.gypi:1430: 'browser/ui/sync/profile_signin_confirmation_helper_unittest.cc', There seems to be stray <Tab> char at the beginning of this line. Remove?
Sreeram, PTAL. https://chromiumcodereview.appspot.com/13932034/diff/23001/chrome/browser/ui/... File chrome/browser/ui/omnibox/omnibox_controller_unittest.cc (right): https://chromiumcodereview.appspot.com/13932034/diff/23001/chrome/browser/ui/... chrome/browser/ui/omnibox/omnibox_controller_unittest.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2013/04/24 20:25:03, sreeram wrote: > No "(c)". Done. https://chromiumcodereview.appspot.com/13932034/diff/23001/chrome/browser/ui/... chrome/browser/ui/omnibox/omnibox_controller_unittest.cc:69: AutocompleteProvider::TYPE_SHORTCUTS; On 2013/04/24 20:25:03, sreeram wrote: > I don't this test buys us much. > > In my opinion, the main point of such a test is to ensure that the two lists > (instant-extended and non-instant-extended) are kept "in sync". So, for example, > if somebody adds or removes a provider from the non-instant-extended list, and > they forget to do so similarly for the instant-extended list, the test should > alert them to it. > > With this test, they'll be reminded to update the list in the test here (in > CheckDefaultAutocompleteProviders), but they may still not realize that they > have to update the other list too. > > So, I think a better technique is to just have one test, like so: > > TEST_F(OmniboxControllerTest, DefaultVersusInstantAutocompleteProviders) { > int default_providers = AutocompleteClassifier::kDefaultOmniboxProviders; > int instant_providers = > AutocompleteClassifier::kInstantExtendedOmniboxProviders; > int expected_difference = AutocompleteProvider::TYPE_HISTORY_CONTENTS | ...; > EXPECT_EQ(expected_difference, default_providers - instant_providers); > } > > This doesn't even need you to enable the Instant extended API, set up a profile, > or any such thing. WDYT? Not sure this test should live in the AutocompleteController since it only checks the constants and "hopes" the AutocompleteController are using them. If that's really what we want we should write a test in AutocompleteClassifier where the constants live. I've taken a different approach where I check that the delta between the actually instantiated providers in both cases are different. It should catch the problem you're thinking of and any other that could stem from the instantiation process. As for the Profile and all, I already need them in tests I've written in the follow-up patch. :) https://chromiumcodereview.appspot.com/13932034/diff/23001/chrome/browser/ui/... chrome/browser/ui/omnibox/omnibox_controller_unittest.cc:82: switches::kEnableInstantExtendedAPI); On 2013/04/24 20:25:03, sreeram wrote: > You might also instead consider #including "c/b/search/search.h" and calling the > EnableInstantExtendedAPIForTesting() method instead. (Of course, if you accept > the suggestion in my other comment in this file, this becomes unnecessary.) Done. https://chromiumcodereview.appspot.com/13932034/diff/23001/chrome/chrome_test... File chrome/chrome_tests_unit.gypi (right): https://chromiumcodereview.appspot.com/13932034/diff/23001/chrome/chrome_test... chrome/chrome_tests_unit.gypi:1430: 'browser/ui/sync/profile_signin_confirmation_helper_unittest.cc', On 2013/04/24 20:25:03, sreeram wrote: > There seems to be stray <Tab> char at the beginning of this line. Remove? Ahah! That's why it was off in my editor. Done.
lgtm
Peter: For owners.
https://chromiumcodereview.appspot.com/13932034/diff/29001/chrome/browser/ui/... File chrome/browser/ui/omnibox/omnibox_controller.cc (right): https://chromiumcodereview.appspot.com/13932034/diff/29001/chrome/browser/ui/... chrome/browser/ui/omnibox/omnibox_controller.cc:11: Tiny nit: I've slowly been moving towards two newlines between the #includes and code (or between one class and the next; i.e. major divisions within the file). https://chromiumcodereview.appspot.com/13932034/diff/29001/chrome/browser/ui/... chrome/browser/ui/omnibox/omnibox_controller.cc:16: // Instant Extended API, as it doesn't support them all. Nit: Comment is unnecessary https://chromiumcodereview.appspot.com/13932034/diff/29001/chrome/browser/ui/... File chrome/browser/ui/omnibox/omnibox_controller.h (right): https://chromiumcodereview.appspot.com/13932034/diff/29001/chrome/browser/ui/... chrome/browser/ui/omnibox/omnibox_controller.h:8: #include "base/basictypes.h" Is this #include needed? https://chromiumcodereview.appspot.com/13932034/diff/29001/chrome/browser/ui/... chrome/browser/ui/omnibox/omnibox_controller.h:17: // This class sits between the OmniboxEditModel and AutocompleteController. Nit: This comment isn't really informative about what the class is/means/does. Instead say what the class _is_, from a high-level perspective. https://chromiumcodereview.appspot.com/13932034/diff/29001/chrome/browser/ui/... chrome/browser/ui/omnibox/omnibox_controller.h:27: AutocompleteController* autocomplete_controller() const { Const methods must not return non-const pointers. https://chromiumcodereview.appspot.com/13932034/diff/29001/chrome/browser/ui/... chrome/browser/ui/omnibox/omnibox_controller.h:31: // AutocompleteControllerDelegate: I suggest putting overrides above non-overrides. https://chromiumcodereview.appspot.com/13932034/diff/29001/chrome/browser/ui/... chrome/browser/ui/omnibox/omnibox_controller.h:37: // this and OmniboxEditModel define and use a Delegate interface instead. Why? Is there a real win in defining such an interface when only one class will ever implement it? It seems like it would just add indirection. https://chromiumcodereview.appspot.com/13932034/diff/29001/chrome/browser/ui/... File chrome/browser/ui/omnibox/omnibox_controller_unittest.cc (right): https://chromiumcodereview.appspot.com/13932034/diff/29001/chrome/browser/ui/... chrome/browser/ui/omnibox/omnibox_controller_unittest.cc:18: virtual void SetUp() OVERRIDE { Nit: Do not define virtual (and other non-cheap) functions inline, even in test files. https://chromiumcodereview.appspot.com/13932034/diff/29001/chrome/browser/ui/... chrome/browser/ui/omnibox/omnibox_controller_unittest.cc:24: profile_.reset(); Nit: Do we actually need to tear these down here instead of in the destructor? If so, add a comment saying why. https://chromiumcodereview.appspot.com/13932034/diff/29001/chrome/browser/ui/... chrome/browser/ui/omnibox/omnibox_controller_unittest.cc:34: scoped_ptr<TestingProfile> profile_; Google style bans non-private data members in classes. Make these private and use accessors or define functions on the class to do what you need. https://chromiumcodereview.appspot.com/13932034/diff/29001/chrome/browser/ui/... chrome/browser/ui/omnibox/omnibox_controller_unittest.cc:36: }; Nit: DISALLOW_COPY_AND_ASSIGN https://chromiumcodereview.appspot.com/13932034/diff/29001/chrome/browser/ui/... chrome/browser/ui/omnibox/omnibox_controller_unittest.cc:47: ASSERT_EQ(type, expected_providers & type); Nit: What about just ASSERT_TRUE(expected_providers & type);? https://chromiumcodereview.appspot.com/13932034/diff/29001/chrome/browser/ui/... chrome/browser/ui/omnibox/omnibox_controller_unittest.cc:68: AutocompleteProvider::TYPE_SHORTCUTS; I'm a little unhappy about this. It doesn't seem like someone changing the provider sets should have to go change the test to be in sync as well -- in other words, this seems like too low-level of a thing to be testing. I read the previous review traffic on this and honestly I think you should just nuke this test entirely. I don't even think we should test that the various lists are "in-sync" with each other. https://chromiumcodereview.appspot.com/13932034/diff/29001/chrome/browser/ui/... File chrome/browser/ui/omnibox/omnibox_edit_model.h (right): https://chromiumcodereview.appspot.com/13932034/diff/29001/chrome/browser/ui/... chrome/browser/ui/omnibox/omnibox_edit_model.h:85: AutocompleteController* autocomplete_controller() const { Ultimately, this accessor will disappear, right? Since this class shouldn't know about the AutocompleteController. What outside this class uses this right now? Can we at least make it private? Should we change the public function to an OmniboxController accessor instead? Also, this is another bad case of a const function returning a non-const pointer.
https://chromiumcodereview.appspot.com/13932034/diff/29001/chrome/browser/ui/... File chrome/browser/ui/omnibox/omnibox_controller_unittest.cc (right): https://chromiumcodereview.appspot.com/13932034/diff/29001/chrome/browser/ui/... chrome/browser/ui/omnibox/omnibox_controller_unittest.cc:68: AutocompleteProvider::TYPE_SHORTCUTS; On 2013/04/25 18:24:55, Peter Kasting wrote: > I'm a little unhappy about this. It doesn't seem like someone changing the > provider sets should have to go change the test to be in sync as well -- in > other words, this seems like too low-level of a thing to be testing. > > I read the previous review traffic on this and honestly I think you should just > nuke this test entirely. I don't even think we should test that the various > lists are "in-sync" with each other. I think the test is valuable. If somebody changes the non-instant provider set, this test will force them to think, "Does this provider change apply to the instant set as well?". If it does, they just have to add it to the instant set, no need to change the test. If it doesn't, they have to update the test; thus, they are making a conscious decision to exclude the change from the instant set.
https://chromiumcodereview.appspot.com/13932034/diff/29001/chrome/browser/ui/... File chrome/browser/ui/omnibox/omnibox_controller_unittest.cc (right): https://chromiumcodereview.appspot.com/13932034/diff/29001/chrome/browser/ui/... chrome/browser/ui/omnibox/omnibox_controller_unittest.cc:68: AutocompleteProvider::TYPE_SHORTCUTS; On 2013/04/25 18:47:51, sreeram wrote: > On 2013/04/25 18:24:55, Peter Kasting wrote: > > I'm a little unhappy about this. It doesn't seem like someone changing the > > provider sets should have to go change the test to be in sync as well -- in > > other words, this seems like too low-level of a thing to be testing. > > > > I read the previous review traffic on this and honestly I think you should > just > > nuke this test entirely. I don't even think we should test that the various > > lists are "in-sync" with each other. > > I think the test is valuable. If somebody changes the non-instant provider set, > this test will force them to think, "Does this provider change apply to the > instant set as well?". If it does, they just have to add it to the instant set, > no need to change the test. If it doesn't, they have to update the test; thus, > they are making a conscious decision to exclude the change from the instant set. We define the two sets right next to each other. You'd have to be blind not to see one set while changing the other :). So I think the value is extremely low, whereas the hassle factor is not (you're guaranteed to have to change this file on literally every change). I've been burned enough by crappy low-level make-work tests with Mac code before that I'm very cognizant of that at this point.
https://chromiumcodereview.appspot.com/13932034/diff/29001/chrome/browser/ui/... File chrome/browser/ui/omnibox/omnibox_controller.h (right): https://chromiumcodereview.appspot.com/13932034/diff/29001/chrome/browser/ui/... chrome/browser/ui/omnibox/omnibox_controller.h:8: #include "base/basictypes.h" On 2013/04/25 18:24:55, Peter Kasting wrote: > Is this #include needed? Gone. Done. https://chromiumcodereview.appspot.com/13932034/diff/29001/chrome/browser/ui/... File chrome/browser/ui/omnibox/omnibox_controller_unittest.cc (right): https://chromiumcodereview.appspot.com/13932034/diff/29001/chrome/browser/ui/... chrome/browser/ui/omnibox/omnibox_controller_unittest.cc:68: AutocompleteProvider::TYPE_SHORTCUTS; On 2013/04/25 19:05:07, Peter Kasting wrote: > On 2013/04/25 18:47:51, sreeram wrote: > > On 2013/04/25 18:24:55, Peter Kasting wrote: > > > I'm a little unhappy about this. It doesn't seem like someone changing the > > > provider sets should have to go change the test to be in sync as well -- in > > > other words, this seems like too low-level of a thing to be testing. > > > > > > I read the previous review traffic on this and honestly I think you should > > just > > > nuke this test entirely. I don't even think we should test that the various > > > lists are "in-sync" with each other. > > > > I think the test is valuable. If somebody changes the non-instant provider > set, > > this test will force them to think, "Does this provider change apply to the > > instant set as well?". If it does, they just have to add it to the instant > set, > > no need to change the test. If it doesn't, they have to update the test; thus, > > they are making a conscious decision to exclude the change from the instant > set. > > We define the two sets right next to each other. You'd have to be blind not to > see one set while changing the other :). So I think the value is extremely low, > whereas the hassle factor is not (you're guaranteed to have to change this file > on literally every change). I've been burned enough by crappy low-level > make-work tests with Mac code before that I'm very cognizant of that at this > point. What if I only checked the difference, instead of looking at what is contained in the first set? This would break only if you break the expectation (ie. instant-extended is exactly the regular providers minus a couple) instead of breaking whenever we change something. I'll do this and send a new patchset.#
https://chromiumcodereview.appspot.com/13932034/diff/29001/chrome/browser/ui/... File chrome/browser/ui/omnibox/omnibox_controller_unittest.cc (right): https://chromiumcodereview.appspot.com/13932034/diff/29001/chrome/browser/ui/... chrome/browser/ui/omnibox/omnibox_controller_unittest.cc:68: AutocompleteProvider::TYPE_SHORTCUTS; On 2013/04/25 19:14:57, beaudoin wrote: > What if I only checked the difference, instead of looking at what is contained > in the first set? That would improve things. I still feel like it's unnecessary given that we intend to change the instant set to support the shortcuts provider at some point (which will require updating this test), and we'll be removing the non-instant set later as well (which will make the test meaningless). The chances of adding a new provider before those happen seems miniscule.
Peter: PTAL. https://chromiumcodereview.appspot.com/13932034/diff/29001/chrome/browser/ui/... File chrome/browser/ui/omnibox/omnibox_controller.cc (right): https://chromiumcodereview.appspot.com/13932034/diff/29001/chrome/browser/ui/... chrome/browser/ui/omnibox/omnibox_controller.cc:11: On 2013/04/25 18:24:55, Peter Kasting wrote: > Tiny nit: I've slowly been moving towards two newlines between the #includes and > code (or between one class and the next; i.e. major divisions within the file). Happy to adopt this. I've done it in the tests too, but it looks weird in the .h. If you see inconsistencies anywhere let me know. https://chromiumcodereview.appspot.com/13932034/diff/29001/chrome/browser/ui/... chrome/browser/ui/omnibox/omnibox_controller.cc:16: // Instant Extended API, as it doesn't support them all. On 2013/04/25 18:24:55, Peter Kasting wrote: > Nit: Comment is unnecessary Done. https://chromiumcodereview.appspot.com/13932034/diff/29001/chrome/browser/ui/... File chrome/browser/ui/omnibox/omnibox_controller.h (right): https://chromiumcodereview.appspot.com/13932034/diff/29001/chrome/browser/ui/... chrome/browser/ui/omnibox/omnibox_controller.h:17: // This class sits between the OmniboxEditModel and AutocompleteController. On 2013/04/25 18:24:55, Peter Kasting wrote: > Nit: This comment isn't really informative about what the class is/means/does. > Instead say what the class _is_, from a high-level perspective. I was torn between describing what this class will do once the refactor is complete and what it does now. I've decided to clarify the comment and expand the TODO so that I keep tracking the comment as functionality moves in. Done. https://chromiumcodereview.appspot.com/13932034/diff/29001/chrome/browser/ui/... chrome/browser/ui/omnibox/omnibox_controller.h:27: AutocompleteController* autocomplete_controller() const { On 2013/04/25 18:24:55, Peter Kasting wrote: > Const methods must not return non-const pointers. The exact same method is defined in omnibox_edit_model. I agree it's nasty and we should solve it everywhere. Making it return const breaks things all over the place so I made it non-const. I've added a TODO to see if we can constify it, let me know if you think it's worth it. Done. https://chromiumcodereview.appspot.com/13932034/diff/29001/chrome/browser/ui/... chrome/browser/ui/omnibox/omnibox_controller.h:31: // AutocompleteControllerDelegate: On 2013/04/25 18:24:55, Peter Kasting wrote: > I suggest putting overrides above non-overrides. Done. https://chromiumcodereview.appspot.com/13932034/diff/29001/chrome/browser/ui/... chrome/browser/ui/omnibox/omnibox_controller.h:37: // this and OmniboxEditModel define and use a Delegate interface instead. On 2013/04/25 18:24:55, Peter Kasting wrote: > Why? Is there a real win in defining such an interface when only one class will > ever implement it? It seems like it would just add indirection. Such an interface may make unit tests easier. In the follow-up patch my unit test setup is significantly more complicated because I have to instantiate the full OmniboxEditModel. Far from sure it's worth it though, so for now I've removed the TODO. Done. https://chromiumcodereview.appspot.com/13932034/diff/29001/chrome/browser/ui/... File chrome/browser/ui/omnibox/omnibox_controller_unittest.cc (right): https://chromiumcodereview.appspot.com/13932034/diff/29001/chrome/browser/ui/... chrome/browser/ui/omnibox/omnibox_controller_unittest.cc:18: virtual void SetUp() OVERRIDE { On 2013/04/25 18:24:55, Peter Kasting wrote: > Nit: Do not define virtual (and other non-cheap) functions inline, even in test > files. Done. https://chromiumcodereview.appspot.com/13932034/diff/29001/chrome/browser/ui/... chrome/browser/ui/omnibox/omnibox_controller_unittest.cc:24: profile_.reset(); On 2013/04/25 18:24:55, Peter Kasting wrote: > Nit: Do we actually need to tear these down here instead of in the destructor? > If so, add a comment saying why. Done. https://chromiumcodereview.appspot.com/13932034/diff/29001/chrome/browser/ui/... chrome/browser/ui/omnibox/omnibox_controller_unittest.cc:34: scoped_ptr<TestingProfile> profile_; On 2013/04/25 18:24:55, Peter Kasting wrote: > Google style bans non-private data members in classes. Make these private and > use accessors or define functions on the class to do what you need. Done. https://chromiumcodereview.appspot.com/13932034/diff/29001/chrome/browser/ui/... chrome/browser/ui/omnibox/omnibox_controller_unittest.cc:36: }; On 2013/04/25 18:24:55, Peter Kasting wrote: > Nit: DISALLOW_COPY_AND_ASSIGN Forced me to add an empty constructor though, so I added a virtual destructor too. Done. https://chromiumcodereview.appspot.com/13932034/diff/29001/chrome/browser/ui/... chrome/browser/ui/omnibox/omnibox_controller_unittest.cc:47: ASSERT_EQ(type, expected_providers & type); On 2013/04/25 18:24:55, Peter Kasting wrote: > Nit: What about just ASSERT_TRUE(expected_providers & type);? Done. https://chromiumcodereview.appspot.com/13932034/diff/29001/chrome/browser/ui/... chrome/browser/ui/omnibox/omnibox_controller_unittest.cc:68: AutocompleteProvider::TYPE_SHORTCUTS; On 2013/04/25 19:45:27, Peter Kasting wrote: > On 2013/04/25 19:14:57, beaudoin wrote: > > What if I only checked the difference, instead of looking at what is contained > > in the first set? > > That would improve things. I still feel like it's unnecessary given that we > intend to change the instant set to support the shortcuts provider at some point > (which will require updating this test), and we'll be removing the non-instant > set later as well (which will make the test meaningless). The chances of adding > a new provider before those happen seems miniscule. I'm leaving it in, if only because it tests that the instantiation process actually worked. :) https://chromiumcodereview.appspot.com/13932034/diff/29001/chrome/browser/ui/... File chrome/browser/ui/omnibox/omnibox_edit_model.h (right): https://chromiumcodereview.appspot.com/13932034/diff/29001/chrome/browser/ui/... chrome/browser/ui/omnibox/omnibox_edit_model.h:85: AutocompleteController* autocomplete_controller() const { On 2013/04/25 18:24:55, Peter Kasting wrote: > Ultimately, this accessor will disappear, right? Since this class shouldn't > know about the AutocompleteController. What outside this class uses this right > now? Can we at least make it private? Should we change the public function to > an OmniboxController accessor instead? > > Also, this is another bad case of a const function returning a non-const > pointer. It's used in ui_test_utils.cc. Instead of expanding this CL too much I've added a TODO to remove this once the refactor is done. There are two other const-returning-non-const methods below. *sigh* Done.
LGTM https://chromiumcodereview.appspot.com/13932034/diff/29001/chrome/browser/ui/... File chrome/browser/ui/omnibox/omnibox_controller.h (right): https://chromiumcodereview.appspot.com/13932034/diff/29001/chrome/browser/ui/... chrome/browser/ui/omnibox/omnibox_controller.h:27: AutocompleteController* autocomplete_controller() const { On 2013/04/25 20:34:51, beaudoin wrote: > On 2013/04/25 18:24:55, Peter Kasting wrote: > > Const methods must not return non-const pointers. > > The exact same method is defined in omnibox_edit_model. I agree it's nasty and > we should solve it everywhere. > > Making it return const breaks things all over the place so I made it non-const. > I've added a TODO to see if we can constify it, let me know if you think it's > worth it. I'd probably skip the TODO. I doubt we can avoid the need for a non-const return pointer. https://chromiumcodereview.appspot.com/13932034/diff/29001/chrome/browser/ui/... chrome/browser/ui/omnibox/omnibox_controller.h:37: // this and OmniboxEditModel define and use a Delegate interface instead. On 2013/04/25 20:34:51, beaudoin wrote: > On 2013/04/25 18:24:55, Peter Kasting wrote: > > Why? Is there a real win in defining such an interface when only one class > will > > ever implement it? It seems like it would just add indirection. > > Such an interface may make unit tests easier. In the follow-up patch my unit > test setup is significantly more complicated because I have to instantiate the > full OmniboxEditModel. > > Far from sure it's worth it though, so for now I've removed the TODO. Ah. That's a reasonable justification. Maybe just leave the TODO as saying something like "consider defining a Delegate interface to ease unittesting" or something. https://chromiumcodereview.appspot.com/13932034/diff/44001/chrome/browser/ui/... File chrome/browser/ui/omnibox/omnibox_controller.h (right): https://chromiumcodereview.appspot.com/13932034/diff/44001/chrome/browser/ui/... chrome/browser/ui/omnibox/omnibox_controller.h:18: // responsible of updating the omnibox content. Nit: of -> for https://chromiumcodereview.appspot.com/13932034/diff/44001/chrome/browser/ui/... File chrome/browser/ui/omnibox/omnibox_controller_unittest.cc (right): https://chromiumcodereview.appspot.com/13932034/diff/44001/chrome/browser/ui/... chrome/browser/ui/omnibox/omnibox_controller_unittest.cc:24: TestingProfile& profile() { return profile_; } Nit: It seems like callers only want the PrefService. Change to a getter for that, and return by pointer instead of non-const ref. https://chromiumcodereview.appspot.com/13932034/diff/44001/chrome/browser/ui/... chrome/browser/ui/omnibox/omnibox_controller_unittest.cc:25: OmniboxController* omnibox_controller() { return omnibox_controller_.get(); } Nit: It seems like callers only want the providers list. Change to a getter for that.
https://codereview.chromium.org/13932034/diff/44001/chrome/browser/ui/omnibox... File chrome/browser/ui/omnibox/omnibox_controller.h (right): https://codereview.chromium.org/13932034/diff/44001/chrome/browser/ui/omnibox... chrome/browser/ui/omnibox/omnibox_controller.h:18: // responsible of updating the omnibox content. On 2013/04/25 20:52:09, Peter Kasting wrote: > Nit: of -> for Done. https://codereview.chromium.org/13932034/diff/44001/chrome/browser/ui/omnibox... File chrome/browser/ui/omnibox/omnibox_controller_unittest.cc (right): https://codereview.chromium.org/13932034/diff/44001/chrome/browser/ui/omnibox... chrome/browser/ui/omnibox/omnibox_controller_unittest.cc:24: TestingProfile& profile() { return profile_; } On 2013/04/25 20:52:09, Peter Kasting wrote: > Nit: It seems like callers only want the PrefService. Change to a getter for > that, and return by pointer instead of non-const ref. Done. https://codereview.chromium.org/13932034/diff/44001/chrome/browser/ui/omnibox... chrome/browser/ui/omnibox/omnibox_controller_unittest.cc:25: OmniboxController* omnibox_controller() { return omnibox_controller_.get(); } On 2013/04/25 20:52:09, Peter Kasting wrote: > Nit: It seems like callers only want the providers list. Change to a getter for > that. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/beaudoin@chromium.org/13932034/51001
Failed to apply patch for chrome/chrome_tests_unit.gypi:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file chrome/chrome_tests_unit.gypi
Hunk #1 succeeded at 1445 (offset 26 lines).
Hunk #2 FAILED at 1427.
1 out of 2 hunks FAILED -- saving rejects to file
chrome/chrome_tests_unit.gypi.rej
Patch: chrome/chrome_tests_unit.gypi
Index: chrome/chrome_tests_unit.gypi
diff --git a/chrome/chrome_tests_unit.gypi b/chrome/chrome_tests_unit.gypi
index
241fcbdfffd60384543bd6c8717c16447ccbbe33..14369505d7e114da7dd3edbc45a1ec3ba85f71d5
100644
--- a/chrome/chrome_tests_unit.gypi
+++ b/chrome/chrome_tests_unit.gypi
@@ -1419,6 +1419,7 @@
'browser/ui/gtk/status_icons/status_tray_gtk_unittest.cc',
'browser/ui/gtk/tabs/tab_renderer_gtk_unittest.cc',
'browser/ui/login/login_prompt_unittest.cc',
+ 'browser/ui/omnibox/omnibox_controller_unittest.cc',
'browser/ui/omnibox/omnibox_edit_unittest.cc',
'browser/ui/omnibox/omnibox_view_unittest.cc',
'browser/ui/panels/display_settings_provider_win_unittest.cc',
@@ -1426,7 +1427,7 @@
'browser/ui/search_engines/keyword_editor_controller_unittest.cc',
'browser/ui/search/search_delegate_unittest.cc',
'browser/ui/sync/one_click_signin_helper_unittest.cc',
- 'browser/ui/sync/profile_signin_confirmation_helper_unittest.cc',
+ 'browser/ui/sync/profile_signin_confirmation_helper_unittest.cc',
'browser/ui/tab_contents/tab_contents_iterator_unittest.cc',
'browser/ui/tabs/dock_info_unittest.cc',
'browser/ui/tabs/pinned_tab_codec_unittest.cc',
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/beaudoin@chromium.org/13932034/56001
https://codereview.chromium.org/13932034/diff/29001/chrome/browser/ui/omnibox... File chrome/browser/ui/omnibox/omnibox_controller.h (right): https://codereview.chromium.org/13932034/diff/29001/chrome/browser/ui/omnibox... chrome/browser/ui/omnibox/omnibox_controller.h:27: AutocompleteController* autocomplete_controller() const { On 2013/04/25 20:52:09, Peter Kasting wrote: > On 2013/04/25 20:34:51, beaudoin wrote: > > On 2013/04/25 18:24:55, Peter Kasting wrote: > > > Const methods must not return non-const pointers. > > > > The exact same method is defined in omnibox_edit_model. I agree it's nasty and > > we should solve it everywhere. > > > > Making it return const breaks things all over the place so I made it > non-const. > > I've added a TODO to see if we can constify it, let me know if you think it's > > worth it. > > I'd probably skip the TODO. I doubt we can avoid the need for a non-const > return pointer. Done.
Retried try job too often on linux_chromeos for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/beaudoin@chromium.org/13932034/56001
Message was sent while issue was closed.
Change committed as 196582 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
