Tracing for NavigationHandle lifetime and state.

Tracing for NavigationHandle lifetime and state.

The goal of this CL is to add trace events to track the lifetime and
state transitions of NavigationHandle. It will be very useful to be able
to debug navigations without debugger.

As part of this support, I'm adding a GetNameForLogging API to
NavigationThrottle, so tracing can attribute throttle checks to the
correct object.

BUG=

Review-Url: https://codereview.chromium.org/2830353003
Cr-Commit-Position: refs/heads/master@{#466667}
Committed: https://chromium.googlesource.com/chromium/src/+/d818557130efc3469ada7c3e03d9f4c308da6226

[nasko, 2017-04-21 23:48:25]

Description was changed from

==========
Tracing for NavigationHandle lifetime and state.

The goal of this CL is to add trace events to track the lifetime and
state transitions of NavigationHandle. It will be very useful to be able
to debug navigations without debugger.

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

to

==========
Tracing for NavigationHandle lifetime and state.

The goal of this CL is to add trace events to track the lifetime and
state transitions of NavigationHandle. It will be very useful to be able
to debug navigations without debugger.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[nasko, 2017-04-21 23:50:02]

Description was changed from

==========
Tracing for NavigationHandle lifetime and state.

The goal of this CL is to add trace events to track the lifetime and
state transitions of NavigationHandle. It will be very useful to be able
to debug navigations without debugger.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Tracing for NavigationHandle lifetime and state.

The goal of this CL is to add trace events to track the lifetime and
state transitions of NavigationHandle. It will be very useful to be able
to debug navigations without debugger.

As part of this support, I'm adding a GetNameForLogging API to
NavigationThrottle, so tracing can attribute throttle checks to the
correct object.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[nasko, 2017-04-21 23:52:48]

nasko@chromium.org changed reviewers:
+ clamy@chromium.org, jam@chromium.org

[nasko, 2017-04-21 23:52:49]

Hey Camille and John,
Can you review this CL for me? It adds tracing support for the lifetime of
NavigationHandle, which will allow us to easily debug issues through tracing
when we can't find local repros.

John - please review the mostly mechanical changes to all NavigationThrottles.
Camille - please review NavigationHandleImpl code.

Thanks in advance!
nasko

[nasko, 2017-04-22 00:03:00]

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

[commit-bot: I haz the power, 2017-04-22 00:03:28]

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-04-22 02:03:21]

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

[commit-bot: I haz the power, 2017-04-22 02:03:22]

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

[Charlie Harrison, 2017-04-22 02:59:38]

On 2017/04/21 23:52:49, nasko wrote:
> Hey Camille and John,
> Can you review this CL for me? It adds tracing support for the lifetime of
> NavigationHandle, which will allow us to easily debug issues through tracing
> when we can't find local repros.
> 
> John - please review the mostly mechanical changes to all NavigationThrottles.
> Camille - please review NavigationHandleImpl code.
> 
> Thanks in advance!
> nasko

Thanks Nasko, I was just about to file a bug for this!

[Charlie Harrison, 2017-04-22 03:13:38]

On 2017/04/22 02:59:38, Charlie Harrison wrote:
> On 2017/04/21 23:52:49, nasko wrote:
> > Hey Camille and John,
> > Can you review this CL for me? It adds tracing support for the lifetime of
> > NavigationHandle, which will allow us to easily debug issues through tracing
> > when we can't find local repros.
> > 
> > John - please review the mostly mechanical changes to all
NavigationThrottles.
> > Camille - please review NavigationHandleImpl code.
> > 
> > Thanks in advance!
> > nasko
> 
> Thanks Nasko, I was just about to file a bug for this!

Fwiw, subresource_filter and page_load_metrics lgtm

[clamy, 2017-04-24 12:30:22]

Thanks for adding the traces! Overall it looks good, and I have one question
below.

https://codereview.chromium.org/2830353003/diff/1/content/public/browser/navi...
File content/public/browser/navigation_throttle.h (right):

https://codereview.chromium.org/2830353003/diff/1/content/public/browser/navi...
content/public/browser/navigation_throttle.h:82: virtual const char*
GetNameForLogging();
Should we make this pure virtual to ensure that implementers of
NavigationThrottles set it? I'm worried that some will forget to define it,
leading to less useful traces. For the record, that's the approach taken by the
ResourceThrottle.

[nasko, 2017-04-24 14:06:39]

