[Android] Support infobars on native page.

[Android] Support infobar on native page.

Make it possible to show infobars on native pages by repurposing
content's FrameLayout parent also for native pages that infobars
can be attached to.

BUG=546601, 508307
Committed: https://crrev.com/57255e034a04ecec605a89b32cf2703dc3636259
Cr-Commit-Position: refs/heads/master@{#362632}

[Kibeom Kim (inactive), 2015-11-12 22:34:03]

Description was changed from

==========
[Android] Support infobars on native page.

Make it possible to show infobars on native pages by introducing a
FrameLayout parent for native pages that infobars can be attached to..

BUG=546601, 508307
==========

to

==========
[Android] Support infobar on native page.

Make it possible to show infobars on native pages by introducing a
FrameLayout parent for native pages that infobars can be attached to.

BUG=546601, 508307
==========

[Kibeom Kim (inactive), 2015-11-12 22:34:49]

kkimlabs@chromium.org changed reviewers:
+ dfalcantara@chromium.org, newt@chromium.org

[Kibeom Kim (inactive), 2015-11-12 22:34:50]

note: split from https://codereview.chromium.org/1435263003/

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

https://codereview.chromium.org/1435393002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1256:
mInfoBarContainer.onParentViewChanged(getId(), mContentViewParent);
re: comment at
https://codereview.chromium.org/1435263003/diff/40001/chrome/android/java/src...

When a native page is loaded, I change the infobar container's parent to
mNativePageParent at #1222 so I need to change it back when user leaves a native
page.

This is called only when user leaves a native page, by #1252 early return, but I
can also double check at InfoBarContainer#onParentViewChanged.

[gone, 2015-11-12 22:56:11]

I admittedly don't know native page code well, so only cursory questions.

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

https://codereview.chromium.org/1435393002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1217:
mNativePageParent = new FrameLayout(mActivity);
mActivity -> mThemedApplicationContext

https://codereview.chromium.org/1435393002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1221:
LayoutParams.MATCH_PARENT, LayoutParams.MATCH_PARENT));
Why do you need another mNativePageParent?  Does this not work if you use the
FrameLayout that gets returned by mNativePage.getView()?

[Kibeom Kim (inactive), 2015-11-12 23:03:27]

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

https://codereview.chromium.org/1435393002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1217:
mNativePageParent = new FrameLayout(mActivity);
On 2015/11/12 22:56:11, dfalcantara wrote:
> mActivity -> mThemedApplicationContext

Done.

https://codereview.chromium.org/1435393002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1221:
LayoutParams.MATCH_PARENT, LayoutParams.MATCH_PARENT));
On 2015/11/12 22:56:11, dfalcantara wrote:
> Why do you need another mNativePageParent?  Does this not work if you use the
> FrameLayout that gets returned by mNativePage.getView()?

Yeah if mNativePage.getView returns FrameLayout then we can just use it, but
currently not all native page returns FrameLayout (e.g., Enhanced bookmark
native page on tablet)

So alternatively we could force to use FrameLayout for native pages. Actually I
was on the  fence, but when I took over newt@'s initial CL, I noticed "private
FrameLayout mNativePageParent;" at #190 so I just followed that path.

[gone, 2015-11-13 01:08:25]

If you navigate from a page that pops up an infobar to a native page (and vice
versa), do the InfoBars disappear gracefully?

l-g-t-m, but newt@ should probably be the one looking at this.

[Kibeom Kim (inactive), 2015-11-13 18:24:44]

On 2015/11/13 01:08:25, dfalcantara wrote:
> If you navigate from a page that pops up an infobar to a native page (and vice
> versa), do the InfoBars disappear gracefully?

InfoBar starts to slide down on navigation but can disappear suddenly depending
on how fast the next page is loaded.

[newt (away), 2015-11-17 23:09:00]

I think a better option is to combine mContentViewParent and mNativePageParent
into a single View (not sure what to call it. Maybe mTabView?).
mContentViewParent and mNativePageParent are both children of the
CompositorViewHolder; they're both FrameLayouts; they both hold the
InfoBarContainer and don't do much else. I don't see any reason that we need
separate views for these two purposes, and using separate views for these
purposes causes problems (like infobar animations being cut off during page
loads) and increases code complexity.

