cc: Don't make AcquireLayerTextures force a commit.

cc: Don't make AcquireLayerTextures force a commit.

Clients will just do it from the main thread when they're ready.

BUG=243311
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=211210

[jamesr, 2013-05-25 00:44:17]

word, lgtm

did Dana's patch provide test coverage for this or is that still tbd?

[piman, 2013-05-25 00:50:46]

On Fri, May 24, 2013 at 5:44 PM, <jamesr@chromium.org> wrote:

> word, lgtm
>
> did Dana's patch provide test coverage for this or is that still tbd?
>

Partial coverage. It tests the assumption that it cause a commit.
I've been trying to come up with a good integration test for this, but I
can't come up with a good answer that's not flaky - trying to test that a
task isn't posted is hard without nasty shenanigans.
But I'm willing to hear any idea!


>
https://codereview.chromium.**org/15730005/<https://codereview.chromium.org/1...
>

[commit-bot: I haz the power, 2013-05-25 00:53:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/piman@chromium.org/15730005/7001

[piman, 2013-05-25 00:54:20]

On Fri, May 24, 2013 at 5:50 PM, Antoine Labour <piman@chromium.org> wrote:

>
>
>
> On Fri, May 24, 2013 at 5:44 PM, <jamesr@chromium.org> wrote:
>
>> word, lgtm
>>
>> did Dana's patch provide test coverage for this or is that still tbd?
>>
>
> Partial coverage. It tests the assumption that it cause a commit.
> I've been trying to come up with a good integration test for this, but I
> can't come up with a good answer that's not flaky - trying to test that a
> task isn't posted is hard without nasty shenanigans.
> But I'm willing to hear any idea!
>

In the mean time, I'm sending this to the CQ, it should reduce flakiness
which is a good thing. If we can find a good way to test this, I'm happy to
follow-up.


>
>
>>
https://codereview.chromium.**org/15730005/<https://codereview.chromium.org/1...
>>
>
>

[jamesr, 2013-05-25 00:57:19]

I can't think of anything brilliant either. I agree this patch is good to have
regardless.

[piman, 2013-05-25 01:17:08]

On 2013/05/25 00:57:19, jamesr wrote:
> I can't think of anything brilliant either. I agree this patch is good to have
> regardless.

Mmh, the Canvas2DAllowed flake on browser_tests looks real. I unchecked the CQ
while I investigate.

[piman, 2013-05-25 01:47:03]

On Fri, May 24, 2013 at 6:17 PM, <piman@chromium.org> wrote:

> On 2013/05/25 00:57:19, jamesr wrote:
>
>> I can't think of anything brilliant either. I agree this patch is good to
>> have
>> regardless.
>>
>
> Mmh, the Canvas2DAllowed flake on browser_tests looks real. I unchecked
> the CQ
> while I investigate.
>

It breaks canvas because it calls it from UpdateLayers, which is after we
reset commit_request_sent_to_impl_thread_, but before we send the commit.
When that happens, the impl thread gets its commit from the bottom of
BeginFrameOnMainThread, so thinks it's done wrt the acquire and commits,
but the main thread thinks it's expecting a new frame which it will never
get, blocking further commits.


>
>
https://codereview.chromium.**org/15730005/<https://codereview.chromium.org/1...
>

[danakj, 2013-06-12 17:30:45]

On 2013/05/25 01:47:03, piman wrote:
> On Fri, May 24, 2013 at 6:17 PM, <mailto:piman@chromium.org> wrote:
> 
> > On 2013/05/25 00:57:19, jamesr wrote:
> >
> >> I can't think of anything brilliant either. I agree this patch is good to
> >> have
> >> regardless.
> >>
> >
> > Mmh, the Canvas2DAllowed flake on browser_tests looks real. I unchecked
> > the CQ
> > while I investigate.
> >
> 
> It breaks canvas because it calls it from UpdateLayers, which is after we
> reset commit_request_sent_to_impl_thread_, but before we send the commit.
> When that happens, the impl thread gets its commit from the bottom of
> BeginFrameOnMainThread, so thinks it's done wrt the acquire and commits,
> but the main thread thinks it's expecting a new frame which it will never
> get, blocking further commits.
> 
> 
> >
> >
>
https://codereview.chromium.**org/15730005/%3Chttps://codereview.chromium.org...>
> >

Do you think you'll explore the first option you gave on the bug, or want to try
make this patch work somehow?

[piman, 2013-06-12 19:38:43]

On 2013/06/12 17:30:45, danakj wrote:
> On 2013/05/25 01:47:03, piman wrote:
> > On Fri, May 24, 2013 at 6:17 PM, <mailto:piman@chromium.org> wrote:
> > 
> > > On 2013/05/25 00:57:19, jamesr wrote:
> > >
> > >> I can't think of anything brilliant either. I agree this patch is good to
> > >> have
> > >> regardless.
> > >>
> > >
> > > Mmh, the Canvas2DAllowed flake on browser_tests looks real. I unchecked
> > > the CQ
> > > while I investigate.
> > >
> > 
> > It breaks canvas because it calls it from UpdateLayers, which is after we
> > reset commit_request_sent_to_impl_thread_, but before we send the commit.
> > When that happens, the impl thread gets its commit from the bottom of
> > BeginFrameOnMainThread, so thinks it's done wrt the acquire and commits,
> > but the main thread thinks it's expecting a new frame which it will never
> > get, blocking further commits.
> > 
> > 
> > >
> > >
> >
>
https://codereview.chromium.**org/15730005/%253Chttps://codereview.chromium.o...>
> > >
> 
> Do you think you'll explore the first option you gave on the bug, or want to
try
> make this patch work somehow?

I have a WIP patch, but it's not ready yet.

[jamesr, 2013-07-09 06:32:20]

On 2013/06/12 19:38:43, piman wrote:
> I have a WIP patch, but it's not ready yet.

Any update here?

[piman, 2013-07-11 02:43:48]

PTAL with a new version that doesn't force a commit to happen (see bug).

[danakj, 2013-07-11 15:38:52]

Removing code from scheduler state machine LGTM

[commit-bot: I haz the power, 2013-07-11 17:25:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/piman@chromium.org/15730005/42001

[commit-bot: I haz the power, 2013-07-11 20:54:25]

Change committed as 211210