[Android] Stop hiding the RWHV layer subtree when hiding the widget

Reland "[Android] Preserve the front buffer when the activity is paused"

This change was reverted in r322170 due to WebView breakage. The
ApplicationStatus dependency has been made optional, allowing
WebView to opt-out of its use. It was speculatively reverted
again in r327092, but that turned out to be a false alarm.

Original description: ----------------------------

Currently, when an activity is stopped, we explicitly hide the
foreground Tab. This is problematic, as current hiding semantics
might clear the visual front buffer before the window is hidden.
This in turn causes an unpleasant flickering during activity
transitions, e.g., when backgrounding Chrome or locking the screen.

Wire Activity onPause/onResume notifications to WindowAndroidObservers,
allowing the foreground tab to preserve its front buffer while hiding
its web content. If the tab is explicitly hidden, or the root window
is lost, the front buffer will be cleared as usual.

BUG=481450, 434401
Committed: https://crrev.com/61c7526b6acb7b29d27da7156bea7ddea71477d7
Cr-Commit-Position: refs/heads/master@{#329283}

[jdduke (slow), 2015-03-11 18:09:29]

jdduke@chromium.org changed reviewers:
+ dtrainor@chromium.org, sievers@chromium.org

[jdduke (slow), 2015-03-11 18:09:30]

This was the simplest solution I could see that avoids any new ambiguous APIs
and complex tracking strategies.

As far as I can tell, this doesn't introduce any visual artifacts when
navigating cross-domain, and our layer tree attachment policies already do the
right thing here when a tab is hidden. This should also be a simply cherry-pick
(assuming it doesn't regress anything, which is why I'd like to land it
soonish).

[no sievers, 2015-03-11 19:42:01]

Ok, but that changes the API so that the embedder now *has* to remove a layer
when it hides a tab.
Otherwise it will draw (and read back) hidden tabs during compositing. We'd have
to revisit the code (tab switching but also readback-before-hide) to make sure
there are no races.
AFAIR we explicitly wanted to make it simpler for the app by making hide
actually hide the window, which is I think also what the method traditionally
implies (hide the HWND) and what it implies on AURA today.
Basically you are changing RWHV::WasHidden() to mean just page-/renderer-side
visibility instead.

At the same time, wouldn't the frame evictor still kick in after hiding the tab
to free the layer, and you'd still potentially get a glitch?

[jdduke (slow), 2015-03-11 19:52:20]

On 2015/03/11 19:42:01, sievers wrote:
> Ok, but that changes the API so that the embedder now *has* to remove a layer
> when it hides a tab.

Doesn't this already have to happen? That is, the ContentViewCore already has an
opaque layer, the visibility of which needs to be managed externally? Isn't that
what is causing the "flicker" here? We're hiding the layer when it's parent is
still visible?

> Otherwise it will draw (and read back) hidden tabs during compositing. We'd
have
> to revisit the code (tab switching but also readback-before-hide) to make sure
> there are no races.
> AFAIR we explicitly wanted to make it simpler for the app by making hide
> actually hide the window, which is I think also what the method traditionally
> implies (hide the HWND) and what it implies on AURA today.
> Basically you are changing RWHV::WasHidden() to mean just page-/renderer-side
> visibility instead.
> 
> At the same time, wouldn't the frame evictor still kick in after hiding the
tab
> to free the layer, and you'd still potentially get a glitch?

I'm not familiar with the frame evictor.

[no sievers, 2015-03-11 20:11:12]

On 2015/03/11 19:52:20, jdduke wrote:
> On 2015/03/11 19:42:01, sievers wrote:
> > Ok, but that changes the API so that the embedder now *has* to remove a
layer
> > when it hides a tab.
> 
> Doesn't this already have to happen? That is, the ContentViewCore already has
an
> opaque layer, the visibility of which needs to be managed externally? Isn't
that
> what is causing the "flicker" here? We're hiding the layer when it's parent is
> still visible?
> 

Good point, I think that's a bug too. It might happen to be less noticeable
because that layer set itself to not drawable as long as the RWHV's layer is
attached.

> > Otherwise it will draw (and read back) hidden tabs during compositing. We'd
> have
> > to revisit the code (tab switching but also readback-before-hide) to make
sure
> > there are no races.
> > AFAIR we explicitly wanted to make it simpler for the app by making hide
> > actually hide the window, which is I think also what the method
traditionally
> > implies (hide the HWND) and what it implies on AURA today.
> > Basically you are changing RWHV::WasHidden() to mean just
page-/renderer-side
> > visibility instead.
> > 
> > At the same time, wouldn't the frame evictor still kick in after hiding the
> tab
> > to free the layer, and you'd still potentially get a glitch?
> 
> I'm not familiar with the frame evictor.

[jdduke (slow), 2015-03-11 20:56:39]

I guess I'm struggling to determine responsibilities, as there are 50 ways I
could wire this up, and to be honest none of them seem cleaner than the other.

I could add activity stop + surface lost notifications for
WindowAndroidObservers. That would allow RWHVAndroid to lock/unlock its surface
as appropriate. Then we can keep the existing hide() mechanism as it is now?

[jdduke (slow), 2015-03-11 20:59:01]

On 2015/03/11 20:56:39, jdduke wrote:
> I guess I'm struggling to determine responsibilities, as there are 50 ways I
> could wire this up, and to be honest none of them seem cleaner than the other.
> 
> I could add activity stop + surface lost notifications for
> WindowAndroidObservers. That would allow RWHVAndroid to lock/unlock its
surface
> as appropriate. Then we can keep the existing hide() mechanism as it is now?

FWIW, I wired a version through TabContentManager but it was unclear to me that
that was any clearer/simpler than the other approaches I had in the previous
patch =/. I could certainly make the locking/unlocking code go through
TabContentManager if that's the class we want in charge, but again, given that
classes other responsibilities it didn't seem immediately appropriate.

[no sievers, 2015-03-11 21:24:40]

On 2015/03/11 20:56:39, jdduke wrote:
> I guess I'm struggling to determine responsibilities, as there are 50 ways I
> could wire this up, and to be honest none of them seem cleaner than the other.
> 

Yes, it's a bit unfortunate. I think the goals should be:
- make it simple and robust for the embedder, in other words, hard to get wrong
or cause corner case issues
- that somewhat implicitly prefers a solution that does not require for example
ContentShell, ChromeShell and Chrome all separately having to implement subtle
calls to various pieces in the code form various other places in an unclear
contract
- otherwise keep the behavior close to desktop (at least as far as the shared
code is concerned, like code in content/)

> I could add activity stop + surface lost notifications for
> WindowAndroidObservers. That would allow RWHVAndroid to lock/unlock its
surface
> as appropriate. Then we can keep the existing hide() mechanism as it is now?

I like that idea. It seems like it mostly satisfies the above.
It can handle the surface going away internally without the app needing to worry
about it.
The stop/resume method could even be static (and on the native side iterate
through views if necessary) - and maybe WindowAndroid is also a good place to
route it through.

Knowing about app visibility would free the application from having to worry
about the implications of this and decouple it from visibility of a view in
terms of whether the app itself is displaying the tab (i.e. whether it's live/in
the foreground) . (Rather than the current mechanism where for example Chrome
deals with tracking the one-and-only 'active' tab to then hide it when the app
becomes stopped or hidden).

[David Trainor- moved to gerrit, 2015-03-11 21:40:25]

On 2015/03/11 21:24:40, sievers wrote:
> On 2015/03/11 20:56:39, jdduke wrote:
> > I guess I'm struggling to determine responsibilities, as there are 50 ways I
> > could wire this up, and to be honest none of them seem cleaner than the
other.
> > 
> 
> Yes, it's a bit unfortunate. I think the goals should be:
> - make it simple and robust for the embedder, in other words, hard to get
wrong
> or cause corner case issues
> - that somewhat implicitly prefers a solution that does not require for
example
> ContentShell, ChromeShell and Chrome all separately having to implement subtle
> calls to various pieces in the code form various other places in an unclear
> contract
> - otherwise keep the behavior close to desktop (at least as far as the shared
> code is concerned, like code in content/)
> 
> > I could add activity stop + surface lost notifications for
> > WindowAndroidObservers. That would allow RWHVAndroid to lock/unlock its
> surface
> > as appropriate. Then we can keep the existing hide() mechanism as it is now?
> 
> I like that idea. It seems like it mostly satisfies the above.
> It can handle the surface going away internally without the app needing to
worry
> about it.
> The stop/resume method could even be static (and on the native side iterate
> through views if necessary) - and maybe WindowAndroid is also a good place to
> route it through.
> 
> Knowing about app visibility would free the application from having to worry
> about the implications of this and decouple it from visibility of a view in
> terms of whether the app itself is displaying the tab (i.e. whether it's
live/in
> the foreground) . (Rather than the current mechanism where for example Chrome
> deals with tracking the one-and-only 'active' tab to then hide it when the app
> becomes stopped or hidden).

I agree I like the idea of notifying of activity stop/surface lost.  I agree
that we should find a way to do this such that each app doesn't have to make
specific calls.  The less the better :).

