Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
879233003:
Initial RemoteCommandService (Closed)
DescriptionInitial RemoteCommandService
This CL adds initial implementation of RemoteCommandService. RemoteCommandService contains a RemoteCommandsQueue and will interacts with DMServer to fetch and run commands in sequence. Protocol modification is made to protobuf as well.
BUG=None
Committed: https://crrev.com/efa8017cfd57b38d0126e994a5e29aba7fbf98e8
Cr-Commit-Position: refs/heads/master@{#321103}
Patch Set 1 : early prototype (untested) #Patch Set 2 : fix lint #Patch Set 3 : fix last_command_id #
Total comments: 17
Patch Set 4 : fixes addressing #3 #Patch Set 5 : rebase #Patch Set 6 : WIP #Patch Set 7 : WIP #
Total comments: 94
Patch Set 8 : fixes addressing #7 #Patch Set 9 : add test to CloudPolicyClientTest #Patch Set 10 : (rebase only) #Patch Set 11 : minor fixes #Patch Set 12 : comments grammar fixes; fix win compile #
Total comments: 106
Patch Set 13 : fixes addressing #10 #11 and #12 #
Total comments: 18
Patch Set 14 : fixes addressing #14 #
Total comments: 40
Patch Set 15 : fixes addressing #16 #Patch Set 16 : minor fixes #
Total comments: 196
Patch Set 17 : fixes addressing #18 and #19 #Patch Set 18 : more fixes #
Total comments: 20
Patch Set 19 : addressed #25 #
Total comments: 32
Patch Set 20 : fixes addressing #28 #
Total comments: 19
Patch Set 21 : rebase, fixes addressing #31 #Messages
Total messages: 45 (10 generated)
binjin@chromium.org changed reviewers: + bartfab@chromium.org
Hello Bartosz, Could you have an early review on this? Bin
https://codereview.chromium.org/879233003/diff/40001/components/policy/core/c... File components/policy/core/common/cloud/cloud_policy_client.h (right): https://codereview.chromium.org/879233003/diff/40001/components/policy/core/c... components/policy/core/common/cloud/cloud_policy_client.h:150: virtual scoped_ptr<DeviceManagementRequestJob> CreateRemoteCommandsJob(); Why not have this method create a job and send it to the outbound queue in one go? https://codereview.chromium.org/879233003/diff/40001/components/policy/core/c... File components/policy/core/common/remote_commands/remote_command_job.h (right): https://codereview.chromium.org/879233003/diff/40001/components/policy/core/c... components/policy/core/common/remote_commands/remote_command_job.h:46: virtual scoped_ptr<RemoteCommandJob> BuildJobForType( We could just make this a static method of RemoteCommandJob. Alternatively, if you want a Factory, I would put it in its own, independent class in a separate header. https://codereview.chromium.org/879233003/diff/40001/components/policy/core/c... File components/policy/core/common/remote_commands/remote_commands_service.cc (right): https://codereview.chromium.org/879233003/diff/40001/components/policy/core/c... components/policy/core/common/remote_commands/remote_commands_service.cc:24: const int kAutoFetchingIntervalInMinutes = 30; The server should send us a Tango invalidation when there are new commands. Auto-fetching would only be a backup that should run rarely, e.g. every 24 hours. Since commands expire after 3 hours, the auto-fetching would actually be quite useless. I think we can remove it completely and fetch on invalidaiton only. https://codereview.chromium.org/879233003/diff/40001/components/policy/core/c... components/policy/core/common/remote_commands/remote_commands_service.cc:50: unhandled_fetch_requests < kMaxUnhandledFetchRequests) { Why do we need a queue of unhandled fetch requests? We could do things analogously to policy: Only the most recent fetch request is sent to the server. If there is a request in flight already, that request is dropped. https://codereview.chromium.org/879233003/diff/40001/components/policy/core/c... components/policy/core/common/remote_commands/remote_commands_service.cc:65: for (const auto& command_result : unsent_results_) This has an interesting interaction with the reboot command, and with reboots in general: Since we do not store unsent results in persistent storage, we should try hard to flush any unsent results before rebooting. https://codereview.chromium.org/879233003/diff/40001/components/policy/core/c... components/policy/core/common/remote_commands/remote_commands_service.cc:90: last_fetched_command_id_ = This means that we auto-acknowledge the command with the next fetch. That is not quite what we want to do: As long as the command has not been processed, we should *not* acknowledge its ID. We should allow the server to send it again and again with subsequent fetches. That way, we can make sure the command is still available to us e.g. after a crash or reboot. But note that this also means we can get the same command from the server multiple times. We need to account for this and ensure that we do not push command duplicates into the queue.
https://codereview.chromium.org/879233003/diff/40001/components/policy/core/c... File components/policy/core/common/cloud/cloud_policy_client.h (right): https://codereview.chromium.org/879233003/diff/40001/components/policy/core/c... components/policy/core/common/cloud/cloud_policy_client.h:150: virtual scoped_ptr<DeviceManagementRequestJob> CreateRemoteCommandsJob(); On 2015/01/29 10:18:05, bartfab wrote: > Why not have this method create a job and send it to the outbound queue in one > go? That will introduce some logic about how to construct the remote command request header into policy specified code, which is what I try to avoid. https://codereview.chromium.org/879233003/diff/40001/components/policy/core/c... File components/policy/core/common/remote_commands/remote_command_job.h (right): https://codereview.chromium.org/879233003/diff/40001/components/policy/core/c... components/policy/core/common/remote_commands/remote_command_job.h:46: virtual scoped_ptr<RemoteCommandJob> BuildJobForType( On 2015/01/29 10:18:05, bartfab wrote: > We could just make this a static method of RemoteCommandJob. Alternatively, if > you want a Factory, I would put it in its own, independent class in a separate > header. I expect chrome and chromeos have different implementation of this class, I will move it into a separate header. It's in component/ so I think make it static method will be bad for extensibility. https://codereview.chromium.org/879233003/diff/40001/components/policy/core/c... File components/policy/core/common/remote_commands/remote_commands_service.cc (right): https://codereview.chromium.org/879233003/diff/40001/components/policy/core/c... components/policy/core/common/remote_commands/remote_commands_service.cc:24: const int kAutoFetchingIntervalInMinutes = 30; On 2015/01/29 10:18:05, bartfab wrote: > The server should send us a Tango invalidation when there are new commands. > Auto-fetching would only be a backup that should run rarely, e.g. every 24 > hours. Since commands expire after 3 hours, the auto-fetching would actually be > quite useless. I think we can remove it completely and fetch on invalidaiton > only. I would like to keep the auto fetching as a backup, since the push connection might still be unreliable with bad network connection. Maybe we can make the interval to 3 hours as the same as command expiration time, in order to reduce the server load. https://codereview.chromium.org/879233003/diff/40001/components/policy/core/c... components/policy/core/common/remote_commands/remote_commands_service.cc:50: unhandled_fetch_requests < kMaxUnhandledFetchRequests) { On 2015/01/29 10:18:05, bartfab wrote: > Why do we need a queue of unhandled fetch requests? We could do things > analogously to policy: Only the most recent fetch request is sent to the server. > If there is a request in flight already, that request is dropped. The entire logic here is for reliability. There is one assumption that I'm not 100% sure of in every corner cases and possibly unstable state of browser: DeviceManagementService will eventually call RemoteCommandsService::OnRemoteCommandsFetched() in reasonable time. After kMaxUnhandledFetchRequests is reached we will simply cancel the current connection job since it's either literally timeout or have a lot of other pending fetching requets. https://codereview.chromium.org/879233003/diff/40001/components/policy/core/c... components/policy/core/common/remote_commands/remote_commands_service.cc:65: for (const auto& command_result : unsent_results_) On 2015/01/29 10:18:05, bartfab wrote: > This has an interesting interaction with the reboot command, and with reboots in > general: Since we do not store unsent results in persistent storage, we should > try hard to flush any unsent results before rebooting. Yes, I will make necessary change. https://codereview.chromium.org/879233003/diff/40001/components/policy/core/c... components/policy/core/common/remote_commands/remote_commands_service.cc:90: last_fetched_command_id_ = On 2015/01/29 10:18:05, bartfab wrote: > This means that we auto-acknowledge the command with the next fetch. That is not > quite what we want to do: As long as the command has not been processed, we > should *not* acknowledge its ID. We should allow the server to send it again and > again with subsequent fetches. That way, we can make sure the command is still > available to us e.g. after a crash or reboot. > > But note that this also means we can get the same command from the server > multiple times. We need to account for this and ensure that we do not push > command duplicates into the queue. Actually this patchset considered "RemoteCommandsService::OnJobStarted()" as an acknowledge by writing |last_executed_command_id_| to a persistent source. So that ideally after OnJobStarted() is called, we won't execute this commands again. I think I will make following changes: * keep both |last_executed_command_id_| and |last_fetched_command_id_| recorded in the same way as now * send |last_executed_command_id_| to server for fetching instead. * compare the fetched commands with |last_fetched_command_id_| to drop existing ones * (possibly) do not write |last_executed_command_id_| to persistent source now
https://codereview.chromium.org/879233003/diff/40001/components/policy/core/c... File components/policy/core/common/cloud/cloud_policy_client.h (right): https://codereview.chromium.org/879233003/diff/40001/components/policy/core/c... components/policy/core/common/cloud/cloud_policy_client.h:150: virtual scoped_ptr<DeviceManagementRequestJob> CreateRemoteCommandsJob(); On 2015/01/29 10:44:10, binjin wrote: > On 2015/01/29 10:18:05, bartfab wrote: > > Why not have this method create a job and send it to the outbound queue in one > > go? > > That will introduce some logic about how to construct the remote command request > header into policy specified code, which is what I try to avoid. If you look at Drew's solution, this is exactly what he does: https://codereview.chromium.org/845313008/diff/160001/components/policy/core/... He passes two protos that the CloudPolicyClient stuffs into the command before sending it off. I think it is a good idea to be consistent with that approach for remote commands as well. The logic that actually builds these protos does not belong into the client. The client simply copies them over and sends the result. https://codereview.chromium.org/879233003/diff/40001/components/policy/core/c... File components/policy/core/common/remote_commands/remote_command_job.h (right): https://codereview.chromium.org/879233003/diff/40001/components/policy/core/c... components/policy/core/common/remote_commands/remote_command_job.h:46: virtual scoped_ptr<RemoteCommandJob> BuildJobForType( On 2015/01/29 10:44:10, binjin wrote: > On 2015/01/29 10:18:05, bartfab wrote: > > We could just make this a static method of RemoteCommandJob. Alternatively, if > > you want a Factory, I would put it in its own, independent class in a separate > > header. > > I expect chrome and chromeos have different implementation of this class, I will > move it into a separate header. > > It's in component/ so I think make it static method will be bad for > extensibility. Good thinking. Yes, the factory will need to be inside *chrome*, not the component, because it provides the bridge between the generic framework and Chrome-specific command types. https://codereview.chromium.org/879233003/diff/40001/components/policy/core/c... File components/policy/core/common/remote_commands/remote_commands_service.cc (right): https://codereview.chromium.org/879233003/diff/40001/components/policy/core/c... components/policy/core/common/remote_commands/remote_commands_service.cc:24: const int kAutoFetchingIntervalInMinutes = 30; On 2015/01/29 10:44:10, binjin wrote: > On 2015/01/29 10:18:05, bartfab wrote: > > The server should send us a Tango invalidation when there are new commands. > > Auto-fetching would only be a backup that should run rarely, e.g. every 24 > > hours. Since commands expire after 3 hours, the auto-fetching would actually > be > > quite useless. I think we can remove it completely and fetch on invalidaiton > > only. > > I would like to keep the auto fetching as a backup, since the push connection > might still be unreliable with bad network connection. Maybe we can make the > interval to 3 hours as the same as command expiration time, in order to reduce > the server load. The purpose of our invalidation work was to significantly reduce server load as we only talk to the server when there is actually something to do. Polling for commands every 3 hours would add a lot of load. Also, I am not at all convinced even a 3 hour interval would produce reasonable results. If pushing really is broken, will an admin be OK waiting 3 hours for a command to execute? Is this not an error state that requires escalation and a fix to pushing? We should discuss this at our meeting with the server team tonight. https://codereview.chromium.org/879233003/diff/40001/components/policy/core/c... components/policy/core/common/remote_commands/remote_commands_service.cc:50: unhandled_fetch_requests < kMaxUnhandledFetchRequests) { On 2015/01/29 10:44:10, binjin wrote: > On 2015/01/29 10:18:05, bartfab wrote: > > Why do we need a queue of unhandled fetch requests? We could do things > > analogously to policy: Only the most recent fetch request is sent to the > server. > > If there is a request in flight already, that request is dropped. > > The entire logic here is for reliability. There is one assumption that I'm not > 100% sure of in every corner cases and possibly unstable state of browser: > DeviceManagementService will eventually call > RemoteCommandsService::OnRemoteCommandsFetched() in reasonable time. After > kMaxUnhandledFetchRequests is reached we will simply cancel the current > connection job since it's either literally timeout or have a lot of other > pending fetching requets. CloudPolicyClient keeps track of timeouts on its own. If a request to the server really gets stuck (e.g. due to packet loss), it will time out and you will be notified. You should resend the request in that case. I really see no reason to keep a queue of requests here. There should only ever be one request. https://codereview.chromium.org/879233003/diff/40001/components/policy/core/c... components/policy/core/common/remote_commands/remote_commands_service.cc:90: last_fetched_command_id_ = On 2015/01/29 10:44:10, binjin wrote: > On 2015/01/29 10:18:05, bartfab wrote: > > This means that we auto-acknowledge the command with the next fetch. That is > not > > quite what we want to do: As long as the command has not been processed, we > > should *not* acknowledge its ID. We should allow the server to send it again > and > > again with subsequent fetches. That way, we can make sure the command is still > > available to us e.g. after a crash or reboot. > > > > But note that this also means we can get the same command from the server > > multiple times. We need to account for this and ensure that we do not push > > command duplicates into the queue. > > Actually this patchset considered "RemoteCommandsService::OnJobStarted()" as an > acknowledge by writing |last_executed_command_id_| to a persistent source. So > that ideally after OnJobStarted() is called, we won't execute this commands > again. > > I think I will make following changes: > * keep both |last_executed_command_id_| and |last_fetched_command_id_| recorded > in the same way as now > * send |last_executed_command_id_| to server for fetching instead. > * compare the fetched commands with |last_fetched_command_id_| to drop existing > ones > * (possibly) do not write |last_executed_command_id_| to persistent source now I think this plan is heading in the right direction. We can discuss the details once you have a prototype implementation of it.
Hello Bartosz, Could you have a look at the latest patchset? Bin
https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/c... File chrome/browser/policy/cloud/test_request_interceptor.cc (right): https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/c... chrome/browser/policy/cloud/test_request_interceptor.cc:150: std::string headers(kGoodHeaders, arraysize(kGoodHeaders)); Nit: const. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/c... chrome/browser/policy/cloud/test_request_interceptor.cc:265: return BadRequestJobCallback(request, network_delegate); This early return will cause the callbacks below not to run. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/c... chrome/browser/policy/cloud/test_request_interceptor.cc:272: if (!request_serviced_callbacks_.empty()) { These callbacks used to run *before* the ob, now they run *after* the job. Are you sure this is OK? https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/c... File chrome/browser/policy/cloud/test_request_interceptor.h (right): https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/c... chrome/browser/policy/cloud/test_request_interceptor.h:37: // If a null URLRequestJob is returned by this callback, it indicates that The grammar is a bit off in this comment, but that is easily fixable. I am more concerned about the logic. IIUC, you want to handle the case where multiple requests arrive in undefined order. Correct? If so, I think we have two options to handle this cleanly: 1) Instead of magically using a BadRequestJob, simply step through the callback queue until a callback is found which can handle the current job. 2) Alternatively, make sure that requests are issued in the expected order. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... File chrome/browser/policy/remote_commands/remote_commands_browsertest.cc (right): https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:65: return nullptr; Nit: Add NOTREACHED(). https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:87: if (job_result.has_timestamp()) { It would be an error for the job not to have a timestamp. Rather than testing for this, always use job_result.timestamp() (this will not crash even if no timestamp was set by the way, courtesy of protobuf). https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:110: class RemoteCommandsBrowserTest : public InProcessBrowserTest { Rather than being a browser test, I think this should be a unit test for the RemoteCommandsService class. This would also avoid the need for all the setup overhead as you can just create a fake/mock CloudPolicyClient and use that in the test. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:138: BrowserPolicyConnector* connector = Nit: s/BrowserPolicyConnector*/BrowserPolicyConnector* const/ https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:144: #if !defined(OS_CHROMEOS) I think it is OK to limit ourselves to Chrome OS now. We are only implementing device commands at the moment, which means we are limited to Chrome OS. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:161: mock_factory_ = nullptr; No need for this, really. It is a non-owner pointer. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:168: return UserCloudPolicyManagerFactoryChromeOS::GetForProfile( We are only implementing device commands for now. Using the user policy infrastructure for this is weird. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:180: MockTestRemoteCommandFactory* mock_factory_; Nit: Initialize with = nullptr. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:192: TestRequestInterceptor::FetchRemoteCommandsJob(server_.get(), true)); Why do we expect two callbacks? https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:204: policy_manager()->core()->remote_commands_service()->FetchRemoteCommands(); Will connecting the client not issue a remote command fetch? If so, we could enqueue a command, *then* connect the client and wait for it to issue a command fetch. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... File chrome/browser/policy/remote_commands/testing_remote_commands_server.cc (right): https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/testing_remote_commands_server.cc:22: namespace em = enterprise_management; Nit: No need to alias the namespace if you only use it a single time. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/testing_remote_commands_server.cc:27: : last_acknowledged_id_(0), Nit: In C++ 11, you can initialize ast_acknowledged_id_ and last_generated_unique_id_ right in the header file. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/testing_remote_commands_server.cc:28: last_generated_unique_id_(1), Nit: 0 is sufficient. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/testing_remote_commands_server.cc:30: task_runner_(base::MessageLoopProxy::current()) { Nit: base::MessageLoopProxy is deprecated. Use base::ThreadTaskRunnerHandle::Get() instead. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/testing_remote_commands_server.cc:34: DCHECK(task_runner_->BelongsToCurrentThread()); Nit: Use a base::ThreadChecker for this. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/testing_remote_commands_server.cc:46: command.set_unique_id(last_generated_unique_id_++); Nit: If this really is the last generated ID, you should pre-, not post-increment to get a new ID. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/testing_remote_commands_server.cc:74: EXPECT_GE(last_command_id, last_acknowledged_id_); Nit: Expected value first, actual second. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/testing_remote_commands_server.cc:81: EXPECT_LE(job_result.has_unique_id(), last_command_id); Nit: Expected value first, actual second. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/testing_remote_commands_server.cc:96: base::Unretained(this), job_result)); Do not use base::Unretained when posting across threads. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... File chrome/browser/policy/remote_commands/testing_remote_commands_server.h (right): https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/testing_remote_commands_server.h:25: class TestingRemoteCommandsServer { Without any documentation, it is hard to understand what the methods do and how they work together. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/testing_remote_commands_server.h:35: void SetClockForTesting(scoped_ptr<base::Clock> clock); No need for the "ForTesting" prefix. This is testing code after all. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/testing_remote_commands_server.h:38: size_t GetUnreportedCommandCount() const; Nit: "Unreported" is not clear to me. This seems to return the number of commands for which no results have been sent back yet. I think it would be good to find a better name that reflects this. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/testing_remote_commands_server.h:45: std::vector<enterprise_management::RemoteCommand>* fetched_commands); Why not have |std::vector<enterprise_management::RemoteCommand> as the return value? https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/testing_remote_commands_server.h:50: const enterprise_management::RemoteCommandResult& job_result) const {} Nit: Never inline virtual methods. The style guide forbids it. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/testing_remote_commands_server.h:54: RemoteCommandJob::UniqueIDType last_acknowledged_id_; Nit: s/id/unique_id/ for consistency. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/testing_remote_commands_server.h:55: RemoteCommandJob::UniqueIDType last_generated_unique_id_; Nit: Document that this test server generates strictly monotonically increasing unique IDs. In the general case, the IDs are opaque and do not have to follow any specific pattern. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/testing_remote_commands_server.h:58: base::Lock lock_; Locks are avoided in Chrome code whenever possible. In your case, the lock is definitely avoidable. The task queue should live on one thread, either UI or IO. When the other thread wishes to access the queue, it must post a task and wait for the reply to come back. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/testing_remote_commands_server.h:59: scoped_refptr<base::SingleThreadTaskRunner> task_runner_; Nit 1: base::SequencedTaskRunner or even base::TaskRunner is sufficient here. Nit 2: Document what this is used for. https://codereview.chromium.org/879233003/diff/120001/components/policy/core/... File components/policy/core/common/cloud/cloud_policy_client.cc (right): https://codereview.chromium.org/879233003/diff/120001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client.cc:317: request->clear_command_results(); This is unnecessary. The command results will always start out empty. https://codereview.chromium.org/879233003/diff/120001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client.cc:559: } You forgot to RemoveJob(job). https://codereview.chromium.org/879233003/diff/120001/components/policy/core/... File components/policy/core/common/cloud/cloud_policy_client.h (right): https://codereview.chromium.org/879233003/diff/120001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client.h:53: typedef base::Callback<void( Nit 1: Use C++ using= instead. Nit 2: Document this type. https://codereview.chromium.org/879233003/diff/120001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client.h:147: virtual void FetchRemoteCommands( This should be covered by CloudPolicyClient's unit tests. https://codereview.chromium.org/879233003/diff/120001/components/policy/core/... File components/policy/core/common/cloud/cloud_policy_core.h (right): https://codereview.chromium.org/879233003/diff/120001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_core.h:92: void StartRemoteCommandsService( Why can this not start automatically on connect? https://codereview.chromium.org/879233003/diff/120001/components/policy/core/... File components/policy/core/common/remote_commands/remote_command_job.h (right): https://codereview.chromium.org/879233003/diff/120001/components/policy/core/... components/policy/core/common/remote_commands/remote_command_job.h:41: class Factory { This should be a separate class, in its own header file, i.e. RemoteCommandJobFactory in remote_command_job_factory.h. https://codereview.chromium.org/879233003/diff/120001/components/policy/core/... components/policy/core/common/remote_commands/remote_command_job.h:43: virtual ~Factory() {} Nit: Never inline virtual methods. https://codereview.chromium.org/879233003/diff/120001/components/policy/core/... File components/policy/core/common/remote_commands/remote_commands_service.cc (right): https://codereview.chromium.org/879233003/diff/120001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:23: : request_job_in_progress_(false), Nit: C++11 allows you to initialize this member and the next two in the header file. https://codereview.chromium.org/879233003/diff/120001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:44: base::Unretained(this))); You must use a weak pointer here. Otherwise, the callback may be invoked after |this| has been destroyed. https://codereview.chromium.org/879233003/diff/120001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:45: unsent_results_.clear(); Nit: It would be nice to make this method reentrant by clearing |unsent_results_| before issuing the fetch. https://codereview.chromium.org/879233003/diff/120001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:55: if (command.unique_id() <= last_fetched_command_id_) You must never compare the command IDs to each other. They are opaque identifiers that can increas and decrease at will. https://codereview.chromium.org/879233003/diff/120001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:76: std::max(last_executed_command_id_, command->unique_id()); Always set it to command->unique_id(). Never compare the opaque command IDs to each other. https://codereview.chromium.org/879233003/diff/120001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:106: request_job_in_progress_ = false; Nit: Add a DCHECK(request_job_in_progress_); https://codereview.chromium.org/879233003/diff/120001/components/policy/core/... File components/policy/core/common/remote_commands/remote_commands_service.h (right): https://codereview.chromium.org/879233003/diff/120001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:44: bool request_job_in_progress_; Nit: Would you mind renaming this? "Request" is such a generic term. Something like command_fetch_in_progress_ would be much clearer, and would avoid any potential confusion of "job" with a remote command job.
Patchset #8 (id:140001) has been deleted
Hello Bartosz, Please take a look at the newest patchset. Thanks -Bin https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/c... File chrome/browser/policy/cloud/test_request_interceptor.cc (right): https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/c... chrome/browser/policy/cloud/test_request_interceptor.cc:150: std::string headers(kGoodHeaders, arraysize(kGoodHeaders)); On 2015/02/12 14:29:17, bartfab wrote: > Nit: const. Done. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/c... chrome/browser/policy/cloud/test_request_interceptor.cc:265: return BadRequestJobCallback(request, network_delegate); On 2015/02/12 14:29:17, bartfab wrote: > This early return will cause the callbacks below not to run. Yes, it's designed to not be consumed from callback queue. I'd like to ignore certain package, and to keep this callback for next packet. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/c... chrome/browser/policy/cloud/test_request_interceptor.cc:272: if (!request_serviced_callbacks_.empty()) { On 2015/02/12 14:29:17, bartfab wrote: > These callbacks used to run *before* the ob, now they run *after* the job. Are > you sure this is OK? I think the callback is for intercepting, parsing packages, the actual response from server will be sent back later. So generally I think it won't cause any problem but I'm not so sure. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/c... File chrome/browser/policy/cloud/test_request_interceptor.h (right): https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/c... chrome/browser/policy/cloud/test_request_interceptor.h:37: // If a null URLRequestJob is returned by this callback, it indicates that On 2015/02/12 14:29:17, bartfab wrote: > The grammar is a bit off in this comment, but that is easily fixable. > > I am more concerned about the logic. IIUC, you want to handle the case where > multiple requests arrive in undefined order. Correct? If so, I think we have two > options to handle this cleanly: > 1) Instead of magically using a BadRequestJob, simply step through the callback > queue until a callback is found which can handle the current job. > 2) Alternatively, make sure that requests are issued in the expected order. It's not for out of order packets. It's meant to ignore certain type of packets, treating them as illegal requests instead. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... File chrome/browser/policy/remote_commands/remote_commands_browsertest.cc (right): https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:65: return nullptr; On 2015/02/12 14:29:18, bartfab wrote: > Nit: Add NOTREACHED(). Done. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:87: if (job_result.has_timestamp()) { On 2015/02/12 14:29:18, bartfab wrote: > It would be an error for the job not to have a timestamp. Rather than testing > for this, always use job_result.timestamp() (this will not crash even if no > timestamp was set by the way, courtesy of protobuf). Done. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:110: class RemoteCommandsBrowserTest : public InProcessBrowserTest { On 2015/02/12 14:29:18, bartfab wrote: > Rather than being a browser test, I think this should be a unit test for the > RemoteCommandsService class. This would also avoid the need for all the setup > overhead as you can just create a fake/mock CloudPolicyClient and use that in > the test. I will change this into a unit test https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:138: BrowserPolicyConnector* connector = On 2015/02/12 14:29:18, bartfab wrote: > Nit: s/BrowserPolicyConnector*/BrowserPolicyConnector* const/ Done. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:144: #if !defined(OS_CHROMEOS) On 2015/02/12 14:29:18, bartfab wrote: > I think it is OK to limit ourselves to Chrome OS now. We are only implementing > device commands at the moment, which means we are limited to Chrome OS. Acknowledged. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:161: mock_factory_ = nullptr; On 2015/02/12 14:29:18, bartfab wrote: > No need for this, really. It is a non-owner pointer. Done. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:168: return UserCloudPolicyManagerFactoryChromeOS::GetForProfile( On 2015/02/12 14:29:18, bartfab wrote: > We are only implementing device commands for now. Using the user policy > infrastructure for this is weird. Acknowledged. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:180: MockTestRemoteCommandFactory* mock_factory_; On 2015/02/12 14:29:18, bartfab wrote: > Nit: Initialize with = nullptr. Done. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:192: TestRequestInterceptor::FetchRemoteCommandsJob(server_.get(), true)); On 2015/02/12 14:29:18, bartfab wrote: > Why do we expect two callbacks? First one is for commands fetching, second one is for commands result return back. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:204: policy_manager()->core()->remote_commands_service()->FetchRemoteCommands(); On 2015/02/12 14:29:18, bartfab wrote: > Will connecting the client not issue a remote command fetch? If so, we could > enqueue a command, *then* connect the client and wait for it to issue a command > fetch. Acknowledged. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... File chrome/browser/policy/remote_commands/testing_remote_commands_server.cc (right): https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/testing_remote_commands_server.cc:22: namespace em = enterprise_management; On 2015/02/12 14:29:18, bartfab wrote: > Nit: No need to alias the namespace if you only use it a single time. Acknowledged. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/testing_remote_commands_server.cc:27: : last_acknowledged_id_(0), On 2015/02/12 14:29:19, bartfab wrote: > Nit: In C++ 11, you can initialize ast_acknowledged_id_ and > last_generated_unique_id_ right in the header file. Done. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/testing_remote_commands_server.cc:28: last_generated_unique_id_(1), On 2015/02/12 14:29:18, bartfab wrote: > Nit: 0 is sufficient. Done. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/testing_remote_commands_server.cc:30: task_runner_(base::MessageLoopProxy::current()) { On 2015/02/12 14:29:18, bartfab wrote: > Nit: base::MessageLoopProxy is deprecated. Use > base::ThreadTaskRunnerHandle::Get() instead. Done. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/testing_remote_commands_server.cc:34: DCHECK(task_runner_->BelongsToCurrentThread()); On 2015/02/12 14:29:19, bartfab wrote: > Nit: Use a base::ThreadChecker for this. Done. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/testing_remote_commands_server.cc:46: command.set_unique_id(last_generated_unique_id_++); On 2015/02/12 14:29:18, bartfab wrote: > Nit: If this really is the last generated ID, you should pre-, not > post-increment to get a new ID. Done. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/testing_remote_commands_server.cc:74: EXPECT_GE(last_command_id, last_acknowledged_id_); On 2015/02/12 14:29:18, bartfab wrote: > Nit: Expected value first, actual second. Done. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/testing_remote_commands_server.cc:81: EXPECT_LE(job_result.has_unique_id(), last_command_id); On 2015/02/12 14:29:19, bartfab wrote: > Nit: Expected value first, actual second. Done. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/testing_remote_commands_server.cc:96: base::Unretained(this), job_result)); On 2015/02/12 14:29:18, bartfab wrote: > Do not use base::Unretained when posting across threads. Done. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... File chrome/browser/policy/remote_commands/testing_remote_commands_server.h (right): https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/testing_remote_commands_server.h:25: class TestingRemoteCommandsServer { On 2015/02/12 14:29:19, bartfab wrote: > Without any documentation, it is hard to understand what the methods do and how > they work together. Acknowledged. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/testing_remote_commands_server.h:35: void SetClockForTesting(scoped_ptr<base::Clock> clock); On 2015/02/12 14:29:19, bartfab wrote: > No need for the "ForTesting" prefix. This is testing code after all. Done. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/testing_remote_commands_server.h:38: size_t GetUnreportedCommandCount() const; On 2015/02/12 14:29:19, bartfab wrote: > Nit: "Unreported" is not clear to me. This seems to return the number of > commands for which no results have been sent back yet. I think it would be good > to find a better name that reflects this. Done. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/testing_remote_commands_server.h:45: std::vector<enterprise_management::RemoteCommand>* fetched_commands); On 2015/02/12 14:29:19, bartfab wrote: > Why not have |std::vector<enterprise_management::RemoteCommand> as the return > value? Done. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/testing_remote_commands_server.h:50: const enterprise_management::RemoteCommandResult& job_result) const {} On 2015/02/12 14:29:19, bartfab wrote: > Nit: Never inline virtual methods. The style guide forbids it. Done. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/testing_remote_commands_server.h:54: RemoteCommandJob::UniqueIDType last_acknowledged_id_; On 2015/02/12 14:29:19, bartfab wrote: > Nit: s/id/unique_id/ for consistency. Done. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/testing_remote_commands_server.h:55: RemoteCommandJob::UniqueIDType last_generated_unique_id_; On 2015/02/12 14:29:19, bartfab wrote: > Nit: Document that this test server generates strictly monotonically increasing > unique IDs. In the general case, the IDs are opaque and do not have to follow > any specific pattern. Done. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/testing_remote_commands_server.h:58: base::Lock lock_; On 2015/02/12 14:29:19, bartfab wrote: > Locks are avoided in Chrome code whenever possible. In your case, the lock is > definitely avoidable. The task queue should live on one thread, either UI or IO. > When the other thread wishes to access the queue, it must post a task and wait > for the reply to come back. I will keep the lock as we discussed offline. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/testing_remote_commands_server.h:59: scoped_refptr<base::SingleThreadTaskRunner> task_runner_; On 2015/02/12 14:29:19, bartfab wrote: > Nit 1: base::SequencedTaskRunner or even base::TaskRunner is sufficient here. > Nit 2: Document what this is used for. SingleThreadTaskRunner is required single I need to ensure that task runner bounds to the same thread from constructor. https://codereview.chromium.org/879233003/diff/120001/components/policy/core/... File components/policy/core/common/cloud/cloud_policy_client.cc (right): https://codereview.chromium.org/879233003/diff/120001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client.cc:317: request->clear_command_results(); On 2015/02/12 14:29:19, bartfab wrote: > This is unnecessary. The command results will always start out empty. Done. https://codereview.chromium.org/879233003/diff/120001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client.cc:559: } On 2015/02/12 14:29:19, bartfab wrote: > You forgot to RemoveJob(job). Done. https://codereview.chromium.org/879233003/diff/120001/components/policy/core/... File components/policy/core/common/cloud/cloud_policy_client.h (right): https://codereview.chromium.org/879233003/diff/120001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client.h:53: typedef base::Callback<void( On 2015/02/12 14:29:19, bartfab wrote: > Nit 1: Use C++ using= instead. > Nit 2: Document this type. Done. https://codereview.chromium.org/879233003/diff/120001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client.h:147: virtual void FetchRemoteCommands( On 2015/02/12 14:29:19, bartfab wrote: > This should be covered by CloudPolicyClient's unit tests. Done. https://codereview.chromium.org/879233003/diff/120001/components/policy/core/... File components/policy/core/common/cloud/cloud_policy_core.h (right): https://codereview.chromium.org/879233003/diff/120001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_core.h:92: void StartRemoteCommandsService( On 2015/02/12 14:29:19, bartfab wrote: > Why can this not start automatically on connect? Done. https://codereview.chromium.org/879233003/diff/120001/components/policy/core/... File components/policy/core/common/remote_commands/remote_command_job.h (right): https://codereview.chromium.org/879233003/diff/120001/components/policy/core/... components/policy/core/common/remote_commands/remote_command_job.h:41: class Factory { On 2015/02/12 14:29:20, bartfab wrote: > This should be a separate class, in its own header file, i.e. > RemoteCommandJobFactory in remote_command_job_factory.h. Done. https://codereview.chromium.org/879233003/diff/120001/components/policy/core/... components/policy/core/common/remote_commands/remote_command_job.h:43: virtual ~Factory() {} On 2015/02/12 14:29:19, bartfab wrote: > Nit: Never inline virtual methods. Done. https://codereview.chromium.org/879233003/diff/120001/components/policy/core/... File components/policy/core/common/remote_commands/remote_commands_service.cc (right): https://codereview.chromium.org/879233003/diff/120001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:23: : request_job_in_progress_(false), On 2015/02/12 14:29:20, bartfab wrote: > Nit: C++11 allows you to initialize this member and the next two in the header > file. Done. https://codereview.chromium.org/879233003/diff/120001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:44: base::Unretained(this))); On 2015/02/12 14:29:20, bartfab wrote: > You must use a weak pointer here. Otherwise, the callback may be invoked after > |this| has been destroyed. Done. https://codereview.chromium.org/879233003/diff/120001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:45: unsent_results_.clear(); On 2015/02/12 14:29:20, bartfab wrote: > Nit: It would be nice to make this method reentrant by clearing > |unsent_results_| before issuing the fetch. Done. https://codereview.chromium.org/879233003/diff/120001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:55: if (command.unique_id() <= last_fetched_command_id_) On 2015/02/12 14:29:20, bartfab wrote: > You must never compare the command IDs to each other. They are opaque > identifiers that can increas and decrease at will. Done. https://codereview.chromium.org/879233003/diff/120001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:76: std::max(last_executed_command_id_, command->unique_id()); On 2015/02/12 14:29:20, bartfab wrote: > Always set it to command->unique_id(). Never compare the opaque command IDs to > each other. Done. https://codereview.chromium.org/879233003/diff/120001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:106: request_job_in_progress_ = false; On 2015/02/12 14:29:20, bartfab wrote: > Nit: Add a DCHECK(request_job_in_progress_); Done. https://codereview.chromium.org/879233003/diff/120001/components/policy/core/... File components/policy/core/common/remote_commands/remote_commands_service.h (right): https://codereview.chromium.org/879233003/diff/120001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:44: bool request_job_in_progress_; On 2015/02/12 14:29:20, bartfab wrote: > Nit: Would you mind renaming this? "Request" is such a generic term. Something > like command_fetch_in_progress_ would be much clearer, and would avoid any > potential confusion of "job" with a remote command job. Done.
I reviewed most of your test infrastructure. I will continue reviewing the rest tomorrow. https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/c... File chrome/browser/policy/cloud/test_request_interceptor.h (right): https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/c... chrome/browser/policy/cloud/test_request_interceptor.h:37: // If a null URLRequestJob is returned by this callback, it indicates that On 2015/02/16 22:46:22, binjin wrote: > On 2015/02/12 14:29:17, bartfab wrote: > > The grammar is a bit off in this comment, but that is easily fixable. > > > > I am more concerned about the logic. IIUC, you want to handle the case where > > multiple requests arrive in undefined order. Correct? If so, I think we have > two > > options to handle this cleanly: > > 1) Instead of magically using a BadRequestJob, simply step through the > callback > > queue until a callback is found which can handle the current job. > > 2) Alternatively, make sure that requests are issued in the expected order. > > It's not for out of order packets. It's meant to ignore certain type of packets, > treating them as illegal requests instead. If the order in which requests arrive is well-defined, why can you not enqueue handlers for all of them? For the requests that you do not care about, the handler can just be a no-op. https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/c... File chrome/browser/policy/cloud/test_request_interceptor.cc (right): https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/c... chrome/browser/policy/cloud/test_request_interceptor.cc:162: if (!ValidRequest(request, "remote_commands", &request_msg)) { Nit: No need for {}. https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/c... chrome/browser/policy/cloud/test_request_interceptor.cc:183: std::vector<em::RemoteCommand> fetched_commands = server->FetchCommandsFromIO( Nit: const. https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/c... chrome/browser/policy/cloud/test_request_interceptor.cc:231: std::vector<std::string> ignored_types; Nit: Add trailing underscore. https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/c... chrome/browser/policy/cloud/test_request_interceptor.cc:271: for (const std::string& ignored_type : ignored_types) Nit: Add curly braces. https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... File components/policy/core/common/cloud/cloud_policy_client.cc (right): https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client.cc:321: DeviceManagementRequestJob::Callback job_callback = base::Bind( Nit: const. https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... File components/policy/core/common/cloud/cloud_policy_client.h (right): https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client.h:53: // A callback which receives fetched remote commands protobuf contained in a Nit: Instead of describing the type (it is clear from looking at the code anyway), document what this callback is used for. https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... File components/policy/core/common/remote_commands/testing_remote_commands_server.cc (right): https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.cc:25: using RemoteCommandVector = TestingRemoteCommandsServer::RemoteCommandVector; Nit: Use the definitions from the header file instead of repeating them here. https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.cc:57: RemoteCommandVector TestingRemoteCommandsServer::FetchCommandsFromIO( 1: You accidentally removed the locking. This method races against e.g. IssueCommand() now. 2: How do you know that |this| still exists? https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.cc:60: DCHECK(!thread_checker_.CalledOnValidThread()); Why do you want this method to be *only* called from other threads? With proper locking in place, it can be called from any thread, including the main one. https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.cc:63: EXPECT_LE(last_acknowledged_unique_id_, last_command_id); You are mixing DCHECK and EXPECT. If this class is to be used in tests only, you can use EXPECT throughout. Otherwise, use DCHECK throughout. https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.cc:90: weak_factory_.GetWeakPtr(), job_result)); This is not correct. If you create a weak pointer on UI, you can pass the pointer to IO and use it in tasks posted back to UI. But you cannot create the pointer on IO. https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... File components/policy/core/common/remote_commands/testing_remote_commands_server.h (right): https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:51: // if it's set to true instead, it will be delayed to add the main queue after Nit: s/it will be delayed to add/it will only be added to/ https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:97: // strictly monotonically increasing unique IDs Nit: Add full stop at the end. https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:103: base::Lock lock_; Nit: Document which members are protected by this lock. https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:104: scoped_refptr<base::SingleThreadTaskRunner> task_runner_; Nit: Document which thread this represents.
I finished reviewing everything except the browser test. https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... File components/policy/core/common/cloud/cloud_policy_client_unittest.cc (right): https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client_unittest.cc:48: const char kResultPayLoad[] = "output_payload"; Nit: s/PayLoad/Payload/ https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client_unittest.cc:51: const int64_t kTimeStamp = 987654321; Nit: s/TimeStamp/Timestamp/ https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client_unittest.cc:71: virtual ~MockRemoteCommandsObserver() {} Nit 1: No need for virtual. Nit 2: No need for an explicit destructor in the first place. https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client_unittest.cc:781: CloudPolicyClient::RemoteCommandCallback callback = Nit: const. https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... File components/policy/core/common/cloud/cloud_policy_core.cc (right): https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_core.cc:14: #include "components/policy/core/common/remote_commands/remote_commands_factory.h" Nit: You do not need this include as you never dereference the type. https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... File components/policy/core/common/remote_commands/remote_commands_factory.h (right): https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_factory.h:23: }; Nit: Add DISALLOW_ASSIGN(RemoteCommandsFactory); https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... File components/policy/core/common/remote_commands/remote_commands_service.cc (right): https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:44: last_executed_command_id_, previous_results, s/last_executed_command_id_/last_fetched_command_id_/ Ideally, there should be a test that verifies we do not confuse the two here. https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:74: last_executed_command_id_ = command->unique_id(); You are currently not using |last_executed_command_id_| for anything. Once you add persistence, you should probably use the |last_executed_command_id_| you read back from persistent storage to initialized |last_fetched_command_id_|. I see why we would want to persist this information. I see no need to actually keep it in a member. https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... File components/policy/core/common/remote_commands/remote_commands_service.h (right): https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:36: // in-progress. Returns true if a new command fetch request is created. You are not actually ignoring further fetch requests. You are enqueuing them. This will work, but I think it would be more efficient to just abort the current fetch and start a new one instead. This is what we do for policy fetches as well. https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:57: bool has_unhanded_fetch_request_ = false; Nit 1: s/unhanded/unhandled/ Nit 2: How about s/unhandled/enqueued/ Nit 3: Document what this is used for.
https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... File chrome/browser/policy/remote_commands/remote_commands_browsertest.cc (right): https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:64: MOCK_METHOD0(BuildTestCommand, TestRemoteCommandJob*()); It looks like BuildTestCommand() always does the same thing. You could use WillByDefault() and then only set the number of times that you expect the method to be called in tests. https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:71: NOTREACHED(); Since this is test code, use ADD_FAILURE(). NOTREACHED() would produce a crash. ADD_FAILURE() produces a test failure. https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:81: class MockRemoteCommandsServer : public TestingRemoteCommandsServer { Is this the only implementation of TestingRemoteCommandsServer? If so, why not combine the two classes into one? https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:95: base::Time timestamp = Nit: const. https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:109: NOTREACHED(); Nit: Use ADD_FAILURE() instead. https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:136: ASSERT_FALSE(service_started_); Nit: s/ASSERT/EXPECT/ https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:139: mock_factory_ = factory.get(); Nit: Not used. https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:149: interceptor_->PushJobCallback( Lines 149-151 are copy & pasted to many places. Could you refactor them into a helper method? A nice solution would be to make the RunLoop a member, i.e.: The member is: scoped_ptr<base::RunLoop> run_loop_; You add a helper: void ExpectFetchCommands(size_t expected_command_results, size_t expected_fetched_commands) { run_loop_.reset(new base::RunLoop); interceptor_->PushJobCallback(TestRequestInterceptor::FetchRemoteCommandsJob( server_.get(), expected_command_results, expected_fetched_commands)); interceptor_->AddRequestServicedCallback(run_loop_->QuitClosure()); } Tests can then do: ExpectFetchCommands(0u, 0u); ... whatever command should trigger the fetch... run_loop_->Run(); https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:179: policy_manager()->Connect( It feels like you are using a browser test so that you can obtain a CloudPolicyClient from somewhere. And since the place where you get it from on Chrome OS does not exist for other OSes, you have to create one yourself here. Why not make this all a unit test, where you explicitly create a CloudPolicyClient and pass it in as a dependency? https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:190: server_.reset(); Nit: Is this actually necessary? https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:191: interceptor_.reset(); Nit: Is this actually necessary? https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:206: scoped_ptr<TestRequestInterceptor> interceptor_; Why can this not just be |TestRequestInterceptor interceptor_;|? https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:207: scoped_ptr<MockRemoteCommandsServer> server_; Why can this not just be |MockRemoteCommandsServer server_;|? https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:208: MockTestRemoteCommandFactory* mock_factory_ = nullptr; Nit: Not used. https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:230: EXPECT_TRUE(policy_manager() Nit: This piece is copy & pasted to other places. Please add a helper method for it. https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:241: EXPECT_CALL(*factory, BuildTestCommand()) This code is copy & pasted to other places. Please add a helper method for it. https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:244: true, base::TimeDelta::FromSeconds(1))); Nit: Can you create a constant for |base::TimeDelta::FromSeconds(1)| with a descriptive name? From looking at the code, it is not clear what this one second indicates. https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:316: // delay between command issued and invalidation service triggerred fetch. I am not sure this test is necessary. You tested the various orders of calls. Adding delays should not be necessary. https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:338: .WillOnce(InvokeWithoutArgs(&run_loop, &base::RunLoop::Quit)); AFAICT, all of the code above is shared with the previous test. Please move it to a helper method. https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:343: base::ThreadTaskRunnerHandle::Get()->PostDelayedTask( You cannot do this in tests. Tests must not slow down execution with artificial delay. You *can* PostDelayedTask() with a zero delay though. https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:358: // network is unstable. Nice. Good to see you are testing such an (sadly, all too common) error case. https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:436: // And second a following up fetch request should be ignored. It is not ignored but enqueued. https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:446: // The delayed command should be issued now. In reality, the order would be different: The second command is issued, an invalidation is sent and the second fetch request is made all *before* the first fetch request finishes. I think the wording can be reworked a bit to make this clearer in the comments.
https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/c... File chrome/browser/policy/cloud/test_request_interceptor.h (right): https://codereview.chromium.org/879233003/diff/120001/chrome/browser/policy/c... chrome/browser/policy/cloud/test_request_interceptor.h:37: // If a null URLRequestJob is returned by this callback, it indicates that On 2015/02/17 18:13:52, bartfab wrote: > On 2015/02/16 22:46:22, binjin wrote: > > On 2015/02/12 14:29:17, bartfab wrote: > > > The grammar is a bit off in this comment, but that is easily fixable. > > > > > > I am more concerned about the logic. IIUC, you want to handle the case where > > > multiple requests arrive in undefined order. Correct? If so, I think we have > > two > > > options to handle this cleanly: > > > 1) Instead of magically using a BadRequestJob, simply step through the > > callback > > > queue until a callback is found which can handle the current job. > > > 2) Alternatively, make sure that requests are issued in the expected order. > > > > It's not for out of order packets. It's meant to ignore certain type of > packets, > > treating them as illegal requests instead. > > If the order in which requests arrive is well-defined, why can you not enqueue > handlers for all of them? For the requests that you do not care about, the > handler can just be a no-op. I don't want to handle certain request (policy request in this case) at all, since I need to construct a valid and empty response to stop the clients from retrying policy fetching. https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/c... File chrome/browser/policy/cloud/test_request_interceptor.cc (right): https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/c... chrome/browser/policy/cloud/test_request_interceptor.cc:162: if (!ValidRequest(request, "remote_commands", &request_msg)) { On 2015/02/17 18:13:52, bartfab wrote: > Nit: No need for {}. Done. https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/c... chrome/browser/policy/cloud/test_request_interceptor.cc:183: std::vector<em::RemoteCommand> fetched_commands = server->FetchCommandsFromIO( On 2015/02/17 18:13:52, bartfab wrote: > Nit: const. Done. https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/c... chrome/browser/policy/cloud/test_request_interceptor.cc:231: std::vector<std::string> ignored_types; On 2015/02/17 18:13:52, bartfab wrote: > Nit: Add trailing underscore. Done. https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/c... chrome/browser/policy/cloud/test_request_interceptor.cc:271: for (const std::string& ignored_type : ignored_types) On 2015/02/17 18:13:52, bartfab wrote: > Nit: Add curly braces. Done. https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... File chrome/browser/policy/remote_commands/remote_commands_browsertest.cc (right): https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:64: MOCK_METHOD0(BuildTestCommand, TestRemoteCommandJob*()); On 2015/02/18 13:21:53, bartfab wrote: > It looks like BuildTestCommand() always does the same thing. You could use > WillByDefault() and then only set the number of times that you expect the method > to be called in tests. Done. https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:71: NOTREACHED(); On 2015/02/18 13:21:53, bartfab wrote: > Since this is test code, use ADD_FAILURE(). NOTREACHED() would produce a crash. > ADD_FAILURE() produces a test failure. Done. https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:81: class MockRemoteCommandsServer : public TestingRemoteCommandsServer { On 2015/02/18 13:21:52, bartfab wrote: > Is this the only implementation of TestingRemoteCommandsServer? If so, why not > combine the two classes into one? I would like to to keep the derived class, since these three methods is just meant to make code clearer, and they are too specified to move into the base class. These method are also mocked. https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:95: base::Time timestamp = On 2015/02/18 13:21:53, bartfab wrote: > Nit: const. Done. https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:109: NOTREACHED(); On 2015/02/18 13:21:52, bartfab wrote: > Nit: Use ADD_FAILURE() instead. Done. https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:136: ASSERT_FALSE(service_started_); On 2015/02/18 13:21:52, bartfab wrote: > Nit: s/ASSERT/EXPECT/ Done. https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:139: mock_factory_ = factory.get(); On 2015/02/18 13:21:52, bartfab wrote: > Nit: Not used. Done. https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:149: interceptor_->PushJobCallback( On 2015/02/18 13:21:53, bartfab wrote: > Lines 149-151 are copy & pasted to many places. Could you refactor them into a > helper method? A nice solution would be to make the RunLoop a member, i.e.: > > The member is: > scoped_ptr<base::RunLoop> run_loop_; > > You add a helper: > > void ExpectFetchCommands(size_t expected_command_results, size_t > expected_fetched_commands) { > run_loop_.reset(new base::RunLoop); > interceptor_->PushJobCallback(TestRequestInterceptor::FetchRemoteCommandsJob( > server_.get(), > expected_command_results, > expected_fetched_commands)); > interceptor_->AddRequestServicedCallback(run_loop_->QuitClosure()); > } > > Tests can then do: > > ExpectFetchCommands(0u, 0u); > ... whatever command should trigger the fetch... > run_loop_->Run(); Done. But with more methods used. https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:179: policy_manager()->Connect( On 2015/02/18 13:21:52, bartfab wrote: > It feels like you are using a browser test so that you can obtain a > CloudPolicyClient from somewhere. And since the place where you get it from on > Chrome OS does not exist for other OSes, you have to create one yourself here. > > Why not make this all a unit test, where you explicitly create a > CloudPolicyClient and pass it in as a dependency? I'm not sure, but TestRequestInteceptor is used in this CL. It's on IO thread and the code is at "chrome/". Eventually I will add browser tests for each remote commands implementation based on classes here. So I prefer to keep these browser tests. https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:190: server_.reset(); On 2015/02/18 13:21:52, bartfab wrote: > Nit: Is this actually necessary? It can only be destroyed on main thread. https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:191: interceptor_.reset(); On 2015/02/18 13:21:52, bartfab wrote: > Nit: Is this actually necessary? It depends on |server_|. https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:206: scoped_ptr<TestRequestInterceptor> interceptor_; On 2015/02/18 13:21:52, bartfab wrote: > Why can this not just be |TestRequestInterceptor interceptor_;|? Use of TestRequestInterceptor is kind of restricted and I think it can only not be initialized before main thread is ready. https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:207: scoped_ptr<MockRemoteCommandsServer> server_; On 2015/02/18 13:21:53, bartfab wrote: > Why can this not just be |MockRemoteCommandsServer server_;|? constructor of TestingRemoteCommandsServer will try to get TaskRunner for current thread, so it cannot be initialized before main thread is ready. https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:208: MockTestRemoteCommandFactory* mock_factory_ = nullptr; On 2015/02/18 13:21:52, bartfab wrote: > Nit: Not used. Done. https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:230: EXPECT_TRUE(policy_manager() On 2015/02/18 13:21:52, bartfab wrote: > Nit: This piece is copy & pasted to other places. Please add a helper method for > it. Done. https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:241: EXPECT_CALL(*factory, BuildTestCommand()) On 2015/02/18 13:21:52, bartfab wrote: > This code is copy & pasted to other places. Please add a helper method for it. Done. Used WillByDefault() though. https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:244: true, base::TimeDelta::FromSeconds(1))); On 2015/02/18 13:21:53, bartfab wrote: > Nit: Can you create a constant for |base::TimeDelta::FromSeconds(1)| with a > descriptive name? From looking at the code, it is not clear what this one second > indicates. I'm not sure if it's okay to create a global instance of TimeDelta which have its own ctor. Added a int constant instead. https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:316: // delay between command issued and invalidation service triggerred fetch. On 2015/02/18 13:21:53, bartfab wrote: > I am not sure this test is necessary. You tested the various orders of calls. > Adding delays should not be necessary. Deleted. https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:338: .WillOnce(InvokeWithoutArgs(&run_loop, &base::RunLoop::Quit)); On 2015/02/18 13:21:53, bartfab wrote: > AFAICT, all of the code above is shared with the previous test. Please move it > to a helper method. Acknowledged. https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:343: base::ThreadTaskRunnerHandle::Get()->PostDelayedTask( On 2015/02/18 13:21:52, bartfab wrote: > You cannot do this in tests. Tests must not slow down execution with artificial > delay. You *can* PostDelayedTask() with a zero delay though. Acknowledged. https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:358: // network is unstable. On 2015/02/18 13:21:52, bartfab wrote: > Nice. Good to see you are testing such an (sadly, all too common) error case. Acknowledged. https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:436: // And second a following up fetch request should be ignored. On 2015/02/18 13:21:52, bartfab wrote: > It is not ignored but enqueued. Done. https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:446: // The delayed command should be issued now. On 2015/02/18 13:21:52, bartfab wrote: > In reality, the order would be different: The second command is issued, an > invalidation is sent and the second fetch request is made all *before* the first > fetch request finishes. I think the wording can be reworked a bit to make this > clearer in the comments. Done. https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... File components/policy/core/common/cloud/cloud_policy_client.cc (right): https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client.cc:321: DeviceManagementRequestJob::Callback job_callback = base::Bind( On 2015/02/17 18:13:52, bartfab wrote: > Nit: const. Done. https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... File components/policy/core/common/cloud/cloud_policy_client.h (right): https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client.h:53: // A callback which receives fetched remote commands protobuf contained in a On 2015/02/17 18:13:52, bartfab wrote: > Nit: Instead of describing the type (it is clear from looking at the code > anyway), document what this callback is used for. Done. https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... File components/policy/core/common/cloud/cloud_policy_client_unittest.cc (right): https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client_unittest.cc:48: const char kResultPayLoad[] = "output_payload"; On 2015/02/18 11:12:53, bartfab wrote: > Nit: s/PayLoad/Payload/ Done. https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client_unittest.cc:51: const int64_t kTimeStamp = 987654321; On 2015/02/18 11:12:53, bartfab wrote: > Nit: s/TimeStamp/Timestamp/ Done. https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client_unittest.cc:71: virtual ~MockRemoteCommandsObserver() {} On 2015/02/18 11:12:53, bartfab wrote: > Nit 1: No need for virtual. > Nit 2: No need for an explicit destructor in the first place. Done. https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client_unittest.cc:781: CloudPolicyClient::RemoteCommandCallback callback = On 2015/02/18 11:12:53, bartfab wrote: > Nit: const. Done. https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... File components/policy/core/common/cloud/cloud_policy_core.cc (right): https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_core.cc:14: #include "components/policy/core/common/remote_commands/remote_commands_factory.h" On 2015/02/18 11:12:53, bartfab wrote: > Nit: You do not need this include as you never dereference the type. Looks like it's required for default deleter due to use of scoped_ptr. https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... File components/policy/core/common/remote_commands/remote_commands_factory.h (right): https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_factory.h:23: }; On 2015/02/18 11:12:53, bartfab wrote: > Nit: Add DISALLOW_ASSIGN(RemoteCommandsFactory); Done. https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... File components/policy/core/common/remote_commands/remote_commands_service.cc (right): https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:44: last_executed_command_id_, previous_results, On 2015/02/18 11:12:53, bartfab wrote: > s/last_executed_command_id_/last_fetched_command_id_/ > > Ideally, there should be a test that verifies we do not confuse the two here. As discussed offline, I think it's better to acknowledge server with last executed id think it will recover better. Also add set based container (instead of queue) and some comments regarding this. https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:74: last_executed_command_id_ = command->unique_id(); On 2015/02/18 11:12:53, bartfab wrote: > You are currently not using |last_executed_command_id_| for anything. Once you > add persistence, you should probably use the |last_executed_command_id_| you > read back from persistent storage to initialized |last_fetched_command_id_|. > > I see why we would want to persist this information. I see no need to actually > keep it in a member. See above comments, |last_executed_command_id_| will be used for fetching requests. Deleted |last_fetched_command_id_| instead since we compare and ignore commands with existing IDs now. https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... File components/policy/core/common/remote_commands/remote_commands_service.h (right): https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:36: // in-progress. Returns true if a new command fetch request is created. On 2015/02/18 11:12:53, bartfab wrote: > You are not actually ignoring further fetch requests. You are enqueuing them. > This will work, but I think it would be more efficient to just abort the current > fetch and start a new one instead. This is what we do for policy fetches as > well. 1. Recent changes (to allow multiple non-policy-fetching connection) to CloudPolicyClient makes resetting connection harder than before. 2. I think reliability (retrying and timeout) should be provided by DeviceManagementService and thus it's safe to take current approach. 3. Changed comments: ignored -> enqueued https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:57: bool has_unhanded_fetch_request_ = false; On 2015/02/18 11:12:53, bartfab wrote: > Nit 1: s/unhanded/unhandled/ > Nit 2: How about s/unhandled/enqueued/ > Nit 3: Document what this is used for. Done. https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... File components/policy/core/common/remote_commands/testing_remote_commands_server.cc (right): https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.cc:25: using RemoteCommandVector = TestingRemoteCommandsServer::RemoteCommandVector; On 2015/02/17 18:13:53, bartfab wrote: > Nit: Use the definitions from the header file instead of repeating them here. Done. https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.cc:57: RemoteCommandVector TestingRemoteCommandsServer::FetchCommandsFromIO( On 2015/02/17 18:13:53, bartfab wrote: > 1: You accidentally removed the locking. This method races against e.g. > IssueCommand() now. > 2: How do you know that |this| still exists? 1. Done. 2. I don't think it's necessary to add a comments here requiring this to be valid when calling this method. Instead, I have comments for TestRequestInterceptor::FetchRemoteCommandsJob which requiring this test server to be valid when the request is intercepted. https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.cc:60: DCHECK(!thread_checker_.CalledOnValidThread()); On 2015/02/17 18:13:53, bartfab wrote: > Why do you want this method to be *only* called from other threads? With proper > locking in place, it can be called from any thread, including the main one. Done. Also Changed name/comments. https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.cc:63: EXPECT_LE(last_acknowledged_unique_id_, last_command_id); On 2015/02/17 18:13:53, bartfab wrote: > You are mixing DCHECK and EXPECT. If this class is to be used in tests only, you > can use EXPECT throughout. Otherwise, use DCHECK throughout. Done. But left thread checking with DCHECK(). https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.cc:90: weak_factory_.GetWeakPtr(), job_result)); On 2015/02/17 18:13:53, bartfab wrote: > This is not correct. If you create a weak pointer on UI, you can pass the > pointer to IO and use it in tasks posted back to UI. But you cannot create the > pointer on IO. Done. https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... File components/policy/core/common/remote_commands/testing_remote_commands_server.h (right): https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:51: // if it's set to true instead, it will be delayed to add the main queue after On 2015/02/17 18:13:53, bartfab wrote: > Nit: s/it will be delayed to add/it will only be added to/ Done. https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:97: // strictly monotonically increasing unique IDs On 2015/02/17 18:13:53, bartfab wrote: > Nit: Add full stop at the end. Done. https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:103: base::Lock lock_; On 2015/02/17 18:13:53, bartfab wrote: > Nit: Document which members are protected by this lock. Done. https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:104: scoped_refptr<base::SingleThreadTaskRunner> task_runner_; On 2015/02/17 18:13:53, bartfab wrote: > Nit: Document which thread this represents. Done.
The CL is coming together nicely. I still think that most if not all of the current testing code should move to a unit test. Other than that, things are looking good now. https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... File chrome/browser/policy/remote_commands/remote_commands_browsertest.cc (right): https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:81: class MockRemoteCommandsServer : public TestingRemoteCommandsServer { On 2015/02/18 15:37:49, binjin wrote: > On 2015/02/18 13:21:52, bartfab wrote: > > Is this the only implementation of TestingRemoteCommandsServer? If so, why not > > combine the two classes into one? > > I would like to to keep the derived class, since these three methods is just > meant to make code clearer, and they are too specified to move into the base > class. These method are also mocked. I understand the purpose of MockRemoteCommandsServer and its mock methods. But what purpose does the bare TestingRemoteCommandsServer serve? Nobody uses it in that form. https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:179: policy_manager()->Connect( On 2015/02/18 15:37:49, binjin wrote: > On 2015/02/18 13:21:52, bartfab wrote: > > It feels like you are using a browser test so that you can obtain a > > CloudPolicyClient from somewhere. And since the place where you get it from on > > Chrome OS does not exist for other OSes, you have to create one yourself here. > > > > Why not make this all a unit test, where you explicitly create a > > CloudPolicyClient and pass it in as a dependency? > > I'm not sure, but TestRequestInteceptor is used in this CL. It's on IO thread > and the code is at "chrome/". > > Eventually I will add browser tests for each remote commands implementation > based on classes here. So I prefer to keep these browser tests. Having a browser test for each command is definitely a good idea. And using a common base class for all these tests is also good. However: 1) We are only implementing device commands for now, so your code, which uses the current user's command queue, will not be appropriate. 2) Tests that verify the actual commands work correctly belong into a browser test. But tests that verify the RemoteCommandsService behaves expected belong into a unit test. That unit test will not need the TestRequestInterceptor, because you can mock out the entire fetching as well. https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:244: true, base::TimeDelta::FromSeconds(1))); On 2015/02/18 15:37:49, binjin wrote: > On 2015/02/18 13:21:53, bartfab wrote: > > Nit: Can you create a constant for |base::TimeDelta::FromSeconds(1)| with a > > descriptive name? From looking at the code, it is not clear what this one > second > > indicates. > > I'm not sure if it's okay to create a global instance of TimeDelta which have > its own ctor. Added a int constant instead. The way you did it is correct. A static initializer would not have been allowed. https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... File components/policy/core/common/cloud/cloud_policy_core.cc (right): https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_core.cc:14: #include "components/policy/core/common/remote_commands/remote_commands_factory.h" On 2015/02/18 15:37:50, binjin wrote: > On 2015/02/18 11:12:53, bartfab wrote: > > Nit: You do not need this include as you never dereference the type. > > Looks like it's required for default deleter due to use of scoped_ptr. Interesting. You learn something new every day :). https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... File components/policy/core/common/remote_commands/remote_commands_service.cc (right): https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:74: last_executed_command_id_ = command->unique_id(); On 2015/02/18 15:37:50, binjin wrote: > On 2015/02/18 11:12:53, bartfab wrote: > > You are currently not using |last_executed_command_id_| for anything. Once you > > add persistence, you should probably use the |last_executed_command_id_| you > > read back from persistent storage to initialized |last_fetched_command_id_|. > > > > I see why we would want to persist this information. I see no need to actually > > keep it in a member. > > See above comments, |last_executed_command_id_| will be used for fetching > requests. Deleted |last_fetched_command_id_| instead since we compare and ignore > commands with existing IDs now. We should think about the error scenario that a command's execution causes a crash. If we do not add some special-case handling for this, a command that triggers a bug can get Chrome into a nasty crash loop (until the command expires). Or maybe just looping until the command expires is good enough. https://codereview.chromium.org/879233003/diff/260001/chrome/browser/policy/r... File chrome/browser/policy/remote_commands/remote_commands_browsertest.cc (right): https://codereview.chromium.org/879233003/diff/260001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:10: #include "base/logging.h" Nit: Not used. https://codereview.chromium.org/879233003/diff/260001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:393: .WillOnce(InvokeWithoutArgs(run_loop_.get(), &base::RunLoop::Quit)); Nit: This EXPECT_CALL() appears in at least four places. Please move it to a helper method. https://codereview.chromium.org/879233003/diff/260001/components/policy/core/... File components/policy/core/common/remote_commands/remote_commands_service.h (right): https://codereview.chromium.org/879233003/diff/260001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:69: // ID for the latest command which has started execution. We will acknowledge This will not work for the reboot command. Currently, the reboot command is implemented as follows: 1) RemoteCommandService receives a reboot command and places it in the RemoteCommandsQueue. 2) The command starts execution and reboots the device. 3) After the reboot, the RemoteCommandService talks to the server, receives the same reboot command and places it in the RemoteCommandsQueue again. 4) The command starts execution. It notices that the device has been rebooted already since the command was issued and reports success to the server, without rebooting again. If you acknowledge commands the moment they start execution, you will remove the command in (2) and the device will not see it again in (3). https://codereview.chromium.org/879233003/diff/260001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:75: // is not guaranteed to be increasing. Nit: s/not guaranteed to be increasing/opaque/ It is not just about decreasing/increasing. The ID is simply 100% opaque and we must never try to reason about it. https://codereview.chromium.org/879233003/diff/260001/components/policy/core/... File components/policy/core/common/remote_commands/testing_remote_commands_server.cc (right): https://codereview.chromium.org/879233003/diff/260001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.cc:59: const TestingRemoteCommandsServer::RemoteCommandResultVector& Nit: No need for the TestingRemoteCommandsServer:: prefix. https://codereview.chromium.org/879233003/diff/260001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.cc:92: TestingRemoteCommandsServer::RemoteCommandVector fetched_commands; Nit: No need for the TestingRemoteCommandsServer:: prefix. https://codereview.chromium.org/879233003/diff/260001/components/policy/core/... File components/policy/core/common/remote_commands/testing_remote_commands_server.h (right): https://codereview.chromium.org/879233003/diff/260001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:40: : public base::SupportsWeakPtr<TestingRemoteCommandsServer> { Nit: Please go back to using a WeakPtrFactory. SupportsWeakPtr is deprecated. https://codereview.chromium.org/879233003/diff/260001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:112: base::WeakPtr<TestingRemoteCommandsServer> weak_ptr_to_this_; Nit: Document that this weak pointer is created early so that it can be accessed from other threads.
https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... File chrome/browser/policy/remote_commands/remote_commands_browsertest.cc (right): https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:81: class MockRemoteCommandsServer : public TestingRemoteCommandsServer { On 2015/02/23 13:13:56, bartfab wrote: > On 2015/02/18 15:37:49, binjin wrote: > > On 2015/02/18 13:21:52, bartfab wrote: > > > Is this the only implementation of TestingRemoteCommandsServer? If so, why > not > > > combine the two classes into one? > > > > I would like to to keep the derived class, since these three methods is just > > meant to make code clearer, and they are too specified to move into the base > > class. These method are also mocked. > > I understand the purpose of MockRemoteCommandsServer and its mock methods. But > what purpose does the bare TestingRemoteCommandsServer serve? Nobody uses it in > that form. Class removed. N/A now https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:179: policy_manager()->Connect( On 2015/02/23 13:13:56, bartfab wrote: > On 2015/02/18 15:37:49, binjin wrote: > > On 2015/02/18 13:21:52, bartfab wrote: > > > It feels like you are using a browser test so that you can obtain a > > > CloudPolicyClient from somewhere. And since the place where you get it from > on > > > Chrome OS does not exist for other OSes, you have to create one yourself > here. > > > > > > Why not make this all a unit test, where you explicitly create a > > > CloudPolicyClient and pass it in as a dependency? > > > > I'm not sure, but TestRequestInteceptor is used in this CL. It's on IO thread > > and the code is at "chrome/". > > > > Eventually I will add browser tests for each remote commands implementation > > based on classes here. So I prefer to keep these browser tests. > > Having a browser test for each command is definitely a good idea. And using a > common base class for all these tests is also good. However: > > 1) We are only implementing device commands for now, so your code, which uses > the current user's command queue, will not be appropriate. > > 2) Tests that verify the actual commands work correctly belong into a browser > test. But tests that verify the RemoteCommandsService behaves expected belong > into a unit test. That unit test will not need the TestRequestInterceptor, > because you can mock out the entire fetching as well. 1) It's always okay to redefine policy_manager() latter or left it being virtual for actual browser tests on various platform. 2) I managed to migrate all browser tests to unit tests. The test server previously runs on a different thread in browser tests and now runs on the main thread in unit tests. Since I inherited a lot of things from browser tests, the current code looks very ugly. https://codereview.chromium.org/879233003/diff/240001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:244: true, base::TimeDelta::FromSeconds(1))); On 2015/02/23 13:13:56, bartfab wrote: > On 2015/02/18 15:37:49, binjin wrote: > > On 2015/02/18 13:21:53, bartfab wrote: > > > Nit: Can you create a constant for |base::TimeDelta::FromSeconds(1)| with a > > > descriptive name? From looking at the code, it is not clear what this one > > second > > > indicates. > > > > I'm not sure if it's okay to create a global instance of TimeDelta which have > > its own ctor. Added a int constant instead. > > The way you did it is correct. A static initializer would not have been allowed. Acknowledged. https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... File components/policy/core/common/cloud/cloud_policy_core.cc (right): https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_core.cc:14: #include "components/policy/core/common/remote_commands/remote_commands_factory.h" On 2015/02/23 13:13:56, bartfab wrote: > On 2015/02/18 15:37:50, binjin wrote: > > On 2015/02/18 11:12:53, bartfab wrote: > > > Nit: You do not need this include as you never dereference the type. > > > > Looks like it's required for default deleter due to use of scoped_ptr. > > Interesting. You learn something new every day :). Acknowledged. https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... File components/policy/core/common/remote_commands/remote_commands_service.cc (right): https://codereview.chromium.org/879233003/diff/240001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:74: last_executed_command_id_ = command->unique_id(); On 2015/02/23 13:13:56, bartfab wrote: > On 2015/02/18 15:37:50, binjin wrote: > > On 2015/02/18 11:12:53, bartfab wrote: > > > You are currently not using |last_executed_command_id_| for anything. Once > you > > > add persistence, you should probably use the |last_executed_command_id_| you > > > read back from persistent storage to initialized |last_fetched_command_id_|. > > > > > > I see why we would want to persist this information. I see no need to > actually > > > keep it in a member. > > > > See above comments, |last_executed_command_id_| will be used for fetching > > requests. Deleted |last_fetched_command_id_| instead since we compare and > ignore > > commands with existing IDs now. > > We should think about the error scenario that a command's execution causes a > crash. If we do not add some special-case handling for this, a command that > triggers a bug can get Chrome into a nasty crash loop (until the command > expires). Or maybe just looping until the command expires is good enough. I think think can be avoided if the server add limits on the number of times a single commands can be fetched. It's reasonable measure although not being neat. For client side code, since the commands needs to crash RemoteCommandService to actually started loops, I think I would rather not acknowledge this commands just after OnJobStarted(), and wait until the results is available. https://codereview.chromium.org/879233003/diff/260001/chrome/browser/policy/r... File chrome/browser/policy/remote_commands/remote_commands_browsertest.cc (right): https://codereview.chromium.org/879233003/diff/260001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:10: #include "base/logging.h" On 2015/02/23 13:13:57, bartfab wrote: > Nit: Not used. Done. https://codereview.chromium.org/879233003/diff/260001/chrome/browser/policy/r... chrome/browser/policy/remote_commands/remote_commands_browsertest.cc:393: .WillOnce(InvokeWithoutArgs(run_loop_.get(), &base::RunLoop::Quit)); On 2015/02/23 13:13:57, bartfab wrote: > Nit: This EXPECT_CALL() appears in at least four places. Please move it to a > helper method. Code removed, N/A now. https://codereview.chromium.org/879233003/diff/260001/components/policy/core/... File components/policy/core/common/remote_commands/remote_commands_service.h (right): https://codereview.chromium.org/879233003/diff/260001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:69: // ID for the latest command which has started execution. We will acknowledge On 2015/02/23 13:13:57, bartfab wrote: > This will not work for the reboot command. Currently, the reboot command is > implemented as follows: > > 1) RemoteCommandService receives a reboot command and places it in the > RemoteCommandsQueue. > 2) The command starts execution and reboots the device. > 3) After the reboot, the RemoteCommandService talks to the server, receives the > same reboot command and places it in the RemoteCommandsQueue again. > 4) The command starts execution. It notices that the device has been rebooted > already since the command was issued and reports success to the server, without > rebooting again. > > If you acknowledge commands the moment they start execution, you will remove the > command in (2) and the device will not see it again in (3). One way to handle this, as we discussed offline, is to add special field to RemoteCommandJob (just like what we did for command timeout time), and allowing certain types of RemoteCommands to force RemoteCommandsService to delay acknowledging of its execution. It's not very hard to implement but it also have a lot of corner cases to consider. I would like to implement this in separated CL since this CL already goes way bigger than what I expected it to be. https://codereview.chromium.org/879233003/diff/260001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:75: // is not guaranteed to be increasing. On 2015/02/23 13:13:57, bartfab wrote: > Nit: s/not guaranteed to be increasing/opaque/ > > It is not just about decreasing/increasing. The ID is simply 100% opaque and we > must never try to reason about it. Done. https://codereview.chromium.org/879233003/diff/260001/components/policy/core/... File components/policy/core/common/remote_commands/testing_remote_commands_server.cc (right): https://codereview.chromium.org/879233003/diff/260001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.cc:59: const TestingRemoteCommandsServer::RemoteCommandResultVector& On 2015/02/23 13:13:57, bartfab wrote: > Nit: No need for the TestingRemoteCommandsServer:: prefix. Done. https://codereview.chromium.org/879233003/diff/260001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.cc:92: TestingRemoteCommandsServer::RemoteCommandVector fetched_commands; On 2015/02/23 13:13:57, bartfab wrote: > Nit: No need for the TestingRemoteCommandsServer:: prefix. Done. https://codereview.chromium.org/879233003/diff/260001/components/policy/core/... File components/policy/core/common/remote_commands/testing_remote_commands_server.h (right): https://codereview.chromium.org/879233003/diff/260001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:40: : public base::SupportsWeakPtr<TestingRemoteCommandsServer> { On 2015/02/23 13:13:57, bartfab wrote: > Nit: Please go back to using a WeakPtrFactory. SupportsWeakPtr is deprecated. Done. https://codereview.chromium.org/879233003/diff/260001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:112: base::WeakPtr<TestingRemoteCommandsServer> weak_ptr_to_this_; On 2015/02/23 13:13:57, bartfab wrote: > Nit: Document that this weak pointer is created early so that it can be accessed > from other threads. Done.
Things are looking good overall, but I am too jet-lagged to approve the CL with confidence tonight. My only remaining question is about persistence - what do we want to persist exactly? Implementing persistence can be done in a separate CL, but I want to make sure that we know what we will store and where we will read it back to. https://codereview.chromium.org/879233003/diff/260001/components/policy/core/... File components/policy/core/common/remote_commands/remote_commands_service.h (right): https://codereview.chromium.org/879233003/diff/260001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:69: // ID for the latest command which has started execution. We will acknowledge On 2015/02/24 05:29:49, binjin wrote: > On 2015/02/23 13:13:57, bartfab wrote: > > This will not work for the reboot command. Currently, the reboot command is > > implemented as follows: > > > > 1) RemoteCommandService receives a reboot command and places it in the > > RemoteCommandsQueue. > > 2) The command starts execution and reboots the device. > > 3) After the reboot, the RemoteCommandService talks to the server, receives > the > > same reboot command and places it in the RemoteCommandsQueue again. > > 4) The command starts execution. It notices that the device has been rebooted > > already since the command was issued and reports success to the server, > without > > rebooting again. > > > > If you acknowledge commands the moment they start execution, you will remove > the > > command in (2) and the device will not see it again in (3). > > One way to handle this, as we discussed offline, is to add special field to > RemoteCommandJob (just like what we did for command timeout time), and allowing > certain types of RemoteCommands to force RemoteCommandsService to delay > acknowledging of its execution. It's not very hard to implement but it also have > a lot of corner cases to consider. I would like to implement this in separated > CL since this CL already goes way bigger than what I expected it to be. I have no problem with separating this piece out into a separate CL, if we have a solid idea how it will work. What ID will we persist in this case? What ID will we send up to the server? Speaking of which, do you want to add persistence in this CL or would you prefer to do that in a follow-up as well? https://codereview.chromium.org/879233003/diff/280001/components/policy/core/... File components/policy/core/common/remote_commands/remote_commands_service.h (right): https://codereview.chromium.org/879233003/diff/280001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:48: friend class RemoteCommandsServiceTest; How about exposing a SetClockForTesting() method instead? https://codereview.chromium.org/879233003/diff/280001/components/policy/core/... File components/policy/core/common/remote_commands/remote_commands_service_unittest.cc (right): https://codereview.chromium.org/879233003/diff/280001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service_unittest.cc:55: ~MockTestRemoteCommandFactory() override {} Nit: Why do you need this empty destructor? https://codereview.chromium.org/879233003/diff/280001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service_unittest.cc:78: : CloudPolicyClient(std::string(), Nit: With argument chains like this one, we tend to document the argument names via in-line commands, e.g.: CloudPolicyClient(std::string(), /* machine_id */ std::string(), /* machine_model */ ... https://codereview.chromium.org/879233003/diff/280001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service_unittest.cc:92: // |commands_fetched_callback| will be executed after the the fetch is Nit: s/the the/the/ https://codereview.chromium.org/879233003/diff/280001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service_unittest.cc:113: size_t expected_command_results; Nit: const. https://codereview.chromium.org/879233003/diff/280001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service_unittest.cc:114: size_t expected_fetched_commands; Nit: const. https://codereview.chromium.org/879233003/diff/280001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service_unittest.cc:115: base::Closure commands_fetched_callback; Nit: const. https://codereview.chromium.org/879233003/diff/280001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service_unittest.cc:122: EXPECT_FALSE(expected_fetch_commands_calls_.empty()); Nit: ASSERT_FALSE as otherwise, you will crash a few lines down. https://codereview.chromium.org/879233003/diff/280001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service_unittest.cc:124: FetchCallExpectation fetch_call_expectation = Nit: const. https://codereview.chromium.org/879233003/diff/280001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service_unittest.cc:144: std::vector<em::RemoteCommand> fetched_commands = Nit: const. https://codereview.chromium.org/879233003/diff/280001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service_unittest.cc:163: TestingRemoteCommandsServer* server_; Nit: const. https://codereview.chromium.org/879233003/diff/280001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service_unittest.cc:171: class ScopedRunner { This is essentially RunLoop for the TestMockTimeTaskRunner. I think it would make sense to move this over to the core TestMockTimeTaskRunner implementation, where it can benefit other tests as well. https://codereview.chromium.org/879233003/diff/280001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service_unittest.cc:176: task_runner_->Attach(); It would be nice to keep track of state entirely within the ScopedRunner. Analogously to RunLoop, it should be possible to attach any number of ScopedRunners to the same TestMockTimeTaskRunner. https://codereview.chromium.org/879233003/diff/280001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service_unittest.cc:242: // Base class for all browser tests regarding remote commands service. Nit: s/browser/unit/ https://codereview.chromium.org/879233003/diff/280001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service_unittest.cc:248: ~RemoteCommandsServiceTest() override {} Nit: Why do you need this empty destructor? https://codereview.chromium.org/879233003/diff/280001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service_unittest.cc:270: void RunUntilIdle() { Nit: This is actually different from the familiar RunLoop::RunUntilIdle(). FastForwardUntilNoTasksRemain() will execute delayed tasks as well, while RunLoop::RunUntilIdle() will not. How about calling this FlushAllTasks()? https://codereview.chromium.org/879233003/diff/280001/components/policy/core/... File components/policy/core/common/remote_commands/testing_remote_commands_server.cc (right): https://codereview.chromium.org/879233003/diff/280001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.cc:30: em::RemoteCommand command_proto; Nit: const. https://codereview.chromium.org/879233003/diff/280001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.cc:31: ResultReportedCallback reported_callback; Nit: const. https://codereview.chromium.org/879233003/diff/280001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.cc:66: RemoteCommandWithCallback command_with_callback(command, reported_callback); Nit: const. https://codereview.chromium.org/879233003/diff/280001/components/policy/core/... File components/policy/core/common/remote_commands/testing_remote_commands_server.h (right): https://codereview.chromium.org/879233003/diff/280001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:11: #include "base/callback.h" Nit: callback_forward.h should be sufficient here.
https://codereview.chromium.org/879233003/diff/260001/components/policy/core/... File components/policy/core/common/remote_commands/remote_commands_service.h (right): https://codereview.chromium.org/879233003/diff/260001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:69: // ID for the latest command which has started execution. We will acknowledge On 2015/02/27 03:59:51, bartfab (traveling - busy) wrote: > On 2015/02/24 05:29:49, binjin wrote: > > On 2015/02/23 13:13:57, bartfab wrote: > > > This will not work for the reboot command. Currently, the reboot command is > > > implemented as follows: > > > > > > 1) RemoteCommandService receives a reboot command and places it in the > > > RemoteCommandsQueue. > > > 2) The command starts execution and reboots the device. > > > 3) After the reboot, the RemoteCommandService talks to the server, receives > > the > > > same reboot command and places it in the RemoteCommandsQueue again. > > > 4) The command starts execution. It notices that the device has been > rebooted > > > already since the command was issued and reports success to the server, > > without > > > rebooting again. > > > > > > If you acknowledge commands the moment they start execution, you will remove > > the > > > command in (2) and the device will not see it again in (3). > > > > One way to handle this, as we discussed offline, is to add special field to > > RemoteCommandJob (just like what we did for command timeout time), and > allowing > > certain types of RemoteCommands to force RemoteCommandsService to delay > > acknowledging of its execution. It's not very hard to implement but it also > have > > a lot of corner cases to consider. I would like to implement this in separated > > CL since this CL already goes way bigger than what I expected it to be. > > I have no problem with separating this piece out into a separate CL, if we have > a solid idea how it will work. What ID will we persist in this case? What ID > will we send up to the server? > > Speaking of which, do you want to add persistence in this CL or would you prefer > to do that in a follow-up as well? I want to add persistence in separated CL. I think the choice between last fetched ID and last executed ID is clear, it only matters if a command is fetched but doesn't get a chance to start execution, so I will write last executed ID to local state, and acknowledging the server with last executed ID as well (We need to remove duplicated remote commands later). There are subtle differences regarding the time we sync the last executed ID to local disk and server, OnJobStarted or OnJobFinished. These two approach only matters if the current running command will crash the system before finishing. In such case, "OnJobStarted" approach will make the commands like reboot command run only once (we discussed and agreed that running reboot command twice is much simpler to implement if we want to report success back to server after reboot), "OnJobFinished" will potentially lead to unintended commands loop. I currently have no better idea on this, one approach is leave this decision to commands implementations, "OnJobStarted", "OnJobFinished" or even "postpone reporting and blocking all commands in the queue". https://codereview.chromium.org/879233003/diff/280001/components/policy/core/... File components/policy/core/common/remote_commands/remote_commands_service.h (right): https://codereview.chromium.org/879233003/diff/280001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:48: friend class RemoteCommandsServiceTest; On 2015/02/27 03:59:51, bartfab (traveling - busy) wrote: > How about exposing a SetClockForTesting() method instead? Done. https://codereview.chromium.org/879233003/diff/280001/components/policy/core/... File components/policy/core/common/remote_commands/remote_commands_service_unittest.cc (right): https://codereview.chromium.org/879233003/diff/280001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service_unittest.cc:55: ~MockTestRemoteCommandFactory() override {} On 2015/02/27 03:59:51, bartfab (traveling - busy) wrote: > Nit: Why do you need this empty destructor? Done. https://codereview.chromium.org/879233003/diff/280001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service_unittest.cc:78: : CloudPolicyClient(std::string(), On 2015/02/27 03:59:51, bartfab (traveling - busy) wrote: > Nit: With argument chains like this one, we tend to document the argument names > via in-line commands, e.g.: > > CloudPolicyClient(std::string(), /* machine_id */ > std::string(), /* machine_model */ > ... Done. https://codereview.chromium.org/879233003/diff/280001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service_unittest.cc:92: // |commands_fetched_callback| will be executed after the the fetch is On 2015/02/27 03:59:52, bartfab (traveling - busy) wrote: > Nit: s/the the/the/ Done. https://codereview.chromium.org/879233003/diff/280001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service_unittest.cc:113: size_t expected_command_results; On 2015/02/27 03:59:52, bartfab (traveling - busy) wrote: > Nit: const. Done. https://codereview.chromium.org/879233003/diff/280001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service_unittest.cc:114: size_t expected_fetched_commands; On 2015/02/27 03:59:52, bartfab (traveling - busy) wrote: > Nit: const. Done. https://codereview.chromium.org/879233003/diff/280001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service_unittest.cc:115: base::Closure commands_fetched_callback; On 2015/02/27 03:59:51, bartfab (traveling - busy) wrote: > Nit: const. Done. https://codereview.chromium.org/879233003/diff/280001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service_unittest.cc:122: EXPECT_FALSE(expected_fetch_commands_calls_.empty()); On 2015/02/27 03:59:52, bartfab (traveling - busy) wrote: > Nit: ASSERT_FALSE as otherwise, you will crash a few lines down. Done. https://codereview.chromium.org/879233003/diff/280001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service_unittest.cc:124: FetchCallExpectation fetch_call_expectation = On 2015/02/27 03:59:52, bartfab (traveling - busy) wrote: > Nit: const. Done. https://codereview.chromium.org/879233003/diff/280001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service_unittest.cc:144: std::vector<em::RemoteCommand> fetched_commands = On 2015/02/27 03:59:52, bartfab (traveling - busy) wrote: > Nit: const. Done. https://codereview.chromium.org/879233003/diff/280001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service_unittest.cc:163: TestingRemoteCommandsServer* server_; On 2015/02/27 03:59:51, bartfab (traveling - busy) wrote: > Nit: const. I didn't get it. server_->FetchCommands() cannot be called with constant pointer. https://codereview.chromium.org/879233003/diff/280001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service_unittest.cc:171: class ScopedRunner { On 2015/02/27 03:59:52, bartfab (traveling - busy) wrote: > This is essentially RunLoop for the TestMockTimeTaskRunner. I think it would > make sense to move this over to the core TestMockTimeTaskRunner implementation, > where it can benefit other tests as well. I want to keep the implementation here, since 1) we already reused a lot of code, it's easy to write another ScopedMockTimeTaskRunner. 2) It's not general enough as RunLoop, for example it cannot be nested. https://codereview.chromium.org/879233003/diff/280001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service_unittest.cc:176: task_runner_->Attach(); On 2015/02/27 03:59:52, bartfab (traveling - busy) wrote: > It would be nice to keep track of state entirely within the ScopedRunner. > Analogously to RunLoop, it should be possible to attach any number of > ScopedRunners to the same TestMockTimeTaskRunner. It's not possible to attach multiple ScopedRunner to the same TestMockTimeTaskRunner, since the condition to stop virtual time elapsing can only be applied to single ScopedRunner. Moved "run_called_" and "quited_called_" into ScopedRunner. Replaced "attached_" With a pointer to ScopedRunner. https://codereview.chromium.org/879233003/diff/280001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service_unittest.cc:242: // Base class for all browser tests regarding remote commands service. On 2015/02/27 03:59:51, bartfab (traveling - busy) wrote: > Nit: s/browser/unit/ Done. https://codereview.chromium.org/879233003/diff/280001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service_unittest.cc:248: ~RemoteCommandsServiceTest() override {} On 2015/02/27 03:59:51, bartfab (traveling - busy) wrote: > Nit: Why do you need this empty destructor? Done. https://codereview.chromium.org/879233003/diff/280001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service_unittest.cc:270: void RunUntilIdle() { On 2015/02/27 03:59:51, bartfab (traveling - busy) wrote: > Nit: This is actually different from the familiar RunLoop::RunUntilIdle(). > FastForwardUntilNoTasksRemain() will execute delayed tasks as well, while > RunLoop::RunUntilIdle() will not. How about calling this FlushAllTasks()? Done. https://codereview.chromium.org/879233003/diff/280001/components/policy/core/... File components/policy/core/common/remote_commands/testing_remote_commands_server.cc (right): https://codereview.chromium.org/879233003/diff/280001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.cc:30: em::RemoteCommand command_proto; On 2015/02/27 03:59:52, bartfab (traveling - busy) wrote: > Nit: const. Unable to mark this constant, since stl container is used below and compiler complains since assignment operator is used in stl code. https://codereview.chromium.org/879233003/diff/280001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.cc:31: ResultReportedCallback reported_callback; On 2015/02/27 03:59:52, bartfab (traveling - busy) wrote: > Nit: const. See above. https://codereview.chromium.org/879233003/diff/280001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.cc:66: RemoteCommandWithCallback command_with_callback(command, reported_callback); On 2015/02/27 03:59:52, bartfab (traveling - busy) wrote: > Nit: const. Done. https://codereview.chromium.org/879233003/diff/280001/components/policy/core/... File components/policy/core/common/remote_commands/testing_remote_commands_server.h (right): https://codereview.chromium.org/879233003/diff/280001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:11: #include "base/callback.h" On 2015/02/27 03:59:52, bartfab (traveling - busy) wrote: > Nit: callback_forward.h should be sufficient here. Done.
I finished reviewing everything except the two unit tests. I will do those next. As I think that this CL is approaching its final state, I included all my comment grammar and include policy nits this time around :). https://codereview.chromium.org/879233003/diff/320001/base/test/test_mock_tim... File base/test/test_mock_time_task_runner.h (right): https://codereview.chromium.org/879233003/diff/320001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.h:95: // Whether the elapsing of virtual time is stopped or not. Subclasses should Nit: s/should/can/ https://codereview.chromium.org/879233003/diff/320001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.h:96: // override this method to perform early exit from running task runner. Nit: s/exit from/exits from a/ https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... File components/policy/core/common/cloud/cloud_policy_client.cc (right): https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client.cc:266: base::Unretained(this), request_job.get(), callback); Nit: #include "base/bind_helpers.h" https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client.cc:310: em::DeviceRemoteCommandRequest* request = Nit: const. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client.cc:318: const DeviceManagementRequestJob::Callback job_callback = base::Bind( Why the nested base::Bind()? https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client.cc:554: NotifyClientError(); NotifyClientError() will tell all consumers that something went wrong. If there are e.g. a policy fetch and a remote command fetch in flight at the same time, the classes responsible for these will both get an error callback and will be unable to tell which one of their requests failed. How about you also report the status as part of |RemoteCommandCallback|? https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... File components/policy/core/common/cloud/cloud_policy_client.h (right): https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client.h:53: // A callback to send fetched remote commands back. Nit: It sounds like the callback will send the commands back to the server :). How about: "A callback which received fetched remote commands?" https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client.h:147: // recorded command id and |command_results| being results for previous Nit: s/id/ID/ https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client.h:151: RemoteCommandJob::UniqueIDType last_command_id, Nit: During the first fetch, there is no |last_command_id|. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client.h:285: // Callback for remote commands fetch requests. Nit: s/commands/command/ https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... File components/policy/core/common/cloud/cloud_policy_core.cc (right): https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_core.cc:53: DCHECK(client_); Nit 1: #include "base/logging.h" Nit 2: Is it worth DCHECK()ing that the client is registered? https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_core.cc:59: // Do an initial remote commands fetching immediately. Nit: s/fetching/fetch/ https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... File components/policy/core/common/cloud/cloud_policy_core.h (right): https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_core.h:91: // Starts a remote commands service, with provided factory. Will attempts to Nit 1: s/with/with the/ Nit 2: s/attempts/attempt/ https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_core.h:92: // fetch commands immediately and thus requiring the cloud policy client to be Nit: s/ and/,/ https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... File components/policy/core/common/cloud/mock_cloud_policy_client.h (right): https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/cloud/mock_cloud_policy_client.h:40: void(RemoteCommandJob::UniqueIDType last_command_id, Nit: #include "components/policy/core/common/remote_commands/remote_command_job.h" https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/cloud/mock_cloud_policy_client.h:41: const std::vector<enterprise_management::RemoteCommandResult>& Nit: #include "policy/proto/device_management_backend.pb.h" https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... File components/policy/core/common/remote_commands/remote_commands_factory.h (right): https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_factory.h:17: // A interface class for creating remote commands based on command type. Nit: s/A/An/ https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... File components/policy/core/common/remote_commands/remote_commands_service.cc (right): https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:7: #include <algorithm> Nit: Is this used anywhere? https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:11: #include "base/bind_helpers.h" Nit: Not used. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:44: client_->FetchRemoteCommands( You should reset has_enqueued_fetch_request_ to false before starting the fetch. Otherwise, OnRemoteCommandsFetched() will see has_enqueued_fetch_request_ = true and will start another fetch https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:62: // If the command is already fetched, ignore it instead. Nit: s/ instead// https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:82: // TODO(binjin): Evaluate the security risks and attempts to sync 1: We agreed on a plan, no need to evaluate anymore. 1: Nit: s/attempts/attempt/ https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:84: // reload it later without relying solely on server to keep our last Nit: s/on/on the/ https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:85: // acknowledged command id. Nit: s/id/ID/ https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:96: scoped_ptr<std::string> result_payload = command->GetResultPayload(); Nit: const. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:103: result.set_result(em::RemoteCommandResult_ResultType_RESULT_FAILURE); Christoph had the good point that it may be useful to send up an error message for a failed command. How about including a payload whenever GetResultPayload() returns a non-nullptr value? https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:125: has_enqueued_fetch_request_ = false; No need for this. If |has_enqueued_fetch_request_| was |true|, we would have taken the other branch. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... File components/policy/core/common/remote_commands/remote_commands_service.h (right): https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:30: // back to server. Nit: s/to/to the/ https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:41: // request is in-progress. And in such case, a following up request will be Nit 1: s/such/such a/ Nit 2: a/a following/another/ https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:42: // made immediately after current ongoing requests finishes. Returns true if a Nit 1: s/current/the current/ Nit 2: s/requests/request/ https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:43: // new command fetch request is created. Nit: It was not clear to me what "Returns true if a new command fetch request is created" means without looking at the code. How about "Returns true if the new request was started immediately. Returns false if another request was in progress already and the new request got enqueued?" https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:55: // Helper function to add an command which we get from server. Nit 1: How about s/add/enqueue/? Nit 2: s/an/a/ https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:62: // Callback to handle commands we get from server. Nit: s/from/from the/ https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:69: // Whether there is an enqueued fetch request, which indicates there are Nit: s/are/were/ https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:70: // additional FetchRemoteCommands() calls while a fetch request is ongoing. Nit: s/is/was/ https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:73: // Commands results have not been sent back to server yet. Nit 1: s/Commands/Command/ Nit 2: s/have/that have/ Nit 3: s/server/the server/ https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:76: // ID for the latest command which has started execution. We will acknowledge Nit: s/for/of/ https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:77: // the server with this ID so that we can re-fetch some commands not Nit: s/some commands not/commands that have not been/ https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:79: RemoteCommandJob::UniqueIDType last_executed_command_id_ = 0; 1: We cannot use 0 as a magical value here. Command IDs are entirely opaque and 0 may just as well be a valid ID that the server uses. We need to send *no* ID instead when no commands have been fetched and processed yet. 2: As discussed offline, I want to make sure we know how persistence will work for the reboot command before we land this CL. If we acknowledge a command the moment it starts running, a reboot command will *not* be re-run after a reboot. This is OK, but it means we need to figure out another way to figure out a reboot just happened and there is a result to send back to the server. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:81: // Collects all IDs for fetched commands. We need this since the command id Nit 1: s/all IDs for/the IDs of all/ Nit 2: Document that we do not keep the IDs of all commands we ever fetched in this set. We only store an ID until the command is executed and acknowledged. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:87: CloudPolicyClient* client_; Nit: const. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... File components/policy/core/common/remote_commands/testing_remote_commands_server.cc (right): https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.cc:7: #include <algorithm> Nit: What is this used for? https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.cc:12: #include "base/run_loop.h" Nit: Not used. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.cc:28: virtual ~RemoteCommandWithCallback() {} Nit: Why virtual? https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.cc:31: ResultReportedCallback reported_callback; Nit: #include "base/callback.h" https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.cc:44: // All reported callbacks explicitly stated must be called. Nit: This is hard to understand. How about: "Commands are removed from the queue when a result is reported. Only commands for which no result was expected should remain in the queue?" https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.cc:74: TestingRemoteCommandsServer::RemoteCommands Nit: Is the TestingRemoteCommandsServer:: prefix really necessary here? https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.cc:80: // Verify that acknowledged ID from client is non-decreasing, since we are Nit: s/that/that the/ https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.cc:103: // Verify that command result is for an existing commands actually expecting Nit 1: s/that/that the/ Nit 2: s/commands/command/ https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.cc:104: // command result. 1: Nit: s/command/a/ 2: You are not actually verifying that the command expects a result. Even if the command was found in the queue, if |reported_callback| is null, the command expected no result. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.cc:108: // Post task to original thread for result reporting. Nit 1: s/task to/a task to the/ Nit 2: s/for result reporting/which will report the result/ https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... File components/policy/core/common/remote_commands/testing_remote_commands_server.h (right): https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:28: // This class implements server side logic for remote commands service. It Nit 1: s/server side/server-side/ Nit 2: s/service/service tests/ https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:30: // purpose. Test authors are expected to call IssueCommand() to add a command Nit: s/a command/commands/ https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:31: // to the queue in their tests. The use of FetchCommands() is internal only for Nit: It is not that internal anymore now that we have at least two very different (external) users of this method. How about something like: "FetchCommands() should be called when serving a request to the server intercepted e.g. via a net::URLRequestInterceptor or a mock CloudPolicyClient." https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:32: // classes like subclasses of net::URLRequestInterceptor, or Mocked Nit 1: s/, or Mocked/ or a mocked/ https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:35: // this purpose when issue a command. Nit: s/issue/issuing/ https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:36: // All method (including ctor/dtor) except FetchCommands should be called Nit 1: s/method/methods/ Nit 2: No need to explicitly list the constructor and destructor. All method means all methods. Nit 3: s/FetchCommands/FetchCommands()/ https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:37: // from the same thread (like UI thread), and FetchCommands() can be called from Nit: s/ (like UI thread)// - This class is entirely agnostic to what thread it runs on, as long as it is the same thread throughout its entire lifetime. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:38: // any thread. Nit: Document that the test author is responsible for ensuring that FetchCommands() is not called from another thread after |this| has been destroyed. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:48: // Callback called when the command job result is reported back to server. Nit 1: s/the command job/a command's/ Nit 2: s/to/to the/ https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:54: // command issued time. |reported_callback| will be called from the same Nit: s/issued/issue/ https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:55: // thread when a command result is reported back to server. Nit: s/to/to the/ https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:56: // |skip_next_fetch| should be set default to false, and if it's set to true Nit: I think we can omit the specification of what should be default. How about "If |skip_next_fetch| is true, the command will only actually be added to the queue after the next fetch completes?" https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:57: // instead, it will only be added to the main queue after next fetch Nit 1: s/it/the command/ Nit 2: s/after/after the/ https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:58: // completes. Both |fetched_callback|s and |reported_callback|s are expected Nit: There is no |fetched_callback|. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:59: // to be called if they are not null and will be considered as failure This sentence is unclear to me. Are you saying the test should be considered failed if the callback is never invoked? https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:66: // Fetch commands with |last_command_id| and provide |previous_job_results| Nit: s/with |last_command_id|/, acknowledging all commands up to and including |last_command_id|,/ https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:67: // as results for previous commands, see protobuf definition for protocol Nit 1: s/, see/. See the/ Nit 2: s/protocol definition/a definition of the protocol/ https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:68: // definition between server and client for remote commands fetching. Nit: s/commands/command/ https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:75: // Set alternative Clock for command issued time. The default clock uses Nit 1: s/Clock/clock/ Nit 2: s/for command issued/for obtaining the command issue/ https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:76: // system clock. Nit: s/system/the system/ https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:79: // Get the number of commands which are still expecting command results for Nit: s/which are still expecting command results for verifying/for which no results have been reported yet/ https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:80: // verifying. Note that it's possible that some command is not even fetched. Nit 1: s/is/have/ Nit 2: s/even/even been/ Nit 3: It is not clear to me how these unfetched commands affect the value returned here. If there are 3 fetched and 2 unfetched commands missing results, will this method return 3 or 5? https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:90: // The main commands queue. Nit: s/commands/command/ https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:96: // The latest acknowledged command ID from client, should be non-decreasing. Nit 1: s/from/from the/ https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:100: // strictly monotonically increasing unique IDs. Nit: If you swap the |last_generated_unique_id_| and |last_acknowledged_unique_id_| members, the command explaining that the IDs are strictly monotonically increasing will come earlier, making it clear why the |last_acknowledged_unique_id_| is expected to be non-decreasing. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:103: // Clock used to generate command issued time when IssueCommand() is called. Nit: s/command issued/the command issze/ https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:106: // A lock protecting the command queues and recorded IDs. Nit: s/recorded/generated and acknowledged/ https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:109: // The thread on which this test server is created, usually UI thread. Nit: s/usually/usually the/ https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:110: scoped_refptr<base::SingleThreadTaskRunner> task_runner_; Nit: You said why you need a |SingleThreadTaskRunner| but I still do not understand. Even though ThreadTaskRunnerHandle::Get() returns a |SingleThreadTaskRunner|, that is a subclass of |TaskRunner|, so you are not obliged to store it in a |SingleThreadTaskRunner| member. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:112: // A weak pointer created early so that it can be accessed from other thread. Nit: s/thread/threads/ https://codereview.chromium.org/879233003/diff/320001/components/policy/proto... File components/policy/proto/device_management_backend.proto (right): https://codereview.chromium.org/879233003/diff/320001/components/policy/proto... components/policy/proto/device_management_backend.proto:458: // The command id of the last command received from the server until Nit: s/id/ID/ https://codereview.chromium.org/879233003/diff/320001/components/policy/proto... components/policy/proto/device_management_backend.proto:463: // The client should send back the command result whenever possible. Nit: s/the/a/ https://codereview.chromium.org/879233003/diff/320001/components/policy/proto... components/policy/proto/device_management_backend.proto:958: // * HTTP Authorization header MUST be in the following formats: What format should the header have for remote command requests?
https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... File components/policy/core/common/cloud/cloud_policy_client_unittest.cc (right): https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client_unittest.cc:65: // A mock class to allow us to set expectations on remote commands fetch Nit: s/commands/command/ https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client_unittest.cc:71: MOCK_METHOD1(OnRemoteCommandsFetched, You could just add this method to CloudPolicyClientTest instead of defining a helper class. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client_unittest.cc:260: StrictMock<MockRemoteCommandsObserver> remote_commands_observer_; Nit: If this is used in a single test, there is no need for it to be a member of the test fixture. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client_unittest.cc:783: const std::vector<enterprise_management::RemoteCommandResult> command_results( Nit: s/enterprise_management/em/ https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client_unittest.cc:784: 1, remote_command_request_.remote_command_request().command_results(0)); This is a clever way to initialize the vector. I think everyone else would have created an empty vector first and then push_back()ed an entry. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... File components/policy/core/common/remote_commands/testing_remote_commands_server.h (right): https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:81: size_t NumberOfCommandsPendingResult() const; Nit: #include <stddef.h>
Addressed most of comments except three. 1 in cloud_policy_client.h 1 in remote_commands_service.h 1 in device_management_backend.proto the first two un-addressed comments are regarding empty last_command_id, which I'm not sure how to do it now. It involves a lot of changes. the last one is regarding HTTP Authorization header in command fetch request https://codereview.chromium.org/879233003/diff/320001/base/test/test_mock_tim... File base/test/test_mock_time_task_runner.h (right): https://codereview.chromium.org/879233003/diff/320001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.h:95: // Whether the elapsing of virtual time is stopped or not. Subclasses should On 2015/02/28 00:01:22, bartfab (traveling - busy) wrote: > Nit: s/should/can/ Done. https://codereview.chromium.org/879233003/diff/320001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.h:96: // override this method to perform early exit from running task runner. On 2015/02/28 00:01:22, bartfab (traveling - busy) wrote: > Nit: s/exit from/exits from a/ Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... File components/policy/core/common/cloud/cloud_policy_client.cc (right): https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client.cc:266: base::Unretained(this), request_job.get(), callback); On 2015/02/28 00:01:22, bartfab (traveling - busy) wrote: > Nit: #include "base/bind_helpers.h" Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client.cc:310: em::DeviceRemoteCommandRequest* request = On 2015/02/28 00:01:22, bartfab (traveling - busy) wrote: > Nit: const. I'm not sure since it's pointer mutable protobuf. Used constant pointer instead. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client.cc:318: const DeviceManagementRequestJob::Callback job_callback = base::Bind( On 2015/02/28 00:01:22, bartfab (traveling - busy) wrote: > Why the nested base::Bind()? Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client.cc:554: NotifyClientError(); On 2015/02/28 00:01:22, bartfab (traveling - busy) wrote: > NotifyClientError() will tell all consumers that something went wrong. If there > are e.g. a policy fetch and a remote command fetch in flight at the same time, > the classes responsible for these will both get an error callback and will be > unable to tell which one of their requests failed. How about you also report the > status as part of |RemoteCommandCallback|? Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... File components/policy/core/common/cloud/cloud_policy_client.h (right): https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client.h:53: // A callback to send fetched remote commands back. On 2015/02/28 00:01:22, bartfab (traveling - busy) wrote: > Nit: It sounds like the callback will send the commands back to the server :). > How about: "A callback which received fetched remote commands?" Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client.h:147: // recorded command id and |command_results| being results for previous On 2015/02/28 00:01:22, bartfab (traveling - busy) wrote: > Nit: s/id/ID/ Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client.h:285: // Callback for remote commands fetch requests. On 2015/02/28 00:01:22, bartfab (traveling - busy) wrote: > Nit: s/commands/command/ Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... File components/policy/core/common/cloud/cloud_policy_client_unittest.cc (right): https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client_unittest.cc:65: // A mock class to allow us to set expectations on remote commands fetch On 2015/02/28 00:32:42, bartfab (traveling - busy) wrote: > Nit: s/commands/command/ Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client_unittest.cc:71: MOCK_METHOD1(OnRemoteCommandsFetched, On 2015/02/28 00:32:42, bartfab (traveling - busy) wrote: > You could just add this method to CloudPolicyClientTest instead of defining a > helper class. I actually need to track calling record of this method, and with helper class it's much simpler. Moving this method into CloudPolicyClientTest doesn't help setting expectations. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client_unittest.cc:260: StrictMock<MockRemoteCommandsObserver> remote_commands_observer_; On 2015/02/28 00:32:42, bartfab (traveling - busy) wrote: > Nit: If this is used in a single test, there is no need for it to be a member of > the test fixture. Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client_unittest.cc:783: const std::vector<enterprise_management::RemoteCommandResult> command_results( On 2015/02/28 00:32:42, bartfab (traveling - busy) wrote: > Nit: s/enterprise_management/em/ Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client_unittest.cc:784: 1, remote_command_request_.remote_command_request().command_results(0)); On 2015/02/28 00:32:42, bartfab (traveling - busy) wrote: > This is a clever way to initialize the vector. I think everyone else would have > created an empty vector first and then push_back()ed an entry. Acknowledged. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... File components/policy/core/common/cloud/cloud_policy_core.cc (right): https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_core.cc:53: DCHECK(client_); On 2015/02/28 00:01:22, bartfab (traveling - busy) wrote: > Nit 1: #include "base/logging.h" > Nit 2: Is it worth DCHECK()ing that the client is registered? 1. Done. 2. Not sure, but I didn't see any guarantee regarding this. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_core.cc:59: // Do an initial remote commands fetching immediately. On 2015/02/28 00:01:22, bartfab (traveling - busy) wrote: > Nit: s/fetching/fetch/ Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... File components/policy/core/common/cloud/cloud_policy_core.h (right): https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_core.h:91: // Starts a remote commands service, with provided factory. Will attempts to On 2015/02/28 00:01:22, bartfab (traveling - busy) wrote: > Nit 1: s/with/with the/ > Nit 2: s/attempts/attempt/ Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_core.h:92: // fetch commands immediately and thus requiring the cloud policy client to be On 2015/02/28 00:01:22, bartfab (traveling - busy) wrote: > Nit: s/ and/,/ Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... File components/policy/core/common/cloud/mock_cloud_policy_client.h (right): https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/cloud/mock_cloud_policy_client.h:40: void(RemoteCommandJob::UniqueIDType last_command_id, On 2015/02/28 00:01:23, bartfab (traveling - busy) wrote: > Nit: #include > "components/policy/core/common/remote_commands/remote_command_job.h" Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/cloud/mock_cloud_policy_client.h:41: const std::vector<enterprise_management::RemoteCommandResult>& On 2015/02/28 00:01:22, bartfab (traveling - busy) wrote: > Nit: #include "policy/proto/device_management_backend.pb.h" Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... File components/policy/core/common/remote_commands/remote_commands_factory.h (right): https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_factory.h:17: // A interface class for creating remote commands based on command type. On 2015/02/28 00:01:23, bartfab (traveling - busy) wrote: > Nit: s/A/An/ Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... File components/policy/core/common/remote_commands/remote_commands_service.cc (right): https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:7: #include <algorithm> On 2015/02/28 00:01:23, bartfab (traveling - busy) wrote: > Nit: Is this used anywhere? Removed. Used previously for std::max() https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:11: #include "base/bind_helpers.h" On 2015/02/28 00:01:23, bartfab (traveling - busy) wrote: > Nit: Not used. Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:44: client_->FetchRemoteCommands( On 2015/02/28 00:01:23, bartfab (traveling - busy) wrote: > You should reset has_enqueued_fetch_request_ to false before starting the fetch. > Otherwise, OnRemoteCommandsFetched() will see has_enqueued_fetch_request_ = true > and will start another fetch Start another fetch immediately is actually intended, and it won't introduce infinite loop since "has_enqueued_fetch_request_" will be set to false before the following fetch. Followed your suggestion, since it's easier to understand. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:62: // If the command is already fetched, ignore it instead. On 2015/02/28 00:01:23, bartfab (traveling - busy) wrote: > Nit: s/ instead// Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:82: // TODO(binjin): Evaluate the security risks and attempts to sync On 2015/02/28 00:01:23, bartfab (traveling - busy) wrote: > 1: We agreed on a plan, no need to evaluate anymore. > 1: Nit: s/attempts/attempt/ Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:84: // reload it later without relying solely on server to keep our last On 2015/02/28 00:01:23, bartfab (traveling - busy) wrote: > Nit: s/on/on the/ Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:85: // acknowledged command id. On 2015/02/28 00:01:23, bartfab (traveling - busy) wrote: > Nit: s/id/ID/ Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:96: scoped_ptr<std::string> result_payload = command->GetResultPayload(); On 2015/02/28 00:01:23, bartfab (traveling - busy) wrote: > Nit: const. Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:103: result.set_result(em::RemoteCommandResult_ResultType_RESULT_FAILURE); On 2015/02/28 00:01:23, bartfab (traveling - busy) wrote: > Christoph had the good point that it may be useful to send up an error message > for a failed command. How about including a payload whenever GetResultPayload() > returns a non-nullptr value? It will require changes to interface of RemoteCommandJob, and actual implementation of commands, I would rather do in in a seperated CL. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:125: has_enqueued_fetch_request_ = false; On 2015/02/28 00:01:23, bartfab (traveling - busy) wrote: > No need for this. If |has_enqueued_fetch_request_| was |true|, we would have > taken the other branch. Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... File components/policy/core/common/remote_commands/remote_commands_service.h (right): https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:30: // back to server. On 2015/02/28 00:01:24, bartfab (traveling - busy) wrote: > Nit: s/to/to the/ Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:41: // request is in-progress. And in such case, a following up request will be On 2015/02/28 00:01:24, bartfab (traveling - busy) wrote: > Nit 1: s/such/such a/ > Nit 2: a/a following/another/ Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:42: // made immediately after current ongoing requests finishes. Returns true if a On 2015/02/28 00:01:24, bartfab (traveling - busy) wrote: > Nit 1: s/current/the current/ > Nit 2: s/requests/request/ Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:43: // new command fetch request is created. On 2015/02/28 00:01:23, bartfab (traveling - busy) wrote: > Nit: It was not clear to me what "Returns true if a new command fetch request is > created" means without looking at the code. How about "Returns true if the new > request was started immediately. Returns false if another request was in > progress already and the new request got enqueued?" Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:55: // Helper function to add an command which we get from server. On 2015/02/28 00:01:23, bartfab (traveling - busy) wrote: > Nit 1: How about s/add/enqueue/? > Nit 2: s/an/a/ Done. (Changed function name as well) https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:62: // Callback to handle commands we get from server. On 2015/02/28 00:01:24, bartfab (traveling - busy) wrote: > Nit: s/from/from the/ Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:69: // Whether there is an enqueued fetch request, which indicates there are On 2015/02/28 00:01:23, bartfab (traveling - busy) wrote: > Nit: s/are/were/ Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:70: // additional FetchRemoteCommands() calls while a fetch request is ongoing. On 2015/02/28 00:01:24, bartfab (traveling - busy) wrote: > Nit: s/is/was/ Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:73: // Commands results have not been sent back to server yet. On 2015/02/28 00:01:23, bartfab (traveling - busy) wrote: > Nit 1: s/Commands/Command/ > Nit 2: s/have/that have/ > Nit 3: s/server/the server/ Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:76: // ID for the latest command which has started execution. We will acknowledge On 2015/02/28 00:01:24, bartfab (traveling - busy) wrote: > Nit: s/for/of/ Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:77: // the server with this ID so that we can re-fetch some commands not On 2015/02/28 00:01:24, bartfab (traveling - busy) wrote: > Nit: s/some commands not/commands that have not been/ Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:81: // Collects all IDs for fetched commands. We need this since the command id On 2015/02/28 00:01:23, bartfab (traveling - busy) wrote: > Nit 1: s/all IDs for/the IDs of all/ > Nit 2: Document that we do not keep the IDs of all commands we ever fetched in > this set. We only store an ID until the command is executed and acknowledged. 1. Done. 2. Actually the current implementation will not remove IDs from this set after executed and acknowledged. IDs have low memory footprint and keeping all IDs will also help avoiding the case that server send commands regardless of what we acknowledged before. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:87: CloudPolicyClient* client_; On 2015/02/28 00:01:24, bartfab (traveling - busy) wrote: > Nit: const. Done. Mark the pointer itself constant instead. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... File components/policy/core/common/remote_commands/testing_remote_commands_server.cc (right): https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.cc:7: #include <algorithm> On 2015/02/28 00:01:24, bartfab (traveling - busy) wrote: > Nit: What is this used for? Removed. It's used for std::max() before. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.cc:12: #include "base/run_loop.h" On 2015/02/28 00:01:24, bartfab (traveling - busy) wrote: > Nit: Not used. Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.cc:28: virtual ~RemoteCommandWithCallback() {} On 2015/02/28 00:01:24, bartfab (traveling - busy) wrote: > Nit: Why virtual? Removed. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.cc:31: ResultReportedCallback reported_callback; On 2015/02/28 00:01:24, bartfab (traveling - busy) wrote: > Nit: #include "base/callback.h" Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.cc:44: // All reported callbacks explicitly stated must be called. On 2015/02/28 00:01:25, bartfab (traveling - busy) wrote: > Nit: This is hard to understand. How about: "Commands are removed from the queue > when a result is reported. Only commands for which no result was expected should > remain in the queue?" Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.cc:74: TestingRemoteCommandsServer::RemoteCommands On 2015/02/28 00:01:24, bartfab (traveling - busy) wrote: > Nit: Is the TestingRemoteCommandsServer:: prefix really necessary here? The compiler tells yes :( https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.cc:80: // Verify that acknowledged ID from client is non-decreasing, since we are On 2015/02/28 00:01:24, bartfab (traveling - busy) wrote: > Nit: s/that/that the/ Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.cc:103: // Verify that command result is for an existing commands actually expecting On 2015/02/28 00:01:24, bartfab (traveling - busy) wrote: > Nit 1: s/that/that the/ > Nit 2: s/commands/command/ Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.cc:104: // command result. On 2015/02/28 00:01:24, bartfab (traveling - busy) wrote: > 1: Nit: s/command/a/ > 2: You are not actually verifying that the command expects a result. Even if the > command was found in the queue, if |reported_callback| is null, the command > expected no result. Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.cc:108: // Post task to original thread for result reporting. On 2015/02/28 00:01:24, bartfab (traveling - busy) wrote: > Nit 1: s/task to/a task to the/ > Nit 2: s/for result reporting/which will report the result/ Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... File components/policy/core/common/remote_commands/testing_remote_commands_server.h (right): https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:28: // This class implements server side logic for remote commands service. It On 2015/02/28 00:01:26, bartfab (traveling - busy) wrote: > Nit 1: s/server side/server-side/ > Nit 2: s/service/service tests/ Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:30: // purpose. Test authors are expected to call IssueCommand() to add a command On 2015/02/28 00:01:26, bartfab (traveling - busy) wrote: > Nit: s/a command/commands/ Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:31: // to the queue in their tests. The use of FetchCommands() is internal only for On 2015/02/28 00:01:25, bartfab (traveling - busy) wrote: > Nit: It is not that internal anymore now that we have at least two very > different (external) users of this method. How about something like: > "FetchCommands() should be called when serving a request to the server > intercepted e.g. via a net::URLRequestInterceptor or a mock CloudPolicyClient." Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:32: // classes like subclasses of net::URLRequestInterceptor, or Mocked On 2015/02/28 00:01:25, bartfab (traveling - busy) wrote: > Nit 1: s/, or Mocked/ or a mocked/ Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:35: // this purpose when issue a command. On 2015/02/28 00:01:25, bartfab (traveling - busy) wrote: > Nit: s/issue/issuing/ Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:36: // All method (including ctor/dtor) except FetchCommands should be called On 2015/02/28 00:01:25, bartfab (traveling - busy) wrote: > Nit 1: s/method/methods/ > Nit 2: No need to explicitly list the constructor and destructor. All method > means all methods. > Nit 3: s/FetchCommands/FetchCommands()/ Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:37: // from the same thread (like UI thread), and FetchCommands() can be called from On 2015/02/28 00:01:25, bartfab (traveling - busy) wrote: > Nit: s/ (like UI thread)// - This class is entirely agnostic to what thread it > runs on, as long as it is the same thread throughout its entire lifetime. Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:38: // any thread. On 2015/02/28 00:01:26, bartfab (traveling - busy) wrote: > Nit: Document that the test author is responsible for ensuring that > FetchCommands() is not called from another thread after |this| has been > destroyed. Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:48: // Callback called when the command job result is reported back to server. On 2015/02/28 00:01:26, bartfab (traveling - busy) wrote: > Nit 1: s/the command job/a command's/ > Nit 2: s/to/to the/ Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:54: // command issued time. |reported_callback| will be called from the same On 2015/02/28 00:01:26, bartfab (traveling - busy) wrote: > Nit: s/issued/issue/ Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:55: // thread when a command result is reported back to server. On 2015/02/28 00:01:25, bartfab (traveling - busy) wrote: > Nit: s/to/to the/ Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:56: // |skip_next_fetch| should be set default to false, and if it's set to true On 2015/02/28 00:01:26, bartfab (traveling - busy) wrote: > Nit: I think we can omit the specification of what should be default. How about > "If |skip_next_fetch| is true, the command will only actually be added to the > queue after the next fetch completes?" Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:57: // instead, it will only be added to the main queue after next fetch On 2015/02/28 00:01:25, bartfab (traveling - busy) wrote: > Nit 1: s/it/the command/ > Nit 2: s/after/after the/ Acknowledged. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:58: // completes. Both |fetched_callback|s and |reported_callback|s are expected On 2015/02/28 00:01:25, bartfab (traveling - busy) wrote: > Nit: There is no |fetched_callback|. Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:59: // to be called if they are not null and will be considered as failure On 2015/02/28 00:01:25, bartfab (traveling - busy) wrote: > This sentence is unclear to me. Are you saying the test should be considered > failed if the callback is never invoked? Rewrite this piece of comments. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:66: // Fetch commands with |last_command_id| and provide |previous_job_results| On 2015/02/28 00:01:25, bartfab (traveling - busy) wrote: > Nit: s/with |last_command_id|/, acknowledging all commands up to and including > |last_command_id|,/ Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:67: // as results for previous commands, see protobuf definition for protocol On 2015/02/28 00:01:25, bartfab (traveling - busy) wrote: > Nit 1: s/, see/. See the/ > Nit 2: s/protocol definition/a definition of the protocol/ Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:68: // definition between server and client for remote commands fetching. On 2015/02/28 00:01:26, bartfab (traveling - busy) wrote: > Nit: s/commands/command/ Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:75: // Set alternative Clock for command issued time. The default clock uses On 2015/02/28 00:01:26, bartfab (traveling - busy) wrote: > Nit 1: s/Clock/clock/ > Nit 2: s/for command issued/for obtaining the command issue/ Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:76: // system clock. On 2015/02/28 00:01:25, bartfab (traveling - busy) wrote: > Nit: s/system/the system/ Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:79: // Get the number of commands which are still expecting command results for On 2015/02/28 00:01:26, bartfab (traveling - busy) wrote: > Nit: s/which are still expecting command results for verifying/for which no > results have been reported yet/ Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:80: // verifying. Note that it's possible that some command is not even fetched. On 2015/02/28 00:01:26, bartfab (traveling - busy) wrote: > Nit 1: s/is/have/ > Nit 2: s/even/even been/ > Nit 3: It is not clear to me how these unfetched commands affect the value > returned here. If there are 3 fetched and 2 unfetched commands missing results, > will this method return 3 or 5? 5 in the case. The server don't have records of fetched commands, so it's difficult to distinguish between fetched and unfetched comments. Updated comments accordingly. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:81: size_t NumberOfCommandsPendingResult() const; On 2015/02/28 00:32:42, bartfab (traveling - busy) wrote: > Nit: #include <stddef.h> Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:90: // The main commands queue. On 2015/02/28 00:01:25, bartfab (traveling - busy) wrote: > Nit: s/commands/command/ Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:96: // The latest acknowledged command ID from client, should be non-decreasing. On 2015/02/28 00:01:25, bartfab (traveling - busy) wrote: > Nit 1: s/from/from the/ Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:100: // strictly monotonically increasing unique IDs. On 2015/02/28 00:01:25, bartfab (traveling - busy) wrote: > Nit: If you swap the |last_generated_unique_id_| and > |last_acknowledged_unique_id_| members, the command explaining that the IDs are > strictly monotonically increasing will come earlier, making it clear why the > |last_acknowledged_unique_id_| is expected to be non-decreasing. Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:103: // Clock used to generate command issued time when IssueCommand() is called. On 2015/02/28 00:01:25, bartfab (traveling - busy) wrote: > Nit: s/command issued/the command issze/ Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:106: // A lock protecting the command queues and recorded IDs. On 2015/02/28 00:01:26, bartfab (traveling - busy) wrote: > Nit: s/recorded/generated and acknowledged/ Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:109: // The thread on which this test server is created, usually UI thread. On 2015/02/28 00:01:26, bartfab (traveling - busy) wrote: > Nit: s/usually/usually the/ Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:110: scoped_refptr<base::SingleThreadTaskRunner> task_runner_; On 2015/02/28 00:01:25, bartfab (traveling - busy) wrote: > Nit: You said why you need a |SingleThreadTaskRunner| but I still do not > understand. Even though ThreadTaskRunnerHandle::Get() returns a > |SingleThreadTaskRunner|, that is a subclass of |TaskRunner|, so you are not > obliged to store it in a |SingleThreadTaskRunner| member. Yes. But I want to state that this task runner runs on single thread, and it will make code easier to read. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:112: // A weak pointer created early so that it can be accessed from other thread. On 2015/02/28 00:01:26, bartfab (traveling - busy) wrote: > Nit: s/thread/threads/ Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/proto... File components/policy/proto/device_management_backend.proto (right): https://codereview.chromium.org/879233003/diff/320001/components/policy/proto... components/policy/proto/device_management_backend.proto:458: // The command id of the last command received from the server until On 2015/02/28 00:01:26, bartfab (traveling - busy) wrote: > Nit: s/id/ID/ Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/proto... components/policy/proto/device_management_backend.proto:463: // The client should send back the command result whenever possible. On 2015/02/28 00:01:27, bartfab (traveling - busy) wrote: > Nit: s/the/a/ Done.
hipporharry@gmail.com changed reviewers: + hipporharry@gmail.com
How would you go about to create a file to log all incognito browsing on android or view every trace you made ? Thanks for your time. hipporharry@gmail.com
Addressed the left three comments https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... File components/policy/core/common/cloud/cloud_policy_client.h (right): https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client.h:151: RemoteCommandJob::UniqueIDType last_command_id, On 2015/02/28 00:01:22, bartfab wrote: > Nit: During the first fetch, there is no |last_command_id|. Done. Used negative numbers to indicate invalid command id. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... File components/policy/core/common/remote_commands/remote_commands_service.h (right): https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:79: RemoteCommandJob::UniqueIDType last_executed_command_id_ = 0; On 2015/02/28 00:01:24, bartfab wrote: > 1: We cannot use 0 as a magical value here. Command IDs are entirely opaque and > 0 may just as well be a valid ID that the server uses. We need to send *no* ID > instead when no commands have been fetched and processed yet. > > 2: As discussed offline, I want to make sure we know how persistence will work > for the reboot command before we land this CL. If we acknowledge a command the > moment it starts running, a reboot command will *not* be re-run after a reboot. > This is OK, but it means we need to figure out another way to figure out a > reboot just happened and there is a result to send back to the server. 1. Done. 2. We will introduce commands specified timing for job report, on Started/Finished/Stop And Blocking the queue. https://codereview.chromium.org/879233003/diff/320001/components/policy/proto... File components/policy/proto/device_management_backend.proto (right): https://codereview.chromium.org/879233003/diff/320001/components/policy/proto... components/policy/proto/device_management_backend.proto:958: // * HTTP Authorization header MUST be in the following formats: On 2015/02/28 00:01:26, bartfab wrote: > What format should the header have for remote command requests? Done.
binjin@chromium.org changed reviewers: - hipporharry@gmail.com
https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... File components/policy/core/common/cloud/cloud_policy_client.cc (right): https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client.cc:310: em::DeviceRemoteCommandRequest* request = On 2015/02/28 02:18:04, binjin wrote: > On 2015/02/28 00:01:22, bartfab (traveling - busy) wrote: > > Nit: const. > > I'm not sure since it's pointer mutable protobuf. Used constant pointer instead. Thanks. const pointer is what I meant. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... File components/policy/core/common/cloud/cloud_policy_client_unittest.cc (right): https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client_unittest.cc:71: MOCK_METHOD1(OnRemoteCommandsFetched, On 2015/02/28 02:18:05, binjin wrote: > On 2015/02/28 00:32:42, bartfab (traveling - busy) wrote: > > You could just add this method to CloudPolicyClientTest instead of defining a > > helper class. > > I actually need to track calling record of this method, and with helper class > it's much simpler. Moving this method into CloudPolicyClientTest doesn't help > setting expectations. CloudPolicyClientTest can also have mock methods. You do not need a separate class just to define mock methods. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... File components/policy/core/common/cloud/cloud_policy_core.cc (right): https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_core.cc:53: DCHECK(client_); On 2015/02/28 02:18:05, binjin wrote: > On 2015/02/28 00:01:22, bartfab (traveling - busy) wrote: > > Nit 1: #include "base/logging.h" > > Nit 2: Is it worth DCHECK()ing that the client is registered? > > 1. Done. > 2. Not sure, but I didn't see any guarantee regarding this. remote_commands_service_->FetchRemoteCommands() a few lines down will trigger CloudPolicyClient::FetchRemoteCommands(), which CHECK()s that is_registered() == true. So it is a hard requirement that the client be requirement. We may want to CHECK() it early here, but I do not feel strongly about it. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... File components/policy/core/common/remote_commands/remote_commands_service.cc (right): https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:44: client_->FetchRemoteCommands( On 2015/02/28 02:18:05, binjin wrote: > On 2015/02/28 00:01:23, bartfab (traveling - busy) wrote: > > You should reset has_enqueued_fetch_request_ to false before starting the > fetch. > > Otherwise, OnRemoteCommandsFetched() will see has_enqueued_fetch_request_ = > true > > and will start another fetch > > Start another fetch immediately is actually intended, and it won't introduce > infinite loop since "has_enqueued_fetch_request_" will be set to false before > the following fetch. > > Followed your suggestion, since it's easier to understand. I know there would not have been an infinite loop, but you would have done two fetches in a row instead of just one. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:103: result.set_result(em::RemoteCommandResult_ResultType_RESULT_FAILURE); On 2015/02/28 02:18:05, binjin wrote: > On 2015/02/28 00:01:23, bartfab (traveling - busy) wrote: > > Christoph had the good point that it may be useful to send up an error message > > for a failed command. How about including a payload whenever > GetResultPayload() > > returns a non-nullptr value? > > It will require changes to interface of RemoteCommandJob, and actual > implementation of commands, > I would rather do in in a seperated CL. Ack, this is definitely a task for a separate CL. But it is something to keep in mind for the future as it would be useful. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... File components/policy/core/common/remote_commands/remote_commands_service.h (right): https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:79: RemoteCommandJob::UniqueIDType last_executed_command_id_ = 0; On 2015/03/09 16:31:56, binjin wrote: > On 2015/02/28 00:01:24, bartfab wrote: > > 1: We cannot use 0 as a magical value here. Command IDs are entirely opaque > and > > 0 may just as well be a valid ID that the server uses. We need to send *no* ID > > instead when no commands have been fetched and processed yet. > > > > 2: As discussed offline, I want to make sure we know how persistence will work > > for the reboot command before we land this CL. If we acknowledge a command the > > moment it starts running, a reboot command will *not* be re-run after a > reboot. > > This is OK, but it means we need to figure out another way to figure out a > > reboot just happened and there is a result to send back to the server. > > 1. Done. > 2. We will introduce commands specified timing for job report, on > Started/Finished/Stop And Blocking the queue. How will 2. work across reboots? How will we know after a reboot that a reboot command was in the queue and just finished? https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:81: // Collects all IDs for fetched commands. We need this since the command id On 2015/02/28 02:18:06, binjin wrote: > On 2015/02/28 00:01:23, bartfab (traveling - busy) wrote: > > Nit 1: s/all IDs for/the IDs of all/ > > Nit 2: Document that we do not keep the IDs of all commands we ever fetched in > > this set. We only store an ID until the command is executed and acknowledged. > > 1. Done. > 2. Actually the current implementation will not remove IDs from this set after > executed and acknowledged. IDs have low memory footprint and keeping all IDs > will also help avoiding the case that server send commands regardless of what we > acknowledged before. I think we should fix 2. Once a command has been acknowledged, the server is free to reuse that command ID in the future. We should remove it from our set of fetched commands. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:87: CloudPolicyClient* client_; On 2015/02/28 02:18:05, binjin wrote: > On 2015/02/28 00:01:24, bartfab (traveling - busy) wrote: > > Nit: const. > > Done. Mark the pointer itself constant instead. Thanks, this is what I meant. https://codereview.chromium.org/879233003/diff/360001/components/policy/core/... File components/policy/core/common/cloud/cloud_policy_client.h (right): https://codereview.chromium.org/879233003/diff/360001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client.h:53: // A callback which received fetched remote commands. Nit: Sorry, I mistyped. I meant s/received/receives/ https://codereview.chromium.org/879233003/diff/360001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client.h:150: // when the operation completes. A negative last command ID indicates that Sorry, but a negative command ID cannot be used as a special value either. Command IDs are entirely opaque to us, the server can use *any* value, positive, zero or negative. There cannot be any special reserved value. The canonical way to handle this is to replace your |RemoteCommandJob::UniqueIDType| arguments with |scoped_ptr<RemoteCommandJob::UniqueIDType>| arguments. That way, you can pass in either an ID or a nullptr, indicating that no ID has been recorded yet. https://codereview.chromium.org/879233003/diff/360001/components/policy/core/... File components/policy/core/common/remote_commands/remote_commands_service.cc (right): https://codereview.chromium.org/879233003/diff/360001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:81: // TODO(binjin): Attempt to sync |last_executed_command_id_| to some Nit: Please reference a bug number. https://codereview.chromium.org/879233003/diff/360001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:115: if (status == DM_STATUS_SUCCESS) { Should we retry on error a few times, with exponential backoff? https://codereview.chromium.org/879233003/diff/360001/components/policy/core/... File components/policy/core/common/remote_commands/remote_commands_service.h (right): https://codereview.chromium.org/879233003/diff/360001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:82: // to indicate that no commands was recorded. Nit: s/was recorded/have started execution yet/. https://codereview.chromium.org/879233003/diff/360001/components/policy/core/... File components/policy/core/common/remote_commands/testing_remote_commands_server.cc (right): https://codereview.chromium.org/879233003/diff/360001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.cc:43: // commands for which no result was expected should remain in the queue Nit: Add a full stop at the end. https://codereview.chromium.org/879233003/diff/360001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.cc:105: EXPECT_FALSE(reported_callback.is_null()); Nit: Make this an ASSERT_FALSE(). If the callback happens to be null, you will pass it to the other thread safely where you will then run into a crash as you try to run the callback. Better catch that problem here early with an assert. https://codereview.chromium.org/879233003/diff/360001/components/policy/proto... File components/policy/proto/device_management_backend.proto (right): https://codereview.chromium.org/879233003/diff/360001/components/policy/proto... components/policy/proto/device_management_backend.proto:962: // * For unregister, policy, status, cert_upload and remot commands requests Nit: s/remote/remote/
https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... File components/policy/core/common/cloud/cloud_policy_client.cc (right): https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client.cc:310: em::DeviceRemoteCommandRequest* request = On 2015/03/10 16:09:20, bartfab wrote: > On 2015/02/28 02:18:04, binjin wrote: > > On 2015/02/28 00:01:22, bartfab (traveling - busy) wrote: > > > Nit: const. > > > > I'm not sure since it's pointer mutable protobuf. Used constant pointer > instead. > > Thanks. const pointer is what I meant. Acknowledged. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... File components/policy/core/common/cloud/cloud_policy_client_unittest.cc (right): https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client_unittest.cc:71: MOCK_METHOD1(OnRemoteCommandsFetched, On 2015/03/10 16:09:20, bartfab wrote: > On 2015/02/28 02:18:05, binjin wrote: > > On 2015/02/28 00:32:42, bartfab (traveling - busy) wrote: > > > You could just add this method to CloudPolicyClientTest instead of defining > a > > > helper class. > > > > I actually need to track calling record of this method, and with helper class > > it's much simpler. Moving this method into CloudPolicyClientTest doesn't help > > setting expectations. > > CloudPolicyClientTest can also have mock methods. You do not need a separate > class just to define mock methods. Making this in seperate class will make lifetime management of mock instance easier. And there are also other class with single mock method existing in same test, it's not worth moving all of these into CloudPolicyClientTest in my opinion. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... File components/policy/core/common/cloud/cloud_policy_core.cc (right): https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_core.cc:53: DCHECK(client_); On 2015/03/10 16:09:20, bartfab wrote: > On 2015/02/28 02:18:05, binjin wrote: > > On 2015/02/28 00:01:22, bartfab (traveling - busy) wrote: > > > Nit 1: #include "base/logging.h" > > > Nit 2: Is it worth DCHECK()ing that the client is registered? > > > > 1. Done. > > 2. Not sure, but I didn't see any guarantee regarding this. > > remote_commands_service_->FetchRemoteCommands() a few lines down will trigger > CloudPolicyClient::FetchRemoteCommands(), which CHECK()s that is_registered() == > true. So it is a hard requirement that the client be requirement. We may want to > CHECK() it early here, but I do not feel strongly about it. Acknowledged. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... File components/policy/core/common/remote_commands/remote_commands_service.cc (right): https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:44: client_->FetchRemoteCommands( On 2015/03/10 16:09:20, bartfab wrote: > On 2015/02/28 02:18:05, binjin wrote: > > On 2015/02/28 00:01:23, bartfab (traveling - busy) wrote: > > > You should reset has_enqueued_fetch_request_ to false before starting the > > fetch. > > > Otherwise, OnRemoteCommandsFetched() will see has_enqueued_fetch_request_ = > > true > > > and will start another fetch > > > > Start another fetch immediately is actually intended, and it won't introduce > > infinite loop since "has_enqueued_fetch_request_" will be set to false before > > the following fetch. > > > > Followed your suggestion, since it's easier to understand. > > I know there would not have been an infinite loop, but you would have done two > fetches in a row instead of just one. Acknowledged. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:103: result.set_result(em::RemoteCommandResult_ResultType_RESULT_FAILURE); On 2015/03/10 16:09:20, bartfab wrote: > On 2015/02/28 02:18:05, binjin wrote: > > On 2015/02/28 00:01:23, bartfab (traveling - busy) wrote: > > > Christoph had the good point that it may be useful to send up an error > message > > > for a failed command. How about including a payload whenever > > GetResultPayload() > > > returns a non-nullptr value? > > > > It will require changes to interface of RemoteCommandJob, and actual > > implementation of commands, > > I would rather do in in a seperated CL. > > Ack, this is definitely a task for a separate CL. But it is something to keep in > mind for the future as it would be useful. Acknowledged. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... File components/policy/core/common/remote_commands/remote_commands_service.h (right): https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:79: RemoteCommandJob::UniqueIDType last_executed_command_id_ = 0; On 2015/03/10 16:09:20, bartfab wrote: > On 2015/03/09 16:31:56, binjin wrote: > > On 2015/02/28 00:01:24, bartfab wrote: > > > 1: We cannot use 0 as a magical value here. Command IDs are entirely opaque > > and > > > 0 may just as well be a valid ID that the server uses. We need to send *no* > ID > > > instead when no commands have been fetched and processed yet. > > > > > > 2: As discussed offline, I want to make sure we know how persistence will > work > > > for the reboot command before we land this CL. If we acknowledge a command > the > > > moment it starts running, a reboot command will *not* be re-run after a > > reboot. > > > This is OK, but it means we need to figure out another way to figure out a > > > reboot just happened and there is a result to send back to the server. > > > > 1. Done. > > 2. We will introduce commands specified timing for job report, on > > Started/Finished/Stop And Blocking the queue. > > How will 2. work across reboots? How will we know after a reboot that a reboot > command was in the queue and just finished? We won't acknowledge the commands before rebooting and thus we will re-fetch it after the reboot with help of timestamps. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:81: // Collects all IDs for fetched commands. We need this since the command id On 2015/03/10 16:09:20, bartfab wrote: > On 2015/02/28 02:18:06, binjin wrote: > > On 2015/02/28 00:01:23, bartfab (traveling - busy) wrote: > > > Nit 1: s/all IDs for/the IDs of all/ > > > Nit 2: Document that we do not keep the IDs of all commands we ever fetched > in > > > this set. We only store an ID until the command is executed and > acknowledged. > > > > 1. Done. > > 2. Actually the current implementation will not remove IDs from this set after > > executed and acknowledged. IDs have low memory footprint and keeping all IDs > > will also help avoiding the case that server send commands regardless of what > we > > acknowledged before. > > I think we should fix 2. Once a command has been acknowledged, the server is > free to reuse that command ID in the future. We should remove it from our set of > fetched commands. Done. https://codereview.chromium.org/879233003/diff/320001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:87: CloudPolicyClient* client_; On 2015/03/10 16:09:20, bartfab wrote: > On 2015/02/28 02:18:05, binjin wrote: > > On 2015/02/28 00:01:24, bartfab (traveling - busy) wrote: > > > Nit: const. > > > > Done. Mark the pointer itself constant instead. > > Thanks, this is what I meant. Acknowledged. https://codereview.chromium.org/879233003/diff/360001/components/policy/core/... File components/policy/core/common/cloud/cloud_policy_client.h (right): https://codereview.chromium.org/879233003/diff/360001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client.h:53: // A callback which received fetched remote commands. On 2015/03/10 16:09:21, bartfab wrote: > Nit: Sorry, I mistyped. I meant s/received/receives/ Done. https://codereview.chromium.org/879233003/diff/360001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client.h:150: // when the operation completes. A negative last command ID indicates that On 2015/03/10 16:09:20, bartfab wrote: > Sorry, but a negative command ID cannot be used as a special value either. > Command IDs are entirely opaque to us, the server can use *any* value, positive, > zero or negative. There cannot be any special reserved value. > > The canonical way to handle this is to replace your > |RemoteCommandJob::UniqueIDType| arguments with > |scoped_ptr<RemoteCommandJob::UniqueIDType>| arguments. That way, you can pass > in either an ID or a nullptr, indicating that no ID has been recorded yet. Done. https://codereview.chromium.org/879233003/diff/360001/components/policy/core/... File components/policy/core/common/remote_commands/remote_commands_service.cc (right): https://codereview.chromium.org/879233003/diff/360001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:81: // TODO(binjin): Attempt to sync |last_executed_command_id_| to some On 2015/03/10 16:09:21, bartfab wrote: > Nit: Please reference a bug number. Done. https://codereview.chromium.org/879233003/diff/360001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:115: if (status == DM_STATUS_SUCCESS) { On 2015/03/10 16:09:21, bartfab wrote: > Should we retry on error a few times, with exponential backoff? Will do it in another CL. Added TODO. https://codereview.chromium.org/879233003/diff/360001/components/policy/core/... File components/policy/core/common/remote_commands/remote_commands_service.h (right): https://codereview.chromium.org/879233003/diff/360001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:82: // to indicate that no commands was recorded. On 2015/03/10 16:09:21, bartfab wrote: > Nit: s/was recorded/have started execution yet/. Acknowledged. https://codereview.chromium.org/879233003/diff/360001/components/policy/core/... File components/policy/core/common/remote_commands/testing_remote_commands_server.cc (right): https://codereview.chromium.org/879233003/diff/360001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.cc:43: // commands for which no result was expected should remain in the queue On 2015/03/10 16:09:21, bartfab wrote: > Nit: Add a full stop at the end. Done. https://codereview.chromium.org/879233003/diff/360001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.cc:105: EXPECT_FALSE(reported_callback.is_null()); On 2015/03/10 16:09:21, bartfab wrote: > Nit: Make this an ASSERT_FALSE(). If the callback happens to be null, you will > pass it to the other thread safely where you will then run into a crash as you > try to run the callback. Better catch that problem here early with an assert. Add a if() statement instead. https://codereview.chromium.org/879233003/diff/360001/components/policy/proto... File components/policy/proto/device_management_backend.proto (right): https://codereview.chromium.org/879233003/diff/360001/components/policy/proto... components/policy/proto/device_management_backend.proto:962: // * For unregister, policy, status, cert_upload and remot commands requests On 2015/03/10 16:09:21, bartfab wrote: > Nit: s/remote/remote/ Done.
https://codereview.chromium.org/879233003/diff/360001/components/policy/core/... File components/policy/core/common/cloud/mock_cloud_policy_client.h (right): https://codereview.chromium.org/879233003/diff/360001/components/policy/core/... components/policy/core/common/cloud/mock_cloud_policy_client.h:40: MOCK_METHOD3( Why are you no longer mocking this method? https://codereview.chromium.org/879233003/diff/360001/components/policy/core/... File components/policy/core/common/remote_commands/testing_remote_commands_server.cc (right): https://codereview.chromium.org/879233003/diff/360001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.cc:105: EXPECT_FALSE(reported_callback.is_null()); On 2015/03/12 11:03:16, binjin wrote: > On 2015/03/10 16:09:21, bartfab wrote: > > Nit: Make this an ASSERT_FALSE(). If the callback happens to be null, you will > > pass it to the other thread safely where you will then run into a crash as you > > try to run the callback. Better catch that problem here early with an assert. > > Add a if() statement instead. Why not an ASSERT? A null callback is an error. We should not have an if() statement that handles it. We should just fail the test. https://codereview.chromium.org/879233003/diff/380001/components/policy/core/... File components/policy/core/common/cloud/cloud_policy_client.h (right): https://codereview.chromium.org/879233003/diff/380001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client.h:147: // Attempts to fetch remote commands, with |last_command_id| being the last What does "recorded" mean here? IIUC, you mean commands that finished running. https://codereview.chromium.org/879233003/diff/380001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client.h:150: // when the operation completes. A negative last command ID indicates that Nit: Remove the comment about negative command IDs. https://codereview.chromium.org/879233003/diff/380001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client.h:152: // Note that sending |last_command_id| will also acknowledge this command, Nit 1: s/also // Nit 2: s/, and it can be set to null indicating that there are no recorded command/. A nullptr indicates that no commands have been recorded yet./ https://codereview.chromium.org/879233003/diff/380001/components/policy/core/... File components/policy/core/common/cloud/cloud_policy_client_unittest.cc (right): https://codereview.chromium.org/879233003/diff/380001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client_unittest.cc:790: scoped_ptr<RemoteCommandJob::UniqueIDType>( Nit: You can abbreviate this with make_scoped_ptr(). https://codereview.chromium.org/879233003/diff/380001/components/policy/core/... File components/policy/core/common/cloud/mock_cloud_policy_client.h (right): https://codereview.chromium.org/879233003/diff/380001/components/policy/core/... components/policy/core/common/cloud/mock_cloud_policy_client.h:11: #include "base/memory/scoped_ptr.h" Nit: Not used. https://codereview.chromium.org/879233003/diff/380001/components/policy/core/... File components/policy/core/common/remote_commands/remote_commands_service.cc (right): https://codereview.chromium.org/879233003/diff/380001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:44: scoped_ptr<RemoteCommandJob::UniqueIDType> id_to_acknowlede = nullptr; Nit 1: s/acknowlede/acknowledge/ Nit 2: The constructor automatically initializes this to a nullptr. https://codereview.chromium.org/879233003/diff/380001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:48: // commands before it from |fetched_command_ids_|. Nit: s/commands/command/ https://codereview.chromium.org/879233003/diff/380001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:54: } What if there is a fetch in progress right now? Since that fetch started in the past, it will contain commands that we just acknowledged. But you already removed these from |fetched_command_ids_|, so we will not realize this and add them back to the queue. https://codereview.chromium.org/879233003/diff/380001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:77: if (std::find(fetched_command_ids_.begin(), fetched_command_ids_.end(), You do not actually need to keep a queue and search through it. Since the server will *always* send commands in order, you can simply keep track of the single last-fetched command ID. When you get a new list of commands from the server, look through it linearly once: - If you find your last-fetched ID in the list, add all commands that follow to your queue. - If you do not find your last-fetched ID in the list, add all commands to your queue. Then, update the last-fetched ID. https://codereview.chromium.org/879233003/diff/380001/components/policy/core/... File components/policy/core/common/remote_commands/remote_commands_service.h (right): https://codereview.chromium.org/879233003/diff/380001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:79: // Whether an executed command exists or not. Nit: "Executed" would mean "finished executing." If this is what you mean, great. If you mean "started executing," please reword. https://codereview.chromium.org/879233003/diff/380001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:82: // ID of the latest command which has started execution if You are saying here that you acknowledge commands when they *start* execution. But you said that you expect to receive the reboot command again after a reboot, which means you must only acknowledge it when it *finishes* execution. https://codereview.chromium.org/879233003/diff/380001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:83: // |has_last_executed_command_id_| is true. We will acknowledge the server Nit: s/the server with this ID/this ID to the server/ https://codereview.chromium.org/879233003/diff/380001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:88: // Collects the IDs of all fetched commands. We need this since the command id Nit: s/id/ID/ https://codereview.chromium.org/879233003/diff/380001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:91: // and acknowledging a command will discard the ID of it from Nit: s/the ID of it/its ID/ https://codereview.chromium.org/879233003/diff/380001/components/policy/core/... File components/policy/core/common/remote_commands/testing_remote_commands_server.h (right): https://codereview.chromium.org/879233003/diff/380001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:70: // previous commands. |last_command_id| can be not which indicates that no Nit: s/not/a nullptr,/ https://codereview.chromium.org/879233003/diff/380001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:71: // commands is recorded. Nit 1: s/is recorded/have been recorded yet/ Nit 2: What does "recorded" mean here?
https://codereview.chromium.org/879233003/diff/360001/components/policy/core/... File components/policy/core/common/cloud/mock_cloud_policy_client.h (right): https://codereview.chromium.org/879233003/diff/360001/components/policy/core/... components/policy/core/common/cloud/mock_cloud_policy_client.h:40: MOCK_METHOD3( On 2015/03/12 14:00:54, bartfab wrote: > Why are you no longer mocking this method? It's not used in any other code, and scoped_ptr as an argument break the compiles. https://codereview.chromium.org/879233003/diff/360001/components/policy/core/... File components/policy/core/common/remote_commands/testing_remote_commands_server.cc (right): https://codereview.chromium.org/879233003/diff/360001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.cc:105: EXPECT_FALSE(reported_callback.is_null()); On 2015/03/12 14:00:54, bartfab wrote: > On 2015/03/12 11:03:16, binjin wrote: > > On 2015/03/10 16:09:21, bartfab wrote: > > > Nit: Make this an ASSERT_FALSE(). If the callback happens to be null, you > will > > > pass it to the other thread safely where you will then run into a crash as > you > > > try to run the callback. Better catch that problem here early with an > assert. > > > > Add a if() statement instead. > > Why not an ASSERT? A null callback is an error. We should not have an if() > statement that handles it. We should just fail the test. Forgot to mention, it's not possible to use ASSERT in non-void returning functions.
https://codereview.chromium.org/879233003/diff/380001/components/policy/core/... File components/policy/core/common/cloud/cloud_policy_client.h (right): https://codereview.chromium.org/879233003/diff/380001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client.h:147: // Attempts to fetch remote commands, with |last_command_id| being the last On 2015/03/12 14:00:55, bartfab wrote: > What does "recorded" mean here? IIUC, you mean commands that finished running. Done. https://codereview.chromium.org/879233003/diff/380001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client.h:150: // when the operation completes. A negative last command ID indicates that On 2015/03/12 14:00:55, bartfab wrote: > Nit: Remove the comment about negative command IDs. Done. https://codereview.chromium.org/879233003/diff/380001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client.h:152: // Note that sending |last_command_id| will also acknowledge this command, On 2015/03/12 14:00:55, bartfab wrote: > Nit 1: s/also // > Nit 2: s/, and it can be set to null indicating that there are no recorded > command/. A nullptr indicates that no commands have been recorded yet./ Done. https://codereview.chromium.org/879233003/diff/380001/components/policy/core/... File components/policy/core/common/cloud/cloud_policy_client_unittest.cc (right): https://codereview.chromium.org/879233003/diff/380001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client_unittest.cc:790: scoped_ptr<RemoteCommandJob::UniqueIDType>( On 2015/03/12 14:00:55, bartfab wrote: > Nit: You can abbreviate this with make_scoped_ptr(). Done. https://codereview.chromium.org/879233003/diff/380001/components/policy/core/... File components/policy/core/common/cloud/mock_cloud_policy_client.h (right): https://codereview.chromium.org/879233003/diff/380001/components/policy/core/... components/policy/core/common/cloud/mock_cloud_policy_client.h:11: #include "base/memory/scoped_ptr.h" On 2015/03/12 14:00:55, bartfab wrote: > Nit: Not used. Done. https://codereview.chromium.org/879233003/diff/380001/components/policy/core/... File components/policy/core/common/remote_commands/remote_commands_service.cc (right): https://codereview.chromium.org/879233003/diff/380001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:44: scoped_ptr<RemoteCommandJob::UniqueIDType> id_to_acknowlede = nullptr; On 2015/03/12 14:00:55, bartfab wrote: > Nit 1: s/acknowlede/acknowledge/ > Nit 2: The constructor automatically initializes this to a nullptr. Done. https://codereview.chromium.org/879233003/diff/380001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:48: // commands before it from |fetched_command_ids_|. On 2015/03/12 14:00:55, bartfab wrote: > Nit: s/commands/command/ Done. https://codereview.chromium.org/879233003/diff/380001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:54: } On 2015/03/12 14:00:55, bartfab wrote: > What if there is a fetch in progress right now? Since that fetch started in the > past, it will contain commands that we just acknowledged. But you already > removed these from |fetched_command_ids_|, so we will not realize this and add > them back to the queue. This piece of code is used to prepare another fetch request and there should be no other fetch ongoing. https://codereview.chromium.org/879233003/diff/380001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:77: if (std::find(fetched_command_ids_.begin(), fetched_command_ids_.end(), On 2015/03/12 14:00:55, bartfab wrote: > You do not actually need to keep a queue and search through it. Since the server > will *always* send commands in order, you can simply keep track of the single > last-fetched command ID. When you get a new list of commands from the server, > look through it linearly once: > - If you find your last-fetched ID in the list, add all commands that follow to > your queue. > - If you do not find your last-fetched ID in the list, add all commands to your > queue. > Then, update the last-fetched ID. As discussed offline, using a queue here will protect us in the case that server is behaving badly. https://codereview.chromium.org/879233003/diff/380001/components/policy/core/... File components/policy/core/common/remote_commands/remote_commands_service.h (right): https://codereview.chromium.org/879233003/diff/380001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:79: // Whether an executed command exists or not. On 2015/03/12 14:00:55, bartfab wrote: > Nit: "Executed" would mean "finished executing." If this is what you mean, > great. If you mean "started executing," please reword. Acknowledged. https://codereview.chromium.org/879233003/diff/380001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:82: // ID of the latest command which has started execution if On 2015/03/12 14:00:55, bartfab wrote: > You are saying here that you acknowledge commands when they *start* execution. > But you said that you expect to receive the reboot command again after a reboot, > which means you must only acknowledge it when it *finishes* execution. Behavior changed to acknowledge finished commands only. Will add necessary support for reboot command in the following CL. https://codereview.chromium.org/879233003/diff/380001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:83: // |has_last_executed_command_id_| is true. We will acknowledge the server On 2015/03/12 14:00:55, bartfab wrote: > Nit: s/the server with this ID/this ID to the server/ Done. https://codereview.chromium.org/879233003/diff/380001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:88: // Collects the IDs of all fetched commands. We need this since the command id On 2015/03/12 14:00:55, bartfab wrote: > Nit: s/id/ID/ Done. https://codereview.chromium.org/879233003/diff/380001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:91: // and acknowledging a command will discard the ID of it from On 2015/03/12 14:00:55, bartfab wrote: > Nit: s/the ID of it/its ID/ Done. https://codereview.chromium.org/879233003/diff/380001/components/policy/core/... File components/policy/core/common/remote_commands/testing_remote_commands_server.h (right): https://codereview.chromium.org/879233003/diff/380001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:70: // previous commands. |last_command_id| can be not which indicates that no On 2015/03/12 14:00:56, bartfab wrote: > Nit: s/not/a nullptr,/ Done. https://codereview.chromium.org/879233003/diff/380001/components/policy/core/... components/policy/core/common/remote_commands/testing_remote_commands_server.h:71: // commands is recorded. On 2015/03/12 14:00:55, bartfab wrote: > Nit 1: s/is recorded/have been recorded yet/ > Nit 2: What does "recorded" mean here? 1. Done. 2. use "acknowledge" term here.
Bartosz, could you have a look?
LGTM with nits. https://codereview.chromium.org/879233003/diff/400001/components/policy/core/... File components/policy/core/common/cloud/cloud_policy_client.h (right): https://codereview.chromium.org/879233003/diff/400001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client.h:147: // Attempts to fetch remote commands, with |last_command_id| being the last Nit: s/last ID of the command/ID of the last command that/ https://codereview.chromium.org/879233003/diff/400001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client.h:148: // ID of the command started execution and |command_results| being results for Nit: s/started/finished/ https://codereview.chromium.org/879233003/diff/400001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client.h:151: // Note that sending |last_command_id| will acknowledge this command. A Nit: "and any previous commands" https://codereview.chromium.org/879233003/diff/400001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client.h:152: // nullptr indicates that no commands have been recorded yet. Nit: As before, what does "recorded" mean? https://codereview.chromium.org/879233003/diff/400001/components/policy/core/... File components/policy/core/common/remote_commands/remote_commands_service.cc (right): https://codereview.chromium.org/879233003/diff/400001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:44: scoped_ptr<RemoteCommandJob::UniqueIDType> id_to_acknowledge = nullptr; Nit: s/ = nullptr// (this happens automatically for scoped_ptrs) https://codereview.chromium.org/879233003/diff/400001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:52: // there is at most one ongoing command fetch request. Maybe it would be even clearer if you wrote: "since it is guaranteed that there is no earlier fetch request in progress anymore that could have returned these IDs." https://codereview.chromium.org/879233003/diff/400001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:100: void RemoteCommandsService::OnJobStarted(RemoteCommandJob* command) { Nit: Is anyone using OnJobStarted() now? If not, you could remove it from RemoteCommandsQueue::Observer. https://codereview.chromium.org/879233003/diff/400001/components/policy/core/... File components/policy/core/common/remote_commands/remote_commands_service.h (right): https://codereview.chromium.org/879233003/diff/400001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:79: // Whether an command exists or not. You changed this comment from "Whether an executed command exists or not," which was ambiguous, to "Whether an command exists or not," which is even more ambiguous. Based on the member name, it should probably be: // Whether at least one command has finished executing. https://codereview.chromium.org/879233003/diff/400001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:92: // |fetched_command_ids_|, as well as IDs of every command before it. Nit: s/IDs/the IDs/
binjin@chromium.org changed reviewers: + jam@chromium.org, mark@chromium.org
mark, jam: Could either of you take a quick look at the changes I made to base/. Thanks
LGTM in base
https://codereview.chromium.org/879233003/diff/400001/components/policy/core/... File components/policy/core/common/cloud/cloud_policy_client.h (right): https://codereview.chromium.org/879233003/diff/400001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client.h:147: // Attempts to fetch remote commands, with |last_command_id| being the last On 2015/03/17 14:01:21, bartfab wrote: > Nit: s/last ID of the command/ID of the last command that/ Done. https://codereview.chromium.org/879233003/diff/400001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client.h:148: // ID of the command started execution and |command_results| being results for On 2015/03/17 14:01:21, bartfab wrote: > Nit: s/started/finished/ Done. https://codereview.chromium.org/879233003/diff/400001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client.h:151: // Note that sending |last_command_id| will acknowledge this command. A On 2015/03/17 14:01:22, bartfab wrote: > Nit: "and any previous commands" Done. https://codereview.chromium.org/879233003/diff/400001/components/policy/core/... components/policy/core/common/cloud/cloud_policy_client.h:152: // nullptr indicates that no commands have been recorded yet. On 2015/03/17 14:01:21, bartfab wrote: > Nit: As before, what does "recorded" mean? Done. https://codereview.chromium.org/879233003/diff/400001/components/policy/core/... File components/policy/core/common/remote_commands/remote_commands_service.cc (right): https://codereview.chromium.org/879233003/diff/400001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:44: scoped_ptr<RemoteCommandJob::UniqueIDType> id_to_acknowledge = nullptr; On 2015/03/17 14:01:22, bartfab wrote: > Nit: s/ = nullptr// (this happens automatically for scoped_ptrs) Done. https://codereview.chromium.org/879233003/diff/400001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:52: // there is at most one ongoing command fetch request. On 2015/03/17 14:01:22, bartfab wrote: > Maybe it would be even clearer if you wrote: > > "since it is guaranteed that there is no earlier fetch request in progress > anymore that could have returned these IDs." Done. https://codereview.chromium.org/879233003/diff/400001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:100: void RemoteCommandsService::OnJobStarted(RemoteCommandJob* command) { On 2015/03/17 14:01:22, bartfab wrote: > Nit: Is anyone using OnJobStarted() now? If not, you could remove it from > RemoteCommandsQueue::Observer. Acknowledged. I'd like to keep this since persistent storing might use this. https://codereview.chromium.org/879233003/diff/400001/components/policy/core/... File components/policy/core/common/remote_commands/remote_commands_service.h (right): https://codereview.chromium.org/879233003/diff/400001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:79: // Whether an command exists or not. On 2015/03/17 14:01:22, bartfab wrote: > You changed this comment from "Whether an executed command exists or not," which > was ambiguous, to "Whether an command exists or not," which is even more > ambiguous. Based on the member name, it should probably be: > > // Whether at least one command has finished executing. Done. https://codereview.chromium.org/879233003/diff/400001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.h:92: // |fetched_command_ids_|, as well as IDs of every command before it. On 2015/03/17 14:01:22, bartfab wrote: > Nit: s/IDs/the IDs/ Done.
The CQ bit was checked by binjin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bartfab@chromium.org, mark@chromium.org Link to the patchset: https://codereview.chromium.org/879233003/#ps420001 (title: "rebase; fixes addressing $31")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/879233003/420001
The CQ bit was unchecked by binjin@chromium.org
binjin@chromium.org changed reviewers: - jam@chromium.org
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/879233003/420001
Still LGTM. How come this has "BUG=None" set? Is there no bug for this work? https://codereview.chromium.org/879233003/diff/400001/components/policy/core/... File components/policy/core/common/remote_commands/remote_commands_service.cc (right): https://codereview.chromium.org/879233003/diff/400001/components/policy/core/... components/policy/core/common/remote_commands/remote_commands_service.cc:100: void RemoteCommandsService::OnJobStarted(RemoteCommandJob* command) { On 2015/03/18 10:09:38, binjin wrote: > On 2015/03/17 14:01:22, bartfab wrote: > > Nit: Is anyone using OnJobStarted() now? If not, you could remove it from > > RemoteCommandsQueue::Observer. > > Acknowledged. I'd like to keep this since persistent storing might use this. I will not stop your CL that is almost landed. But please clean this up in a follow-up CL: We should not be adding observer methods that might be used in the future. If there is no current use, remove the method. If you need it in the future, add it back.
Message was sent while issue was closed.
Committed patchset #21 (id:420001)
Message was sent while issue was closed.
Patchset 21 (id:??) landed as https://crrev.com/efa8017cfd57b38d0126e994a5e29aba7fbf98e8 Cr-Commit-Position: refs/heads/master@{#321103} |
