Make TabRestoreService::Entry noncopyable and fix up surrounding code.

Make TabRestoreService::Entry noncopyable and fix up surrounding code.

Instances of TabRestoreService::Entry (Window and Tab) were managed by
hand, and had special support for copying even though they never
actually need to be copied.

This refactor starts by making Entry noncopyable and branches out.

Bigger changes:
- Entry is noncopyable. PlatformSpecificTabData::Clone is gone.
- Entry is usually wrapped in unique_ptr, including inside collections.
- [Important] ValidateEntry/ValidateTab/ValidateWindow take const
  references and return a result. They don't try to fix damaged entries.

Smaller changes:
- Entry is taken by const reference whenever possible, not const pointer
  or non-const pointer/reference.
- ifs that check if an entry is a Tab and assume it's a Window otherwise
  are now switch statements.
- Loops that iterate over collections of Entry are range-based when
  appropriate.

BUG=633689
Committed: https://crrev.com/7710d22ccb2ffb236be1bc99d91e5ed5f7483a13
Cr-Commit-Position: refs/heads/master@{#410655}

[Sidney San Martín, 2016-08-02 21:10:38]

Description was changed from

==========
Make TabRestoreService::Entry noncopyable, fix up surrounding code

TODO(sdy): Write a description

BUG=633689
==========

to

==========
Make TabRestoreService::Entry noncopyable and fix up surrounding code.

Instances of TabRestoreService::Entry (Window and Tab) were managed by
hand, and had special support for copying even though they never
actually need to be copied.

This refactor starts by making Entry noncopyable and branches out.

Bigger changes:
- Entry is noncopyable. PlatformSpecificTabData::Clone is gone.
- Entry is usually wrapped in unique_ptr, including inside collections.
- RemoveEntryByID, which scanned the whole collection for nested tabs,
  is now a one-off class (RemoveByIDIndex).
- [Important] ValidateEntry/ValidateTab/ValidateWindow take const
  references and return a result. They don't try to fix damaged entries.

Smaller changes:
- Entry is taken by const reference whenever possible, not const pointer
  or non-const pointer/reference.
- type is a virtual member function, not a member variable.
- ifs that check if an entry is a Tab and assume it's a Window otherwise
  (or invoke DCHECK or NOTREACHED) are now switch statements.
- Loops that iterate over collections of Entry are range-based when
  appropriate.
- current_navigation_index is a size_t instead of an int, since it
  represents an index in a vector.

BUG=633689
==========

[Sidney San Martín, 2016-08-02 21:11:48]

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

[commit-bot: I haz the power, 2016-08-02 21:13:13]

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-08-02 21:15:00]

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

[commit-bot: I haz the power, 2016-08-02 21:22:16]

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

[commit-bot: I haz the power, 2016-08-02 21:22:17]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sidney San Martín, 2016-08-02 22:15:58]

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

[commit-bot: I haz the power, 2016-08-02 22:16:46]

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, 2016-08-02 22:27:58]

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

[commit-bot: I haz the power, 2016-08-02 22:27:59]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[sky, 2016-08-02 23:12:31]

https://codereview.chromium.org/2200993004/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/sessions/sessions_api.cc (right):