[jdduke (slow), 2015-03-11 23:36:16]

On 2015/03/11 21:40:25, David Trainor wrote:
> On 2015/03/11 21:24:40, sievers wrote:
> > On 2015/03/11 20:56:39, jdduke wrote:
> > > I guess I'm struggling to determine responsibilities, as there are 50 ways
I
> > > could wire this up, and to be honest none of them seem cleaner than the
> other.
> > > 
> > 
> > Yes, it's a bit unfortunate. I think the goals should be:
> > - make it simple and robust for the embedder, in other words, hard to get
> wrong
> > or cause corner case issues
> > - that somewhat implicitly prefers a solution that does not require for
> example
> > ContentShell, ChromeShell and Chrome all separately having to implement
subtle
> > calls to various pieces in the code form various other places in an unclear
> > contract
> > - otherwise keep the behavior close to desktop (at least as far as the
shared
> > code is concerned, like code in content/)
> > 
> > > I could add activity stop + surface lost notifications for
> > > WindowAndroidObservers. That would allow RWHVAndroid to lock/unlock its
> > surface
> > > as appropriate. Then we can keep the existing hide() mechanism as it is
now?
> > 
> > I like that idea. It seems like it mostly satisfies the above.
> > It can handle the surface going away internally without the app needing to
> worry
> > about it.
> > The stop/resume method could even be static (and on the native side iterate
> > through views if necessary) - and maybe WindowAndroid is also a good place
to
> > route it through.
> > 
> > Knowing about app visibility would free the application from having to worry
> > about the implications of this and decouple it from visibility of a view in
> > terms of whether the app itself is displaying the tab (i.e. whether it's
> live/in
> > the foreground) . (Rather than the current mechanism where for example
Chrome
> > deals with tracking the one-and-only 'active' tab to then hide it when the
app
> > becomes stopped or hidden).
> 
> I agree I like the idea of notifying of activity stop/surface lost.  I agree
> that we should find a way to do this such that each app doesn't have to make
> specific calls.  The less the better :).

