Getting rid of CompositorFrameMetadata::satisfies_sequences

Getting rid of CompositorFrameMetadata::satisfies_sequences

This CL helps unify the way we destroy surface references.
We currently have two code paths for satisfying a surface sequence.
One is through calling the satisfy callback, and the other one is through
populating satisfies_sequences in CompositorFrameMetadata and letting
Surface::QueueFrame take care of it. Eventually they both do
the same thing: they call SurfaceManager::DidSatisfySequences. This CL
gets rid of satisfies_sequences in CompositorFrameMetadata and from now
on the only way to satisfy a sequence is through the callback. This
is a necessary change because we are phasing out SurfaceSequence and
exposing it in CompositorFrameMetadata is not a good idea.

BUG=659598
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Committed: https://crrev.com/d0de6cf72a25d789a507d6eb904a8f1ae314d00b
Cr-Commit-Position: refs/heads/master@{#439043}

[Saman Sami, 2016-11-29 22:33:39]

Description was changed from

==========
Using satisfy callback in SatisfySwapPromise::DidSwap
==========

to

==========
Using satisfy callback in SatisfySwapPromise::DidSwap
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Saman Sami, 2016-11-29 22:33:51]

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

[Saman Sami, 2016-11-29 22:34:02]

The CQ bit was unchecked by samans@chromium.org

[Saman Sami, 2016-11-29 22:34:43]

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

[Saman Sami, 2016-11-29 22:34:48]

The CQ bit was unchecked by samans@chromium.org

[Saman Sami, 2016-11-29 22:35:07]

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

[commit-bot: I haz the power, 2016-11-29 22:35:59]

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

[Saman Sami, 2016-11-29 22:52:44]

Description was changed from

==========
Using satisfy callback in SatisfySwapPromise::DidSwap
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Using satisfy callback in SatisfySwapPromise::DidSwap
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Saman Sami, 2016-11-29 22:52:45]

samans@chromium.org changed reviewers:
+ fsamuel@chromium.org, jbauman@chromium.org

[commit-bot: I haz the power, 2016-11-29 23:13:21]

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

[commit-bot: I haz the power, 2016-11-29 23:13:21]

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

[Fady Samuel, 2016-11-30 15:35:28]

https://codereview.chromium.org/2537943002/diff/1/cc/layers/surface_layer.cc
File cc/layers/surface_layer.cc (right):

https://codereview.chromium.org/2537943002/diff/1/cc/layers/surface_layer.cc#...
cc/layers/surface_layer.cc:30: satisfy_callback_.Run(sequence_);
I think this runs on the compositor thread whereas DidNotSwap runs on main? Not
sure.

[jbauman, 2016-11-30 21:23:24]

On 2016/11/30 15:35:28, Fady Samuel wrote:
> https://codereview.chromium.org/2537943002/diff/1/cc/layers/surface_layer.cc
> File cc/layers/surface_layer.cc (right):
> 
>
https://codereview.chromium.org/2537943002/diff/1/cc/layers/surface_layer.cc#...
> cc/layers/surface_layer.cc:30: satisfy_callback_.Run(sequence_);
> I think this runs on the compositor thread whereas DidNotSwap runs on main?
Not
> sure.

This runs on the compositor thread, but DidNotSwap can run on either thread.

This call actually happens before the swap IPC (it needs to be able to modify
the frame metadata) so we might need to post a task to do the satisfy after the
draw actually happens.

[Saman Sami, 2016-12-08 03:33:49]

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

[commit-bot: I haz the power, 2016-12-08 03:34:11]

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

[commit-bot: I haz the power, 2016-12-08 04:09:46]

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

[commit-bot: I haz the power, 2016-12-08 04:09:49]

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

[Saman Sami, 2016-12-08 04:55:51]

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

[commit-bot: I haz the power, 2016-12-08 04:56:09]

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

[Saman Sami, 2016-12-08 05:37:22]

Description was changed from

==========
Using satisfy callback in SatisfySwapPromise::DidSwap
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Getting rid of CompositorFrameMetadata::satisfies_sequences
==========

[commit-bot: I haz the power, 2016-12-08 06:16:27]

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

