Replace deprecated base::NonThreadSafe in chrome/browser/extensions in favor of SequenceChecker.

Replace deprecated base::NonThreadSafe in chrome/browser/extensions in favor of SequenceChecker.

Note to crash team: This CL is a refactor and has no intended behavior change.

This change was scripted by https://crbug.com/676387#c8.

... and then replaced by DCHECK_CURRENTLY_ON(UI) per request.

Which forced a fix to .../chromeos/printing/printers_manager_unittest.cc
as its TestBrowserThreadBundle was initialized too late for TestingProfile's
constructor (which would now crash in DCHECK_CURRENTLY_ON()), switched to 
using proper member stack ordering instead of trying to do more or less 
that but not quite correctly with heap allocations.

BUG=676387
This CL was uploaded by git cl split.

Review-Url: https://codereview.chromium.org/2912003002
Cr-Commit-Position: refs/heads/master@{#476061}
Committed: https://chromium.googlesource.com/chromium/src/+/234cd78265fdc3965626d10b14d37e8b09b39c86

[gab, 2017-05-30 02:41:55]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[gab, 2017-05-30 02:41:57]

Hi, this is an automated change made by https://crbug.com/676387#c8 and sharded
across directories for OWNERS.

No human has looked at this specific output yet, see cl description for things
to watch for.

If it LGTY, please CQ.

If it didn't pass CQ dry-run it may be a simple patch failure or an IWYU failure
from having this being a partial CL... please review anyways and a human will
look at the failure (and ping for re-review if anything major is required..). It
is known to compile all targets on Windows at r474679...  manual tweaks will
likely be required to smoothen things off, bear with me!

Thanks!
Gab

[commit-bot: I haz the power, 2017-05-30 02:42:06]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2017-05-30 02:57:58]

gab@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org
- lazyboy@chromium.org

[gab, 2017-05-30 02:57:59]

s/lazyboy (OOO)/rdevlin.cronin/

[commit-bot: I haz the power, 2017-05-30 03:36:29]

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

[commit-bot: I haz the power, 2017-05-30 03:36:29]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-05-30 20:04:25]

https://codereview.chromium.org/2912003002/diff/1/chrome/browser/extensions/w...
File chrome/browser/extensions/warning_badge_service.h (right):

https://codereview.chromium.org/2912003002/diff/1/chrome/browser/extensions/w...
chrome/browser/extensions/warning_badge_service.h:53:
SEQUENCE_CHECKER(sequence_checker_);
This class should only ever be used on the UI thread, so we should probably use
ThreadChecker (or DCHECK_CURRENTLY_ON)

[gab, 2017-05-31 16:35:59]

Description was changed from

==========
Replace deprecated base::NonThreadSafe in chrome/browser/extensions in favor of
SequenceChecker.

Note to crash team: This CL is a refactor and has no intended behavior change.

This change was scripted by https://crbug.com/676387#c8.

Note-worthy for the reviewer:
 * SequenceChecker enforces thread-safety but not thread-affinity!
   If the classes that were updated are thread-affine (use thread local
   storage or a third-party API that does) they should be migrated to
   ThreadChecker instead.
 * ~NonThreadSafe() used to implcitly check in its destructor
   ~Sequence/ThreadChecker() doesn't by design. To keep this CL a
   no-op, an explicit check was added to the destructor of migrated
   classes.
 * NonThreadSafe used to provide access to subclasses, as such
   the |sequence_checker_| member was made protected rather than
   private where necessary.

BUG=676387
This CL was uploaded by git cl split.

R=lazyboy@chromium.org
==========

to

==========
Replace deprecated base::NonThreadSafe in chrome/browser/extensions in favor of
SequenceChecker.

Note to crash team: This CL is a refactor and has no intended behavior change.

This change was scripted by https://crbug.com/676387#c8.

(and then replaced by DCHECK_CURRENTLY_ON(UI) per request.

BUG=676387
This CL was uploaded by git cl split.

R=lazyboy@chromium.org
==========

[gab, 2017-05-31 16:36:24]

https://codereview.chromium.org/2912003002/diff/1/chrome/browser/extensions/w...
File chrome/browser/extensions/warning_badge_service.h (right):

https://codereview.chromium.org/2912003002/diff/1/chrome/browser/extensions/w...
chrome/browser/extensions/warning_badge_service.h:53:
SEQUENCE_CHECKER(sequence_checker_);
On 2017/05/30 20:04:25, Devlin wrote:
> This class should only ever be used on the UI thread, so we should probably
use
> ThreadChecker (or DCHECK_CURRENTLY_ON)

Done.

[gab, 2017-05-31 16:36:33]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-31 16:37:11]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-31 18:18:43]

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

