Issue 2564403002: [android] Make RWHVAndroid a BeginFrameObserver.

Eric Seckler

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

4 years ago (2016-12-12 13:11:20 UTC) #1

commit-bot: I haz the power

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

4 years ago (2016-12-12 13:11:26 UTC) #2

commit-bot: I haz the power

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

4 years ago (2016-12-12 13:35:22 UTC) #3

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_android/builds/178741)

4 years ago (2016-12-12 13:35:23 UTC) #4

Eric Seckler

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

4 years ago (2016-12-12 15:04:21 UTC) #5

commit-bot: I haz the power

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

4 years ago (2016-12-12 15:04:38 UTC) #6

commit-bot: I haz the power

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

4 years ago (2016-12-12 15:26:10 UTC) #7

commit-bot: I haz the power

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

4 years ago (2016-12-12 15:26:11 UTC) #8

Eric Seckler

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

4 years ago (2016-12-12 15:30:39 UTC) #9

commit-bot: I haz the power

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

4 years ago (2016-12-12 15:31:03 UTC) #10

Eric Seckler

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

4 years ago (2016-12-12 16:27:59 UTC) #11

commit-bot: I haz the power

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

4 years ago (2016-12-12 16:53:37 UTC) #15

boliu

https://codereview.chromium.org/2564403002/diff/40001/content/browser/android/synchronous_compositor_browser_filter.cc File content/browser/android/synchronous_compositor_browser_filter.cc (right): https://codereview.chromium.org/2564403002/diff/40001/content/browser/android/synchronous_compositor_browser_filter.cc#newcode220 content/browser/android/synchronous_compositor_browser_filter.cc:220: static cc::BeginFrameArgs args; static is overkill https://codereview.chromium.org/2564403002/diff/40001/content/browser/renderer_host/render_widget_host_view_android.cc File content/browser/renderer_host/render_widget_host_view_android.cc ...

4 years ago (2016-12-12 21:41:48 UTC) #17

Eric Seckler

Thanks, Bo! https://codereview.chromium.org/2564403002/diff/40001/content/browser/android/synchronous_compositor_browser_filter.cc File content/browser/android/synchronous_compositor_browser_filter.cc (right): https://codereview.chromium.org/2564403002/diff/40001/content/browser/android/synchronous_compositor_browser_filter.cc#newcode220 content/browser/android/synchronous_compositor_browser_filter.cc:220: static cc::BeginFrameArgs args; On 2016/12/12 21:41:48, boliu ...

4 years ago (2016-12-13 10:03:56 UTC) #19

Thanks, Bo!

https://codereview.chromium.org/2564403002/diff/40001/content/browser/android...
File content/browser/android/synchronous_compositor_browser_filter.cc (right):

https://codereview.chromium.org/2564403002/diff/40001/content/browser/android...
content/browser/android/synchronous_compositor_browser_filter.cc:220: static
cc::BeginFrameArgs args;
On 2016/12/12 21:41:48, boliu wrote:
> static is overkill

hmm, this returns a reference - don't think I can return a reference to a local
var without the compiler complaining?

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

https://codereview.chromium.org/2564403002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:857: // the
current vsync phase.
On 2016/12/12 21:41:48, boliu wrote:
> I think previous comment is fine.
> 
> RWHVA could already be observing begin frames, in which case the missed frame
> won't help. Also looking at https://codereview.chromium.org/1879833002 when
the
> MISSED frame behavior was added, it was more for the browser compositor rather
> than the renderer one.

Ok, reverted. We probably don't want to get rid of missed frames entirely, see
below.

https://codereview.chromium.org/2564403002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1444:
DCHECK(this);
On 2016/12/12 21:41:48, boliu wrote:
> hmm?

thanks, was debugging here.. removed :)

https://codereview.chromium.org/2564403002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1453:
DCHECK(windowAndroid);
On 2016/12/12 21:41:48, boliu wrote:
> So windowAndroid pointer can change throughout the lifetime of the RWHVA, and
> the start/stopObservingWindowAndroid calls roughly corresponds to when the
> window android is set/unset
> 
> For the webview case, I think this is still handled correctly.
> 
> What's not clear for me is for chrome after begin frame is unified, will the
> begin frame source ever change, even when the windowAndroid changes? And does
> the begin frame call still go through windowAndroid?
> 
> If not, then technically, this DCHECK isn't really valid for chrome anymore.
> 
> If yes, then will need to ensure that sequence still works correctly

In android-chrome, there's a direct relationship between a WindowAndroid and
cc::Display (one CompositorImpl instance, which owns cc::Display, per
WindowAndroid). The cc::Display receives the WindowAndroid's BFS, and registers
it at SurfaceManager, which then distributes it to SurfaceFactoryClients, one of
which is DelegatedFrameHostAndroid.

Thus, in chrome:
 - The BFS of RWHVAndroid changes when WindowAndroid changes.
 - The BFS provided by WindowAndroid feeds both the browser compositor
(cc::Display) and (possibly multiple) RWHVAndroid.

Irrespective of this, I'm not sure if the behavior you saw in crbug.com/639868
may still occur - and whether this check is still needed. In tests, the
DCHECK(windowAndroid) isn't hit - but it probably didn't earlier either.

https://codereview.chromium.org/2564403002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1520:
DCHECK_EQ(nullptr, begin_frame_source_);
On 2016/12/12 21:41:48, boliu wrote:
> nit: DCHECK(!begin_frame_source_)?

Done.

