Track task ids for navigations cross multiple tabs.

Track task ids for navigations cross multiple tabs.

This is a following step to generate task ids for navigations. Now we
can track navigation relationship cross multiple tabs. If navigation B
in tab2 is clicked from navigation A in tab1, then navigation A is B's
parent task. And this is generalized to more than 2 tabs.

BUG=707978
R=zea@chromium.org

Review-Url: https://codereview.chromium.org/2868043003
Cr-Commit-Position: refs/heads/master@{#472667}
Committed: https://chromium.googlesource.com/chromium/src/+/854b8bcfc787413293c86eddd979a3fd1936fb1a

[shenchao1, 2017-05-09 17:20:33]

shenchao@google.com changed reviewers:
+ shenchao@google.com

[shenchao1, 2017-05-09 17:20:34]

Hi Nicolas, 
PTAL. Thanks.

[shenchao, 2017-05-09 17:27:52]

shenchao@chromium.org changed reviewers:
- shenchao@google.com

[Nicolas Zea, 2017-05-10 00:10:56]

Nice! One main comment.

https://codereview.chromium.org/2868043003/diff/1/components/sync_sessions/ta...
File components/sync_sessions/task_tracker.h (right):

https://codereview.chromium.org/2868043003/diff/1/components/sync_sessions/ta...
components/sync_sessions/task_tracker.h:71: std::vector<int64_t>
source_tab_task_ids_;
Why store the source tab task in a separate vector? Why not just fill
|task_ids_| with the source tab's tasks?

[shenchao, 2017-05-10 17:22:23]

On 2017/05/10 00:10:56, Nicolas Zea wrote:
> Nice! One main comment.
> 
>
https://codereview.chromium.org/2868043003/diff/1/components/sync_sessions/ta...
> File components/sync_sessions/task_tracker.h (right):
> 
>
https://codereview.chromium.org/2868043003/diff/1/components/sync_sessions/ta...
> components/sync_sessions/task_tracker.h:71: std::vector<int64_t>
> source_tab_task_ids_;
> Why store the source tab task in a separate vector? Why not just fill
> |task_ids_| with the source tab's tasks?

I think having a separate vector is more easy:
1. We just need source tab task id, while task_ids_ contains TaskIdAndRoot where
root is not needed.
2. For now index of task_ids_ corresponds to navigation index of the tab (if no
task is excluded). If we let it also hold
source tab task id, its index become less straightforward.

Pro of using task_ids_ is that we can apply max task number. We can still apply
a separated max task number to source_tab_task_ids_ to make it bounded.
Although in this way we can't tell if ancestors are completed from size of
ancestor_task_id.

[Nicolas Zea, 2017-05-10 18:09:34]

On 2017/05/10 17:22:23, shenchao wrote:
> On 2017/05/10 00:10:56, Nicolas Zea wrote:
> > Nice! One main comment.
> > 
> >
>
https://codereview.chromium.org/2868043003/diff/1/components/sync_sessions/ta...
> > File components/sync_sessions/task_tracker.h (right):
> > 
> >
>
https://codereview.chromium.org/2868043003/diff/1/components/sync_sessions/ta...
> > components/sync_sessions/task_tracker.h:71: std::vector<int64_t>
> > source_tab_task_ids_;
> > Why store the source tab task in a separate vector? Why not just fill
> > |task_ids_| with the source tab's tasks?
> 
> I think having a separate vector is more easy:
> 1. We just need source tab task id, while task_ids_ contains TaskIdAndRoot
where
> root is not needed.
> 2. For now index of task_ids_ corresponds to navigation index of the tab (if
no
> task is excluded). If we let it also hold
> source tab task id, its index become less straightforward.
> 
> Pro of using task_ids_ is that we can apply max task number. We can still
apply
> a separated max task number to source_tab_task_ids_ to make it bounded.
> Although in this way we can't tell if ancestors are completed from size of
> ancestor_task_id.

Got it, makes sense. I think the main issue then is that the root to self task
ids are no longer going to be limitted to 100. If you do 99 within one tab, than
open a new tab from a link and do another 99, you can wind up with more than 100
tasks being sent to the server. We should ensure that the ancestor list is a max
of 100, regardless of how the ancestors are generated.

[shenchao, 2017-05-11 17:56:58]

On 2017/05/10 18:09:34, Nicolas Zea wrote:
> On 2017/05/10 17:22:23, shenchao wrote:
> > On 2017/05/10 00:10:56, Nicolas Zea wrote:
> > > Nice! One main comment.
> > > 
> > >
> >
>
https://codereview.chromium.org/2868043003/diff/1/components/sync_sessions/ta...
> > > File components/sync_sessions/task_tracker.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2868043003/diff/1/components/sync_sessions/ta...
> > > components/sync_sessions/task_tracker.h:71: std::vector<int64_t>
> > > source_tab_task_ids_;
> > > Why store the source tab task in a separate vector? Why not just fill
> > > |task_ids_| with the source tab's tasks?
> > 
> > I think having a separate vector is more easy:
> > 1. We just need source tab task id, while task_ids_ contains TaskIdAndRoot
> where
> > root is not needed.
> > 2. For now index of task_ids_ corresponds to navigation index of the tab (if
> no
> > task is excluded). If we let it also hold
> > source tab task id, its index become less straightforward.
> > 
> > Pro of using task_ids_ is that we can apply max task number. We can still
> apply
> > a separated max task number to source_tab_task_ids_ to make it bounded.
> > Although in this way we can't tell if ancestors are completed from size of
> > ancestor_task_id.
> 
> Got it, makes sense. I think the main issue then is that the root to self task
> ids are no longer going to be limitted to 100. If you do 99 within one tab,
than
> open a new tab from a link and do another 99, you can wind up with more than
100
> tasks being sent to the server. We should ensure that the ancestor list is a
max
> of 100, regardless of how the ancestors are generated.

Adding limit of task number from source tab. So the size of root to self task
ids is limited to 200.
Although it's not well defined when to apply the limit, i.e., if the size of
root to self task is 120, we don't know if the first id is root id or not, 
I think it's ok, because before this cl, we don't know if the first id is root
id is root or not from the size either. e.g.

after tasks reach max number, go back to an earlier navigation and fork some
tasks. These tasks have less than 100 ancestors but the root has been excluded
already.

[shenchao, 2017-05-11 17:58:41]

PTAL. Thanks

[Nicolas Zea, 2017-05-11 19:48:09]

The maximum task length really should be applied at the level of what we send to
the server. Introducing a second limit I think is going to make things
confusing, and it makes it difficult to reason about what we're actually
sending. I guess at this point you're changing the ancestor_task_ids length
limit to 200?

I'd much rather just have the total limit remain 100, as measured by root to
self (what would get sent in the proto).

[shenchao, 2017-05-15 21:03:11]

Patchset #3 (id:40001) has been deleted

[shenchao, 2017-05-15 21:04:45]

using task_ids_ to hold tasks from source tab as suggested. PTAL. Thanks

[Nicolas Zea, 2017-05-16 17:13:33]

LGTM with nits, thanks!

https://codereview.chromium.org/2868043003/diff/60001/components/sync_session...
File components/sync_sessions/task_tracker.cc (right):

https://codereview.chromium.org/2868043003/diff/60001/components/sync_session...
components/sync_sessions/task_tracker.cc:192: auto iter =
local_tab_tasks_map_.find(source_tab_id);
nit: maybe rename iter to source_tab_iter for clarity?

https://codereview.chromium.org/2868043003/diff/60001/components/sync_session...
File components/sync_sessions/task_tracker.h (right):

https://codereview.chromium.org/2868043003/diff/60001/components/sync_session...
components/sync_sessions/task_tracker.h:67: // Get position of task_ids_ for the
navigation at |navigation_index| of the
nit: position of -> position within

https://codereview.chromium.org/2868043003/diff/60001/components/sync_session...
components/sync_sessions/task_tracker.h:71: // Get navigation_index of
corresponding navigation of the tab at
nit: Get navigation_index of -> Get index of

[shenchao, 2017-05-17 16:48:16]

https://codereview.chromium.org/2868043003/diff/1/components/sync_sessions/ta...
File components/sync_sessions/task_tracker.h (right):

https://codereview.chromium.org/2868043003/diff/1/components/sync_sessions/ta...
components/sync_sessions/task_tracker.h:71: std::vector<int64_t>
source_tab_task_ids_;
On 2017/05/10 00:10:56, Nicolas Zea wrote:
> Why store the source tab task in a separate vector? Why not just fill
> |task_ids_| with the source tab's tasks?

Done.

https://codereview.chromium.org/2868043003/diff/60001/components/sync_session...
File components/sync_sessions/task_tracker.cc (right):

https://codereview.chromium.org/2868043003/diff/60001/components/sync_session...
components/sync_sessions/task_tracker.cc:192: auto iter =
local_tab_tasks_map_.find(source_tab_id);
On 2017/05/16 17:13:33, Nicolas Zea wrote:
> nit: maybe rename iter to source_tab_iter for clarity? 

Done.

https://codereview.chromium.org/2868043003/diff/60001/components/sync_session...
File components/sync_sessions/task_tracker.h (right):

https://codereview.chromium.org/2868043003/diff/60001/components/sync_session...
components/sync_sessions/task_tracker.h:67: // Get position of task_ids_ for the
navigation at |navigation_index| of the
On 2017/05/16 17:13:33, Nicolas Zea wrote:
> nit: position of -> position within

Done.

https://codereview.chromium.org/2868043003/diff/60001/components/sync_session...
components/sync_sessions/task_tracker.h:71: // Get navigation_index of
corresponding navigation of the tab at
On 2017/05/16 17:13:33, Nicolas Zea wrote:
> nit: Get navigation_index of -> Get index of

Done.

[shenchao, 2017-05-17 16:48:45]

The CQ bit was checked by shenchao@chromium.org

[shenchao, 2017-05-17 16:48:46]

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/2868043003/#ps80001
(title: "fixing nits")

[shenchao, 2017-05-17 16:49:17]

The CQ bit was unchecked by shenchao@chromium.org

[shenchao, 2017-05-17 16:49:26]

The CQ bit was checked by shenchao@chromium.org

[shenchao, 2017-05-17 16:49:30]

The CQ bit was unchecked by shenchao@chromium.org

[shenchao, 2017-05-17 16:49:52]

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

[shenchao, 2017-05-17 16:50:50]

The CQ bit was unchecked by shenchao@chromium.org

[shenchao, 2017-05-17 16:50:51]

The CQ bit was checked by shenchao@chromium.org

[commit-bot: I haz the power, 2017-05-17 16:51:07]

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-05-17 21:22:17]

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

