Reland [Media Router] Custom Controls 3 - add plumbing through MRUI and MRWebUIMessageHandler

Reland [Media Router] Custom Controls 3 - add plumbing through MRUI and
MRWebUIMessageHandler

This is a reland of http://crrev/2731033002/. There were DCHECKs with side
effects that should have been CHECKs.

BUG=684638
Review-Url: https://codereview.chromium.org/2859953002
Cr-Commit-Position: refs/heads/master@{#469492}
Committed: https://chromium.googlesource.com/chromium/src/+/8d142c1674a4233fc63b367b1bc100ac00eaa4f7

[takumif, 2017-05-03 20:32:29]

Description was changed from

==========
Reland


.

BUG=
==========

to

==========
Reland [Media Router] Custom Controls 4 - Implement details view WebUI

This is a reland of http://crrev/2731033002/. There were DCHECKs with side
effects that should have been CHECKs.

BUG=684638
==========

[takumif, 2017-05-03 20:35:09]

Description was changed from

==========
Reland [Media Router] Custom Controls 4 - Implement details view WebUI

This is a reland of http://crrev/2731033002/. There were DCHECKs with side
effects that should have been CHECKs.

BUG=684638
==========

to

==========
Reland [Media Router] Custom Controls 4 - Implement details view WebUI

This is a reland of http://crrev/2731033002/. There were DCHECKs with side
effects that should have been CHECKs.

BUG=684638
==========

[takumif, 2017-05-03 20:35:54]

Description was changed from

==========
Reland [Media Router] Custom Controls 4 - Implement details view WebUI

This is a reland of http://crrev/2731033002/. There were DCHECKs with side
effects that should have been CHECKs.

BUG=684638
==========

to

==========
Reland [Media Router] Custom Controls 3 - add plumbing through MRUI and
MRWebUIMessageHandler

This is a reland of http://crrev/2731033002/. There were DCHECKs with side
effects that should have been CHECKs.

BUG=684638
==========

[takumif, 2017-05-03 20:37:42]

takumif@chromium.org changed reviewers:
+ imcheng@chromium.org

[takumif, 2017-05-03 20:37:43]

Please take a look, thanks!

[imcheng, 2017-05-03 20:58:45]

lgtm, thanks. It is easy to be bitten by DCHECK/CHECK when they come with side
effects, so I generally prefer the expression in DCHECK/CHECK to be as simple as
possible.

[takumif, 2017-05-03 21:03:04]

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

[commit-bot: I haz the power, 2017-05-03 21:03:48]

Dry run: CQ is trying da patch.

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

[mark a. foltz, 2017-05-03 21:32:45]

On 2017/05/03 at 20:58:45, imcheng wrote:
> lgtm, thanks. It is easy to be bitten by DCHECK/CHECK when they come with side
effects, so I generally prefer the expression in DCHECK/CHECK to be as simple as
possible.

You shouldn't blindly change DCHECK() to CHECK() to capture side effects. 
CHECK() are generally for conditions where it's insecure to continue and it's
worth taking down the browser (esp. when handling untrusted input).  To maintain
the DCHECK()s you can rewrite to separate the side effect and the test.

[commit-bot: I haz the power, 2017-05-03 21:54:07]

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

[commit-bot: I haz the power, 2017-05-03 21:54:07]

Dry run: This issue passed the CQ dry run.

[takumif, 2017-05-03 22:03:28]

On 2017/05/03 21:32:45, mark a. foltz wrote:
> On 2017/05/03 at 20:58:45, imcheng wrote:
> > lgtm, thanks. It is easy to be bitten by DCHECK/CHECK when they come with
side
> effects, so I generally prefer the expression in DCHECK/CHECK to be as simple
as
> possible.
> 
> You shouldn't blindly change DCHECK() to CHECK() to capture side effects. 
> CHECK() are generally for conditions where it's insecure to continue and it's
> worth taking down the browser (esp. when handling untrusted input).  To
maintain
> the DCHECK()s you can rewrite to separate the side effect and the test.

Acknowledged. In this case the tests can't continue without passing the
assertions, so I think CHECKs are appropriate.

[imcheng, 2017-05-03 22:55:44]

On 2017/05/03 22:03:28, takumif wrote:
> On 2017/05/03 21:32:45, mark a. foltz wrote:
> > On 2017/05/03 at 20:58:45, imcheng wrote:
> > > lgtm, thanks. It is easy to be bitten by DCHECK/CHECK when they come with
> side
> > effects, so I generally prefer the expression in DCHECK/CHECK to be as
simple
> as
> > possible.
> > 
> > You shouldn't blindly change DCHECK() to CHECK() to capture side effects. 
> > CHECK() are generally for conditions where it's insecure to continue and
it's
> > worth taking down the browser (esp. when handling untrusted input).  To
> maintain
> > the DCHECK()s you can rewrite to separate the side effect and the test.
> 
> Acknowledged. In this case the tests can't continue without passing the
> assertions, so I think CHECKs are appropriate.

Since this is test code, you could try ASSERT_TRUE, though last time I checked,
ASSERT_* did not work inside a function with non-void return type. So to use it,
you'll have to change the method signature as well.

[takumif, 2017-05-03 23:05:43]

On 2017/05/03 22:55:44, imcheng wrote:
> On 2017/05/03 22:03:28, takumif wrote:
> > On 2017/05/03 21:32:45, mark a. foltz wrote:
> > > On 2017/05/03 at 20:58:45, imcheng wrote:
> > > > lgtm, thanks. It is easy to be bitten by DCHECK/CHECK when they come
with
> > side
> > > effects, so I generally prefer the expression in DCHECK/CHECK to be as
> simple
> > as
> > > possible.
> > > 
> > > You shouldn't blindly change DCHECK() to CHECK() to capture side effects. 
> > > CHECK() are generally for conditions where it's insecure to continue and
> it's
> > > worth taking down the browser (esp. when handling untrusted input).  To
> > maintain
> > > the DCHECK()s you can rewrite to separate the side effect and the test.
> > 
> > Acknowledged. In this case the tests can't continue without passing the
> > assertions, so I think CHECKs are appropriate.
> 
> Since this is test code, you could try ASSERT_TRUE, though last time I
checked,
> ASSERT_* did not work inside a function with non-void return type. So to use
it,
> you'll have to change the method signature as well.

Right, that was the reason I originally used DCHECK instead of ASSERT_TRUE.

[takumif, 2017-05-04 20:47:18]

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

[commit-bot: I haz the power, 2017-05-04 20:47:44]

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-04 20:53:47]

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