(Sorry for leading you astray a bit with my initial CL, which created a separate
mNativePageParent member variable. In retrospect, I think I was just trying to
prove to myself that infobars could be shown on native pages pretty easily. I
don't recommend that exact design in any case.)

[Kibeom Kim (inactive), 2015-11-25 02:09:01]

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

[Kibeom Kim (inactive), 2015-11-25 02:09:40]

Yeah I think that's better. mTabView Done.

[commit-bot: I haz the power, 2015-11-25 02:12:04]

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

[commit-bot: I haz the power, 2015-11-25 03:48:08]

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

[commit-bot: I haz the power, 2015-11-25 03:48:09]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[newt (away), 2015-11-25 16:41:43]

Looks better. One question though.

https://codereview.chromium.org/1435393002/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (left):

https://codereview.chromium.org/1435393002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:2179:
destroyNativePageInternal(previousNativePage);
why was this removed?

[Kibeom Kim (inactive), 2015-11-25 18:52:08]

https://codereview.chromium.org/1435393002/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (left):

https://codereview.chromium.org/1435393002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:2179:
destroyNativePageInternal(previousNativePage);
On 2015/11/25 16:41:43, newt wrote:
> why was this removed?

I noticed that setContentViewCore(...) at #2170 already does that exactly.

[Kibeom Kim (inactive), 2015-11-25 21:20:55]

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

[commit-bot: I haz the power, 2015-11-25 21:23:07]

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

[commit-bot: I haz the power, 2015-11-25 23:31:04]

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

[commit-bot: I haz the power, 2015-11-25 23:31:05]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[Kibeom Kim (inactive), 2015-11-29 20:38:26]

Description was changed from

==========
[Android] Support infobar on native page.

Make it possible to show infobars on native pages by introducing a
FrameLayout parent for native pages that infobars can be attached to.

BUG=546601, 508307
==========

to

==========
[Android] Support infobar on native page.

Make it possible to show infobars on native pages by repurposing
content's FrameLayout parent also for native pages that infobars
can be attached to.

BUG=546601, 508307
==========

[newt (away), 2015-11-30 18:02:04]

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

[newt (away), 2015-11-30 18:02:05]

dtrainor: Could you look at the comment/question about CompositorViewHolder?

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

https://codereview.chromium.org/1435393002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1222:
observer.onContentChanged(this);
CompositorViewHolder.onContentChanged() will now early exit when a native page
is shown or hidden since Tab.getView() will now always return mTabView, even
after the native page is shown. See "if (mView == newView)" in
CompositorViewHolder.setTab()
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...

Is this OK, or will this cause problems with CompositorViewHolder's state?

https://codereview.chromium.org/1435393002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1224:
destroyNativePageInternal(previousNativePage);
Could you rename destroyNativePageInternal() to destroyNativePage() and update
it to look like this:

  private void destroyNativePage() {
      mNativePage.destroy();
      mNativePage = null;
  }

This will simplify all the cases where we destroy native pages. Currently we do
this silly dance to destroy native pages:

    NativePage previousNativePage = mNativePage;
    mNativePage = null;
    for (TabObserver observer : mObservers) observer.onContentChanged(this);
    destroyNativePageInternal(previousNativePage);

But this isn't necessary anymore. We could simplify this to

    destroyNativePage();
    for (TabObserver observer : mObservers) observer.onContentChanged(this);

The reason we needed that silly dance in the past was that previously
CompositorViewHolder (which is a TabObserver) removed the native page's view
from the view hierarchy when onContentChanged() was called, and we didn't want
to destroy the native page until after its view was removed from the hierarchy.
Now, the Tab itself is responsible for managing the NativePage's view (which is
simpler/better IMO), so we can simplify this.

[newt (away), 2015-11-30 18:08:35]

This patch doesn't (yet) interact correctly with the NativePageAssassin.
Somehow, before the NativePageAssassin destroys the native page, we need to
remove the native page's view from mTabView. Also, we need to update the
"pageToFreeze.getView().getParent() != null" check in NativePageAssassin, since
this always return true now.

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