[commit-bot: I haz the power, 2017-05-17 21:22:18]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[shenchao, 2017-05-17 21:40:10]

The CQ bit was checked by shenchao@chromium.org

[commit-bot: I haz the power, 2017-05-17 21:42:13]

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-05-18 04:43:04]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495057210135660,
"parent_rev": "9eaa9ffde680190e6c32965bf66ec92be1df7ef1", "commit_rev":
"854b8bcfc787413293c86eddd979a3fd1936fb1a"}

[commit-bot: I haz the power, 2017-05-18 04:43:16]

Description was changed from

==========
Track task ids for navigations cross multiple tabs.

This is a following step to generate task ids for navigations. Now we
can track navigation relationship cross multiple tabs. If navigation B
in tab2 is clicked from navigation A in tab1, then navigation A is B's
parent task. And this is generalized to more than 2 tabs.

BUG=707978
R=zea@chromium.org
==========

to

==========
Track task ids for navigations cross multiple tabs.

This is a following step to generate task ids for navigations. Now we
can track navigation relationship cross multiple tabs. If navigation B
in tab2 is clicked from navigation A in tab1, then navigation A is B's
parent task. And this is generalized to more than 2 tabs.

BUG=707978
R=zea@chromium.org

Review-Url: https://codereview.chromium.org/2868043003
Cr-Commit-Position: refs/heads/master@{#472667}
Committed:
https://chromium.googlesource.com/chromium/src/+/854b8bcfc787413293c86eddd979...
==========

[commit-bot: I haz the power, 2017-05-18 04:43:18]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/854b8bcfc787413293c86eddd979...

[Nicolas Zea, 2017-05-23 01:25:12]

A revert of this CL (patchset #4 id:80001) has been created in
https://codereview.chromium.org/2901533003/ by zea@chromium.org.

The reason for reverting is: crbug.com/725026 and crbug.com/724497.