https://codereview.chromium.org/2564403002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1910: if
(observing_root_window_) {
On 2016/12/12 21:41:48, boliu wrote:
> this should not be needed, this still comes from the attached WindowAndroid,
so
> this should always be true here, hopefully
> 
> does this not hold anymore? that could indicate a bug (that I'm not seeing..)

I think this was a preexisting bug. What you say amounts to
|view_.GetWindowAndroid()| not being null. But we only observe the window if it
is actually showing. Thus, there are valid cases for |observing_root_window_| to
be false here. (And since Register/UnregisterFrameSinkHierarchy is coupled to
Start/StopObservingRootWindow, we should only RegisterFrameSinkHierarchy here if
|observing_root_window_| is true.)

Previously, this wouldn't have made much of a difference, I suppose. But now, we
receive the BFS from |delegated_frame_host_| if we RegisterFrameSinkHierarchy().
As a result, we wouldn't necessarily de-register it later because
StopObservingRootWindow() might early-out because |!observing_root_window_|,
which might leave our pointer dangling (and if I recall correctly, this happened
in some tests).

I can remove this again and see which (if any) tests fail if that's of interest
to you.

https://codereview.chromium.org/2564403002/diff/40001/ui/android/window_andro...
File ui/android/window_android.cc (right):

https://codereview.chromium.org/2564403002/diff/40001/ui/android/window_andro...
ui/android/window_android.cc:57: DCHECK(observers_.find(obs) ==
observers_.end());
On 2016/12/12 21:41:48, boliu wrote:
> nit: use base::ContainsKey? more readable

Done.

https://codereview.chromium.org/2564403002/diff/40001/ui/android/window_andro...
ui/android/window_android.cc:63: // Send a MISSED BeginFrame if possible and
necessary. If an observer is added
On 2016/12/12 21:41:48, boliu wrote:
> any reason this is needed?

I'm somewhat new to BeginFrames myself, so brianderson@ might correct me here.
Since this source also feeds the DisplayScheduler, I don't think we can get rid
of missed BeginFrames. Here an attempt at an explanation :)

One note on background of (missed) BeginFrames vs. vsyncs:
WindowAndroidObservers don't remove themselves when they aren't interested in
BeginFrames, while BeginFrameObservers do. Thus, with WindowAndroidObservers,
even if just one observer requests a vsync, all observers will be notified of it
through OnVSync(). With BeginFrameObservers, this is not the case: Only those
that are interested will receive it.

Some observers may awake during a vsync interval (i.e. decide that they need a
BeginFrame afterall) -- and for those, receiving a MISSED BeginFrame might be
advantageous.

In the android-chrome case, I think one such BeginFrameObserver is the
DisplayScheduler: It may go idle (remove itself as observer) when a BeginFrame
doesn't lead to a DrawAndSwap. Later, it will awake again (add itself as
observer) when damage to a surface is detected. It'll then receive a MISSED
BeginFrame for the currently active BeginFrame and (likely) DrawAndSwap the
surface damage during that BeginFrame's deadline. If it didn't receive the
missed BeginFrame, it'd have to wait until the next BeginFrame's deadline.

https://codereview.chromium.org/2564403002/diff/40001/ui/android/window_andro...
ui/android/window_android.cc:84: DCHECK(observers_.find(obs) !=
observers_.end());
On 2016/12/12 21:41:48, boliu wrote:
> ditto ContainsKey

Done.

https://codereview.chromium.org/2564403002/diff/40001/ui/android/window_andro...
ui/android/window_android.cc:110: while (observer_was_notified) {
On 2016/12/12 21:41:48, boliu wrote:
> any reason we don't keep using base::ObserverList and keep getting this
behavior
> for free?

For two (rather subjective) reasons:
1) This makes it explicit that SyncCompBrowserFilter is supported.
2) We get an immediate feedback about whether |observers_| is empty during
RemoveObserver() -- which we wouldn't get with base::ObserverList.

That said, I think we could also use base::ObserverList and solve
1) by adding a comment somewhere.
2) by checking after observer iteration in OnVSync() whether the list became
empty during the iteration.

WDYT?

https://codereview.chromium.org/2564403002/diff/40001/ui/android/window_andro...
File ui/android/window_android.h (right):

https://codereview.chromium.org/2564403002/diff/40001/ui/android/window_andro...
ui/android/window_android.h:69: void SetNeedsBeginFrames(bool
needs_begin_frames);
On 2016/12/12 21:41:48, boliu wrote:
> private, and friend WindowBeginFrameSource?

Done.

boliu

haven't looked at new code yet, just replying to comments https://codereview.chromium.org/2564403002/diff/40001/content/browser/android/synchronous_compositor_browser_filter.cc File content/browser/android/synchronous_compositor_browser_filter.cc (right): https://codereview.chromium.org/2564403002/diff/40001/content/browser/android/synchronous_compositor_browser_filter.cc#newcode220 ...

4 years ago (2016-12-13 18:07:48 UTC) #20

haven't looked at new code yet, just replying to comments

https://codereview.chromium.org/2564403002/diff/40001/content/browser/android...
File content/browser/android/synchronous_compositor_browser_filter.cc (right):

https://codereview.chromium.org/2564403002/diff/40001/content/browser/android...
content/browser/android/synchronous_compositor_browser_filter.cc:220: static
cc::BeginFrameArgs args;
On 2016/12/13 10:03:55, Eric Seckler wrote:
> On 2016/12/12 21:41:48, boliu wrote:
> > static is overkill
> 
> hmm, this returns a reference - don't think I can return a reference to a
local
> var without the compiler complaining?

Doh, yeah, ok.

