Add thread checking to RunLoop, deprecate MessageLoopRunner.

Add thread checker to base::RunLoop.

The purpose is to make sure that after switching users of
MessageLoopRunner to RunLoop we won't lose any correctness checks.

Also add a comment that encourages to use RunLoop where possible, and
rename GetQuitTaskForRunLoop to GetDeferredQuitTaskForRunLoop to convey
is purpose clearly.

BUG=668707
Committed: https://crrev.com/dd10720676efff002490f702118e052dcd210f18
Cr-Commit-Position: refs/heads/master@{#435924}

[Alexander Semashko, 2016-11-29 19:33:10]

The CQ bit was checked by ahest@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2016-11-29 19:33:32]

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, 2016-11-29 19:58:24]

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

[commit-bot: I haz the power, 2016-11-29 19:58:25]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Alexander Semashko, 2016-11-29 21:55:06]

The CQ bit was checked by ahest@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2016-11-29 21:56:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Alexander Semashko, 2016-11-29 22:23:08]

Description was changed from

==========
Add thread checking in RL, deprecate MLR.

BUG=668707
==========

to

==========
Add thread checker to base::RunLoop.

The purpose is to make sure that after switching users of MessageLoopRunner to
RunLoop we won't lose any correctness checks.

Also add a comment that encourages to use RunLoop where possible, and rename
GetQuitTaskForRunLoop to GetDeferredQuitTaskForRunLoop to convey is purpose
clearly.

BUG=668707
==========

[Alexander Semashko, 2016-11-29 22:38:19]

Description was changed from

==========
Add thread checker to base::RunLoop.

The purpose is to make sure that after switching users of MessageLoopRunner to
RunLoop we won't lose any correctness checks.

Also add a comment that encourages to use RunLoop where possible, and rename
GetQuitTaskForRunLoop to GetDeferredQuitTaskForRunLoop to convey is purpose
clearly.

BUG=668707
==========

to

==========
Add thread checker to base::RunLoop.

The purpose is to make sure that after switching users of
MessageLoopRunner to RunLoop we won't lose any correctness checks.

Also add a comment that encourages to use RunLoop where possible, and
rename GetQuitTaskForRunLoop to GetDeferredQuitTaskForRunLoop to convey
is purpose clearly.

BUG=668707
==========

[commit-bot: I haz the power, 2016-11-29 23:20:27]

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

[commit-bot: I haz the power, 2016-11-29 23:20:28]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Alexander Semashko, 2016-11-30 10:24:57]

The CQ bit was checked by ahest@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 10:25:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Alexander Semashko, 2016-11-30 10:25:37]

ahest@yandex-team.ru changed reviewers:
+ avi@chromium.org, finnur@chromium.org, thakis@chromium.org

[Alexander Semashko, 2016-11-30 10:25:38]

Please take a look.

[commit-bot: I haz the power, 2016-11-30 11:28:49]

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

[commit-bot: I haz the power, 2016-11-30 11:28:50]

Dry run: This issue passed the CQ dry run.

[Finnur, 2016-11-30 11:32:07]

finnur@chromium.org changed reviewers:
+ asargent@chromium.org
- finnur@chromium.org

[Finnur, 2016-11-30 11:32:46]

When you have multiple reviewers assigned, it helps in general if you state what
you expect from each reviewer. 

For this CL, specifically, I'm not sure what you want my input for. If it is an
owners check, then the only file I'm owner for is the result_catcher.cc test
file (for historical reasons). But it's probably better I defer to someone
actually working on the Extensions platform (e.g. asargent).

[Alexander Semashko, 2016-11-30 11:42:43]

On 2016/11/30 11:32:46, Finnur wrote:
> When you have multiple reviewers assigned, it helps in general if you state
what
> you expect from each reviewer. 
> 
> For this CL, specifically, I'm not sure what you want my input for. If it is
an
> owners check, then the only file I'm owner for is the result_catcher.cc test
> file (for historical reasons). But it's probably better I defer to someone
> actually working on the Extensions platform (e.g. asargent).

Yeah, my bad.

Nico, please review changes in base/ and chrome/
Avi, please review changes in content/
Antony, please review changes in extensions/

[Avi (use Gerrit), 2016-11-30 17:19:37]

lgtm

[asargent_no_longer_on_chrome, 2016-11-30 18:28:29]

extensions lgtm

[Nico, 2016-12-02 01:10:35]

lgtm, thanks!

https://codereview.chromium.org/2537893002/diff/20002/base/run_loop.h
File base/run_loop.h (right):

https://codereview.chromium.org/2537893002/diff/20002/base/run_loop.h#newcode109
base/run_loop.h:109: base::ThreadChecker thread_checker_;
should this be in an #if DCHECK_IS_ON() block?

[Alexander Semashko, 2016-12-02 10:47:34]

https://codereview.chromium.org/2537893002/diff/20002/base/run_loop.h
File base/run_loop.h (right):

https://codereview.chromium.org/2537893002/diff/20002/base/run_loop.h#newcode109
base/run_loop.h:109: base::ThreadChecker thread_checker_;
On 2016/12/02 01:10:35, Nico wrote:
> should this be in an #if DCHECK_IS_ON() block?