https://codereview.chromium.org/2200993004/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/sessions/sessions_api.cc:230: for (const auto
&entry : tab_restore_service->entries()) {
'auto &' -> 'auto&'

https://codereview.chromium.org/2200993004/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/history_menu_bridge.mm (right):

https://codereview.chromium.org/2200993004/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/history_menu_bridge.mm:141: if (added_count >=
kRecentlyClosedCount) {
no {}

https://codereview.chromium.org/2200993004/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/history_menu_bridge.mm:148: const auto &tabs =
entry_win.tabs;
'auto &' -> 'auto&' that said, in this case I would be inclined to not use auto
as it isn't readily obvious what the type is.

https://codereview.chromium.org/2200993004/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/history_menu_bridge.mm:180: for (const auto &tab : tabs)
{
'auto &' -> 'auto&'

https://codereview.chromium.org/2200993004/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/history_menu_bridge.mm:204: const auto &tab =
static_cast<sessions::TabRestoreService::Tab&>(*entry);
'auto &' -> 'auto&'

https://codereview.chromium.org/2200993004/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/history_menu_bridge_unittest.mm (right):

https://codereview.chromium.org/2200993004/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/history_menu_bridge_unittest.mm:127: for (auto* tab :
tabs) {
nit: no {}

https://codereview.chromium.org/2200993004/diff/60001/components/sessions/cor...
File components/sessions/core/tab_restore_service.h (right):

https://codereview.chromium.org/2200993004/diff/60001/components/sessions/cor...
components/sessions/core/tab_restore_service.h:57: virtual Type type() const =
0;
Virtual functions shouldn't use unix_hacker_style. That said, can you keep type
as a member with Entry(Type) the only constructor?

https://codereview.chromium.org/2200993004/diff/60001/components/sessions/cor...
components/sessions/core/tab_restore_service.h:69: bool
from_last_session{false};
Why the {} for many of these? = works just fine.

https://codereview.chromium.org/2200993004/diff/60001/components/sessions/cor...
components/sessions/core/tab_restore_service.h:85: size_t
current_navigation_index{-1};
Please keep these as ints, I don't want to have to worry about size_t
comparisons.

[Sidney San Martín, 2016-08-03 14:26:37]

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

[commit-bot: I haz the power, 2016-08-03 14:26:57]

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-08-03 14:35:32]

Changed most of these, waiting for clarification on a couple.

https://codereview.chromium.org/2200993004/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/sessions/sessions_api.cc (right):

https://codereview.chromium.org/2200993004/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/sessions/sessions_api.cc:230: for (const auto
&entry : tab_restore_service->entries()) {
On 2016/08/02 23:12:30, sky wrote:
> 'auto &' -> 'auto&'

Done. Sorry about these, I ran `git cl format` this time.

https://codereview.chromium.org/2200993004/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/history_menu_bridge.mm (right):

https://codereview.chromium.org/2200993004/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/history_menu_bridge.mm:141: if (added_count >=
kRecentlyClosedCount) {
On 2016/08/02 23:12:30, sky wrote:
> no {}

Done. Not used to this style yet :).

https://codereview.chromium.org/2200993004/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/history_menu_bridge.mm:148: const auto &tabs =
entry_win.tabs;
On 2016/08/02 23:12:30, sky wrote:
> 'auto &' -> 'auto&' that said, in this case I would be inclined to not use
auto
> as it isn't readily obvious what the type is.

Done. However, the full type name is a little long. Take a look at it in the new
patch set.

https://codereview.chromium.org/2200993004/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/history_menu_bridge_unittest.mm (right):

