cc: Switch ListContainer to use composition instead of inheritance.

cc: Switch ListContainer to use composition instead of inheritance.

This patch switches ListContainer to be a non-derived type, and instead
uses the base (which is renamed to helper) as a member. This allows
us to add other ListContainer-like types without the worry that they
will be somehow passed around as base classes.

R=danakj, weiliangc
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/988b5dee7b043fb1ae72bd64aac5b278f91b4146
Cr-Commit-Position: refs/heads/master@{#348925}

[vmpstr, 2015-09-11 20:34:48]

Please take a look.

[weiliangc, 2015-09-14 19:57:13]

Like the idea of composition instead of inheritance. Not too sure about friend
class.

https://codereview.chromium.org/1336763006/diff/20001/cc/base/list_container.h
File cc/base/list_container.h (right):

https://codereview.chromium.org/1336763006/diff/20001/cc/base/list_container....
cc/base/list_container.h:30: friend class CC_EXPORT ListContainer;
I'm kinda torn on the friend idea. Maybe stop CC_EXPORT-ing Helper class is
enough? 

Just that putting entire public interface into private w/ friend seems like
exact same level of data encapsulation?

[vmpstr, 2015-09-14 20:05:07]

https://codereview.chromium.org/1336763006/diff/20001/cc/base/list_container.h
File cc/base/list_container.h (right):

https://codereview.chromium.org/1336763006/diff/20001/cc/base/list_container....
cc/base/list_container.h:30: friend class CC_EXPORT ListContainer;
On 2015/09/14 19:57:13, weiliangc wrote:
> I'm kinda torn on the friend idea. Maybe stop CC_EXPORT-ing Helper class is
> enough? 

I'm not really sure. Wouldn't that still allow any cc code to create and use the
helper directly?

> 
> Just that putting entire public interface into private w/ friend seems like
> exact same level of data encapsulation?

Ya, I intended to keep the same level of encapsulation, since I don't want to
expose ListContainerHelper functionality to other classes other than the ones
explicitly specified. 

Are you proposing making the helper's functionality public and then removing
CC_EXPORT or keeping the inheritance and removing CC_EXPORT?

[hendrikw, 2015-09-14 20:10:02]

On 2015/09/14 20:05:07, vmpstr wrote:
> https://codereview.chromium.org/1336763006/diff/20001/cc/base/list_container.h
> File cc/base/list_container.h (right):
> 
>
https://codereview.chromium.org/1336763006/diff/20001/cc/base/list_container....
> cc/base/list_container.h:30: friend class CC_EXPORT ListContainer;
> On 2015/09/14 19:57:13, weiliangc wrote:
> > I'm kinda torn on the friend idea. Maybe stop CC_EXPORT-ing Helper class is
> > enough? 
> 
> I'm not really sure. Wouldn't that still allow any cc code to create and use
the
> helper directly?
> 
> > 
> > Just that putting entire public interface into private w/ friend seems like
> > exact same level of data encapsulation?
> 
> Ya, I intended to keep the same level of encapsulation, since I don't want to
> expose ListContainerHelper functionality to other classes other than the ones
> explicitly specified. 
> 
> Are you proposing making the helper's functionality public and then removing
> CC_EXPORT or keeping the inheritance and removing CC_EXPORT?

It's a template class, it would be instantiated in each module, so you shouldn't
need to export it... no?

[weiliangc, 2015-09-14 21:17:14]

LGTM :)

On 2015/09/14 at 20:10:02, hendrikw wrote:
> On 2015/09/14 20:05:07, vmpstr wrote:
> >
https://codereview.chromium.org/1336763006/diff/20001/cc/base/list_container.h
> > File cc/base/list_container.h (right):
> > 
> >
https://codereview.chromium.org/1336763006/diff/20001/cc/base/list_container....
> > cc/base/list_container.h:30: friend class CC_EXPORT ListContainer;
> > On 2015/09/14 19:57:13, weiliangc wrote:
> > > I'm kinda torn on the friend idea. Maybe stop CC_EXPORT-ing Helper class
is
> > > enough? 
> > 
> > I'm not really sure. Wouldn't that still allow any cc code to create and use
the
> > helper directly?
> >
Per IRC chat, agree that friend class is a bigger caution sign.
 
> > > 
> > > Just that putting entire public interface into private w/ friend seems
like
> > > exact same level of data encapsulation?
> > 
> > Ya, I intended to keep the same level of encapsulation, since I don't want
to
> > expose ListContainerHelper functionality to other classes other than the
ones
> > explicitly specified. 
> > 
> > Are you proposing making the helper's functionality public and then removing
> > CC_EXPORT or keeping the inheritance and removing CC_EXPORT?
> 
> It's a template class, it would be instantiated in each module, so you
shouldn't need to export it... no?

