Add TabControlFeature to BlimpClientContextImpl

Add TabControlFeature to BlimpClientContextImpl

This CL includes:
1. BlimpClientContextImpl owns the TabControlFeature, and register the
feature in its constructor.
2. When a BlimpContents is created, set a pointer to TabControlFeature
in BlimpContentsImpl.
3. Add a method to SetSizeAndScale in BlimpContents.
4. call CreateTab in BlimpClientContextImplAndroid.

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

[Menglin, 2016-08-16 19:22:49]

[email protected] changed reviewers:
+ [email protected], [email protected]

[Menglin, 2016-08-16 19:22:49]

Hi Tommy and David, ptal. Thanks!

Menglin

[nyquist, 2016-08-16 20:41:04]

https://codereview.chromium.org/2255533002/diff/1/blimp/client/core/android/b...
File blimp/client/core/android/blimp_client_context_impl_android.cc (right):

https://codereview.chromium.org/2255533002/diff/1/blimp/client/core/android/b...
blimp/client/core/android/blimp_client_context_impl_android.cc:58:
GetTabControlFeature()->CreateTab(kDummyTabId);
Could this be in the cross-platform code?

https://codereview.chromium.org/2255533002/diff/1/blimp/client/core/blimp_cli...
File blimp/client/core/blimp_client_context_impl.cc (right):

https://codereview.chromium.org/2255533002/diff/1/blimp/client/core/blimp_cli...
blimp/client/core/blimp_client_context_impl.cc:87:
blimp_contents_impl->SetTabControlFeature(tab_control_feature_.get());
Did you consider doing this in the constructor instead? That would probably mean
passing a raw pointer of the feature to BlimpContentsManager of course.

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

https://codereview.chromium.org/2255533002/diff/1/blimp/client/core/contents/...
blimp/client/core/contents/blimp_contents_impl.h:53: void
SetTabControlFeature(TabControlFeature* tab_control_feature);
seems like this could just be set_tab_control_feature and inlined?

https://codereview.chromium.org/2255533002/diff/1/blimp/client/public/content...
File blimp/client/public/contents/blimp_contents.h (right):