https://codereview.chromium.org/2200993004/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/history_menu_bridge_unittest.mm:127: for (auto* tab :
tabs) {
On 2016/08/02 23:12:30, sky wrote:
> nit: no {}

Done.

https://codereview.chromium.org/2200993004/diff/60001/components/sessions/cor...
File components/sessions/core/tab_restore_service.h (right):

https://codereview.chromium.org/2200993004/diff/60001/components/sessions/cor...
components/sessions/core/tab_restore_service.h:57: virtual Type type() const =
0;
On 2016/08/02 23:12:30, sky wrote:
> Virtual functions shouldn't use unix_hacker_style.

Just to confirm, the non-unix_hacker_style would just be Type()?

https://codereview.chromium.org/2200993004/diff/60001/components/sessions/cor...
components/sessions/core/tab_restore_service.h:57: virtual Type type() const =
0;
On 2016/08/02 23:12:30, sky wrote:
> That said, can you keep type as a member with Entry(Type) the only
> constructor?

Could you fill me in on why (I'm still new on the team)? Since every instance
will be a specific subclass of Entry, I thought a member func made more sense. I
can change it back if you disagree.

https://codereview.chromium.org/2200993004/diff/60001/components/sessions/cor...
components/sessions/core/tab_restore_service.h:69: bool
from_last_session{false};
On 2016/08/02 23:12:30, sky wrote:
> Why the {} for many of these? = works just fine.

I do uniform initialization because it doesn't allow narrowing, works for any
kind of initialization, and doesn't trigger the Most Vexing Parse. Consistently
using it means I don't have to think about when `=` and `()` are safe.

What makes `=` preferable? It looks like C++ Dos and Don'ts advises against
using it for "simple" initialization, with the justification that Google's style
guide says the same thing… but it doesn't say that anymore, as far as I can
tell.

If you think it's particularly weird, I'm happy to change it.

https://www.chromium.org/developers/coding-style/cpp-dos-and-donts?pli=1#TOC-...
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/GF96FshwHLw
https://google.github.io/styleguide/cppguide.html#Variable_and_Array_Initiali...

https://codereview.chromium.org/2200993004/diff/60001/components/sessions/cor...
components/sessions/core/tab_restore_service.h:85: size_t
current_navigation_index{-1};
On 2016/08/02 23:12:30, sky wrote:
> Please keep these as ints, I don't want to have to worry about size_t
> comparisons.

Done. For future reference, what issues come up around this? I changed it to get
rid of some casts, and the style guide suggests using size_t to represent
indices.

[commit-bot: I haz the power, 2016-08-03 14:41:19]

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

[commit-bot: I haz the power, 2016-08-03 14:41:20]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sky, 2016-08-03 15:16:08]

https://codereview.chromium.org/2200993004/diff/60001/components/sessions/cor...
File components/sessions/core/tab_restore_service.h (right):

https://codereview.chromium.org/2200993004/diff/60001/components/sessions/cor...
components/sessions/core/tab_restore_service.h:57: virtual Type type() const =
0;
On 2016/08/03 14:35:32, Sidney San Martín wrote:
> On 2016/08/02 23:12:30, sky wrote:
> > That said, can you keep type as a member with Entry(Type) the only
> > constructor?
> 
> Could you fill me in on why (I'm still new on the team)? Since every instance
> will be a specific subclass of Entry, I thought a member func made more sense.
I
> can change it back if you disagree.

Expressing the type in the constructor implies the type isn't going to change. A
virtual function has no such contract. Additionally a virtual function is more
expensive.

https://codereview.chromium.org/2200993004/diff/60001/components/sessions/cor...
components/sessions/core/tab_restore_service.h:69: bool
from_last_session{false};
On 2016/08/03 14:35:32, Sidney San Martín wrote:
> On 2016/08/02 23:12:30, sky wrote:
> > Why the {} for many of these? = works just fine.
> 
> I do uniform initialization because it doesn't allow narrowing, works for any
> kind of initialization, and doesn't trigger the Most Vexing Parse.
Consistently
> using it means I don't have to think about when `=` and `()` are safe.
> 
> What makes `=` preferable? It looks like C++ Dos and Don'ts advises against
> using it for "simple" initialization, with the justification that Google's
style
> guide says the same thing… but it doesn't say that anymore, as far as I can
> tell.
> 
> If you think it's particularly weird, I'm happy to change it.
> 
>
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts?pli=1#TOC-...
>
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/GF96FshwHLw
>
https://google.github.io/styleguide/cppguide.html#Variable_and_Array_Initiali...

"Use C++11 "uniform init" syntax ("{}" without '=') only when neither of the
above work:" where the above includes the '='. I take that to mean prefer '='.

https://codereview.chromium.org/2200993004/diff/60001/components/sessions/cor...
components/sessions/core/tab_restore_service.h:85: size_t
current_navigation_index{-1};
On 2016/08/03 14:35:32, Sidney San Martín wrote:
> On 2016/08/02 23:12:30, sky wrote:
> > Please keep these as ints, I don't want to have to worry about size_t
> > comparisons.
> 
> Done. For future reference, what issues come up around this? I changed it to
get
> rid of some casts, and the style guide suggests using size_t to represent
> indices.

My concern is around overflow and special casing -1.

[Sidney San Martín, 2016-08-03 15:50:23]

https://codereview.chromium.org/2200993004/diff/60001/components/sessions/cor...
File components/sessions/core/tab_restore_service.h (right):

https://codereview.chromium.org/2200993004/diff/60001/components/sessions/cor...
components/sessions/core/tab_restore_service.h:57: virtual Type type() const =
0;
On 2016/08/03 15:16:07, sky wrote:
> Expressing the type in the constructor implies the type isn't going to change.
A
> virtual function has no such contract. Additionally a virtual function is more
> expensive.

OK. Can I make the member const, and the constructor protected?

[Sidney San Martín, 2016-08-03 18:50:33]

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

[commit-bot: I haz the power, 2016-08-03 18:51:02]

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, 2016-08-03 19:01:15]

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

[commit-bot: I haz the power, 2016-08-03 19:01:16]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[Sidney San Martín, 2016-08-03 19:19:15]

Patchset #8 (id:140001) has been deleted

[Sidney San Martín, 2016-08-03 19:28:55]

Patchset #8 (id:160001) has been deleted

[Sidney San Martín, 2016-08-03 19:35:22]

Description was changed from

==========
Make TabRestoreService::Entry noncopyable and fix up surrounding code.

Instances of TabRestoreService::Entry (Window and Tab) were managed by
hand, and had special support for copying even though they never
actually need to be copied.

This refactor starts by making Entry noncopyable and branches out.

Bigger changes:
- Entry is noncopyable. PlatformSpecificTabData::Clone is gone.
- Entry is usually wrapped in unique_ptr, including inside collections.
- RemoveEntryByID, which scanned the whole collection for nested tabs,
  is now a one-off class (RemoveByIDIndex).
- [Important] ValidateEntry/ValidateTab/ValidateWindow take const
  references and return a result. They don't try to fix damaged entries.

Smaller changes:
- Entry is taken by const reference whenever possible, not const pointer
  or non-const pointer/reference.
- type is a virtual member function, not a member variable.
- ifs that check if an entry is a Tab and assume it's a Window otherwise
  (or invoke DCHECK or NOTREACHED) are now switch statements.
- Loops that iterate over collections of Entry are range-based when
  appropriate.
- current_navigation_index is a size_t instead of an int, since it
  represents an index in a vector.

BUG=633689
==========

to

==========
Make TabRestoreService::Entry noncopyable and fix up surrounding code.

Instances of TabRestoreService::Entry (Window and Tab) were managed by
hand, and had special support for copying even though they never
actually need to be copied.

This refactor starts by making Entry noncopyable and branches out.

Bigger changes:
- Entry is noncopyable. PlatformSpecificTabData::Clone is gone.
- Entry is usually wrapped in unique_ptr, including inside collections.
- RemoveEntryByID, which scanned the whole collection for nested tabs,
  is now a one-off class (RemoveByIDIndex).
- [Important] ValidateEntry/ValidateTab/ValidateWindow take const
  references and return a result. They don't try to fix damaged entries.

Smaller changes:
- Entry is taken by const reference whenever possible, not const pointer
  or non-const pointer/reference.
- ifs that check if an entry is a Tab and assume it's a Window otherwise
  are now switch statements.
- Loops that iterate over collections of Entry are range-based when
  appropriate.

BUG=633689
==========

[sky, 2016-08-03 19:36:15]

On Wed, Aug 3, 2016 at 8:50 AM,  <sdy@chromium.org> wrote:
>
>
https://codereview.chromium.org/2200993004/diff/60001/components/sessions/cor...
> File components/sessions/core/tab_restore_service.h (right):
>
>
https://codereview.chromium.org/2200993004/diff/60001/components/sessions/cor...
> components/sessions/core/tab_restore_service.h:57: virtual Type type()
> const = 0;
> On 2016/08/03 15:16:07, sky wrote:
>> Expressing the type in the constructor implies the type isn't going to
> change. A
>> virtual function has no such contract. Additionally a virtual function
> is more
>> expensive.
>
> OK. Can I make the member const, and the constructor protected?

SGTM.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Sidney San Martín, 2016-08-03 19:48:33]

