Add a header when fetching pages under the DMServer URL.

Add a header when fetching pages under the DMServer URL.

PolicyHeaderService now generates a json-style header to send up with web requests sent to DMServer containing information about the current policy state. This allows DMServer to return policy information via SAML assertions to authorized service providers.

BUG=326799
TBR=jochen

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=240019

[Andrew T Wilson (Slow), 2013-12-09 10:24:59]

Joao,Daniel PTAL. I can move PolicyHeaderService to components/ after the latest
bulk-file-move, but I'll do that in a separate CL.

[dconnelly, 2013-12-09 12:47:34]

lgtm

https://codereview.chromium.org/99433004/diff/20001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/policy_header_service.h (right):

https://codereview.chromium.org/99433004/diff/20001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/policy_header_service.h:36: // Creates a
PolicyHeaderIOHelper object to be run on the IO thread and
Here and elsewhere I would prefer to eliminate the "on the I/O" and "on the UI"
thread stuff. The methods of PolicyHeaderIOHelper could be renamed to
"UpdateHeader' and "UpdateHeaderOnTaskRunner" instead, or similar.

https://codereview.chromium.org/99433004/diff/20001/components/policy/core/br...
File components/policy/core/browser/DEPS (left):

https://codereview.chromium.org/99433004/diff/20001/components/policy/core/br...
components/policy/core/browser/DEPS:3: "+ui/base/l10n",
why are these removed?

https://codereview.chromium.org/99433004/diff/20001/components/policy/core/br...
File components/policy/core/browser/DEPS (right):

https://codereview.chromium.org/99433004/diff/20001/components/policy/core/br...
components/policy/core/browser/DEPS:2: "+content/public/test",
I'm not convinced this is necessary, but if it is, the tests that require it
should move to ../content.

https://codereview.chromium.org/99433004/diff/20001/components/policy/core/br...
File components/policy/core/browser/policy_header_io_helper_unittest.cc (right):

https://codereview.chromium.org/99433004/diff/20001/components/policy/core/br...
components/policy/core/browser/policy_header_io_helper_unittest.cc:4: #include
"base/test/test_simple_task_runner.h"
newline above

https://codereview.chromium.org/99433004/diff/20001/components/policy/core/br...
components/policy/core/browser/policy_header_io_helper_unittest.cc:50:
task_runner_->RunUntilIdle();
Can this move to the end of SetUp instead of reproducing at the beginning of
every test?

[Joao da Silva, 2013-12-09 14:46:21]

https://codereview.chromium.org/99433004/diff/40001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/policy_header_service.h (right):

https://codereview.chromium.org/99433004/diff/40001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/policy_header_service.h:63: };
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/99433004/diff/40001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/policy_header_service_factory.cc (right):

https://codereview.chromium.org/99433004/diff/40001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/policy_header_service_factory.cc:22: class
PolicyHeaderServiceWrapper : public BrowserContextKeyedService {
Put this in an anonymous namespace.

A wrapper like this works; another possibility is to inhering from both BCKS and
PolicyHeaderService. You'd have to forwards the Shutdown() call to the
PolicyHeaderService though.

https://codereview.chromium.org/99433004/diff/40001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/policy_header_service_factory.cc:38:
scoped_ptr<PolicyHeaderService> policy_header_service_;
#include scoped_ptr.h

https://codereview.chromium.org/99433004/diff/40001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/policy_header_service_factory.cc:45:
DependsOn(UserCloudPolicyManagerFactory::GetInstance());
On ChromeOS this should be UserCloudPolicyManagerFactoryChromeOS

https://codereview.chromium.org/99433004/diff/40001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/policy_header_service_factory.cc:56:
GetInstance()->GetServiceForBrowserContext(context, true));
|wrapper| will be NULL for incognito profiles. I think a NULL check is a good
idea though this isn't called for those profiles. WDYT?

https://codereview.chromium.org/99433004/diff/40001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/policy_header_service_factory.cc:68:
UserCloudPolicyManagerFactory::GetForBrowserContext(context);
This needs an OS_CHROMEOS version