[commit-bot: I haz the power, 2016-12-08 06:16:28]

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

[Saman Sami, 2016-12-08 15:19:40]

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

[commit-bot: I haz the power, 2016-12-08 15:20:09]

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

[Saman Sami, 2016-12-08 15:43:08]

Description was changed from

==========
Getting rid of CompositorFrameMetadata::satisfies_sequences
==========

to

==========
Getting rid of CompositorFrameMetadata::satisfies_sequences
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Saman Sami, 2016-12-08 15:43:14]

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

[commit-bot: I haz the power, 2016-12-08 15:43:31]

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

[Saman Sami, 2016-12-08 15:51:10]

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

[commit-bot: I haz the power, 2016-12-08 15:51:42]

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

[Saman Sami, 2016-12-08 16:12:59]

Description was changed from

==========
Getting rid of CompositorFrameMetadata::satisfies_sequences
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Getting rid of CompositorFrameMetadata::satisfies_sequences

We are transitioning from SurfaceSequences to SurfaceReferences, and this
should help.
satisfies_sequences was not widely used in the codebase. It was pretty much
solely used in SurfaceLayer, and I replaced its usage with a satisfy callback.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Saman Sami, 2016-12-08 16:20:02]

Description was changed from

==========
Getting rid of CompositorFrameMetadata::satisfies_sequences

We are transitioning from SurfaceSequences to SurfaceReferences, and this
should help.
satisfies_sequences was not widely used in the codebase. It was pretty much
solely used in SurfaceLayer, and I replaced its usage with a satisfy callback.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Getting rid of CompositorFrameMetadata::satisfies_sequences

We are transitioning from SurfaceSequences to SurfaceReferences, and this
should help.
satisfies_sequences was not widely used in the codebase. It was pretty much
solely used in SurfaceLayer, and I replaced it with a satisfy callback.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Saman Sami, 2016-12-08 16:20:02]

samans@chromium.org changed reviewers:
+ danakj@chromium.org, dcheng@chromium.org

[Saman Sami, 2016-12-08 17:16:52]

samans@chromium.org changed reviewers:
- jbauman@chromium.org

[Saman Sami, 2016-12-08 17:16:52]

@dcheng cc/ipc
@danakj everything else in cc/

[commit-bot: I haz the power, 2016-12-08 17:21:58]

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

[commit-bot: I haz the power, 2016-12-08 17:21:59]

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

[Saman Sami, 2016-12-08 17:25:18]

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

[commit-bot: I haz the power, 2016-12-08 17:25:54]

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

[Saman Sami, 2016-12-08 17:52:49]

samans@chromium.org changed reviewers:
+ jbauman@chromium.org

[Fady Samuel, 2016-12-08 18:01:33]

https://codereview.chromium.org/2537943002/diff/80001/cc/layers/surface_layer.cc
File cc/layers/surface_layer.cc (right):

https://codereview.chromium.org/2537943002/diff/80001/cc/layers/surface_layer...
cc/layers/surface_layer.cc:23: SurfaceSequence sequence,
nit: const SurfaceSequence& sequence

[commit-bot: I haz the power, 2016-12-08 18:56:45]

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

[commit-bot: I haz the power, 2016-12-08 18:56:46]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-12-08 19:25:38]

ipc lgtm

https://codereview.chromium.org/2537943002/diff/80001/cc/layers/surface_layer.cc
File cc/layers/surface_layer.cc (right):

https://codereview.chromium.org/2537943002/diff/80001/cc/layers/surface_layer...
cc/layers/surface_layer.cc:28: main_task_runner_(main_task_runner) {}
NIt: std::move() this

[Saman Sami, 2016-12-08 20:28:58]

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

[commit-bot: I haz the power, 2016-12-08 20:29:43]

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

[danakj, 2016-12-08 20:57:53]

I'm having a hard time saying if this breaks anything, esp just not calling
DidSatisfySequences. I think jbauman should look.

[danakj, 2016-12-08 21:39:19]

It would be nice to explain in the CL description perhaps why this change is
okay. Like I can see that DidSatisfy is called in the satisfy callback which
will now be used and wasn't before. But that happens at a different time.. when
is it and can u justify that it's as good/better/correct?