https://codereview.chromium.org/1435393002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1224:
destroyNativePageInternal(previousNativePage);
On 2015/11/30 18:02:04, newt wrote:
> Could you rename destroyNativePageInternal() to destroyNativePage() and update
> it to look like this:
> 
>   private void destroyNativePage() {
>       mNativePage.destroy();
>       mNativePage = null;
>   }
> 
> This will simplify all the cases where we destroy native pages. Currently we
do
> this silly dance to destroy native pages:
> 
>     NativePage previousNativePage = mNativePage;
>     mNativePage = null;
>     for (TabObserver observer : mObservers) observer.onContentChanged(this);
>     destroyNativePageInternal(previousNativePage);
> 
> But this isn't necessary anymore. We could simplify this to
> 
>     destroyNativePage();
>     for (TabObserver observer : mObservers) observer.onContentChanged(this);
> 
> The reason we needed that silly dance in the past was that previously
> CompositorViewHolder (which is a TabObserver) removed the native page's view
> from the view hierarchy when onContentChanged() was called, and we didn't want
> to destroy the native page until after its view was removed from the
hierarchy.
> Now, the Tab itself is responsible for managing the NativePage's view (which
is
> simpler/better IMO), so we can simplify this.

Also, destroyNativePage() should remove the native page's view.

   private void destroyNativePage() {
       mTabView.removeView(mNativePage.getView());
       mNativePage.destroy();
       mNativePage = null;
   }

[David Trainor- moved to gerrit, 2015-11-30 18:27:03]

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

https://codereview.chromium.org/1435393002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1222:
observer.onContentChanged(this);
On 2015/11/30 18:02:04, newt wrote:
> CompositorViewHolder.onContentChanged() will now early exit when a native page
> is shown or hidden since Tab.getView() will now always return mTabView, even
> after the native page is shown. See "if (mView == newView)" in
> CompositorViewHolder.setTab()
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...
> 
> Is this OK, or will this cause problems with CompositorViewHolder's state?

The only thing I'm not sure about is whether or not we'll properly set the
correct properties on the ContentViewCore, but it looks like we'll just set it
immediately when we load the first native page anyway.  So I think we'll be okay
here.

Have you noticed any issues with general navigations?

[Kibeom Kim (inactive), 2015-12-02 01:15:10]

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

[Kibeom Kim (inactive), 2015-12-02 01:15:16]

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

https://codereview.chromium.org/1435393002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1222:
observer.onContentChanged(this);
On 2015/11/30 18:27:03, David Trainor wrote:
> On 2015/11/30 18:02:04, newt wrote:
> > CompositorViewHolder.onContentChanged() will now early exit when a native
page
> > is shown or hidden since Tab.getView() will now always return mTabView, even
> > after the native page is shown. See "if (mView == newView)" in
> > CompositorViewHolder.setTab()
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...
> > 
> > Is this OK, or will this cause problems with CompositorViewHolder's state?
> 
> The only thing I'm not sure about is whether or not we'll properly set the
> correct properties on the ContentViewCore, but it looks like we'll just set it
> immediately when we load the first native page anyway.  So I think we'll be
okay
> here.
> 
> Have you noticed any issues with general navigations?

Hmmm. no I haven't noticed any...

https://codereview.chromium.org/1435393002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1224:
destroyNativePageInternal(previousNativePage);
On 2015/11/30 18:08:35, newt wrote:
> On 2015/11/30 18:02:04, newt wrote:
> > Could you rename destroyNativePageInternal() to destroyNativePage() and
update
> > it to look like this:
> > 
> >   private void destroyNativePage() {
> >       mNativePage.destroy();
> >       mNativePage = null;
> >   }
> > 
> > This will simplify all the cases where we destroy native pages. Currently we
> do
> > this silly dance to destroy native pages:
> > 
> >     NativePage previousNativePage = mNativePage;
> >     mNativePage = null;
> >     for (TabObserver observer : mObservers) observer.onContentChanged(this);
> >     destroyNativePageInternal(previousNativePage);
> > 
> > But this isn't necessary anymore. We could simplify this to
> > 
> >     destroyNativePage();
> >     for (TabObserver observer : mObservers) observer.onContentChanged(this);
> > 
> > The reason we needed that silly dance in the past was that previously
> > CompositorViewHolder (which is a TabObserver) removed the native page's view
> > from the view hierarchy when onContentChanged() was called, and we didn't
want
> > to destroy the native page until after its view was removed from the
> hierarchy.
> > Now, the Tab itself is responsible for managing the NativePage's view (which
> is
> > simpler/better IMO), so we can simplify this.
> 
> Also, destroyNativePage() should remove the native page's view.
> 
>    private void destroyNativePage() {
>        mTabView.removeView(mNativePage.getView());
>        mNativePage.destroy();
>        mNativePage = null;
>    }