[commit-bot: I haz the power, 2017-05-31 18:18:44]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[gab, 2017-05-31 18:29:25]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[gab, 2017-05-31 18:29:54]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-31 18:29:58]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2017-05-31 18:31:52]

gab@chromium.org changed reviewers:
+ thestig@chromium.org

[gab, 2017-05-31 18:31:53]

+Lei for corresponding fix to .../chromeos/printing/printers_manager_unittest.cc

(it's TestBrowserThreadBundle was initialized too late for TestingProfile's
constructor, switched to using proper member stack ordering instead of trying to
do more or less that but not quite correctly with heap allocations)

[gab, 2017-05-31 18:33:04]

Description was changed from

==========
Replace deprecated base::NonThreadSafe in chrome/browser/extensions in favor of
SequenceChecker.

Note to crash team: This CL is a refactor and has no intended behavior change.

This change was scripted by https://crbug.com/676387#c8.

(and then replaced by DCHECK_CURRENTLY_ON(UI) per request.

BUG=676387
This CL was uploaded by git cl split.

R=lazyboy@chromium.org
==========

to

==========
Replace deprecated base::NonThreadSafe in chrome/browser/extensions in favor of
SequenceChecker.

Note to crash team: This CL is a refactor and has no intended behavior change.

This change was scripted by https://crbug.com/676387#c8.

... and then replaced by DCHECK_CURRENTLY_ON(UI) per request.

Which forced a fix to .../chromeos/printing/printers_manager_unittest.cc
as its TestBrowserThreadBundle was initialized too late for TestingProfile's
constructor (which would now crash in DCHECK_CURRENTLY_ON()), switched to 
using proper member stack ordering instead of trying to do more or less 
that but not quite correctly with heap allocations.

BUG=676387
This CL was uploaded by git cl split.

R=lazyboy@chromium.org
==========

[Devlin, 2017-05-31 18:39:19]

extensions lgtm

[commit-bot: I haz the power, 2017-05-31 19:09:35]

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

[commit-bot: I haz the power, 2017-05-31 19:09:36]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Lei Zhang, 2017-05-31 19:10:23]

Looking...

+skau who actually wrote the printing code, FYI.

[gab, 2017-05-31 19:12:01]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-31 19:12:47]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Lei Zhang, 2017-05-31 19:14:23]

- R= line is out of date in the CL description.
- "git cl split" is a thing?

https://codereview.chromium.org/2912003002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/printing/printers_manager_unittest.cc (right):

https://codereview.chromium.org/2912003002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/printers_manager_unittest.cc:100: // Must be
first to be alive throughout |profile_|'s lifetime.
Just say "Must outlive |profile_|" ?

https://codereview.chromium.org/2912003002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/printers_manager_unittest.cc:103:
TestingProfile profile_;
Add a comment to say this must outlive |manager_| ?

[Lei Zhang, 2017-05-31 19:14:50]

and otherwise lgtm

[skau, 2017-05-31 19:22:09]

skau@chromium.org changed reviewers:
+ skau@chromium.org

[skau, 2017-05-31 19:22:10]

lgtm

Just a request for a comment on the constructor as I tend to forget how
initializers are ordered.

https://codereview.chromium.org/2912003002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/printing/printers_manager_unittest.cc (right):

https://codereview.chromium.org/2912003002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/printers_manager_unittest.cc:90: : manager_(
Can you note that member initialization happens before initializer lists are
evaluated?  I don't remember what the spec says and it's not obvious that this
is doing the right thing.

[commit-bot: I haz the power, 2017-05-31 20:39:45]

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

[commit-bot: I haz the power, 2017-05-31 20:39:46]

Dry run: This issue passed the CQ dry run.

[gab, 2017-05-31 21:16:39]

Description was changed from

==========
Replace deprecated base::NonThreadSafe in chrome/browser/extensions in favor of
SequenceChecker.

Note to crash team: This CL is a refactor and has no intended behavior change.

This change was scripted by https://crbug.com/676387#c8.

... and then replaced by DCHECK_CURRENTLY_ON(UI) per request.

Which forced a fix to .../chromeos/printing/printers_manager_unittest.cc
as its TestBrowserThreadBundle was initialized too late for TestingProfile's
constructor (which would now crash in DCHECK_CURRENTLY_ON()), switched to 
using proper member stack ordering instead of trying to do more or less 
that but not quite correctly with heap allocations.