Patchset #8 (id:180001) has been deleted

[Sidney San Martín, 2016-08-03 20:25:02]

Patchset #7 (id:120001) has been deleted

[Sidney San Martín, 2016-08-03 20:41:17]

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

[commit-bot: I haz the power, 2016-08-03 20:41:47]

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, 2016-08-03 21:23:30]

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

[commit-bot: I haz the power, 2016-08-03 21:23:31]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Sidney San Martín, 2016-08-03 22:24:33]

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

[commit-bot: I haz the power, 2016-08-03 22:25:34]

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, 2016-08-03 23:45:22]

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

[commit-bot: I haz the power, 2016-08-03 23:45:23]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Sidney San Martín, 2016-08-04 04:24:57]

This should address your comments so far, and I got rid of a few unnecessary
changes.

https://codereview.chromium.org/2200993004/diff/60001/components/sessions/cor...
File components/sessions/core/tab_restore_service.h (right):

https://codereview.chromium.org/2200993004/diff/60001/components/sessions/cor...
components/sessions/core/tab_restore_service.h:69: bool
from_last_session{false};
On 2016/08/03 15:16:07, sky wrote:
>
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts?pli=1#TOC-...
>
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/GF96FshwHLw
>
https://google.github.io/styleguide/cppguide.html#Variable_and_Array_Initiali...
> 
> "Use C++11 "uniform init" syntax ("{}" without '=') only when neither of the
> above work:" where the above includes the '='. I take that to mean prefer '='.

