Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2759523003:
Add trace event in NavigationControllerImpl::GoToIndex. (Closed)
DescriptionAdd trace event in NavigationControllerImpl::GoToIndex.
Currently, we have added trace events in NavigationControllerImpl::back/forward
functions. We just found out that both of them call NavigationControllerImpl::GoToIndex.
Therefore, only adding one trace event in NavigationControllerImpl::GoToIndex will
cover both of NavigationControllerImpl::back and forward calls.
BUG=702739
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2759523003
Cr-Commit-Position: refs/heads/master@{#458558}
Committed: https://chromium.googlesource.com/chromium/src/+/30574a61c2972095e8dff5a0dc0f7c66e1fcf092
Patch Set 1 #
Total comments: 2
Patch Set 2 : addressed comments #
Total comments: 1
Patch Set 3 : sync #Patch Set 4 : sync #Messages
Total messages: 23 (13 generated)
Description was changed from ========== Add trace event in NavigationControllerImpl::GoToIndex. Currently, we have added trace events in NavigationControllerImpl::back/forward functions. We just found out that both of them call NavigationControllerImpl::GoToIndex. Therefore, only adding one trace event in NavigationControllerImpl::GoToIndex will cover both of NavigationControllerImpl::back and forward calls. BUG=702739 ========== to ========== Add trace event in NavigationControllerImpl::GoToIndex. Currently, we have added trace events in NavigationControllerImpl::back/forward functions. We just found out that both of them call NavigationControllerImpl::GoToIndex. Therefore, only adding one trace event in NavigationControllerImpl::GoToIndex will cover both of NavigationControllerImpl::back and forward calls. BUG=702739 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Add trace event in NavigationControllerImpl::GoToIndex. Currently, we have added trace events in NavigationControllerImpl::back/forward functions. We just found out that both of them call NavigationControllerImpl::GoToIndex. Therefore, only adding one trace event in NavigationControllerImpl::GoToIndex will cover both of NavigationControllerImpl::back and forward calls. BUG=702739 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Add trace event in NavigationControllerImpl::GoToIndex. Currently, we have added trace events in NavigationControllerImpl::back/forward functions. We just found out that both of them call NavigationControllerImpl::GoToIndex. Therefore, only adding one trace event in NavigationControllerImpl::GoToIndex will cover both of NavigationControllerImpl::back and forward calls. BUG=702739 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
sunjian@chromium.org changed reviewers: + nasko@chromium.org, tdresser@chromium.org
Please take a look!
LGTM, thanks. https://codereview.chromium.org/2759523003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2759523003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl.cc:599: TRACE_EVENT0("browser,navigation", Add ,benchmark We haven't been consistent about this, but it sounds like we'll likely want to start adding the benchmark category for metrics used in benchmarking.
https://codereview.chromium.org/2759523003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2759523003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl.cc:599: TRACE_EVENT0("browser,navigation", On 2017/03/17 19:24:42, tdresser wrote: > Add ,benchmark > > We haven't been consistent about this, but it sounds like we'll likely want to > start adding the benchmark category for metrics used in benchmarking. > Done.
LGTM https://codereview.chromium.org/2759523003/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2759523003/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:600: "NavigationControllerImpl::GoToIndex"); It would've been nice if this was done in the previous CL as commented on.
The CQ bit was checked by sunjian@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdresser@chromium.org Link to the patchset: https://codereview.chromium.org/2759523003/#ps20001 (title: "addressed comments")
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
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...)
The CQ bit was checked by sunjian@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdresser@chromium.org, nasko@chromium.org Link to the patchset: https://codereview.chromium.org/2759523003/#ps40001 (title: "sync")
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
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by sunjian@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdresser@chromium.org, nasko@chromium.org Link to the patchset: https://codereview.chromium.org/2759523003/#ps60001 (title: "sync")
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": 60001, "attempt_start_ts": 1490122872449150,
"parent_rev": "a713ef602b71c72b903539abf21e6f9f10d21915", "commit_rev":
"30574a61c2972095e8dff5a0dc0f7c66e1fcf092"}
Message was sent while issue was closed.
Description was changed from ========== Add trace event in NavigationControllerImpl::GoToIndex. Currently, we have added trace events in NavigationControllerImpl::back/forward functions. We just found out that both of them call NavigationControllerImpl::GoToIndex. Therefore, only adding one trace event in NavigationControllerImpl::GoToIndex will cover both of NavigationControllerImpl::back and forward calls. BUG=702739 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Add trace event in NavigationControllerImpl::GoToIndex. Currently, we have added trace events in NavigationControllerImpl::back/forward functions. We just found out that both of them call NavigationControllerImpl::GoToIndex. Therefore, only adding one trace event in NavigationControllerImpl::GoToIndex will cover both of NavigationControllerImpl::back and forward calls. BUG=702739 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2759523003 Cr-Commit-Position: refs/heads/master@{#458558} Committed: https://chromium.googlesource.com/chromium/src/+/30574a61c2972095e8dff5a0dc0f... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/30574a61c2972095e8dff5a0dc0f... |