[commit-bot: I haz the power, 2017-05-04 20:53:48]

Dry run: This issue passed the CQ dry run.

[takumif, 2017-05-04 21:30:26]

The CQ bit was checked by takumif@chromium.org

[commit-bot: I haz the power, 2017-05-04 21:31:08]

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-04 22:23:37]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493933426773080,
"parent_rev": "6abba8e52ba13f660d2350b08f3d1829ce282edd", "commit_rev":
"8d142c1674a4233fc63b367b1bc100ac00eaa4f7"}

[commit-bot: I haz the power, 2017-05-04 22:23:50]

Description was changed from

==========
Reland [Media Router] Custom Controls 3 - add plumbing through MRUI and
MRWebUIMessageHandler

This is a reland of http://crrev/2731033002/. There were DCHECKs with side
effects that should have been CHECKs.

BUG=684638
==========

to

==========
Reland [Media Router] Custom Controls 3 - add plumbing through MRUI and
MRWebUIMessageHandler

This is a reland of http://crrev/2731033002/. There were DCHECKs with side
effects that should have been CHECKs.

BUG=684638

Review-Url: https://codereview.chromium.org/2859953002
Cr-Commit-Position: refs/heads/master@{#469492}
Committed:
https://chromium.googlesource.com/chromium/src/+/8d142c1674a4233fc63b367b1bc1...
==========

[commit-bot: I haz the power, 2017-05-04 22:23:51]

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