Done. I misread the mailing list thread — I thought that quote was based on an
old version of the Google style guide, but it comes from go/totw:88, which still
makes that recommendation. The justification is:

- Ambiguity when an initializer list constructor and another constructor both
apply.
- "no other common language uses something like it"

Fair enough.

https://codereview.chromium.org/2200993004/diff/60001/components/sessions/cor...
components/sessions/core/tab_restore_service.h:85: size_t
current_navigation_index{-1};
On 2016/08/03 15:16:07, sky wrote:
> My concern is around overflow and special casing -1.

Done.

https://codereview.chromium.org/2200993004/diff/220001/components/sessions/co...
File components/sessions/core/tab_restore_service_helper.cc (right):

https://codereview.chromium.org/2200993004/diff/220001/components/sessions/co...
components/sessions/core/tab_restore_service_helper.cc:226: }
I had a use-after free here, so I'm using a block to take `tab` out of scope
when it's erased from `window.tabs`. `tab_i` also becomes invalid here, but I
feel like it should already get more scrutiny. Right after I uploaded this patch
set, rsesek@ noted that a comment might be sufficient, but said to leave it up
to you.

[sky, 2016-08-04 16:34:39]

https://codereview.chromium.org/2200993004/diff/220001/chrome/browser/extensi...
File chrome/browser/extensions/api/sessions/sessions_api.cc (right):

https://codereview.chromium.org/2200993004/diff/220001/chrome/browser/extensi...
chrome/browser/extensions/api/sessions/sessions_api.cc:153: int session_id,
While you're cleaning up it seems like session_id is always tab.id. Is that
right? If so, how about removing it.

https://codereview.chromium.org/2200993004/diff/220001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/history_menu_bridge_unittest.mm (right):