https://codereview.chromium.org/2537943002/diff/100001/cc/layers/surface_laye...
File cc/layers/surface_layer.cc (right):

https://codereview.chromium.org/2537943002/diff/100001/cc/layers/surface_laye...
cc/layers/surface_layer.cc:40: satisfy_callback_.Run(sequence_);
I can't help but notice this is running the callback directly without posting
it. From what I can tell this works because the only user in the renderer (where
the compositor is threaded) is canvas which only sends something over mojo IPC
when it runs this callback. But I think you should be consistent here, either
both run directly or both post. Perhaps both run directly is fine for now..

https://codereview.chromium.org/2537943002/diff/100001/cc/layers/surface_laye...
cc/layers/surface_layer.cc:127: std::unique_ptr<SatisfySwapPromise> satisfy(
can u change this to auto and base::MakeUnique while u'r changing it

[commit-bot: I haz the power, 2016-12-08 21:48:59]

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

[commit-bot: I haz the power, 2016-12-08 21:49:01]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Saman Sami, 2016-12-08 21:51:39]

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

[commit-bot: I haz the power, 2016-12-08 21:52:22]

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

[jbauman, 2016-12-08 22:14:49]

lgtm if you post to the main thread (don't need to post to the same thread
first) in DidNotSwap

On 2016/12/08 21:39:19, danakj_OOO_and_slow wrote:
> It would be nice to explain in the CL description perhaps why this change is
> okay. Like I can see that DidSatisfy is called in the satisfy callback which
> will now be used and wasn't before. But that happens at a different time..
when
> is it and can u justify that it's as good/better/correct?
> 
>
https://codereview.chromium.org/2537943002/diff/100001/cc/layers/surface_laye...
> File cc/layers/surface_layer.cc (right):
> 
>
https://codereview.chromium.org/2537943002/diff/100001/cc/layers/surface_laye...
> cc/layers/surface_layer.cc:40: satisfy_callback_.Run(sequence_);
> I can't help but notice this is running the callback directly without posting
> it. From what I can tell this works because the only user in the renderer
(where
> the compositor is threaded) is canvas which only sends something over mojo IPC
> when it runs this callback. But I think you should be consistent here, either
> both run directly or both post. Perhaps both run directly is fine for now..

Yeah, looks like this is actually broken with offscreen canvas, because the mojo
interface is bound to the main thread and shouldn't be run from the compositor
thread. So we need to change both.
> 
>
https://codereview.chromium.org/2537943002/diff/100001/cc/layers/surface_laye...
> cc/layers/surface_layer.cc:127: std::unique_ptr<SatisfySwapPromise> satisfy(
> can u change this to auto and base::MakeUnique while u'r changing it

[danakj, 2016-12-08 22:20:29]

On 2016/12/08 22:14:49, jbauman wrote:
> lgtm

rs lgtm for ownerythings

[Fady Samuel, 2016-12-08 22:21:17]

https://codereview.chromium.org/2537943002/diff/100001/cc/layers/surface_laye...
File cc/layers/surface_layer.cc (left):

https://codereview.chromium.org/2537943002/diff/100001/cc/layers/surface_laye...
cc/layers/surface_layer.cc:30:
metadata->satisfies_sequences.push_back(sequence_.sequence);
DCHECK(main_task_runner_->BelongsToCurrentThread()); would be helpful here to
clarify.

[Saman Sami, 2016-12-08 22:56:38]

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

[commit-bot: I haz the power, 2016-12-08 22:57:17]

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

[commit-bot: I haz the power, 2016-12-08 23:11:18]

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

[commit-bot: I haz the power, 2016-12-08 23:11:19]

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_androi...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Saman Sami, 2016-12-08 23:16:37]

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

[commit-bot: I haz the power, 2016-12-08 23:17:09]

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

[commit-bot: I haz the power, 2016-12-08 23:27:59]

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

[commit-bot: I haz the power, 2016-12-08 23:28:00]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Saman Sami, 2016-12-08 23:32:28]

Description was changed from

==========
Getting rid of CompositorFrameMetadata::satisfies_sequences