https://codereview.chromium.org/99433004/diff/40001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/policy_header_service_factory.cc:78: return new
PolicyHeaderServiceWrapper(service.Pass());
indentation

https://codereview.chromium.org/99433004/diff/40001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/policy_header_service_factory.h (right):

https://codereview.chromium.org/99433004/diff/40001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/policy_header_service_factory.h:11: class Profile;
not used

https://codereview.chromium.org/99433004/diff/40001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/99433004/diff/40001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_io_data.cc:51: #include
"chrome/browser/policy/cloud/user_cloud_policy_manager_factory.h"
#if defined(ENABLE_CONFIGURATION_POLICY)

https://codereview.chromium.org/99433004/diff/40001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_io_data.cc:62: #include
"components/policy/core/browser/policy_header_io_helper.h"
#if defined(...)

https://codereview.chromium.org/99433004/diff/40001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/99433004/diff/40001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_io_data.h:500: mutable
scoped_ptr<policy::PolicyHeaderIOHelper> policy_header_helper_;
I think this needs an #ifdef, otherwise non-policy builds won't know how to
destroy this object.

Try sending this CL to linux_redux, it's actually building lately. It will tell
whether this breaks the iOS build.

https://codereview.chromium.org/99433004/diff/40001/chrome/browser/renderer_h...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/99433004/diff/40001/chrome/browser/renderer_h...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:41:
#include "components/policy/core/browser/policy_header_io_helper.h"
#ifdef

https://codereview.chromium.org/99433004/diff/40001/components/policy/core/br...
File components/policy/core/browser/DEPS (right):

https://codereview.chromium.org/99433004/diff/40001/components/policy/core/br...
components/policy/core/browser/DEPS:5: ]
This file shouldn't need to change:

- ../DEPS already adds +net
- +content shouldn't be here; seems like it's for the TestBrowserThreadBundle,
but I don't think the bundle is necessary.

https://codereview.chromium.org/99433004/diff/40001/components/policy/core/br...
File components/policy/core/browser/policy_header_io_helper.cc (right):

