Enable BeginFrame scheduling on aura

Enable BeginFrame scheduling on aura

This cl is last partial cl from https://codereview.chromium.org/775143003/
for easy review.
Until now, BeginFrame is initiated by each renderer process.
With this cl(with --enable-begin-frame-scheduling), BeginFrame is scheduled from parent(browser) cc scheduler
to child(renderer) cc scheduler.
For more detailed information, please see this (http://goo.gl/D1Qxrr)

R=brianderson@chromium.org, danakj@chromium.org, mithro@mithis.com
BUG=372086
Committed: https://crrev.com/17dd2f109f3155ebf183627b63df25f97f34b67f
Cr-Commit-Position: refs/heads/master@{#322622}

Committed: https://crrev.com/62a759e582743bca13f27d1ce5db3dc2fe66fbb6
Cr-Commit-Position: refs/heads/master@{#328170}

Committed: https://crrev.com/047d61bf35b1142e2fb58ac0fca463bccefb4d74
Cr-Commit-Position: refs/heads/master@{#331050}

[simonhong, 2015-03-23 21:45:25]

I need to make this code run only on aura not ash.
I tried to use #if defined(USE_AURA) && !defined(USE_ASH). but it seems not
work.
Except that, this code works well.
PTAL without this flag setting.

[simonhong, 2015-03-23 22:21:52]

On 2015/03/23 21:45:25, simonhong wrote:
> I need to make this code run only on aura not ash.
> I tried to use #if defined(USE_AURA) && !defined(USE_ASH). but it seems not
> work.
> Except that, this code works well.
> PTAL without this flag setting.

brian, I think I should include removing CompositorVSyncManager in this cl
because we can't check ash is used or not in content layer.
CompositorVSyncManager is used by ash.

[danakj, 2015-03-24 17:46:24]

https://codereview.chromium.org/1016033006/diff/60001/content/browser/composi...
File content/browser/compositor/delegated_frame_host.cc (right):

https://codereview.chromium.org/1016033006/diff/60001/content/browser/composi...
content/browser/compositor/delegated_frame_host.cc:983: #if 0
I'm not sure what is going on in this CL sorry. You should remove code not if 0
it.

I don't understand why you want to remove things on ash either. I need some
explanations..

[simonhong, 2015-03-24 21:07:30]

On 2015/03/24 17:46:24, danakj wrote:
>
https://codereview.chromium.org/1016033006/diff/60001/content/browser/composi...
> File content/browser/compositor/delegated_frame_host.cc (right):
> 
>
https://codereview.chromium.org/1016033006/diff/60001/content/browser/composi...
> content/browser/compositor/delegated_frame_host.cc:983: #if 0
> I'm not sure what is going on in this CL sorry. You should remove code not if
0
> it.
> 
> I don't understand why you want to remove things on ash either. I need some
> explanations..

dana, sorry for this  incomplete cl.
I meant to complete this cl after landing
https://codereview.chromium.org/1005553004/.
After that, all remained code will be included in this cl.

[danakj, 2015-03-24 21:08:18]

OK I'll wait for reviewing then.

On Tue, Mar 24, 2015 at 2:07 PM, <simonhong@chromium.org> wrote:

> On 2015/03/24 17:46:24, danakj wrote:
>
> https://codereview.chromium.org/1016033006/diff/60001/
> content/browser/compositor/delegated_frame_host.cc
>
>> File content/browser/compositor/delegated_frame_host.cc (right):
>>
>
>
> https://codereview.chromium.org/1016033006/diff/60001/
> content/browser/compositor/delegated_frame_host.cc#newcode983
>
>> content/browser/compositor/delegated_frame_host.cc:983: #if 0
>> I'm not sure what is going on in this CL sorry. You should remove code
>> not if
>>
> 0
>
>> it.
>>
>
>  I don't understand why you want to remove things on ash either. I need
>> some
>> explanations..
>>
>
> dana, sorry for this  incomplete cl.
> I meant to complete this cl after landing
> https://codereview.chromium.org/1005553004/.
> After that, all remained code will be included in this cl.
>
> https://codereview.chromium.org/1016033006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[simonhong, 2015-03-25 17:06:47]

simonhong@chromium.org changed reviewers:
+ mithro@mithis.com

[simonhong, 2015-03-25 17:06:48]

This is last partial patch for enabling BeginFrame scheduling on aura.
PTAL!

[danakj, 2015-03-25 17:40:38]

https://codereview.chromium.org/1016033006/diff/100001/content/browser/compos...
File content/browser/compositor/browser_compositor_output_surface.cc (right):

https://codereview.chromium.org/1016033006/diff/100001/content/browser/compos...
content/browser/compositor/browser_compositor_output_surface.cc:40: bool
BrowserCompositorOutputSurface::BindToClient(
You can remove this function override then

https://codereview.chromium.org/1016033006/diff/100001/content/browser/compos...
File content/browser/compositor/delegated_frame_host.cc (right):

https://codereview.chromium.org/1016033006/diff/100001/content/browser/compos...
content/browser/compositor/delegated_frame_host.cc:908: void
DelegatedFrameHost::UpdateVSyncParameters(
s/Update/Set/ ?

https://codereview.chromium.org/1016033006/diff/100001/content/browser/compos...
File content/browser/compositor/reflector_impl_unittest.cc (right):

https://codereview.chromium.org/1016033006/diff/100001/content/browser/compos...
content/browser/compositor/reflector_impl_unittest.cc:84: new
TestOutputSurface(context_provider_)).Pass();
(the .Pass() is redundant and not needed here. "new Foo()" doesn't have a
variable name so it's an rvalue already)

https://codereview.chromium.org/1016033006/diff/100001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_mac.mm (left):

https://codereview.chromium.org/1016033006/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac.mm:443: void
RenderWidgetHostViewMac::DelegatedFrameHostUpdateVSyncParameters(
So what's the expected outcome on Mac for this patch?

https://codereview.chromium.org/1016033006/diff/100001/content/renderer/gpu/r...
File content/renderer/gpu/render_widget_compositor.cc (left):

https://codereview.chromium.org/1016033006/diff/100001/content/renderer/gpu/r...
content/renderer/gpu/render_widget_compositor.cc:205:
cmd->HasSwitch(switches::kEnableBeginFrameScheduling);
no longer used then? shall we remove the switch?

[danakj, 2015-03-25 17:41:15]

> Enable BeginFrame scheduling on aura

Can you give some explanation of what this means in the CL description for
sheriffs/bystanders/etc?

[simonhong, 2015-03-25 18:05:22]

All comments are addressed!

https://codereview.chromium.org/1016033006/diff/100001/content/browser/compos...
File content/browser/compositor/browser_compositor_output_surface.cc (right):

https://codereview.chromium.org/1016033006/diff/100001/content/browser/compos...
content/browser/compositor/browser_compositor_output_surface.cc:40: bool
BrowserCompositorOutputSurface::BindToClient(
On 2015/03/25 17:40:38, danakj wrote:
> You can remove this function override then

Done.

https://codereview.chromium.org/1016033006/diff/100001/content/browser/compos...
File content/browser/compositor/delegated_frame_host.cc (right):

https://codereview.chromium.org/1016033006/diff/100001/content/browser/compos...
content/browser/compositor/delegated_frame_host.cc:908: void
DelegatedFrameHost::UpdateVSyncParameters(
On 2015/03/25 17:40:38, danakj wrote:
> s/Update/Set/ ?

Done.

https://codereview.chromium.org/1016033006/diff/100001/content/browser/compos...
File content/browser/compositor/reflector_impl_unittest.cc (right):

https://codereview.chromium.org/1016033006/diff/100001/content/browser/compos...
content/browser/compositor/reflector_impl_unittest.cc:84: new
TestOutputSurface(context_provider_)).Pass();
On 2015/03/25 17:40:38, danakj wrote:
> (the .Pass() is redundant and not needed here. "new Foo()" doesn't have a
> variable name so it's an rvalue already)

Done.

https://codereview.chromium.org/1016033006/diff/100001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_mac.mm (left):

https://codereview.chromium.org/1016033006/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac.mm:443: void
RenderWidgetHostViewMac::DelegatedFrameHostUpdateVSyncParameters(
On 2015/03/25 17:40:38, danakj wrote:
> So what's the expected outcome on Mac for this patch?

So far, this function was never called because Mac doesn't get vsync from gpu.
So, we can delete this.

https://codereview.chromium.org/1016033006/diff/100001/content/renderer/gpu/r...
File content/renderer/gpu/render_widget_compositor.cc (left):

https://codereview.chromium.org/1016033006/diff/100001/content/renderer/gpu/r...
content/renderer/gpu/render_widget_compositor.cc:205:
cmd->HasSwitch(switches::kEnableBeginFrameScheduling);
On 2015/03/25 17:40:38, danakj wrote:
> no longer used then? shall we remove the switch?

Yep. removed.

[danakj, 2015-03-25 18:09:50]

LGTM

[simonhong, 2015-03-25 18:24:03]

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

[simonhong, 2015-03-25 18:26:23]

simonhong@chromium.org changed reviewers:
+ oshima@chromium.org

[simonhong, 2015-03-25 18:28:15]

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

[simonhong, 2015-03-25 18:30:07]

@boliu: android_webview/*
@oshima: ash/display/*, chrome/browser/*
@sievers: content/browser/android/*, content/browser/renderer_host/*

PTAL!

[brianderson, 2015-03-25 20:25:54]

Two nits, then lgtm.

So much deleted code!

https://codereview.chromium.org/1016033006/diff/120001/content/browser/compos...
File content/browser/compositor/delegated_frame_host.h (right):

https://codereview.chromium.org/1016033006/diff/120001/content/browser/compos...
content/browser/compositor/delegated_frame_host.h:96: void
SetVSyncParameters(base::TimeTicks timebase, base::TimeDelta interval);
Can we keep this UpdateVSyncParameters to keep it consistent with the rest of
the code and rename it everywhere as a separate patch all at once?

https://codereview.chromium.org/1016033006/diff/120001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.h (right):

https://codereview.chromium.org/1016033006/diff/120001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.h:660:
scoped_ptr<BeginFrameObserverProxy> begin_frame_observer_proxy_;
Can this be a straight up BeginFrameObserverProxy rather than a scoped_ptr?

[boliu, 2015-03-25 20:29:08]

android_webview rs lgtm

[danakj, 2015-03-25 20:31:19]

https://codereview.chromium.org/1016033006/diff/120001/content/browser/compos...
File content/browser/compositor/delegated_frame_host.h (right):

https://codereview.chromium.org/1016033006/diff/120001/content/browser/compos...
content/browser/compositor/delegated_frame_host.h:96: void
SetVSyncParameters(base::TimeTicks timebase, base::TimeDelta interval);
On 2015/03/25 20:25:54, brianderson wrote:
> Can we keep this UpdateVSyncParameters to keep it consistent with the rest of
> the code and rename it everywhere as a separate patch all at once?

What other places are you thinking of? Update sounds like it's doing work, but
this is a simple setter. Maybe other places do things with these parameters?

[brianderson, 2015-03-25 21:00:40]

On 2015/03/25 20:31:19, danakj wrote:
>
https://codereview.chromium.org/1016033006/diff/120001/content/browser/compos...
> File content/browser/compositor/delegated_frame_host.h (right):
> 
>
https://codereview.chromium.org/1016033006/diff/120001/content/browser/compos...
> content/browser/compositor/delegated_frame_host.h:96: void
> SetVSyncParameters(base::TimeTicks timebase, base::TimeDelta interval);
> On 2015/03/25 20:25:54, brianderson wrote:
> > Can we keep this UpdateVSyncParameters to keep it consistent with the rest
of
> > the code and rename it everywhere as a separate patch all at once?
> 
> What other places are you thinking of? Update sounds like it's doing work, but
> this is a simple setter. Maybe other places do things with these parameters?

Ok, let's keep SetVSyncParameters as Dana suggested since it is a simple setter
and the style guide suggests prefixing simple setters with "set".

To me, "Update" implied that these are the latest vsync parameters, but you
should expect better ones to come soon. We don't get much from extending that
concept into the simple setters though.

[oshima, 2015-03-25 22:42:41]

ash/display, chrome/browser/chromeos lgtm.

[simonhong, 2015-03-25 23:21:49]

https://codereview.chromium.org/1016033006/diff/120001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.h (right):

https://codereview.chromium.org/1016033006/diff/120001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.h:660:
scoped_ptr<BeginFrameObserverProxy> begin_frame_observer_proxy_;
On 2015/03/25 20:25:54, brianderson wrote:
> Can this be a straight up BeginFrameObserverProxy rather than a scoped_ptr?

Yep, scoped_ptr is not needed in this case because it is only used by RWHVAura
and
its life cycle is also tied to RWHV.
Changed.

[no sievers, 2015-03-26 20:01:03]

content lgtm

[simonhong, 2015-03-26 22:48:32]

The CQ bit was checked by simonhong@chromium.org

[simonhong, 2015-03-26 22:48:33]

The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org,
brianderson@chromium.org, boliu@chromium.org, danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/1016033006/#ps140001
(title: " ")

[commit-bot: I haz the power, 2015-03-26 22:49:00]

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

[simonhong, 2015-03-27 02:27:31]

The CQ bit was unchecked by simonhong@chromium.org

[simonhong, 2015-03-27 15:44:06]

@danakj, compositor is destroyed earlier than RWHV  in some browser tests.
For more strict observer managing, BeginFrameObserverProxy overrided
CompositorObserver to remove observation from compositor by using
OnCompositingShuttingDown().
PTAL agian!

[danakj, 2015-03-27 16:25:24]

> @danakj, compositor is destroyed earlier than RWHV  in some browser tests.

How many tests/which ones? Really we should have consistent shutdown ordering..
Can you maybe leave a TODO to fix this shutdown weirdness and remove the
CompositorObserver?

https://codereview.chromium.org/1016033006/diff/150001/content/browser/render...
File content/browser/renderer_host/begin_frame_observer_proxy.cc (right):

https://codereview.chromium.org/1016033006/diff/150001/content/browser/render...
content/browser/renderer_host/begin_frame_observer_proxy.cc:50:
compositor_->RemoveObserver(this);
You need to remove observer on destruction yes? Should destructor call
ResetCompositor?

I guess this applies to StopObservingBeginFrames too.

[simonhong, 2015-03-27 17:37:55]

PTAL again.
There are many failed tests in browser_tests.
You can check it in
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_....

https://codereview.chromium.org/1016033006/diff/150001/content/browser/render...
File content/browser/renderer_host/begin_frame_observer_proxy.cc (right):

https://codereview.chromium.org/1016033006/diff/150001/content/browser/render...
content/browser/renderer_host/begin_frame_observer_proxy.cc:50:
compositor_->RemoveObserver(this);
On 2015/03/27 16:25:24, danakj wrote:
> You need to remove observer on destruction yes? Should destructor call
> ResetCompositor?
Right, we need to call ResetCompositor() in dtor.
> 
> I guess this applies to StopObservingBeginFrames too.
I think handling this is sufficient when dtor is called and window is removed
from root window.

https://codereview.chromium.org/1016033006/diff/170001/content/browser/render...
File content/browser/renderer_host/begin_frame_observer_proxy.cc (right):

https://codereview.chromium.org/1016033006/diff/170001/content/browser/render...
content/browser/renderer_host/begin_frame_observer_proxy.cc:40: if (!compositor)
I found that when RWHV is destroyed it calls SetCompositor() with null pointer.
So, I added this like DelegatedFrameHost does.

[danakj, 2015-03-27 17:41:47]

https://codereview.chromium.org/1016033006/diff/170001/content/browser/render...
File content/browser/renderer_host/begin_frame_observer_proxy.cc (right):

https://codereview.chromium.org/1016033006/diff/170001/content/browser/render...
content/browser/renderer_host/begin_frame_observer_proxy.cc:40: if (!compositor)
On 2015/03/27 17:37:54, simonhong wrote:
> I found that when RWHV is destroyed it calls SetCompositor() with null
pointer.
> So, I added this like DelegatedFrameHost does.

I don't think we should have SetCompositor(nullptr) and ResetCompositor. We
should change it to call ResetCompositor (for both cases).

[simonhong, 2015-03-27 17:53:09]

On 2015/03/27 17:41:47, danakj wrote:
>
https://codereview.chromium.org/1016033006/diff/170001/content/browser/render...
> File content/browser/renderer_host/begin_frame_observer_proxy.cc (right):
> 
>
https://codereview.chromium.org/1016033006/diff/170001/content/browser/render...
> content/browser/renderer_host/begin_frame_observer_proxy.cc:40: if
(!compositor)
> On 2015/03/27 17:37:54, simonhong wrote:
> > I found that when RWHV is destroyed it calls SetCompositor() with null
> pointer.
> > So, I added this like DelegatedFrameHost does.
> 
> I don't think we should have SetCompositor(nullptr) and ResetCompositor. We
> should change it to call ResetCompositor (for both cases).

Ok, I reverted and RenderWidgetHostViewAura::AddedToRootWindow() is changed to
call SetCompositor()
when compositor() is not null.

[danakj, 2015-03-27 18:03:24]

LGTM

https://codereview.chromium.org/1016033006/diff/190001/content/browser/render...
File content/browser/renderer_host/begin_frame_observer_proxy.cc (right):

https://codereview.chromium.org/1016033006/diff/190001/content/browser/render...
content/browser/renderer_host/begin_frame_observer_proxy.cc:17:
ResetCompositor();
Oh.. there was a dcheck ensuring ResetCompositor was already called. If that
DCHECK was not firing, then leaving it as it was would be fine! Sorry I mis-read
that.

https://codereview.chromium.org/1016033006/diff/190001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/1016033006/diff/190001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:2592:
delegated_frame_host_->SetCompositor(window_->GetHost()->compositor());
Would you mind moving this into the if and remove the early out in DFH in
another CL too?

[simonhong, 2015-03-27 18:15:07]

Thanks for review, dana!

https://codereview.chromium.org/1016033006/diff/190001/content/browser/render...
File content/browser/renderer_host/begin_frame_observer_proxy.cc (right):

https://codereview.chromium.org/1016033006/diff/190001/content/browser/render...
content/browser/renderer_host/begin_frame_observer_proxy.cc:17:
ResetCompositor();
On 2015/03/27 18:03:24, danakj wrote:
> Oh.. there was a dcheck ensuring ResetCompositor was already called. If that
> DCHECK was not firing, then leaving it as it was would be fine! Sorry I
mis-read
> that.

Done.

https://codereview.chromium.org/1016033006/diff/190001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/1016033006/diff/190001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:2592:
delegated_frame_host_->SetCompositor(window_->GetHost()->compositor());
On 2015/03/27 18:03:24, danakj wrote:
> Would you mind moving this into the if and remove the early out in DFH in
> another CL too?

Ok, I will. :)

[simonhong, 2015-03-27 18:19:37]

The CQ bit was checked by simonhong@chromium.org

[simonhong, 2015-03-27 18:19:38]

The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org,
brianderson@chromium.org, boliu@chromium.org, sievers@chromium.org,
danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/1016033006/#ps210001
(title: " ")

[commit-bot: I haz the power, 2015-03-27 18:19:58]

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

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

Committed patchset #12 (id:210001)

[commit-bot: I haz the power, 2015-03-27 19:26:30]

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

[danakj, 2015-03-27 21:36:34]

There's a number of bots that got flaky/failures at the time of this CL. I think
we should revert to see if that fixes them:

https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%...
https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%...

[danakj, 2015-03-27 21:37:07]

On 2015/03/27 21:36:34, danakj wrote:
> There's a number of bots that got flaky/failures at the time of this CL. I
think
> we should revert to see if that fixes them:
> 
>
https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%...
>
https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%...

https://build.chromium.org/p/chromium.win/builders/Vista%20Tests%20%281%29/bu...

Messed up that paste.

[danakj, 2015-03-27 21:40:32]

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

The reason for reverting is: Bots are failing:
AutofillInteractiveTest.AutofillFormWithNonAutofillableField
WebViewTest.InterstitialTeardown
TouchExplorationTest.RewritesEventsWhenOn

Not sure if it's this CL but reverting to see..

[danakj, 2015-03-27 22:02:40]

Also
https://build.chromium.org/p/chromium.win/builders/XP%20Tests%20%281%29/build...

[lazyboy, 2015-03-27 22:45:46]

On 2015/03/27 22:02:40, danakj wrote:
> Also
>
https://build.chromium.org/p/chromium.win/builders/XP%20Tests%20%281%29/build...

FYI, this also breaks out-of-process iframe rendering. iframes stay white with
--site-per-process flag.

[danakj, 2015-03-27 23:09:27]

On Fri, Mar 27, 2015 at 3:45 PM, <lazyboy@chromium.org> wrote:

> On 2015/03/27 22:02:40, danakj wrote:
>
>> Also
>>
>
> https://build.chromium.org/p/chromium.win/builders/XP%
> 20Tests%20%281%29/builds/36571/steps/telemetry_perf_unittests/logs/stdio
>
> FYI, this also breaks out-of-process iframe rendering. iframes stay white
> with
> --site-per-process flag.
>

Are there any tests for this?

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[brianderson, 2015-03-27 23:28:35]

On 2015/03/27 22:45:46, lazyboy wrote:
> On 2015/03/27 22:02:40, danakj wrote:
> > Also
> >
>
https://build.chromium.org/p/chromium.win/builders/XP%20Tests%20%281%29/build...
> 
> FYI, this also breaks out-of-process iframe rendering. iframes stay white with
> --site-per-process flag.

Opened https://code.google.com/p/chromium/issues/detail?id=471411 figure this
out.

+1 to danakj's comment for tests.

[mithro-old, 2015-04-08 05:50:05]

A possible approach is to not enable external begin frame messages for OOPIF and
then land a subsequent patch to fix them. Dana / Brain - what do you think about
doing that?

After chatting with kenrb@ about OOPIF we both don't know the exact right
approach for getting the BeginFrame messages to the OOPIF but had some ideas on
what to be looking at.

Naively we want the BeginFrame message to be received by the outer frame and
then forwarded to it's children (with any deadline adjustments needed). However,
with Surfaces this seems like we could just send the BeginFrame directly but I'm
still very fuzzy on how Surfaces and OOPIF interact.

A couple more thoughts;

 * The CompositorFrameSwappedACK should be a good example of a Compositor->Frame
message we can follow for the "SendBeginFrame" part.

 * The OnSwapCompositorFrame should be a good example of a Frame->Compositor
message we can follow for the "SetNeedsBeginFrame" part.

 * I'm currently thinking that the OnSetNeedsBeginFrames and SendBeginFrame
should probably be virtual methods on either RenderWidgetHostView or
RenderWidgetHostViewBase. Is there an OWNER of that interface we should be
chatting to about the right level these should be added?

[brianderson, 2015-04-09 20:44:56]

On 2015/04/08 05:50:05, mithro wrote:
> A possible approach is to not enable external begin frame messages for OOPIF
and
> then land a subsequent patch to fix them. Dana / Brain - what do you think
about
> doing that?

That sounds like a good plan if doing OOPIF correctly is going to take a while.
Then we can figure out OOPIF in parallel with other work that depends on this
patch.

> 
> After chatting with kenrb@ about OOPIF we both don't know the exact right
> approach for getting the BeginFrame messages to the OOPIF but had some ideas
on
> what to be looking at.
> 
> Naively we want the BeginFrame message to be received by the outer frame and
> then forwarded to it's children (with any deadline adjustments needed).
However,
> with Surfaces this seems like we could just send the BeginFrame directly but
I'm
> still very fuzzy on how Surfaces and OOPIF interact.

My understanding is that Surfaces will enable OOPIF to swap directly without
having to go through its embedder. I'm not sure if we are at that point yet, but
we shouldn't waste our time supporting the old way of doing things. Let's just
send the BeginFrame directly. If the deadline is a problem, we can adjust it or
prioritize getting OOPIF to swap directly.

> 
> A couple more thoughts;
> 
>  * The CompositorFrameSwappedACK should be a good example of a
Compositor->Frame
> message we can follow for the "SendBeginFrame" part.
> 
>  * The OnSwapCompositorFrame should be a good example of a Frame->Compositor
> message we can follow for the "SetNeedsBeginFrame" part.
> 
>  * I'm currently thinking that the OnSetNeedsBeginFrames and SendBeginFrame
> should probably be virtual methods on either RenderWidgetHostView or
> RenderWidgetHostViewBase. Is there an OWNER of that interface we should be
> chatting to about the right level these should be added?

Sounds good. I've copied your comments to the OOPIF bug. Let's follow up there.

[simonhong, 2015-04-09 23:11:55]

On Fri, Apr 10, 2015 at 5:44 AM, <brianderson@chromium.org> wrote:

> On 2015/04/08 05:50:05, mithro wrote:
>
>> A possible approach is to not enable external begin frame messages for
>> OOPIF
>>
> and
>
>> then land a subsequent patch to fix them. Dana / Brain - what do you think
>>
> about
>
>> doing that?
>>
>
> That sounds like a good plan if doing OOPIF correctly is going to take a
> while.
> Then we can figure out OOPIF in parallel with other work that depends on
> this
> patch.
>
> Yep, that way could be a good work-around if we can easily know whether
that renderer is for child frame or not.

>
>  After chatting with kenrb@ about OOPIF we both don't know the exact right
>> approach for getting the BeginFrame messages to the OOPIF but had some
>> ideas
>>
> on
>
>> what to be looking at.
>>
>
>  Naively we want the BeginFrame message to be received by the outer frame
>> and
>> then forwarded to it's children (with any deadline adjustments needed).
>>
> However,
>
>> with Surfaces this seems like we could just send the BeginFrame directly
>> but
>>
> I'm
>
>> still very fuzzy on how Surfaces and OOPIF interact.
>>
>
> My understanding is that Surfaces will enable OOPIF to swap directly
> without
> having to go through its embedder. I'm not sure if we are at that point
> yet, but
> we shouldn't waste our time supporting the old way of doing things. Let's
> just
> send the BeginFrame directly. If the deadline is a problem, we can adjust
> it or
> prioritize getting OOPIF to swap directly.
>
>
>  A couple more thoughts;
>>
>
>    * The CompositorFrameSwappedACK should be a good example of a
>>
> Compositor->Frame
>
>> message we can follow for the "SendBeginFrame" part.
>>
>
>    * The OnSwapCompositorFrame should be a good example of a
>> Frame->Compositor
>> message we can follow for the "SetNeedsBeginFrame" part.
>>
>
>    * I'm currently thinking that the OnSetNeedsBeginFrames and
>> SendBeginFrame
>> should probably be virtual methods on either RenderWidgetHostView or
>> RenderWidgetHostViewBase. Is there an OWNER of that interface we should be
>> chatting to about the right level these should be added?
>>
>
> Sounds good. I've copied your comments to the OOPIF bug. Let's follow up
> there.
>

> https://codereview.chromium.org/1016033006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[simonhong, 2015-04-28 13:51:04]

Rebased on master.
Could I try to land this again?

[brianderson, 2015-04-28 22:58:55]

On 2015/04/28 at 13:51:04, simonhong wrote:
> Rebased on master.
> Could I try to land this again?

Yeah, let's try landing this again. Thanks for rebasing.

Dana pointed out that the following tests were flaking after this patch:
1) AutofillInteractiveTest.AutofillFormWithNonAutofillableField
2) WebViewTest.InterstitialTeardown
3) TouchExplorationTest.RewritesEventsWhenOn

2 is OOPIF related.
3 has since been disabled for being flaky in general: crbug.com/469119.
The only one I'm not sure about is 1, but maybe we can re-land and see if it
starts flaking again?

[danakj, 2015-04-28 23:00:28]

On Tue, Apr 28, 2015 at 3:58 PM, <brianderson@chromium.org> wrote:

> On 2015/04/28 at 13:51:04, simonhong wrote:
>
>> Rebased on master.
>> Could I try to land this again?
>>
>
> Yeah, let's try landing this again. Thanks for rebasing.
>
> Dana pointed out that the following tests were flaking after this patch:
> 1) AutofillInteractiveTest.AutofillFormWithNonAutofillableField
> 2) WebViewTest.InterstitialTeardown
> 3) TouchExplorationTest.RewritesEventsWhenOn
>
> 2 is OOPIF related.
> 3 has since been disabled for being flaky in general: crbug.com/469119.
> The only one I'm not sure about is 1, but maybe we can re-land and see if
> it
> starts flaking again?
>

Have you tried repro locally?


>
>
>
> https://codereview.chromium.org/1016033006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[simonhong, 2015-04-28 23:41:17]

Ah.. I didn't checked that test locally.
But, I found that it was passes in the above latest trybot result.
2015. 4. 29. 오전 8:08에 "Dana Jansens" <danakj@chromium.org>님이 작성:

> On Tue, Apr 28, 2015 at 3:58 PM, <brianderson@chromium.org> wrote:
>
>> On 2015/04/28 at 13:51:04, simonhong wrote:
>>
>>> Rebased on master.
>>> Could I try to land this again?
>>>
>>
>> Yeah, let's try landing this again. Thanks for rebasing.
>>
>> Dana pointed out that the following tests were flaking after this patch:
>> 1) AutofillInteractiveTest.AutofillFormWithNonAutofillableField
>> 2) WebViewTest.InterstitialTeardown
>> 3) TouchExplorationTest.RewritesEventsWhenOn
>>
>> 2 is OOPIF related.
>> 3 has since been disabled for being flaky in general: crbug.com/469119.
>> The only one I'm not sure about is 1, but maybe we can re-land and see if
>> it
>> starts flaking again?
>>
>
> Have you tried repro locally?
>
>
>>
>>
>>
>> https://codereview.chromium.org/1016033006/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2015-04-29 19:34:13]

On Tue, Apr 28, 2015 at 4:41 PM, Simon Hong <simonhong@chromium.org> wrote:

> Ah.. I didn't checked that test locally.
> But, I found that it was passes in the above latest trybot result.
>

If it's flaky that doesn't mean a whole lot. Try run locally on repeat many
times?


> 2015. 4. 29. 오전 8:08에 "Dana Jansens" <danakj@chromium.org>님이 작성:
>
> On Tue, Apr 28, 2015 at 3:58 PM, <brianderson@chromium.org> wrote:
>>
>>> On 2015/04/28 at 13:51:04, simonhong wrote:
>>>
>>>> Rebased on master.
>>>> Could I try to land this again?
>>>>
>>>
>>> Yeah, let's try landing this again. Thanks for rebasing.
>>>
>>> Dana pointed out that the following tests were flaking after this patch:
>>> 1) AutofillInteractiveTest.AutofillFormWithNonAutofillableField
>>> 2) WebViewTest.InterstitialTeardown
>>> 3) TouchExplorationTest.RewritesEventsWhenOn
>>>
>>> 2 is OOPIF related.
>>> 3 has since been disabled for being flaky in general: crbug.com/469119.
>>> The only one I'm not sure about is 1, but maybe we can re-land and see
>>> if it
>>> starts flaking again?
>>>
>>
>> Have you tried repro locally?
>>
>>
>>>
>>>
>>>
>>> https://codereview.chromium.org/1016033006/
>>>
>>
>>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[simonhong, 2015-04-30 12:37:41]

On 2015/04/29 19:34:13, danakj wrote:
> On Tue, Apr 28, 2015 at 4:41 PM, Simon Hong <mailto:simonhong@chromium.org>
wrote:
> 
> > Ah.. I didn't checked that test locally.
> > But, I found that it was passes in the above latest trybot result.
> >
> 
> If it's flaky that doesn't mean a whole lot. Try run locally on repeat many
> times?

@danakj, Sorry for late response.
More than 100 local repeated test were passed. ;)
> 
> 
> > 2015. 4. 29. 오전 8:08에 "Dana Jansens"
<mailto:danakj@chromium.xn--org>-om0qi30h 작성:
> >
> > On Tue, Apr 28, 2015 at 3:58 PM, <mailto:brianderson@chromium.org> wrote:
> >>
> >>> On 2015/04/28 at 13:51:04, simonhong wrote:
> >>>
> >>>> Rebased on master.
> >>>> Could I try to land this again?
> >>>>
> >>>
> >>> Yeah, let's try landing this again. Thanks for rebasing.
> >>>
> >>> Dana pointed out that the following tests were flaking after this patch:
> >>> 1) AutofillInteractiveTest.AutofillFormWithNonAutofillableField
> >>> 2) WebViewTest.InterstitialTeardown
> >>> 3) TouchExplorationTest.RewritesEventsWhenOn
> >>>
> >>> 2 is OOPIF related.
> >>> 3 has since been disabled for being flaky in general: crbug.com/469119.
> >>> The only one I'm not sure about is 1, but maybe we can re-land and see
> >>> if it
> >>> starts flaking again?
> >>>
> >>
> >> Have you tried repro locally?
> >>
> >>
> >>>
> >>>
> >>>
> >>> https://codereview.chromium.org/1016033006/
> >>>
> >>
> >>
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[simonhong, 2015-04-30 21:15:41]

The CQ bit was checked by simonhong@chromium.org

[simonhong, 2015-04-30 21:15:42]

The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org,
brianderson@chromium.org, boliu@chromium.org, danakj@chromium.org,
sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1016033006/#ps250001
(title: " ")

[commit-bot: I haz the power, 2015-04-30 21:17:33]

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

[brianderson, 2015-04-30 21:17:37]

The CQ bit was unchecked by brianderson@chromium.org

[brianderson, 2015-04-30 21:21:13]

Feature freeze for M43 is tomorrow. To avoid throwing a wrench into anyone
trying to cherry-pick changes for M43, should we hold off on trying to land this
until next week since it's risky and not needed for M43?

[simonhong, 2015-04-30 21:30:36]

ah! It's good plan.
2015. 5. 1. 오전 6:21에 <brianderson@chromium.org>님이 작성:

> Feature freeze for M43 is tomorrow. To avoid throwing a wrench into anyone
> trying to cherry-pick changes for M43, should we hold off on trying to
> land this
> until next week since it's risky and not needed for M43?
>
> https://codereview.chromium.org/1016033006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[brianderson, 2015-05-01 22:58:06]

On 2015/04/30 21:30:36, simonhong wrote:
> ah! It's good plan.
> 2015. 5. 1. 오전 6:21에 <mailto:brianderson@chromium.xn--org>-om0qi30h 작성:
> 
> > Feature freeze for M43 is tomorrow. To avoid throwing a wrench into anyone
> > trying to cherry-pick changes for M43, should we hold off on trying to
> > land this
> > until next week since it's risky and not needed for M43?
> >
> > https://codereview.chromium.org/1016033006/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Hmm. Seems I was confused. I was concerned that this patch might interfere with
patches (related to video and DisplayScheduler) that I thought we might merge
into M43. Turns out we aren't going to cherry-pick either of those - so let's
try landing this on Monday when we're all back.

[simonhong, 2015-05-04 19:44:10]

On 2015/05/01 22:58:06, brianderson wrote:
> On 2015/04/30 21:30:36, simonhong wrote:
> > ah! It's good plan.
> > 2015. 5. 1. 오전 6:21에 <mailto:brianderson@chromium.xn--org>-om0qi30h 작성:
> > 
> > > Feature freeze for M43 is tomorrow. To avoid throwing a wrench into anyone
> > > trying to cherry-pick changes for M43, should we hold off on trying to
> > > land this
> > > until next week since it's risky and not needed for M43?
> > >
> > > https://codereview.chromium.org/1016033006/
> > >
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> Hmm. Seems I was confused. I was concerned that this patch might interfere
with
> patches (related to video and DisplayScheduler) that I thought we might merge
> into M43. Turns out we aren't going to cherry-pick either of those - so let's
> try landing this on Monday when we're all back.

Let's try again :)

[simonhong, 2015-05-04 19:44:45]

The CQ bit was checked by simonhong@chromium.org

[simonhong, 2015-05-04 19:44:46]

The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org,
brianderson@chromium.org, boliu@chromium.org, danakj@chromium.org,
sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1016033006/#ps270001
(title: " ")

[commit-bot: I haz the power, 2015-05-04 19:45:51]

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

[commit-bot: I haz the power, 2015-05-04 19:51:16]

Committed patchset #15 (id:270001)

[commit-bot: I haz the power, 2015-05-04 19:52:18]

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

[Dan Beam, 2015-05-05 03:24:11]

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

The reason for reverting is: Making Autofill interactive tests incredibly flaky
again:
http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2...
http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2....

[Dan Beam, 2015-05-05 17:50:40]

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

[Dan Beam, 2015-05-05 17:50:42]

here's an example of the failures:
http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2...
http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2...

[simonhong, 2015-05-05 22:19:34]

On 2015/05/05 17:50:42, Dan Beam wrote:
> here's an example of the failures:
>
http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2...
>
http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2...

brian, Is it possible that this cl affect autofill behaviour?
autofill test looks like ui interaction test.
I can't imagine how my cl affects that test.

[brianderson, 2015-05-05 22:26:57]

On 2015/05/05 22:19:34, simonhong wrote:
> On 2015/05/05 17:50:42, Dan Beam wrote:
> > here's an example of the failures:
> >
>
http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2...
> >
>
http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2...
> 
> brian, Is it possible that this cl affect autofill behaviour?
> autofill test looks like ui interaction test.
> I can't imagine how my cl affects that test.

No clue. Maybe something is indirectly preventing us from sending a BeginFrame
by blocking the ui thread?

Will need to see a trace. Have you tried to build with the linux_chromeos
configuration rather than the linux config?
For gyp, use GYP_DEFINES='chromeos=1'
If you use cr:
cr init -o out_linuxchromeos/Release --platform linuxchromeos

Then use the trace startup directions from
https://www.chromium.org/developers/how-tos/trace-event-profiling-tool/record...

[brianderson, 2015-05-20 03:25:40]

Simon, can you work on landing this behind a flag so we can iron out some
integration work in parallel while you figure out the auto-fill test failure?
I'm hoping to land the DisplayScheduler soon and it will be nice to connect the
two. Jared's also hoping to use this for vsync aligned input.

[simonhong, 2015-05-20 03:40:58]

Yep! I will land with flag first soon.
2015. 5. 20. 오후 12:25에 <brianderson@chromium.org>님이 작성:

> Simon, can you work on landing this behind a flag so we can iron out some
> integration work in parallel while you figure out the auto-fill test
> failure?
> I'm hoping to land the DisplayScheduler soon and it will be nice to
> connect the
> two. Jared's also hoping to use this for vsync aligned input.
>
> https://codereview.chromium.org/1016033006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[simonhong, 2015-05-21 15:17:35]

On 2015/05/20 03:40:58, simonhong wrote:
> Yep! I will land with flag first soon.
> 2015. 5. 20. 오후 12:25에 <mailto:brianderson@chromium.xn--org>-om0qi30h 작성:
> 
> > Simon, can you work on landing this behind a flag so we can iron out some
> > integration work in parallel while you figure out the auto-fill test
> > failure?
> > I'm hoping to land the DisplayScheduler soon and it will be nice to
> > connect the
> > two. Jared's also hoping to use this for vsync aligned input.
> >
> > https://codereview.chromium.org/1016033006/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

PTAL, again!
I made BeginFrame scheduling work under flag.

[brianderson, 2015-05-21 21:52:59]

Thanks Simon! lgtm

[simonhong, 2015-05-22 01:00:57]

The CQ bit was checked by simonhong@chromium.org

[simonhong, 2015-05-22 01:00:58]

The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org,
boliu@chromium.org, danakj@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1016033006/#ps290001
(title: "Enable BeginFrame scheduling under flag")

[commit-bot: I haz the power, 2015-05-22 01:02:07]

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

[commit-bot: I haz the power, 2015-05-22 02:38:06]

Committed patchset #16 (id:290001)

[commit-bot: I haz the power, 2015-05-22 02:38:50]

Patchset 16 (id:??) landed as
https://crrev.com/047d61bf35b1142e2fb58ac0fca463bccefb4d74
Cr-Commit-Position: refs/heads/master@{#331050}