Fix restoring individual tabs from recently closed windows.

Fix restoring individual tabs from recently closed windows.

`sessions::TabRestoreService::Tab`s get new ids when they're copied (see
the constructor at tab_restore_service.cc:40 — it delegates to the
`Entry` constructor at tab_restore_service.cc:40, which assigns a unique
id).

As a quick fix, I'm avoiding the copy. I'm planning to refactor
`sessions::TabRestoreService::Tab` and its consumers this week to get
rid of this trap.

BUG=585564
TEST=Close a window with multiple tabs. Try to restore one of them from
the history menu. With this change, it should happen.

Committed: https://crrev.com/c2aa85e42d6d85b15e46e82f2df516e2e896bd1b
Cr-Commit-Position: refs/heads/master@{#406281}

[Sidney San Martín, 2016-07-18 21:16:01]

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

[commit-bot: I haz the power, 2016-07-18 21:16:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sidney San Martín, 2016-07-18 21:17:28]

Description was changed from

==========
Fix restoring individual tabs from recently-closed windows.

`sessions::TabRestoreService::Tab` get new `id`s when they're copied
(see the constructor at tab_restore_service.cc:40 — it delegates to the
Entry constructor at tab_restore_service.cc:40, which assigns a unique
`id`). As a result, it's not safe to copy `Tab`s.

This is a quick fix, I'm planning to refactor
sessions::TabRestoreService::Tab to get rid of this behavior.

BUG=627170
TEST=Close a window with multiple tabs. Try to restore one of them from
the history menu. With this change, it should happen.
==========

to

==========
Fix restoring individual tabs from recently-closed windows.

`sessions::TabRestoreService::Tab` get new `id`s when they're copied
(see the constructor at tab_restore_service.cc:40 — it delegates to the
Entry constructor at tab_restore_service.cc:40, which assigns a unique
`id`). As a result, it's not safe to copy `Tab`s.

This is a quick fix, I'm planning to refactor
sessions::TabRestoreService::Tab to get rid of this behavior.

BUG=627170
TEST=Close a window with multiple tabs. Try to restore one of them from
the history menu. With this change, it should happen.
==========

[Sidney San Martín, 2016-07-18 21:17:28]

sdy@chromium.org changed reviewers:
+ rsesek@chromium.org

[Sidney San Martín, 2016-07-18 21:22:54]

rsesek@ — It might be good to add a test for this, which I've never done. Let me
know if you have a few minutes to help me get started.

Also let me know if I should pick more/different reviewers… You became my
default somehow :).

I'm going to start working on the "real" fix now. Since this is a P1 bug, I
guessed that it's better to land a minimal fix and then a more thorough one
later. Let me know if that's not right.

[Sidney San Martín, 2016-07-18 21:28:43]

Description was changed from

==========
Fix restoring individual tabs from recently-closed windows.

`sessions::TabRestoreService::Tab` get new `id`s when they're copied
(see the constructor at tab_restore_service.cc:40 — it delegates to the
Entry constructor at tab_restore_service.cc:40, which assigns a unique
`id`). As a result, it's not safe to copy `Tab`s.

This is a quick fix, I'm planning to refactor
sessions::TabRestoreService::Tab to get rid of this behavior.

BUG=627170
TEST=Close a window with multiple tabs. Try to restore one of them from
the history menu. With this change, it should happen.
==========

to

==========
Fix restoring individual tabs from recently closed windows.

`sessions::TabRestoreService::Tab`s get new ids when they're copied (see
the constructor at tab_restore_service.cc:40 — it delegates to the
`Entry` constructor at tab_restore_service.cc:40, which assigns a unique
id).

As a quick fix, I'm avoiding the copy. I'm planning to refactor
`sessions::TabRestoreService::Tab` and its consumers this week to get
rid of this trap.

BUG=585564

TEST=Close a window with multiple tabs. Try to restore one of them from
the history menu. With this change, it should happen.
==========