We are transitioning from SurfaceSequences to SurfaceReferences, and this
should help.
satisfies_sequences was not widely used in the codebase. It was pretty much
solely used in SurfaceLayer, and I replaced it with a satisfy callback.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Getting rid of CompositorFrameMetadata::satisfies_sequences

This CL helps unify the way we destroy references to surfaces.
We currently have two code paths for satisfying a surface sequence.
One is through calling the satisfy callback which is given to the 
reference holder, and the other one is through populating
satisfies_sequences in CompositorFrameMetadata and letting 
Surface::QueueFrame do the rest of the work. Eventually they both do 
the same thing: they call SurfaceManager::DidSatisfySequences. This CL 
gets rid of satisfies_sequences in CompositorFrameMetadata and from now 
on the only way for satisfying a sequence is through the callback. This 
is a necessary change because we are phasing out SurfaceSequence and
exposing it in CompositorFrameMetadata is not a good idea.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Saman Sami, 2016-12-09 00:44:23]

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

[commit-bot: I haz the power, 2016-12-09 00:44:50]

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

[commit-bot: I haz the power, 2016-12-09 01:39:15]

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

[commit-bot: I haz the power, 2016-12-09 01:39:15]

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

[Saman Sami, 2016-12-09 03:16:20]

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

[commit-bot: I haz the power, 2016-12-09 03:16:46]

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

[Saman Sami, 2016-12-09 03:26:52]

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

[commit-bot: I haz the power, 2016-12-09 03:27:25]

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

[commit-bot: I haz the power, 2016-12-09 04:25:49]

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

[commit-bot: I haz the power, 2016-12-09 04:25:50]

Dry run: This issue passed the CQ dry run.

[Saman Sami, 2016-12-14 00:25:32]

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

[commit-bot: I haz the power, 2016-12-14 00:25:52]

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

[commit-bot: I haz the power, 2016-12-14 00:55:11]

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

[commit-bot: I haz the power, 2016-12-14 00:55:12]

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

[Saman Sami, 2016-12-14 01:36:17]

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

[Saman Sami, 2016-12-14 01:36:23]

Patchset #12 (id:220001) has been deleted

[commit-bot: I haz the power, 2016-12-14 01:37:06]

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

[Saman Sami, 2016-12-14 01:55:28]

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

[commit-bot: I haz the power, 2016-12-14 01:55:52]

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

[Saman Sami, 2016-12-14 01:57:08]

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

[commit-bot: I haz the power, 2016-12-14 01:57:54]

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

[commit-bot: I haz the power, 2016-12-14 03:01:28]

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

[commit-bot: I haz the power, 2016-12-14 03:01:29]

Dry run: This issue passed the CQ dry run.

[Saman Sami, 2016-12-14 03:01:51]

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

[commit-bot: I haz the power, 2016-12-14 03:03:03]

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

[commit-bot: I haz the power, 2016-12-14 03:35:18]

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

[commit-bot: I haz the power, 2016-12-14 03:35:19]

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

[Saman Sami, 2016-12-14 03:37:26]

Patchset #15 (id:300001) has been deleted

[Fady Samuel, 2016-12-14 05:03:06]

https://codereview.chromium.org/2537943002/diff/280001/cc/layers/surface_laye...
File cc/layers/surface_layer.cc (right):

