Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2650113005:
Convert NavigationTracingObserver to use the new navigation callbacks. (Closed)
DescriptionConvert NavigationTracingObserver to use the new navigation callbacks.
BUG=682002
Review-Url: https://codereview.chromium.org/2650113005
Cr-Commit-Position: refs/heads/master@{#446359}
Committed: https://chromium.googlesource.com/chromium/src/+/6cbafbc8215e53a5e8689b46c58ef58ea0bc6ae4
Patch Set 1 #Patch Set 2 : use trailing underscore per style guide to avoid name clash #
Total comments: 4
Patch Set 3 : review comments #
Messages
Total messages: 24 (18 generated)
The CQ bit was checked by jam@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jam@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Convert NavigationTracingObserver to use the new navigation callbacks. BUG=682002 ========== to ========== Convert NavigationTracingObserver to use the new navigation callbacks. BUG=682002 ==========
jam@chromium.org changed reviewers: + fmeawad@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Some questions, I will patch it and see if this is the end-to-end expected behavior. https://codereview.chromium.org/2650113005/diff/20001/chrome/browser/tracing/... File chrome/browser/tracing/navigation_tracing.cc (right): https://codereview.chromium.org/2650113005/diff/20001/chrome/browser/tracing/... chrome/browser/tracing/navigation_tracing.cc:117: if (navigation_handle->IsInMainFrame()) { Does this excludes sub-frames? We are only interested in main frame navigations. (the "In" part of InMainFrame is confusing) https://codereview.chromium.org/2650113005/diff/20001/chrome/browser/tracing/... File chrome/browser/tracing/navigation_tracing.h (right): https://codereview.chromium.org/2650113005/diff/20001/chrome/browser/tracing/... chrome/browser/tracing/navigation_tracing.h:34: static content::BackgroundTracingManager::TriggerHandle navigation_handle_; I think we should rename navigation_handle_ to navigation_trigger_handle_ otherwise the code is harder to read as it would assume that all navigation_handle* are of the same type.
https://codereview.chromium.org/2650113005/diff/20001/chrome/browser/tracing/... File chrome/browser/tracing/navigation_tracing.cc (right): https://codereview.chromium.org/2650113005/diff/20001/chrome/browser/tracing/... chrome/browser/tracing/navigation_tracing.cc:117: if (navigation_handle->IsInMainFrame()) { On 2017/01/26 15:44:37, fmeawad wrote: > Does this excludes sub-frames? We are only interested in main frame navigations. > (the "In" part of InMainFrame is confusing) right !render_frame_host->GetParent() is equivalent to IsInMainFrame(). The "in" is referring to "is navigation IN main frame" https://codereview.chromium.org/2650113005/diff/20001/chrome/browser/tracing/... File chrome/browser/tracing/navigation_tracing.h (right): https://codereview.chromium.org/2650113005/diff/20001/chrome/browser/tracing/... chrome/browser/tracing/navigation_tracing.h:34: static content::BackgroundTracingManager::TriggerHandle navigation_handle_; On 2017/01/26 15:44:37, fmeawad wrote: > I think we should rename navigation_handle_ to navigation_trigger_handle_ > otherwise the code is harder to read as it would assume that all > navigation_handle* are of the same type. Done.
The CQ bit was checked by jam@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by jam@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by jam@chromium.org
The CQ bit was checked by jam@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485449914535400,
"parent_rev": "9ba4a643d92a0ec3890aa1c9b7f2be349ad2c928", "commit_rev":
"6cbafbc8215e53a5e8689b46c58ef58ea0bc6ae4"}
Message was sent while issue was closed.
Description was changed from ========== Convert NavigationTracingObserver to use the new navigation callbacks. BUG=682002 ========== to ========== Convert NavigationTracingObserver to use the new navigation callbacks. BUG=682002 Review-Url: https://codereview.chromium.org/2650113005 Cr-Commit-Position: refs/heads/master@{#446359} Committed: https://chromium.googlesource.com/chromium/src/+/6cbafbc8215e53a5e8689b46c58e... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/6cbafbc8215e53a5e8689b46c58e... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
