Manual fixups in components/policy for scoped_refptr operator T* removal

Manual fixups in components/policy for scoped_refptr operator T* removal

BUG=110610
Committed: https://crrev.com/baf35970d40c43e2c6913f292207f49249e456fa
Cr-Commit-Position: refs/heads/master@{#292663}

[dcheng, 2014-08-28 09:51:39]

dcheng@chromium.org changed reviewers:
+ bartfab@chromium.org

[dcheng, 2014-08-28 09:51:40]

Mind taking a look? These are manual fixups for cases that the automated tool
opted to skip.

I'm not really sure what the intended behavior for CreateJob() is. I think you
mentioned previously that perhaps this should take a scoped_refptr as well.
Would you like me to make that change instead?

[bartfab (slow), 2014-08-29 08:49:03]

https://codereview.chromium.org/514143002/diff/1/components/policy/core/commo...
File components/policy/core/common/cloud/cloud_policy_client.cc (right):

https://codereview.chromium.org/514143002/diff/1/components/policy/core/commo...
components/policy/core/common/cloud/cloud_policy_client.cc:112: //
DeviceManagementRequestJobImpl, which only keeps a raw pointer to it.
The job is owned by |this| and |this| will go away before the system request
context getter, so a raw pointer should work in practice. But I see no harm in
keeping a scoped_refptr in DeviceManagementRequestJobImpl instead. It makes it
much more explicit that the job expects the context getter not to go away during
its lifetime.

[dcheng, 2014-08-29 09:08:36]

PTAL. Unfortunately, I can't plumb it all the way through yet--URLFetcher should
probably take a const scoped_refptr<T>& for setting the request context getter,
but that's something I'd like to address outside of this patch, if that's OK
with you.

https://codereview.chromium.org/514143002/diff/1/components/policy/core/commo...
File components/policy/core/common/cloud/cloud_policy_client.cc (right):

https://codereview.chromium.org/514143002/diff/1/components/policy/core/commo...
components/policy/core/common/cloud/cloud_policy_client.cc:112: //
DeviceManagementRequestJobImpl, which only keeps a raw pointer to it.
On 2014/08/29 08:49:03, bartfab wrote:
> The job is owned by |this| and |this| will go away before the system request
> context getter, so a raw pointer should work in practice. But I see no harm in
> keeping a scoped_refptr in DeviceManagementRequestJobImpl instead. It makes it
> much more explicit that the job expects the context getter not to go away
during
> its lifetime.

Done.

[bartfab (slow), 2014-08-29 09:42:39]

lgtm

https://codereview.chromium.org/514143002/diff/20001/components/policy/core/c...
File components/policy/core/common/cloud/cloud_policy_client.cc (right):

https://codereview.chromium.org/514143002/diff/20001/components/policy/core/c...
components/policy/core/common/cloud/cloud_policy_client.cc:231:
service_->CreateJob(DeviceManagementRequestJob::TYPE_API_AUTH_CODE_FETCH,
Nit: Why did this get reformatted? I see no code change.

https://codereview.chromium.org/514143002/diff/20001/components/policy/core/c...
File components/policy/core/common/cloud/cloud_policy_client_unittest.cc
(right):

https://codereview.chromium.org/514143002/diff/20001/components/policy/core/c...
components/policy/core/common/cloud/cloud_policy_client_unittest.cc:208:
scoped_refptr<net::URLRequestContextGetter> request_context_;
Nit: #include "base/memory/ref_counted.h"

https://codereview.chromium.org/514143002/diff/20001/components/policy/core/c...
File components/policy/core/common/cloud/device_management_service.h (right):

https://codereview.chromium.org/514143002/diff/20001/components/policy/core/c...
components/policy/core/common/cloud/device_management_service.h:135: const
scoped_refptr<net::URLRequestContextGetter>& request_context);
Nit: #include "base/memory/ref_counted.h"

https://codereview.chromium.org/514143002/diff/20001/components/policy/core/c...
File components/policy/core/common/cloud/mock_device_management_service.h
(right):

https://codereview.chromium.org/514143002/diff/20001/components/policy/core/c...
components/policy/core/common/cloud/mock_device_management_service.h:51: const
scoped_refptr<net::URLRequestContextGetter>&);
Nit: #include "base/memory/ref_counted.h"