Done.

[commit-bot: I haz the power, 2015-12-02 01:16:51]

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

[newt (away), 2015-12-02 01:40:37]

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

https://codereview.chromium.org/1435393002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1226: public
void freezeNativePage() {
This method needs to be updated

https://codereview.chromium.org/1435393002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1228: assert
mNativePage.getView().getParent() == null : "Cannot freeze visible native page";
this assertion no longer checks the right condition

https://codereview.chromium.org/1435393002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1229:
mNativePage = FrozenNativePage.freeze(mNativePage);
before replacing mNativePage with a FrozenNativePage, we need to remove
mNativePage.getView() from mTabView

[Kibeom Kim (inactive), 2015-12-02 02:06:04]

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

[Kibeom Kim (inactive), 2015-12-02 02:06:11]

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

https://codereview.chromium.org/1435393002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1226: public
void freezeNativePage() {
On 2015/12/02 01:40:37, newt wrote:
> This method needs to be updated

Done.

https://codereview.chromium.org/1435393002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1228: assert
mNativePage.getView().getParent() == null : "Cannot freeze visible native page";
On 2015/12/02 01:40:37, newt wrote:
> this assertion no longer checks the right condition

Done.

https://codereview.chromium.org/1435393002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1229:
mNativePage = FrozenNativePage.freeze(mNativePage);
On 2015/12/02 01:40:37, newt wrote:
> before replacing mNativePage with a FrozenNativePage, we need to remove
> mNativePage.getView() from mTabView

Done.

[commit-bot: I haz the power, 2015-12-02 02:10:59]

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

[newt (away), 2015-12-02 02:49:47]

lgtm

[commit-bot: I haz the power, 2015-12-02 03:06:29]

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

[commit-bot: I haz the power, 2015-12-02 03:06:30]

Dry run: This issue passed the CQ dry run.

[Kibeom Kim (inactive), 2015-12-02 04:31:33]

The CQ bit was checked by kkimlabs@chromium.org

[commit-bot: I haz the power, 2015-12-02 04:31:55]

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

[commit-bot: I haz the power, 2015-12-02 04:36:29]

Description was changed from

==========
[Android] Support infobar on native page.

Make it possible to show infobars on native pages by repurposing
content's FrameLayout parent also for native pages that infobars
can be attached to.

BUG=546601, 508307
==========

to

==========
[Android] Support infobar on native page.

Make it possible to show infobars on native pages by repurposing
content's FrameLayout parent also for native pages that infobars
can be attached to.

BUG=546601, 508307
==========

[commit-bot: I haz the power, 2015-12-02 04:36:29]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-12-02 04:37:22]

Description was changed from

==========
[Android] Support infobar on native page.

Make it possible to show infobars on native pages by repurposing
content's FrameLayout parent also for native pages that infobars
can be attached to.

BUG=546601, 508307
==========

to

==========
[Android] Support infobar on native page.

Make it possible to show infobars on native pages by repurposing
content's FrameLayout parent also for native pages that infobars
can be attached to.

BUG=546601, 508307

Committed: https://crrev.com/57255e034a04ecec605a89b32cf2703dc3636259
Cr-Commit-Position: refs/heads/master@{#362632}
==========

[commit-bot: I haz the power, 2015-12-02 04:37:23]

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

[Kibeom Kim (inactive), 2015-12-07 08:30:21]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/1502153002/ by kkimlabs@chromium.org.

The reason for reverting is: http://crbug.com/565094.