https://codereview.chromium.org/2255533002/diff/1/blimp/client/public/content...
blimp/client/public/contents/blimp_contents.h:37: virtual void
SetSizeAndScale(const gfx::Size& size,
Could this possibly be moved to the impl instead? I think size and scale can
come from the BlimpView. If so, remember to move the comment over.

[nyquist, 2016-08-16 20:57:18]

Also, should we move //blimp/client:feature_unit_tests to
//blimp/client/core/contents somewhere now?

[Menglin, 2016-08-16 21:10:03]

On 2016/08/16 20:57:18, nyquist wrote:
> Also, should we move //blimp/client:feature_unit_tests to
> //blimp/client/core/contents somewhere now?

//blimp/client:feature_unit_tests has two other source files besides
tab_control_feature_unittest.cc
https://cs.chromium.org/chromium/src/blimp/client/BUILD.gn?q=feature_unit_tes....
I think eventually it'll be moved to blimp/client/core/contents. Do you want to
move it now?

Not sure where render_widget_feature will be moved

[Menglin, 2016-08-16 21:10:09]

https://codereview.chromium.org/2255533002/diff/1/blimp/client/core/android/b...
File blimp/client/core/android/blimp_client_context_impl_android.cc (right):

https://codereview.chromium.org/2255533002/diff/1/blimp/client/core/android/b...
blimp/client/core/android/blimp_client_context_impl_android.cc:58:
GetTabControlFeature()->CreateTab(kDummyTabId);
On 2016/08/16 20:41:04, nyquist wrote:
> Could this be in the cross-platform code?

you mean BlimpClientContextImpl? I think so. I added it this way because i saw
this pattern between BlimpClientSession and BlimpClientSessionAndroid.

https://codereview.chromium.org/2255533002/diff/1/blimp/client/core/blimp_cli...
File blimp/client/core/blimp_client_context_impl.cc (right):

https://codereview.chromium.org/2255533002/diff/1/blimp/client/core/blimp_cli...
blimp/client/core/blimp_client_context_impl.cc:87:
blimp_contents_impl->SetTabControlFeature(tab_control_feature_.get());
On 2016/08/16 20:41:04, nyquist wrote:
> Did you consider doing this in the constructor instead? That would probably
mean
> passing a raw pointer of the feature to BlimpContentsManager of course.

Do you mean BlimpContentsImpl::BlimpContentsImpl(int id, TabControlFeature*
tab_control_feature) so that a BlimpContentsImpl is guaranteed to have a
TabControlFeature? Yeah if that's the case, we can do this in the constructor.

question not for this CL but in the future, are the other features (navigation
feature, ime feature) going to be added in similar way?

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

https://codereview.chromium.org/2255533002/diff/1/blimp/client/core/contents/...
blimp/client/core/contents/blimp_contents_impl.h:53: void
SetTabControlFeature(TabControlFeature* tab_control_feature);
On 2016/08/16 20:41:04, nyquist wrote:
> seems like this could just be set_tab_control_feature and inlined?

if we pass in the tab control feature in BlimpContentsImpl's constructor, we
don't need this function any more

https://codereview.chromium.org/2255533002/diff/1/blimp/client/public/content...
File blimp/client/public/contents/blimp_contents.h (right):

https://codereview.chromium.org/2255533002/diff/1/blimp/client/public/content...
blimp/client/public/contents/blimp_contents.h:37: virtual void
SetSizeAndScale(const gfx::Size& size,
On 2016/08/16 20:41:04, nyquist wrote:
> Could this possibly be moved to the impl instead? I think size and scale can
> come from the BlimpView. If so, remember to move the comment over.

impl has this function now. it overrides this the virtual function here. If we
only need to call it on BlimpContentsImpl, i can remove the virtual function
from here

[nyquist, 2016-08-16 21:41:41]

https://codereview.chromium.org/2255533002/diff/1/blimp/client/core/android/b...
File blimp/client/core/android/blimp_client_context_impl_android.cc (right):

https://codereview.chromium.org/2255533002/diff/1/blimp/client/core/android/b...
blimp/client/core/android/blimp_client_context_impl_android.cc:58:
GetTabControlFeature()->CreateTab(kDummyTabId);
On 2016/08/16 21:10:08, Menglin wrote:
> On 2016/08/16 20:41:04, nyquist wrote:
> > Could this be in the cross-platform code?
> 
> you mean BlimpClientContextImpl? I think so. I added it this way because i saw
> this pattern between BlimpClientSession and BlimpClientSessionAndroid.

Right, but I don't think that class is necessarily something to guide our lives
towards :-p

This doesn't seem to be Android-specific.

https://codereview.chromium.org/2255533002/diff/1/blimp/client/core/blimp_cli...
File blimp/client/core/blimp_client_context_impl.cc (right):

https://codereview.chromium.org/2255533002/diff/1/blimp/client/core/blimp_cli...
blimp/client/core/blimp_client_context_impl.cc:87:
blimp_contents_impl->SetTabControlFeature(tab_control_feature_.get());
On 2016/08/16 21:10:09, Menglin wrote:
> On 2016/08/16 20:41:04, nyquist wrote:
> > Did you consider doing this in the constructor instead? That would probably
> mean
> > passing a raw pointer of the feature to BlimpContentsManager of course.
> 
> Do you mean BlimpContentsImpl::BlimpContentsImpl(int id, TabControlFeature*
> tab_control_feature) so that a BlimpContentsImpl is guaranteed to have a
> TabControlFeature? Yeah if that's the case, we can do this in the constructor.
> 
> question not for this CL but in the future, are the other features (navigation
> feature, ime feature) going to be added in similar way?

Yes, that's what I meant. I don't feel strongly about it though, but it surely
seems like a dependency of BlimpContentsImpl, so it feels like it should be in
the constructor.

And about the future: Yeah, they will have to be done in a similar manner.

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

https://codereview.chromium.org/2255533002/diff/1/blimp/client/core/contents/...
blimp/client/core/contents/blimp_contents_impl.h:53: void
SetTabControlFeature(TabControlFeature* tab_control_feature);
On 2016/08/16 21:10:09, Menglin wrote:
> On 2016/08/16 20:41:04, nyquist wrote:
> > seems like this could just be set_tab_control_feature and inlined?
> 
> if we pass in the tab control feature in BlimpContentsImpl's constructor, we
> don't need this function any more

yeah

https://codereview.chromium.org/2255533002/diff/1/blimp/client/public/content...
File blimp/client/public/contents/blimp_contents.h (right):

https://codereview.chromium.org/2255533002/diff/1/blimp/client/public/content...
blimp/client/public/contents/blimp_contents.h:37: virtual void
SetSizeAndScale(const gfx::Size& size,
On 2016/08/16 21:10:09, Menglin wrote:
> On 2016/08/16 20:41:04, nyquist wrote:
> > Could this possibly be moved to the impl instead? I think size and scale can
> > come from the BlimpView. If so, remember to move the comment over.
> 
> impl has this function now. it overrides this the virtual function here. If we
> only need to call it on BlimpContentsImpl, i can remove the virtual function
> from here

yeah, you should be able to do that.

[Menglin, 2016-08-17 19:48:45]

[email protected] changed reviewers:
+ [email protected]

[Menglin, 2016-08-17 19:48:46]

[email protected]: Please review changes in 
ui/gfx/geometry/size.h

Hi Tommy and David,

Please look at Patch 3. 
Also I learned if I want to use gfx:Size in the test code, I should include
dependency //ui/gfx:test_support

Thanks

Menglin

https://codereview.chromium.org/2255533002/diff/1/blimp/client/core/android/b...
File blimp/client/core/android/blimp_client_context_impl_android.cc (right):

https://codereview.chromium.org/2255533002/diff/1/blimp/client/core/android/b...
blimp/client/core/android/blimp_client_context_impl_android.cc:58:
GetTabControlFeature()->CreateTab(kDummyTabId);
On 2016/08/16 21:41:40, nyquist wrote:
> On 2016/08/16 21:10:08, Menglin wrote:
> > On 2016/08/16 20:41:04, nyquist wrote:
> > > Could this be in the cross-platform code?
> > 
> > you mean BlimpClientContextImpl? I think so. I added it this way because i
saw
> > this pattern between BlimpClientSession and BlimpClientSessionAndroid.
> 
> Right, but I don't think that class is necessarily something to guide our
lives
> towards :-p
> 
> This doesn't seem to be Android-specific.

Done.

https://codereview.chromium.org/2255533002/diff/1/blimp/client/core/blimp_cli...
File blimp/client/core/blimp_client_context_impl.cc (right):

https://codereview.chromium.org/2255533002/diff/1/blimp/client/core/blimp_cli...
blimp/client/core/blimp_client_context_impl.cc:87:
blimp_contents_impl->SetTabControlFeature(tab_control_feature_.get());
On 2016/08/16 21:41:40, nyquist wrote:
> On 2016/08/16 21:10:09, Menglin wrote:
> > On 2016/08/16 20:41:04, nyquist wrote:
> > > Did you consider doing this in the constructor instead? That would
probably
> > mean
> > > passing a raw pointer of the feature to BlimpContentsManager of course.
> > 
> > Do you mean BlimpContentsImpl::BlimpContentsImpl(int id, TabControlFeature*
> > tab_control_feature) so that a BlimpContentsImpl is guaranteed to have a
> > TabControlFeature? Yeah if that's the case, we can do this in the
constructor.
> > 
> > question not for this CL but in the future, are the other features
(navigation
> > feature, ime feature) going to be added in similar way?
> 
> Yes, that's what I meant. I don't feel strongly about it though, but it surely
> seems like a dependency of BlimpContentsImpl, so it feels like it should be in
> the constructor.
> 
> And about the future: Yeah, they will have to be done in a similar manner.
> 

Done.

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

https://codereview.chromium.org/2255533002/diff/1/blimp/client/core/contents/...
blimp/client/core/contents/blimp_contents_impl.h:53: void
SetTabControlFeature(TabControlFeature* tab_control_feature);
On 2016/08/16 21:41:40, nyquist wrote:
> On 2016/08/16 21:10:09, Menglin wrote:
> > On 2016/08/16 20:41:04, nyquist wrote:
> > > seems like this could just be set_tab_control_feature and inlined?
> > 
> > if we pass in the tab control feature in BlimpContentsImpl's constructor, we
> > don't need this function any more
> 
> yeah

Done.

https://codereview.chromium.org/2255533002/diff/1/blimp/client/public/content...
File blimp/client/public/contents/blimp_contents.h (right):

https://codereview.chromium.org/2255533002/diff/1/blimp/client/public/content...
blimp/client/public/contents/blimp_contents.h:37: virtual void
SetSizeAndScale(const gfx::Size& size,
On 2016/08/16 21:41:40, nyquist wrote:
> On 2016/08/16 21:10:09, Menglin wrote:
> > On 2016/08/16 20:41:04, nyquist wrote:
> > > Could this possibly be moved to the impl instead? I think size and scale
can
> > > come from the BlimpView. If so, remember to move the comment over.
> > 
> > impl has this function now. it overrides this the virtual function here. If
we
> > only need to call it on BlimpContentsImpl, i can remove the virtual function
> > from here
> 
> yeah, you should be able to do that.

Done.

[danakj, 2016-08-17 19:50:52]

https://codereview.chromium.org/2255533002/diff/40001/ui/gfx/geometry/size.h
File ui/gfx/geometry/size.h (right):

https://codereview.chromium.org/2255533002/diff/40001/ui/gfx/geometry/size.h#...
ui/gfx/geometry/size.h:84: // test. This should not be used in production code -
call ToString() instead.
Would you mind fixing other cases of this comment? Like
https://cs.chromium.org/chromium/src/ui/gfx/geometry/rect.h?rcl=0&l=273 and
friends.

LGTM tho, thanks!

[Menglin, 2016-08-17 21:05:59]

On 2016/08/17 19:50:52, danakj wrote:
> https://codereview.chromium.org/2255533002/diff/40001/ui/gfx/geometry/size.h
> File ui/gfx/geometry/size.h (right):
> 
>
https://codereview.chromium.org/2255533002/diff/40001/ui/gfx/geometry/size.h#...
> ui/gfx/geometry/size.h:84: // test. This should not be used in production code
-
> call ToString() instead.
> Would you mind fixing other cases of this comment? Like
> https://cs.chromium.org/chromium/src/ui/gfx/geometry/rect.h?rcl=0&l=273 and
> friends.
> 
> LGTM tho, thanks!

OK. I'll change all other comments in the next patch after I receive feedback on
the blimp code from Tommy/David

[David Trainor- moved to gerrit, 2016-08-18 17:41:58]

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

https://codereview.chromium.org/2255533002/diff/20001/blimp/client/core/blimp...
blimp/client/core/blimp_client_context_impl.cc:75:
GetTabControlFeature()->CreateTab(kDummyTabId);
Don't do this here.  We 100% need to rely on the BlimpContentsManager to
actually drive the TabControlManager.  We also don't want to do this in the
blimp/ layer at all.  Let the Chrome layer drive this.

https://codereview.chromium.org/2255533002/diff/20001/blimp/client/core/blimp...
blimp/client/core/blimp_client_context_impl.cc:88:
blimp_contents_manager_->CreateBlimpContents(tab_control_feature_.get());
Should we pass this into BlimpContentsManager's constructor?

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

https://codereview.chromium.org/2255533002/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_impl.h:57: TabControlFeature*
tab_control_feature() { return tab_control_feature_; }
Do we need to expose this here?  Could we just rely on passing the right thing
into the constructor?

https://codereview.chromium.org/2255533002/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_impl.h:74: TabControlFeature*
tab_control_feature_ = nullptr;
Can we add a TODO and a bug to get rid of this and use the right feature to pass
size and scale information when we refactor the protocols?

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

https://codereview.chromium.org/2255533002/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_manager.cc:42: BlimpContents* contents
= blimp_contents();
It might make sense to change this now.  We don't properly interact with the
TabControlFeature here.  We probably want:

blimp_contents_manager_->CloseTab(blimp_contents);

CloseTab can properly remove call tab_control_feature->CloseTab(id) and can then
post a task to remove the observer from the map.

https://codereview.chromium.org/2255533002/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_manager.cc:54:
std::unique_ptr<BlimpContentsImpl> BlimpContentsManager::CreateBlimpContents(
This class should also call tab_control_feature->CreateTab.

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

https://codereview.chromium.org/2255533002/diff/40001/blimp/client/core/blimp...
blimp/client/core/blimp_client_context_impl.cc:75:
GetTabControlFeature()->CreateTab(kDummyTabId);
We don't need this.  It'll be driven by the Chrome layer.

[Menglin, 2016-08-18 18:59:06]

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

https://codereview.chromium.org/2255533002/diff/20001/blimp/client/core/blimp...
blimp/client/core/blimp_client_context_impl.cc:88:
blimp_contents_manager_->CreateBlimpContents(tab_control_feature_.get());
On 2016/08/18 17:41:58, David Trainor wrote:
> Should we pass this into BlimpContentsManager's constructor?

You mean BlimpContentsManager will keep a TabControlFeature* as well?

[Menglin, 2016-08-19 00:50:21]

In patch 4, i address all David's comments. David, ptal.

danakj, I also update all the comments related to //ui/gfx:test_support

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

https://codereview.chromium.org/2255533002/diff/20001/blimp/client/core/blimp...
blimp/client/core/blimp_client_context_impl.cc:75:
GetTabControlFeature()->CreateTab(kDummyTabId);
On 2016/08/18 17:41:58, David Trainor wrote:
> Don't do this here.  We 100% need to rely on the BlimpContentsManager to
> actually drive the TabControlManager.  We also don't want to do this in the
> blimp/ layer at all.  Let the Chrome layer drive this.

Done.

https://codereview.chromium.org/2255533002/diff/20001/blimp/client/core/blimp...
blimp/client/core/blimp_client_context_impl.cc:88:
blimp_contents_manager_->CreateBlimpContents(tab_control_feature_.get());
On 2016/08/18 18:59:06, Menglin wrote:
> On 2016/08/18 17:41:58, David Trainor wrote:
> > Should we pass this into BlimpContentsManager's constructor?
> 
> You mean BlimpContentsManager will keep a TabControlFeature* as well?

Done.

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

https://codereview.chromium.org/2255533002/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_impl.h:57: TabControlFeature*
tab_control_feature() { return tab_control_feature_; }
On 2016/08/18 17:41:58, David Trainor wrote:
> Do we need to expose this here?  Could we just rely on passing the right thing
> into the constructor?

Done.

https://codereview.chromium.org/2255533002/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_impl.h:74: TabControlFeature*
tab_control_feature_ = nullptr;
On 2016/08/18 17:41:58, David Trainor wrote:
> Can we add a TODO and a bug to get rid of this and use the right feature to
pass
> size and scale information when we refactor the protocols?

Done.

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

https://codereview.chromium.org/2255533002/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_manager.cc:42: BlimpContents* contents
= blimp_contents();
On 2016/08/18 17:41:58, David Trainor wrote:
> It might make sense to change this now.  We don't properly interact with the
> TabControlFeature here.  We probably want:
> 
> blimp_contents_manager_->CloseTab(blimp_contents);
> 
> CloseTab can properly remove call tab_control_feature->CloseTab(id) and can
then
> post a task to remove the observer from the map.

Done.

https://codereview.chromium.org/2255533002/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_manager.cc:54:
std::unique_ptr<BlimpContentsImpl> BlimpContentsManager::CreateBlimpContents(
On 2016/08/18 17:41:58, David Trainor wrote:
> This class should also call tab_control_feature->CreateTab.

Done.

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

https://codereview.chromium.org/2255533002/diff/40001/blimp/client/core/blimp...
blimp/client/core/blimp_client_context_impl.cc:75:
GetTabControlFeature()->CreateTab(kDummyTabId);
On 2016/08/18 17:41:58, David Trainor wrote:
> We don't need this.  It'll be driven by the Chrome layer.

Done.

[danakj, 2016-08-19 00:51:19]

On 2016/08/19 00:50:21, Menglin wrote:
> In patch 4, i address all David's comments. David, ptal.
> 
> danakj, I also update all the comments related to //ui/gfx:test_support

Thank you!

[David Trainor- moved to gerrit, 2016-08-19 20:55:05]

lgtm % nits.  Thanks! :)

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

https://codereview.chromium.org/2255533002/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_manager.cc:60: // Create an observer
entry for the contents
. at end

https://codereview.chromium.org/2255533002/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_manager.cc:67: // Creates a tab for
the contents.
Maybe: Notifies the engine that we've created a new BlimpContents

https://codereview.chromium.org/2255533002/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_manager.cc:100: // Close the contents'
tab
Maybe: Notify the engine that we've destroyed the BlimpContents.

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

https://codereview.chromium.org/2255533002/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_manager.h:23: // Create a new
BlimpContentsImpl, owns it, and create a tab for it.
This comment is a bit confusing.  This class doesn't end up owning the
BlimpContents or creating a tab (it does notify the engine though).  I might
word it like:

// Builds a BlimpContentsImpl and notifies the engine.

[Menglin, 2016-08-19 21:33:02]

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

https://codereview.chromium.org/2255533002/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_manager.cc:60: // Create an observer
entry for the contents
On 2016/08/19 20:55:05, David Trainor wrote:
> . at end

Done.

https://codereview.chromium.org/2255533002/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_manager.cc:67: // Creates a tab for
the contents.
On 2016/08/19 20:55:05, David Trainor wrote:
> Maybe: Notifies the engine that we've created a new BlimpContents

Done.

https://codereview.chromium.org/2255533002/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_manager.cc:100: // Close the contents'
tab
On 2016/08/19 20:55:05, David Trainor wrote:
> Maybe: Notify the engine that we've destroyed the BlimpContents.

Done.

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

https://codereview.chromium.org/2255533002/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_manager.h:23: // Create a new
BlimpContentsImpl, owns it, and create a tab for it.
On 2016/08/19 20:55:05, David Trainor wrote:
> This comment is a bit confusing.  This class doesn't end up owning the
> BlimpContents or creating a tab (it does notify the engine though).  I might
> word it like:
> 
> // Builds a BlimpContentsImpl and notifies the engine.

Done.

[Menglin, 2016-08-19 21:33:36]

The CQ bit was checked by [email protected]

[Menglin, 2016-08-19 21:33:36]

The patchset sent to the CQ was uploaded after l-g-t-m from [email protected],
[email protected]
Link to the patchset: https://codereview.chromium.org/2255533002/#ps80001
(title: "nits and sync to head")

[commit-bot: I haz the power, 2016-08-19 21:34:47]

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-19 21:40:17]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2016-08-19 21:40:19]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Menglin, 2016-08-19 22:49:41]

The CQ bit was checked by [email protected]

[Menglin, 2016-08-19 22:49:42]

The patchset sent to the CQ was uploaded after l-g-t-m from [email protected],
[email protected]
Link to the patchset: https://codereview.chromium.org/2255533002/#ps120001
(title: "sync to head")

[commit-bot: I haz the power, 2016-08-19 22:50:15]

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-20 00:47:50]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2016-08-20 00:47:52]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Menglin, 2016-08-20 00:59:15]

The CQ bit was checked by [email protected]

[commit-bot: I haz the power, 2016-08-20 00:59:48]

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-20 02:18:12]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-08-20 02:19:35]

Description was changed from

==========
Add TabControlFeature to BlimpClientContextImpl

This CL includes:
1. BlimpClientContextImpl owns the TabControlFeature, and register the
feature in its constructor.
2. When a BlimpContents is created, set a pointer to TabControlFeature
in BlimpContentsImpl.
3. Add a method to SetSizeAndScale in BlimpContents.
4. call CreateTab in BlimpClientContextImplAndroid.

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

to

==========
Add TabControlFeature to BlimpClientContextImpl

This CL includes:
1. BlimpClientContextImpl owns the TabControlFeature, and register the
feature in its constructor.
2. When a BlimpContents is created, set a pointer to TabControlFeature
in BlimpContentsImpl.
3. Add a method to SetSizeAndScale in BlimpContents.
4. call CreateTab in BlimpClientContextImplAndroid.

BUG=628802

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

[commit-bot: I haz the power, 2016-08-20 02:19:36]

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