But then I think this causes a static initializer, which is supposed to be
banned (I'm not sure if there are bots on android that checks this though)

Just use a member variable

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

https://codereview.chromium.org/2564403002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1453:
DCHECK(windowAndroid);
On 2016/12/13 10:03:56, Eric Seckler wrote:
> On 2016/12/12 21:41:48, boliu wrote:
> > So windowAndroid pointer can change throughout the lifetime of the RWHVA,
and
> > the start/stopObservingWindowAndroid calls roughly corresponds to when the
> > window android is set/unset
> > 
> > For the webview case, I think this is still handled correctly.
> > 
> > What's not clear for me is for chrome after begin frame is unified, will the
> > begin frame source ever change, even when the windowAndroid changes? And
does
> > the begin frame call still go through windowAndroid?
> > 
> > If not, then technically, this DCHECK isn't really valid for chrome anymore.
> > 
> > If yes, then will need to ensure that sequence still works correctly
> 
> In android-chrome, there's a direct relationship between a WindowAndroid and
> cc::Display (one CompositorImpl instance, which owns cc::Display, per
> WindowAndroid). The cc::Display receives the WindowAndroid's BFS, and
registers
> it at SurfaceManager, which then distributes it to SurfaceFactoryClients, one
of
> which is DelegatedFrameHostAndroid.
> 
> Thus, in chrome:
>  - The BFS of RWHVAndroid changes when WindowAndroid changes.
>  - The BFS provided by WindowAndroid feeds both the browser compositor
> (cc::Display) and (possibly multiple) RWHVAndroid.
> 
> Irrespective of this, I'm not sure if the behavior you saw in crbug.com/639868
> may still occur - and whether this check is still needed. In tests, the
> DCHECK(windowAndroid) isn't hit - but it probably didn't earlier either.

And it looks like RegisterFrameSinkHierarchy->RegisterSurfaceFactoryClient is
where this whole BFS path is hooked up.

Ok, just remove getting the window android here then. It's definitely not going
to prevent any crash anymore afaict.

https://codereview.chromium.org/2564403002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1910: if
(observing_root_window_) {
On 2016/12/13 10:03:56, Eric Seckler wrote:
> On 2016/12/12 21:41:48, boliu wrote:
> > this should not be needed, this still comes from the attached WindowAndroid,
> so
> > this should always be true here, hopefully
> > 
> > does this not hold anymore? that could indicate a bug (that I'm not
seeing..)
> 
> I think this was a preexisting bug. What you say amounts to
> |view_.GetWindowAndroid()| not being null. But we only observe the window if
it
> is actually showing. Thus, there are valid cases for |observing_root_window_|
to
> be false here. (And since Register/UnregisterFrameSinkHierarchy is coupled to
> Start/StopObservingRootWindow, we should only RegisterFrameSinkHierarchy here
if
> |observing_root_window_| is true.)
> 
> Previously, this wouldn't have made much of a difference, I suppose. But now,
we
> receive the BFS from |delegated_frame_host_| if we
RegisterFrameSinkHierarchy().
> As a result, we wouldn't necessarily de-register it later because
> StopObservingRootWindow() might early-out because |!observing_root_window_|,
> which might leave our pointer dangling (and if I recall correctly, this
happened
> in some tests).
> 
> I can remove this again and see which (if any) tests fail if that's of
interest
> to you.

Keep it. Your analysis looks correct to me

https://codereview.chromium.org/2564403002/diff/40001/ui/android/window_andro...
File ui/android/window_android.cc (right):

https://codereview.chromium.org/2564403002/diff/40001/ui/android/window_andro...
ui/android/window_android.cc:63: // Send a MISSED BeginFrame if possible and
necessary. If an observer is added
On 2016/12/13 10:03:56, Eric Seckler wrote:
> On 2016/12/12 21:41:48, boliu wrote:
> > any reason this is needed?
> 
> I'm somewhat new to BeginFrames myself, so brianderson@ might correct me here.
> Since this source also feeds the DisplayScheduler, I don't think we can get
rid
> of missed BeginFrames. Here an attempt at an explanation :)
> 
> One note on background of (missed) BeginFrames vs. vsyncs:
> WindowAndroidObservers don't remove themselves when they aren't interested in
> BeginFrames, while BeginFrameObservers do. Thus, with WindowAndroidObservers,
> even if just one observer requests a vsync, all observers will be notified of
it
> through OnVSync(). With BeginFrameObservers, this is not the case: Only those
> that are interested will receive it.
> 
> Some observers may awake during a vsync interval (i.e. decide that they need a
> BeginFrame afterall) -- and for those, receiving a MISSED BeginFrame might be
> advantageous.
> 
> In the android-chrome case, I think one such BeginFrameObserver is the
> DisplayScheduler: It may go idle (remove itself as observer) when a BeginFrame
> doesn't lead to a DrawAndSwap. Later, it will awake again (add itself as
> observer) when damage to a surface is detected. It'll then receive a MISSED
> BeginFrame for the currently active BeginFrame and (likely) DrawAndSwap the
> surface damage during that BeginFrame's deadline. If it didn't receive the
> missed BeginFrame, it'd have to wait until the next BeginFrame's deadline.

Sorry, typed that comment before I realized this code was moved from
CompositorImpl.. No justification needed :p

https://codereview.chromium.org/2564403002/diff/40001/ui/android/window_andro...
ui/android/window_android.cc:110: while (observer_was_notified) {
On 2016/12/13 10:03:56, Eric Seckler wrote:
> On 2016/12/12 21:41:48, boliu wrote:
> > any reason we don't keep using base::ObserverList and keep getting this
> behavior
> > for free?
> 
> For two (rather subjective) reasons:
> 1) This makes it explicit that SyncCompBrowserFilter is supported.
> 2) We get an immediate feedback about whether |observers_| is empty during
> RemoveObserver() -- which we wouldn't get with base::ObserverList.
> 
> That said, I think we could also use base::ObserverList and solve
> 1) by adding a comment somewhere.
> 2) by checking after observer iteration in OnVSync() whether the list became
> empty during the iteration.

Or you can just keep a count of the number of observers inserted. Then you can
remove this complexity. Can still keep a comment about the requirement for
AddObserver and iteration behavior.

I guess ObserverList is implemented as a sparse array, so getting the number of
things is actually expensive..

> 
> WDYT?

Eric Seckler

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

4 years ago (2016-12-14 14:16:59 UTC) #21

commit-bot: I haz the power

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

4 years ago (2016-12-14 14:17:12 UTC) #22

commit-bot: I haz the power

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

4 years ago (2016-12-14 15:28:46 UTC) #23

commit-bot: I haz the power

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

4 years ago (2016-12-14 15:28:46 UTC) #24

Eric Seckler

https://codereview.chromium.org/2564403002/diff/40001/content/browser/android/synchronous_compositor_browser_filter.cc File content/browser/android/synchronous_compositor_browser_filter.cc (right): https://codereview.chromium.org/2564403002/diff/40001/content/browser/android/synchronous_compositor_browser_filter.cc#newcode220 content/browser/android/synchronous_compositor_browser_filter.cc:220: static cc::BeginFrameArgs args; On 2016/12/13 18:07:47, boliu wrote: > ...