https://codereview.chromium.org/2537943002/diff/280001/cc/layers/surface_laye...
cc/layers/surface_layer.cc:51: base::ThreadTaskRunnerHandle::Get()->PostTask(
Not a huge fan of this, but if jbauman@ is OK with it then so am I.

[danakj, 2016-12-14 16:17:21]

https://codereview.chromium.org/2537943002/diff/280001/cc/layers/surface_laye...
File cc/layers/surface_layer.cc (right):

https://codereview.chromium.org/2537943002/diff/280001/cc/layers/surface_laye...
cc/layers/surface_layer.cc:51: base::ThreadTaskRunnerHandle::Get()->PostTask(
On 2016/12/14 05:03:06, Fady Samuel wrote:
> Not a huge fan of this, but if jbauman@ is OK with it then so am I.

Hm.. this will ofc wait for a fresh callstack, but it is pretty fragile as it
depends on behaviour of unrelated implementation details. It should have a test
that verifies that doing a post will run the task after the swap is done on that
thread.

Another option would be DidSwap => WillSwap (and DidNotSwap => WillNotSwap) and
then add a new DidSwap that is actually after. Then its less magical and spelled
out with APIs.

[Fady Samuel, 2016-12-14 16:20:42]

On 2016/12/14 16:17:21, danakj_OOO_and_slow wrote:
>
https://codereview.chromium.org/2537943002/diff/280001/cc/layers/surface_laye...
> File cc/layers/surface_layer.cc (right):
> 
>
https://codereview.chromium.org/2537943002/diff/280001/cc/layers/surface_laye...
> cc/layers/surface_layer.cc:51: base::ThreadTaskRunnerHandle::Get()->PostTask(
> On 2016/12/14 05:03:06, Fady Samuel wrote:
> > Not a huge fan of this, but if jbauman@ is OK with it then so am I.
> 
> Hm.. this will ofc wait for a fresh callstack, but it is pretty fragile as it
> depends on behaviour of unrelated implementation details. It should have a
test
> that verifies that doing a post will run the task after the swap is done on
that
> thread.
> 
> Another option would be DidSwap => WillSwap (and DidNotSwap => WillNotSwap)
and
> then add a new DidSwap that is actually after. Then its less magical and
spelled
> out with APIs.

+1 I like the idea of renaming DidSwap => WillSwap, DidNotSwap => WillNotSwap as
a separate mechanical CL first, then introducing DidSwap, and DidNotSwap as
necessary in this CL. APIs that do what their names suggest is always a good
thing :-)

[Saman Sami, 2016-12-15 21:28:35]

Description was changed from

==========
Getting rid of CompositorFrameMetadata::satisfies_sequences

This CL helps unify the way we destroy references to surfaces.
We currently have two code paths for satisfying a surface sequence.
One is through calling the satisfy callback which is given to the 
reference holder, and the other one is through populating
satisfies_sequences in CompositorFrameMetadata and letting 
Surface::QueueFrame do the rest of the work. Eventually they both do 
the same thing: they call SurfaceManager::DidSatisfySequences. This CL 
gets rid of satisfies_sequences in CompositorFrameMetadata and from now 
on the only way for satisfying a sequence is through the callback. This 
is a necessary change because we are phasing out SurfaceSequence and
exposing it in CompositorFrameMetadata is not a good idea.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Getting rid of CompositorFrameMetadata::satisfies_sequences

This CL helps unify the way we destroy references to surfaces.
We currently have two code paths for satisfying a surface sequence.
One is through calling the satisfy callback which is given to the 
reference holder, and the other one is through populating
satisfies_sequences in CompositorFrameMetadata and letting 
Surface::QueueFrame do the rest of the work. Eventually they both do 
the same thing: they call SurfaceManager::DidSatisfySequences. This CL 
gets rid of satisfies_sequences in CompositorFrameMetadata and from now 
on the only way for satisfying a sequence is through the callback. This 
is a necessary change because we are phasing out SurfaceSequence and
exposing it in CompositorFrameMetadata is not a good idea.

BUG=659598
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Saman Sami, 2016-12-15 22:50:10]

samans@chromium.org changed reviewers:
+ dtrainor@chromium.org, jam@chromium.org

[Saman Sami, 2016-12-15 22:50:11]

dtrainor@chromium.org: Please review changes in blimp/ and ui/android

jam@chromium.org: Please review changes in content/

[David Trainor- moved to gerrit, 2016-12-15 23:06:02]

blimp/ and ui/android lgtm

[Saman Sami, 2016-12-15 23:09:32]

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

[commit-bot: I haz the power, 2016-12-15 23:10:11]

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

[Saman Sami, 2016-12-15 23:10:48]

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

[Saman Sami, 2016-12-15 23:11:03]

Patchset #15 (id:320001) has been deleted

[commit-bot: I haz the power, 2016-12-15 23:11:53]

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

[jam, 2016-12-16 01:02:17]

lgtm with nit

https://codereview.chromium.org/2537943002/diff/340001/content/browser/render...
File content/browser/renderer_host/delegated_frame_host.cc (right):

https://codereview.chromium.org/2537943002/diff/340001/content/browser/render...
content/browser/renderer_host/delegated_frame_host.cc:55: DCHECK(manager);
nit: remove this, it's not necessary. in release builds, it does nothing. in
debug builds, the line below will crash immediately which is just as useful of a
signal

[Saman Sami, 2016-12-16 01:55:43]

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

[commit-bot: I haz the power, 2016-12-16 01:56:35]

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

[commit-bot: I haz the power, 2016-12-16 02:42:37]

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

[commit-bot: I haz the power, 2016-12-16 02:42:39]

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

[Saman Sami, 2016-12-16 03:06:34]

The CQ bit was checked by samans@chromium.org

[Saman Sami, 2016-12-16 03:06:34]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org,
danakj@chromium.org, jbauman@chromium.org, dtrainor@chromium.org,
jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2537943002/#ps380001
(title: "x")

[commit-bot: I haz the power, 2016-12-16 03:07:37]

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

[commit-bot: I haz the power, 2016-12-16 06:15:54]

CQ is committing da patch.

Bot data: {"patchset_id": 380001, "attempt_start_ts": 1481857594115000,
"parent_rev": "a722a9d9beb7562596abd8bdeca8167c0aee50ab", "commit_rev":
"4f41f96c59ca7b0d3d9c2d41db407335fe9e2103"}

[commit-bot: I haz the power, 2016-12-16 06:16:43]

Description was changed from

==========
Getting rid of CompositorFrameMetadata::satisfies_sequences

This CL helps unify the way we destroy references to surfaces.
We currently have two code paths for satisfying a surface sequence.
One is through calling the satisfy callback which is given to the 
reference holder, and the other one is through populating
satisfies_sequences in CompositorFrameMetadata and letting 
Surface::QueueFrame do the rest of the work. Eventually they both do 
the same thing: they call SurfaceManager::DidSatisfySequences. This CL 
gets rid of satisfies_sequences in CompositorFrameMetadata and from now 
on the only way for satisfying a sequence is through the callback. This 
is a necessary change because we are phasing out SurfaceSequence and
exposing it in CompositorFrameMetadata is not a good idea.

BUG=659598
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Getting rid of CompositorFrameMetadata::satisfies_sequences

This CL helps unify the way we destroy references to surfaces.
We currently have two code paths for satisfying a surface sequence.
One is through calling the satisfy callback which is given to the 
reference holder, and the other one is through populating
satisfies_sequences in CompositorFrameMetadata and letting 
Surface::QueueFrame do the rest of the work. Eventually they both do 
the same thing: they call SurfaceManager::DidSatisfySequences. This CL 
gets rid of satisfies_sequences in CompositorFrameMetadata and from now 
on the only way for satisfying a sequence is through the callback. This 
is a necessary change because we are phasing out SurfaceSequence and
exposing it in CompositorFrameMetadata is not a good idea.

BUG=659598
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2537943002
==========

[commit-bot: I haz the power, 2016-12-16 06:16:44]

Committed patchset #17 (id:380001)

[commit-bot: I haz the power, 2016-12-16 06:21:11]

Description was changed from

==========
Getting rid of CompositorFrameMetadata::satisfies_sequences

This CL helps unify the way we destroy references to surfaces.
We currently have two code paths for satisfying a surface sequence.
One is through calling the satisfy callback which is given to the 
reference holder, and the other one is through populating
satisfies_sequences in CompositorFrameMetadata and letting 
Surface::QueueFrame do the rest of the work. Eventually they both do 
the same thing: they call SurfaceManager::DidSatisfySequences. This CL 
gets rid of satisfies_sequences in CompositorFrameMetadata and from now 
on the only way for satisfying a sequence is through the callback. This 
is a necessary change because we are phasing out SurfaceSequence and
exposing it in CompositorFrameMetadata is not a good idea.

BUG=659598
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2537943002
==========

to

==========
Getting rid of CompositorFrameMetadata::satisfies_sequences

This CL helps unify the way we destroy references to surfaces.
We currently have two code paths for satisfying a surface sequence.
One is through calling the satisfy callback which is given to the
reference holder, and the other one is through populating
satisfies_sequences in CompositorFrameMetadata and letting
Surface::QueueFrame do the rest of the work. Eventually they both do
the same thing: they call SurfaceManager::DidSatisfySequences. This CL
gets rid of satisfies_sequences in CompositorFrameMetadata and from now
on the only way for satisfying a sequence is through the callback. This
is a necessary change because we are phasing out SurfaceSequence and
exposing it in CompositorFrameMetadata is not a good idea.

BUG=659598
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Committed: https://crrev.com/d0de6cf72a25d789a507d6eb904a8f1ae314d00b
Cr-Commit-Position: refs/heads/master@{#439043}
==========

[commit-bot: I haz the power, 2016-12-16 06:21:13]

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

[Kunihiko Sakamoto, 2016-12-16 06:54:09]

A revert of this CL (patchset #17 id:380001) has been created in
https://codereview.chromium.org/2585543003/ by ksakamoto@chromium.org.

The reason for reverting is: Suspected cause of ChromiumOS compile failure.
https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%...
.

[Kunihiko Sakamoto, 2016-12-16 08:01:09]

On 2016/12/16 06:54:09, Kunihiko Sakamoto wrote:
> A revert of this CL (patchset #17 id:380001) has been created in
> https://codereview.chromium.org/2585543003/ by mailto:ksakamoto@chromium.org.
> 
> The reason for reverting is: Suspected cause of ChromiumOS compile failure.
>
https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%...
> .

The real culprit was https://codereview.chromium.org/2578893003/. Relanding
this.

[Kunihiko Sakamoto, 2016-12-16 08:10:39]

On 2016/12/16 08:01:09, Kunihiko Sakamoto wrote:
> On 2016/12/16 06:54:09, Kunihiko Sakamoto wrote:
> > A revert of this CL (patchset #17 id:380001) has been created in
> > https://codereview.chromium.org/2585543003/ by
mailto:ksakamoto@chromium.org.
> > 
> > The reason for reverting is: Suspected cause of ChromiumOS compile failure.
> >
>
https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%...
> > .
> 
> The real culprit was https://codereview.chromium.org/2578893003/. Relanding
> this.

Relanded at https://codereview.chromium.org/2583803002/

[Saman Sami, 2016-12-16 21:46:20]

Description was changed from

==========
Getting rid of CompositorFrameMetadata::satisfies_sequences

This CL helps unify the way we destroy references to surfaces.
We currently have two code paths for satisfying a surface sequence.
One is through calling the satisfy callback which is given to the
reference holder, and the other one is through populating
satisfies_sequences in CompositorFrameMetadata and letting
Surface::QueueFrame do the rest of the work. Eventually they both do
the same thing: they call SurfaceManager::DidSatisfySequences. This CL
gets rid of satisfies_sequences in CompositorFrameMetadata and from now
on the only way for satisfying a sequence is through the callback. This
is a necessary change because we are phasing out SurfaceSequence and
exposing it in CompositorFrameMetadata is not a good idea.

BUG=659598
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Committed: https://crrev.com/d0de6cf72a25d789a507d6eb904a8f1ae314d00b
Cr-Commit-Position: refs/heads/master@{#439043}
==========

to

==========
Getting rid of CompositorFrameMetadata::satisfies_sequences

This CL helps unify the way we destroy surface references.
We currently have two code paths for satisfying a surface sequence.
One is through calling the satisfy callback, and the other one is through
populating satisfies_sequences in CompositorFrameMetadata and letting
Surface::QueueFrame take care of it. Eventually they both do
the same thing: they call SurfaceManager::DidSatisfySequences. This CL
gets rid of satisfies_sequences in CompositorFrameMetadata and from now
on the only way to satisfy a sequence is through the callback. This
is a necessary change because we are phasing out SurfaceSequence and
exposing it in CompositorFrameMetadata is not a good idea.

BUG=659598
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Committed: https://crrev.com/d0de6cf72a25d789a507d6eb904a8f1ae314d00b
Cr-Commit-Position: refs/heads/master@{#439043}
==========