[chrome/browser/renderer_host] favor DCHECK_CURRENTLY_ON for better logs

favor DCHECK_CURRENTLY_ON for better logs in chrome/browser/renderer_host

BUG=466848
Committed: https://crrev.com/c747af5900eb7adfcae2a4038b0fc52c413a0a1d
Cr-Commit-Position: refs/heads/master@{#329918}

[anujsharma, 2015-04-27 10:23:50]

anujk.sharma@samsung.com changed reviewers:
+ thakis@chromium.org

[anujsharma, 2015-04-27 10:23:51]

PTAL

[anujsharma, 2015-04-29 07:39:14]

Could you please give a thumbs up for this CL.

Thank you!!

[anujsharma, 2015-05-04 07:38:29]

anujk.sharma@samsung.com changed reviewers:
+ kalman@chromium.org, raymes@chromium.org

[anujsharma, 2015-05-04 07:47:01]

Nico looks busy.

@Kalman: could you please review chrome/browser/renderer_host cahnges

@Raymes: could you please review chrome/browser/renderer_host/pepper changes

Thank you!!

[not at google - send to devlin, 2015-05-04 13:52:58]

On 2015/05/04 07:47:01, anujsharma wrote:
> Nico looks busy.
> 
> @Kalman: could you please review chrome/browser/renderer_host cahnges

I am not an owner of renderer_host.

> 
> @Raymes: could you please review chrome/browser/renderer_host/pepper changes
> 
> Thank you!!

[Nico, 2015-05-04 16:00:32]

I'm not super excited about this since DCHECK_CURRENTLY_ON does a bunch of
string concatenation unconditionally, even if the check doesn't fire. True, it's
only in dcheck-enabled builds, but for that reason I'd do this conversion on
demand if someone is debugging something, and not eagerly.

[raymes, 2015-05-05 00:39:23]

lgtm for chrome/browser/renderer_host/pepper/

[anujsharma, 2015-05-05 04:44:02]

anujk.sharma@samsung.com changed reviewers:
- kalman@chromium.org

[anujsharma, 2015-05-05 05:15:26]

On 2015/05/04 16:00:32, Nico wrote:
> I'm not super excited about this since DCHECK_CURRENTLY_ON does a bunch of
> string concatenation unconditionally, even if the check doesn't fire. True,
it's
> only in dcheck-enabled builds, but for that reason I'd do this conversion on
> demand if someone is debugging something, and not eagerly.

Thanks nico for your time. I am dropping the changes of
chrome/browser/renderer_host.

[raymes, 2015-05-05 05:18:06]

Please don't land this until Nico has had time to air his concerns. I am fine
with this change but it's unclear whether Nico was commenting specifically about
chrome/browser/renderer_host or about this change more broadly.

Nico: do you have broad concerns about this? If so it may be worth commenting on
the bug linked. There has already been a substantial number of these changes
that have landed.

[anujsharma, 2015-05-05 05:18:08]

On 2015/05/05 00:39:23, raymes wrote:
> lgtm for chrome/browser/renderer_host/pepper/

Thanks rayms for your lgtm.
I am proceeding to land only with chrome/browser/renderer_host/pepper/ changes.

[anujsharma, 2015-05-05 05:18:13]

The CQ bit was checked by anujk.sharma@samsung.com

[anujsharma, 2015-05-05 05:18:14]

The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org
Link to the patchset: https://codereview.chromium.org/1100293004/#ps20001
(title: "Removing changes")

[raymes, 2015-05-05 05:18:27]

The CQ bit was unchecked by raymes@chromium.org

[anujsharma, 2015-05-05 05:20:56]

On 2015/05/05 05:18:06, raymes wrote:
> Please don't land this until Nico has had time to air his concerns. I am fine
> with this change but it's unclear whether Nico was commenting specifically
about
> chrome/browser/renderer_host or about this change more broadly.
> 
> Nico: do you have broad concerns about this? If so it may be worth commenting
on
> the bug linked. There has already been a substantial number of these changes
> that have landed.

Thanks raymes for raising this concern.

I will wait for Nico's go-ahead signal.

[Mostyn Bramley-Moore, 2015-05-05 15:15:17]

CC'ed Jeffrey Yasskin, the author of DCHECK_CURRENTLY_ON.

[Nico, 2015-05-06 00:48:45]

Said the same thing on the bug.

On Mon, May 4, 2015 at 9:00 AM, <thakis@chromium.org> wrote:

> I'm not super excited about this since DCHECK_CURRENTLY_ON does a bunch of
> string concatenation unconditionally, even if the check doesn't fire.
> True, it's
> only in dcheck-enabled builds, but for that reason I'd do this conversion
> on
> demand if someone is debugging something, and not eagerly.
>
> https://codereview.chromium.org/1100293004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Jeffrey Yasskin, 2015-05-14 15:10:00]

The LAZY_STREAM() macro is supposed to make DCHECKs in general avoid evaluating
their arguments when the condition is false, and DCHECK_CURRENTLY_ON uses
DCHECK. Are you seeing that not happen?

On 2015/05/06 00:48:45, Nico wrote:
> Said the same thing on the bug.
> 
> On Mon, May 4, 2015 at 9:00 AM, <mailto:thakis@chromium.org> wrote:
> 
> > I'm not super excited about this since DCHECK_CURRENTLY_ON does a bunch of
> > string concatenation unconditionally, even if the check doesn't fire.
> > True, it's
> > only in dcheck-enabled builds, but for that reason I'd do this conversion
> > on
> > demand if someone is debugging something, and not eagerly.
> >
> > https://codereview.chromium.org/1100293004/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Nico, 2015-05-14 18:24:47]

On 2015/05/14 15:10:00, Jeffrey Yasskin wrote:
> The LAZY_STREAM() macro is supposed to make DCHECKs in general avoid
evaluating
> their arguments when the condition is false, and DCHECK_CURRENTLY_ON uses
> DCHECK. Are you seeing that not happen?

TIL. lgtm then, thanks.

[anujsharma, 2015-05-14 19:25:06]

On 2015/05/14 18:24:47, Nico wrote:
> On 2015/05/14 15:10:00, Jeffrey Yasskin wrote:
> > The LAZY_STREAM() macro is supposed to make DCHECKs in general avoid
> evaluating
> > their arguments when the condition is false, and DCHECK_CURRENTLY_ON uses
> > DCHECK. Are you seeing that not happen?
> 
> TIL. lgtm then, thanks.

Thanks Nico for lgtm.

[anujsharma, 2015-05-14 19:25:22]

The CQ bit was checked by anujk.sharma@samsung.com

[commit-bot: I haz the power, 2015-05-14 19:28:04]

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

[commit-bot: I haz the power, 2015-05-14 20:25:26]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-05-14 20:26:23]

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