https://codereview.chromium.org/99433004/diff/40001/components/policy/core/br...
components/policy/core/browser/policy_header_io_helper.cc:39:
url.spec().compare(0, server_url_.size(), server_url_) == 0) {
I guess this works for our purposes. I wonder if this will correctly match
"https://m.google.com:443/devicemanagement/api" though?

https://codereview.chromium.org/99433004/diff/40001/components/policy/core/br...
File components/policy/core/browser/policy_header_io_helper.h (right):

https://codereview.chromium.org/99433004/diff/40001/components/policy/core/br...
components/policy/core/browser/policy_header_io_helper.h:9: #include
"base/memory/ref_counted.h"
nit: usually a newline is left between <system> and "chrome" headers

https://codereview.chromium.org/99433004/diff/40001/components/policy/core/br...
File components/policy/core/browser/policy_header_io_helper_unittest.cc (right):

https://codereview.chromium.org/99433004/diff/40001/components/policy/core/br...
components/policy/core/browser/policy_header_io_helper_unittest.cc:4: #include
"base/test/test_simple_task_runner.h"
nit: newline above this one

https://codereview.chromium.org/99433004/diff/40001/components/policy/core/br...
components/policy/core/browser/policy_header_io_helper_unittest.cc:31: virtual
void TearDown() {
OVERRIDE

https://codereview.chromium.org/99433004/diff/40001/components/policy/core/br...
components/policy/core/browser/policy_header_io_helper_unittest.cc:46:
content::TestBrowserThreadBundle thread_bundle_;
This can be replaced by a MessageLoop, possibly initialized as
message_loop_(base::MessageLoop::TYPE_IO). That would avoid the need for the
+content DEPS exception.

It's probably a good idea to have the loop as the first member field too, so
that it gets destroyed after all the other stuff, which may try to post.

[Andrew T Wilson (Slow), 2013-12-10 07:15:55]

Joao, PTAL

jam: need an OWNERS stamp for chrome_resource_dispatcher_host_delegate.cc
willchan: need an OWNERS stamp for profile_io_data.h/cc

https://codereview.chromium.org/99433004/diff/20001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/policy_header_service.h (right):

https://codereview.chromium.org/99433004/diff/20001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/policy_header_service.h:36: // Creates a
PolicyHeaderIOHelper object to be run on the IO thread and
On 2013/12/09 12:47:35, dconnelly wrote:
> Here and elsewhere I would prefer to eliminate the "on the I/O" and "on the
UI"
> thread stuff. The methods of PolicyHeaderIOHelper could be renamed to
> "UpdateHeader' and "UpdateHeaderOnTaskRunner" instead, or similar.

Done.

https://codereview.chromium.org/99433004/diff/20001/components/policy/core/br...
File components/policy/core/browser/DEPS (left):

https://codereview.chromium.org/99433004/diff/20001/components/policy/core/br...
components/policy/core/browser/DEPS:3: "+ui/base/l10n",
On 2013/12/09 12:47:35, dconnelly wrote:
> why are these removed?

This was a merge error.

https://codereview.chromium.org/99433004/diff/20001/components/policy/core/br...
File components/policy/core/browser/DEPS (right):

https://codereview.chromium.org/99433004/diff/20001/components/policy/core/br...
components/policy/core/browser/DEPS:2: "+content/public/test",
On 2013/12/09 12:47:35, dconnelly wrote:
> I'm not convinced this is necessary, but if it is, the tests that require it
> should move to ../content.

Reverted this change to this DEPS file.

https://codereview.chromium.org/99433004/diff/20001/components/policy/core/br...
File components/policy/core/browser/policy_header_io_helper_unittest.cc (right):

https://codereview.chromium.org/99433004/diff/20001/components/policy/core/br...
components/policy/core/browser/policy_header_io_helper_unittest.cc:4: #include
"base/test/test_simple_task_runner.h"
On 2013/12/09 12:47:35, dconnelly wrote:
> newline above

Done.

https://codereview.chromium.org/99433004/diff/20001/components/policy/core/br...
components/policy/core/browser/policy_header_io_helper_unittest.cc:50:
task_runner_->RunUntilIdle();
On 2013/12/09 12:47:35, dconnelly wrote:
> Can this move to the end of SetUp instead of reproducing at the beginning of
> every test?

Done.

https://codereview.chromium.org/99433004/diff/20001/components/policy/core/br...
components/policy/core/browser/policy_header_io_helper_unittest.cc:50:
task_runner_->RunUntilIdle();
On 2013/12/09 12:47:35, dconnelly wrote:
> Can this move to the end of SetUp instead of reproducing at the beginning of
> every test?

Done.

https://codereview.chromium.org/99433004/diff/40001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/policy_header_service.h (right):

https://codereview.chromium.org/99433004/diff/40001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/policy_header_service.h:63: };
On 2013/12/09 14:46:21, Joao da Silva wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/99433004/diff/40001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/policy_header_service_factory.cc (right):

https://codereview.chromium.org/99433004/diff/40001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/policy_header_service_factory.cc:22: class
PolicyHeaderServiceWrapper : public BrowserContextKeyedService {
On 2013/12/09 14:46:21, Joao da Silva wrote:
> Put this in an anonymous namespace.

Done. Good catch.

> 
> A wrapper like this works; another possibility is to inhering from both BCKS
and
> PolicyHeaderService. You'd have to forwards the Shutdown() call to the
> PolicyHeaderService though.

Yeah, "favor composition over inheritance" seems to dictate that my current impl
is better. Especially since PolicyHeaderService doesn't have a Shutdown() method
(it does its cleanup in the destructor).

https://codereview.chromium.org/99433004/diff/40001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/policy_header_service_factory.cc:38:
scoped_ptr<PolicyHeaderService> policy_header_service_;
On 2013/12/09 14:46:21, Joao da Silva wrote:
> #include scoped_ptr.h

Done.

https://codereview.chromium.org/99433004/diff/40001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/policy_header_service_factory.cc:45:
DependsOn(UserCloudPolicyManagerFactory::GetInstance());
On 2013/12/09 14:46:21, Joao da Silva wrote:
> On ChromeOS this should be UserCloudPolicyManagerFactoryChromeOS
Hmm, that seems like an API wart. But done (plus sent an email about this - we
should really change this API).

https://codereview.chromium.org/99433004/diff/40001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/policy_header_service_factory.cc:56:
GetInstance()->GetServiceForBrowserContext(context, true));
On 2013/12/09 14:46:21, Joao da Silva wrote:
> |wrapper| will be NULL for incognito profiles. I think a NULL check is a good
> idea though this isn't called for those profiles. WDYT?

Done.

https://codereview.chromium.org/99433004/diff/40001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/policy_header_service_factory.cc:68:
UserCloudPolicyManagerFactory::GetForBrowserContext(context);
On 2013/12/09 14:46:21, Joao da Silva wrote:
> This needs an OS_CHROMEOS version

Done.

https://codereview.chromium.org/99433004/diff/40001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/policy_header_service_factory.cc:78: return new
PolicyHeaderServiceWrapper(service.Pass());
On 2013/12/09 14:46:21, Joao da Silva wrote:
> indentation

Done.

https://codereview.chromium.org/99433004/diff/40001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/policy_header_service_factory.h (right):

https://codereview.chromium.org/99433004/diff/40001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/policy_header_service_factory.h:11: class Profile;
On 2013/12/09 14:46:21, Joao da Silva wrote:
> not used

Done.

https://codereview.chromium.org/99433004/diff/40001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/99433004/diff/40001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_io_data.cc:51: #include
"chrome/browser/policy/cloud/user_cloud_policy_manager_factory.h"
On 2013/12/09 14:46:21, Joao da Silva wrote:
> #if defined(ENABLE_CONFIGURATION_POLICY)

Done.

https://codereview.chromium.org/99433004/diff/40001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_io_data.cc:62: #include
"components/policy/core/browser/policy_header_io_helper.h"
On 2013/12/09 14:46:21, Joao da Silva wrote:
> #if defined(...)

Done.

https://codereview.chromium.org/99433004/diff/40001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/99433004/diff/40001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_io_data.h:500: mutable
scoped_ptr<policy::PolicyHeaderIOHelper> policy_header_helper_;
On 2013/12/09 14:46:21, Joao da Silva wrote:
> I think this needs an #ifdef, otherwise non-policy builds won't know how to
> destroy this object.
> 
> Try sending this CL to linux_redux, it's actually building lately. It will
tell
> whether this breaks the iOS build.

Done.

https://codereview.chromium.org/99433004/diff/40001/chrome/browser/renderer_h...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/99433004/diff/40001/chrome/browser/renderer_h...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:41:
#include "components/policy/core/browser/policy_header_io_helper.h"
On 2013/12/09 14:46:21, Joao da Silva wrote:
> #ifdef

Done.

https://codereview.chromium.org/99433004/diff/40001/components/policy/core/br...
File components/policy/core/browser/DEPS (right):

https://codereview.chromium.org/99433004/diff/40001/components/policy/core/br...
components/policy/core/browser/DEPS:5: ]
On 2013/12/09 14:46:21, Joao da Silva wrote:
> This file shouldn't need to change:
> 
> - ../DEPS already adds +net
> - +content shouldn't be here; seems like it's for the TestBrowserThreadBundle,
> but I don't think the bundle is necessary.

Reverted this file.

https://codereview.chromium.org/99433004/diff/40001/components/policy/core/br...
File components/policy/core/browser/policy_header_io_helper.cc (right):

https://codereview.chromium.org/99433004/diff/40001/components/policy/core/br...
components/policy/core/browser/policy_header_io_helper.cc:39:
url.spec().compare(0, server_url_.size(), server_url_) == 0) {
On 2013/12/09 14:46:21, Joao da Silva wrote:
> I guess this works for our purposes. I wonder if this will correctly match
> "https://m.google.com:443/devicemanagement/api" though?

I don't think we care - I'm OK with requiring specific URL formats for this
mechanism to work. If you configure your extension and/or SAML endpoint to try
to contact m.google.com:443 or directly via the IP address or via some other
method, Chrome won't report policy, and that's OK.

https://codereview.chromium.org/99433004/diff/40001/components/policy/core/br...
File components/policy/core/browser/policy_header_io_helper.h (right):

https://codereview.chromium.org/99433004/diff/40001/components/policy/core/br...
components/policy/core/browser/policy_header_io_helper.h:9: #include
"base/memory/ref_counted.h"
On 2013/12/09 14:46:21, Joao da Silva wrote:
> nit: usually a newline is left between <system> and "chrome" headers

Done.

https://codereview.chromium.org/99433004/diff/40001/components/policy/core/br...
File components/policy/core/browser/policy_header_io_helper_unittest.cc (right):

https://codereview.chromium.org/99433004/diff/40001/components/policy/core/br...
components/policy/core/browser/policy_header_io_helper_unittest.cc:4: #include
"base/test/test_simple_task_runner.h"
On 2013/12/09 14:46:21, Joao da Silva wrote:
> nit: newline above this one

Done.

https://codereview.chromium.org/99433004/diff/40001/components/policy/core/br...
components/policy/core/browser/policy_header_io_helper_unittest.cc:31: virtual
void TearDown() {
On 2013/12/09 14:46:21, Joao da Silva wrote:
> OVERRIDE

Done.

https://codereview.chromium.org/99433004/diff/40001/components/policy/core/br...
components/policy/core/browser/policy_header_io_helper_unittest.cc:46:
content::TestBrowserThreadBundle thread_bundle_;
On 2013/12/09 14:46:21, Joao da Silva wrote:
> This can be replaced by a MessageLoop, possibly initialized as
> message_loop_(base::MessageLoop::TYPE_IO). That would avoid the need for the
> +content DEPS exception.
> 
> It's probably a good idea to have the loop as the first member field too, so
> that it gets destroyed after all the other stuff, which may try to post.

Done.

[Andrew T Wilson (Slow), 2013-12-10 07:17:19]

Adding jam/willchan for realz this time:

John: ptal at chrome_resource_dispatcher_host_delegate.cc
Will: ptal at profile_io_data.h/cc

[Joao da Silva, 2013-12-10 08:34:21]

lgtm with a couple nits

https://codereview.chromium.org/99433004/diff/70001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/policy_header_service_factory.cc (right):

https://codereview.chromium.org/99433004/diff/70001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/policy_header_service_factory.cc:12: #include
"chrome/browser/policy/cloud/user_cloud_policy_manager_factory.h"
Note that user_cloud_policy_manager[_factory].(h|cc) are excluded on ChromeOS
builds. These 2 includes should be conditional on !defined(OS_CHROMEOS)

https://codereview.chromium.org/99433004/diff/70001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/policy_header_service_factory.cc:81:
UserCloudPolicyManager* manager =
UserCloudPolicyManagerChromeOS

https://codereview.chromium.org/99433004/diff/70001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/policy_header_service_unittest.cc (right):

https://codereview.chromium.org/99433004/diff/70001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/policy_header_service_unittest.cc:96:
DCHECK(helper_);
Replace DCHECKs with EXPECT_TRUE

https://codereview.chromium.org/99433004/diff/70001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/99433004/diff/70001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_io_data.cc:53: #endif
Move this block below, after the unconditional #includes

https://codereview.chromium.org/99433004/diff/70001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_io_data.cc:66: #endif
ditto

https://codereview.chromium.org/99433004/diff/70001/chrome/browser/renderer_h...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/99433004/diff/70001/chrome/browser/renderer_h...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:43:
#endif
Move to after the unconditional includes

[Andrew T Wilson (Slow), 2013-12-10 20:04:56]

https://codereview.chromium.org/99433004/diff/70001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/policy_header_service_factory.cc (right):

https://codereview.chromium.org/99433004/diff/70001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/policy_header_service_factory.cc:12: #include
"chrome/browser/policy/cloud/user_cloud_policy_manager_factory.h"
On 2013/12/10 08:34:21, Joao da Silva wrote:
> Note that user_cloud_policy_manager[_factory].(h|cc) are excluded on ChromeOS
> builds. These 2 includes should be conditional on !defined(OS_CHROMEOS)

Done.

https://codereview.chromium.org/99433004/diff/70001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/policy_header_service_factory.cc:81:
UserCloudPolicyManager* manager =
On 2013/12/10 08:34:22, Joao da Silva wrote:
> UserCloudPolicyManagerChromeOS

Done.

https://codereview.chromium.org/99433004/diff/70001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/policy_header_service_unittest.cc (right):

https://codereview.chromium.org/99433004/diff/70001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/policy_header_service_unittest.cc:96:
DCHECK(helper_);
On 2013/12/10 08:34:22, Joao da Silva wrote:
> Replace DCHECKs with EXPECT_TRUE

Done.

https://codereview.chromium.org/99433004/diff/70001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/99433004/diff/70001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_io_data.cc:53: #endif
On 2013/12/10 08:34:22, Joao da Silva wrote:
> Move this block below, after the unconditional #includes

Done.

https://codereview.chromium.org/99433004/diff/70001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_io_data.cc:66: #endif
On 2013/12/10 08:34:22, Joao da Silva wrote:
> ditto

Done.

https://codereview.chromium.org/99433004/diff/70001/chrome/browser/renderer_h...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/99433004/diff/70001/chrome/browser/renderer_h...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:43:
#endif
On 2013/12/10 08:34:22, Joao da Silva wrote:
> Move to after the unconditional includes

Done.

[jam, 2013-12-10 20:14:48]

On 2013/12/10 07:17:19, Andrew T Wilson wrote:
> Adding jam/willchan for realz this time:
> 
> John: ptal at chrome_resource_dispatcher_host_delegate.cc
lgtm
> Will: ptal at profile_io_data.h/cc

[Joao da Silva, 2013-12-10 20:53:29]

lgtm

[willchan no longer on Chromium, 2013-12-10 23:27:08]

https://codereview.chromium.org/99433004/diff/90001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/99433004/diff/90001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_io_data.cc:488: io_message_loop_proxy).Pass();
I don't think you need the .Pass() here, do you?

https://codereview.chromium.org/99433004/diff/90001/components/policy/core/br...
File components/policy/core/browser/policy_header_io_helper.cc (right):

https://codereview.chromium.org/99433004/diff/90001/components/policy/core/br...
components/policy/core/browser/policy_header_io_helper.cc:51:
weak_factory_.GetWeakPtr(), new_header));
Is this correct? It looks like the WeakPtr is acquired on the UI thread and
destroyed on the IO thread. GetWeakPtr() will set the thread for the WeakPtr,
and it's not threadsafe...