[Robert Sesek, 2016-07-18 21:29:20]

I am a good reviewer for this (I wrote the code initially ;-).

Wow that's a really bad/unintuitive copy ctor behavior. That should definitely
be changed.

I don't think this bug is Pri-1 (per my duping of the bug, so please also update
the BUG=). It should also be possible to write a test for this. See
https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/history_menu_bri...
for some examples, and let me know if you need help.

[Sidney San Martín, 2016-07-18 21:33:22]

Cool, on it.

[commit-bot: I haz the power, 2016-07-18 21:56:49]

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

[commit-bot: I haz the power, 2016-07-18 21:56:50]

Dry run: This issue passed the CQ dry run.

[Sidney San Martín, 2016-07-19 01:01:51]

Adding tests (actually just tweaking an existing test to check the ids of tabs
which are owned by windows) revealed further nastiness, but they work. I'll
clean them up on the second pass.

[Robert Sesek, 2016-07-19 14:51:19]

LGTM

[Sidney San Martín, 2016-07-19 15:02:16]

The CQ bit was checked by sdy@chromium.org

[commit-bot: I haz the power, 2016-07-19 15:02:34]

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, 2016-07-19 15:36:12]

Description was changed from

==========
Fix restoring individual tabs from recently closed windows.

`sessions::TabRestoreService::Tab`s get new ids when they're copied (see
the constructor at tab_restore_service.cc:40 — it delegates to the
`Entry` constructor at tab_restore_service.cc:40, which assigns a unique
id).

As a quick fix, I'm avoiding the copy. I'm planning to refactor
`sessions::TabRestoreService::Tab` and its consumers this week to get
rid of this trap.

BUG=585564

TEST=Close a window with multiple tabs. Try to restore one of them from
the history menu. With this change, it should happen.
==========

to

==========
Fix restoring individual tabs from recently closed windows.

`sessions::TabRestoreService::Tab`s get new ids when they're copied (see
the constructor at tab_restore_service.cc:40 — it delegates to the
`Entry` constructor at tab_restore_service.cc:40, which assigns a unique
id).

As a quick fix, I'm avoiding the copy. I'm planning to refactor
`sessions::TabRestoreService::Tab` and its consumers this week to get
rid of this trap.

BUG=585564

TEST=Close a window with multiple tabs. Try to restore one of them from
the history menu. With this change, it should happen.
==========

[commit-bot: I haz the power, 2016-07-19 15:36:13]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-07-19 15:36:27]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-19 15:38:01]

Description was changed from

==========
Fix restoring individual tabs from recently closed windows.

`sessions::TabRestoreService::Tab`s get new ids when they're copied (see
the constructor at tab_restore_service.cc:40 — it delegates to the
`Entry` constructor at tab_restore_service.cc:40, which assigns a unique
id).

As a quick fix, I'm avoiding the copy. I'm planning to refactor
`sessions::TabRestoreService::Tab` and its consumers this week to get
rid of this trap.

BUG=585564

TEST=Close a window with multiple tabs. Try to restore one of them from
the history menu. With this change, it should happen.
==========

to

==========
Fix restoring individual tabs from recently closed windows.

`sessions::TabRestoreService::Tab`s get new ids when they're copied (see
the constructor at tab_restore_service.cc:40 — it delegates to the
`Entry` constructor at tab_restore_service.cc:40, which assigns a unique
id).

As a quick fix, I'm avoiding the copy. I'm planning to refactor
`sessions::TabRestoreService::Tab` and its consumers this week to get
rid of this trap.

BUG=585564

TEST=Close a window with multiple tabs. Try to restore one of them from
the history menu. With this change, it should happen.

Committed: https://crrev.com/c2aa85e42d6d85b15e46e82f2df516e2e896bd1b
Cr-Commit-Position: refs/heads/master@{#406281}
==========

[commit-bot: I haz the power, 2016-07-19 15:38:02]

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