Migrate TabControlFeature from 0.5 to 0.6

Migrate TabControlFeature from 0.5 to 0.6

This CL includes:
1. Moving TabControlFeature to blimp/client/core/feature/
2. Create BlimpContentsManager which creates BlimpContentsImpl, and
return a BlimpContentsImpl given a content id
3. Have BlimpClientContextImpl calls
BlimpContentsManager->CreateBlimpContents()

BUG=628802
Committed: https://crrev.com/bdee0b7cac3fdbcfc476ccb8f3d203a7ad58a626
Cr-Commit-Position: refs/heads/master@{#410900}

[Menglin, 2016-07-29 20:56:46]

mlliu@chromium.org changed reviewers:
+ dtrainor@chromium.org

[Menglin, 2016-07-29 20:56:47]

Hi David, please review this CL. Thanks!

Menglin

[David Trainor- moved to gerrit, 2016-08-03 18:55:43]

https://codereview.chromium.org/2201433002/diff/1/blimp/client/core/contents/...
File blimp/client/core/contents/blimp_contents_impl.cc (right):

https://codereview.chromium.org/2201433002/diff/1/blimp/client/core/contents/...
blimp/client/core/contents/blimp_contents_impl.cc:25:
BlimpContentsImpl::BlimpContentsImpl() : navigation_controller_(this) {}
What do we initialize id_ to in the default constructor?  Should we remove this?

https://codereview.chromium.org/2201433002/diff/1/blimp/client/core/contents/...
File blimp/client/core/contents/blimp_contents_manager.cc (right):

https://codereview.chromium.org/2201433002/diff/1/blimp/client/core/contents/...
blimp/client/core/contents/blimp_contents_manager.cc:46: BlimpContentsImpl*
blimp_contents,
Can we just make BlimpContentsObserver take a BlimpContents in the constructor
and auto-clear it once it sees the destroy call?  That way we can just expose a
blimp_contents() method to access the BlimpContents and we can guarantee all
future observers will never point to a destroyed contents by accident.

https://codereview.chromium.org/2201433002/diff/1/blimp/client/core/contents/...
File blimp/client/core/contents/blimp_contents_manager.h (right):

https://codereview.chromium.org/2201433002/diff/1/blimp/client/core/contents/...
blimp/client/core/contents/blimp_contents_manager.h:27: std::map<int,
std::unique_ptr<BlimpContentsDeletionObserver>> observer_map_;
Can this just be BlimpContentsObserver?  Then you can remove the class line
above and put the actual class in an anonymous namespace.

[Menglin, 2016-08-03 19:25:48]

https://codereview.chromium.org/2201433002/diff/1/blimp/client/core/contents/...
File blimp/client/core/contents/blimp_contents_manager.cc (right):

https://codereview.chromium.org/2201433002/diff/1/blimp/client/core/contents/...
blimp/client/core/contents/blimp_contents_manager.cc:46: BlimpContentsImpl*
blimp_contents,
On 2016/08/03 18:55:42, David Trainor wrote:
> Can we just make BlimpContentsObserver take a BlimpContents in the constructor
> and auto-clear it once it sees the destroy call?  That way we can just expose
a
> blimp_contents() method to access the BlimpContents and we can guarantee all
> future observers will never point to a destroyed contents by accident.

But BlimpContentsManager still needs to keep a map to hold all observers. So
when the content is destroyed, the observer needs to be destroyed from the map
as well. So it needs a BlimpContentsManager* as an input in the constructor.

or am i misunderstood your point?

https://codereview.chromium.org/2201433002/diff/1/blimp/client/core/contents/...
File blimp/client/core/contents/blimp_contents_manager.h (right):

https://codereview.chromium.org/2201433002/diff/1/blimp/client/core/contents/...
blimp/client/core/contents/blimp_contents_manager.h:27: std::map<int,
std::unique_ptr<BlimpContentsDeletionObserver>> observer_map_;
On 2016/08/03 18:55:43, David Trainor wrote:
> Can this just be BlimpContentsObserver?  Then you can remove the class line
> above and put the actual class in an anonymous namespace.

There is an existing BlimpContentsObserver
blimp/client/public/contents/blimp_contents_observer.h. You mean that?

[Menglin, 2016-08-03 23:49:59]

Hi David, I uploaded a new patch. ptal. Thanks!

Menglin

https://codereview.chromium.org/2201433002/diff/1/blimp/client/core/contents/...
File blimp/client/core/contents/blimp_contents_impl.cc (right):

https://codereview.chromium.org/2201433002/diff/1/blimp/client/core/contents/...
blimp/client/core/contents/blimp_contents_impl.cc:25:
BlimpContentsImpl::BlimpContentsImpl() : navigation_controller_(this) {}
On 2016/08/03 18:55:42, David Trainor wrote:
> What do we initialize id_ to in the default constructor?  Should we remove
this?

Yeah we can remove the default constructor.

https://codereview.chromium.org/2201433002/diff/1/blimp/client/core/contents/...
File blimp/client/core/contents/blimp_contents_manager.cc (right):

https://codereview.chromium.org/2201433002/diff/1/blimp/client/core/contents/...
blimp/client/core/contents/blimp_contents_manager.cc:46: BlimpContentsImpl*
blimp_contents,
On 2016/08/03 18:55:42, David Trainor wrote:
> Can we just make BlimpContentsObserver take a BlimpContents in the constructor
> and auto-clear it once it sees the destroy call?  That way we can just expose
a
> blimp_contents() method to access the BlimpContents and we can guarantee all
> future observers will never point to a destroyed contents by accident.

Yeah. BlimpContentsDeletionObserver doesn't need to take an id in the
constructor.

I think to keep BlimpContentsImpl* blimp_contents() within
BlimpContentsDeletionObserver is better. Reason, it needs to return a
BlimpContentsImpl* (instead of BlimpContent*) because the caller of
blimp_contents() may want to know the id() of the content, which is only
available in BlimpContentImpl.

https://codereview.chromium.org/2201433002/diff/1/blimp/client/core/contents/...
File blimp/client/core/contents/blimp_contents_manager.h (right):

https://codereview.chromium.org/2201433002/diff/1/blimp/client/core/contents/...
blimp/client/core/contents/blimp_contents_manager.h:27: std::map<int,
std::unique_ptr<BlimpContentsDeletionObserver>> observer_map_;
On 2016/08/03 18:55:43, David Trainor wrote:
> Can this just be BlimpContentsObserver?  Then you can remove the class line
> above and put the actual class in an anonymous namespace.

Actually as i looked at the code again. I think first I should move the
declaration of BlimpContentsDeletionObserver from blimp_contents_manager.cc to
blimp_contents_manager.h, because BlimpContentsDeletionObserver is an inner
class of BlimpContentsManager. The benefit of being the inner class makes
BlimpContentsDeletionObserver able to access private method
EraseObserverFromMap, which should be private.

And as I move the declaration of BlimpContentsDeletionObserver here, I think
it's ok use BlimpContentsDeletionObserver in the observer_map_, wdyt? The inner
class won't be used by anywhere else since it's private.

[Menglin, 2016-08-03 23:50:31]

mlliu@chromium.org changed reviewers:
+ nyquist@chromium.org

[Menglin, 2016-08-03 23:50:32]

I forgot to add Tommy as reviewer last time

[David Trainor- moved to gerrit, 2016-08-04 16:21:18]

https://codereview.chromium.org/2201433002/diff/1/blimp/client/core/contents/...
File blimp/client/core/contents/blimp_contents_manager.cc (right):

https://codereview.chromium.org/2201433002/diff/1/blimp/client/core/contents/...
blimp/client/core/contents/blimp_contents_manager.cc:46: BlimpContentsImpl*
blimp_contents,
On 2016/08/03 23:49:59, Menglin wrote:
> On 2016/08/03 18:55:42, David Trainor wrote:
> > Can we just make BlimpContentsObserver take a BlimpContents in the
constructor
> > and auto-clear it once it sees the destroy call?  That way we can just
expose
> a
> > blimp_contents() method to access the BlimpContents and we can guarantee all
> > future observers will never point to a destroyed contents by accident.
> 
> Yeah. BlimpContentsDeletionObserver doesn't need to take an id in the
> constructor.
> 
> I think to keep BlimpContentsImpl* blimp_contents() within
> BlimpContentsDeletionObserver is better. Reason, it needs to return a
> BlimpContentsImpl* (instead of BlimpContent*) because the caller of
> blimp_contents() may want to know the id() of the content, which is only
> available in BlimpContentImpl. 

I think the problem is we're solving the observer -> contents lifecycle
dependency in a child class.  It would probably be cleaner to solve it in the
base class so all observers don't have to reimplement it.  I'd be fine with a
static_cast<BlimpContentsImpl*>(blimp_contents())->id() if we needed it.

wdyt?

https://codereview.chromium.org/2201433002/diff/1/blimp/client/core/contents/...
File blimp/client/core/contents/blimp_contents_manager.h (right):

https://codereview.chromium.org/2201433002/diff/1/blimp/client/core/contents/...
blimp/client/core/contents/blimp_contents_manager.h:27: std::map<int,
std::unique_ptr<BlimpContentsDeletionObserver>> observer_map_;
On 2016/08/03 23:49:59, Menglin wrote:
> On 2016/08/03 18:55:43, David Trainor wrote:
> > Can this just be BlimpContentsObserver?  Then you can remove the class line
> > above and put the actual class in an anonymous namespace.
> 
> Actually as i looked at the code again. I think first I should move the
> declaration of BlimpContentsDeletionObserver from blimp_contents_manager.cc to
> blimp_contents_manager.h, because BlimpContentsDeletionObserver is an inner
> class of BlimpContentsManager. The benefit of being the inner class makes
> BlimpContentsDeletionObserver able to access private method
> EraseObserverFromMap, which should be private.
> 
> And as I move the declaration of BlimpContentsDeletionObserver here, I think
> it's ok use BlimpContentsDeletionObserver in the observer_map_, wdyt? The
inner
> class won't be used by anywhere else since it's private.

Hmm maybe put this back in the cc file and make it a friend class if that works?
 Inlining all of this makes it kind of hard to quickly read.

https://codereview.chromium.org/2201433002/diff/20001/blimp/client/core/conte...
File blimp/client/core/contents/blimp_contents_manager.h (right):

https://codereview.chromium.org/2201433002/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_manager.h:23: class
BlimpContentsDeletionObserver : public BlimpContentsObserver {
Leave this in the cc file and surround it with anonymous namespace?

[Menglin, 2016-08-05 02:08:24]

https://codereview.chromium.org/2201433002/diff/1/blimp/client/core/contents/...
File blimp/client/core/contents/blimp_contents_manager.cc (right):

https://codereview.chromium.org/2201433002/diff/1/blimp/client/core/contents/...
blimp/client/core/contents/blimp_contents_manager.cc:46: BlimpContentsImpl*
blimp_contents,
On 2016/08/04 16:21:18, David Trainor wrote:
> On 2016/08/03 23:49:59, Menglin wrote:
> > On 2016/08/03 18:55:42, David Trainor wrote:
> > > Can we just make BlimpContentsObserver take a BlimpContents in the
> constructor
> > > and auto-clear it once it sees the destroy call?  That way we can just
> expose
> > a
> > > blimp_contents() method to access the BlimpContents and we can guarantee
all
> > > future observers will never point to a destroyed contents by accident.
> > 
> > Yeah. BlimpContentsDeletionObserver doesn't need to take an id in the
> > constructor.
> > 
> > I think to keep BlimpContentsImpl* blimp_contents() within
> > BlimpContentsDeletionObserver is better. Reason, it needs to return a
> > BlimpContentsImpl* (instead of BlimpContent*) because the caller of
> > blimp_contents() may want to know the id() of the content, which is only
> > available in BlimpContentImpl. 
> 
> I think the problem is we're solving the observer -> contents lifecycle
> dependency in a child class.  It would probably be cleaner to solve it in the
> base class so all observers don't have to reimplement it.  I'd be fine with a
> static_cast<BlimpContentsImpl*>(blimp_contents())->id() if we needed it.
> 
> wdyt?

ok. got your point. yeah it makes sense to solve it in the base class

https://codereview.chromium.org/2201433002/diff/20001/blimp/client/core/conte...
File blimp/client/core/contents/blimp_contents_manager.h (right):

https://codereview.chromium.org/2201433002/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_manager.h:23: class
BlimpContentsDeletionObserver : public BlimpContentsObserver {
On 2016/08/04 16:21:18, David Trainor wrote:
> Leave this in the cc file and surround it with anonymous namespace?

BlimpContentsDeletionObserver doesn't work with anonymous namespace :( I tried,
but all kinds of errors.
http://stackoverflow.com/questions/33267839/how-to-forward-declare-class-whic...

I can move the declaration to cc file and make it a friend of
BlimpContentsManager. And since the declaration is in cc file, it's guaranteed
that no where else can use it. wdyt?

[David Trainor- moved to gerrit, 2016-08-05 16:10:24]

https://codereview.chromium.org/2201433002/diff/20001/blimp/client/core/conte...
File blimp/client/core/contents/blimp_contents_manager.h (right):

https://codereview.chromium.org/2201433002/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_manager.h:23: class
BlimpContentsDeletionObserver : public BlimpContentsObserver {
On 2016/08/05 02:08:24, Menglin wrote:
> On 2016/08/04 16:21:18, David Trainor wrote:
> > Leave this in the cc file and surround it with anonymous namespace?
> 
> BlimpContentsDeletionObserver doesn't work with anonymous namespace :( I
tried,
> but all kinds of errors.
>
http://stackoverflow.com/questions/33267839/how-to-forward-declare-class-whic...
> 
> I can move the declaration to cc file and make it a friend of
> BlimpContentsManager. And since the declaration is in cc file, it's guaranteed
> that no where else can use it. wdyt?

Sorry I suggested that before I wrote my other comment on the other patch and
forgot to delete this one.  Yeah just move this to native and make it a friend
like we discussed in patch 1.  Thanks! :)

[Menglin, 2016-08-05 16:39:02]

HI David, in patch 3, I move BlimpContentsDeletionObserver to native, and move
observer-content life cycle to BlimpContentsObserver. I also sync to head. ptal.
Thanks!

https://codereview.chromium.org/2201433002/diff/20001/blimp/client/core/conte...
File blimp/client/core/contents/blimp_contents_manager.h (right):

https://codereview.chromium.org/2201433002/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_manager.h:23: class
BlimpContentsDeletionObserver : public BlimpContentsObserver {
On 2016/08/05 16:10:23, David Trainor wrote:
> On 2016/08/05 02:08:24, Menglin wrote:
> > On 2016/08/04 16:21:18, David Trainor wrote:
> > > Leave this in the cc file and surround it with anonymous namespace?
> > 
> > BlimpContentsDeletionObserver doesn't work with anonymous namespace :( I
> tried,
> > but all kinds of errors.
> >
>
http://stackoverflow.com/questions/33267839/how-to-forward-declare-class-whic...
> > 
> > I can move the declaration to cc file and make it a friend of
> > BlimpContentsManager. And since the declaration is in cc file, it's
guaranteed
> > that no where else can use it. wdyt?
> 
> Sorry I suggested that before I wrote my other comment on the other patch and
> forgot to delete this one.  Yeah just move this to native and make it a friend
> like we discussed in patch 1.  Thanks! :)

Done.

[Menglin, 2016-08-05 21:05:33]

Hi David, please look at patch 4. I forgot to git add
blimp/client/public/contents/blimp_contents_observer.cc in patch3..

[nyquist, 2016-08-05 23:03:23]

https://codereview.chromium.org/2201433002/diff/60001/blimp/client/core/blimp...
File blimp/client/core/blimp_client_context_impl.cc (right):

https://codereview.chromium.org/2201433002/diff/60001/blimp/client/core/blimp...
blimp/client/core/blimp_client_context_impl.cc:72:
delegate_->AttachBlimpContentsHelpers(blimp_contents.get());
This call to the delegate is part of the public API, so we have to ensure that
we never expose BlimpContents that don't have had this call.

https://codereview.chromium.org/2201433002/diff/60001/blimp/client/core/blimp...
File blimp/client/core/blimp_client_context_impl.h (right):

https://codereview.chromium.org/2201433002/diff/60001/blimp/client/core/blimp...
blimp/client/core/blimp_client_context_impl.h:28: class
BlimpClientContextImplAndroid;
Is this still necessary?

https://codereview.chromium.org/2201433002/diff/60001/blimp/client/core/conte...
File blimp/client/core/contents/blimp_contents_impl.h (right):

https://codereview.chromium.org/2201433002/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_impl.h:57: int id_;
Could you explain what this ID is for?

https://codereview.chromium.org/2201433002/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_impl.h:59:
DISALLOW_IMPLICIT_CONSTRUCTORS(BlimpContentsImpl);
Is there a reason we can't DISALLOW_COPY_AND_ASSIGN here?

https://codereview.chromium.org/2201433002/diff/60001/blimp/client/core/conte...
File blimp/client/core/contents/blimp_contents_impl_unittest.cc (right):

https://codereview.chromium.org/2201433002/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_impl_unittest.cc:36: BlimpContentsImpl
blimp_contents{kDummyTabId};
I think we often use just () for constructors. Is this intentional?

https://codereview.chromium.org/2201433002/diff/60001/blimp/client/core/conte...
File blimp/client/core/contents/blimp_contents_manager.cc (right):

https://codereview.chromium.org/2201433002/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_manager.cc:59: base::WrapUnique(new
BlimpContentsImpl(id));
Could you use base::MakeUnique here and on the line below?

https://codereview.chromium.org/2201433002/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_manager.cc:69: if
(observer_map_.find(id) != observer_map_.end()) {
Could you just early out if this conditional fails? Then you don't have to
indent the rest.

https://codereview.chromium.org/2201433002/diff/60001/blimp/client/core/conte...
File blimp/client/core/contents/blimp_contents_manager.h (right):

https://codereview.chromium.org/2201433002/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_manager.h:13: class
BlimpContentsManager {
It would be great with a high level description of this class.

https://codereview.chromium.org/2201433002/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_manager.h:18:
std::unique_ptr<BlimpContentsImpl> CreateBlimpContents();
It's probably helpful to add a comment for each of these public methods.

https://codereview.chromium.org/2201433002/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_manager.h:25: int
CreateBlimpContentsId();
I think it's helpful to in general document what these methods do, even though
they're private.

https://codereview.chromium.org/2201433002/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_manager.h:29: std::map<int,
std::unique_ptr<BlimpContentsDeletionObserver>> observer_map_;
Could you explain what this does?

https://codereview.chromium.org/2201433002/diff/60001/blimp/client/core/featu...
File blimp/client/core/feature/tab_control_feature.h (right):

https://codereview.chromium.org/2201433002/diff/60001/blimp/client/core/featu...
blimp/client/core/feature/tab_control_feature.h:5: #ifndef
BLIMP_CLIENT_CORE_FEATURE_TAB_CONTROL_FEATURE_H_
Could you instead move this to //blimp/client/core/contents?

https://codereview.chromium.org/2201433002/diff/60001/blimp/client/public/con...
File blimp/client/public/contents/blimp_contents_observer.h (right):

https://codereview.chromium.org/2201433002/diff/60001/blimp/client/public/con...
blimp/client/public/contents/blimp_contents_observer.h:30: void
ClearBlimpContents();
Could you explain what this is for and why this is not just inlined in the
destructor?

[Menglin, 2016-08-05 23:37:15]

https://codereview.chromium.org/2201433002/diff/60001/blimp/client/core/blimp...
File blimp/client/core/blimp_client_context_impl.cc (right):

https://codereview.chromium.org/2201433002/diff/60001/blimp/client/core/blimp...
blimp/client/core/blimp_client_context_impl.cc:72:
delegate_->AttachBlimpContentsHelpers(blimp_contents.get());
On 2016/08/05 23:03:22, nyquist wrote:
> This call to the delegate is part of the public API, so we have to ensure that
> we never expose BlimpContents that don't have had this call.

Maybe define
BlimpContentsManager::CreateBlimpContents(BlimpClientContextDelegate*) ? so that
we essentially can move this line to
CreateBlimpContents(BlimpClientContextDelegate*) ?

https://codereview.chromium.org/2201433002/diff/60001/blimp/client/core/conte...
File blimp/client/core/contents/blimp_contents_impl.h (right):

https://codereview.chromium.org/2201433002/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_impl.h:57: int id_;
On 2016/08/05 23:03:22, nyquist wrote:
> Could you explain what this ID is for?

yeah, when a BlimpContentsImpl is destroyed, its BlimpContentsDeletionObserver
is able to retrieve the id, and use that destroy its entry from the
observer_map_. It is used to update the observer_map_

https://codereview.chromium.org/2201433002/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_impl.h:59:
DISALLOW_IMPLICIT_CONSTRUCTORS(BlimpContentsImpl);
On 2016/08/05 23:03:23, nyquist wrote:
> Is there a reason we can't DISALLOW_COPY_AND_ASSIGN here?

since I removed BlimpContentsImpl(), and I had a compile error if i still use
DISALLOW_COPY_AND_ASSIGN. I searched the error and found an answer suggesting
using this instead

looking at your other comment
https://codereview.chromium.org/2201433002/diff/60001/blimp/client/core/conte...,
should i keep BlimpContentsImpl() ?

https://codereview.chromium.org/2201433002/diff/60001/blimp/client/core/conte...
File blimp/client/core/contents/blimp_contents_manager.cc (right):

https://codereview.chromium.org/2201433002/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_manager.cc:45: ClearBlimpContents();
here it needs to first make sure it contents is not null. Then get its id, then
clear its contents. So I define a method just for contents_ = nullptr

https://codereview.chromium.org/2201433002/diff/60001/blimp/client/public/con...
File blimp/client/public/contents/blimp_contents_observer.h (right):

https://codereview.chromium.org/2201433002/diff/60001/blimp/client/public/con...
blimp/client/public/contents/blimp_contents_observer.h:30: void
ClearBlimpContents();
On 2016/08/05 23:03:23, nyquist wrote:
> Could you explain what this is for and why this is not just inlined in the
> destructor?

yeah please see my other comment

[Menglin, 2016-08-06 00:57:00]

In patch 5. I made changes according to tommy's suggestions, except
https://codereview.chromium.org/2201433002/diff/60001/blimp/client/core/blimp...
and we think maybe it's ok to keep it as it is.

https://codereview.chromium.org/2201433002/diff/60001/blimp/client/core/blimp...
File blimp/client/core/blimp_client_context_impl.h (right):

https://codereview.chromium.org/2201433002/diff/60001/blimp/client/core/blimp...
blimp/client/core/blimp_client_context_impl.h:28: class
BlimpClientContextImplAndroid;
On 2016/08/05 at 23:03:22, nyquist wrote:
> Is this still necessary?

none. deleted it

https://codereview.chromium.org/2201433002/diff/60001/blimp/client/core/conte...
File blimp/client/core/contents/blimp_contents_impl.h (right):

https://codereview.chromium.org/2201433002/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_impl.h:57: int id_;
On 2016/08/05 at 23:37:15, Menglin wrote:
> On 2016/08/05 23:03:22, nyquist wrote:
> > Could you explain what this ID is for?
> 
> yeah, when a BlimpContentsImpl is destroyed, its BlimpContentsDeletionObserver
is able to retrieve the id, and use that destroy its entry from the
observer_map_. It is used to update the observer_map_

comments added

https://codereview.chromium.org/2201433002/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_impl.h:59:
DISALLOW_IMPLICIT_CONSTRUCTORS(BlimpContentsImpl);
On 2016/08/05 at 23:37:15, Menglin wrote:
> On 2016/08/05 23:03:23, nyquist wrote:
> > Is there a reason we can't DISALLOW_COPY_AND_ASSIGN here?
> 
> since I removed BlimpContentsImpl(), and I had a compile error if i still use
DISALLOW_COPY_AND_ASSIGN. I searched the error and found an answer suggesting
using this instead
> 
> looking at your other comment
https://codereview.chromium.org/2201433002/diff/60001/blimp/client/core/conte...,
should i keep BlimpContentsImpl() ?

Sorry i double checked. there is no problem using DISALLOW_COPY_AND_ASSIGN here

https://codereview.chromium.org/2201433002/diff/60001/blimp/client/core/conte...
File blimp/client/core/contents/blimp_contents_impl_unittest.cc (right):

https://codereview.chromium.org/2201433002/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_impl_unittest.cc:36: BlimpContentsImpl
blimp_contents{kDummyTabId};
On 2016/08/05 at 23:03:23, nyquist wrote:
> I think we often use just () for constructors. Is this intentional?

use () instead

https://codereview.chromium.org/2201433002/diff/60001/blimp/client/core/conte...
File blimp/client/core/contents/blimp_contents_manager.cc (right):

https://codereview.chromium.org/2201433002/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_manager.cc:59: base::WrapUnique(new
BlimpContentsImpl(id));
On 2016/08/05 at 23:03:23, nyquist wrote:
> Could you use base::MakeUnique here and on the line below?

Done.

https://codereview.chromium.org/2201433002/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_manager.cc:69: if
(observer_map_.find(id) != observer_map_.end()) {
On 2016/08/05 at 23:03:23, nyquist wrote:
> Could you just early out if this conditional fails? Then you don't have to
indent the rest.

Done

https://codereview.chromium.org/2201433002/diff/60001/blimp/client/core/conte...
File blimp/client/core/contents/blimp_contents_manager.h (right):

https://codereview.chromium.org/2201433002/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_manager.h:13: class
BlimpContentsManager {
On 2016/08/05 at 23:03:23, nyquist wrote:
> It would be great with a high level description of this class.

Done

https://codereview.chromium.org/2201433002/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_manager.h:18:
std::unique_ptr<BlimpContentsImpl> CreateBlimpContents();
On 2016/08/05 at 23:03:23, nyquist wrote:
> It's probably helpful to add a comment for each of these public methods.

Given I added high level description for the class. I think it's obvious what
CreateBlimpContents() does and so save the comment for it

https://codereview.chromium.org/2201433002/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_manager.h:25: int
CreateBlimpContentsId();
On 2016/08/05 at 23:03:23, nyquist wrote:
> I think it's helpful to in general document what these methods do, even though
they're private.

Done.

https://codereview.chromium.org/2201433002/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_manager.h:29: std::map<int,
std::unique_ptr<BlimpContentsDeletionObserver>> observer_map_;
On 2016/08/05 at 23:03:23, nyquist wrote:
> Could you explain what this does?

Done.

https://codereview.chromium.org/2201433002/diff/60001/blimp/client/core/featu...
File blimp/client/core/feature/tab_control_feature.h (right):

https://codereview.chromium.org/2201433002/diff/60001/blimp/client/core/featu...
blimp/client/core/feature/tab_control_feature.h:5: #ifndef
BLIMP_CLIENT_CORE_FEATURE_TAB_CONTROL_FEATURE_H_
On 2016/08/05 at 23:03:23, nyquist wrote:
> Could you instead move this to //blimp/client/core/contents?

Done

https://codereview.chromium.org/2201433002/diff/60001/blimp/client/public/con...
File blimp/client/public/contents/blimp_contents_observer.h (right):

https://codereview.chromium.org/2201433002/diff/60001/blimp/client/public/con...
blimp/client/public/contents/blimp_contents_observer.h:30: void
ClearBlimpContents();
On 2016/08/05 at 23:37:15, Menglin wrote:
> On 2016/08/05 23:03:23, nyquist wrote:
> > Could you explain what this is for and why this is not just inlined in the
> > destructor?
> 
> yeah please see my other comment

Done.

[David Trainor- moved to gerrit, 2016-08-06 04:25:39]

https://codereview.chromium.org/2201433002/diff/60001/blimp/client/core/blimp...
File blimp/client/core/blimp_client_context_impl.cc (right):

https://codereview.chromium.org/2201433002/diff/60001/blimp/client/core/blimp...
blimp/client/core/blimp_client_context_impl.cc:72:
delegate_->AttachBlimpContentsHelpers(blimp_contents.get());
On 2016/08/05 23:37:15, Menglin wrote:
> On 2016/08/05 23:03:22, nyquist wrote:
> > This call to the delegate is part of the public API, so we have to ensure
that
> > we never expose BlimpContents that don't have had this call.
> 
> Maybe define
> BlimpContentsManager::CreateBlimpContents(BlimpClientContextDelegate*) ? so
that
> we essentially can move this line to
> CreateBlimpContents(BlimpClientContextDelegate*) ?

Saw you two talked about it and decided this was okay.  I think you both have
good points.  It's probably fine to keep it this way, it kind of makes sense
because the delegate is part of the contract with BlimpClientContext and we
could probably argue that the manager shouldn't need to care about helpers...

It is a bit scary being able to create BlimpContents without helpers, but I
think WebContents has this issue currently as well.  BlimpContents doesn't
*need* helpers, they're just helping :).

[David Trainor- moved to gerrit, 2016-08-06 04:32:59]

https://codereview.chromium.org/2201433002/diff/80001/blimp/client/core/conte...
File blimp/client/core/contents/blimp_contents_manager.cc (right):

https://codereview.chromium.org/2201433002/diff/80001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_manager.cc:45: ClearBlimpContents();
This should happen in the base class.  Maybe do something like the following in
BlimpContentsObserver:

BlimpContentsObserver::ContentsIsDying() (nonvirtual) {
  OnContentsDestroyed(); (virtual);
  contents_ = nullptr;
}

https://codereview.chromium.org/2201433002/diff/80001/blimp/client/public/con...
File blimp/client/public/contents/blimp_contents_observer.cc (right):

https://codereview.chromium.org/2201433002/diff/80001/blimp/client/public/con...
blimp/client/public/contents/blimp_contents_observer.cc:16:
BlimpContentsObserver::~BlimpContentsObserver() {
Can we have two unit tests for this?

1. When this dies BlimpContents should drop the reference to it.
2. When BlimpContents dies, this should get a call to OnContentsDestroyed (which
has a valid BlimpContents), and after that BlimpContents should be null.

https://codereview.chromium.org/2201433002/diff/80001/blimp/client/public/con...
File blimp/client/public/contents/blimp_contents_observer.h (right):

https://codereview.chromium.org/2201433002/diff/80001/blimp/client/public/con...
blimp/client/public/contents/blimp_contents_observer.h:32: void
ClearBlimpContents();
See above, now that I think about it this should happen all in the base class
and not require any overriding by the subclasses.

[Menglin, 2016-08-09 00:31:18]

Hi David! ptal at Patch 6. I changed it according your suggestions. thank!

https://codereview.chromium.org/2201433002/diff/80001/blimp/client/core/conte...
File blimp/client/core/contents/blimp_contents_manager.cc (right):

https://codereview.chromium.org/2201433002/diff/80001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_manager.cc:45: ClearBlimpContents();
On 2016/08/06 04:32:58, David Trainor wrote:
> This should happen in the base class.  Maybe do something like the following
in
> BlimpContentsObserver:
> 
> BlimpContentsObserver::ContentsIsDying() (nonvirtual) {
>   OnContentsDestroyed(); (virtual);
>   contents_ = nullptr;
> }

Done.

https://codereview.chromium.org/2201433002/diff/80001/blimp/client/public/con...
File blimp/client/public/contents/blimp_contents_observer.cc (right):

https://codereview.chromium.org/2201433002/diff/80001/blimp/client/public/con...
blimp/client/public/contents/blimp_contents_observer.cc:16:
BlimpContentsObserver::~BlimpContentsObserver() {
On 2016/08/06 04:32:58, David Trainor wrote:
> Can we have two unit tests for this?
> 
> 1. When this dies BlimpContents should drop the reference to it.
> 2. When BlimpContents dies, this should get a call to OnContentsDestroyed
(which
> has a valid BlimpContents), and after that BlimpContents should be null.

Done.

https://codereview.chromium.org/2201433002/diff/80001/blimp/client/public/con...
File blimp/client/public/contents/blimp_contents_observer.h (right):

https://codereview.chromium.org/2201433002/diff/80001/blimp/client/public/con...
blimp/client/public/contents/blimp_contents_observer.h:32: void
ClearBlimpContents();
On 2016/08/06 04:32:58, David Trainor wrote:
> See above, now that I think about it this should happen all in the base class
> and not require any overriding by the subclasses.

Done.

[David Trainor- moved to gerrit, 2016-08-09 05:34:12]

lgtm % nits.

https://codereview.chromium.org/2201433002/diff/100001/blimp/client/core/cont...
File blimp/client/core/contents/blimp_contents_observer_unittest.cc (right):

https://codereview.chromium.org/2201433002/diff/100001/blimp/client/core/cont...
blimp/client/core/contents/blimp_contents_observer_unittest.cc:51:
EXPECT_EQ(observer.get()->blimp_contents(), contents.get());
I don't think you need .get() before the ->

https://codereview.chromium.org/2201433002/diff/100001/blimp/client/public/co...
File blimp/client/public/contents/blimp_contents_observer.h (right):

https://codereview.chromium.org/2201433002/diff/100001/blimp/client/public/co...
blimp/client/public/contents/blimp_contents_observer.h:27: virtual void
BlimpContentsDying();
Don't make this virtual and put it at the end of the public methods (not in the
middle of the virtual methods).

[Menglin, 2016-08-09 18:19:31]

https://codereview.chromium.org/2201433002/diff/100001/blimp/client/core/cont...
File blimp/client/core/contents/blimp_contents_observer_unittest.cc (right):

https://codereview.chromium.org/2201433002/diff/100001/blimp/client/core/cont...
blimp/client/core/contents/blimp_contents_observer_unittest.cc:51:
EXPECT_EQ(observer.get()->blimp_contents(), contents.get());
On 2016/08/09 05:34:12, David Trainor wrote:
> I don't think you need .get() before the ->

Done.

https://codereview.chromium.org/2201433002/diff/100001/blimp/client/public/co...
File blimp/client/public/contents/blimp_contents_observer.h (right):

https://codereview.chromium.org/2201433002/diff/100001/blimp/client/public/co...
blimp/client/public/contents/blimp_contents_observer.h:27: virtual void
BlimpContentsDying();
On 2016/08/09 05:34:12, David Trainor wrote:
> Don't make this virtual and put it at the end of the public methods (not in
the
> middle of the virtual methods).

Right. I think i somehow added virtual when i was debugging some error and
forgot to delete it. Thanks for the good catch!

[Menglin, 2016-08-09 18:19:39]

The CQ bit was checked by mlliu@chromium.org

[Menglin, 2016-08-09 18:19:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/2201433002/#ps120001
(title: "nit and sync to head")

[commit-bot: I haz the power, 2016-08-09 18:20:45]

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 18:37:40]

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

[commit-bot: I haz the power, 2016-08-09 18:37:42]

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

[Menglin, 2016-08-09 18:56:52]

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

[commit-bot: I haz the power, 2016-08-09 18:57:32]

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

[Menglin, 2016-08-09 18:58:19]

The CQ bit was unchecked by mlliu@chromium.org

[Menglin, 2016-08-09 23:30:40]

The CQ bit was checked by mlliu@chromium.org

[Menglin, 2016-08-09 23:30:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/2201433002/#ps160001
(title: "some more compile error fix")

[commit-bot: I haz the power, 2016-08-09 23:31:22]

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-10 00:39:45]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-08-10 00:41:41]

Description was changed from

==========
Migrate TabControlFeature from 0.5 to 0.6

This CL includes:
1. Moving TabControlFeature to blimp/client/core/feature/
2. Create BlimpContentsManager which creates BlimpContentsImpl, and
return a BlimpContentsImpl given a content id
3. Have BlimpClientContextImpl calls
BlimpContentsManager->CreateBlimpContents()

BUG=628802
==========

to

==========
Migrate TabControlFeature from 0.5 to 0.6

This CL includes:
1. Moving TabControlFeature to blimp/client/core/feature/
2. Create BlimpContentsManager which creates BlimpContentsImpl, and
return a BlimpContentsImpl given a content id
3. Have BlimpClientContextImpl calls
BlimpContentsManager->CreateBlimpContents()

BUG=628802

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

[commit-bot: I haz the power, 2016-08-10 00:41:43]

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