4 years ago (2016-12-14 15:36:57 UTC) #25

https://codereview.chromium.org/2564403002/diff/40001/content/browser/android...
File content/browser/android/synchronous_compositor_browser_filter.cc (right):

https://codereview.chromium.org/2564403002/diff/40001/content/browser/android...
content/browser/android/synchronous_compositor_browser_filter.cc:220: static
cc::BeginFrameArgs args;
On 2016/12/13 18:07:47, boliu wrote:
> On 2016/12/13 10:03:55, Eric Seckler wrote:
> > On 2016/12/12 21:41:48, boliu wrote:
> > > static is overkill
> > 
> > hmm, this returns a reference - don't think I can return a reference to a
> local
> > var without the compiler complaining?
> 
> Doh, yeah, ok.
> 
> But then I think this causes a static initializer, which is supposed to be
> banned (I'm not sure if there are bots on android that checks this though)
> 
> Just use a member variable

Don't think it would cause a static initializer - static in function scope
should be initialized at first invocation. Anyway, replaced by a member variable
since this seems to be irritating :)

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

https://codereview.chromium.org/2564403002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1453:
DCHECK(windowAndroid);
On 2016/12/13 18:07:47, boliu wrote:
> On 2016/12/13 10:03:56, Eric Seckler wrote:
> > On 2016/12/12 21:41:48, boliu wrote:
> > > So windowAndroid pointer can change throughout the lifetime of the RWHVA,
> and
> > > the start/stopObservingWindowAndroid calls roughly corresponds to when the
> > > window android is set/unset
> > > 
> > > For the webview case, I think this is still handled correctly.
> > > 
> > > What's not clear for me is for chrome after begin frame is unified, will
the
> > > begin frame source ever change, even when the windowAndroid changes? And
> does
> > > the begin frame call still go through windowAndroid?
> > > 
> > > If not, then technically, this DCHECK isn't really valid for chrome
anymore.
> > > 
> > > If yes, then will need to ensure that sequence still works correctly
> > 
> > In android-chrome, there's a direct relationship between a WindowAndroid and
> > cc::Display (one CompositorImpl instance, which owns cc::Display, per
> > WindowAndroid). The cc::Display receives the WindowAndroid's BFS, and
> registers
> > it at SurfaceManager, which then distributes it to SurfaceFactoryClients,
one
> of
> > which is DelegatedFrameHostAndroid.
> > 
> > Thus, in chrome:
> >  - The BFS of RWHVAndroid changes when WindowAndroid changes.
> >  - The BFS provided by WindowAndroid feeds both the browser compositor
> > (cc::Display) and (possibly multiple) RWHVAndroid.
> > 
> > Irrespective of this, I'm not sure if the behavior you saw in
crbug.com/639868
> > may still occur - and whether this check is still needed. In tests, the
> > DCHECK(windowAndroid) isn't hit - but it probably didn't earlier either.
> 
> And it looks like RegisterFrameSinkHierarchy->RegisterSurfaceFactoryClient is
> where this whole BFS path is hooked up.
> 
> Ok, just remove getting the window android here then. It's definitely not
going
> to prevent any crash anymore afaict.

Ok, got rid of it.

https://codereview.chromium.org/2564403002/diff/40001/ui/android/window_andro...
File ui/android/window_android.cc (right):

https://codereview.chromium.org/2564403002/diff/40001/ui/android/window_andro...
ui/android/window_android.cc:110: while (observer_was_notified) {
On 2016/12/13 18:07:47, boliu wrote:
> On 2016/12/13 10:03:56, Eric Seckler wrote:
> > On 2016/12/12 21:41:48, boliu wrote:
> > > any reason we don't keep using base::ObserverList and keep getting this
> > behavior
> > > for free?
> > 
> > For two (rather subjective) reasons:
> > 1) This makes it explicit that SyncCompBrowserFilter is supported.
> > 2) We get an immediate feedback about whether |observers_| is empty during
> > RemoveObserver() -- which we wouldn't get with base::ObserverList.
> > 
> > That said, I think we could also use base::ObserverList and solve
> > 1) by adding a comment somewhere.
> > 2) by checking after observer iteration in OnVSync() whether the list became
> > empty during the iteration.
> 
> Or you can just keep a count of the number of observers inserted. Then you can
> remove this complexity. Can still keep a comment about the requirement for
> AddObserver and iteration behavior.
> 
> I guess ObserverList is implemented as a sparse array, so getting the number
of
> things is actually expensive..
> 
> > 
> > WDYT?
> 

Sounds good, done.

boliu

lgtm % last comment https://codereview.chromium.org/2564403002/diff/100001/content/browser/renderer_host/render_widget_host_view_android.cc File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2564403002/diff/100001/content/browser/renderer_host/render_widget_host_view_android.cc#newcode1451 content/browser/renderer_host/render_widget_host_view_android.cc:1451: if (source && requests && ...

4 years ago (2016-12-14 18:59:48 UTC) #26

Eric Seckler

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

4 years ago (2016-12-15 11:08:55 UTC) #27

commit-bot: I haz the power

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

4 years ago (2016-12-15 11:09:13 UTC) #28

Eric Seckler

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

4 years ago (2016-12-15 11:40:21 UTC) #29

Eric Seckler

+tedchoc for ui/android/ Thanks! https://codereview.chromium.org/2564403002/diff/100001/content/browser/renderer_host/render_widget_host_view_android.cc File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2564403002/diff/100001/content/browser/renderer_host/render_widget_host_view_android.cc#newcode1451 content/browser/renderer_host/render_widget_host_view_android.cc:1451: if (source && requests && ...

4 years ago (2016-12-15 11:40:22 UTC) #30

commit-bot: I haz the power

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

4 years ago (2016-12-15 11:46:59 UTC) #31

Ted C

On 2016/12/15 11:40:22, Eric Seckler wrote: > +tedchoc for ui/android/ > > Thanks! > > ...

4 years ago (2016-12-15 19:14:20 UTC) #33

Eric Seckler

The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org Link to the ...