OK, sorry for the noise here, I'll post another update when it's ready for
review :)

[jdduke (slow), 2015-03-16 16:54:12]

I'd like some feedback on the latest patchset. I now route
pause/resume/visibility updates from the WindowAndroid to its observers.

However, when we hide the RWHVAndroid, we stop observing the root window, which
makes it somewhat difficult to predict what the observed callback sequence from
WindowAndroid will be.

Some choices here include:

Continue observing the window when we're hidden, simply ignoring (most) of the
callbacks.

Figure out an additional way to notify RWHVAndroid that resources have been
lost.

Thoughts?

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

https://codereview.chromium.org/1001573003/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1887: if
(visible)
We never see this callback, as we stop observing the window when we're hidden.

[no sievers, 2015-03-17 21:37:47]

Would something like this work?

RWHVA::OnWindowVisibilityChanged(bool visible) {
  DCHECK(is_showing_); // or we wouldn't be observing which implies being
hidden, so nothing to do since everything is freed

  if (visible) {
    ShowInternal();
  } else {
    HideInternal(true, false);
  }
}

RWHVA::Show() {
  if (is_showing_)
    return;
  is_showing_ = true;
  ShowInternal();
}

RWHVA::Hide() {
  if (!is_showing_)
    return;
  is_showing_ = false;
  HideInternal(true, true);
}