https://codereview.chromium.org/2200993004/diff/220001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/history_menu_bridge_unittest.mm:103: for (auto* entry :
entries) {
no {}

https://codereview.chromium.org/2200993004/diff/220001/components/sessions/co...
File components/sessions/core/persistent_tab_restore_service.cc (right):

https://codereview.chromium.org/2200993004/diff/220001/components/sessions/co...
components/sessions/core/persistent_tab_restore_service.cc:568: class
RemoveByIDIndex {
This code is far too subtle. Please document it all.

https://codereview.chromium.org/2200993004/diff/220001/components/sessions/co...
components/sessions/core/persistent_tab_restore_service.cc:569:
std::unordered_map<SessionID::id_type, std::function<void()>> index_;
public section is first, then private.

https://codereview.chromium.org/2200993004/diff/220001/components/sessions/co...
components/sessions/core/persistent_tab_restore_service.cc:575:
index_[(*entry)->id] = [=]() {
Oy, this is hard to parse. Please be explicit about what you are capturing.

https://codereview.chromium.org/2200993004/diff/220001/components/sessions/co...
components/sessions/core/persistent_tab_restore_service.cc:577: if (it !=
collection->end()) {
no {}

https://codereview.chromium.org/2200993004/diff/220001/components/sessions/co...
components/sessions/core/persistent_tab_restore_service.cc:585: if (it !=
index_.end()) {
no {}

https://codereview.chromium.org/2200993004/diff/220001/components/sessions/co...
components/sessions/core/persistent_tab_restore_service.cc:586: it->second();
This is subtle and woth a comment.

https://codereview.chromium.org/2200993004/diff/220001/components/sessions/co...
File components/sessions/core/tab_restore_service.cc (right):

https://codereview.chromium.org/2200993004/diff/220001/components/sessions/co...
components/sessions/core/tab_restore_service.cc:21:
TabRestoreService::Tab::Tab() : Entry(TAB){};
'(TAB){};' -> '(TAB) {}'

https://codereview.chromium.org/2200993004/diff/220001/components/sessions/co...
components/sessions/core/tab_restore_service.cc:24:
TabRestoreService::Window::Window() : Entry(WINDOW){};
Same comment as above here.

[Sidney San Martín, 2016-08-04 17:04:50]

https://codereview.chromium.org/2200993004/diff/220001/components/sessions/co...
File components/sessions/core/persistent_tab_restore_service.cc (right):

https://codereview.chromium.org/2200993004/diff/220001/components/sessions/co...
components/sessions/core/persistent_tab_restore_service.cc:568: class
RemoveByIDIndex {
On 2016/08/04 16:34:39, sky wrote:
> This code is far too subtle. Please document it all.

Can you think of a simpler way to model this? I may have missed something
obvious. The problem is "The below function needs to be able to remove an Entry
from a collection given an ID, and the collection may be either a
std::list<unique_ptr<Entry>> or a std::vector<unique_ptr<Tab>>".

https://codereview.chromium.org/2200993004/diff/220001/components/sessions/co...
File components/sessions/core/tab_restore_service.cc (right):

https://codereview.chromium.org/2200993004/diff/220001/components/sessions/co...
components/sessions/core/tab_restore_service.cc:21:
TabRestoreService::Tab::Tab() : Entry(TAB){};
On 2016/08/04 16:34:39, sky wrote:
> '(TAB){};' -> '(TAB) {}'

Interestingly enough, `git cl format` did this. Should I file a bug?

[sky, 2016-08-04 19:09:52]

https://codereview.chromium.org/2200993004/diff/220001/components/sessions/co...
File components/sessions/core/persistent_tab_restore_service.cc (right):

https://codereview.chromium.org/2200993004/diff/220001/components/sessions/co...
components/sessions/core/persistent_tab_restore_service.cc:568: class
RemoveByIDIndex {
On 2016/08/04 17:04:50, Sidney San Martín wrote:
> On 2016/08/04 16:34:39, sky wrote:
> > This code is far too subtle. Please document it all.
> 
> Can you think of a simpler way to model this? I may have missed something
> obvious. The problem is "The below function needs to be able to remove an
Entry
> from a collection given an ID, and the collection may be either a
> std::list<unique_ptr<Entry>> or a std::vector<unique_ptr<Tab>>".

How about changing Entries to a std::vector? TabRestoreService is hardly
performance critical. I can't imagine converting to a list matters much.

https://codereview.chromium.org/2200993004/diff/220001/components/sessions/co...
File components/sessions/core/tab_restore_service.cc (right):

https://codereview.chromium.org/2200993004/diff/220001/components/sessions/co...
components/sessions/core/tab_restore_service.cc:21:
TabRestoreService::Tab::Tab() : Entry(TAB){};
On 2016/08/04 17:04:50, Sidney San Martín wrote:
> On 2016/08/04 16:34:39, sky wrote:
> > '(TAB){};' -> '(TAB) {}'
> 
> Interestingly enough, `git cl format` did this. Should I file a bug?

Interesting indeed. Yes, go ahead and file a bug.

[Sidney San Martín, 2016-08-04 20:17:19]

https://codereview.chromium.org/2200993004/diff/220001/components/sessions/co...
File components/sessions/core/persistent_tab_restore_service.cc (right):

https://codereview.chromium.org/2200993004/diff/220001/components/sessions/co...
components/sessions/core/persistent_tab_restore_service.cc:568: class
RemoveByIDIndex {
On 2016/08/04 19:09:52, sky wrote:
> How about changing Entries to a std::vector? TabRestoreService is hardly
> performance critical. I can't imagine converting to a list matters much.

I like this idea in general, but it doesn't solve the problem: they're still
different types.

https://codereview.chromium.org/2200993004/diff/220001/components/sessions/co...
File components/sessions/core/tab_restore_service.cc (right):

https://codereview.chromium.org/2200993004/diff/220001/components/sessions/co...
components/sessions/core/tab_restore_service.cc:21:
TabRestoreService::Tab::Tab() : Entry(TAB){};
On 2016/08/04 19:09:52, sky wrote:
> Interesting indeed. Yes, go ahead and file a bug.

Oh, it's my goof. The semicolon convinces it to remove the space. Will fix.

[sky, 2016-08-04 22:57:12]

https://codereview.chromium.org/2200993004/diff/220001/components/sessions/co...
File components/sessions/core/persistent_tab_restore_service.cc (right):

https://codereview.chromium.org/2200993004/diff/220001/components/sessions/co...
components/sessions/core/persistent_tab_restore_service.cc:568: class
RemoveByIDIndex {
On 2016/08/04 20:17:19, Sidney San Martín wrote:
> On 2016/08/04 19:09:52, sky wrote:
> > How about changing Entries to a std::vector? TabRestoreService is hardly
> > performance critical. I can't imagine converting to a list matters much.
> 
> I like this idea in general, but it doesn't solve the problem: they're still
> different types.

I prefer the old code, while not as efficient it's readable. A map to functions
with templates is very cryptic.

[Sidney San Martín, 2016-08-08 21:40:32]

Patchset #9 (id:240001) has been deleted

[Sidney San Martín, 2016-08-09 03:52:33]

Patchset #10 (id:250027) has been deleted

[Sidney San Martín, 2016-08-09 03:59:14]

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

[commit-bot: I haz the power, 2016-08-09 03:59:30]

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-08-09 04:10:53]

Addressed this round of comments. Take a look when you have a chance!

https://codereview.chromium.org/2200993004/diff/220001/chrome/browser/extensi...
File chrome/browser/extensions/api/sessions/sessions_api.cc (right):

https://codereview.chromium.org/2200993004/diff/220001/chrome/browser/extensi...
chrome/browser/extensions/api/sessions/sessions_api.cc:153: int session_id,
On 2016/08/04 16:34:38, sky wrote:
> While you're cleaning up it seems like session_id is always tab.id. Is that
> right? If so, how about removing it.

Done! I also made a couple of other small changes here, LMK how they look.

https://codereview.chromium.org/2200993004/diff/220001/components/sessions/co...
File components/sessions/core/persistent_tab_restore_service.cc (right):

https://codereview.chromium.org/2200993004/diff/220001/components/sessions/co...
components/sessions/core/persistent_tab_restore_service.cc:568: class
RemoveByIDIndex {
On 2016/08/04 22:57:12, sky wrote:
> I prefer the old code, while not as efficient it's readable. A map to
functions
> with templates is very cryptic.

OK, agreed. I still want to find a simple and efficient way to model this, but
I'll save it for another CL.

For now, how does this change look? Finding entries was already linear, so I
dropped `id_to_entry`.

[commit-bot: I haz the power, 2016-08-09 05:22:18]

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

[commit-bot: I haz the power, 2016-08-09 05:22:19]

Dry run: This issue passed the CQ dry run.

[sky, 2016-08-09 13:29:30]

LGTM - nice cleanup!

[Sidney San Martín, 2016-08-09 13:56:27]

Description was changed from

==========
Make TabRestoreService::Entry noncopyable and fix up surrounding code.

Instances of TabRestoreService::Entry (Window and Tab) were managed by
hand, and had special support for copying even though they never
actually need to be copied.

This refactor starts by making Entry noncopyable and branches out.

Bigger changes:
- Entry is noncopyable. PlatformSpecificTabData::Clone is gone.
- Entry is usually wrapped in unique_ptr, including inside collections.
- RemoveEntryByID, which scanned the whole collection for nested tabs,
  is now a one-off class (RemoveByIDIndex).
- [Important] ValidateEntry/ValidateTab/ValidateWindow take const
  references and return a result. They don't try to fix damaged entries.

Smaller changes:
- Entry is taken by const reference whenever possible, not const pointer
  or non-const pointer/reference.
- ifs that check if an entry is a Tab and assume it's a Window otherwise
  are now switch statements.
- Loops that iterate over collections of Entry are range-based when
  appropriate.

BUG=633689
==========

to

==========
Make TabRestoreService::Entry noncopyable and fix up surrounding code.

Instances of TabRestoreService::Entry (Window and Tab) were managed by
hand, and had special support for copying even though they never
actually need to be copied.

This refactor starts by making Entry noncopyable and branches out.

Bigger changes:
- Entry is noncopyable. PlatformSpecificTabData::Clone is gone.
- Entry is usually wrapped in unique_ptr, including inside collections.
- [Important] ValidateEntry/ValidateTab/ValidateWindow take const
  references and return a result. They don't try to fix damaged entries.

Smaller changes:
- Entry is taken by const reference whenever possible, not const pointer
  or non-const pointer/reference.
- ifs that check if an entry is a Tab and assume it's a Window otherwise
  are now switch statements.
- Loops that iterate over collections of Entry are range-based when
  appropriate.

BUG=633689
==========

[Sidney San Martín, 2016-08-09 14:00:22]

The CQ bit was checked by sdy@chromium.org

[commit-bot: I haz the power, 2016-08-09 14:00:46]

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-08-09 14:04:49]

Description was changed from

==========
Make TabRestoreService::Entry noncopyable and fix up surrounding code.

Instances of TabRestoreService::Entry (Window and Tab) were managed by
hand, and had special support for copying even though they never
actually need to be copied.

This refactor starts by making Entry noncopyable and branches out.

Bigger changes:
- Entry is noncopyable. PlatformSpecificTabData::Clone is gone.
- Entry is usually wrapped in unique_ptr, including inside collections.
- [Important] ValidateEntry/ValidateTab/ValidateWindow take const
  references and return a result. They don't try to fix damaged entries.

Smaller changes:
- Entry is taken by const reference whenever possible, not const pointer
  or non-const pointer/reference.
- ifs that check if an entry is a Tab and assume it's a Window otherwise
  are now switch statements.
- Loops that iterate over collections of Entry are range-based when
  appropriate.

BUG=633689
==========

to

==========
Make TabRestoreService::Entry noncopyable and fix up surrounding code.

Instances of TabRestoreService::Entry (Window and Tab) were managed by
hand, and had special support for copying even though they never
actually need to be copied.

This refactor starts by making Entry noncopyable and branches out.

Bigger changes:
- Entry is noncopyable. PlatformSpecificTabData::Clone is gone.
- Entry is usually wrapped in unique_ptr, including inside collections.
- [Important] ValidateEntry/ValidateTab/ValidateWindow take const
  references and return a result. They don't try to fix damaged entries.

Smaller changes:
- Entry is taken by const reference whenever possible, not const pointer
  or non-const pointer/reference.
- ifs that check if an entry is a Tab and assume it's a Window otherwise
  are now switch statements.
- Loops that iterate over collections of Entry are range-based when
  appropriate.

BUG=633689
==========

[commit-bot: I haz the power, 2016-08-09 14:04:50]

Committed patchset #10 (id:290001)

[commit-bot: I haz the power, 2016-08-09 14:06:57]

Description was changed from

==========
Make TabRestoreService::Entry noncopyable and fix up surrounding code.

Instances of TabRestoreService::Entry (Window and Tab) were managed by
hand, and had special support for copying even though they never
actually need to be copied.

This refactor starts by making Entry noncopyable and branches out.

Bigger changes:
- Entry is noncopyable. PlatformSpecificTabData::Clone is gone.
- Entry is usually wrapped in unique_ptr, including inside collections.
- [Important] ValidateEntry/ValidateTab/ValidateWindow take const
  references and return a result. They don't try to fix damaged entries.

Smaller changes:
- Entry is taken by const reference whenever possible, not const pointer
  or non-const pointer/reference.
- ifs that check if an entry is a Tab and assume it's a Window otherwise
  are now switch statements.
- Loops that iterate over collections of Entry are range-based when
  appropriate.

BUG=633689
==========

to

==========
Make TabRestoreService::Entry noncopyable and fix up surrounding code.

Instances of TabRestoreService::Entry (Window and Tab) were managed by
hand, and had special support for copying even though they never
actually need to be copied.

This refactor starts by making Entry noncopyable and branches out.

Bigger changes:
- Entry is noncopyable. PlatformSpecificTabData::Clone is gone.
- Entry is usually wrapped in unique_ptr, including inside collections.
- [Important] ValidateEntry/ValidateTab/ValidateWindow take const
  references and return a result. They don't try to fix damaged entries.

Smaller changes:
- Entry is taken by const reference whenever possible, not const pointer
  or non-const pointer/reference.
- ifs that check if an entry is a Tab and assume it's a Window otherwise
  are now switch statements.
- Loops that iterate over collections of Entry are range-based when
  appropriate.

BUG=633689

Committed: https://crrev.com/7710d22ccb2ffb236be1bc99d91e5ed5f7483a13
Cr-Commit-Position: refs/heads/master@{#410655}
==========

[commit-bot: I haz the power, 2016-08-09 14:06:59]

Patchset 10 (id:??) landed as
https://crrev.com/7710d22ccb2ffb236be1bc99d91e5ed5f7483a13
Cr-Commit-Position: refs/heads/master@{#410655}