It won't compile this way: code inside DCHECK's still need to be correct.
Also, it makes little of a difference, since ThreadChecker is an empty class
when DCHECKs are off.

But there is a difference, because even empty objects have non-zero size. So
ThreadChecker can add 8 bytes (4 in 32-bit builds) to the size of objects that
use it, including ones in production code. This is not the case with
NonThreadSafe which is used as a base class instead of a member. But comments in
thread_checker.h say that ThreadChecker should be used wherever possible.
Why is it so? Shouldn't the use of NonThreadSafe be encouraged because of that?

I think this broader question should be resolved first. Then, if there is a
change in policy, we can update RunLoop as well as other classes. For now, imo,
it does not need to be special-cased, because it is definitely not used a lot in
production.

[Nico, 2016-12-02 11:22:10]

Sounds good, please land :-)

Maybe we can try to figure out why ThreadChecker is currently recommended
over the base class, which would allow the empty base class optimization,
at some later time.

On Dec 2, 2016 5:47 AM, <ahest@yandex-team.ru> wrote:

>
> https://codereview.chromium.org/2537893002/diff/20002/base/run_loop.h
> File base/run_loop.h (right):
>
> https://codereview.chromium.org/2537893002/diff/20002/
> base/run_loop.h#newcode109
> base/run_loop.h:109: base::ThreadChecker thread_checker_;
> On 2016/12/02 01:10:35, Nico wrote:
> > should this be in an #if DCHECK_IS_ON() block?
>
> It won't compile this way: code inside DCHECK's still need to be
> correct.
> Also, it makes little of a difference, since ThreadChecker is an empty
> class when DCHECKs are off.
>
> But there is a difference, because even empty objects have non-zero
> size. So ThreadChecker can add 8 bytes (4 in 32-bit builds) to the size
> of objects that use it, including ones in production code. This is not
> the case with NonThreadSafe which is used as a base class instead of a
> member. But comments in thread_checker.h say that ThreadChecker should
> be used wherever possible.
> Why is it so? Shouldn't the use of NonThreadSafe be encouraged because
> of that?
>
> I think this broader question should be resolved first. Then, if there
> is a change in policy, we can update RunLoop as well as other classes.
> For now, imo, it does not need to be special-cased, because it is
> definitely not used a lot in production.
>
> https://codereview.chromium.org/2537893002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Alexander Semashko, 2016-12-02 11:24:43]

The CQ bit was checked by ahest@yandex-team.ru

[commit-bot: I haz the power, 2016-12-02 11:25:07]

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, 2016-12-02 12:32:37]

CQ is committing da patch.

Bot data: {"patchset_id": 20002, "attempt_start_ts": 1480677883043630,
"parent_rev": "c89fdf9bf89407560852e44fd3893c2721c01589", "commit_rev":
"53dc1e30a6cfdc0542d834bfbfbd2135efa446c0"}

[commit-bot: I haz the power, 2016-12-02 12:33:02]

Description was changed from

==========
Add thread checker to base::RunLoop.

The purpose is to make sure that after switching users of
MessageLoopRunner to RunLoop we won't lose any correctness checks.

Also add a comment that encourages to use RunLoop where possible, and
rename GetQuitTaskForRunLoop to GetDeferredQuitTaskForRunLoop to convey
is purpose clearly.

BUG=668707
==========

to

==========
Add thread checker to base::RunLoop.

The purpose is to make sure that after switching users of
MessageLoopRunner to RunLoop we won't lose any correctness checks.

Also add a comment that encourages to use RunLoop where possible, and
rename GetQuitTaskForRunLoop to GetDeferredQuitTaskForRunLoop to convey
is purpose clearly.

BUG=668707
==========

[commit-bot: I haz the power, 2016-12-02 12:33:04]

Committed patchset #4 (id:20002)

[commit-bot: I haz the power, 2016-12-02 12:34:27]

Description was changed from

==========
Add thread checker to base::RunLoop.

The purpose is to make sure that after switching users of
MessageLoopRunner to RunLoop we won't lose any correctness checks.

Also add a comment that encourages to use RunLoop where possible, and
rename GetQuitTaskForRunLoop to GetDeferredQuitTaskForRunLoop to convey
is purpose clearly.

BUG=668707
==========

to

==========
Add thread checker to base::RunLoop.

The purpose is to make sure that after switching users of
MessageLoopRunner to RunLoop we won't lose any correctness checks.

Also add a comment that encourages to use RunLoop where possible, and
rename GetQuitTaskForRunLoop to GetDeferredQuitTaskForRunLoop to convey
is purpose clearly.

BUG=668707

Committed: https://crrev.com/dd10720676efff002490f702118e052dcd210f18
Cr-Commit-Position: refs/heads/master@{#435924}
==========

[commit-bot: I haz the power, 2016-12-02 12:34:29]

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

[Ken Rockot(use gerrit already), 2016-12-02 21:30:48]

A revert of this CL (patchset #4 id:20002) has been created in
https://codereview.chromium.org/2548883002/ by rockot@chromium.org.

The reason for reverting is: Speculative revert in an effort to pinpoint the
cause of http://crbug.com/670844 - will reland if the flake doesn't clear.