Furthermore, stuff owned by ProfileIOData will always get deleted on the IO
thread after the Profile is already destroyed on the UI thread. So UI=>IO thread
is always safe, whereas IO=>UI task posting must be protected by a WeakPtr.

In short, I think you should just ditch the WeakPtr stuff here.

https://codereview.chromium.org/99433004/diff/90001/components/policy/core/br...
File components/policy/core/browser/policy_header_io_helper.h (right):

https://codereview.chromium.org/99433004/diff/90001/components/policy/core/br...
components/policy/core/browser/policy_header_io_helper.h:31:
scoped_refptr<base::SequencedTaskRunner> task_runner);
const reference to eliminate the extra copy for the scoped_refptr?

[willchan no longer on Chromium, 2013-12-10 23:32:14]

https://codereview.chromium.org/99433004/diff/70001/components/policy/core/br...
File components/policy/core/browser/policy_header_io_helper.cc (right):

https://codereview.chromium.org/99433004/diff/70001/components/policy/core/br...
components/policy/core/browser/policy_header_io_helper.cc:50:
base::Bind(&PolicyHeaderIOHelper::UpdateHeaderOnTaskRunner,
Suggestions:
s/task_runner_/io_task_runner_/
s/UpdateHeaderOnTaskRunner/UpdateHeaderOnIOThread/

I don't think there's an advantage to being generic in naming here, since use of
this object is fairly tightly coupled to assumptions around content/'s threading
architecture.

[Andrew T Wilson (Slow), 2013-12-10 23:53:05]

PTAL

https://codereview.chromium.org/99433004/diff/70001/components/policy/core/br...
File components/policy/core/browser/policy_header_io_helper.cc (right):

https://codereview.chromium.org/99433004/diff/70001/components/policy/core/br...
components/policy/core/browser/policy_header_io_helper.cc:50:
base::Bind(&PolicyHeaderIOHelper::UpdateHeaderOnTaskRunner,
On 2013/12/10 23:32:14, willchan wrote:
> Suggestions:
> s/task_runner_/io_task_runner_/
> s/UpdateHeaderOnTaskRunner/UpdateHeaderOnIOThread/
> 
> I don't think there's an advantage to being generic in naming here, since use
of
> this object is fairly tightly coupled to assumptions around content/'s
threading
> architecture.

Done. This was my original intent, but I got pushback from another reviewer. But
since I agree with you, I'm changing this back to be more specific :)

https://codereview.chromium.org/99433004/diff/90001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/99433004/diff/90001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_io_data.cc:488: io_message_loop_proxy).Pass();
On 2013/12/10 23:27:09, willchan wrote:
> I don't think you need the .Pass() here, do you?

Removed.

https://codereview.chromium.org/99433004/diff/90001/components/policy/core/br...
File components/policy/core/browser/policy_header_io_helper.cc (right):

https://codereview.chromium.org/99433004/diff/90001/components/policy/core/br...
components/policy/core/browser/policy_header_io_helper.cc:51:
weak_factory_.GetWeakPtr(), new_header));
On 2013/12/10 23:27:09, willchan wrote:
> Is this correct? It looks like the WeakPtr is acquired on the UI thread and
> destroyed on the IO thread. GetWeakPtr() will set the thread for the WeakPtr,
> and it's not threadsafe...
> 
> Furthermore, stuff owned by ProfileIOData will always get deleted on the IO
> thread after the Profile is already destroyed on the UI thread. So UI=>IO
thread
> is always safe, whereas IO=>UI task posting must be protected by a WeakPtr.
> 
> In short, I think you should just ditch the WeakPtr stuff here.