[dcheng, 2014-08-29 09:56:38]

https://codereview.chromium.org/514143002/diff/20001/components/policy/core/c...
File components/policy/core/common/cloud/cloud_policy_client.cc (right):

https://codereview.chromium.org/514143002/diff/20001/components/policy/core/c...
components/policy/core/common/cloud/cloud_policy_client.cc:231:
service_->CreateJob(DeviceManagementRequestJob::TYPE_API_AUTH_CODE_FETCH,
On 2014/08/29 09:42:38, bartfab wrote:
> Nit: Why did this get reformatted? I see no code change.

I've reverted this change. It's because the line was changed originally and got
reformatted then.

https://codereview.chromium.org/514143002/diff/20001/components/policy/core/c...
File components/policy/core/common/cloud/cloud_policy_client_unittest.cc
(right):

https://codereview.chromium.org/514143002/diff/20001/components/policy/core/c...
components/policy/core/common/cloud/cloud_policy_client_unittest.cc:208:
scoped_refptr<net::URLRequestContextGetter> request_context_;
On 2014/08/29 09:42:38, bartfab wrote:
> Nit: #include "base/memory/ref_counted.h"

Done.

https://codereview.chromium.org/514143002/diff/20001/components/policy/core/c...
File components/policy/core/common/cloud/device_management_service.h (right):

https://codereview.chromium.org/514143002/diff/20001/components/policy/core/c...
components/policy/core/common/cloud/device_management_service.h:135: const
scoped_refptr<net::URLRequestContextGetter>& request_context);
On 2014/08/29 09:42:38, bartfab wrote:
> Nit: #include "base/memory/ref_counted.h"

Done.

https://codereview.chromium.org/514143002/diff/20001/components/policy/core/c...
File components/policy/core/common/cloud/mock_device_management_service.h
(right):

https://codereview.chromium.org/514143002/diff/20001/components/policy/core/c...
components/policy/core/common/cloud/mock_device_management_service.h:51: const
scoped_refptr<net::URLRequestContextGetter>&);
On 2014/08/29 09:42:39, bartfab wrote:
> Nit: #include "base/memory/ref_counted.h"

Generally I don't #include headers that are already included in the interface
header I'm implementing (DeviceManagementService). Please let me know if you
feel differently about this and I'll update accordingly.

[bartfab (slow), 2014-08-29 11:18:45]

https://codereview.chromium.org/514143002/diff/20001/components/policy/core/c...
File components/policy/core/common/cloud/mock_device_management_service.h
(right):

https://codereview.chromium.org/514143002/diff/20001/components/policy/core/c...
components/policy/core/common/cloud/mock_device_management_service.h:51: const
scoped_refptr<net::URLRequestContextGetter>&);
On 2014/08/29 09:56:37, dcheng (OOO) wrote:
> On 2014/08/29 09:42:39, bartfab wrote:
> > Nit: #include "base/memory/ref_counted.h"
> 
> Generally I don't #include headers that are already included in the interface
> header I'm implementing (DeviceManagementService). Please let me know if you
> feel differently about this and I'll update accordingly.

In policy code, we do not include files in a .cc file if the corresponding .h
file has them already. I am not aware of any similar rule when implementing
interfaces, but it seems reasonable, so SGTM.

[dcheng, 2014-08-29 16:02:48]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-08-29 16:03:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dcheng@chromium.org/514143002/40001

[commit-bot: I haz the power, 2014-08-29 17:20:36]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[commit-bot: I haz the power, 2014-08-29 17:32:18]

Committed patchset #3 (id:40001) as 0c8a50fa04d88fab3525674561e1dd5cdc338030

[commit-bot: I haz the power, 2014-09-10 03:09:50]

Patchset 3 (id:??) landed as
https://crrev.com/baf35970d40c43e2c6913f292207f49249e456fa
Cr-Commit-Position: refs/heads/master@{#292663}