Use ThreadChecker rather than DCHECK_CURRENTLY_ON.

Use ThreadChecker rather than DCHECK_CURRENTLY_ON.

This is to remove the code dependency on //content/ so that more
content_settings files can be in components/content_settings/core.

At the first glance, it may look like adding a new restriction
that HostContentSettingsMap has to be initialized on UI thread.
However, this doesn't change any conditions. Actually, that
restriction already exists on the current code implicitly on:
- HostContentSettingsMap creates OverrideProvider
- OverrideProvider ctor calls ReadOverrideSettings
- ReadOverrideSettings assumes called on UI thread

So this CL simply moves the thread checking.

BUG=384876
R=bauerb@chromium.org, vabr@chromium.org, rlp@chromium.org, rocket@chromium.org, phajdan.jr@chromium.org
TEST=no breaks on existing tests

Committed: https://crrev.com/a1aa3518032217fd605d9435a31fc2b6c4e5e3f9
Cr-Commit-Position: refs/heads/master@{#300733}

[Jun Mukai, 2014-10-01 08:13:33]

mukai@chromium.org changed reviewers:
+ bauerb@chromium.org, rlp@chromium.org, vabr@chromium.org

[blundell, 2014-10-01 08:15:45]

On 2014/10/01 08:13:53, Jun Mukai wrote:

(you should specify which parts of the CL you want different people to review)

[Jun Mukai, 2014-10-01 08:54:46]

On 2014/10/01 08:15:45, blundell wrote:
> On 2014/10/01 08:13:53, Jun Mukai wrote:
> 
> (you should specify which parts of the CL you want different people to review)

vabr, bauerb: for content_settings, general design
rlp: for profile

[vabr (Chromium), 2014-10-01 11:14:48]

Thanks Jun!
LGTM with two comments/questions.
Vaclav

https://codereview.chromium.org/615083004/diff/20001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/615083004/diff/20001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.cc:635: // Make sure HostContentSettingsMap
is created on the UI thread.
This only really makes sure that HCSM is created on the UI, if we expect HCSM is
actually created here. If that's the case, should there be a
DCHECK(!host_content_settings_map_); just before the GetHostContentSettingsMap()
call?

Alternatively, should we somehow let HCSM check through its new thread_checker_,
that this is the thread it got created on?

If you change something here to accommodate the above, please also change the
OTR profile accordingly.

https://codereview.chromium.org/615083004/diff/20001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.cc:635: // Make sure HostContentSettingsMap
is created on the UI thread.
Also, should chrome/test/base/testing_profile.cc get a similar call to
GetHostContentSettingsMap() in its Init() or FinishInit()?

[Bernhard Bauer, 2014-10-01 11:41:09]

https://codereview.chromium.org/615083004/diff/20001/chrome/browser/content_s...
File chrome/browser/content_settings/host_content_settings_map.h (right):

https://codereview.chromium.org/615083004/diff/20001/chrome/browser/content_s...
chrome/browser/content_settings/host_content_settings_map.h:48: // Note that
HostContentSettingsMap assumes it's created on the UI thread.
Nit: Move this to the constructor, and change to match the style of the other
thread safety comments in here ("This should only be called on the UI thread").

https://codereview.chromium.org/615083004/diff/20001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/615083004/diff/20001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.cc:635: // Make sure HostContentSettingsMap
is created on the UI thread.
On 2014/10/01 11:14:48, vabr (Chromium) wrote:
> This only really makes sure that HCSM is created on the UI, if we expect HCSM
is
> actually created here. If that's the case, should there be a
> DCHECK(!host_content_settings_map_); just before the
GetHostContentSettingsMap()
> call?
> 
> Alternatively, should we somehow let HCSM check through its new
thread_checker_,
> that this is the thread it got created on?
> 
> If you change something here to accommodate the above, please also change the
> OTR profile accordingly.

Really, if we have a defined place where we construct the HCSM, we don't need to
do the lazy construction at all anyway.

Alternatively, we could check that GetHostContentSettingsMap() is only called on
the UI thread? That's a valid precondition, because Profile is UI-thread only.

[Jun Mukai, 2014-10-14 23:53:13]

Sorry for the long interval but addressed the comments. Please take another
look.

https://codereview.chromium.org/615083004/diff/20001/chrome/browser/content_s...
File chrome/browser/content_settings/host_content_settings_map.h (right):

https://codereview.chromium.org/615083004/diff/20001/chrome/browser/content_s...
chrome/browser/content_settings/host_content_settings_map.h:48: // Note that
HostContentSettingsMap assumes it's created on the UI thread.
On 2014/10/01 11:41:09, bauerb (OOO til Oct 15) wrote:
> Nit: Move this to the constructor, and change to match the style of the other
> thread safety comments in here ("This should only be called on the UI
thread").

Done.

https://codereview.chromium.org/615083004/diff/20001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/615083004/diff/20001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.cc:635: // Make sure HostContentSettingsMap
is created on the UI thread.
On 2014/10/01 11:41:09, bauerb (OOO til Oct 15) wrote:
> On 2014/10/01 11:14:48, vabr (Chromium) wrote:
> > This only really makes sure that HCSM is created on the UI, if we expect
HCSM
> is
> > actually created here. If that's the case, should there be a
> > DCHECK(!host_content_settings_map_); just before the
> GetHostContentSettingsMap()
> > call?
> > 
> > Alternatively, should we somehow let HCSM check through its new
> thread_checker_,
> > that this is the thread it got created on?
> > 
> > If you change something here to accommodate the above, please also change
the
> > OTR profile accordingly.
> 
> Really, if we have a defined place where we construct the HCSM, we don't need
to
> do the lazy construction at all anyway.
> 
> Alternatively, we could check that GetHostContentSettingsMap() is only called
on
> the UI thread? That's a valid precondition, because Profile is UI-thread only.

Indeed, I was confused previously but we can add DCHECK in
GetHostContentSettingsMap() (ProfileIOData has the pointer to the instance, but
it invokes GetHostContentSettingsMap in UI thread and then remembers the value).
 Fixed.

[rpetterson, 2014-10-15 00:07:02]

profiles LGTM

[vabr (Chromium), 2014-10-15 10:41:05]

Still LGTM,
Thanks!
Vaclav

[Bernhard Bauer, 2014-10-15 12:37:03]

https://codereview.chromium.org/615083004/diff/40001/chrome/browser/content_s...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

https://codereview.chromium.org/615083004/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/host_content_settings_map.cc:80: // This should
be called on the UI thread, otherwise |thread_checker_| handles
Why is this comment here? It might make more sense in the header.

[Jun Mukai, 2014-10-15 23:46:19]

https://codereview.chromium.org/615083004/diff/40001/chrome/browser/content_s...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

https://codereview.chromium.org/615083004/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/host_content_settings_map.cc:80: // This should
be called on the UI thread, otherwise |thread_checker_| handles
On 2014/10/15 12:37:02, Bernhard Bauer wrote:
> Why is this comment here? It might make more sense in the header.

I misunderstood your previous comment. Moved back to the header.

[Bernhard Bauer, 2014-10-16 08:25:01]

LGTM!

[Jun Mukai, 2014-10-16 16:34:45]

The CQ bit was checked by mukai@chromium.org

[commit-bot: I haz the power, 2014-10-16 16:35:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/615083004/60001

[commit-bot: I haz the power, 2014-10-16 16:44:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-16 16:44:12]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Jun Mukai, 2014-10-17 18:04:56]

mukai@chromium.org changed reviewers:
+ phajdan.jr@chromium.org, rockot@chromium.org

[Jun Mukai, 2014-10-17 18:05:45]

rockot, pajdan.jr, please review:
- rockot: chrome/browser/extensions/extension_service.cc
- phajdan.jr: chrome/test/base/testing_profile.cc

thanks!

[Ken Rockot(use gerrit already), 2014-10-17 18:09:42]

On 2014/10/17 18:05:45, Jun Mukai wrote:
> rockot, pajdan.jr, please review:
> - rockot: chrome/browser/extensions/extension_service.cc
> - phajdan.jr: chrome/test/base/testing_profile.cc
> 
> thanks!

chrome/browser/extensions/extension_service.cc lgtm

[Paweł Hajdan Jr., 2014-10-20 09:11:51]

https://codereview.chromium.org/615083004/diff/60001/chrome/test/base/testing...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/615083004/diff/60001/chrome/test/base/testing...
chrome/test/base/testing_profile.cc:833:
DCHECK(!content::BrowserThread::IsThreadInitialized(
This seems the only place in this patch where IsThreadInitialized is used. Why
is that needed?

[Jun Mukai, 2014-10-20 17:02:51]

https://codereview.chromium.org/615083004/diff/60001/chrome/test/base/testing...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/615083004/diff/60001/chrome/test/base/testing...
chrome/test/base/testing_profile.cc:833:
DCHECK(!content::BrowserThread::IsThreadInitialized(
On 2014/10/20 09:11:51, Paweł Hajdan Jr. wrote:
> This seems the only place in this patch where IsThreadInitialized is used. Why
> is that needed?

I copied this check from TestingProfile::Init().  I haven't tried the case
without this check, but I believe that some unit tests may not run
BrowserThread::UI, and enforcing to run BrowserThread for any tests using
TestingProfile is a bad idea.

[Paweł Hajdan Jr., 2014-10-21 09:30:58]

https://codereview.chromium.org/615083004/diff/60001/chrome/test/base/testing...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/615083004/diff/60001/chrome/test/base/testing...
chrome/test/base/testing_profile.cc:833:
DCHECK(!content::BrowserThread::IsThreadInitialized(
On 2014/10/20 17:02:51, Jun Mukai wrote:
> On 2014/10/20 09:11:51, Paweł Hajdan Jr. wrote:
> > This seems the only place in this patch where IsThreadInitialized is used.
Why
> > is that needed?
> 
> I copied this check from TestingProfile::Init().

If it's really needed, please extract it to a common method.

> I haven't tried the case
> without this check, but I believe that some unit tests may not run
> BrowserThread::UI,

Could you try it?

> and enforcing to run BrowserThread for any tests using
> TestingProfile is a bad idea.

Why?

[Jun Mukai, 2014-10-22 06:30:13]

https://codereview.chromium.org/615083004/diff/60001/chrome/test/base/testing...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/615083004/diff/60001/chrome/test/base/testing...
chrome/test/base/testing_profile.cc:833:
DCHECK(!content::BrowserThread::IsThreadInitialized(
On 2014/10/21 09:30:58, Paweł Hajdan Jr. wrote:
> On 2014/10/20 17:02:51, Jun Mukai wrote:
> > On 2014/10/20 09:11:51, Paweł Hajdan Jr. wrote:
> > > This seems the only place in this patch where IsThreadInitialized is used.
> Why
> > > is that needed?
> > 
> > I copied this check from TestingProfile::Init().
> 
> If it's really needed, please extract it to a common method.
> 
> > I haven't tried the case
> > without this check, but I believe that some unit tests may not run
> > BrowserThread::UI,
> 
> Could you try it?
> 

Tried and I was wrong. Changed to use DCHECK_CURRENTLY_ON.  PTAL.

> > and enforcing to run BrowserThread for any tests using
> > TestingProfile is a bad idea.
> 
> Why?

[Paweł Hajdan Jr., 2014-10-22 11:20:48]

chrome/test LGTM

Thank you for your patience with my review comment and doing the test. I'm glad
we've managed to slightly simplify the code this way.

[Jun Mukai, 2014-10-22 16:50:37]

The CQ bit was checked by mukai@chromium.org

[commit-bot: I haz the power, 2014-10-22 16:51:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/615083004/80001

[commit-bot: I haz the power, 2014-10-22 17:03:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-22 17:03:52]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Jun Mukai, 2014-10-22 18:08:34]

The CQ bit was checked by mukai@chromium.org

[commit-bot: I haz the power, 2014-10-22 18:09:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/615083004/80001

[commit-bot: I haz the power, 2014-10-22 19:28:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-22 19:28:25]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Jun Mukai, 2014-10-22 19:35:46]

The CQ bit was checked by mukai@chromium.org

[commit-bot: I haz the power, 2014-10-22 19:37:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/615083004/80001

[commit-bot: I haz the power, 2014-10-22 20:14:20]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2014-10-22 20:15:28]

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