Done. Makes sense to me.

https://codereview.chromium.org/99433004/diff/90001/components/policy/core/br...
File components/policy/core/browser/policy_header_io_helper.h (right):

https://codereview.chromium.org/99433004/diff/90001/components/policy/core/br...
components/policy/core/browser/policy_header_io_helper.h:31:
scoped_refptr<base::SequencedTaskRunner> task_runner);
On 2013/12/10 23:27:09, willchan wrote:
> const reference to eliminate the extra copy for the scoped_refptr?

OK, I didn't think the cost of creating the scoped_refptr was a big deal, but
I've changed it as you suggest.

[willchan no longer on Chromium, 2013-12-10 23:55:20]

LGTM

https://codereview.chromium.org/99433004/diff/90001/components/policy/core/br...
File components/policy/core/browser/policy_header_io_helper.h (right):

https://codereview.chromium.org/99433004/diff/90001/components/policy/core/br...
components/policy/core/browser/policy_header_io_helper.h:31:
scoped_refptr<base::SequencedTaskRunner> task_runner);
On 2013/12/10 23:53:06, Andrew T Wilson wrote:
> On 2013/12/10 23:27:09, willchan wrote:
> > const reference to eliminate the extra copy for the scoped_refptr?
> 
> OK, I didn't think the cost of creating the scoped_refptr was a big deal, but
> I've changed it as you suggest.

Just one of those death by a thousand cut things. Especially for ARM devices
since atomics are more expensive there (this uses an atomic increment/decrement
pair for the threadsafe refcounted objects).

[commit-bot: I haz the power, 2013-12-10 23:59:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/atwilson@chromium.org/99433004/130001

[commit-bot: I haz the power, 2013-12-11 00:19:26]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Andrew T Wilson (Slow), 2013-12-11 00:31:33]

On 2013/12/11 00:19:26, I haz the power (commit-bot) wrote:
> Retried try job too often on chromium_presubmit for step(s) presubmit
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

TBR jochen for components_tests.gyp. We should consider relaxing the ownership
on this file.

[commit-bot: I haz the power, 2013-12-11 00:34:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/atwilson@chromium.org/99433004/130001

[commit-bot: I haz the power, 2013-12-11 04:58:03]

Change committed as 240019