Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1094493003:
Initial RemoteCommandsInvalidator (Closed)
|
Created:
5 years, 8 months ago by binjin Modified:
5 years, 7 months ago Reviewers:
bartfab (slow) CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@lkcr Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionInitial RemoteCommandsInvalidator
This CL adds the invalidator class for remote commands service.
BUG=480982
Committed: https://crrev.com/9b94f2db07ffd0bad793e960f7a2d1cb428db7ea
Cr-Commit-Position: refs/heads/master@{#330114}
Patch Set 1 #Patch Set 2 : Initial RemoteCommandsInvalidator #Patch Set 3 : fixes #Patch Set 4 : more fixes #Patch Set 5 : more #Patch Set 6 : minor fixes #Patch Set 7 : fixes, add unit tests #Patch Set 8 : all unit tests, fix lints #
Total comments: 135
Patch Set 9 : fixes addressing #3, except classname/filepath change #
Total comments: 30
Patch Set 10 : rename / move files #
Total comments: 18
Patch Set 11 : fixes addressing #5 #
Total comments: 12
Patch Set 12 : fixes addressing #7 and #8 #
Total comments: 3
Messages
Total messages: 16 (2 generated)
binjin@chromium.org changed reviewers: + bartfab@chromium.org
A few general observations: 1) Class names ending in Base are typically used when you want to provide basic functionality to be shared by several sub-classes. In your case, there is only one sub-class (RemoteCommandsInvalidator) and a mock. The canonical naming pattern in this case would be: RemoteCommandsInvalidatorBase -> RemoteCommandsInvalidator RemoteCommandsInvalidator -> RemoteCommandsInvalidatorImpl MockRemoteCommandsInvalidator -> stays as is 2) We are slowly switching from gyp to gn. Please check whether any of the .gypi files you modified have corresponding .gn files and if so, update these as well. 3) Please file a bug report to remind us to add a browser test for this. No need to add one in this CL, but important not to lose track of it. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h (right): https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/chromeo... chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h:38: class AffiliatedRemoteCommandsInvalidator; Please keep this list alphabetic (as best as you can, after others already broke the alphabetic order somewhat :). https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/remote_commands/remote_commands_invalidator.cc (right): https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/chromeo... chrome/browser/chromeos/policy/remote_commands/remote_commands_invalidator.cc:9: #include "components/policy/core/common/cloud/cloud_policy_core.h" Nit: Already included by header. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/chromeo... chrome/browser/chromeos/policy/remote_commands/remote_commands_invalidator.cc:20: RemoteCommandsInvalidator::~RemoteCommandsInvalidator() { Nit: No need to override the destructor if you are happy with the default. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/chromeo... chrome/browser/chromeos/policy/remote_commands/remote_commands_invalidator.cc:35: core_->store()->AddObserver(this); Nit: Here, you simulate an event one-off and then become a subscriber. In OnInitialize(), you did the same thing in opposite order. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/chromeo... chrome/browser/chromeos/policy/remote_commands/remote_commands_invalidator.cc:91: DCHECK(!invalidator_); Nit: You can easily fold this method into OnInvalidationServiceSet(). https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/remote_commands/remote_commands_invalidator.h (right): https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/chromeo... chrome/browser/chromeos/policy/remote_commands/remote_commands_invalidator.h:17: // XXX Nit: Missing comment. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/chromeo... chrome/browser/chromeos/policy/remote_commands/remote_commands_invalidator.h:18: class RemoteCommandsInvalidator : public RemoteCommandsInvalidatorBase, This file should be split. The |RemoteCommandsInvalidator| will be used for per-user commands on Chrome OS and other platforms as well. It should reside in "chrome/browser/policy." The |AffiliatedRemoteCommandsInvalidator| is Chrome OS-specific and should stay here. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/chromeo... chrome/browser/chromeos/policy/remote_commands/remote_commands_invalidator.h:48: class AffiliatedRemoteCommandsInvalidator I know you are following an existing naming pattern here but I am not sure the pattern is ideal. |AffiliatedRemoteCommandsInvalidator| sounds like it inherits from |RemoteCommandsInvalidator| while in fact, it is a wrapper around one. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/chromeo... chrome/browser/chromeos/policy/remote_commands/remote_commands_invalidator.h:62: invalidation::InvalidationService* invalidation_service); Nit: Forward-declare |invalidation::InvalidationService|. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... File chrome/browser/policy/cloud/remote_commands_invalidator_base.cc (right): https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.cc:15: #include "components/policy/core/common/remote_commands/remote_commands_service.h" Nit: Not used. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.cc:21: : state_(SHUT_DOWN), task_runner_(task_runner), weak_factory_(this) { Nit: Why not initialize |state_| on declaration, like you do with all the other POD members? https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.cc:22: DCHECK(task_runner.get()); Nit: s/.get()// https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.cc:46: Stop(); Since Stop() handles |state_ != STOPPED| gracefully, you could just unconditionally call it here. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.cc:78: invalidation_service_enabled_ = state == syncer::INVALIDATIONS_ENABLED; Nit: #include "components/invalidation/invalidator_state.h" https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.cc:99: it.Acknowledge(); Nit: #include "components/invalidation/invalidation.h" https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.cc:101: task_runner_->PostTask( Why can we not run this synchronously? https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.cc:108: return "RemoteCommands"; Nit: #include <string> https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.cc:125: invalidation::ObjectId object_id(policy->command_invalidation_source(), Nit: const. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.cc:130: if (!is_registered_ || !(object_id == object_id_)) Nit: Is there no operator != for object IDs? https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.cc:142: is_registered_ = true; Nit: Move this inside the conditional above. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... File chrome/browser/policy/cloud/remote_commands_invalidator_base.h (right): https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.h:8: #include <string> Nit: Not used (only used in overriden method declaration and for that, you should not include it). https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.h:14: #include "components/invalidation/invalidation.h" Nit: Not used. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.h:28: class CloudPolicyCore; Nit: Not used. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.h:30: // XXX I presume you will add comments before this lands :). https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.h:34: RemoteCommandsInvalidatorBase( Nit: explicit. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.h:35: const scoped_refptr<base::SequencedTaskRunner>& task_runner); What is this |task_runner| used for? If it is used for running background tasks on another thread, rename it to |background_task_runner| instead. If it is meant for posting asynchronous tasks on the current thread, why not just grab it from ThreadTaskRunnerHandle::Get() instead of passing it in? https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.h:63: virtual void OnInitialize() {} Nit 1: The style guide forbids inlining virtual methods. Nit 2: Why do you provide empty default implementations for these four methods if every sub-class actually overrides them? https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.h:74: void Register(const invalidation::ObjectId& object_id); Nit: #include "google/cacheinvalidation/include/types.h" https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.h:79: // Updates invalidations_enabled_ and calls the invalidation handler if the Which handler are you talking about here? I cannot see any handlers other than |this|. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... File chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc (right): https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:5: #include <string> Nit: Not used. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:7: #include "base/macros.h" Nit: Already included by header. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:8: #include "base/memory/ref_counted.h" Nit: Already included by header. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:12: #include "chrome/browser/policy/cloud/remote_commands_invalidator_base.h" Nit: Make this the first included, followed by a blank line. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:15: #include "policy/proto/device_management_backend.pb.h" Nit: Already included by header. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:21: using ::testing::InSequence; Nit: Not actually needed. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:30: const scoped_refptr<base::SequencedTaskRunner>& task_runner) Nit: #include "base/sequenced_task_runner.h" (scoped_refptr requires the type to be defined to compile, even if you never dereference it) https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:32: ~MockRemoteCommandInvalidator() override {} Nit: No need to override the destructor here at all. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:41: scoped_ptr<em::PolicyData> policy_data(new em::PolicyData()); Why do you not just use a stack-allocated |em::PolicyData| variable? https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:48: scoped_ptr<em::PolicyData> policy_data(new em::PolicyData()); As above, why not just allcoate the |em::PolicyData| on the stack? https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:61: invalidation_service_(new invalidation::FakeInvalidationService), Nit: Why does this need to be a scoped_ptr? Why can you not just have a |invalidation::FakeInvalidationService| member directly? https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:62: invalidator_(new StrictMock<MockRemoteCommandInvalidator>( Nit: Why does this need to be a scoped_ptr? Why can you not just have a |StrictMock<MockRemoteCommandInvalidator>| member directly? https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:63: loop_.message_loop_proxy())) {} Nit 1: MessageLoopProxy is deprecated. Use base::ThreadTaskRunnerHandle::Get() instead. Nit 2: When you have a multi-line inline method definition, the {} should be split over two lines. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:66: invalidation_service_->SetInvalidatorState(syncer::INVALIDATIONS_ENABLED); Nit: #include "components/invalidation/invalidator_state.h" https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:76: auto invalidation = syncer::Invalidation::InitUnknownVersion(object_id); Nit 1: Do not use |auto| here. We only use it in tight loops where it significantly improves readability, not to obscure types in general. Nit 2: const. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:82: return !invalidation_service_->GetMockAckHandler()->IsUnsent(invalidation); Nit: #include "components/invalidation/mock_ack_handler.h" https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:91: return !invalidation_service_->invalidator_registrar() Nit: #include "components/invalidation/invalidator_registrar.h" https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:105: EXPECT_CALL(*invalidator_, OnStart()).Times(1); GMock does not allow mixing EXPECT_CALL() with invocations. You must always do a VerifyAndClearExpectations() before setting the next EXPECT_CALL() after a method invocation. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:120: // Fire an invalidation to verify that invalidaion is not working. Nit: s/invalidaion/invalidation/ https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:121: void VerifiesInvalidationDisabled(const invalidation::ObjectId& object_id) { Nit: s/Verifies/Verify/ https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:122: VerifyExpectations(); Why do you need to do this? Does any of the tests call this method without verifying pending expectations first? https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:126: syncer::Invalidation inv = FireInvalidation(object_id); Nit 1: Avoid abbreviaitons like |inv|. Nit 2: const. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:128: loop_.RunUntilIdle(); Nit: Here and throughout the file: Never spin the loop directly. Use RunLoop().RunUntilIdle(). https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:130: VerifyExpectations(); What expectations are you verifying here? https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:133: // Fire an invalidation to verify that invalidaion works. Nit: s/invalidaion/invalidation/ https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:134: void VerifiesInvalidationEnabled(const invalidation::ObjectId& object_id) { Nit: s/Verifies/Verify/ https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:135: VerifyExpectations(); Why do you need to do this? Does any of the tests call this method without verifying pending expectations first? https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:140: syncer::Invalidation inv = FireInvalidation(object_id); Nit 1: Avoid abbreviaitons like |inv|. Nit 2: const. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:162: InSequence dummy; Nit: There is no need for a sequence in this test. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:174: DisableInvalidationService(); Nit: The disabling and re-enabling is already being tested by InvalidationServiceEnabledStateChanged(). Just move the EXPECT_*(invalidator_->invalidations_enabled()) to that test and remove the disabling and re-enabling here. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:182: VerifyExpectations(); Nit 1: Should you not spin the loop before verifying this, to make sure there is no pending task that receives a spurious invalidation? Nit 2: This VerifyExpectations() is superfluous. The one in line 190 is sufficient to catch any unexpected callbacks. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:186: syncer::Invalidation inv1 = FireInvalidation(kTestingObjectId2); Nit 1: Avoid abbreviaitons like |inv1|. Nit 2: const. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:259: // Verifies that registered state changes works as expected. Nit: s/works/work/ https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:269: VerifiesInvalidationEnabled(kTestingObjectId2); Nit: Maybe double-check that |kTestingObjectId1| is disabled now? https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:272: VerifiesInvalidationEnabled(kTestingObjectId1); Nit: Maybe double-check that |kTestingObjectId2| is disabled now? https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:278: VerifiesInvalidationEnabled(kTestingObjectId2); Nit: Maybe double-check that |kTestingObjectId1| is disabled now? https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:294: VerifiesInvalidationDisabled(kTestingObjectId2); Here and below: Why are you sometimes testing the state of |kTestingObjectId2| instead of |kTestingObjectId1|? https://codereview.chromium.org/1094493003/diff/130001/components/policy/core... File components/policy/core/common/cloud/cloud_policy_core.h (right): https://codereview.chromium.org/1094493003/diff/130001/components/policy/core... components/policy/core/common/cloud/cloud_policy_core.h:55: virtual void OnRemoteCommandsServiceStarted(CloudPolicyCore* core) {} Why is this not pure virtual, like the other methoda above?
https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h (right): https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/chromeo... chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h:38: class AffiliatedRemoteCommandsInvalidator; On 2015/05/15 13:28:07, bartfab wrote: > Please keep this list alphabetic (as best as you can, after others already broke > the alphabetic order somewhat :). Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/remote_commands/remote_commands_invalidator.cc (right): https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/chromeo... chrome/browser/chromeos/policy/remote_commands/remote_commands_invalidator.cc:9: #include "components/policy/core/common/cloud/cloud_policy_core.h" On 2015/05/15 13:28:07, bartfab wrote: > Nit: Already included by header. Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/chromeo... chrome/browser/chromeos/policy/remote_commands/remote_commands_invalidator.cc:20: RemoteCommandsInvalidator::~RemoteCommandsInvalidator() { On 2015/05/15 13:28:07, bartfab wrote: > Nit: No need to override the destructor if you are happy with the default. Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/chromeo... chrome/browser/chromeos/policy/remote_commands/remote_commands_invalidator.cc:35: core_->store()->AddObserver(this); On 2015/05/15 13:28:07, bartfab wrote: > Nit: Here, you simulate an event one-off and then become a subscriber. In > OnInitialize(), you did the same thing in opposite order. Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/chromeo... chrome/browser/chromeos/policy/remote_commands/remote_commands_invalidator.cc:91: DCHECK(!invalidator_); On 2015/05/15 13:28:07, bartfab wrote: > Nit: You can easily fold this method into OnInvalidationServiceSet(). Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/remote_commands/remote_commands_invalidator.h (right): https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/chromeo... chrome/browser/chromeos/policy/remote_commands/remote_commands_invalidator.h:17: // XXX On 2015/05/15 13:28:07, bartfab wrote: > Nit: Missing comment. Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/chromeo... chrome/browser/chromeos/policy/remote_commands/remote_commands_invalidator.h:48: class AffiliatedRemoteCommandsInvalidator On 2015/05/15 13:28:07, bartfab wrote: > I know you are following an existing naming pattern here but I am not sure the > pattern is ideal. |AffiliatedRemoteCommandsInvalidator| sounds like it inherits > from |RemoteCommandsInvalidator| while in fact, it is a wrapper around one. Acknowledged. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/chromeo... chrome/browser/chromeos/policy/remote_commands/remote_commands_invalidator.h:62: invalidation::InvalidationService* invalidation_service); On 2015/05/15 13:28:07, bartfab wrote: > Nit: Forward-declare |invalidation::InvalidationService|. Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... File chrome/browser/policy/cloud/remote_commands_invalidator_base.cc (right): https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.cc:15: #include "components/policy/core/common/remote_commands/remote_commands_service.h" On 2015/05/15 13:28:07, bartfab wrote: > Nit: Not used. Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.cc:21: : state_(SHUT_DOWN), task_runner_(task_runner), weak_factory_(this) { On 2015/05/15 13:28:07, bartfab wrote: > Nit: Why not initialize |state_| on declaration, like you do with all the other > POD members? Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.cc:22: DCHECK(task_runner.get()); On 2015/05/15 13:28:07, bartfab wrote: > Nit: s/.get()// Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.cc:46: Stop(); On 2015/05/15 13:28:08, bartfab wrote: > Since Stop() handles |state_ != STOPPED| gracefully, you could just > unconditionally call it here. Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.cc:78: invalidation_service_enabled_ = state == syncer::INVALIDATIONS_ENABLED; On 2015/05/15 13:28:08, bartfab wrote: > Nit: #include "components/invalidation/invalidator_state.h" Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.cc:99: it.Acknowledge(); On 2015/05/15 13:28:07, bartfab wrote: > Nit: #include "components/invalidation/invalidation.h" Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.cc:101: task_runner_->PostTask( On 2015/05/15 13:28:07, bartfab wrote: > Why can we not run this synchronously? It's mainly due to the messageloopproxy used in unit tests. Also maybe there are need to use PostDelayedTask later. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.cc:108: return "RemoteCommands"; On 2015/05/15 13:28:08, bartfab wrote: > Nit: #include <string> Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.cc:125: invalidation::ObjectId object_id(policy->command_invalidation_source(), On 2015/05/15 13:28:07, bartfab wrote: > Nit: const. Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.cc:130: if (!is_registered_ || !(object_id == object_id_)) On 2015/05/15 13:28:07, bartfab wrote: > Nit: Is there no operator != for object IDs? Yes, it's not implemented. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.cc:142: is_registered_ = true; On 2015/05/15 13:28:08, bartfab wrote: > Nit: Move this inside the conditional above. Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... File chrome/browser/policy/cloud/remote_commands_invalidator_base.h (right): https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.h:8: #include <string> On 2015/05/15 13:28:08, bartfab wrote: > Nit: Not used (only used in overriden method declaration and for that, you > should not include it). Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.h:14: #include "components/invalidation/invalidation.h" On 2015/05/15 13:28:08, bartfab wrote: > Nit: Not used. Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.h:28: class CloudPolicyCore; On 2015/05/15 13:28:08, bartfab wrote: > Nit: Not used. Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.h:30: // XXX On 2015/05/15 13:28:08, bartfab wrote: > I presume you will add comments before this lands :). Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.h:34: RemoteCommandsInvalidatorBase( On 2015/05/15 13:28:08, bartfab wrote: > Nit: explicit. Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.h:35: const scoped_refptr<base::SequencedTaskRunner>& task_runner); On 2015/05/15 13:28:08, bartfab wrote: > What is this |task_runner| used for? If it is used for running background tasks > on another thread, rename it to |background_task_runner| instead. If it is meant > for posting asynchronous tasks on the current thread, why not just grab it from > ThreadTaskRunnerHandle::Get() instead of passing it in? It's used to post |DoRemoteCommandsFetch|, and subclasses might want to use a different task runner. For example, the MockRemoteCommandsInvalidator actually had to post tasks to MessageLoopProxy, due to limitation of FakeInvalidationService. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.h:63: virtual void OnInitialize() {} On 2015/05/15 13:28:08, bartfab wrote: > Nit 1: The style guide forbids inlining virtual methods. > Nit 2: Why do you provide empty default implementations for these four methods > if every sub-class actually overrides them? Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.h:74: void Register(const invalidation::ObjectId& object_id); On 2015/05/15 13:28:08, bartfab wrote: > Nit: #include "google/cacheinvalidation/include/types.h" Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.h:79: // Updates invalidations_enabled_ and calls the invalidation handler if the On 2015/05/15 13:28:08, bartfab wrote: > Which handler are you talking about here? I cannot see any handlers other than > |this|. Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... File chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc (right): https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:5: #include <string> On 2015/05/15 13:28:09, bartfab wrote: > Nit: Not used. Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:7: #include "base/macros.h" On 2015/05/15 13:28:09, bartfab wrote: > Nit: Already included by header. Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:8: #include "base/memory/ref_counted.h" On 2015/05/15 13:28:08, bartfab wrote: > Nit: Already included by header. Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:12: #include "chrome/browser/policy/cloud/remote_commands_invalidator_base.h" On 2015/05/15 13:28:10, bartfab wrote: > Nit: Make this the first included, followed by a blank line. Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:15: #include "policy/proto/device_management_backend.pb.h" On 2015/05/15 13:28:09, bartfab wrote: > Nit: Already included by header. Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:21: using ::testing::InSequence; On 2015/05/15 13:28:10, bartfab wrote: > Nit: Not actually needed. I think it's used later. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:30: const scoped_refptr<base::SequencedTaskRunner>& task_runner) On 2015/05/15 13:28:09, bartfab wrote: > Nit: #include "base/sequenced_task_runner.h" (scoped_refptr requires the type to > be defined to compile, even if you never dereference it) Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:32: ~MockRemoteCommandInvalidator() override {} On 2015/05/15 13:28:08, bartfab wrote: > Nit: No need to override the destructor here at all. Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:41: scoped_ptr<em::PolicyData> policy_data(new em::PolicyData()); On 2015/05/15 13:28:09, bartfab wrote: > Why do you not just use a stack-allocated |em::PolicyData| variable? Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:48: scoped_ptr<em::PolicyData> policy_data(new em::PolicyData()); On 2015/05/15 13:28:10, bartfab wrote: > As above, why not just allcoate the |em::PolicyData| on the stack? Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:61: invalidation_service_(new invalidation::FakeInvalidationService), On 2015/05/15 13:28:10, bartfab wrote: > Nit: Why does this need to be a scoped_ptr? Why can you not just have a > |invalidation::FakeInvalidationService| member directly? Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:62: invalidator_(new StrictMock<MockRemoteCommandInvalidator>( On 2015/05/15 13:28:09, bartfab wrote: > Nit: Why does this need to be a scoped_ptr? Why can you not just have a > |StrictMock<MockRemoteCommandInvalidator>| member directly? Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:63: loop_.message_loop_proxy())) {} On 2015/05/15 13:28:08, bartfab wrote: > Nit 1: MessageLoopProxy is deprecated. Use base::ThreadTaskRunnerHandle::Get() > instead. > Nit 2: When you have a multi-line inline method definition, the {} should be > split over two lines. 1: I can't use ThreadTaskRunnerHandle. It's my previous approach but it break tests since MockAckHandler used by FakeInvalidationServices post tasks to MessageLoopProxy. 2: Done https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:66: invalidation_service_->SetInvalidatorState(syncer::INVALIDATIONS_ENABLED); On 2015/05/15 13:28:09, bartfab wrote: > Nit: #include "components/invalidation/invalidator_state.h" Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:76: auto invalidation = syncer::Invalidation::InitUnknownVersion(object_id); On 2015/05/15 13:28:10, bartfab wrote: > Nit 1: Do not use |auto| here. We only use it in tight loops where it > significantly improves readability, not to obscure types in general. > Nit 2: const. Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:82: return !invalidation_service_->GetMockAckHandler()->IsUnsent(invalidation); On 2015/05/15 13:28:09, bartfab wrote: > Nit: #include "components/invalidation/mock_ack_handler.h" Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:91: return !invalidation_service_->invalidator_registrar() On 2015/05/15 13:28:09, bartfab wrote: > Nit: #include "components/invalidation/invalidator_registrar.h" Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:105: EXPECT_CALL(*invalidator_, OnStart()).Times(1); On 2015/05/15 13:28:09, bartfab wrote: > GMock does not allow mixing EXPECT_CALL() with invocations. You must always do a > VerifyAndClearExpectations() before setting the next EXPECT_CALL() after a > method invocation. Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:120: // Fire an invalidation to verify that invalidaion is not working. On 2015/05/15 13:28:08, bartfab wrote: > Nit: s/invalidaion/invalidation/ Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:121: void VerifiesInvalidationDisabled(const invalidation::ObjectId& object_id) { On 2015/05/15 13:28:10, bartfab wrote: > Nit: s/Verifies/Verify/ Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:122: VerifyExpectations(); On 2015/05/15 13:28:09, bartfab wrote: > Why do you need to do this? Does any of the tests call this method without > verifying pending expectations first? Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:126: syncer::Invalidation inv = FireInvalidation(object_id); On 2015/05/15 13:28:08, bartfab wrote: > Nit 1: Avoid abbreviaitons like |inv|. > Nit 2: const. Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:128: loop_.RunUntilIdle(); On 2015/05/15 13:28:09, bartfab wrote: > Nit: Here and throughout the file: Never spin the loop directly. Use > RunLoop().RunUntilIdle(). Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:130: VerifyExpectations(); On 2015/05/15 13:28:09, bartfab wrote: > What expectations are you verifying here? Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:133: // Fire an invalidation to verify that invalidaion works. On 2015/05/15 13:28:09, bartfab wrote: > Nit: s/invalidaion/invalidation/ Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:134: void VerifiesInvalidationEnabled(const invalidation::ObjectId& object_id) { On 2015/05/15 13:28:09, bartfab wrote: > Nit: s/Verifies/Verify/ Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:135: VerifyExpectations(); On 2015/05/15 13:28:09, bartfab wrote: > Why do you need to do this? Does any of the tests call this method without > verifying pending expectations first? Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:140: syncer::Invalidation inv = FireInvalidation(object_id); On 2015/05/15 13:28:09, bartfab wrote: > Nit 1: Avoid abbreviaitons like |inv|. > Nit 2: const. Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:162: InSequence dummy; On 2015/05/15 13:28:10, bartfab wrote: > Nit: There is no need for a sequence in this test. Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:174: DisableInvalidationService(); On 2015/05/15 13:28:08, bartfab wrote: > Nit: The disabling and re-enabling is already being tested by > InvalidationServiceEnabledStateChanged(). Just move the > EXPECT_*(invalidator_->invalidations_enabled()) to that test and remove the > disabling and re-enabling here. Done. Note that the invalidations_enabled() is actually already been tested in InvalidationServiceEnabledStateChanged. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:182: VerifyExpectations(); On 2015/05/15 13:28:10, bartfab wrote: > Nit 1: Should you not spin the loop before verifying this, to make sure there is > no pending task that receives a spurious invalidation? > Nit 2: This VerifyExpectations() is superfluous. The one in line 190 is > sufficient to catch any unexpected callbacks. Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:186: syncer::Invalidation inv1 = FireInvalidation(kTestingObjectId2); On 2015/05/15 13:28:10, bartfab wrote: > Nit 1: Avoid abbreviaitons like |inv1|. > Nit 2: const. Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:259: // Verifies that registered state changes works as expected. On 2015/05/15 13:28:08, bartfab wrote: > Nit: s/works/work/ Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:269: VerifiesInvalidationEnabled(kTestingObjectId2); On 2015/05/15 13:28:09, bartfab wrote: > Nit: Maybe double-check that |kTestingObjectId1| is disabled now? Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:272: VerifiesInvalidationEnabled(kTestingObjectId1); On 2015/05/15 13:28:09, bartfab wrote: > Nit: Maybe double-check that |kTestingObjectId2| is disabled now? Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:278: VerifiesInvalidationEnabled(kTestingObjectId2); On 2015/05/15 13:28:09, bartfab wrote: > Nit: Maybe double-check that |kTestingObjectId1| is disabled now? Done. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:294: VerifiesInvalidationDisabled(kTestingObjectId2); On 2015/05/15 13:28:09, bartfab wrote: > Here and below: Why are you sometimes testing the state of |kTestingObjectId2| > instead of |kTestingObjectId1|? They are meant to be "EXPECT_FALSE(invalidator_.invalidations_enabled())", replaced instead. https://codereview.chromium.org/1094493003/diff/130001/components/policy/core... File components/policy/core/common/cloud/cloud_policy_core.h (right): https://codereview.chromium.org/1094493003/diff/130001/components/policy/core... components/policy/core/common/cloud/cloud_policy_core.h:55: virtual void OnRemoteCommandsServiceStarted(CloudPolicyCore* core) {} On 2015/05/15 13:28:10, bartfab wrote: > Why is this not pure virtual, like the other methoda above? I believe others won't be interested in getting notified by remote commands service. So I added a default implementation. Moved to cc file.
https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... File chrome/browser/policy/cloud/remote_commands_invalidator_base.h (right): https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.h:35: const scoped_refptr<base::SequencedTaskRunner>& task_runner); On 2015/05/15 14:50:39, binjin wrote: > On 2015/05/15 13:28:08, bartfab wrote: > > What is this |task_runner| used for? If it is used for running background > tasks > > on another thread, rename it to |background_task_runner| instead. If it is > meant > > for posting asynchronous tasks on the current thread, why not just grab it > from > > ThreadTaskRunnerHandle::Get() instead of passing it in? > > It's used to post |DoRemoteCommandsFetch|, and subclasses might want to use a > different > task runner. For example, the MockRemoteCommandsInvalidator actually had to post > tasks to > MessageLoopProxy, due to limitation of FakeInvalidationService. The MessageLoopProxy you are using in that class is also accessible via ThreadTaskRunnerHandle::Get(). By definition, the current thread's task runner will *always* be accessible via ThreadTaskRunnerHandle::Get(). I see no reason to pass it as a dependency. https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... File chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc (right): https://codereview.chromium.org/1094493003/diff/130001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:63: loop_.message_loop_proxy())) {} On 2015/05/15 14:50:41, binjin wrote: > On 2015/05/15 13:28:08, bartfab wrote: > > Nit 1: MessageLoopProxy is deprecated. Use base::ThreadTaskRunnerHandle::Get() > > instead. > > Nit 2: When you have a multi-line inline method definition, the {} should be > > split over two lines. > > 1: I can't use ThreadTaskRunnerHandle. It's my previous approach but it break > tests since MockAckHandler used by FakeInvalidationServices post tasks to > MessageLoopProxy. > 2: Done You are not passing the MessageLoopProxy you grab here to anyone. MockAckHandler will grab its own MessageLoopProxy if it needs one (and it also could use a ThreadTaskRunnerHandle instead, but that is another refactor for another day). https://codereview.chromium.org/1094493003/diff/150001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/remote_commands/remote_commands_invalidator.h (right): https://codereview.chromium.org/1094493003/diff/150001/chrome/browser/chromeo... chrome/browser/chromeos/policy/remote_commands/remote_commands_invalidator.h:16: class InvalidationService; Nit: No need to forward-declare this as you use it in an overridden declaration only. https://codereview.chromium.org/1094493003/diff/150001/chrome/browser/chromeo... chrome/browser/chromeos/policy/remote_commands/remote_commands_invalidator.h:22: // listens to events from CloudPolicyCore and CloudPolicyStore and interact Nit: s/interact/interacts/ https://codereview.chromium.org/1094493003/diff/150001/chrome/browser/policy/... File chrome/browser/policy/cloud/remote_commands_invalidator_base.cc (right): https://codereview.chromium.org/1094493003/diff/150001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.cc:103: task_runner_->PostTask( As discussed offline, you should be able to do this synchronously, also in tests. https://codereview.chromium.org/1094493003/diff/150001/chrome/browser/policy/... File chrome/browser/policy/cloud/remote_commands_invalidator_base.h (right): https://codereview.chromium.org/1094493003/diff/150001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.h:32: // |task_runner| will be used to post DoRemoteCommandsFetch() tasks. 1: Nit: Document that this needs to be the current thread's task runner. 2: Remove this dependency and use ThreadTaskRunnerHandle::Get() instead. I feel strongly about this. It will ensure that one cannot shoot themselves in the foot by passing a task runner for some other thread by accident. https://codereview.chromium.org/1094493003/diff/150001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.h:42: // shutting down, the invalidator can't be started anymore. Well, it can be started by calling Initialize() again. https://codereview.chromium.org/1094493003/diff/150001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.h:45: // Starts to process the invalidations. Nit: s/the // https://codereview.chromium.org/1094493003/diff/150001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.h:48: // Stops to process the invalidation. Must be called after Start(). Nit 1: s/the invalidation/invalidations/ Nit 2: "Must be called after Start()" makes it sound like you must always call this method after starting. How about "May only be called after Start() has been called?" https://codereview.chromium.org/1094493003/diff/150001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.h:64: // Subclasses should override the following functions, if they want to get Subclasses actually *must* override these methods, since there are pure virtual :). I think you can remove the comment. https://codereview.chromium.org/1094493003/diff/150001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.h:71: // Subclasses must override this method to implements the actual remote Nit: s/implements/implement/ https://codereview.chromium.org/1094493003/diff/150001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.h:72: // commands fetch. A callback to this function will be posted to Nit: s/callback/call/ https://codereview.chromium.org/1094493003/diff/150001/chrome/browser/policy/... File chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc (right): https://codereview.chromium.org/1094493003/diff/150001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:7: #include "base/memory/scoped_ptr.h" Nit: Not used. https://codereview.chromium.org/1094493003/diff/150001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:22: using ::testing::InSequence; All your tests that use |InSequence| do not actually need it. You can remove the |InSequence| from all your tests and from here. https://codereview.chromium.org/1094493003/diff/150001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:48: em::PolicyData policy_data; Nit: const. https://codereview.chromium.org/1094493003/diff/150001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:96: void VerifyExpectations() { Mock::VerifyAndClearExpectations(&invalidator_); } Nit: Per style guide, always put a line break after {. https://codereview.chromium.org/1094493003/diff/150001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:276: // Verifies that invalidation service enabled state changea work as expected. Nit: s/changea/changes/
https://codereview.chromium.org/1094493003/diff/150001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/remote_commands/remote_commands_invalidator.h (right): https://codereview.chromium.org/1094493003/diff/150001/chrome/browser/chromeo... chrome/browser/chromeos/policy/remote_commands/remote_commands_invalidator.h:16: class InvalidationService; On 2015/05/15 15:37:12, bartfab wrote: > Nit: No need to forward-declare this as you use it in an overridden declaration > only. Done. https://codereview.chromium.org/1094493003/diff/150001/chrome/browser/chromeo... chrome/browser/chromeos/policy/remote_commands/remote_commands_invalidator.h:22: // listens to events from CloudPolicyCore and CloudPolicyStore and interact On 2015/05/15 15:37:12, bartfab wrote: > Nit: s/interact/interacts/ Done. https://codereview.chromium.org/1094493003/diff/150001/chrome/browser/policy/... File chrome/browser/policy/cloud/remote_commands_invalidator_base.cc (right): https://codereview.chromium.org/1094493003/diff/150001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.cc:103: task_runner_->PostTask( On 2015/05/15 15:37:12, bartfab wrote: > As discussed offline, you should be able to do this synchronously, also in > tests. Done. https://codereview.chromium.org/1094493003/diff/150001/chrome/browser/policy/... File chrome/browser/policy/cloud/remote_commands_invalidator_base.h (right): https://codereview.chromium.org/1094493003/diff/150001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.h:32: // |task_runner| will be used to post DoRemoteCommandsFetch() tasks. On 2015/05/15 15:37:12, bartfab wrote: > 1: Nit: Document that this needs to be the current thread's task runner. > 2: Remove this dependency and use ThreadTaskRunnerHandle::Get() instead. I feel > strongly about this. It will ensure that one cannot shoot themselves in the foot > by passing a task runner for some other thread by accident. Acknowledged. task_runner was removed. https://codereview.chromium.org/1094493003/diff/150001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.h:42: // shutting down, the invalidator can't be started anymore. On 2015/05/15 15:37:12, bartfab wrote: > Well, it can be started by calling Initialize() again. Done. https://codereview.chromium.org/1094493003/diff/150001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.h:45: // Starts to process the invalidations. On 2015/05/15 15:37:12, bartfab wrote: > Nit: s/the // Done. https://codereview.chromium.org/1094493003/diff/150001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.h:48: // Stops to process the invalidation. Must be called after Start(). On 2015/05/15 15:37:12, bartfab wrote: > Nit 1: s/the invalidation/invalidations/ > Nit 2: "Must be called after Start()" makes it sound like you must always call > this method after starting. How about "May only be called after Start() has been > called?" Done. https://codereview.chromium.org/1094493003/diff/150001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.h:64: // Subclasses should override the following functions, if they want to get On 2015/05/15 15:37:12, bartfab wrote: > Subclasses actually *must* override these methods, since there are pure virtual > :). I think you can remove the comment. Done. https://codereview.chromium.org/1094493003/diff/150001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.h:71: // Subclasses must override this method to implements the actual remote On 2015/05/15 15:37:12, bartfab wrote: > Nit: s/implements/implement/ Done. https://codereview.chromium.org/1094493003/diff/150001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base.h:72: // commands fetch. A callback to this function will be posted to On 2015/05/15 15:37:12, bartfab wrote: > Nit: s/callback/call/ Done. https://codereview.chromium.org/1094493003/diff/150001/chrome/browser/policy/... File chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc (right): https://codereview.chromium.org/1094493003/diff/150001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:7: #include "base/memory/scoped_ptr.h" On 2015/05/15 15:37:12, bartfab wrote: > Nit: Not used. Done. https://codereview.chromium.org/1094493003/diff/150001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:22: using ::testing::InSequence; On 2015/05/15 15:37:13, bartfab wrote: > All your tests that use |InSequence| do not actually need it. You can remove the > |InSequence| from all your tests and from here. Done. https://codereview.chromium.org/1094493003/diff/150001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:48: em::PolicyData policy_data; On 2015/05/15 15:37:12, bartfab wrote: > Nit: const. Done. https://codereview.chromium.org/1094493003/diff/150001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:96: void VerifyExpectations() { Mock::VerifyAndClearExpectations(&invalidator_); } On 2015/05/15 15:37:12, bartfab wrote: > Nit: Per style guide, always put a line break after {. Done. https://codereview.chromium.org/1094493003/diff/150001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_base_unittest.cc:276: // Verifies that invalidation service enabled state changea work as expected. On 2015/05/15 15:37:12, bartfab wrote: > Nit: s/changea/changes/ Done.
https://codereview.chromium.org/1094493003/diff/170001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/remote_commands/affiliated_remote_commands_invalidator.cc (right): https://codereview.chromium.org/1094493003/diff/170001/chrome/browser/chromeo... chrome/browser/chromeos/policy/remote_commands/affiliated_remote_commands_invalidator.cc:7: #include "base/logging.h" Nit: Not used. https://codereview.chromium.org/1094493003/diff/170001/chrome/browser/chromeo... chrome/browser/chromeos/policy/remote_commands/affiliated_remote_commands_invalidator.cc:8: #include "base/thread_task_runner_handle.h" Nit: Not used. https://codereview.chromium.org/1094493003/diff/170001/chrome/browser/chromeo... chrome/browser/chromeos/policy/remote_commands/affiliated_remote_commands_invalidator.cc:9: #include "components/policy/core/common/remote_commands/remote_commands_service.h" Nit: Not used. https://codereview.chromium.org/1094493003/diff/170001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/remote_commands/affiliated_remote_commands_invalidator.h (right): https://codereview.chromium.org/1094493003/diff/170001/chrome/browser/chromeo... chrome/browser/chromeos/policy/remote_commands/affiliated_remote_commands_invalidator.h:10: #include "chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.h" Nit: Forward-declare RemoteCommandsInvalidatorImpl instead and move the #include to the .cc file. https://codereview.chromium.org/1094493003/diff/170001/chrome/browser/chromeo... chrome/browser/chromeos/policy/remote_commands/affiliated_remote_commands_invalidator.h:14: class InvalidationService; Nit: Not needed (only used in the declaration of a forward-declared method). https://codereview.chromium.org/1094493003/diff/170001/chrome/browser/chromeo... chrome/browser/chromeos/policy/remote_commands/affiliated_remote_commands_invalidator.h:21: class AffiliatedRemoteCommandsInvalidator Nit: Document that this is a wrapper to be used for device commands and device-local account commands. https://codereview.chromium.org/1094493003/diff/170001/chrome/browser/policy/... File chrome/browser/policy/cloud/remote_commands_invalidator.h (right): https://codereview.chromium.org/1094493003/diff/170001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator.h:26: // Base class for RemoteCommandsInvalidator, this class doesn't interact with Nit: Update the comment: 1) The class was renamed. 2) This is not really a base for "subclasses" (plural) to build on. It is the guts of the implementation with a few methods abstracted out for easier mocking. https://codereview.chromium.org/1094493003/diff/170001/chrome/browser/policy/... File chrome/browser/policy/cloud/remote_commands_invalidator_impl.h (right): https://codereview.chromium.org/1094493003/diff/170001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_impl.h:14: class InvalidationService; Nit: Not used. https://codereview.chromium.org/1094493003/diff/170001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_impl.h:20: // listens to events from CloudPolicyCore and CloudPolicyStore and interact Nit: s/and interact/and builds/ would be better. After all, the two are not separate objects - they are the same object.
Almost there now. https://codereview.chromium.org/1094493003/diff/190001/chrome/browser/policy/... File chrome/browser/policy/cloud/remote_commands_invalidator.cc (right): https://codereview.chromium.org/1094493003/diff/190001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator.cc:9: #include "base/bind.h" Nit: Not used. https://codereview.chromium.org/1094493003/diff/190001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator.cc:10: #include "base/location.h" Nit: Not used. https://codereview.chromium.org/1094493003/diff/190001/chrome/browser/policy/... File chrome/browser/policy/cloud/remote_commands_invalidator.h (right): https://codereview.chromium.org/1094493003/diff/190001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator.h:9: #include "base/memory/ref_counted.h" Nit: Not used. https://codereview.chromium.org/1094493003/diff/190001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator.h:10: #include "base/memory/weak_ptr.h" Nit: Not used. https://codereview.chromium.org/1094493003/diff/190001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator.h:17: class SequencedTaskRunner; Nit: Not used. https://codereview.chromium.org/1094493003/diff/190001/chrome/browser/policy/... File chrome/browser/policy/cloud/remote_commands_invalidator_unittest.cc (right): https://codereview.chromium.org/1094493003/diff/190001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_unittest.cc:9: #include "base/sequenced_task_runner.h" Nit: Not used.
https://codereview.chromium.org/1094493003/diff/170001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/remote_commands/affiliated_remote_commands_invalidator.cc (right): https://codereview.chromium.org/1094493003/diff/170001/chrome/browser/chromeo... chrome/browser/chromeos/policy/remote_commands/affiliated_remote_commands_invalidator.cc:7: #include "base/logging.h" On 2015/05/15 15:55:08, bartfab wrote: > Nit: Not used. Done. https://codereview.chromium.org/1094493003/diff/170001/chrome/browser/chromeo... chrome/browser/chromeos/policy/remote_commands/affiliated_remote_commands_invalidator.cc:8: #include "base/thread_task_runner_handle.h" On 2015/05/15 15:55:08, bartfab wrote: > Nit: Not used. Done. https://codereview.chromium.org/1094493003/diff/170001/chrome/browser/chromeo... chrome/browser/chromeos/policy/remote_commands/affiliated_remote_commands_invalidator.cc:9: #include "components/policy/core/common/remote_commands/remote_commands_service.h" On 2015/05/15 15:55:08, bartfab wrote: > Nit: Not used. Done. https://codereview.chromium.org/1094493003/diff/170001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/remote_commands/affiliated_remote_commands_invalidator.h (right): https://codereview.chromium.org/1094493003/diff/170001/chrome/browser/chromeo... chrome/browser/chromeos/policy/remote_commands/affiliated_remote_commands_invalidator.h:10: #include "chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.h" On 2015/05/15 15:55:08, bartfab wrote: > Nit: Forward-declare RemoteCommandsInvalidatorImpl instead and move the #include > to the .cc file. Done. https://codereview.chromium.org/1094493003/diff/170001/chrome/browser/chromeo... chrome/browser/chromeos/policy/remote_commands/affiliated_remote_commands_invalidator.h:14: class InvalidationService; On 2015/05/15 15:55:08, bartfab wrote: > Nit: Not needed (only used in the declaration of a forward-declared method). Done. https://codereview.chromium.org/1094493003/diff/170001/chrome/browser/chromeo... chrome/browser/chromeos/policy/remote_commands/affiliated_remote_commands_invalidator.h:21: class AffiliatedRemoteCommandsInvalidator On 2015/05/15 15:55:08, bartfab wrote: > Nit: Document that this is a wrapper to be used for device commands and > device-local account commands. Done. https://codereview.chromium.org/1094493003/diff/170001/chrome/browser/policy/... File chrome/browser/policy/cloud/remote_commands_invalidator.h (right): https://codereview.chromium.org/1094493003/diff/170001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator.h:26: // Base class for RemoteCommandsInvalidator, this class doesn't interact with On 2015/05/15 15:55:08, bartfab wrote: > Nit: Update the comment: > 1) The class was renamed. > 2) This is not really a base for "subclasses" (plural) to build on. It is the > guts of the implementation with a few methods abstracted out for easier mocking. Done. https://codereview.chromium.org/1094493003/diff/170001/chrome/browser/policy/... File chrome/browser/policy/cloud/remote_commands_invalidator_impl.h (right): https://codereview.chromium.org/1094493003/diff/170001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_impl.h:14: class InvalidationService; On 2015/05/15 15:55:08, bartfab wrote: > Nit: Not used. Done. https://codereview.chromium.org/1094493003/diff/170001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_impl.h:20: // listens to events from CloudPolicyCore and CloudPolicyStore and interact On 2015/05/15 15:55:08, bartfab wrote: > Nit: s/and interact/and builds/ would be better. After all, the two are not > separate objects - they are the same object. Done. https://codereview.chromium.org/1094493003/diff/190001/chrome/browser/policy/... File chrome/browser/policy/cloud/remote_commands_invalidator.cc (right): https://codereview.chromium.org/1094493003/diff/190001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator.cc:9: #include "base/bind.h" On 2015/05/15 16:00:30, bartfab wrote: > Nit: Not used. Done. https://codereview.chromium.org/1094493003/diff/190001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator.cc:10: #include "base/location.h" On 2015/05/15 16:00:30, bartfab wrote: > Nit: Not used. Done. https://codereview.chromium.org/1094493003/diff/190001/chrome/browser/policy/... File chrome/browser/policy/cloud/remote_commands_invalidator.h (right): https://codereview.chromium.org/1094493003/diff/190001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator.h:9: #include "base/memory/ref_counted.h" On 2015/05/15 16:00:30, bartfab wrote: > Nit: Not used. Done. https://codereview.chromium.org/1094493003/diff/190001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator.h:10: #include "base/memory/weak_ptr.h" On 2015/05/15 16:00:30, bartfab wrote: > Nit: Not used. Done. https://codereview.chromium.org/1094493003/diff/190001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator.h:17: class SequencedTaskRunner; On 2015/05/15 16:00:30, bartfab wrote: > Nit: Not used. Done. https://codereview.chromium.org/1094493003/diff/190001/chrome/browser/policy/... File chrome/browser/policy/cloud/remote_commands_invalidator_unittest.cc (right): https://codereview.chromium.org/1094493003/diff/190001/chrome/browser/policy/... chrome/browser/policy/cloud/remote_commands_invalidator_unittest.cc:9: #include "base/sequenced_task_runner.h" On 2015/05/15 16:00:30, bartfab wrote: > Nit: Not used. Done.
lgtm https://codereview.chromium.org/1094493003/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/remote_commands/affiliated_remote_commands_invalidator.h (right): https://codereview.chromium.org/1094493003/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/policy/remote_commands/affiliated_remote_commands_invalidator.h:10: #include "chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.h" Nit: Not used.
https://codereview.chromium.org/1094493003/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/remote_commands/affiliated_remote_commands_invalidator.h (right): https://codereview.chromium.org/1094493003/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/policy/remote_commands/affiliated_remote_commands_invalidator.h:10: #include "chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.h" On 2015/05/15 16:23:54, bartfab wrote: > Nit: Not used. It's used since we inherit from AffiliatedInvalidationServiceProvider::Consumer.
The CQ bit was checked by binjin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1094493003/200001
https://codereview.chromium.org/1094493003/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/remote_commands/affiliated_remote_commands_invalidator.h (right): https://codereview.chromium.org/1094493003/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/policy/remote_commands/affiliated_remote_commands_invalidator.h:10: #include "chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.h" On 2015/05/15 16:27:02, binjin wrote: > On 2015/05/15 16:23:54, bartfab wrote: > > Nit: Not used. > > It's used since we inherit from AffiliatedInvalidationServiceProvider::Consumer. Sorry, doing reviews while in a meeting... I am a tad distracted :).
Message was sent while issue was closed.
Committed patchset #12 (id:200001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/9b94f2db07ffd0bad793e960f7a2d1cb428db7ea Cr-Commit-Position: refs/heads/master@{#330114} |