BUG=676387
This CL was uploaded by git cl split.

R=lazyboy@chromium.org
==========

to

==========
Replace deprecated base::NonThreadSafe in chrome/browser/extensions in favor of
SequenceChecker.

Note to crash team: This CL is a refactor and has no intended behavior change.

This change was scripted by https://crbug.com/676387#c8.

... and then replaced by DCHECK_CURRENTLY_ON(UI) per request.

Which forced a fix to .../chromeos/printing/printers_manager_unittest.cc
as its TestBrowserThreadBundle was initialized too late for TestingProfile's
constructor (which would now crash in DCHECK_CURRENTLY_ON()), switched to 
using proper member stack ordering instead of trying to do more or less 
that but not quite correctly with heap allocations.

BUG=676387
This CL was uploaded by git cl split.
==========

[gab, 2017-05-31 21:19:25]

https://codereview.chromium.org/2912003002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/printing/printers_manager_unittest.cc (right):

https://codereview.chromium.org/2912003002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/printers_manager_unittest.cc:90: : manager_(
On 2017/05/31 19:22:09, skau wrote:
> Can you note that member initialization happens before initializer lists are
> evaluated?  I don't remember what the spec says and it's not obvious that this
> is doing the right thing.

This is not what's happening, members are initialized in their declaration order
regardless of whether value is specified on the member (default or inline member
init) or in the initializer list.

I don't think I want to add a comment on that because this is used all over the
codebase and explaining C++ everywhere this is used seems overkill.

https://codereview.chromium.org/2912003002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/printers_manager_unittest.cc:100: // Must be
first to be alive throughout |profile_|'s lifetime.
On 2017/05/31 19:14:23, Lei Zhang wrote:
> Just say "Must outlive |profile_|" ?

Done.

https://codereview.chromium.org/2912003002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/printers_manager_unittest.cc:103:
TestingProfile profile_;
On 2017/05/31 19:14:23, Lei Zhang wrote:
> Add a comment to say this must outlive |manager_| ?

Done.

[gab, 2017-05-31 21:19:41]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[gab, 2017-05-31 21:19:44]

The CQ bit was checked by gab@chromium.org

[gab, 2017-05-31 21:19:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, rdevlin.cronin@chromium.org, skau@chromium.org
Link to the patchset: https://codereview.chromium.org/2912003002/#ps60001
(title: "review")

[commit-bot: I haz the power, 2017-05-31 21:20:36]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-31 22:26:54]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1496265584271260,
"parent_rev": "d84ddac6a6b61604f0057ccb8046144287f795ec", "commit_rev":
"234cd78265fdc3965626d10b14d37e8b09b39c86"}

[commit-bot: I haz the power, 2017-05-31 22:27:01]

Description was changed from

==========
Replace deprecated base::NonThreadSafe in chrome/browser/extensions in favor of
SequenceChecker.

Note to crash team: This CL is a refactor and has no intended behavior change.

This change was scripted by https://crbug.com/676387#c8.

... and then replaced by DCHECK_CURRENTLY_ON(UI) per request.

Which forced a fix to .../chromeos/printing/printers_manager_unittest.cc
as its TestBrowserThreadBundle was initialized too late for TestingProfile's
constructor (which would now crash in DCHECK_CURRENTLY_ON()), switched to 
using proper member stack ordering instead of trying to do more or less 
that but not quite correctly with heap allocations.

BUG=676387
This CL was uploaded by git cl split.
==========

to

==========
Replace deprecated base::NonThreadSafe in chrome/browser/extensions in favor of
SequenceChecker.

Note to crash team: This CL is a refactor and has no intended behavior change.

This change was scripted by https://crbug.com/676387#c8.

... and then replaced by DCHECK_CURRENTLY_ON(UI) per request.

Which forced a fix to .../chromeos/printing/printers_manager_unittest.cc
as its TestBrowserThreadBundle was initialized too late for TestingProfile's
constructor (which would now crash in DCHECK_CURRENTLY_ON()), switched to 
using proper member stack ordering instead of trying to do more or less 
that but not quite correctly with heap allocations.

BUG=676387
This CL was uploaded by git cl split.

Review-Url: https://codereview.chromium.org/2912003002
Cr-Commit-Position: refs/heads/master@{#476061}
Committed:
https://chromium.googlesource.com/chromium/src/+/234cd78265fdc3965626d10b14d3...
==========

[commit-bot: I haz the power, 2017-05-31 22:27:03]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/234cd78265fdc3965626d10b14d3...