RWHVA::ShowInternal() {
  DCHECK(is_showing_);

  basically what Show() currently does, but:

  bool was_showing_frontbuffer = layer_.get() &&
!layer_->hide_layer_and_subtree();
  if (!was_showing_frontbuffer)
    frame_evictor_->SetVisible(true);

  (just because that class doesn't look robust to redundant calls)
}

RWHVA::HideInternal(bool hide_frontbuffer, bool stop_observing_root_window) {
 if (hide_frontbuffer) {
   if (layer_.get() && locks_on_frame_count_ == 0)
     layer_->SetHideLayerAndSubtree(true);

   frame_evictor_->SetVisible(false);
 }

 AbortPendingReadbackRequests();
 RunAckCallbacks(cc::SurfaceDrawStatus::DRAW_SKIPPED);

 if (overscroll_controller_)
    overscroll_controller_->Disable();

 host_->WasHidden();
 
 if (stop_observing_root_window)
   StopObservingRootWindow();
}

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

https://codereview.chromium.org/1001573003/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1887: if
(visible)
On 2015/03/16 16:54:12, jdduke wrote:
> We never see this callback, as we stop observing the window when we're hidden.

Maybe that's ok. If somebody called Hide() we already dropped everything and
shouldn't be visible anymore.

https://codereview.chromium.org/1001573003/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1890: if
(should_unlock_surface_on_visibility_lost_or_activity_resumed_) {
nit: maybe |surface_locked_for_paused_activity_| or so?

https://codereview.chromium.org/1001573003/diff/20001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java
(right):

https://codereview.chromium.org/1001573003/diff/20001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java:24:
implements ApplicationStatus.ActivityStateListener {
There is also a native interface in base/android/application_status_listener.cc.
So maybe we don't have to plumb it all the way through java, but could just
register the callback in window_android.cc.

[no sievers, 2015-03-17 21:39:52]

meh, that dropped stuff:

...

RWHVA::OnActivityPaused() {
  DCHECK(is_showing_); // or we wouldn't be observing which implies being
hidden, so nothing to do since everything is freed
  
  HideInternal(false, false);
}

RWHVA::OnActivityResumed() {
  DCHECK(is_showing_); // or we wouldn't be observing which implies being
hidden, so nothing to do since everything is freed

  ShowInternal();
}

[no sievers, 2015-03-17 21:41:30]

and we might be able to put something into content_unittests (there are a bunch
of RWHV tests) to test all the sequences,
incl. unexpected ones (like Show() being called while the activity is paused
etc.).

[jdduke (slow), 2015-03-17 22:11:53]

On 2015/03/17 21:41:30, sievers wrote:
> and we might be able to put something into content_unittests (there are a
bunch
> of RWHV tests) to test all the sequences,
> incl. unexpected ones (like Show() being called while the activity is paused
> etc.).

Right, this design is about like we discussed yesterday. I suspect we'll still
want to hide Tab/ContentViewCore-widgets when we're explicitly backgrounded,
which we can achieve if we move the foreground Tab hiding logic to the
onWindowVisibilityChanged call as changwan@ had done.

[jdduke (slow), 2015-03-17 22:13:09]

On 2015/03/17 22:11:53, jdduke wrote:
> On 2015/03/17 21:41:30, sievers wrote:
> > and we might be able to put something into content_unittests (there are a
> bunch
> > of RWHV tests) to test all the sequences,
> > incl. unexpected ones (like Show() being called while the activity is paused
> > etc.).
> 
> Right, this design is about like we discussed yesterday. I suspect we'll still
> want to hide Tab/ContentViewCore-widgets when we're explicitly backgrounded,
> which we can achieve if we move the foreground Tab hiding logic to the
> onWindowVisibilityChanged call as changwan@ had done.

I'll upload a new patchset later today or early tomorrow with the changes we
discussed.

[jdduke (slow), 2015-03-17 22:22:18]

https://codereview.chromium.org/1001573003/diff/20001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java
(right):

https://codereview.chromium.org/1001573003/diff/20001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java:24:
implements ApplicationStatus.ActivityStateListener {
On 2015/03/17 21:37:47, sievers wrote:
> There is also a native interface in
base/android/application_status_listener.cc.
> So maybe we don't have to plumb it all the way through java, but could just
> register the callback in window_android.cc.

Hmm, I didn't see that, though it looks like that doesn't support per-activity
state listening, e.g., for Hera I don't think it gives us enough fine-grained
listening behavior, as you can't listen to state changes for a single activity.

I could always add that ability, though that might require the native
WindowAndroid knowing about the activity.

[no sievers, 2015-03-17 23:54:59]

On 2015/03/17 22:22:18, jdduke wrote:
>
https://codereview.chromium.org/1001573003/diff/20001/ui/android/java/src/org...
> File ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java
> (right):
> 
>
https://codereview.chromium.org/1001573003/diff/20001/ui/android/java/src/org...
> ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java:24:
> implements ApplicationStatus.ActivityStateListener {
> On 2015/03/17 21:37:47, sievers wrote:
> > There is also a native interface in
> base/android/application_status_listener.cc.
> > So maybe we don't have to plumb it all the way through java, but could just
> > register the callback in window_android.cc.
> 
> Hmm, I didn't see that, though it looks like that doesn't support per-activity
> state listening, e.g., for Hera I don't think it gives us enough fine-grained
> listening behavior, as you can't listen to state changes for a single
activity.
> 

Aaah ok, makes sense. Not a big deal, WindowAndroid seems fine.
As long as there is one per tab in Hera.

[David Trainor- moved to gerrit, 2015-03-18 18:07:51]

On 2015/03/17 23:54:59, sievers wrote:
> On 2015/03/17 22:22:18, jdduke wrote:
> >
>
https://codereview.chromium.org/1001573003/diff/20001/ui/android/java/src/org...
> > File ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/1001573003/diff/20001/ui/android/java/src/org...
> > ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java:24:
> > implements ApplicationStatus.ActivityStateListener {
> > On 2015/03/17 21:37:47, sievers wrote:
> > > There is also a native interface in
> > base/android/application_status_listener.cc.
> > > So maybe we don't have to plumb it all the way through java, but could
just
> > > register the callback in window_android.cc.
> > 
> > Hmm, I didn't see that, though it looks like that doesn't support
per-activity
> > state listening, e.g., for Hera I don't think it gives us enough
fine-grained
> > listening behavior, as you can't listen to state changes for a single
> activity.
> > 
> 
> Aaah ok, makes sense. Not a big deal, WindowAndroid seems fine.
> As long as there is one per tab in Hera.

This change looks fine to me.  I'll let sievers@ handle the final +2 though.

[jdduke (slow), 2015-03-18 18:18:04]

Patchset #3 (id:40001) has been deleted

[jdduke (slow), 2015-03-18 18:18:10]

Patchset #3 (id:60001) has been deleted

[jdduke (slow), 2015-03-18 19:51:42]

OK, what about the latest approach? This aligns with your suggestions sievers@,
with a few tweaks to accommodate some corner cases. I played with hiding the tab
while different widgets are visible, and it seemed to play nicely. I'll confirm
with Ted to see if there are any cases that should concern us.

[jdduke (slow), 2015-03-18 20:16:51]

https://codereview.chromium.org/1001573003/diff/100001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_android.h (right):

https://codereview.chromium.org/1001573003/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_android.h:196: void
OnVisibilityChanged(bool visible) override;
OnVisibilityChanged is a little generic for me... what about
|OnSurfaceVisilbityChanged| or |OnWindowVisibilityChanged|?

https://codereview.chromium.org/1001573003/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_android.h:296: void
ShowInternal();
I'll definitely add some comments here should we decide this is the approach we
want.

[no sievers, 2015-03-18 21:04:48]

https://codereview.chromium.org/1001573003/diff/100001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/1001573003/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_android.cc:1546: if
(!host_ || host_->is_hidden())
see question below

https://codereview.chromium.org/1001573003/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_android.cc:1928: if
(!host_ || host_->is_hidden())
Doesn't the renderer turn beginFrame off when it gets hidden?  And why do we
need this check now and didn't before, is it because we now might keep observing
the root window while the window is not visible (and Hide() was not called)
while previously we relied on being completely hidden and cut off from
notifications?

Maybe it's good paranoia, but it does make it a bit more confusing on what the
vsync and beginFrame contract really is. We could also consider not ticking in
WindowAndroid if it's hidden.

https://codereview.chromium.org/1001573003/diff/100001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_android.h (right):

https://codereview.chromium.org/1001573003/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_android.h:196: void
OnVisibilityChanged(bool visible) override;
On 2015/03/18 20:16:50, jdduke wrote:
> OnVisibilityChanged is a little generic for me... what about
> |OnSurfaceVisilbityChanged| or |OnWindowVisibilityChanged|?

Maybe not 'Surface' since it's a bit overloaded already (see
'CompositingSurface' in RWH/RWHV, which means the delegated layer contents, or
'use surfaces' in here which is something different again).

OnWindowVisibilityChanged sounds good, or even OnRootWindowVisibilityChanged.

[jdduke (slow), 2015-03-18 23:29:14]

jdduke@chromium.org changed reviewers:
+ tedchoc@chromium.org

[jdduke (slow), 2015-03-18 23:29:14]

Ted: Do you anticipate any issues if we stop explicitly calling Tab.hide() on
the foreground tab when the activity is stopped or the screen is locked? I've
confirmed that all of the widgets in ContentViewCore seem to play nicely when we
are backgrounded without an explicit hide() (i.e., they restore nicely), but
there may be some lingering side-effects from Tab.hide that I'm not factoring in
here.

There don't appear to be any immediate consumers of the TabObserver.onHidden
callback, and the downstream hideInternal implementation appears to be a no-op.
Also, for now we'll continue to call Tab.onActivityStart, which should
(hopefully) assure the right fullscreen behavior.

[Ted C, 2015-03-18 23:46:32]

On 2015/03/18 23:29:14, jdduke wrote:
> Ted: Do you anticipate any issues if we stop explicitly calling Tab.hide() on
> the foreground tab when the activity is stopped or the screen is locked? I've
> confirmed that all of the widgets in ContentViewCore seem to play nicely when
we
> are backgrounded without an explicit hide() (i.e., they restore nicely), but
> there may be some lingering side-effects from Tab.hide that I'm not factoring
in
> here.
> 
> There don't appear to be any immediate consumers of the TabObserver.onHidden
> callback, and the downstream hideInternal implementation appears to be a
no-op.
> Also, for now we'll continue to call Tab.onActivityStart, which should
> (hopefully) assure the right fullscreen behavior.

The primary reason hide() was called (in my recollection) was to stop
rendering/updates.  If there is some other way this is being handled, then I
don't know of any major issues of not calling hide explicitly on the tab.

As long as hide is called on the tab if you launch a new one, then I think we're
ok here.

[no sievers, 2015-03-19 20:21:52]

https://codereview.chromium.org/1001573003/diff/100001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/1001573003/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_android.cc:1472: return;
Can we move this early-out to the top?

We should never drop the frontbuffer, while not telling the renderer to hide.
Then it should also avoid redundant frame_evictor_->SetVisible() calls, and
don't need the |was_showing_frontbuffer| check.

https://codereview.chromium.org/1001573003/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_android.cc:1504: if
(!host_ || host_->is_hidden())
Same here.

[jdduke (slow), 2015-03-19 21:32:42]

Patchset #5 (id:120001) has been deleted

[jdduke (slow), 2015-03-20 14:58:44]

Patchset #5 (id:140001) has been deleted

[jdduke (slow), 2015-03-20 14:59:29]

https://codereview.chromium.org/1001573003/diff/100001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/1001573003/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_android.cc:1472: return;
On 2015/03/19 20:21:52, sievers wrote:
> Can we move this early-out to the top?
> 
> We should never drop the frontbuffer, while not telling the renderer to hide.
> Then it should also avoid redundant frame_evictor_->SetVisible() calls, and
> don't need the |was_showing_frontbuffer| check.

We can probably move this call up, but I'm not sure about the hide call.

https://codereview.chromium.org/1001573003/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_android.cc:1504: if
(!host_ || host_->is_hidden())
On 2015/03/19 20:21:52, sievers wrote:
> Same here.

We can't move this call up because the the host may be hidden but the
frontbuffer is still showing.

https://codereview.chromium.org/1001573003/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_android.cc:1928: if
(!host_ || host_->is_hidden())
On 2015/03/18 21:04:48, sievers wrote:
> Doesn't the renderer turn beginFrame off when it gets hidden?  And why do we
> need this check now and didn't before, is it because we now might keep
observing
> the root window while the window is not visible (and Hide() was not called)
> while previously we relied on being completely hidden and cut off from
> notifications?

Right, this preserves the original behavior which is having a hard cutoff of
BeginFrame dispatch. The compositor *should* unsubscribe from BeginFrame after
RenderWidget receives the hidden notification, but I kept the hidden check as a
precauation. I'd be OK with removing it, but I'm not sure there's a great
non-racy way to verify that we get the unsubscribe call.


> 
> Maybe it's good paranoia, but it does make it a bit more confusing on what the
> vsync and beginFrame contract really is. We could also consider not ticking in
> WindowAndroid if it's hidden.

Yeah, I'm fine restoring the original code, as our begin frame subscription
logic should in theory be enough to guard here. Either way.

[no sievers, 2015-03-20 18:41:51]

sievers@chromium.org changed reviewers:
+ piman@chromium.org

[no sievers, 2015-03-20 18:45:55]

+piman for the DelegatedFrameEvictor change.

Does that look ok, and if so should we remove the |visible| argument from
SwappedFrame() also and use the internal state?

We are adding a new visibility state to RWHVAndroid, where we keep the current
frame but tell the renderer to hide for when the activity is paused.
Since it's possible that we become fully hidden either before or after we
already told the renderer to hide (i.e. HideInternal() might be called more than
once without a Show), we have to add another state bit. And seems nicest to just
make DFH::SetVisible() robust to redundant calls.

[piman, 2015-03-20 20:06:25]

LGTM for DelegatedFrameEvictor.

[no sievers, 2015-03-20 21:48:25]

lgtm w/2nits

https://codereview.chromium.org/1001573003/diff/160001/content/browser/render...
File content/browser/renderer_host/delegated_frame_evictor.cc (right):

https://codereview.chromium.org/1001573003/diff/160001/content/browser/render...
content/browser/renderer_host/delegated_frame_evictor.cc:18: has_frame_ = true;
Should this DCHECK_EQ(visible, visible_)?

https://codereview.chromium.org/1001573003/diff/160001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/1001573003/diff/160001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_android.cc:1500: if
(!host_ || host_->is_hidden())
Can you move this up to line 1487 though?

[jdduke (slow), 2015-03-20 22:48:18]

Thanks for review (and for being patient :).

https://codereview.chromium.org/1001573003/diff/160001/content/browser/render...
File content/browser/renderer_host/delegated_frame_evictor.cc (right):

https://codereview.chromium.org/1001573003/diff/160001/content/browser/render...
content/browser/renderer_host/delegated_frame_evictor.cc:18: has_frame_ = true;
On 2015/03/20 21:48:24, sievers wrote:
> Should this DCHECK_EQ(visible, visible_)?

Good catch, I think so, yes. Done.

https://codereview.chromium.org/1001573003/diff/160001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/1001573003/diff/160001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_android.cc:1500: if
(!host_ || host_->is_hidden())
On 2015/03/20 21:48:25, sievers wrote:
> Can you move this up to line 1487 though?

I'll move it up to 1491. I can't move it to 1487, as
|stop_observing_root_window| for our case will be true *after* the host has
already been hidden by a previous HideInternal call.

[Ted C, 2015-03-20 23:26:03]

lgtm

https://codereview.chromium.org/1001573003/diff/180001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java
(right):

https://codereview.chromium.org/1001573003/diff/180001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java:109: if
(newState == ActivityState.PAUSED)
all on a single line or braces always required in java land

[jdduke (slow), 2015-03-23 16:16:57]

https://codereview.chromium.org/1001573003/diff/180001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java
(right):

https://codereview.chromium.org/1001573003/diff/180001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java:109: if
(newState == ActivityState.PAUSED)
On 2015/03/20 23:26:03, Ted C wrote:
> all on a single line or braces always required in java land

Done.

[jdduke (slow), 2015-03-23 16:17:11]

The CQ bit was checked by jdduke@chromium.org

[jdduke (slow), 2015-03-23 16:17:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
sievers@chromium.org, piman@chromium.org, tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/1001573003/#ps190001
(title: "Java fix")

[commit-bot: I haz the power, 2015-03-23 16:17:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1001573003/190001

[jdduke (slow), 2015-03-23 18:35:28]

The CQ bit was unchecked by jdduke@chromium.org

[jdduke (slow), 2015-03-23 18:36:32]

On 2015/03/23 16:17:48, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1001573003/190001

Hmm, I'm seeing a good number of these failures in the tests:

FATAL:delegated_frame_evictor.cc(18)] Check failed: visible == visible_ (1 vs. 0

[jdduke (slow), 2015-03-23 20:14:32]

Patchset #8 (id:210001) has been deleted

[jdduke (slow), 2015-03-23 20:15:02]

On 2015/03/23 18:36:32, jdduke wrote:
> On 2015/03/23 16:17:48, I haz the power (commit-bot) wrote:
> > CQ is trying da patch. Follow status at
> >  https://chromium-cq-status.appspot.com/patch-status/1001573003/190001
> 
> Hmm, I'm seeing a good number of these failures in the tests:
> 
> FATAL:delegated_frame_evictor.cc(18)] Check failed: visible == visible_ (1 vs.
0

Looks like we just need to make sure the frame evictor is initialized with the
right visibility setting. sievers@ can you validate the changes in patchset #8?

[no sievers, 2015-03-23 20:19:52]

lgtm

[jdduke (slow), 2015-03-23 20:49:56]

The CQ bit was checked by jdduke@chromium.org

[jdduke (slow), 2015-03-23 20:49:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, piman@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1001573003/#ps250001
(title: "Rebase")

[commit-bot: I haz the power, 2015-03-23 20:51:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1001573003/250001

[no sievers, 2015-03-23 22:03:40]

sievers@chromium.org changed reviewers:
+ boliu@chromium.org

[no sievers, 2015-03-23 22:04:03]

+Bo to double-check that this doesn't do something unexpected for WebView.

[jdduke (slow), 2015-03-23 22:12:38]

The CQ bit was unchecked by jdduke@chromium.org

[jdduke (slow), 2015-03-23 22:14:42]

https://codereview.chromium.org/1001573003/diff/250001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right):

https://codereview.chromium.org/1001573003/diff/250001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:242: protected void
onActivityPaused() {
I believe WebView hides/shows the ContentViewCore on pause/resume. I suppose we
could change these methods to Stop/Start to prevent any ordering issues,
although it shouldn't be strictly necessary.

[no sievers, 2015-03-23 22:45:19]

On 2015/03/23 22:14:42, jdduke wrote:
>
https://codereview.chromium.org/1001573003/diff/250001/ui/android/java/src/or...
> File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right):
> 
>
https://codereview.chromium.org/1001573003/diff/250001/ui/android/java/src/or...
> ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:242: protected
void
> onActivityPaused() {
> I believe WebView hides/shows the ContentViewCore on pause/resume. I suppose
we
> could change these methods to Stop/Start to prevent any ordering issues,
> although it shouldn't be strictly necessary.

Originally I was not expecting WebView to use ActivityWindowAndroid.java (it
actually didn't use to have a WindowAndroid at all, I think). But Bo just said
it might be instantiating that class.

So maybe if WebView doesn't want the behavior you are implementing here, it
should implement WindowAndroid differently.

[boliu, 2015-03-23 23:14:15]

https://codereview.chromium.org/1001573003/diff/250001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right):

https://codereview.chromium.org/1001573003/diff/250001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:242: protected void
onActivityPaused() {
On 2015/03/23 22:14:42, jdduke wrote:
> I believe WebView hides/shows the ContentViewCore on pause/resume. I suppose
we
> could change these methods to Stop/Start to prevent any ordering issues,
> although it shouldn't be strictly necessary.

From reading code, looks like webview doesn't call ApplicationStatus.initialize,
so these activity callbacks will never happen.

I'll check if this is actually true.

[boliu, 2015-03-23 23:18:54]

https://codereview.chromium.org/1001573003/diff/250001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right):

https://codereview.chromium.org/1001573003/diff/250001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:242: protected void
onActivityPaused() {
On 2015/03/23 23:14:15, boliu wrote:
> On 2015/03/23 22:14:42, jdduke wrote:
> > I believe WebView hides/shows the ContentViewCore on pause/resume. I suppose
> we
> > could change these methods to Stop/Start to prevent any ordering issues,
> > although it shouldn't be strictly necessary.
> 
> From reading code, looks like webview doesn't call
ApplicationStatus.initialize,
> so these activity callbacks will never happen.
> 
> I'll check if this is actually true.

Yep, in the real system webview, ApplicationStatus is never initialized.

It is initialized in the aw test shell, but I don't think any of this matters
too much there.

[jdduke (slow), 2015-03-24 18:59:36]

The CQ bit was checked by jdduke@chromium.org

[jdduke (slow), 2015-03-24 18:59:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, piman@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1001573003/#ps270001
(title: "Rebase   trace pause/resume")

[commit-bot: I haz the power, 2015-03-24 19:00:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1001573003/270001

[commit-bot: I haz the power, 2015-03-24 19:54:25]

Committed patchset #10 (id:270001)

[commit-bot: I haz the power, 2015-03-24 19:55:31]

Patchset 10 (id:??) landed as
https://crrev.com/764f31d287a4995f0c89915745f5749e1c7063af
Cr-Commit-Position: refs/heads/master@{#322052}

[Torne, 2015-03-25 12:56:52]

torne@chromium.org changed reviewers:
+ torne@chromium.org

[Torne, 2015-03-25 12:56:53]

This broke WebView on ToT (which doesn't have a current activity, and probably
doesn't set up ApplicationStatus at all? not sure):

E/AndroidRuntime(13823): Caused by: java.lang.NullPointerException: Attempt to
invoke virtual method 'org.chromium.base.ObserverList
org.chromium.base.ApplicationStatus$ActivityInfo.getListeners()' on a null
object reference
E/AndroidRuntime(13823):        at
org.chromium.base.ApplicationStatus.registerStateListenerForActivity(ApplicationStatus.java:359)
E/AndroidRuntime(13823):        at
org.chromium.ui.base.ActivityWindowAndroid.<init>(ActivityWindowAndroid.java:36)
E/AndroidRuntime(13823):        at
org.chromium.android_webview.AwContents.setNewAwContents(AwContents.java:873)
E/AndroidRuntime(13823):        at
org.chromium.android_webview.AwContents.<init>(AwContents.java:694)
E/AndroidRuntime(13823):        at
org.chromium.android_webview.AwContents.<init>(AwContents.java:623)
E/AndroidRuntime(13823):        at
com.android.webview.chromium.WebViewChromium.initForReal(WebViewChromium.java:249)
E/AndroidRuntime(13823):        at
com.android.webview.chromium.WebViewChromium.access$100(WebViewChromium.java:70)
E/AndroidRuntime(13823):        at
com.android.webview.chromium.WebViewChromium$1.run(WebViewChromium.java:236)
E/AndroidRuntime(13823):        at
com.android.webview.chromium.WebViewChromium$WebViewChromiumRunQueue.drainQueue(WebViewChromium.java:96)
E/AndroidRuntime(13823):        at
com.android.webview.chromium.WebViewChromium$WebViewChromiumRunQueue$1.run(WebViewChromium.java:83)
E/AndroidRuntime(13823):        at
org.chromium.base.ThreadUtils.runOnUiThread(ThreadUtils.java:144)
E/AndroidRuntime(13823):        at
com.android.webview.chromium.WebViewChromium$WebViewChromiumRunQueue.addTask(WebViewChromium.java:80)
E/AndroidRuntime(13823):        at
com.android.webview.chromium.WebViewChromium.init(WebViewChromium.java:233)

[boliu, 2015-03-25 14:35:58]

The issue is probably not because the activity is null (otherwise webview
wouldn't create ActivityWindowAndroid).

Webview doesn't call ApplicationStatus.initialize and I thought it would be ok
since it simply doesn't any application callbacks, so all the observers will not
be called.

But afaict registerStateListenerForActivity can only register the activity
that's already monitored by ApplicationStatus, ie it must be associated with the
Application in ApplicationStatus.initialize, *and* that it must have been made
the current activity once.

Webview doesn't satisfy the first requirement, but that second one feels weird
as well of the rest of chrome.

Revert for now while we sort this out? I have some ideas how to test this in
webview instrumentation tests..

[boliu, 2015-03-25 14:41:14]

A revert of this CL (patchset #10 id:270001) has been created in
https://codereview.chromium.org/1038573004/ by boliu@chromium.org.

The reason for reverting is: Breaks Android WebView which does not call
ApplicationStatus.initialize.

[Ted C, 2015-03-25 15:36:48]

On 2015/03/25 14:41:14, boliu wrote:
> A revert of this CL (patchset #10 id:270001) has been created in
> https://codereview.chromium.org/1038573004/ by mailto:boliu@chromium.org.
> 
> The reason for reverting is: Breaks Android WebView which does not call
> ApplicationStatus.initialize.

If ApplicationStatus does not work for WebView, I think we need to
look into moving it into the Chrome package as this will continue to
bite us in the future.

[jdduke (slow), 2015-03-25 17:21:28]

Patchset #11 (id:290001) has been deleted

[jdduke (slow), 2015-03-25 17:23:05]

Per offline discussion, I've updated ActivityWindowAndroid to make activity
state listening optional.

boliu@: could you take a look at the changes to AwContents and
ActivityWindowAndroid?

sievers@: I also made the observer method rename we talked about but I failed to
add (OnVisibilityChanged -> OnRootWindowVisibilityChanged).

[no sievers, 2015-03-25 17:27:14]

lgtm

[boliu, 2015-03-25 17:30:11]

android_webview lgtm

Actually ran webview this time and make sure it doesn't crash anymore..

[jdduke (slow), 2015-03-25 17:31:31]

The CQ bit was checked by jdduke@chromium.org

[jdduke (slow), 2015-03-25 17:31:32]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/1001573003/#ps330001
(title: "Link to the WebView bug")

[commit-bot: I haz the power, 2015-03-25 17:32:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1001573003/330001

[commit-bot: I haz the power, 2015-03-25 19:27:44]

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

[commit-bot: I haz the power, 2015-03-25 19:27:46]

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

[jdduke (slow), 2015-03-25 19:42:44]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2015-03-25 19:43:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1001573003/330001

[commit-bot: I haz the power, 2015-03-25 20:15:09]

Committed patchset #12 (id:330001)

[commit-bot: I haz the power, 2015-03-25 20:16:12]

Patchset 12 (id:??) landed as
https://crrev.com/7954daf991a2adda234dc2e886b2d1ddf0049221
Cr-Commit-Position: refs/heads/master@{#322228}

[jdduke (slow), 2015-04-27 17:35:38]

A revert of this CL (patchset #12 id:330001) has been created in
https://codereview.chromium.org/1109863003/ by jdduke@chromium.org.

The reason for reverting is: Speculative fix for blank display after cold
startup, crbug.com/481115..

[jdduke (slow), 2015-05-11 20:46:48]

Patchset #14 (id:370001) has been deleted

[jdduke (slow), 2015-05-11 20:51:04]

jdduke@chromium.org changed reviewers:
- dtrainor@chromium.org, torne@chromium.org

[jdduke (slow), 2015-05-11 20:51:05]

Ted, any thoughts on the changes in patchset #13 to Tab.java? That change
resolves the problems in crbug.com/481450.

[jdduke (slow), 2015-05-11 20:51:22]

On 2015/05/11 20:51:05, jdduke wrote:
> Ted, any thoughts on the changes in patchset #13 to Tab.java? That change
> resolves the problems in crbug.com/481450.

And by #13 I mean patchset #14.

[Ted C, 2015-05-11 20:59:26]

On 2015/05/11 20:51:22, jdduke wrote:
> On 2015/05/11 20:51:05, jdduke wrote:
> > Ted, any thoughts on the changes in patchset #13 to Tab.java? That change
> > resolves the problems in crbug.com/481450.
> 
> And by #13 I mean patchset #14.

lgtm -- can we write a test for the bug?

[jdduke (slow), 2015-05-11 22:28:56]

On 2015/05/11 20:59:26, Ted C wrote:
> On 2015/05/11 20:51:22, jdduke wrote:
> > On 2015/05/11 20:51:05, jdduke wrote:
> > > Ted, any thoughts on the changes in patchset #13 to Tab.java? That change
> > > resolves the problems in crbug.com/481450.
> > 
> > And by #13 I mean patchset #14.
> 
> lgtm -- can we write a test for the bug?

Yup, done.

[jdduke (slow), 2015-05-11 22:31:00]

The CQ bit was checked by jdduke@chromium.org

[jdduke (slow), 2015-05-11 22:31:01]

The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org,
piman@chromium.org, sievers@chromium.org, tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/1001573003/#ps410001
(title: "Add test")

[commit-bot: I haz the power, 2015-05-11 22:31:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1001573003/410001

[commit-bot: I haz the power, 2015-05-11 23:54:39]

Committed patchset #15 (id:410001)

[commit-bot: I haz the power, 2015-05-11 23:56:13]

Patchset 15 (id:??) landed as
https://crrev.com/61c7526b6acb7b29d27da7156bea7ddea71477d7
Cr-Commit-Position: refs/heads/master@{#329283}