https://codereview.chromium.org/2830353003/diff/1/content/public/browser/navi...
File content/public/browser/navigation_throttle.h (right):

https://codereview.chromium.org/2830353003/diff/1/content/public/browser/navi...
content/public/browser/navigation_throttle.h:82: virtual const char*
GetNameForLogging();
On 2017/04/24 12:30:22, clamy wrote:
> Should we make this pure virtual to ensure that implementers of
> NavigationThrottles set it? I'm worried that some will forget to define it,
> leading to less useful traces. For the record, that's the approach taken by
the
> ResourceThrottle.

Yes. Meant to, but missed it. Thanks!

[nasko, 2017-04-24 14:06:59]

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

[commit-bot: I haz the power, 2017-04-24 14:07:38]

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-04-24 14:17:43]

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

[commit-bot: I haz the power, 2017-04-24 14:17:44]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[jam, 2017-04-24 15:22:56]

lgtm

https://codereview.chromium.org/2830353003/diff/20001/content/public/browser/...
File content/public/browser/navigation_throttle.cc (right):

https://codereview.chromium.org/2830353003/diff/20001/content/public/browser/...
content/public/browser/navigation_throttle.cc:28: const char*
NavigationThrottle::GetNameForLogging() {
nit: remove

[nasko, 2017-04-24 15:59:27]

https://codereview.chromium.org/2830353003/diff/20001/content/public/browser/...
File content/public/browser/navigation_throttle.cc (right):

https://codereview.chromium.org/2830353003/diff/20001/content/public/browser/...
content/public/browser/navigation_throttle.cc:28: const char*
NavigationThrottle::GetNameForLogging() {
On 2017/04/24 15:22:55, jam wrote:
> nit: remove

Done.

[nasko, 2017-04-24 15:59:38]

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

[commit-bot: I haz the power, 2017-04-24 16:00:09]

Dry run: CQ is trying da patch.

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

[clamy, 2017-04-24 16:01:04]

Thanks! Lgtm.

[nasko, 2017-04-24 17:11:38]

Description was changed from

==========
Tracing for NavigationHandle lifetime and state.

The goal of this CL is to add trace events to track the lifetime and
state transitions of NavigationHandle. It will be very useful to be able
to debug navigations without debugger.

As part of this support, I'm adding a GetNameForLogging API to
NavigationThrottle, so tracing can attribute throttle checks to the
correct object.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Tracing for NavigationHandle lifetime and state.

The goal of this CL is to add trace events to track the lifetime and
state transitions of NavigationHandle. It will be very useful to be able
to debug navigations without debugger.

As part of this support, I'm adding a GetNameForLogging API to
NavigationThrottle, so tracing can attribute throttle checks to the
correct object.

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

[nasko, 2017-04-24 17:11:46]

The CQ bit was unchecked by nasko@chromium.org

[nasko, 2017-04-24 17:11:56]

The CQ bit was checked by nasko@chromium.org

[nasko, 2017-04-24 17:11:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
csharrison@chromium.org, jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2830353003/#ps40001
(title: "Rebase.")

[commit-bot: I haz the power, 2017-04-24 17:12: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, 2017-04-24 17:17:24]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493053916215150,
"parent_rev": "80f6eac95d5a0b54894b1526ba505308efd5ab23", "commit_rev":
"d818557130efc3469ada7c3e03d9f4c308da6226"}

[commit-bot: I haz the power, 2017-04-24 17:17:33]

Description was changed from

==========
Tracing for NavigationHandle lifetime and state.

The goal of this CL is to add trace events to track the lifetime and
state transitions of NavigationHandle. It will be very useful to be able
to debug navigations without debugger.

As part of this support, I'm adding a GetNameForLogging API to
NavigationThrottle, so tracing can attribute throttle checks to the
correct object.

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

to

==========
Tracing for NavigationHandle lifetime and state.

The goal of this CL is to add trace events to track the lifetime and
state transitions of NavigationHandle. It will be very useful to be able
to debug navigations without debugger.

As part of this support, I'm adding a GetNameForLogging API to
NavigationThrottle, so tracing can attribute throttle checks to the
correct object.

BUG=

Review-Url: https://codereview.chromium.org/2830353003
Cr-Commit-Position: refs/heads/master@{#466667}
Committed:
https://chromium.googlesource.com/chromium/src/+/d818557130efc3469ada7c3e03d9...
==========

[commit-bot: I haz the power, 2017-04-24 17:17:35]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/d818557130efc3469ada7c3e03d9...