Add tab reparenting

Add tab reparenting

- Adds updateWindowAndroid to ContentViewCore native and java
- Uses the above for calling onAttachedToWindow and onDetachedFromWindow
  for CVC observers
- Add updateDelegates to TabAndroid
- Uses all above for adding Tab#reparentToActivity detaching from old
  activity and related objects and attaching to new activity
- Add AsyncTabParamsManager call to check whether there is a saved tab.
- Uses all above to add reparenting to CustomTabActivity to other
  activities.

BUG=584712
Committed: https://crrev.com/e454b1df52fa75a27583a02f85070bdca44f28c6
Cr-Commit-Position: refs/heads/master@{#378027}

[Yusuf, 2016-02-24 01:15:59]

yusufo@chromium.org changed reviewers:
+ dfalcantara@chromium.org, sievers@chromium.org, tedchoc@chromium.org

[Yusuf, 2016-02-24 01:16:00]

sievers@ for render_widget_host_view_android

tedchoc@ for content/android

dfalcantara@ for all else

[Yusuf, 2016-02-24 16:42:31]

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

[commit-bot: I haz the power, 2016-02-24 16:44:04]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1731673002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1731673002/1

[gone, 2016-02-24 18:40:38]

Looked mainly at the Java stuff, but I did a layman's pass on the C++ stuff.

https://chromiumcodereview.appspot.com/1731673002/diff/1/chrome/android/java/...
File
chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java
(right):

https://chromiumcodereview.appspot.com/1731673002/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java:679:
if (!AsyncTabParamsManager.hasParamsWithTabToReparent()) {
Why is this case special?  If you're trying to avoid overwriting existing
params, shouldn't it be doing something with the tab ID instead of just checking
if there are _any_ params with something to reparent?

https://chromiumcodereview.appspot.com/1731673002/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java:703:
AsyncTabParamsManager.remove(tabId);
You guarded the case above; do you have to do something special here?  Would you
have to destroy the Tab in that case?

https://chromiumcodereview.appspot.com/1731673002/diff/1/chrome/android/java/...
File
chrome/android/java/src/org/chromium/chrome/browser/document/DocumentActivity.java
(right):

https://chromiumcodereview.appspot.com/1731673002/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/document/DocumentActivity.java:580:
if (AsyncTabParamsManager.hasParamsWithTabToReparent()) {
Why can't you combine your extra getting logic with
ActivityDelegate#getTabIdFromIntent()?  It could be a check above the
DOCUMENT_SCHEME check.  That would prevent you from having to try to remove
something from the AsyncTabParamsManager twice.  If you did that, then you
wouldn't have to randomly check if the AsyncTabParamsManager had any random tab
to reparent and could check for the correct one directly.

https://chromiumcodereview.appspot.com/1731673002/diff/1/chrome/android/java/...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://chromiumcodereview.appspot.com/1731673002/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1371: // Update
for the controllers that needs the Compositor from the new Activity.
needs -> need

https://chromiumcodereview.appspot.com/1731673002/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1383:
mAppBannerManager = mDelegateFactory.createAppBannerManager(this);
You're not calling mAppBannerManager.destroy() on the old one, which has a
native pointer.  Are you missing any other destroy() calls for the ones you're
replacing?

https://chromiumcodereview.appspot.com/1731673002/diff/1/chrome/android/java/...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/SingleTabModel.java
(right):

https://chromiumcodereview.appspot.com/1731673002/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/SingleTabModel.java:191:

nit: I've got a blank space, baby.

https://chromiumcodereview.appspot.com/1731673002/diff/1/chrome/android/java/...
File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModel.java
(right):

https://chromiumcodereview.appspot.com/1731673002/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModel.java:53: *
Changed windows by moving from one activity to another. Will be opened in the f
nit: f------?

https://chromiumcodereview.appspot.com/1731673002/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModel.java:192:
* @return Whether removing a tab is supported.
I don't think @return means what you think it means.

https://chromiumcodereview.appspot.com/1731673002/diff/1/chrome/android/java/...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelImpl.java
(right):

https://chromiumcodereview.appspot.com/1731673002/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelImpl.java:127:

nit: remove whitespace

https://chromiumcodereview.appspot.com/1731673002/diff/1/chrome/android/javat...
File chrome/android/javatests/src/org/chromium/chrome/browser/TabTest.java
(right):

https://chromiumcodereview.appspot.com/1731673002/diff/1/chrome/android/javat...
chrome/android/javatests/src/org/chromium/chrome/browser/TabTest.java:58:
assertFalse("The tab context can not be an activity",
can not -> cannot

https://chromiumcodereview.appspot.com/1731673002/diff/1/chrome/browser/andro...
File chrome/browser/android/tab_android.cc (right):

https://chromiumcodereview.appspot.com/1731673002/diff/1/chrome/browser/andro...
chrome/browser/android/tab_android.cc:447:
web_contents_delegate_->LoadProgressChanged(web_contents(), 0);
What happens if the tab is reparented while the tab is loading?  Setting this to
0 seems incorrect.

https://chromiumcodereview.appspot.com/1731673002/diff/1/content/browser/andr...
File content/browser/android/content_view_core_impl.cc (right):

https://chromiumcodereview.appspot.com/1731673002/diff/1/content/browser/andr...
content/browser/android/content_view_core_impl.cc:280: if (!window_android) {
1) Do you not have to set window_android_ to null?
2) Flip the conditional ordering so that positive is on top.

[Yusuf, 2016-02-24 19:40:36]

https://chromiumcodereview.appspot.com/1731673002/diff/1/chrome/android/java/...
File
chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java
(right):

https://chromiumcodereview.appspot.com/1731673002/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java:679:
if (!AsyncTabParamsManager.hasParamsWithTabToReparent()) {
On 2016/02/24 18:40:37, dfalcantara wrote:
> Why is this case special?  If you're trying to avoid overwriting existing
> params, shouldn't it be doing something with the tab ID instead of just
checking
> if there are _any_ params with something to reparent?

Done.

https://chromiumcodereview.appspot.com/1731673002/diff/1/chrome/android/java/...
File
chrome/android/java/src/org/chromium/chrome/browser/document/DocumentActivity.java
(right):

https://chromiumcodereview.appspot.com/1731673002/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/document/DocumentActivity.java:580:
if (AsyncTabParamsManager.hasParamsWithTabToReparent()) {
On 2016/02/24 18:40:37, dfalcantara wrote:
> Why can't you combine your extra getting logic with
> ActivityDelegate#getTabIdFromIntent()?  It could be a check above the
> DOCUMENT_SCHEME check.  That would prevent you from having to try to remove
> something from the AsyncTabParamsManager twice.  If you did that, then you
> wouldn't have to randomly check if the AsyncTabParamsManager had any random
tab
> to reparent and could check for the correct one directly.

Done.

https://chromiumcodereview.appspot.com/1731673002/diff/1/chrome/android/java/...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://chromiumcodereview.appspot.com/1731673002/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1371: // Update
for the controllers that needs the Compositor from the new Activity.
On 2016/02/24 18:40:37, dfalcantara wrote:
> needs -> need

Done.

https://chromiumcodereview.appspot.com/1731673002/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1383:
mAppBannerManager = mDelegateFactory.createAppBannerManager(this);
On 2016/02/24 18:40:37, dfalcantara wrote:
> You're not calling mAppBannerManager.destroy() on the old one, which has a
> native pointer.  Are you missing any other destroy() calls for the ones you're
> replacing?
Done.
No, the appbannermanager is the only one. And I had totally checked this all
before you told me to...

https://chromiumcodereview.appspot.com/1731673002/diff/1/chrome/android/java/...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/SingleTabModel.java
(right):

https://chromiumcodereview.appspot.com/1731673002/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/SingleTabModel.java:191:

On 2016/02/24 18:40:37, dfalcantara wrote:
> nit: I've got a blank space, baby.

Done.

https://chromiumcodereview.appspot.com/1731673002/diff/1/chrome/android/java/...
File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModel.java
(right):

https://chromiumcodereview.appspot.com/1731673002/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModel.java:53: *
Changed windows by moving from one activity to another. Will be opened in the f
On 2016/02/24 18:40:37, dfalcantara wrote:
> nit: f------?

Done.

https://chromiumcodereview.appspot.com/1731673002/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModel.java:192:
* @return Whether removing a tab is supported.
On 2016/02/24 18:40:37, dfalcantara wrote:
> I don't think @return means what you think it means.

Done.

https://chromiumcodereview.appspot.com/1731673002/diff/1/chrome/android/java/...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelImpl.java
(right):

https://chromiumcodereview.appspot.com/1731673002/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelImpl.java:127:

On 2016/02/24 18:40:37, dfalcantara wrote:
> nit: remove whitespace

Done.

https://chromiumcodereview.appspot.com/1731673002/diff/1/chrome/android/javat...
File chrome/android/javatests/src/org/chromium/chrome/browser/TabTest.java
(right):

https://chromiumcodereview.appspot.com/1731673002/diff/1/chrome/android/javat...
chrome/android/javatests/src/org/chromium/chrome/browser/TabTest.java:58:
assertFalse("The tab context can not be an activity",
On 2016/02/24 18:40:37, dfalcantara wrote:
> can not -> cannot

Done.

https://chromiumcodereview.appspot.com/1731673002/diff/1/content/browser/andr...
File content/browser/android/content_view_core_impl.cc (right):

https://chromiumcodereview.appspot.com/1731673002/diff/1/content/browser/andr...
content/browser/android/content_view_core_impl.cc:280: if (!window_android) {
On 2016/02/24 18:40:37, dfalcantara wrote:
> 1) Do you not have to set window_android_ to null?
> 2) Flip the conditional ordering so that positive is on top.

1) Yes, I should. (and it still works! Phew!)
2)Done.

[gone, 2016-02-24 21:01:37]

lgtm on my end

https://chromiumcodereview.appspot.com/1731673002/diff/20001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/ActivityDelegate.java
(right):

https://chromiumcodereview.appspot.com/1731673002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/ActivityDelegate.java:129:
if (AsyncTabParamsManager.hasParamsWithTabToReparent()) {
Still really gross that you need to guard with this, but I guess I understand
why.  Please add a comment.

https://chromiumcodereview.appspot.com/1731673002/diff/20001/content/browser/...
File content/browser/android/content_view_core_impl.cc (right):

https://chromiumcodereview.appspot.com/1731673002/diff/20001/content/browser/...
content/browser/android/content_view_core_impl.cc:287: observer_list_,
nit: indentation is wonky.

[Yusuf, 2016-02-24 21:07:37]

https://chromiumcodereview.appspot.com/1731673002/diff/20001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/ActivityDelegate.java
(right):

https://chromiumcodereview.appspot.com/1731673002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/ActivityDelegate.java:129:
if (AsyncTabParamsManager.hasParamsWithTabToReparent()) {
On 2016/02/24 21:01:37, dfalcantara wrote:
> Still really gross that you need to guard with this, but I guess I understand
> why.  Please add a comment.

Done.

https://chromiumcodereview.appspot.com/1731673002/diff/20001/content/browser/...
File content/browser/android/content_view_core_impl.cc (right):

https://chromiumcodereview.appspot.com/1731673002/diff/20001/content/browser/...
content/browser/android/content_view_core_impl.cc:287: observer_list_,
On 2016/02/24 21:01:37, dfalcantara wrote:
> nit: indentation is wonky.

Done.

[no sievers, 2016-02-25 19:23:16]

https://codereview.chromium.org/1731673002/diff/60001/content/browser/android...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/1731673002/diff/60001/content/browser/android...
content/browser/android/content_view_core_impl.cc:280: if (window_android) {
What about |window_android_ != null|?

If that is not supposed to happen, can you DCHECK?
Otherwise we might still want to notify OnDetachedFromWindow() first.

https://codereview.chromium.org/1731673002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/1731673002/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1763:
content_view_core_->RemoveObserver(this);
So that fixes the else-path where we might not have attached to the new CVC
previously if |content_view_core_| was null.

But also: why do we want to stay attached to |content_view_core_| if
|content_view_core| is not null?

[Yusuf, 2016-02-25 19:37:03]

https://codereview.chromium.org/1731673002/diff/60001/content/browser/android...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/1731673002/diff/60001/content/browser/android...
content/browser/android/content_view_core_impl.cc:280: if (window_android) {
On 2016/02/25 19:23:16, sievers wrote:
> What about |window_android_ != null|?
> 
> If that is not supposed to happen, can you DCHECK?
> Otherwise we might still want to notify OnDetachedFromWindow() first.

Good point. I would rather DCHECK. Whoever uses this call should be setting it
to null first on the old activity.

https://codereview.chromium.org/1731673002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/1731673002/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1763:
content_view_core_->RemoveObserver(this);
On 2016/02/25 19:23:16, sievers wrote:
> So that fixes the else-path where we might not have attached to the new CVC
> previously if |content_view_core_| was null.
> 
> But also: why do we want to stay attached to |content_view_core_| if
> |content_view_core| is not null?

Another good point. Right now, we never call a single widget with multiple CVCs
I believe. That is why I didn't add it. Added a condition for it as well just in
case.

[no sievers, 2016-02-25 19:46:36]

lgtm

[Ted C, 2016-02-25 20:05:22]

https://chromiumcodereview.appspot.com/1731673002/diff/60001/chrome/android/j...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(right):

https://chromiumcodereview.appspot.com/1731673002/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:701: if
(mCompositorViewHolder != null &&
AsyncTabParamsManager.hasParamsWithTabToReparent()) {
Looking at the code, it isn't possible for this to be null.

mCompositorViewHolder is set in setContentView, which is final.  And it is
called from AsyncInitializationActivity#onCreate.

I think all the null checks within this file must be carried over from before
all converging on using the compositor.

Let's buck the trend and not add another!  :-)

https://chromiumcodereview.appspot.com/1731673002/diff/60001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java
(right):

https://chromiumcodereview.appspot.com/1731673002/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java:310:
public void onActiveTabReparented() {
I would call this prepareForTabReparenting or something.

https://chromiumcodereview.appspot.com/1731673002/diff/60001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
(right):

https://chromiumcodereview.appspot.com/1731673002/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:655:
getCurrentTabModel().removeTab(tab);
what does CCT do when it has no tabs left it in?

(like if you use mMainTab from your other change after this gets removed).

https://chromiumcodereview.appspot.com/1731673002/diff/60001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java
(right):

https://chromiumcodereview.appspot.com/1731673002/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java:705:
asyncParams.getTabToReparent().destroy();
we might want to consider a destroy on the async params themselves.

https://chromiumcodereview.appspot.com/1731673002/diff/60001/chrome/android/j...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://chromiumcodereview.appspot.com/1731673002/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1378:
mWebContentsDelegate = mDelegateFactory.createWebContentsDelegate(this);
can the bulk of this method be shared w/ the default construction path?  I would
be nice to make sure things don't diverge and have one way of handling clearing
old state if needed, etc.., etc...

https://chromiumcodereview.appspot.com/1731673002/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1386: if
(oldActivity != null && !oldActivity.isActivityDestroyed())
oldActivity.finish();
this is...umm...scary.

I don't think this is where this should be done.

If anything, I think the other activity should be able to add a callback to the
asynctabparams and be notified when it is consumed.

https://chromiumcodereview.appspot.com/1731673002/diff/60001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/AsyncTabParamsManager.java
(right):

https://chromiumcodereview.appspot.com/1731673002/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/AsyncTabParamsManager.java:31:
public static boolean peek(int tabId) {
i like the previous name personally

unless you want to add another method call aboo.  Then i'm on board.

https://chromiumcodereview.appspot.com/1731673002/diff/80001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/ChromeTabCreator.java
(right):

https://chromiumcodereview.appspot.com/1731673002/diff/80001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/ChromeTabCreator.java:115:
|| webContents != null;
if you are reparenting, don't you always want to open in the foreground?

https://chromiumcodereview.appspot.com/1731673002/diff/80001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/ChromeTabCreator.java:119:
if (tab != null) {
I would be tempted to move the assignment of tab and webcontents into these
blocks.

In my opinion, Tab should be the undefined variable.  WebContents should be
removed and used only within the conditionals that deal with them.

if (asyncParams.getTabToReparent() != null) {
    tab = ...
    
} else if (asyncParams.getWebContents() != null) {
    openInForeground = true;
    ....
} else ...

https://chromiumcodereview.appspot.com/1731673002/diff/80001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelImpl.java
(right):

https://chromiumcodereview.appspot.com/1731673002/diff/80001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelImpl.java:88:
mTabs.remove(tab);
at this point, mIndex is invalid.

"something" should probably happen.

I wouldn't be surprised if observers should be notified as well, but I guess we
can add that as needed.

I again wonder if we "shouldn't" be removing a tab from the model until it is
used by the asynctabparams.  Then this is happening immediately before
finish/destroy call on the activity.

Sigh...i'm not sure what is best here.

[Yusuf, 2016-02-25 23:05:00]

https://codereview.chromium.org/1731673002/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(right):

https://codereview.chromium.org/1731673002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:701: if
(mCompositorViewHolder != null &&
AsyncTabParamsManager.hasParamsWithTabToReparent()) {
On 2016/02/25 20:05:21, Ted C wrote:
> Looking at the code, it isn't possible for this to be null.
> 
> mCompositorViewHolder is set in setContentView, which is final.  And it is
> called from AsyncInitializationActivity#onCreate.
> 
> I think all the null checks within this file must be carried over from before
> all converging on using the compositor.
> 
> Let's buck the trend and not add another!  :-)

Done.

https://codereview.chromium.org/1731673002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java
(right):

https://codereview.chromium.org/1731673002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java:310:
public void onActiveTabReparented() {
On 2016/02/25 20:05:21, Ted C wrote:
> I would call this prepareForTabReparenting or something.

Done.

https://codereview.chromium.org/1731673002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
(right):

https://codereview.chromium.org/1731673002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:655:
getCurrentTabModel().removeTab(tab);
On 2016/02/25 20:05:21, Ted C wrote:
> what does CCT do when it has no tabs left it in?
> 
> (like if you use mMainTab from your other change after this gets removed).

If this activity doesn't get destroyed, it is essentially an empty shell with a
toolbar and blank content area.

https://codereview.chromium.org/1731673002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java
(right):

https://codereview.chromium.org/1731673002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java:705:
asyncParams.getTabToReparent().destroy();
On 2016/02/25 20:05:22, Ted C wrote:
> we might want to consider a destroy on the async params themselves.

Done.

https://codereview.chromium.org/1731673002/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://codereview.chromium.org/1731673002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1378:
mWebContentsDelegate = mDelegateFactory.createWebContentsDelegate(this);
On 2016/02/25 20:05:22, Ted C wrote:
> can the bulk of this method be shared w/ the default construction path?  I
would
> be nice to make sure things don't diverge and have one way of handling
clearing
> old state if needed, etc.., etc...

No?.... :S I mean yes, but it gets so hairy that becomes a pretty large change.
WindowAndroid is in constructor. AttachingTabContentManager is initialize.
FullscreenManager has an individual setter. Delegates are all in
setContentViewCore which gets called multiple times possibly, but is also in
initialize. Then two of the delegates are initialized as a part of
initWebContents. We can call the new one there except for the new one we don't
set the progress to 0.

TODO?

https://codereview.chromium.org/1731673002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1386: if
(oldActivity != null && !oldActivity.isActivityDestroyed())
oldActivity.finish();
On 2016/02/25 20:05:22, Ted C wrote:
> this is...umm...scary.
> 
> I don't think this is where this should be done.
> 
> If anything, I think the other activity should be able to add a callback to
the
> asynctabparams and be notified when it is consumed.

Done.

https://codereview.chromium.org/1731673002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/AsyncTabParamsManager.java
(right):

https://codereview.chromium.org/1731673002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/AsyncTabParamsManager.java:31:
public static boolean peek(int tabId) {
On 2016/02/25 20:05:22, Ted C wrote:
> i like the previous name personally
> 
> unless you want to add another method call aboo.  Then i'm on board.

Tempting....

Done.

https://codereview.chromium.org/1731673002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/ChromeTabCreator.java
(right):

https://codereview.chromium.org/1731673002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/ChromeTabCreator.java:115:
|| webContents != null;
On 2016/02/25 20:05:22, Ted C wrote:
> if you are reparenting, don't you always want to open in the foreground?

Done.

https://codereview.chromium.org/1731673002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/ChromeTabCreator.java:119:
if (tab != null) {
On 2016/02/25 20:05:22, Ted C wrote:
> I would be tempted to move the assignment of tab and webcontents into these
> blocks.
> 
> In my opinion, Tab should be the undefined variable.  WebContents should be
> removed and used only within the conditionals that deal with them.
> 
> if (asyncParams.getTabToReparent() != null) {
>     tab = ...
>     
> } else if (asyncParams.getWebContents() != null) {
>     openInForeground = true;
>     ....
> } else ...

Done.

https://codereview.chromium.org/1731673002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelImpl.java
(right):

https://codereview.chromium.org/1731673002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelImpl.java:88:
mTabs.remove(tab);
On 2016/02/25 20:05:22, Ted C wrote:
> at this point, mIndex is invalid.
> 
> "something" should probably happen.
> 
> I wouldn't be surprised if observers should be notified as well, but I guess
we
> can add that as needed.
> 
> I again wonder if we "shouldn't" be removing a tab from the model until it is
> used by the asynctabparams.  Then this is happening immediately before
> finish/destroy call on the activity.
> 
> Sigh...i'm not sure what is best here.

This one is a bit tricky to handle, since not removing it when the activity gets
paused causes the tab to react. To fake the tab not moving, we have to deselect
it and severe its connections to the activity somehow.

Added a todo about this, but can look at it further if we are concerned with
this landing as is. Right now I don't have a solution for tab not hiding itself
if we call remove in onDestroy.

[Yusuf, 2016-02-26 02:58:55]

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

[commit-bot: I haz the power, 2016-02-26 03:03:11]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1731673002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1731673002/140001

[commit-bot: I haz the power, 2016-02-26 05:58:10]

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

[commit-bot: I haz the power, 2016-02-26 05:58:11]

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

[Yusuf, 2016-02-26 07:26:43]

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

[commit-bot: I haz the power, 2016-02-26 07:26:58]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1731673002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1731673002/160001

[commit-bot: I haz the power, 2016-02-26 07:47:12]

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

[commit-bot: I haz the power, 2016-02-26 07:47:13]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)

[Yusuf, 2016-02-26 08:03:26]

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

[commit-bot: I haz the power, 2016-02-26 08:04:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1731673002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1731673002/180001

[commit-bot: I haz the power, 2016-02-26 11:23:18]

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

[commit-bot: I haz the power, 2016-02-26 11:23:19]

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

[Yusuf, 2016-02-26 18:07:24]

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

[commit-bot: I haz the power, 2016-02-26 18:08:31]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1731673002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1731673002/200001

[Ted C, 2016-02-26 18:46:29]

https://codereview.chromium.org/1731673002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java
(right):

https://codereview.chromium.org/1731673002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java:311:
mCompositorView.setBackgroundColor(Color.WHITE);
do we always want white?  or should we use the background color of the page?

https://codereview.chromium.org/1731673002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
(right):

https://codereview.chromium.org/1731673002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:667:
// TODO(yusufo): The removal should happen as a part of the callback or as a
part of
file bugs for the todos and put them in here.

https://codereview.chromium.org/1731673002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:676:
finish();
we could do the tab model cleanup here, but I guess it doesn't need to be done
in this change.

https://codereview.chromium.org/1731673002/diff/200001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://codereview.chromium.org/1731673002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1367: public
void reparentToActivity(ChromeActivity activity, TabDelegateFactory
tabDelegateFactory) {
we might want this to be a two part call eventually.

one is disconnectFromActivity and connectToActivity where they would be
symmetric.

Follow up.

https://codereview.chromium.org/1731673002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/AsyncTabParamsManager.java
(right):

https://codereview.chromium.org/1731673002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/AsyncTabParamsManager.java:41:
return sNumOfParamsWithTabToReparent > 0;
if you think the number of pending tabs will be at most one.  I would just
iterate across here and return if getTabToReparent() != null.

No need to keep an extra param for now.

https://codereview.chromium.org/1731673002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/SingleTabModel.java
(right):

https://codereview.chromium.org/1731673002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/SingleTabModel.java:175:
public void removeTab(Tab tab) {
I would add an assert false to each of the empty ones you're adding since it is
not supported.

[commit-bot: I haz the power, 2016-02-26 19:18:43]

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

[commit-bot: I haz the power, 2016-02-26 19:18:44]

Dry run: This issue passed the CQ dry run.

[Yusuf, 2016-02-26 19:39:52]

https://codereview.chromium.org/1731673002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java
(right):

https://codereview.chromium.org/1731673002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java:311:
mCompositorView.setBackgroundColor(Color.WHITE);
On 2016/02/26 18:46:29, Ted C wrote:
> do we always want white?  or should we use the background color of the page?

initNative inside CompositorView (which does the same thing for initialization
only) uses WHITE. I think at this point, we may or may not have the background
color from the page yet. For this particular case, at this point, the activity
isn't even aware of the tab it will reparent yet, but knows only that it will
reparent one pretty soon.

https://codereview.chromium.org/1731673002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
(right):

https://codereview.chromium.org/1731673002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:667:
// TODO(yusufo): The removal should happen as a part of the callback or as a
part of
On 2016/02/26 18:46:29, Ted C wrote:
> file bugs for the todos and put them in here.

Done.

https://codereview.chromium.org/1731673002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:676:
finish();
On 2016/02/26 18:46:29, Ted C wrote:
> we could do the tab model cleanup here, but I guess it doesn't need to be done
> in this change.

Agreed. Added to the bug and TODO.

https://codereview.chromium.org/1731673002/diff/200001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://codereview.chromium.org/1731673002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1367: public
void reparentToActivity(ChromeActivity activity, TabDelegateFactory
tabDelegateFactory) {
On 2016/02/26 18:46:29, Ted C wrote:
> we might want this to be a two part call eventually.
> 
> one is disconnectFromActivity and connectToActivity where they would be
> symmetric.
> 
> Follow up.

Acknowledged.

https://codereview.chromium.org/1731673002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/AsyncTabParamsManager.java
(right):

https://codereview.chromium.org/1731673002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/AsyncTabParamsManager.java:41:
return sNumOfParamsWithTabToReparent > 0;
On 2016/02/26 18:46:29, Ted C wrote:
> if you think the number of pending tabs will be at most one.  I would just
> iterate across here and return if getTabToReparent() != null.
> 
> No need to keep an extra param for now.

Done.

https://codereview.chromium.org/1731673002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/SingleTabModel.java
(right):

https://codereview.chromium.org/1731673002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/SingleTabModel.java:175:
public void removeTab(Tab tab) {
On 2016/02/26 18:46:29, Ted C wrote:
> I would add an assert false to each of the empty ones you're adding since it
is
> not supported.

Done.

[Yusuf, 2016-02-26 20:41:22]

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

[commit-bot: I haz the power, 2016-02-26 20:41:46]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1731673002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1731673002/220001

[Ted C, 2016-02-26 20:49:36]

lgtm w/ the unnecessary test change removed per offline discussion

https://codereview.chromium.org/1731673002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java
(right):

https://codereview.chromium.org/1731673002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java:311:
mCompositorView.setBackgroundColor(Color.WHITE);
On 2016/02/26 19:39:52, Yusuf wrote:
> On 2016/02/26 18:46:29, Ted C wrote:
> > do we always want white?  or should we use the background color of the page?
> 
> initNative inside CompositorView (which does the same thing for initialization
> only) uses WHITE. I think at this point, we may or may not have the background
> color from the page yet. For this particular case, at this point, the activity
> isn't even aware of the tab it will reparent yet, but knows only that it will
> reparent one pretty soon.

Can you reparent with a tab w/ a different background color?  Does this cause
a flicker?

If so, we should figure out a way to get the background color.  This is called
from onStart, which would have the intent if cold start.  Not sure if
onNewIntent
happens before or after onStart.

But if we have the intent then we know the tab id to be reparented and could
get the async params and call tab.getBackgroundColor and pass it in here.

It definitely makes chromeactivity more involved, but we could do something
of the like.

Not needed for this patch though.

[Yusuf, 2016-02-26 20:50:15]

The CQ bit was checked by yusufo@chromium.org

[Yusuf, 2016-02-26 20:50:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
dfalcantara@chromium.org, sievers@chromium.org, tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/1731673002/#ps240001
(title: "remove test")

[commit-bot: I haz the power, 2016-02-26 20:51:40]

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

[commit-bot: I haz the power, 2016-02-26 23:55:23]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-02-26 23:56:25]

Description was changed from

==========
Add tab reparenting

- Adds updateWindowAndroid to ContentViewCore native and java
- Uses the above for calling onAttachedToWindow and onDetachedFromWindow
  for CVC observers
- Add updateDelegates to TabAndroid
- Uses all above for adding Tab#reparentToActivity detaching from old
  activity and related objects and attaching to new activity
- Add AsyncTabParamsManager call to check whether there is a saved tab.
- Uses all above to add reparenting to CustomTabActivity to other
  activities.

BUG=584712
==========

to

==========
Add tab reparenting

- Adds updateWindowAndroid to ContentViewCore native and java
- Uses the above for calling onAttachedToWindow and onDetachedFromWindow
  for CVC observers
- Add updateDelegates to TabAndroid
- Uses all above for adding Tab#reparentToActivity detaching from old
  activity and related objects and attaching to new activity
- Add AsyncTabParamsManager call to check whether there is a saved tab.
- Uses all above to add reparenting to CustomTabActivity to other
  activities.

BUG=584712

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

[commit-bot: I haz the power, 2016-02-26 23:56:26]

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