4 years ago (2016-12-15 20:45:26 UTC) #35

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2564403002/120001

4 years ago (2016-12-15 20:46:05 UTC) #36

commit-bot: I haz the power

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

4 years ago (2016-12-15 20:55:03 UTC) #37

commit-bot: I haz the power

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

4 years ago (2016-12-15 20:55:04 UTC) #38

Eric Seckler

eseckler@chromium.org changed reviewers: + brianderson@chromium.org

4 years ago (2016-12-15 21:01:17 UTC) #39

Eric Seckler

Brian, could you rubber-stamp the new dependency on '+cc/scheduler/begin_frame_source.h' From ui/android/DEPS? Thanks :)

4 years ago (2016-12-15 21:01:17 UTC) #40

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2564403002/120001

4 years ago (2016-12-16 09:04:27 UTC) #43

commit-bot: I haz the power

CQ is committing da patch. Bot data: {"patchset_id": 120001, "attempt_start_ts": 1481879050317510, "parent_rev": "1233c3b7a536783760a918d7d1756cb8d67a9a94", "commit_rev": "d3149dd85e8d4cf6acd5357d0e9aa5940d268a7a"}

4 years ago (2016-12-16 09:09:08 UTC) #44

commit-bot: I haz the power

Description was changed from ========== [android] Make RWHVAndroid a BeginFrameObserver. Instead of directly observing the ...

4 years ago (2016-12-16 09:09:40 UTC) #45

commit-bot: I haz the power

Description was changed from ========== [android] Make RWHVAndroid a BeginFrameObserver. Instead of directly observing the ...

4 years ago (2016-12-16 09:12:08 UTC) #47

commit-bot: I haz the power

Patchset 4 (id:??) landed as https://crrev.com/1a6bd5037fc10e45646715d675e63146d23fc28d Cr-Commit-Position: refs/heads/master@{#439072}

4 years ago (2016-12-16 09:12:09 UTC) #48

rnephew (Reviews Here)

A revert of this CL (patchset #4 id:120001) has been created in https://codereview.chromium.org/2585993002/ by rnephew@chromium.org. ...

4 years ago (2016-12-17 04:11:05 UTC) #49

Eric Seckler

Description was changed from ========== [android] Make RWHVAndroid a BeginFrameObserver. Instead of directly observing the ...

4 years ago (2016-12-19 12:34:59 UTC) #50

Eric Seckler

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

4 years ago (2016-12-20 11:13:56 UTC) #53

commit-bot: I haz the power

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

4 years ago (2016-12-20 11:14:09 UTC) #54

commit-bot: I haz the power

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

4 years ago (2016-12-20 11:50:59 UTC) #55

Eric Seckler

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

4 years ago (2016-12-20 12:44:04 UTC) #58

commit-bot: I haz the power

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

4 years ago (2016-12-20 12:44:20 UTC) #59

Eric Seckler

Bo, can you have another look at changes in RWHVA and WindowAndroid please - Regression ...

4 years ago (2016-12-20 12:46:53 UTC) #60

Eric Seckler

Description was changed from ========== [android] Make RWHVAndroid a BeginFrameObserver. Instead of directly observing the ...

4 years ago (2016-12-20 12:54:54 UTC) #61

commit-bot: I haz the power

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

4 years ago (2016-12-20 13:47:41 UTC) #62

boliu

https://codereview.chromium.org/2564403002/diff/200001/content/browser/renderer_host/render_widget_host_view_android.cc File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2564403002/diff/200001/content/browser/renderer_host/render_widget_host_view_android.cc#newcode1926 content/browser/renderer_host/render_widget_host_view_android.cc:1926: // can lead to |begin_frame_source_->AddObserver()| and OnBeginFrame(). how does ...

4 years ago (2016-12-20 16:13:50 UTC) #66

Eric Seckler

https://codereview.chromium.org/2564403002/diff/200001/content/browser/renderer_host/render_widget_host_view_android.cc File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2564403002/diff/200001/content/browser/renderer_host/render_widget_host_view_android.cc#newcode1926 content/browser/renderer_host/render_widget_host_view_android.cc:1926: // can lead to |begin_frame_source_->AddObserver()| and OnBeginFrame(). On 2016/12/20 ...

4 years ago (2016-12-20 16:35:07 UTC) #67

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

https://codereview.chromium.org/2564403002/diff/200001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_android.cc:1926: // can
lead to |begin_frame_source_->AddObserver()| and OnBeginFrame().
On 2016/12/20 16:13:50, boliu wrote:
> how does moving last_begin_frame_args_ up fix this exactly? That the
FlushInput
> gets the right args?
It prevents the BeginFrameSource from sending us the same args again: During
AddObserver(), it checks if our last_begin_frame_args_ are older than its latest
ones. If they are older, it sends us a missed BeginFrame.

The reason we were getting stuck is that we called (RemoveObserver() and then)
AddObserver() before updating our last_begin_frame_args_, indicating to the
BeginFrameSource that we did not yet receive/handle its latest BeginFrameArgs.

> instead of the complicated thing in window_android, how about this fix:
> Only call Add/RemoveObserver at the *end* of this method. This way, it would
be
> impossible to toggle back and forth between states
I have two reservations against this and would rather like to keep the check in
WindowAndroid. Note though that this is a different issue only tangentially
related to crbug.com/675289, which I discovered during debugging. (Sorry, should
have made this clear I guess :))

The problem there is that we might send the same BeginFrameArgs to an observer
twice during OnVSync(), which is forbidden as noted in the comment of
BeginFrameObserver::OnBeginFrame().

The RWHVA is only one BeginFrameObserver, so changing it to not exhibit the
forbidden behavior doesn't really solve it, I think. There's no contract
forbidding other observers to remove/add themselves during iteration.
(WindowBeginFrameSource is exposed widely through the compositor.)

Regarding RWHVA: If possible, I'd like to avoid moving Add/RemoveObserver out of
the Add/ClearBeginFrameRequest methods - since otherwise we'd have to check if
we need to AddObserver() in all callsites of AddBeginFrameRequest().