[weiliangc, 2015-09-14 21:17:56]

On 2015/09/14 at 21:17:14, weiliangc wrote:
> LGTM :)
> 
> On 2015/09/14 at 20:10:02, hendrikw wrote:
> > On 2015/09/14 20:05:07, vmpstr wrote:
> > >
https://codereview.chromium.org/1336763006/diff/20001/cc/base/list_container.h
> > > File cc/base/list_container.h (right):
> > > 
> > >
https://codereview.chromium.org/1336763006/diff/20001/cc/base/list_container....
> > > cc/base/list_container.h:30: friend class CC_EXPORT ListContainer;
> > > On 2015/09/14 19:57:13, weiliangc wrote:
> > > > I'm kinda torn on the friend idea. Maybe stop CC_EXPORT-ing Helper class
is
> > > > enough? 
> > > 
> > > I'm not really sure. Wouldn't that still allow any cc code to create and
use the
> > > helper directly?
> > >
> Per IRC chat, agree that friend class is a bigger caution sign.
>  
> > > > 
> > > > Just that putting entire public interface into private w/ friend seems
like
> > > > exact same level of data encapsulation?
> > > 
> > > Ya, I intended to keep the same level of encapsulation, since I don't want
to
> > > expose ListContainerHelper functionality to other classes other than the
ones
> > > explicitly specified. 
> > > 
> > > Are you proposing making the helper's functionality public and then
removing
> > > CC_EXPORT or keeping the inheritance and removing CC_EXPORT?

What you have now is good. With friend class don't think CC_EXPORT make too much
difference.
> > 
> > It's a template class, it would be instantiated in each module, so you
shouldn't need to export it... no?

[danakj, 2015-09-14 21:25:59]

LGTM

https://codereview.chromium.org/1336763006/diff/40001/cc/base/list_container.h
File cc/base/list_container.h (right):

https://codereview.chromium.org/1336763006/diff/40001/cc/base/list_container....
cc/base/list_container.h:188: class CC_EXPORT ListContainer {
can you put this in its own file? (or rather, put the helper in its own file)
now, since you have another container class coming (and put that one in its own
too)

https://codereview.chromium.org/1336763006/diff/40001/cc/base/list_container....
cc/base/list_container.h:352: : ListContainerHelper::Iterator(container,
dat git cl format. surely this is a bug :P

https://codereview.chromium.org/1336763006/diff/40001/cc/base/list_container....
cc/base/list_container.h:427:
ReverseIterator(ListContainerHelper::ListContainerCharAllocator* container,
Having ListContainer in the name twice, maybe a little verbose? Drop the second
one?

[vmpstr, 2015-09-15 17:32:33]

https://codereview.chromium.org/1336763006/diff/40001/cc/base/list_container.h
File cc/base/list_container.h (right):

https://codereview.chromium.org/1336763006/diff/40001/cc/base/list_container....
cc/base/list_container.h:188: class CC_EXPORT ListContainer {
On 2015/09/14 21:25:59, danakj wrote:
> can you put this in its own file? (or rather, put the helper in its own file)
> now, since you have another container class coming (and put that one in its
own
> too)

Done.

https://codereview.chromium.org/1336763006/diff/40001/cc/base/list_container....
cc/base/list_container.h:427:
ReverseIterator(ListContainerHelper::ListContainerCharAllocator* container,
On 2015/09/14 21:25:59, danakj wrote:
> Having ListContainer in the name twice, maybe a little verbose? Drop the
second
> one?

Done.

[danakj, 2015-09-15 17:33:32]

Thanks :)

[vmpstr, 2015-09-15 17:33:35]

The CQ bit was checked by vmpstr@chromium.org

[vmpstr, 2015-09-15 17:33:36]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
weiliangc@chromium.org
Link to the patchset: https://codereview.chromium.org/1336763006/#ps80001
(title: "multiple files")

[commit-bot: I haz the power, 2015-09-15 17:34:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1336763006/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1336763006/80001

[commit-bot: I haz the power, 2015-09-15 17:44:21]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-09-15 17:44:57]

Patchset 5 (id:??) landed as
https://crrev.com/988b5dee7b043fb1ae72bd64aac5b278f91b4146
Cr-Commit-Position: refs/heads/master@{#348925}

[commit-bot: I haz the power, 2015-09-23 12:46:35]

Patchset 5 (id:??) landed as
https://crrev.com/988b5dee7b043fb1ae72bd64aac5b278f91b4146
Cr-Commit-Position: refs/heads/master@{#348925}