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

Replace deprecated base::NonThreadSafe in chrome/browser/safe_browsing 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=nparker@chromium.org

Review-Url: https://codereview.chromium.org/2909123003
Cr-Commit-Position: refs/heads/master@{#476399}
Committed: https://chromium.googlesource.com/chromium/src/+/79ba0231316d391f0636c071f2d745c045d49fbd

[gab, 2017-05-30 02:53:17]

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

[gab, 2017-05-30 02:53:19]

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:53:51]

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-30 04:31:17]

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

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

Dry run: This issue passed the CQ dry run.

[Nathan Parker, 2017-05-30 17:16:48]

https://codereview.chromium.org/2909123003/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/download_feedback.h (right):

https://codereview.chromium.org/2909123003/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/download_feedback.h:23: class DownloadFeedback {
NonThreadSafe was removed, but no SequenceChecker was added here.

[gab, 2017-05-30 21:53:43]

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

[gab, 2017-05-30 21:54:00]

https://codereview.chromium.org/2909123003/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/download_feedback.h (right):

https://codereview.chromium.org/2909123003/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/download_feedback.h:23: class DownloadFeedback {
On 2017/05/30 17:16:47, Nathan Parker wrote:
> NonThreadSafe was removed, but no SequenceChecker was added here.

Forgot to say in CL description but I found a few like this one where the impl
didn't use any NonThreadSafe methods... so I guess it was merely verifying on
destruction... unless the destructor was more crucial than anything, I'll leave
it up to the OWNERS to add missing checks.

[commit-bot: I haz the power, 2017-05-30 21:54:51]

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 00:41:56]

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

[commit-bot: I haz the power, 2017-05-31 00:41:57]

Dry run: This issue passed the CQ dry run.

[gab, 2017-06-01 14:17:15]

On 2017/05/30 21:54:00, gab wrote:
>
https://codereview.chromium.org/2909123003/diff/1/chrome/browser/safe_browsin...
> File chrome/browser/safe_browsing/download_feedback.h (right):
> 
>
https://codereview.chromium.org/2909123003/diff/1/chrome/browser/safe_browsin...
> chrome/browser/safe_browsing/download_feedback.h:23: class DownloadFeedback {
> On 2017/05/30 17:16:47, Nathan Parker wrote:
> > NonThreadSafe was removed, but no SequenceChecker was added here.
> 
> Forgot to say in CL description but I found a few like this one where the impl
> didn't use any NonThreadSafe methods... so I guess it was merely verifying on
> destruction... unless the destructor was more crucial than anything, I'll
leave
> it up to the OWNERS to add missing checks.

friendly ping

[Nathan Parker, 2017-06-01 18:41:44]

https://codereview.chromium.org/2909123003/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/download_feedback.h (right):

https://codereview.chromium.org/2909123003/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/download_feedback.h:23: class DownloadFeedback {
On 2017/05/30 21:54:00, gab wrote:
> On 2017/05/30 17:16:47, Nathan Parker wrote:
> > NonThreadSafe was removed, but no SequenceChecker was added here.
> 
> Forgot to say in CL description but I found a few like this one where the impl
> didn't use any NonThreadSafe methods... so I guess it was merely verifying on
> destruction... unless the destructor was more crucial than anything, I'll
leave
> it up to the OWNERS to add missing checks.


I'm confused -- Why not add DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_)
to the dtor, like you did for the other file?  That way this retains the dtor
checking.

[Nathan Parker, 2017-06-01 18:52:18]

lgtm

Talked off line: 

gab: "basically my script flagged the few classes that had an inline empty
destructor (as adding things to it made clang unhappy and required manually
moving out-of-line), in this case I noticed the NonThreadSafe wasn't otherwise
used. It felt weird to add a SequenceChecker merely used on deletion... If the
class isn't thread-safe the checker should be used on every method.. but I'm not
going to fix this in this mass refactor...

nparker: Ah, ok.  Makes sense.  So this removes the inherit dtor checking, with
the assumption that it's not worth adding it back when that's all that's
checked.

[gab, 2017-06-01 18:56:58]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2017-06-01 18:57:50]

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-06-01 20:00:53]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1496343418926390,
"parent_rev": "1283be78a5b797b02f92acd254cefb7678267226", "commit_rev":
"79ba0231316d391f0636c071f2d745c045d49fbd"}

[commit-bot: I haz the power, 2017-06-01 20:01:04]

Description was changed from

==========
Replace deprecated base::NonThreadSafe in chrome/browser/safe_browsing 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=nparker@chromium.org
==========

to

==========
Replace deprecated base::NonThreadSafe in chrome/browser/safe_browsing 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=nparker@chromium.org

Review-Url: https://codereview.chromium.org/2909123003
Cr-Commit-Position: refs/heads/master@{#476399}
Committed:
https://chromium.googlesource.com/chromium/src/+/79ba0231316d391f0636c071f2d7...
==========

[commit-bot: I haz the power, 2017-06-01 20:01:06]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/79ba0231316d391f0636c071f2d7...