Eric Seckler

On 2016/12/20 16:35:07, Eric Seckler wrote: > The problem there is that we might send ...

4 years ago (2016-12-20 16:52:46 UTC) #68

boliu

https://codereview.chromium.org/2564403002/diff/200001/content/browser/renderer_host/render_widget_host_view_android.cc File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2564403002/diff/200001/content/browser/renderer_host/render_widget_host_view_android.cc#newcode1926 content/browser/renderer_host/render_widget_host_view_android.cc:1926: // can lead to |begin_frame_source_->AddObserver()| and OnBeginFrame(). On 2016/12/20 ...

4 years ago (2016-12-20 16:56:09 UTC) #69

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

https://codereview.chromium.org/2564403002/diff/200001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_android.cc:1926: // can
lead to |begin_frame_source_->AddObserver()| and OnBeginFrame().
On 2016/12/20 16:35:07, Eric Seckler wrote:
> On 2016/12/20 16:13:50, boliu wrote:
> > how does moving last_begin_frame_args_ up fix this exactly? That the
> FlushInput
> > gets the right args?
> It prevents the BeginFrameSource from sending us the same args again: During
> AddObserver(), it checks if our last_begin_frame_args_ are older than its
latest
> ones. If they are older, it sends us a missed BeginFrame.
> 
> The reason we were getting stuck is that we called (RemoveObserver() and then)
> AddObserver() before updating our last_begin_frame_args_, indicating to the
> BeginFrameSource that we did not yet receive/handle its latest BeginFrameArgs.

oh, then do say that in the comment, that updating last begin frame args will
prevent infinite recursion. also don't need the bug number I guess, since it
will be fixed

> 
> > instead of the complicated thing in window_android, how about this fix:
> > Only call Add/RemoveObserver at the *end* of this method. This way, it would
> be
> > impossible to toggle back and forth between states
> I have two reservations against this and would rather like to keep the check
in
> WindowAndroid. Note though that this is a different issue only tangentially
> related to crbug.com/675289, which I discovered during debugging. (Sorry,
should
> have made this clear I guess :))
> 
> The problem there is that we might send the same BeginFrameArgs to an observer
> twice during OnVSync(), which is forbidden as noted in the comment of
> BeginFrameObserver::OnBeginFrame().
> 
> The RWHVA is only one BeginFrameObserver, so changing it to not exhibit the
> forbidden behavior doesn't really solve it, I think. There's no contract
> forbidding other observers to remove/add themselves during iteration.
> (WindowBeginFrameSource is exposed widely through the compositor.)
> 
> Regarding RWHVA: If possible, I'd like to avoid moving Add/RemoveObserver out
of
> the Add/ClearBeginFrameRequest methods - since otherwise we'd have to check if
> we need to AddObserver() in all callsites of AddBeginFrameRequest().

how about actually comparing LastBeginFrameArgs then, rather than keeping the
set?

But then probably should implement LastUsedBeginFrameArgs properly in
synchronous_compositor_browser_filter

Eric Seckler

On 2016/12/20 16:56:09, boliu wrote: > oh, then do say that in the comment, that ...

4 years ago (2016-12-20 17:29:21 UTC) #70

boliu

On 2016/12/20 16:52:46, Eric Seckler wrote: > On 2016/12/20 16:35:07, Eric Seckler wrote: > > ...

4 years ago (2016-12-20 17:35:48 UTC) #71

Eric Seckler

On 2016/12/20 17:35:48, boliu wrote: > On 2016/12/20 16:52:46, Eric Seckler wrote: > > On ...

4 years ago (2016-12-20 17:41:52 UTC) #72

Eric Seckler

On 2016/12/20 17:35:48, boliu wrote: > On 2016/12/20 16:52:46, Eric Seckler wrote: > > On ...

4 years ago (2016-12-20 17:42:07 UTC) #73

Eric Seckler

On 2016/12/20 17:43:35, boliu wrote: > lgtm Thanks again, Bo! :)

4 years ago (2016-12-20 17:48:25 UTC) #75

Eric Seckler

The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, brianderson@chromium.org, skyostil@chromium.org Link ...

4 years ago (2016-12-20 17:48:55 UTC) #77

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2564403002/240001

4 years ago (2016-12-20 17:49:28 UTC) #78

commit-bot: I haz the power

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

4 years ago (2016-12-20 19:04:48 UTC) #79

commit-bot: I haz the power

Failed to apply patch for content/browser/renderer_host/render_widget_host_view_android.cc: While running git apply --index -p1; error: patch failed: ...

4 years ago (2016-12-20 19:04:53 UTC) #80

Failed to apply patch for
content/browser/renderer_host/render_widget_host_view_android.cc:
While running git apply --index -p1;
  error: patch failed:
content/browser/renderer_host/render_widget_host_view_android.cc:798
  error: content/browser/renderer_host/render_widget_host_view_android.cc: patch
does not apply

Patch:       content/browser/renderer_host/render_widget_host_view_android.cc
Index: content/browser/renderer_host/render_widget_host_view_android.cc
diff --git a/content/browser/renderer_host/render_widget_host_view_android.cc
b/content/browser/renderer_host/render_widget_host_view_android.cc
index
08885281bf791989e0ea5321f566ba61adb0212a..ed7ca1e0373c621aace834ff809c274f2ce410c6
100644
--- a/content/browser/renderer_host/render_widget_host_view_android.cc
+++ b/content/browser/renderer_host/render_widget_host_view_android.cc
@@ -77,7 +77,6 @@
 #include "third_party/khronos/GLES2/gl2.h"
 #include "third_party/khronos/GLES2/gl2ext.h"
 #include "third_party/skia/include/core/SkCanvas.h"
-#include "ui/android/delegated_frame_host_android.h"
 #include "ui/android/window_android.h"
 #include "ui/android/window_android_compositor.h"
 #include "ui/base/layout.h"
@@ -432,7 +431,8 @@ RenderWidgetHostViewAndroid::RenderWidgetHostViewAndroid(
     RenderWidgetHostImpl* widget_host,
     ContentViewCoreImpl* content_view_core)
     : host_(widget_host),
