Fix tab insertion position after moving tab or nesting openers

Fix tab insertion position after moving tab or nesting openers

Depends on https://codereview.chromium.org/800193006

BUG=446997, 446998
Committed: https://crrev.com/4e37c9fa01a1683bd1e3460864e6813c8ea0313b
Cr-Commit-Position: refs/heads/master@{#311729}

[johnme, 2015-01-07 22:50:26]

johnme@chromium.org changed reviewers:
+ sky@chromium.org

[sky, 2015-01-08 21:19:44]

Both of the bugs you reference are expected behavior. Openers are explicitly
forgotten on a move. This is by design.

[johnme, 2015-01-09 01:22:53]

On 2015/01/08 21:19:44, sky wrote:
> Both of the bugs you reference are expected behavior. Openers are explicitly
> forgotten on a move. This is by design.

Please re-read the bugs - in neither case do openers get forgotten at any point
in the steps to reproduce.

In crbug.com/446997, where a tab is moved, the problem is precisely caused by
the fact that openers are not forgotten, and hence the 2nd child tab ends up
next to the 1st child rather than next to the parent, even though the 1st child
has been moved far away.

And crbug.com/446998 doesn't involve tab moving, nor are any tabs activated
(selected) except going back and forth between an opener and its child, so
TabStripModelOrderController::ActiveTabChanged does not reset openers during
this either.

[sky, 2015-01-14 15:53:46]

sky@chromium.org changed reviewers:
+ pkasting@chromium.org

[sky, 2015-01-14 15:55:30]

Sorry for the delay in responding.

I'm tempted to think that moving a tab should forget it's opener. Currently that
isn't the case, but is another way to address 446997.

I'm adding Peter as he likely he has an opinion here. Peter, see 446997 for what
this is trying to fix.

[sky, 2015-01-14 15:56:30]

On 2015/01/14 15:55:30, sky wrote:
> Sorry for the delay in responding.
> 
> I'm tempted to think that moving a tab should forget it's opener. Currently
that
> isn't the case, but is another way to address 446997.
> 
> I'm adding Peter as he likely he has an opinion here. Peter, see 446997 for
what
> this is trying to fix.

Peter, you don't need to review the fix (although it's pretty small). The main
question I have is whether moving a tab should forget the opener. We do this
when navigating, but not moving.

[johnme, 2015-01-14 17:35:29]

On 2015/01/14 15:56:30, sky wrote:
> On 2015/01/14 15:55:30, sky wrote:
> > Sorry for the delay in responding.
> > 
> > I'm tempted to think that moving a tab should forget it's opener. Currently
> that
> > isn't the case, but is another way to address 446997.
> > 
> > I'm adding Peter as he likely he has an opinion here. Peter, see 446997 for
> what
> > this is trying to fix.
> 
> Peter, you don't need to review the fix (although it's pretty small). The main
> question I have is whether moving a tab should forget the opener. We do this
> when navigating, but not moving.

Forgetting all openers (as done by TabStripModel::TabNavigating) could be
overkill. For example a user might open search results in new background tabs,
swap the order of two of the results, then go back to the search results page
and open more tabs. I wouldn't consider the user to have started a new task in
this case.

However it would definitely make sense for moving a tab to adjust its opener
(moving it far away would forget the opener, but moving it in between two
sibling tabs should ideally give it the same opener as the siblings, etc).

I'm happy to implement that, and it'll singlehandedly fix 446997, but it won't
help with 446998. So ideally we'd still land this patch to fix 446998.

[Peter Kasting, 2015-01-14 19:57:36]

Regarding the bugs linked, after some thought, I think John is correct about
these being undesirable, and I think his proposed new behaviors are correct as
well.

I am not sure whether the implementation here is correct or not.  My primary
concern is about whether we could get a situation where we have many tabs open
due to a deep hierarchy tree:

1 2 2.1 2.1.1 2.1.2 2.2 2.2.1 2.2.1.1 2.2.1.1.1 2.3 (etc.)

...and then, with this patch, have newly opened tabs from "2" get placed after
the entire huge subtree.  I wonder if there's a point where the subtree gets so
big that the new tab position begins to appear random.  But I don't have a good
sense on whether that actually happens, or if so, what the heuristic is to
detect it and what alternate behavior makes more sense.

So I support the patch in this bug.  I think it's likely better than the current
behavior, at least.

We could really use someone beating on all possible cases of our tab opening
position heuristics.  It's been a long time since someone did so, and I imagine
there are more cases like this that could be improved.

[sky, 2015-01-14 20:55:09]

LGTM

[johnme, 2015-01-15 11:18:09]

The CQ bit was checked by johnme@chromium.org

[commit-bot: I haz the power, 2015-01-15 11:18:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/836223004/20001

[commit-bot: I haz the power, 2015-01-15 12:08:56]

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

[commit-bot: I haz the power, 2015-01-15 12:08:57]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[johnme, 2015-01-15 19:45:58]

The CQ bit was checked by johnme@chromium.org

[commit-bot: I haz the power, 2015-01-15 19:46:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/836223004/20001

[commit-bot: I haz the power, 2015-01-15 19:47:41]

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

[commit-bot: I haz the power, 2015-01-15 19:47:42]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  mac_chromium_rel_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[johnme, 2015-01-15 20:57:02]

The CQ bit was checked by johnme@chromium.org

[commit-bot: I haz the power, 2015-01-15 20:58:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/836223004/20001

[commit-bot: I haz the power, 2015-01-15 20:59:24]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-01-15 21:00:43]

Patchset 2 (id:??) landed as
https://crrev.com/4e37c9fa01a1683bd1e3460864e6813c8ea0313b
Cr-Commit-Position: refs/heads/master@{#311729}