Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2748393003:
Add trace events in NavigationControllerImpl::back/forward(...). (Closed)
DescriptionAdd trace events in NavigationControllerImpl::back/forward(...).
Currently, there is a small set of user input events that will trigger
SPA (Single Page App) navigations. We just found out that pressing
back/forward buttons can also trigger SPA navigations. When that happens,
there is no input handling in the browser process, which is what we use
to capture as navigation start event when user-input-triggered SPA navigations
happened. Therefore, we need to add trace events in
NavigationControllerImpl::back/forward(...) function calls and capture them
as navigation start events when SPA navigations happen.
BUG=702024
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2748393003
Cr-Commit-Position: refs/heads/master@{#457624}
Committed: https://chromium.googlesource.com/chromium/src/+/666f71455a4f01ee74a1dee3bab82ed62813c255
Patch Set 1 #
Total comments: 2
Patch Set 2 : addressed comments #
Total comments: 2
Messages
Total messages: 21 (10 generated)
Description was changed from ========== Add trace events in NavigationControllerImpl::back/forward(...). Currently, there are a small set of user input events that will trigger SPA (Single Page Navigation) navigations. We just found out that pressing back/forward buttons can also trigger SPA navigations. When that happens, there is no input handling in the browser process, which is what we use to capture as navigation start event when user-input-triggered SPA navigations happened. Therefore, we need to add trace events in NavigationControllerImpl::back/forward(...) function calls and capture them as navigation start events when SPA navigations happen. BUG=702024 ========== to ========== Add trace events in NavigationControllerImpl::back/forward(...). Currently, there are a small set of user input events that will trigger SPA (Single Page Navigation) navigations. We just found out that pressing back/forward buttons can also trigger SPA navigations. When that happens, there is no input handling in the browser process, which is what we use to capture as navigation start event when user-input-triggered SPA navigations happened. Therefore, we need to add trace events in NavigationControllerImpl::back/forward(...) function calls and capture them as navigation start events when SPA navigations happen. BUG=702024 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Add trace events in NavigationControllerImpl::back/forward(...). Currently, there are a small set of user input events that will trigger SPA (Single Page Navigation) navigations. We just found out that pressing back/forward buttons can also trigger SPA navigations. When that happens, there is no input handling in the browser process, which is what we use to capture as navigation start event when user-input-triggered SPA navigations happened. Therefore, we need to add trace events in NavigationControllerImpl::back/forward(...) function calls and capture them as navigation start events when SPA navigations happen. BUG=702024 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Add trace events in NavigationControllerImpl::back/forward(...). Currently, there is a small set of user input events that will trigger SPA (Single Page Navigation) navigations. We just found out that pressing back/forward buttons can also trigger SPA navigations. When that happens, there is no input handling in the browser process, which is what we use to capture as navigation start event when user-input-triggered SPA navigations happened. Therefore, we need to add trace events in NavigationControllerImpl::back/forward(...) function calls and capture them as navigation start events when SPA navigations happen. BUG=702024 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Add trace events in NavigationControllerImpl::back/forward(...). Currently, there is a small set of user input events that will trigger SPA (Single Page Navigation) navigations. We just found out that pressing back/forward buttons can also trigger SPA navigations. When that happens, there is no input handling in the browser process, which is what we use to capture as navigation start event when user-input-triggered SPA navigations happened. Therefore, we need to add trace events in NavigationControllerImpl::back/forward(...) function calls and capture them as navigation start events when SPA navigations happen. BUG=702024 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Add trace events in NavigationControllerImpl::back/forward(...). Currently, there is a small set of user input events that will trigger SPA (Single Page App) navigations. We just found out that pressing back/forward buttons can also trigger SPA navigations. When that happens, there is no input handling in the browser process, which is what we use to capture as navigation start event when user-input-triggered SPA navigations happened. Therefore, we need to add trace events in NavigationControllerImpl::back/forward(...) function calls and capture them as navigation start events when SPA navigations happen. BUG=702024 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
sunjian@chromium.org changed reviewers: + tdresser@chromium.org
LGTM https://codereview.chromium.org/2748393003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2748393003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl.cc:589: // Call GoToIndex rather than GoToOffset to get the NOTREACHED() check. Move the comment below the trace event, it's referring to the GoToIndex call. (And below)
sunjian@chromium.org changed reviewers: + nasko@chromium.org
Hey Nasko, can you take a pass at the change please? https://codereview.chromium.org/2748393003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2748393003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl.cc:589: // Call GoToIndex rather than GoToOffset to get the NOTREACHED() check. On 2017/03/16 12:24:38, tdresser wrote: > Move the comment below the trace event, it's referring to the GoToIndex call. > (And below) Acknowledged.
LGTM
https://codereview.chromium.org/2748393003/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2748393003/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:629: void NavigationControllerImpl::GoToOffset(int offset) { Might be useful to add tracing to this one and GoToIndex, to cover other cases you might not have seen yet.
On 2017/03/16 20:34:44, nasko (slow) wrote: > https://codereview.chromium.org/2748393003/diff/20001/content/browser/frame_h... > File content/browser/frame_host/navigation_controller_impl.cc (right): > > https://codereview.chromium.org/2748393003/diff/20001/content/browser/frame_h... > content/browser/frame_host/navigation_controller_impl.cc:629: void > NavigationControllerImpl::GoToOffset(int offset) { > Might be useful to add tracing to this one and GoToIndex, to cover other cases > you might not have seen yet. So if back/forward buttons are pressed, GoBack() and GoForward() are definitely gonna be called right? If that's the case, we may only need these two for now. Can you say more about the other cases that you referred please?
On 2017/03/16 21:29:59, sunjian wrote: > On 2017/03/16 20:34:44, nasko (slow) wrote: > > > https://codereview.chromium.org/2748393003/diff/20001/content/browser/frame_h... > > File content/browser/frame_host/navigation_controller_impl.cc (right): > > > > > https://codereview.chromium.org/2748393003/diff/20001/content/browser/frame_h... > > content/browser/frame_host/navigation_controller_impl.cc:629: void > > NavigationControllerImpl::GoToOffset(int offset) { > > Might be useful to add tracing to this one and GoToIndex, to cover other cases > > you might not have seen yet. > > So if back/forward buttons are pressed, GoBack() and GoForward() are definitely > gonna be called right? > If that's the case, we may only need these two for now. Can you say more about > the other cases that you > referred please? If you long press the back button, you get a list of all navigations and you can pick specific one. If it is further back in history than your previous one, I would expect to see one of these two methods to be called, not just the GoBack/GoForward. https://cs.chromium.org/chromium/src/chrome/browser/ui/browser_commands.cc?rc...
On 2017/03/16 21:52:38, nasko (slow) wrote: > On 2017/03/16 21:29:59, sunjian wrote: > > On 2017/03/16 20:34:44, nasko (slow) wrote: > > > > > > https://codereview.chromium.org/2748393003/diff/20001/content/browser/frame_h... > > > File content/browser/frame_host/navigation_controller_impl.cc (right): > > > > > > > > > https://codereview.chromium.org/2748393003/diff/20001/content/browser/frame_h... > > > content/browser/frame_host/navigation_controller_impl.cc:629: void > > > NavigationControllerImpl::GoToOffset(int offset) { > > > Might be useful to add tracing to this one and GoToIndex, to cover other > cases > > > you might not have seen yet. > > > > So if back/forward buttons are pressed, GoBack() and GoForward() are > definitely > > gonna be called right? > > If that's the case, we may only need these two for now. Can you say more about > > the other cases that you > > referred please? > > If you long press the back button, you get a list of all navigations and you can > pick specific one. If it is further back in history than your previous one, I > would expect to see one of these two methods to be called, not just the > GoBack/GoForward. > https://cs.chromium.org/chromium/src/chrome/browser/ui/browser_commands.cc?rc... I see. Thanks for the information. We will come back and add more tracing events when we are sure we need to include more. But for now we are going to land with only these two.
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/2748393003/#ps20001 (title: "addressed comments")
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": 20001, "attempt_start_ts": 1489701967862980,
"parent_rev": "8086b9ac7fe4598925a84e6169760187022c3eb3", "commit_rev":
"51a271258fe44f4499b4894883cdc11123542e60"}
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1489701967862980,
"parent_rev": "4d824f56c1adbc453215841fa395927205058cd1", "commit_rev":
"666f71455a4f01ee74a1dee3bab82ed62813c255"}
Message was sent while issue was closed.
Description was changed from ========== Add trace events in NavigationControllerImpl::back/forward(...). Currently, there is a small set of user input events that will trigger SPA (Single Page App) navigations. We just found out that pressing back/forward buttons can also trigger SPA navigations. When that happens, there is no input handling in the browser process, which is what we use to capture as navigation start event when user-input-triggered SPA navigations happened. Therefore, we need to add trace events in NavigationControllerImpl::back/forward(...) function calls and capture them as navigation start events when SPA navigations happen. BUG=702024 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Add trace events in NavigationControllerImpl::back/forward(...). Currently, there is a small set of user input events that will trigger SPA (Single Page App) navigations. We just found out that pressing back/forward buttons can also trigger SPA navigations. When that happens, there is no input handling in the browser process, which is what we use to capture as navigation start event when user-input-triggered SPA navigations happened. Therefore, we need to add trace events in NavigationControllerImpl::back/forward(...) function calls and capture them as navigation start events when SPA navigations happen. BUG=702024 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2748393003 Cr-Commit-Position: refs/heads/master@{#457624} Committed: https://chromium.googlesource.com/chromium/src/+/666f71455a4f01ee74a1dee3bab8... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/666f71455a4f01ee74a1dee3bab8...
Message was sent while issue was closed.
https://codereview.chromium.org/2748393003/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2748393003/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:629: void NavigationControllerImpl::GoToOffset(int offset) { On 2017/03/16 20:34:44, nasko (slow) wrote: > Might be useful to add tracing to this one and GoToIndex, to cover other cases > you might not have seen yet. If we only instrumented GoToIndex, we'd have full coverage here. I'd be in favor of switching to only tracing in GoToIndex. |