-      outstanding_vsync_requests_(0),
+      begin_frame_source_(nullptr),
+      outstanding_begin_frame_requests_(0),
       is_showing_(!widget_host->is_hidden()),
       is_window_visible_(true),
       is_window_activity_started_(true),
@@ -456,9 +456,7 @@ RenderWidgetHostViewAndroid::RenderWidgetHostViewAndroid(
   view_.SetLayer(cc::Layer::Create());
   if (using_browser_compositor_) {
     delegated_frame_host_.reset(new ui::DelegatedFrameHostAndroid(
-        &view_, cached_background_color_,
-        base::Bind(&RenderWidgetHostViewAndroid::ReturnResources,
-                   weak_ptr_factory_.GetWeakPtr())));
+        &view_, cached_background_color_, this));
   }
 
   host_->SetView(this);
@@ -798,9 +796,9 @@ void RenderWidgetHostViewAndroid::SetNeedsBeginFrames(bool
needs_begin_frames) {
   TRACE_EVENT1("cc", "RenderWidgetHostViewAndroid::SetNeedsBeginFrames",
                "needs_begin_frames", needs_begin_frames);
   if (needs_begin_frames)
-    RequestVSyncUpdate(PERSISTENT_BEGIN_FRAME);
+    AddBeginFrameRequest(PERSISTENT_BEGIN_FRAME);
   else
-    outstanding_vsync_requests_ &= ~PERSISTENT_BEGIN_FRAME;
+    ClearBeginFrameRequest(PERSISTENT_BEGIN_FRAME);
 }
 
 void RenderWidgetHostViewAndroid::OnStartContentIntent(
@@ -854,7 +852,7 @@ bool RenderWidgetHostViewAndroid::OnTouchEvent(
   // that BeginFrame requests made during ACTION_MOVE dispatch will be honored
   // in the same vsync phase.
   if (observing_root_window_ && result.moved_beyond_slop_region)
-    RequestVSyncUpdate(BEGIN_FRAME);
+    AddBeginFrameRequest(BEGIN_FRAME);
 
   return true;
 }
@@ -1385,9 +1383,9 @@ void RenderWidgetHostViewAndroid::ShowInternal() {
 
   host_->WasShown(ui::LatencyInfo());
 
-  if (content_view_core_) {
+  if (content_view_core_ && view_.GetWindowAndroid()) {
     StartObservingRootWindow();
-    RequestVSyncUpdate(BEGIN_FRAME);
+    AddBeginFrameRequest(BEGIN_FRAME);
   }
 }
 
@@ -1430,29 +1428,43 @@ void RenderWidgetHostViewAndroid::HideInternal() {
   host_->WasHidden();
 }
 
-void RenderWidgetHostViewAndroid::RequestVSyncUpdate(uint32_t requests) {
-  bool should_request_vsync = !outstanding_vsync_requests_ && requests;
-  outstanding_vsync_requests_ |= requests;
-
-  // Note that if we're not currently observing the root window, outstanding
-  // vsync requests will be pushed if/when we resume observing in
-  // |StartObservingRootWindow()|.
-  if (observing_root_window_ && should_request_vsync) {
-    ui::WindowAndroid* windowAndroid = view_.GetWindowAndroid();
-    DCHECK(windowAndroid);
-    // TODO(boliu): This check should be redundant with
-    // |observing_root_window_| check above. However we are receiving trickle
-    // of crash reports (crbug.com/639868) with no root cause. Should
-    // investigate more when time allows what corner case is missed.
-    if (windowAndroid)
-      windowAndroid->RequestVSyncUpdate();
-  }
+void RenderWidgetHostViewAndroid::SetBeginFrameSource(
+    cc::BeginFrameSource* begin_frame_source) {
+  if (begin_frame_source_ == begin_frame_source)
+    return;
+
+  if (begin_frame_source_ && outstanding_begin_frame_requests_)
+    begin_frame_source_->RemoveObserver(this);
+  begin_frame_source_ = begin_frame_source;
+  if (begin_frame_source_ && outstanding_begin_frame_requests_)
+    begin_frame_source_->AddObserver(this);
+}
+
+void RenderWidgetHostViewAndroid::AddBeginFrameRequest(
+    BeginFrameRequestType request) {
+  uint32_t prior_requests = outstanding_begin_frame_requests_;
+  outstanding_begin_frame_requests_ = prior_requests | request;
+
+  // Note that if we don't currently have a BeginFrameSource, outstanding begin
+  // frame requests will be pushed if/when we get one during
+  // |StartObservingRootWindow()| or when the DelegatedFrameHostAndroid sets
it.
+  cc::BeginFrameSource* source = begin_frame_source_;
+  if (source && outstanding_begin_frame_requests_ && !prior_requests)
+    source->AddObserver(this);
+}
+
+void RenderWidgetHostViewAndroid::ClearBeginFrameRequest(
+    BeginFrameRequestType request) {
+  uint32_t prior_requests = outstanding_begin_frame_requests_;
+  outstanding_begin_frame_requests_ = prior_requests & ~request;
+
+  cc::BeginFrameSource* source = begin_frame_source_;
+  if (source && !outstanding_begin_frame_requests_ && prior_requests)
+    source->RemoveObserver(this);
 }
 
 void RenderWidgetHostViewAndroid::StartObservingRootWindow() {
   DCHECK(content_view_core_);
-  // TODO(yusufo): This will need to have a better fallback for cases where
-  // setContentViewCore is called with a valid ContentViewCore without a
window.
   DCHECK(view_.GetWindowAndroid());
   DCHECK(is_showing_);
   if (observing_root_window_)
@@ -1462,11 +1474,9 @@ void
RenderWidgetHostViewAndroid::StartObservingRootWindow() {
   if (host_)
     host_->Send(new ViewMsg_SetBeginFramePaused(host_->GetRoutingID(), false));
   view_.GetWindowAndroid()->AddObserver(this);
-
-  // Clear existing vsync requests to allow a request to the new window.
-  uint32_t outstanding_vsync_requests = outstanding_vsync_requests_;
-  outstanding_vsync_requests_ = 0;
-  RequestVSyncUpdate(outstanding_vsync_requests);
+  // When using browser compositor, DelegatedFrameHostAndroid provides the BFS.
+  if (!using_browser_compositor_)
+    SetBeginFrameSource(view_.GetWindowAndroid()->GetBeginFrameSource());
 
   ui::WindowAndroidCompositor* compositor =
       view_.GetWindowAndroid()->GetCompositor();
@@ -1492,26 +1502,25 @@ void
RenderWidgetHostViewAndroid::StopObservingRootWindow() {
   if (host_)
     host_->Send(new ViewMsg_SetBeginFramePaused(host_->GetRoutingID(), true));
   view_.GetWindowAndroid()->RemoveObserver(this);
+  if (!using_browser_compositor_)
+    SetBeginFrameSource(nullptr);
   // If the DFH has already been destroyed, it will have cleaned itself up.
   // This happens in some WebView cases.
   if (delegated_frame_host_)
     delegated_frame_host_->UnregisterFrameSinkHierarchy();
+  DCHECK(!begin_frame_source_);
 }
 
-void RenderWidgetHostViewAndroid::SendBeginFrame(base::TimeTicks frame_time,
-                                                 base::TimeDelta vsync_period)
{
+void RenderWidgetHostViewAndroid::SendBeginFrame(cc::BeginFrameArgs args) {
   TRACE_EVENT1("cc", "RenderWidgetHostViewAndroid::SendBeginFrame",
-               "frame_time_us", frame_time.ToInternalValue());
+               "frame_time_us", args.frame_time.ToInternalValue());
 
   // Synchronous compositor does not use deadline-based scheduling.
   // TODO(brianderson): Replace this hardcoded deadline after Android
-  // switches to Surfaces and the Browser's commit isn't in the critcal path.
-  base::TimeTicks deadline =
-      sync_compositor_ ? base::TimeTicks() : frame_time + (vsync_period * 0.6);
-  host_->Send(new ViewMsg_BeginFrame(
-      host_->GetRoutingID(),
-      cc::BeginFrameArgs::Create(BEGINFRAME_FROM_HERE, frame_time, deadline,
-                                 vsync_period, cc::BeginFrameArgs::NORMAL)));
+  // switches to Surfaces and the Browser's commit isn't in the critical path.
+  args.deadline = sync_compositor_ ? base::TimeTicks()
+  : args.frame_time + (args.interval * 0.6);
+  host_->Send(new ViewMsg_BeginFrame(host_->GetRoutingID(), args));
   if (sync_compositor_)
     sync_compositor_->DidSendBeginFrame(view_.GetWindowAndroid());
 }
@@ -1625,7 +1634,7 @@ InputEventAckState
RenderWidgetHostViewAndroid::FilterInputEvent(
 
 void RenderWidgetHostViewAndroid::OnSetNeedsFlushInput() {
   TRACE_EVENT0("input", "RenderWidgetHostViewAndroid::OnSetNeedsFlushInput");
-  RequestVSyncUpdate(FLUSH_INPUT);
+  AddBeginFrameRequest(FLUSH_INPUT);
 }
 
 BrowserAccessibilityManager*
@@ -1807,7 +1816,7 @@ void RenderWidgetHostViewAndroid::SetContentViewCore(
     return;
   }
 
-  if (is_showing_)
+  if (is_showing_ && view_.GetWindowAndroid())
     StartObservingRootWindow();
 
   if (resize)
@@ -1883,10 +1892,12 @@ void RenderWidgetHostViewAndroid::OnDetachedFromWindow()
{
 void RenderWidgetHostViewAndroid::OnAttachCompositor() {
   DCHECK(content_view_core_);
   CreateOverscrollControllerIfPossible();
-  ui::WindowAndroidCompositor* compositor =
-      view_.GetWindowAndroid()->GetCompositor();
-  delegated_frame_host_->RegisterFrameSinkHierarchy(
-      compositor->GetFrameSinkId());
+  if (observing_root_window_) {
+    ui::WindowAndroidCompositor* compositor =
+        view_.GetWindowAndroid()->GetCompositor();
+    delegated_frame_host_->RegisterFrameSinkHierarchy(
+        compositor->GetFrameSinkId());
+  }
 }
 
 void RenderWidgetHostViewAndroid::OnDetachCompositor() {
@@ -1897…
(message too large)

Eric Seckler

The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, brianderson@chromium.org, skyostil@chromium.org, boliu@chromium.org ...

4 years ago (2016-12-20 19:13:26 UTC) #82

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2564403002/250001

4 years ago (2016-12-20 19:13:55 UTC) #83

commit-bot: I haz the power

CQ is committing da patch. Bot data: {"patchset_id": 250001, "attempt_start_ts": 1482261206136920, "parent_rev": "7cf588ab111bf146c9ab7a3d16efcecf9901ef4c", "commit_rev": "b1270640d88a21e686b29c305ae64149a9bf8ab5"}

4 years ago (2016-12-20 20:22:26 UTC) #84

commit-bot: I haz the power

Description was changed from ========== [android] Make RWHVAndroid a BeginFrameObserver. Instead of directly observing the ...

4 years ago (2016-12-20 20:22:55 UTC) #85

commit-bot: I haz the power

Description was changed from ========== [android] Make RWHVAndroid a BeginFrameObserver. Instead of directly observing the ...

4 years ago (2016-12-20 20:26:23 UTC) #87

commit-bot: I haz the power

4 years ago (2016-12-20 20:26:24 UTC) #88

Message was sent while issue was closed.

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

Issue 2564403002: [android] Make RWHVAndroid a BeginFrameObserver. (Closed)

Description

Patch Set 1 : . #

Patch Set 2 : Address Bo's comments. #

Patch Set 3 : Address Bo's comments pt2. #

Patch Set 4 : Fix ordering in Add/ClearBeginFrameRequest. #

Patch Set 5 : fix crbug.com/675289 #

Patch Set 6 : fix comment #

Patch Set 7 : add todo #

Patch Set 8 : rebase #

Messages