Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2613713005:
Make ToolbarActionsModel own ComponentToolbarActionsFactory (Closed)
DescriptionMake ToolbarActionsModel own ComponentToolbarActionsFactory
This CL makes ToolbarActionsModel own ComponentToolbarActionsFactory, which used
to be a stateless singleton.
This change also makes ComponentToolbarActionsFactory keep track of component
actions that call ToolbarActionsModel::AddComponentAction() before
ToolbarActionsModel is initialized. Those actions are added to the set obtained
via ComponentToolbarActionsFactory::GetInitialComponentIds(), and are added to
the toolbar in ToolbarActionsModel::Populate().
MockComponentToolbarActionsFactory and tests are modified to reflect the changes
to ComponentToolbarActionsFactory.
BUG=660972
Review-Url: https://codereview.chromium.org/2613713005
Cr-Commit-Position: refs/heads/master@{#452992}
Committed: https://chromium.googlesource.com/chromium/src/+/e6007d0b43c4a3ee34598349aa3246086f451feb
Patch Set 1 #
Total comments: 48
Patch Set 2 : Rebase with ComponentMigrationHelper removal #Patch Set 3 : Rebase with 2693063003 #Patch Set 4 : Address Devlin and Mike's comments #
Total comments: 4
Patch Set 5 : Address Mike's comments #
Total comments: 1
Patch Set 6 : Add a newline #Patch Set 7 : Fix buildbot failure #
Total comments: 2
Messages
Total messages: 49 (33 generated)
Description was changed from ========== Toolbar Redesign BUG= ========== to ========== Toolbar Redesign BUG=660972 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Toolbar Redesign BUG=660972 ========== to ========== Make ToolbarActionsModel own ComponentToolbarActionsFactory This CL makes ToolbarActionsModel own ComponentToolbarActionsFactory, which used to be a stateless singleton. We are also moving the ownership of ComponentMigrationHelper from ToolbarActionsModel to ComponentToolbarActionsFactory, since ToolbarActionsModel doesn't call any of ComponentMigrationHelper's methods. This change also makes ComponentToolbarActionsFactory keep track of component actions that call ToolbarActionsModel::AddComponentAction() before ToolbarActionsModel is initialized. Those actions can be obtained via ComponentToolbarActionsFactory::GetInitialComponentIds(), and are added to the toolbar in ToolbarActionsModel::Populate(). BUG=660972 ==========
Description was changed from ========== Make ToolbarActionsModel own ComponentToolbarActionsFactory This CL makes ToolbarActionsModel own ComponentToolbarActionsFactory, which used to be a stateless singleton. We are also moving the ownership of ComponentMigrationHelper from ToolbarActionsModel to ComponentToolbarActionsFactory, since ToolbarActionsModel doesn't call any of ComponentMigrationHelper's methods. This change also makes ComponentToolbarActionsFactory keep track of component actions that call ToolbarActionsModel::AddComponentAction() before ToolbarActionsModel is initialized. Those actions can be obtained via ComponentToolbarActionsFactory::GetInitialComponentIds(), and are added to the toolbar in ToolbarActionsModel::Populate(). BUG=660972 ========== to ========== Make ToolbarActionsModel own ComponentToolbarActionsFactory This CL makes ToolbarActionsModel own ComponentToolbarActionsFactory, which used to be a stateless singleton. We are also moving the ownership of ComponentMigrationHelper from ToolbarActionsModel to ComponentToolbarActionsFactory, since ToolbarActionsModel doesn't call any of ComponentMigrationHelper's methods. This change also makes ComponentToolbarActionsFactory keep track of component actions that call ToolbarActionsModel::AddComponentAction() before ToolbarActionsModel is initialized. Those actions can be obtained via ComponentToolbarActionsFactory::GetInitialComponentIds(), and are added to the toolbar in ToolbarActionsModel::Populate(). MockComponentToolbarActionsFactory and tests are modified to reflect the changes to ComponentToolbarActionsFactory. BUG=660972 ==========
Description was changed from ========== Make ToolbarActionsModel own ComponentToolbarActionsFactory This CL makes ToolbarActionsModel own ComponentToolbarActionsFactory, which used to be a stateless singleton. We are also moving the ownership of ComponentMigrationHelper from ToolbarActionsModel to ComponentToolbarActionsFactory, since ToolbarActionsModel doesn't call any of ComponentMigrationHelper's methods. This change also makes ComponentToolbarActionsFactory keep track of component actions that call ToolbarActionsModel::AddComponentAction() before ToolbarActionsModel is initialized. Those actions can be obtained via ComponentToolbarActionsFactory::GetInitialComponentIds(), and are added to the toolbar in ToolbarActionsModel::Populate(). MockComponentToolbarActionsFactory and tests are modified to reflect the changes to ComponentToolbarActionsFactory. BUG=660972 ========== to ========== Make ToolbarActionsModel own ComponentToolbarActionsFactory This CL makes ToolbarActionsModel own ComponentToolbarActionsFactory, which used to be a stateless singleton. We are also moving the ownership of ComponentMigrationHelper from ToolbarActionsModel to ComponentToolbarActionsFactory, since ToolbarActionsModel doesn't call any of ComponentMigrationHelper's methods. This change also makes ComponentToolbarActionsFactory keep track of component actions that call ToolbarActionsModel::AddComponentAction() before ToolbarActionsModel is initialized. Those actions can be obtained via ComponentToolbarActionsFactory::GetInitialComponentIds(), and are added to the toolbar in ToolbarActionsModel::Populate(). MockComponentToolbarActionsFactory and tests are modified to reflect the changes to ComponentToolbarActionsFactory. BUG=660972 ==========
Patchset #1 (id:40001) has been deleted
takumif@chromium.org changed reviewers: + apacible@chromium.org, msw@chromium.org, rdevlin.cronin@chromium.org
Please take a look, thank you!
When citing multiple reviewers, please clarify which portion of the CL each are responsible for reviewing. I also wonder if this CL could be split up into separate changes for ease of review.
https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/component_toolbar_actions_factory.h (right): https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/component_toolbar_actions_factory.h:47: // Returns a collection of controllers for component actions. Declared not this patch, but this is out of date (it's not a collection) https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/component_toolbar_actions_factory.h:65: extensions::ComponentMigrationHelper migration_helper_; If we have the accessor above, why does this need to be protected (instead of private)? https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/toolbar_actions_model.cc (right): https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_model.cc:642: component_actions_factory_->OnAddComponentActionBeforeInit(action_id); I'm not entirely sure this is less convoluted than before... it seems like we still have this strange triangle of ToolbarActionsModel ComponentToolbarActionsFactory ComponentMigrationHelper where each has to know about the other in some way, shape, or form (though they might be hidden behind delegate interfaces). I think I'd prefer it if we could make these a little more unidirectional. What if instead, we have ComponentToolbarActionsFactory own the ComponentMigrationHelper (like you have), but also implement its delegate interface? And then have the factory call directly into ToolbarActionsModel to add component actions, and have the model have the caching mechanism. Then, we have a little more orderly flow of knowledge. Does that make sense? WDYT? (I'm brainstorming right now, so lemme know if I'm totally off base :)) https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/toolbar_actions_model.h (right): https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_model.h:209: // |mock_factory| is transferred to |this|. if ownership is transferred, use a unique_ptr (and then the comment is unnecessary).
although separately, there's discussion on the bug about just removing the migration helper - which sounds best of all :) Do we need this, or can we skip a step and just remove the code?
+1 for removing the migration code if possible. I don't understand this code terribly well, hopefully another reviewer does. https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/component_toolbar_actions_browsertest.cc (left): https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/component_toolbar_actions_browsertest.cc:36: std::unique_ptr<extensions::FeatureSwitch::ScopedOverride> enable_redesign_; Is this no longer needed? https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/component_toolbar_actions_browsertest.cc:57: EXPECT_EQ(1u, ComponentToolbarActionsFactory::GetInstance() Should GetInitialComponentIds still be checked in this test? https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/component_toolbar_actions_browsertest.cc (right): https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/component_toolbar_actions_browsertest.cc:27: ToolbarActionsModel::Get(browser()->profile()) optional nit: cache a local profile pointer https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc (right): https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc:83: if (media_router::MediaRouterEnabled(profile_)) { optional nit: curlies not needed https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/component_toolbar_actions_factory.h (right): https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/component_toolbar_actions_factory.h:21: class ComponentMigrationHelper; nit: remove https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/component_toolbar_actions_factory.h:32: ComponentToolbarActionsFactory( nit: comment (explain params, especially the action delegate) https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/component_toolbar_actions_factory.h:65: extensions::ComponentMigrationHelper migration_helper_; Why is this protected if there's a public accessor? https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/component_toolbar_actions_factory_unittest.cc (right): https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/component_toolbar_actions_factory_unittest.cc:25: ComponentToolbarActionsFactory* actions_factory_; nit: = nullptr https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.cc (right): https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.cc:47: if (!migrated_extension_id_.empty()) { optional nit: curlies not needed https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.cc:56: if (migrated_feature_enabled_) { optional nit: curlies not needed https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.h (right): https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.h:30: nit: no blank lines between overrides for the same base class https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.h:32: ditto nit https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/toolbar_actions_model.h (right): https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_model.h:15: #include "chrome/browser/extensions/component_migration_helper.h" nit: remove this include and use a forward declaration instead, if possible https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_model.h:137: extensions::ComponentMigrationHelper* GetComponentMigrationHelper(); Is this necessary if the ComponentToolbarActionsFactory is exposed, and that exposes the migration helper? If so, nit: add a comment here https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_model.h:208: // Swaps |component_actions_factory_| with |mock_factory|. The ownership of nit: s/Swaps/Replaces/ or something like: "Set the component action factory for this object; transfers ownership." https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_model.h:210: void SetMockActionsFactory(ComponentToolbarActionsFactory* mock_factory); Ownership transfers should usually be done with unique_ptr. https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_model.h:210: void SetMockActionsFactory(ComponentToolbarActionsFactory* mock_factory); nit: append ForTest[ing] to function name (or remove 'mock' from param name) https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc (right): https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc:259: void ToolbarActionsModelUnitTest::InitWithMockActionsFactory() { nit: this is called once, maybe inline the functionality there? (or find a way to consolidate some InitForMigrationTest code with this) https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc:264: new MockComponentToolbarActionsFactory(profile(), toolbar_model_); Use base::MakeUnique https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc:269: MockComponentToolbarActionsFactory* mock_actions_factory) { Use unique_ptr for ownership passing
Thank you for the reviews. You two are right, this CL would be simpler after removing the ComponentMigrationHelper first. I'll come back to this after that.
Description was changed from ========== Make ToolbarActionsModel own ComponentToolbarActionsFactory This CL makes ToolbarActionsModel own ComponentToolbarActionsFactory, which used to be a stateless singleton. We are also moving the ownership of ComponentMigrationHelper from ToolbarActionsModel to ComponentToolbarActionsFactory, since ToolbarActionsModel doesn't call any of ComponentMigrationHelper's methods. This change also makes ComponentToolbarActionsFactory keep track of component actions that call ToolbarActionsModel::AddComponentAction() before ToolbarActionsModel is initialized. Those actions can be obtained via ComponentToolbarActionsFactory::GetInitialComponentIds(), and are added to the toolbar in ToolbarActionsModel::Populate(). MockComponentToolbarActionsFactory and tests are modified to reflect the changes to ComponentToolbarActionsFactory. BUG=660972 ========== to ========== Make ToolbarActionsModel own ComponentToolbarActionsFactory This CL makes ToolbarActionsModel own ComponentToolbarActionsFactory, which used to be a stateless singleton. This change also makes ComponentToolbarActionsFactory keep track of component actions that call ToolbarActionsModel::AddComponentAction() before ToolbarActionsModel is initialized. Those actions can be obtained via ComponentToolbarActionsFactory::GetInitialComponentIds(), and are added to the toolbar in ToolbarActionsModel::Populate(). MockComponentToolbarActionsFactory and tests are modified to reflect the changes to ComponentToolbarActionsFactory. BUG=660972 ==========
Patchset #3 (id:90001) has been deleted
Description was changed from ========== Make ToolbarActionsModel own ComponentToolbarActionsFactory This CL makes ToolbarActionsModel own ComponentToolbarActionsFactory, which used to be a stateless singleton. This change also makes ComponentToolbarActionsFactory keep track of component actions that call ToolbarActionsModel::AddComponentAction() before ToolbarActionsModel is initialized. Those actions can be obtained via ComponentToolbarActionsFactory::GetInitialComponentIds(), and are added to the toolbar in ToolbarActionsModel::Populate(). MockComponentToolbarActionsFactory and tests are modified to reflect the changes to ComponentToolbarActionsFactory. BUG=660972 ========== to ========== Make ToolbarActionsModel own ComponentToolbarActionsFactory This CL makes ToolbarActionsModel own ComponentToolbarActionsFactory, which used to be a stateless singleton. This change also makes ComponentToolbarActionsFactory keep track of component actions that call ToolbarActionsModel::AddComponentAction() before ToolbarActionsModel is initialized. Those actions are added to the set obtained via ComponentToolbarActionsFactory::GetInitialComponentIds(), and are added to the toolbar in ToolbarActionsModel::Populate(). MockComponentToolbarActionsFactory and tests are modified to reflect the changes to ComponentToolbarActionsFactory. BUG=660972 ==========
Patchset #4 (id:130001) has been deleted
takumif@chromium.org changed reviewers: + pastarmovj@chromium.org
Now that ComponentMigrationHelper has been removed and this CL has been simplified, I'd like to ask for a review again. Please take a look at: apacible: overall msw: media_router_contextual_menu.cc (I feel like our team should just own the *media_router* files) pastarmovj: policy_browsertest.cc rdcronin: *toolbar_action* Thank you! https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/component_toolbar_actions_browsertest.cc (left): https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/component_toolbar_actions_browsertest.cc:36: std::unique_ptr<extensions::FeatureSwitch::ScopedOverride> enable_redesign_; On 2017/01/10 23:23:59, msw wrote: > Is this no longer needed? No, removing. https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/component_toolbar_actions_browsertest.cc:57: EXPECT_EQ(1u, ComponentToolbarActionsFactory::GetInstance() On 2017/01/10 23:23:59, msw wrote: > Should GetInitialComponentIds still be checked in this test? This was just to make sure that the MockComponentToolbarActionsFactory returns a test action as expected. I've changed to explicitly adding the action by calling AddComponentAction() above, so this is no longer necessary. https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/component_toolbar_actions_browsertest.cc (right): https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/component_toolbar_actions_browsertest.cc:27: ToolbarActionsModel::Get(browser()->profile()) On 2017/01/10 23:23:59, msw wrote: > optional nit: cache a local profile pointer Done. https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc (right): https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc:83: if (media_router::MediaRouterEnabled(profile_)) { On 2017/01/10 23:23:59, msw wrote: > optional nit: curlies not needed Removed migration-related code. https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/component_toolbar_actions_factory.h (right): https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/component_toolbar_actions_factory.h:21: class ComponentMigrationHelper; On 2017/01/10 23:23:59, msw wrote: > nit: remove Removed migration-related code. https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/component_toolbar_actions_factory.h:32: ComponentToolbarActionsFactory( On 2017/01/10 23:23:59, msw wrote: > nit: comment (explain params, especially the action delegate) This class no longer needs a reference to the action delegate, so removing. https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/component_toolbar_actions_factory.h:47: // Returns a collection of controllers for component actions. Declared On 2017/01/10 23:07:11, Devlin wrote: > not this patch, but this is out of date (it's not a collection) Updated the comment. https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/component_toolbar_actions_factory.h:65: extensions::ComponentMigrationHelper migration_helper_; On 2017/01/10 23:23:59, msw wrote: > Why is this protected if there's a public accessor? Removed migration-related code. https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/component_toolbar_actions_factory.h:65: extensions::ComponentMigrationHelper migration_helper_; On 2017/01/10 23:07:11, Devlin wrote: > If we have the accessor above, why does this need to be protected (instead of > private)? Removed migration-related code. https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/component_toolbar_actions_factory_unittest.cc (right): https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/component_toolbar_actions_factory_unittest.cc:25: ComponentToolbarActionsFactory* actions_factory_; On 2017/01/10 23:23:59, msw wrote: > nit: = nullptr Done. https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.cc (right): https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.cc:47: if (!migrated_extension_id_.empty()) { On 2017/01/10 23:23:59, msw wrote: > optional nit: curlies not needed Removed migration-related code. https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.cc:56: if (migrated_feature_enabled_) { On 2017/01/10 23:23:59, msw wrote: > optional nit: curlies not needed Removed migration-related code. https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.h (right): https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.h:30: On 2017/01/10 23:23:59, msw wrote: > nit: no blank lines between overrides for the same base class Removed migration-related code. https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.h:32: On 2017/01/10 23:23:59, msw wrote: > ditto nit Removed migration-related code. https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/toolbar_actions_model.cc (right): https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_model.cc:642: component_actions_factory_->OnAddComponentActionBeforeInit(action_id); On 2017/01/10 23:07:11, Devlin wrote: > I'm not entirely sure this is less convoluted than before... it seems like we > still have this strange triangle of > ToolbarActionsModel > ComponentToolbarActionsFactory ComponentMigrationHelper > > where each has to know about the other in some way, shape, or form (though they > might be hidden behind delegate interfaces). I think I'd prefer it if we could > make these a little more unidirectional. > > What if instead, we have ComponentToolbarActionsFactory own the > ComponentMigrationHelper (like you have), but also implement its delegate > interface? And then have the factory call directly into ToolbarActionsModel to > add component actions, and have the model have the caching mechanism. Then, we > have a little more orderly flow of knowledge. Does that make sense? WDYT? > (I'm brainstorming right now, so lemme know if I'm totally off base :)) I think things are a bit less complex after the removal of CompomentMigrationHelper. ToolbarActionsModel owns ComponentToolbarActionsFactory, and ComponentToolbarActionsFactory has a reference to ToolbarActionsModel through ComponentActionDelegate interface. https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/toolbar_actions_model.h (right): https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_model.h:15: #include "chrome/browser/extensions/component_migration_helper.h" On 2017/01/10 23:23:59, msw wrote: > nit: remove this include and use a forward declaration instead, if possible Done. https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_model.h:137: extensions::ComponentMigrationHelper* GetComponentMigrationHelper(); On 2017/01/10 23:23:59, msw wrote: > Is this necessary if the ComponentToolbarActionsFactory is exposed, and that > exposes the migration helper? If so, nit: add a comment here Removed migration-related code. https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_model.h:208: // Swaps |component_actions_factory_| with |mock_factory|. The ownership of On 2017/01/10 23:23:59, msw wrote: > nit: s/Swaps/Replaces/ or something like: "Set the component action factory for > this object; transfers ownership." Done. https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_model.h:209: // |mock_factory| is transferred to |this|. On 2017/01/10 23:07:11, Devlin wrote: > if ownership is transferred, use a unique_ptr (and then the comment is > unnecessary). Done. https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_model.h:210: void SetMockActionsFactory(ComponentToolbarActionsFactory* mock_factory); On 2017/01/10 23:23:59, msw wrote: > Ownership transfers should usually be done with unique_ptr. Done. https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_model.h:210: void SetMockActionsFactory(ComponentToolbarActionsFactory* mock_factory); On 2017/01/10 23:23:59, msw wrote: > nit: append ForTest[ing] to function name (or remove 'mock' from param name) Done. https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc (right): https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc:259: void ToolbarActionsModelUnitTest::InitWithMockActionsFactory() { On 2017/01/10 23:23:59, msw wrote: > nit: this is called once, maybe inline the functionality there? > (or find a way to consolidate some InitForMigrationTest code with this) The test that calls this was already very long, so I didn't want to make it longer. https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc:264: new MockComponentToolbarActionsFactory(profile(), toolbar_model_); On 2017/01/10 23:23:59, msw wrote: > Use base::MakeUnique Done. https://codereview.chromium.org/2613713005/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc:269: MockComponentToolbarActionsFactory* mock_actions_factory) { On 2017/01/10 23:23:59, msw wrote: > Use unique_ptr for ownership passing Done.
policy_browsertest LGTM.
lgtm
Thanks for the cleanup and patience with my review. I just one question and one minor nit. https://codereview.chromium.org/2613713005/diff/150001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc (right): https://codereview.chromium.org/2613713005/diff/150001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc:39: initial_ids_.insert(kMediaRouterActionId); If this function is called multiple times, wouldn't this add the media router id to |initial_ids_| each time? (perhaps do this in the ctor instead?) https://codereview.chromium.org/2613713005/diff/150001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/component_toolbar_actions_factory.h (right): https://codereview.chromium.org/2613713005/diff/150001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/component_toolbar_actions_factory.h:65: Profile* profile_ = nullptr; nit: sorry if I suggested this |= nullptr;|, it's not necessary, since you assign a value in the ctor initializer list.
https://codereview.chromium.org/2613713005/diff/150001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc (right): https://codereview.chromium.org/2613713005/diff/150001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc:39: initial_ids_.insert(kMediaRouterActionId); On 2017/02/17 23:55:05, msw wrote: > If this function is called multiple times, wouldn't this add the media router id > to |initial_ids_| each time? (perhaps do this in the ctor instead?) I thought it'd be fine since it's a set, but yes, it'd be better if we didn't have to do this check multiple times. Moving this to the ctor as suggested. https://codereview.chromium.org/2613713005/diff/150001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/component_toolbar_actions_factory.h (right): https://codereview.chromium.org/2613713005/diff/150001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/component_toolbar_actions_factory.h:65: Profile* profile_ = nullptr; On 2017/02/17 23:55:05, msw wrote: > nit: sorry if I suggested this |= nullptr;|, it's not necessary, since you > assign a value in the ctor initializer list. Ah, right. Removed.
lgtm with a nit https://codereview.chromium.org/2613713005/diff/170001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc (right): https://codereview.chromium.org/2613713005/diff/170001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc:40: ComponentToolbarActionsFactory::~ComponentToolbarActionsFactory() {} nit: add a blank line above
The CQ bit was checked by takumif@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by takumif@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by takumif@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2613713005/diff/210001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_contextual_menu.cc (right): https://codereview.chromium.org/2613713005/diff/210001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_contextual_menu.cc:30: #include "ui/gfx/vector_icons_public.h" Is this needed?
https://codereview.chromium.org/2613713005/diff/210001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_contextual_menu.cc (right): https://codereview.chromium.org/2613713005/diff/210001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_contextual_menu.cc:30: #include "ui/gfx/vector_icons_public.h" On 2017/02/24 19:51:22, apacible wrote: > Is this needed? Yes, this confused me a bit too. This file included component_toolbar_actions_factory.h that included toolbar_actions_bar.h and so on to vector_icons_public.h, but I forward declared ToolbarActionsBar there, so this became necessary.
The CQ bit was checked by takumif@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by takumif@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org, pastarmovj@chromium.org, msw@chromium.org Link to the patchset: https://codereview.chromium.org/2613713005/#ps210001 (title: "Fix buildbot failure")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 210001, "attempt_start_ts": 1487978465688990,
"parent_rev": "85fff922d53f16ad66d8d39cb066cbcda1640052", "commit_rev":
"e6007d0b43c4a3ee34598349aa3246086f451feb"}
Message was sent while issue was closed.
Description was changed from ========== Make ToolbarActionsModel own ComponentToolbarActionsFactory This CL makes ToolbarActionsModel own ComponentToolbarActionsFactory, which used to be a stateless singleton. This change also makes ComponentToolbarActionsFactory keep track of component actions that call ToolbarActionsModel::AddComponentAction() before ToolbarActionsModel is initialized. Those actions are added to the set obtained via ComponentToolbarActionsFactory::GetInitialComponentIds(), and are added to the toolbar in ToolbarActionsModel::Populate(). MockComponentToolbarActionsFactory and tests are modified to reflect the changes to ComponentToolbarActionsFactory. BUG=660972 ========== to ========== Make ToolbarActionsModel own ComponentToolbarActionsFactory This CL makes ToolbarActionsModel own ComponentToolbarActionsFactory, which used to be a stateless singleton. This change also makes ComponentToolbarActionsFactory keep track of component actions that call ToolbarActionsModel::AddComponentAction() before ToolbarActionsModel is initialized. Those actions are added to the set obtained via ComponentToolbarActionsFactory::GetInitialComponentIds(), and are added to the toolbar in ToolbarActionsModel::Populate(). MockComponentToolbarActionsFactory and tests are modified to reflect the changes to ComponentToolbarActionsFactory. BUG=660972 Review-Url: https://codereview.chromium.org/2613713005 Cr-Commit-Position: refs/heads/master@{#452992} Committed: https://chromium.googlesource.com/chromium/src/+/e6007d0b43c4a3ee34598349aa32... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:210001) as https://chromium.googlesource.com/chromium/src/+/e6007d0b43c4a3ee34598349aa32... |
