Implement 1:N mapping from ios::ChromeBrowserState to TabModel.

Implement 1:N mapping from ios::ChromeBrowserState to TabModel.

In preparation of the removal of BrowserListIOS, implements
free function to maintain explicitly the 1:N mapping from
ios::ChromeBrowserState to TabModel.

BUG=None

Review-Url: https://codereview.chromium.org/2621083003
Cr-Commit-Position: refs/heads/master@{#442958}
Committed: https://chromium.googlesource.com/chromium/src/+/8cfdf8fa9481346a3324901efcd93e281e2e757a

[sdefresne, 2017-01-10 17:45:48]

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

[commit-bot: I haz the power, 2017-01-10 17:46:02]

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

[sdefresne, 2017-01-10 17:49:27]

sdefresne@chromium.org changed reviewers:
+ marq@chromium.org

[sdefresne, 2017-01-10 17:49:28]

Please take a look.

Note that the mapping owns a reference to the TabModel but since
-browserStateDestroyed needs to be called anyway, this should not be a problem.

If you want I can change the implementation of TabModelList to use a
std::vector<weak TabModel*> which will give us a weak list (though it will make
implementing the function returning the list slightly more complex).

https://codereview.chromium.org/2621083003/diff/1/ios/chrome/browser/tabs/tab...
File ios/chrome/browser/tabs/tab_model_unittest.mm (right):

https://codereview.chromium.org/2621083003/diff/1/ios/chrome/browser/tabs/tab...
ios/chrome/browser/tabs/tab_model_unittest.mm:150:
sessionWindow_.reset([[SessionWindowIOS alloc] init]);
nit: this is unrelated but I based my tab_model_list_unittest.mm on this file
and saw this use of -new that is forbidden according to the style guide and
decided to remove it.

[commit-bot: I haz the power, 2017-01-10 19:03:41]

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

[commit-bot: I haz the power, 2017-01-10 19:03:41]

Dry run: This issue passed the CQ dry run.

[sdefresne, 2017-01-11 15:46:22]

ping?

[marq (ping after 24h), 2017-01-11 15:56:47]

https://codereview.chromium.org/2621083003/diff/1/ios/chrome/browser/tabs/tab...
File ios/chrome/browser/tabs/tab_model_list.h (right):

https://codereview.chromium.org/2621083003/diff/1/ios/chrome/browser/tabs/tab...
ios/chrome/browser/tabs/tab_model_list.h:10: // This file contains free function
to help maintain 1:N relationship
"free functions to help maintain a 1:N relationship"

https://codereview.chromium.org/2621083003/diff/1/ios/chrome/browser/tabs/tab...
ios/chrome/browser/tabs/tab_model_list.h:20: // be retained until
|UnregisterTabModelFromChromeBrowserState| is called.
Add: "it is an error if |tab_model is already registered as associated to
|browser_state|".

https://codereview.chromium.org/2621083003/diff/1/ios/chrome/browser/tabs/tab...
File ios/chrome/browser/tabs/tab_model_list.mm (right):

https://codereview.chromium.org/2621083003/diff/1/ios/chrome/browser/tabs/tab...
ios/chrome/browser/tabs/tab_model_list.mm:30: NSMutableArray<TabModel*>*
tab_models() const { return tab_models_; }
Should this be a set instead of an array? That means there's no need to check
for duplicates, and GetTabModelsForBrowserState can just return allObjects from
the set.

https://codereview.chromium.org/2621083003/diff/1/ios/chrome/browser/tabs/tab...
File ios/chrome/browser/tabs/tab_model_unittest.mm (right):

https://codereview.chromium.org/2621083003/diff/1/ios/chrome/browser/tabs/tab...
ios/chrome/browser/tabs/tab_model_unittest.mm:150:
sessionWindow_.reset([[SessionWindowIOS alloc] init]);
On 2017/01/10 17:49:28, sdefresne wrote:
> nit: this is unrelated but I based my tab_model_list_unittest.mm on this file
> and saw this use of -new that is forbidden according to the style guide and
> decided to remove it.

Acknowledged.

[sdefresne, 2017-01-11 16:14:42]

I've not yet changed the implementation to use an NSSet<> as I don't know
whether keeping the order is important. If you feel it is not, then I'll make
the change (I'll still have to test presence of the object as the documentation
say it is an error is the registration happens twice or if the unregister is
called when no association exists though).

https://codereview.chromium.org/2621083003/diff/1/ios/chrome/browser/tabs/tab...
File ios/chrome/browser/tabs/tab_model_list.h (right):

https://codereview.chromium.org/2621083003/diff/1/ios/chrome/browser/tabs/tab...
ios/chrome/browser/tabs/tab_model_list.h:10: // This file contains free function
to help maintain 1:N relationship
On 2017/01/11 15:56:47, marq wrote:
> "free functions to help maintain a 1:N relationship"

Done.

https://codereview.chromium.org/2621083003/diff/1/ios/chrome/browser/tabs/tab...
ios/chrome/browser/tabs/tab_model_list.h:20: // be retained until
|UnregisterTabModelFromChromeBrowserState| is called.
On 2017/01/11 15:56:47, marq wrote:
> Add: "it is an error if |tab_model is already registered as associated to
> |browser_state|".

Done.

https://codereview.chromium.org/2621083003/diff/1/ios/chrome/browser/tabs/tab...
File ios/chrome/browser/tabs/tab_model_list.mm (right):

https://codereview.chromium.org/2621083003/diff/1/ios/chrome/browser/tabs/tab...
ios/chrome/browser/tabs/tab_model_list.mm:30: NSMutableArray<TabModel*>*
tab_models() const { return tab_models_; }
On 2017/01/11 15:56:47, marq wrote:
> Should this be a set instead of an array? That means there's no need to check
> for duplicates, and GetTabModelsForBrowserState can just return allObjects
from
> the set.

I need to be able to implement BrowserListIOS::GetLastActiveWithBrowserState()
using this function and thus somehow maintain the order of the inserted objects.
Unless we don't care about ordering and
BrowserListIOS::GetLastActiveWithBrowserState() can just return any TabModel*.

[marq (ping after 24h), 2017-01-11 16:20:52]

https://codereview.chromium.org/2621083003/diff/1/ios/chrome/browser/tabs/tab...
File ios/chrome/browser/tabs/tab_model_list.mm (right):

https://codereview.chromium.org/2621083003/diff/1/ios/chrome/browser/tabs/tab...
ios/chrome/browser/tabs/tab_model_list.mm:30: NSMutableArray<TabModel*>*
tab_models() const { return tab_models_; }
On 2017/01/11 16:14:42, sdefresne wrote:
> On 2017/01/11 15:56:47, marq wrote:
> > Should this be a set instead of an array? That means there's no need to
check
> > for duplicates, and GetTabModelsForBrowserState can just return allObjects
> from
> > the set.
> 
> I need to be able to implement BrowserListIOS::GetLastActiveWithBrowserState()
> using this function and thus somehow maintain the order of the inserted
objects.
> Unless we don't care about ordering and
> BrowserListIOS::GetLastActiveWithBrowserState() can just return any TabModel*.

Even if there's an array, we don't have any way of changing which was the last
active tab model; it will always be the last one added, right?

Since we don't expect there to be more than one tab model per browser any time
soon, I think that's OK. But GetLastActiveWithBrowserState()n should probably
also DCHECK that there's at most one browser state for now, and we can update it
when we are actually using multiples.

[sdefresne, 2017-01-11 16:34:12]

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

[commit-bot: I haz the power, 2017-01-11 16:34:40]

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

[sdefresne, 2017-01-11 16:35:36]

On 2017/01/11 16:20:52, marq wrote:
>
https://codereview.chromium.org/2621083003/diff/1/ios/chrome/browser/tabs/tab...
> File ios/chrome/browser/tabs/tab_model_list.mm (right):
> 
>
https://codereview.chromium.org/2621083003/diff/1/ios/chrome/browser/tabs/tab...
> ios/chrome/browser/tabs/tab_model_list.mm:30: NSMutableArray<TabModel*>*
> tab_models() const { return tab_models_; }
> On 2017/01/11 16:14:42, sdefresne wrote:
> > On 2017/01/11 15:56:47, marq wrote:
> > > Should this be a set instead of an array? That means there's no need to
> check
> > > for duplicates, and GetTabModelsForBrowserState can just return allObjects
> > from
> > > the set.
> > 
> > I need to be able to implement
BrowserListIOS::GetLastActiveWithBrowserState()
> > using this function and thus somehow maintain the order of the inserted
> objects.
> > Unless we don't care about ordering and
> > BrowserListIOS::GetLastActiveWithBrowserState() can just return any
TabModel*.
> 
> Even if there's an array, we don't have any way of changing which was the last
> active tab model; it will always be the last one added, right?
> 
> Since we don't expect there to be more than one tab model per browser any time
> soon, I think that's OK. But GetLastActiveWithBrowserState()n should probably
> also DCHECK that there's at most one browser state for now, and we can update
it
> when we are actually using multiples.

Done. PTAL.

[sdefresne, 2017-01-11 16:36:06]

On 2017/01/11 16:35:36, sdefresne wrote:
> On 2017/01/11 16:20:52, marq wrote:
> >
>
https://codereview.chromium.org/2621083003/diff/1/ios/chrome/browser/tabs/tab...
> > File ios/chrome/browser/tabs/tab_model_list.mm (right):
> > 
> >
>
https://codereview.chromium.org/2621083003/diff/1/ios/chrome/browser/tabs/tab...
> > ios/chrome/browser/tabs/tab_model_list.mm:30: NSMutableArray<TabModel*>*
> > tab_models() const { return tab_models_; }
> > On 2017/01/11 16:14:42, sdefresne wrote:
> > > On 2017/01/11 15:56:47, marq wrote:
> > > > Should this be a set instead of an array? That means there's no need to
> > check
> > > > for duplicates, and GetTabModelsForBrowserState can just return
allObjects
> > > from
> > > > the set.
> > > 
> > > I need to be able to implement
> BrowserListIOS::GetLastActiveWithBrowserState()
> > > using this function and thus somehow maintain the order of the inserted
> > objects.
> > > Unless we don't care about ordering and
> > > BrowserListIOS::GetLastActiveWithBrowserState() can just return any
> TabModel*.
> > 
> > Even if there's an array, we don't have any way of changing which was the
last
> > active tab model; it will always be the last one added, right?
> > 
> > Since we don't expect there to be more than one tab model per browser any
time
> > soon, I think that's OK. But GetLastActiveWithBrowserState()n should
probably
> > also DCHECK that there's at most one browser state for now, and we can
update
> it
> > when we are actually using multiples.
> 
> Done. PTAL.

BTW, I've not added a DCHECK in GetLastActiveWithBrowserState() as it would fail
during some of the unit tests.

[commit-bot: I haz the power, 2017-01-11 17:34:28]

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

[commit-bot: I haz the power, 2017-01-11 17:34:28]

Dry run: This issue passed the CQ dry run.

[marq (ping after 24h), 2017-01-11 18:00:29]

lgtm

[sdefresne, 2017-01-11 18:17:14]

The CQ bit was checked by sdefresne@chromium.org

[commit-bot: I haz the power, 2017-01-11 18:17:52]

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-01-11 18:22:19]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484158634266630,
"parent_rev": "c164a25463bbe2aee8e6dd4672b77afe16559ba7", "commit_rev":
"8cfdf8fa9481346a3324901efcd93e281e2e757a"}

[commit-bot: I haz the power, 2017-01-11 18:22:55]

Description was changed from

==========
Implement 1:N mapping from ios::ChromeBrowserState to TabModel.

In preparation of the removal of BrowserListIOS, implements
free function to maintain explicitly the 1:N mapping from
ios::ChromeBrowserState to TabModel.

BUG=None
==========

to

==========
Implement 1:N mapping from ios::ChromeBrowserState to TabModel.

In preparation of the removal of BrowserListIOS, implements
free function to maintain explicitly the 1:N mapping from
ios::ChromeBrowserState to TabModel.

BUG=None

Review-Url: https://codereview.chromium.org/2621083003
Cr-Commit-Position: refs/heads/master@{#442958}
Committed:
https://chromium.googlesource.com/chromium/src/+/8cfdf8fa9481346a3324901efcd9...
==========

[commit-bot: I haz the power, 2017-01-11 18:23:00]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/8cfdf8fa9481346a3324901efcd9...