Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2776633003:
Add taskid for navigation, created in session sync (Closed)
DescriptionAdd taskids for navigation, created in session sync.
This is the first step to introduce task in Chrome, which links and
groups navigations. Each navigation has a task id and a list of its
ancestor's task ids. A navigation's task id is TabNavigation's global
id when the navigation is first visited and then reused by going
back/forward visits. The task ids are created and sent to backend in
session sync.
And for now we only track navigation relationship in a tab.
BUG=707978
Review-Url: https://codereview.chromium.org/2776633003
Cr-Commit-Position: refs/heads/master@{#466188}
Committed: https://chromium.googlesource.com/chromium/src/+/965fab94d2a8faef6f9ed4c6feaf99ef18e81049
Patch Set 1 #
Total comments: 18
Patch Set 2 : comments and refactoring #
Total comments: 34
Patch Set 3 : Fixing a bug where root of the task is not considered and addressing comments. #
Total comments: 12
Patch Set 4 : addressing comments and fixing a crash in test #Patch Set 5 : initialize task id by global id #
Total comments: 9
Patch Set 6 : Limiting max number of tracked tasks per tab #
Total comments: 4
Patch Set 7 : Add taskids for navigation, created in session sync. #
Messages
Total messages: 51 (29 generated)
Description was changed from ========== Add taskid for navigation, created in session sync This is the first step to introduce task in Chrome, which links and groups navigations. Each navigation has a task id with prefix of task id of its parent navigation. The task ids are created and sent to backend in session sync. For simplicity, the task id for now is composed of timestamps. And for now we only track navigation relationship in a tab. BUG= ========== to ========== Add taskid for navigation, created in session sync This is the first step to introduce task in Chrome, which links and groups navigations. Each navigation has a task id with prefix of task id of its parent navigation. The task ids are created and sent to backend in session sync. For simplicity, the task id for now is composed of timestamps. And for now we only track navigation relationship in a tab. BUG= ==========
shenchao@chromium.org changed reviewers: + pnoland@chromium.org, zea@chromium.org
The CQ bit was checked by shenchao@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...
Hi Nicolas and Patrick, Sorry for delay of this CL. Although some auto tests still fail and I need to update tests of sessions_sync_manager_unittests.cc, could you provide some early feedback? BTW, do I need to create a bug? Thank you.
Comments from my first impressions https://codereview.chromium.org/2776633003/diff/1/components/sessions/core/se... File components/sessions/core/serialized_navigation_entry.h (right): https://codereview.chromium.org/2776633003/diff/1/components/sessions/core/se... components/sessions/core/serialized_navigation_entry.h:82: sync_pb::TabNavigation ToSyncData() const; I think you'll need to update the definition of ToSyncData to set the task ids on the returned TabNavigation https://codereview.chromium.org/2776633003/diff/1/components/sessions/core/se... components/sessions/core/serialized_navigation_entry.h:176: // Task id of a Chrome task, which is prefixed by its parent task. Can you explain the prefixing more fully; i.e. it's not prefixed by just its parent, but all the way up to the root. https://codereview.chromium.org/2776633003/diff/1/components/sync/protocol/se... File components/sync/protocol/session_specifics.proto (right): https://codereview.chromium.org/2776633003/diff/1/components/sync/protocol/se... components/sync/protocol/session_specifics.proto:159: Please add a comment, ideally explaining why it's repeated int64 https://codereview.chromium.org/2776633003/diff/1/components/sync_sessions/ta... File components/sync_sessions/task_tracker.cc (right): https://codereview.chromium.org/2776633003/diff/1/components/sync_sessions/ta... components/sync_sessions/task_tracker.cc:26: // Assuming the first navigation is chrome://newtab Is that always true? What about links opened in a new tab or window? https://codereview.chromium.org/2776633003/diff/1/components/sync_sessions/ta... components/sync_sessions/task_tracker.cc:50: if (current_nav_index == current_task_index_ + 1) { This doesn't seem to account for transitions that start new tasks https://codereview.chromium.org/2776633003/diff/1/components/sync_sessions/ta... File components/sync_sessions/task_tracker.h (right): https://codereview.chromium.org/2776633003/diff/1/components/sync_sessions/ta... components/sync_sessions/task_tracker.h:22: // Tracks tasks of a tab. Can you expand this comment? It's not currently explaining anything that the name of the class doesn't communicate already. https://codereview.chromium.org/2776633003/diff/1/components/sync_sessions/ta... components/sync_sessions/task_tracker.h:30: std::vector<int64_t> GetTaskIdAtNav(int nav_index) const; AtIndex or at AtNavIndex would be more descriptive https://codereview.chromium.org/2776633003/diff/1/components/sync_sessions/ta... components/sync_sessions/task_tracker.h:32: int GetNavigationsNum() const { return ids_.size(); } count or size would be more descriptive than Num https://codereview.chromium.org/2776633003/diff/1/components/sync_sessions/ta... components/sync_sessions/task_tracker.h:39: int64_t new_id); I'm a little confused that new_id is supplied as an argument. Why isn't the id set by TabTasks?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Thanks for the review. PTAL https://codereview.chromium.org/2776633003/diff/1/components/sessions/core/se... File components/sessions/core/serialized_navigation_entry.h (right): https://codereview.chromium.org/2776633003/diff/1/components/sessions/core/se... components/sessions/core/serialized_navigation_entry.h:82: sync_pb::TabNavigation ToSyncData() const; On 2017/03/24 20:51:57, Patrick Noland wrote: > I think you'll need to update the definition of ToSyncData to set the task ids > on the returned TabNavigation Done. https://codereview.chromium.org/2776633003/diff/1/components/sessions/core/se... components/sessions/core/serialized_navigation_entry.h:176: // Task id of a Chrome task, which is prefixed by its parent task. On 2017/03/24 20:51:57, Patrick Noland wrote: > Can you explain the prefixing more fully; i.e. it's not prefixed by just its > parent, but all the way up to the root. Move it above to the variables corresponding to NavigationEntry fields, so people can go to proto to see explanation https://codereview.chromium.org/2776633003/diff/1/components/sync/protocol/se... File components/sync/protocol/session_specifics.proto (right): https://codereview.chromium.org/2776633003/diff/1/components/sync/protocol/se... components/sync/protocol/session_specifics.proto:159: On 2017/03/24 20:51:57, Patrick Noland wrote: > Please add a comment, ideally explaining why it's repeated int64 Done. https://codereview.chromium.org/2776633003/diff/1/components/sync_sessions/ta... File components/sync_sessions/task_tracker.cc (right): https://codereview.chromium.org/2776633003/diff/1/components/sync_sessions/ta... components/sync_sessions/task_tracker.cc:26: // Assuming the first navigation is chrome://newtab On 2017/03/24 20:51:57, Patrick Noland wrote: > Is that always true? What about links opened in a new tab or window? Not unless the if-condition is true. Added comment to clarify. https://codereview.chromium.org/2776633003/diff/1/components/sync_sessions/ta... components/sync_sessions/task_tracker.cc:50: if (current_nav_index == current_task_index_ + 1) { On 2017/03/24 20:51:57, Patrick Noland wrote: > This doesn't seem to account for transitions that start new tasks current_nav_index == current_task_index_ + 1 && current_nav_transition & ui::PAGE_TRANSITION_FORWARD_BACK is covered by previous if. Could you help me find out what else I missed? Thanks https://codereview.chromium.org/2776633003/diff/1/components/sync_sessions/ta... File components/sync_sessions/task_tracker.h (right): https://codereview.chromium.org/2776633003/diff/1/components/sync_sessions/ta... components/sync_sessions/task_tracker.h:22: // Tracks tasks of a tab. On 2017/03/24 20:51:57, Patrick Noland wrote: > Can you expand this comment? It's not currently explaining anything that the > name of the class doesn't communicate already. Done. https://codereview.chromium.org/2776633003/diff/1/components/sync_sessions/ta... components/sync_sessions/task_tracker.h:30: std::vector<int64_t> GetTaskIdAtNav(int nav_index) const; On 2017/03/24 20:51:57, Patrick Noland wrote: > AtIndex or at AtNavIndex would be more descriptive Done. https://codereview.chromium.org/2776633003/diff/1/components/sync_sessions/ta... components/sync_sessions/task_tracker.h:32: int GetNavigationsNum() const { return ids_.size(); } On 2017/03/24 20:51:57, Patrick Noland wrote: > count or size would be more descriptive than Num Done. https://codereview.chromium.org/2776633003/diff/1/components/sync_sessions/ta... components/sync_sessions/task_tracker.h:39: int64_t new_id); On 2017/03/24 20:51:57, Patrick Noland wrote: > I'm a little confused that new_id is supplied as an argument. Why isn't the id > set by TabTasks? Done. I did that to make test easy but agree it was hacky. Injecting a clock now.
https://codereview.chromium.org/2776633003/diff/60001/components/sessions/cor... File components/sessions/core/serialized_navigation_entry.h (right): https://codereview.chromium.org/2776633003/diff/60001/components/sessions/cor... components/sessions/core/serialized_navigation_entry.h:166: std::vector<int64_t> task_id_; This class is exclusively meant to wrap data stored within a NavigationEntry. As such, I don't think this is the right place for this task id to live. What we probably need to do is stop using the SerializedNavigationEntry directly in the SessionsSyncManager, and instead have our own wrapper around it (e.g. SyncSessionTab). https://codereview.chromium.org/2776633003/diff/60001/components/sync/protoco... File components/sync/protocol/session_specifics.proto (right): https://codereview.chromium.org/2776633003/diff/60001/components/sync/protoco... components/sync/protocol/session_specifics.proto:165: // we can get ids of its all ancester tasks. Explain what a task node id is (namely a GUID that defines a specific navigation that matches global_id above) https://codereview.chromium.org/2776633003/diff/60001/components/sync/protoco... components/sync/protocol/session_specifics.proto:182: repeated int64 task_node_id = 1; Should we bother encoding the current task id/ It's basically going to be the same as the global id above right? https://codereview.chromium.org/2776633003/diff/60001/components/sync_session... File components/sync_sessions/sessions_sync_manager.cc (right): https://codereview.chromium.org/2776633003/diff/60001/components/sync_session... components/sync_sessions/sessions_sync_manager.cc:1064: tab_tasks->UpdateTask(current_index, nit: curly braces around multi-line if https://codereview.chromium.org/2776633003/diff/60001/components/sync_session... components/sync_sessions/sessions_sync_manager.cc:1067: DVLOG(0) << "current_index: " << current_index << " of " Did you mean to leave this? https://codereview.chromium.org/2776633003/diff/60001/components/sync_session... File components/sync_sessions/sessions_sync_manager.h (right): https://codereview.chromium.org/2776633003/diff/60001/components/sync_session... components/sync_sessions/sessions_sync_manager.h:331: std::unique_ptr<TaskTracker> task_tracker_; nit: comment what this is for https://codereview.chromium.org/2776633003/diff/60001/components/sync_session... File components/sync_sessions/task_tracker.cc (right): https://codereview.chromium.org/2776633003/diff/60001/components/sync_session... components/sync_sessions/task_tracker.cc:18: CHECK(base::checked_cast<size_t>(nav_index) < ids_.size()); CHECK_LT https://codereview.chromium.org/2776633003/diff/60001/components/sync_session... components/sync_sessions/task_tracker.cc:32: if (current_nav_index == 1 && current_task_index_ == -1) { This seems a bit brittle, as it assumes behavior of the caller. For example, what will happen on restart, when a tab with a navigation stack is restored? If seems like we should have the same behavior regardless of current_nav_index, right? https://codereview.chromium.org/2776633003/diff/60001/components/sync_session... components/sync_sessions/task_tracker.cc:39: DVLOG(1) << "Doing nothing"; nit: for debugability, might be good to also print out what the transition we're ignoring is (here and below) https://codereview.chromium.org/2776633003/diff/60001/components/sync_session... components/sync_sessions/task_tracker.cc:54: // a new navigation grammar nit: "A new navigation." https://codereview.chromium.org/2776633003/diff/60001/components/sync_session... components/sync_sessions/task_tracker.cc:55: if (current_nav_index == current_task_index_ + 1) { This seems to create a new task regardless of transition type. Shouldn't this check for link transition first? https://codereview.chromium.org/2776633003/diff/60001/components/sync_session... components/sync_sessions/task_tracker.cc:59: ids_.push_back(clock_->Now().ToTimeT()); I think the ids can't be based on their own clock. They need to match the global id that are already being synced (so that we can use task id to identify individual navigations) https://codereview.chromium.org/2776633003/diff/60001/components/sync_session... File components/sync_sessions/task_tracker.h (right): https://codereview.chromium.org/2776633003/diff/60001/components/sync_session... components/sync_sessions/task_tracker.h:28: // int64_t new_id) new_id doesn't seem to be a param anymore? https://codereview.chromium.org/2776633003/diff/60001/components/sync_session... components/sync_sessions/task_tracker.h:44: ui::PageTransition current_nav_transition); Would be good to also explain what happens when a tab goes back/forward through the nav stack. Presumably the current_nav_transition param is ignored? https://codereview.chromium.org/2776633003/diff/60001/components/sync_session... components/sync_sessions/task_tracker.h:48: std::vector<int64_t> ids_; ids_ is pretty ambiguous. Perhaps call this task_ids_? https://codereview.chromium.org/2776633003/diff/60001/components/sync_session... components/sync_sessions/task_tracker.h:49: int current_task_index_ = -1; Add DISALLOW_COPY_AND_ASSIGN macro (here and below) https://codereview.chromium.org/2776633003/diff/60001/components/sync_session... components/sync_sessions/task_tracker.h:59: // Returns a TabTasks pointer, which is owned by this object, for the tab of nit: newline above
Also, it would be good to go ahead and file a bug.
Hi Nicolas, Please check the comment below. I should have found this issue early. Any suggestions? Thanks https://codereview.chromium.org/2776633003/diff/60001/components/sync/protoco... File components/sync/protocol/session_specifics.proto (right): https://codereview.chromium.org/2776633003/diff/60001/components/sync/protoco... components/sync/protocol/session_specifics.proto:182: repeated int64 task_node_id = 1; On 2017/03/27 20:43:52, Nicolas Zea wrote: > Should we bother encoding the current task id/ It's basically going to be the > same as the global id above right? Just found we can't use global_id, which changes when the page is visited via forward_back. But we want to keep task_node_id same for different visits over time, right?
On 2017/04/04 18:37:50, shenchao wrote: > Hi Nicolas, > Please check the comment below. I should have found this issue early. Any > suggestions? Thanks > > https://codereview.chromium.org/2776633003/diff/60001/components/sync/protoco... > File components/sync/protocol/session_specifics.proto (right): > > https://codereview.chromium.org/2776633003/diff/60001/components/sync/protoco... > components/sync/protocol/session_specifics.proto:182: repeated int64 > task_node_id = 1; > On 2017/03/27 20:43:52, Nicolas Zea wrote: > > Should we bother encoding the current task id/ It's basically going to be the > > same as the global id above right? > > Just found we can't use global_id, which changes when the page is visited via > forward_back. But we want to keep task_node_id same for different visits over > time, right? I kind of think like we should treat this as a new navigation in that case, but part of the same chain. So, imagine you have the following chain of navigations id_a, id_b, id_c You go back: id_a, id_b You go forward again id_a, id_b, id_d id_c and id_d refer to navigations to the same url at different times. Make sense? Their relationship is still encoded by having a shared ancestor, but I don't know that we need to reuse the same exact id.
On 2017/04/05 19:44:23, Nicolas Zea wrote: > On 2017/04/04 18:37:50, shenchao wrote: > > Hi Nicolas, > > Please check the comment below. I should have found this issue early. Any > > suggestions? Thanks > > > > > https://codereview.chromium.org/2776633003/diff/60001/components/sync/protoco... > > File components/sync/protocol/session_specifics.proto (right): > > > > > https://codereview.chromium.org/2776633003/diff/60001/components/sync/protoco... > > components/sync/protocol/session_specifics.proto:182: repeated int64 > > task_node_id = 1; > > On 2017/03/27 20:43:52, Nicolas Zea wrote: > > > Should we bother encoding the current task id/ It's basically going to be > the > > > same as the global id above right? > > > > Just found we can't use global_id, which changes when the page is visited via > > forward_back. But we want to keep task_node_id same for different visits over > > time, right? > > I kind of think like we should treat this as a new navigation in that case, but > part of the same chain. So, imagine you have the following chain of navigations > id_a, id_b, id_c > You go back: > id_a, id_b > You go forward again > id_a, id_b, id_d > > id_c and id_d refer to navigations to the same url at different times. Make > sense? Their relationship is still encoded by having a shared ancestor, but I > don't know that we need to reuse the same exact id. Problem is id_b. Assume we have id_a, id_b, id_c When we get back to id_b, it's actually id_a, id_b' (id_b' > id_b in term of time, and they are indeed different navigations, with different navigation_forward_back value) When we click a new link and get id_a, id_b', id_d So we lost the info that id_c and id_b are from the same page
On 2017/04/05 21:01:03, shenchao wrote: > On 2017/04/05 19:44:23, Nicolas Zea wrote: > > On 2017/04/04 18:37:50, shenchao wrote: > > > Hi Nicolas, > > > Please check the comment below. I should have found this issue early. Any > > > suggestions? Thanks > > > > > > > > > https://codereview.chromium.org/2776633003/diff/60001/components/sync/protoco... > > > File components/sync/protocol/session_specifics.proto (right): > > > > > > > > > https://codereview.chromium.org/2776633003/diff/60001/components/sync/protoco... > > > components/sync/protocol/session_specifics.proto:182: repeated int64 > > > task_node_id = 1; > > > On 2017/03/27 20:43:52, Nicolas Zea wrote: > > > > Should we bother encoding the current task id/ It's basically going to be > > the > > > > same as the global id above right? > > > > > > Just found we can't use global_id, which changes when the page is visited > via > > > forward_back. But we want to keep task_node_id same for different visits > over > > > time, right? > > > > I kind of think like we should treat this as a new navigation in that case, > but > > part of the same chain. So, imagine you have the following chain of > navigations > > id_a, id_b, id_c > > You go back: > > id_a, id_b > > You go forward again > > id_a, id_b, id_d > > > > id_c and id_d refer to navigations to the same url at different times. Make > > sense? Their relationship is still encoded by having a shared ancestor, but I > > don't know that we need to reuse the same exact id. > > Problem is id_b. Assume we have > id_a, id_b, id_c > > When we get back to id_b, it's actually > id_a, id_b' (id_b' > id_b in term of time, and they are indeed different > navigations, with different navigation_forward_back value) > > When we click a new link and get > id_a, id_b', id_d > > So we lost the info that id_c and id_b are from the same page Ah, got it. so we should encode a separate task id. I do think it would be useful to have the task id be the original global id though, even if it doesn't match the current one. That way you can see that the user resumed a task at a later time. But you're right we'll need this to be a different field.
Description was changed from ========== Add taskid for navigation, created in session sync This is the first step to introduce task in Chrome, which links and groups navigations. Each navigation has a task id with prefix of task id of its parent navigation. The task ids are created and sent to backend in session sync. For simplicity, the task id for now is composed of timestamps. And for now we only track navigation relationship in a tab. BUG= ========== to ========== Add taskid for navigation, created in session sync This is the first step to introduce task in Chrome, which links and groups navigations. Each navigation has a task id with prefix of task id of its parent navigation. The task ids are created and sent to backend in session sync. For simplicity, the task id for now is composed of timestamps. And for now we only track navigation relationship in a tab. BUG=707978 ==========
Patchset #3 (id:80001) has been deleted
On 2017/04/05 22:50:36, Nicolas Zea wrote: > On 2017/04/05 21:01:03, shenchao wrote: > > On 2017/04/05 19:44:23, Nicolas Zea wrote: > > > On 2017/04/04 18:37:50, shenchao wrote: > > > > Hi Nicolas, > > > > Please check the comment below. I should have found this issue early. Any > > > > suggestions? Thanks > > > > > > > > > > > > > > https://codereview.chromium.org/2776633003/diff/60001/components/sync/protoco... > > > > File components/sync/protocol/session_specifics.proto (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2776633003/diff/60001/components/sync/protoco... > > > > components/sync/protocol/session_specifics.proto:182: repeated int64 > > > > task_node_id = 1; > > > > On 2017/03/27 20:43:52, Nicolas Zea wrote: > > > > > Should we bother encoding the current task id/ It's basically going to > be > > > the > > > > > same as the global id above right? > > > > > > > > Just found we can't use global_id, which changes when the page is visited > > via > > > > forward_back. But we want to keep task_node_id same for different visits > > over > > > > time, right? > > > > > > I kind of think like we should treat this as a new navigation in that case, > > but > > > part of the same chain. So, imagine you have the following chain of > > navigations > > > id_a, id_b, id_c > > > You go back: > > > id_a, id_b > > > You go forward again > > > id_a, id_b, id_d > > > > > > id_c and id_d refer to navigations to the same url at different times. Make > > > sense? Their relationship is still encoded by having a shared ancestor, but > I > > > don't know that we need to reuse the same exact id. > > > > Problem is id_b. Assume we have > > id_a, id_b, id_c > > > > When we get back to id_b, it's actually > > id_a, id_b' (id_b' > id_b in term of time, and they are indeed different > > navigations, with different navigation_forward_back value) > > > > When we click a new link and get > > id_a, id_b', id_d > > > > So we lost the info that id_c and id_b are from the same page > > Ah, got it. so we should encode a separate task id. I do think it would be > useful to have the task id be the original global id though, even if it doesn't > match the current one. That way you can see that the user resumed a task at a > later time. But you're right we'll need this to be a different field. Wouldn't the separated task id also help user resume an earlier task?
PTAL, while I am adding unit tests for sessions_sync_manager. Thanks https://codereview.chromium.org/2776633003/diff/60001/components/sessions/cor... File components/sessions/core/serialized_navigation_entry.h (right): https://codereview.chromium.org/2776633003/diff/60001/components/sessions/cor... components/sessions/core/serialized_navigation_entry.h:166: std::vector<int64_t> task_id_; On 2017/03/27 20:43:52, Nicolas Zea wrote: > This class is exclusively meant to wrap data stored within a NavigationEntry. As > such, I don't think this is the right place for this task id to live. > > What we probably need to do is stop using the SerializedNavigationEntry directly > in the SessionsSyncManager, and instead have our own wrapper around it (e.g. > SyncSessionTab). Acknowledged. Stopped using the SerializedNavigationEntry, but maybe leaving having a new wrapper as future work. https://codereview.chromium.org/2776633003/diff/60001/components/sync/protoco... File components/sync/protocol/session_specifics.proto (right): https://codereview.chromium.org/2776633003/diff/60001/components/sync/protoco... components/sync/protocol/session_specifics.proto:165: // we can get ids of its all ancester tasks. On 2017/03/27 20:43:52, Nicolas Zea wrote: > Explain what a task node id is (namely a GUID that defines a specific navigation > that matches global_id above) Done. https://codereview.chromium.org/2776633003/diff/60001/components/sync_session... File components/sync_sessions/sessions_sync_manager.cc (right): https://codereview.chromium.org/2776633003/diff/60001/components/sync_session... components/sync_sessions/sessions_sync_manager.cc:1064: tab_tasks->UpdateTask(current_index, On 2017/03/27 20:43:52, Nicolas Zea wrote: > nit: curly braces around multi-line if Done. https://codereview.chromium.org/2776633003/diff/60001/components/sync_session... components/sync_sessions/sessions_sync_manager.cc:1067: DVLOG(0) << "current_index: " << current_index << " of " On 2017/03/27 20:43:52, Nicolas Zea wrote: > Did you mean to leave this? Done. https://codereview.chromium.org/2776633003/diff/60001/components/sync_session... File components/sync_sessions/sessions_sync_manager.h (right): https://codereview.chromium.org/2776633003/diff/60001/components/sync_session... components/sync_sessions/sessions_sync_manager.h:331: std::unique_ptr<TaskTracker> task_tracker_; On 2017/03/27 20:43:52, Nicolas Zea wrote: > nit: comment what this is for Done. https://codereview.chromium.org/2776633003/diff/60001/components/sync_session... File components/sync_sessions/task_tracker.cc (right): https://codereview.chromium.org/2776633003/diff/60001/components/sync_session... components/sync_sessions/task_tracker.cc:18: CHECK(base::checked_cast<size_t>(nav_index) < ids_.size()); On 2017/03/27 20:43:52, Nicolas Zea wrote: > CHECK_LT Done. https://codereview.chromium.org/2776633003/diff/60001/components/sync_session... components/sync_sessions/task_tracker.cc:32: if (current_nav_index == 1 && current_task_index_ == -1) { On 2017/03/27 20:43:52, Nicolas Zea wrote: > This seems a bit brittle, as it assumes behavior of the caller. For example, > what will happen on restart, when a tab with a navigation stack is restored? > > If seems like we should have the same behavior regardless of current_nav_index, > right? Done. https://codereview.chromium.org/2776633003/diff/60001/components/sync_session... components/sync_sessions/task_tracker.cc:39: DVLOG(1) << "Doing nothing"; On 2017/03/27 20:43:52, Nicolas Zea wrote: > nit: for debugability, might be good to also print out what the transition we're > ignoring is (here and below) Done. https://codereview.chromium.org/2776633003/diff/60001/components/sync_session... components/sync_sessions/task_tracker.cc:54: // a new navigation On 2017/03/27 20:43:52, Nicolas Zea wrote: > grammar nit: "A new navigation." Done. https://codereview.chromium.org/2776633003/diff/60001/components/sync_session... components/sync_sessions/task_tracker.cc:55: if (current_nav_index == current_task_index_ + 1) { On 2017/03/27 20:43:52, Nicolas Zea wrote: > This seems to create a new task regardless of transition type. Shouldn't this > check for link transition first? Done. https://codereview.chromium.org/2776633003/diff/60001/components/sync_session... components/sync_sessions/task_tracker.cc:59: ids_.push_back(clock_->Now().ToTimeT()); On 2017/03/27 20:43:52, Nicolas Zea wrote: > I think the ids can't be based on their own clock. They need to match the global > id that are already being synced (so that we can use task id to identify > individual navigations) We can't use global id so have to create our own task id. https://codereview.chromium.org/2776633003/diff/60001/components/sync_session... File components/sync_sessions/task_tracker.h (right): https://codereview.chromium.org/2776633003/diff/60001/components/sync_session... components/sync_sessions/task_tracker.h:28: // int64_t new_id) On 2017/03/27 20:43:52, Nicolas Zea wrote: > new_id doesn't seem to be a param anymore? Done. https://codereview.chromium.org/2776633003/diff/60001/components/sync_session... components/sync_sessions/task_tracker.h:44: ui::PageTransition current_nav_transition); On 2017/03/27 20:43:52, Nicolas Zea wrote: > Would be good to also explain what happens when a tab goes back/forward through > the nav stack. Presumably the current_nav_transition param is ignored? Actually the current_nav_transition param is not ignored but used to decided if it's back/forward nav. Added comments. https://codereview.chromium.org/2776633003/diff/60001/components/sync_session... components/sync_sessions/task_tracker.h:48: std::vector<int64_t> ids_; On 2017/03/27 20:43:53, Nicolas Zea wrote: > ids_ is pretty ambiguous. Perhaps call this task_ids_? Done. https://codereview.chromium.org/2776633003/diff/60001/components/sync_session... components/sync_sessions/task_tracker.h:49: int current_task_index_ = -1; On 2017/03/27 20:43:52, Nicolas Zea wrote: > Add DISALLOW_COPY_AND_ASSIGN macro (here and below) Done. https://codereview.chromium.org/2776633003/diff/60001/components/sync_session... components/sync_sessions/task_tracker.h:59: // Returns a TabTasks pointer, which is owned by this object, for the tab of On 2017/03/27 20:43:53, Nicolas Zea wrote: > nit: newline above Done.
Getting close! https://codereview.chromium.org/2776633003/diff/100001/components/sync_sessio... File components/sync_sessions/sessions_sync_manager.cc (right): https://codereview.chromium.org/2776633003/diff/100001/components/sync_sessio... components/sync_sessions/sessions_sync_manager.cc:426: SessionID::id_type tab_id, given tab_id comes from tab_delegate, it seems like an unnecessary parameter? https://codereview.chromium.org/2776633003/diff/100001/components/sync_sessio... File components/sync_sessions/task_tracker.cc (right): https://codereview.chromium.org/2776633003/diff/100001/components/sync_sessio... components/sync_sessions/task_tracker.cc:41: if (navigation_index == current_navigation_index_) { This will also capture a reload right? https://codereview.chromium.org/2776633003/diff/100001/components/sync_sessio... components/sync_sessions/task_tracker.cc:61: ui::PageTransitionCoreTypeIs(transition, ui::PAGE_TRANSITION_LINK)) { What about other non-link transitions that should be treated as the same task, like form_submit or server redirect? https://codereview.chromium.org/2776633003/diff/100001/components/sync_sessio... components/sync_sessions/task_tracker.cc:77: clock_->Now().ToInternalValue()}; I still think we shouldn't be introducing a new timestamp. Why can't we reuse the global id here? You already have logic above checking for the back/forward case, so here it should be fine to reuse the global id right? https://codereview.chromium.org/2776633003/diff/100001/components/sync_sessio... File components/sync_sessions/task_tracker.h (right): https://codereview.chromium.org/2776633003/diff/100001/components/sync_sessio... components/sync_sessions/task_tracker.h:30: // necessary the first navigation in this case. Need to fix it by initalizing nit: necessary -> necessarily https://codereview.chromium.org/2776633003/diff/100001/components/sync_sessio... components/sync_sessions/task_tracker.h:40: std::vector<int64_t> RootToSelfTaskIdsOfNavigationIndex( This name is pretty tough to understand. How about just GetTaskIdsForNavigation?
Made suggested changes, and leaving global id usage open for further discussion. https://codereview.chromium.org/2776633003/diff/100001/components/sync_sessio... File components/sync_sessions/sessions_sync_manager.cc (right): https://codereview.chromium.org/2776633003/diff/100001/components/sync_sessio... components/sync_sessions/sessions_sync_manager.cc:426: SessionID::id_type tab_id, On 2017/04/10 17:09:00, Nicolas Zea wrote: > given tab_id comes from tab_delegate, it seems like an unnecessary parameter? Done. https://codereview.chromium.org/2776633003/diff/100001/components/sync_sessio... File components/sync_sessions/task_tracker.cc (right): https://codereview.chromium.org/2776633003/diff/100001/components/sync_sessio... components/sync_sessions/task_tracker.cc:41: if (navigation_index == current_navigation_index_) { On 2017/04/10 17:09:00, Nicolas Zea wrote: > This will also capture a reload right? Yes https://codereview.chromium.org/2776633003/diff/100001/components/sync_sessio... components/sync_sessions/task_tracker.cc:61: ui::PageTransitionCoreTypeIs(transition, ui::PAGE_TRANSITION_LINK)) { On 2017/04/10 17:09:00, Nicolas Zea wrote: > What about other non-link transitions that should be treated as the same task, > like form_submit or server redirect? Done. https://codereview.chromium.org/2776633003/diff/100001/components/sync_sessio... File components/sync_sessions/task_tracker.h (right): https://codereview.chromium.org/2776633003/diff/100001/components/sync_sessio... components/sync_sessions/task_tracker.h:30: // necessary the first navigation in this case. Need to fix it by initalizing On 2017/04/10 17:09:01, Nicolas Zea wrote: > nit: necessary -> necessarily Done. https://codereview.chromium.org/2776633003/diff/100001/components/sync_sessio... components/sync_sessions/task_tracker.h:40: std::vector<int64_t> RootToSelfTaskIdsOfNavigationIndex( On 2017/04/10 17:09:00, Nicolas Zea wrote: > This name is pretty tough to understand. How about just GetTaskIdsForNavigation? Done.
Patchset #5 (id:140001) has been deleted
Patchset #5 (id:160001) has been deleted
PTAL https://codereview.chromium.org/2776633003/diff/100001/components/sync_sessio... File components/sync_sessions/task_tracker.cc (right): https://codereview.chromium.org/2776633003/diff/100001/components/sync_sessio... components/sync_sessions/task_tracker.cc:77: clock_->Now().ToInternalValue()}; On 2017/04/10 17:09:00, Nicolas Zea wrote: > I still think we shouldn't be introducing a new timestamp. Why can't we reuse > the global id here? You already have logic above checking for the back/forward > case, so here it should be fine to reuse the global id right? Done.
Mostly LG with one change related to handling task id limiting. https://codereview.chromium.org/2776633003/diff/180001/components/sync_sessio... File components/sync_sessions/sessions_sync_manager_unittest.cc (right): https://codereview.chromium.org/2776633003/diff/180001/components/sync_sessio... components/sync_sessions/sessions_sync_manager_unittest.cc:2433: // We only test changes for tab add and tab update, and ingore header updates. nit: ingore -> ignore https://codereview.chromium.org/2776633003/diff/180001/components/sync_sessio... components/sync_sessions/sessions_sync_manager_unittest.cc:2436: SyncDataLocal(out[1].sync_data()).GetSpecifics().session().tab(); If you're ignoring header updates, you can use FilterOutLocalHeaderChanges https://codereview.chromium.org/2776633003/diff/180001/components/sync_sessio... File components/sync_sessions/task_tracker.cc (right): https://codereview.chromium.org/2776633003/diff/180001/components/sync_sessio... components/sync_sessions/task_tracker.cc:48: return; I think the limiting should be done at the oldest ancestor. In other words, keep only the most recent kMaxNumTasksPerTab ancestors in the TaskTracker. We would always want to have a task id for a new navigation though. Basically, task_ids_ should be limited to a max of kMaxNumTasksPerTab in size. Once you hit that limit you have to start shifting task ids down (and updating their navigation indices accordingly). https://codereview.chromium.org/2776633003/diff/180001/components/sync_sessio... components/sync_sessions/task_tracker.cc:103: // Erase those for forward for the previous navigations. nit: This comment is unclear. How about "Erase all task ids associated with an outdated forward navigation stack"
https://codereview.chromium.org/2776633003/diff/180001/components/sync_sessio... File components/sync_sessions/task_tracker.cc (right): https://codereview.chromium.org/2776633003/diff/180001/components/sync_sessio... components/sync_sessions/task_tracker.cc:48: return; On 2017/04/17 23:18:46, Nicolas Zea wrote: > I think the limiting should be done at the oldest ancestor. In other words, keep > only the most recent kMaxNumTasksPerTab ancestors in the TaskTracker. We would > always want to have a task id for a new navigation though. > > Basically, task_ids_ should be limited to a max of kMaxNumTasksPerTab in size. > Once you hit that limit you have to start shifting task ids down (and updating > their navigation indices accordingly). We can do that. Here is the plan: 1. Add size_t excluded_ancestor_num_, and use navigation_index - excluded_ancestor_num_ as vector position. 2. Use vector::erase to exclude oldest ancestor. It is a little expensive (shifting all elements in the vector). Is it ok? 3. Once an ancestor is excluded, it won't be accessible when going back. If happens, it's task_id is empty. And if a new sub task from this excluded task is created, the new subtask is consider as a root task.
On 2017/04/18 16:59:36, shenchao wrote: > https://codereview.chromium.org/2776633003/diff/180001/components/sync_sessio... > File components/sync_sessions/task_tracker.cc (right): > > https://codereview.chromium.org/2776633003/diff/180001/components/sync_sessio... > components/sync_sessions/task_tracker.cc:48: return; > On 2017/04/17 23:18:46, Nicolas Zea wrote: > > I think the limiting should be done at the oldest ancestor. In other words, > keep > > only the most recent kMaxNumTasksPerTab ancestors in the TaskTracker. We would > > always want to have a task id for a new navigation though. > > > > Basically, task_ids_ should be limited to a max of kMaxNumTasksPerTab in size. > > Once you hit that limit you have to start shifting task ids down (and updating > > their navigation indices accordingly). > > We can do that. Here is the plan: > 1. Add size_t excluded_ancestor_num_, and use navigation_index - > excluded_ancestor_num_ as vector position. > 2. Use vector::erase to exclude oldest ancestor. It is a little expensive > (shifting all elements in the vector). Is it ok? > 3. Once an ancestor is excluded, it won't be accessible when going back. If > happens, it's task_id is empty. And if a new sub task from this excluded task is > created, the new subtask is consider as a root task. Sounds good. And yeah, given this only happens on certain navigations, once per navigation, and for a max of 100 entries, the overhead should be fine.
Patchset #6 (id:200001) has been deleted
PTAL. Thanks. https://codereview.chromium.org/2776633003/diff/180001/components/sync_sessio... File components/sync_sessions/sessions_sync_manager_unittest.cc (right): https://codereview.chromium.org/2776633003/diff/180001/components/sync_sessio... components/sync_sessions/sessions_sync_manager_unittest.cc:2433: // We only test changes for tab add and tab update, and ingore header updates. On 2017/04/17 23:18:46, Nicolas Zea wrote: > nit: ingore -> ignore Done. https://codereview.chromium.org/2776633003/diff/180001/components/sync_sessio... components/sync_sessions/sessions_sync_manager_unittest.cc:2436: SyncDataLocal(out[1].sync_data()).GetSpecifics().session().tab(); On 2017/04/17 23:18:46, Nicolas Zea wrote: > If you're ignoring header updates, you can use FilterOutLocalHeaderChanges Done. https://codereview.chromium.org/2776633003/diff/180001/components/sync_sessio... File components/sync_sessions/task_tracker.cc (right): https://codereview.chromium.org/2776633003/diff/180001/components/sync_sessio... components/sync_sessions/task_tracker.cc:48: return; On 2017/04/18 16:59:36, shenchao wrote: > On 2017/04/17 23:18:46, Nicolas Zea wrote: > > I think the limiting should be done at the oldest ancestor. In other words, > keep > > only the most recent kMaxNumTasksPerTab ancestors in the TaskTracker. We would > > always want to have a task id for a new navigation though. > > > > Basically, task_ids_ should be limited to a max of kMaxNumTasksPerTab in size. > > Once you hit that limit you have to start shifting task ids down (and updating > > their navigation indices accordingly). > > We can do that. Here is the plan: > 1. Add size_t excluded_ancestor_num_, and use navigation_index - > excluded_ancestor_num_ as vector position. > 2. Use vector::erase to exclude oldest ancestor. It is a little expensive > (shifting all elements in the vector). Is it ok? > 3. Once an ancestor is excluded, it won't be accessible when going back. If > happens, it's task_id is empty. And if a new sub task from this excluded task is > created, the new subtask is consider as a root task. > Done. https://codereview.chromium.org/2776633003/diff/180001/components/sync_sessio... components/sync_sessions/task_tracker.cc:103: // Erase those for forward for the previous navigations. On 2017/04/17 23:18:46, Nicolas Zea wrote: > nit: This comment is unclear. How about "Erase all task ids associated with an > outdated forward navigation stack" Done.
LGTM with some nits. Nice! https://codereview.chromium.org/2776633003/diff/220001/components/sync_sessio... File components/sync_sessions/task_tracker.cc (right): https://codereview.chromium.org/2776633003/diff/220001/components/sync_sessio... components/sync_sessions/task_tracker.cc:125: task_ids_.push_back({-1, -1}); nit: typically we use curly braces around loop body if either condition or body don't fit on one line https://codereview.chromium.org/2776633003/diff/220001/components/sync_sessio... components/sync_sessions/task_tracker.cc:142: << " oldest ancester(s) from navigation index " nit: ancester -> ancestor
The CQ bit was checked by shenchao@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 shenchao@chromium.org
Patchset #7 (id:240001) has been deleted
Description was changed from
==========
Add taskid for navigation, created in session sync
This is the first step to introduce task in Chrome, which links and
groups navigations. Each navigation has a task id with prefix of task id
of its parent navigation. The task ids are created and sent to backend
in session sync.
For simplicity, the task id for now is composed of timestamps. And for
now we only track navigation relationship in a tab.
BUG=707978
==========
to
==========
Add taskids for navigation, created in session sync.
This is the first step to introduce task in Chrome, which links and groups
navigations. Each navigation has a task id and a list of its ancestor's task
ids. A navigation's task id is TabNavigation's global id when the navigation is
first visited and then reused by going back/forward visits. The task ids are
created and sent to backend in session sync.
And for now we only track navigation relationship in a tab.
BUG=707978
==========
Description was changed from
==========
Add taskids for navigation, created in session sync.
This is the first step to introduce task in Chrome, which links and groups
navigations. Each navigation has a task id and a list of its ancestor's task
ids. A navigation's task id is TabNavigation's global id when the navigation is
first visited and then reused by going back/forward visits. The task ids are
created and sent to backend in session sync.
And for now we only track navigation relationship in a tab.
BUG=707978
==========
to
==========
Add taskids for navigation, created in session sync.
This is the first step to introduce task in Chrome, which links and
groups navigations. Each navigation has a task id and a list of its
ancestor's task ids. A navigation's task id is TabNavigation's global
id when the navigation is first visited and then reused by going
back/forward visits. The task ids are created and sent to backend in
session sync.
And for now we only track navigation relationship in a tab.
BUG=707978
==========
https://codereview.chromium.org/2776633003/diff/220001/components/sync_sessio... File components/sync_sessions/task_tracker.cc (right): https://codereview.chromium.org/2776633003/diff/220001/components/sync_sessio... components/sync_sessions/task_tracker.cc:125: task_ids_.push_back({-1, -1}); On 2017/04/19 22:25:44, Nicolas Zea wrote: > nit: typically we use curly braces around loop body if either condition or body > don't fit on one line Done. https://codereview.chromium.org/2776633003/diff/220001/components/sync_sessio... components/sync_sessions/task_tracker.cc:142: << " oldest ancester(s) from navigation index " On 2017/04/19 22:25:43, Nicolas Zea wrote: > nit: ancester -> ancestor Done.
The CQ bit was checked by shenchao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org Link to the patchset: https://codereview.chromium.org/2776633003/#ps260001 (title: "Add taskids for navigation, created in session sync.")
The CQ bit was unchecked by shenchao@chromium.org
The CQ bit was checked by shenchao@chromium.org
The CQ bit was unchecked by shenchao@chromium.org
The CQ bit was checked by shenchao@chromium.org
The CQ bit was unchecked by shenchao@chromium.org
The CQ bit was checked by shenchao@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": 260001, "attempt_start_ts": 1492724453157940,
"parent_rev": "f206da906ef1e94ee03feeb67e11a82c565baa3b", "commit_rev":
"965fab94d2a8faef6f9ed4c6feaf99ef18e81049"}
Message was sent while issue was closed.
Description was changed from
==========
Add taskids for navigation, created in session sync.
This is the first step to introduce task in Chrome, which links and
groups navigations. Each navigation has a task id and a list of its
ancestor's task ids. A navigation's task id is TabNavigation's global
id when the navigation is first visited and then reused by going
back/forward visits. The task ids are created and sent to backend in
session sync.
And for now we only track navigation relationship in a tab.
BUG=707978
==========
to
==========
Add taskids for navigation, created in session sync.
This is the first step to introduce task in Chrome, which links and
groups navigations. Each navigation has a task id and a list of its
ancestor's task ids. A navigation's task id is TabNavigation's global
id when the navigation is first visited and then reused by going
back/forward visits. The task ids are created and sent to backend in
session sync.
And for now we only track navigation relationship in a tab.
BUG=707978
Review-Url: https://codereview.chromium.org/2776633003
Cr-Commit-Position: refs/heads/master@{#466188}
Committed:
https://chromium.googlesource.com/chromium/src/+/965fab94d2a8faef6f9ed4c6feaf...
==========
Message was sent while issue was closed.
Committed patchset #7 (id:260001) as https://chromium.googlesource.com/chromium/src/+/965fab94d2a8faef6f9ed4c6feaf... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
