Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2616403003:
Replacing SurfaceReferenceBase and SequenceSurfaceReference with Closures (Closed)
|
Created:
3 years, 11 months ago by Saman Sami Modified:
3 years, 11 months ago CC:
chromium-reviews, kalyank, cc-bugs_chromium.org, sadrul Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReplacing SurfaceReferenceBase and SequenceSurfaceReference with Closures
The sole purpose of SurfaceReferenceBase and SequenceSurfaceReference
was to be able to return the reference later on, but the same goal can
be achieved using closures with much less complexity. From now on,
SurfaceReferenceFactory::CreateReference returns a closure that returns
the reference once its called.
TBR=sadrul@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2616403003
Cr-Commit-Position: refs/heads/master@{#442984}
Committed: https://chromium.googlesource.com/chromium/src/+/6c267ca6a4bc8f207d58a682a4c98f0b9b697ad4
Patch Set 1 #Patch Set 2 : comment #Patch Set 3 : build #
Total comments: 1
Patch Set 4 : c #
Total comments: 8
Patch Set 5 : up #Patch Set 6 : c #Patch Set 7 : c #Patch Set 8 : c #
Total comments: 15
Patch Set 9 : c #
Total comments: 3
Patch Set 10 : callback unittest #
Total comments: 1
Patch Set 11 : dot #Patch Set 12 : c #
Messages
Total messages: 72 (48 generated)
Description was changed from ========== Refactoring SurfaceReferenceBase, Introducing ScopedSurfaceReference ========== to ========== Refactoring SurfaceReferenceBase, Introducing ScopedSurfaceReference CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Refactoring SurfaceReferenceBase, Introducing ScopedSurfaceReference CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Refactoring SurfaceReferenceBase, Introducing ScopedSurfaceReference This CL does the following renames: SurfaceReferenceBase -> ScopedSurfaceReferenceBase SequenceSurfaceReference -> ScopedSurfaceSequence and introduces ScopedSurfaceReference, which is a SurfaceReference that automatically removes itself when it goes out of scope. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by samans@chromium.org to run a CQ dry run
samans@chromium.org changed reviewers: + fsamuel@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
samans@chromium.org changed reviewers: + danakj@chromium.org, jbauman@chromium.org
danakj and jbauman: Please review all files.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by samans@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_presub...)
https://codereview.chromium.org/2616403003/diff/40001/ui/aura/mus/client_surf... File ui/aura/mus/client_surface_embedder.cc (right): https://codereview.chromium.org/2616403003/diff/40001/ui/aura/mus/client_surf... ui/aura/mus/client_surface_embedder.cc:15: class StubSurfaceReference : public cc::ScopedSurfaceReferenceBase { I just noticed this doesn't actually do anything? How about deleting this and simply using ScopedSurfaceReference?
samans@chromium.org changed reviewers: + sadrul@chromium.org
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Refactoring SurfaceReferenceBase, Introducing ScopedSurfaceReference This CL does the following renames: SurfaceReferenceBase -> ScopedSurfaceReferenceBase SequenceSurfaceReference -> ScopedSurfaceSequence and introduces ScopedSurfaceReference, which is a SurfaceReference that automatically removes itself when it goes out of scope. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Refactoring SurfaceReferenceBase, Introducing ScopedSurfaceReference This CL does the following renames: SurfaceReferenceBase -> ScopedSurfaceReferenceBase SequenceSurfaceReference -> ScopedSurfaceSequence and introduces ScopedSurfaceReference, which is a SurfaceReference that automatically removes itself when it goes out of scope. TBR=sadrul@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2616403003/diff/60001/cc/layers/surface_layer.cc File cc/layers/surface_layer.cc (right): https://codereview.chromium.org/2616403003/diff/60001/cc/layers/surface_layer... cc/layers/surface_layer.cc:70: current_ref_ = Everything is using the base types so I don't understand really the point of the subclasses for sequence and for reference. They seem like internal details of the factory at this point. CreateReference could return a base::Closure() to call when you're done with it, and has the sequence/reference bound into the Closure so it goes back to the factory, for instance. Can you help me understand why this class hierarchy is more helpful? https://codereview.chromium.org/2616403003/diff/60001/cc/surfaces/scoped_surf... File cc/surfaces/scoped_surface_reference_base.cc (right): https://codereview.chromium.org/2616403003/diff/60001/cc/surfaces/scoped_surf... cc/surfaces/scoped_surface_reference_base.cc:12: scoped_refptr<const SurfaceReferenceFactory> factory) why const? https://codereview.chromium.org/2616403003/diff/60001/cc/surfaces/scoped_surf... cc/surfaces/scoped_surface_reference_base.cc:16: DCHECK(!factory_) << "Each leaf subclass must call Destroy in its destructor"; Why do subclasses have to call it? https://codereview.chromium.org/2616403003/diff/60001/cc/surfaces/sequence_su... File cc/surfaces/sequence_surface_reference_factory.cc (right): https://codereview.chromium.org/2616403003/diff/60001/cc/surfaces/sequence_su... cc/surfaces/sequence_surface_reference_factory.cc:18: return base::MakeUnique<ScopedSurfaceSequence>(make_scoped_refptr(this), seq); Do the references have to control the lifetime of the factory? Under what conditions is the factory destroyed before the references?
https://codereview.chromium.org/2616403003/diff/60001/cc/layers/surface_layer.cc File cc/layers/surface_layer.cc (right): https://codereview.chromium.org/2616403003/diff/60001/cc/layers/surface_layer... cc/layers/surface_layer.cc:70: current_ref_ = On 2017/01/10 16:46:30, danakj (slow) wrote: > Everything is using the base types so I don't understand really the point of the > subclasses for sequence and for reference. They seem like internal details of > the factory at this point. > > CreateReference could return a base::Closure() to call when you're done with it, > and has the sequence/reference bound into the Closure so it goes back to the > factory, for instance. > > Can you help me understand why this class hierarchy is more helpful? All we care about is that the details of the reference are not part of the SurfaceLayer, so yes, I think a callback can work here without the class hierarchy. https://codereview.chromium.org/2616403003/diff/60001/cc/surfaces/scoped_surf... File cc/surfaces/scoped_surface_reference_base.cc (right): https://codereview.chromium.org/2616403003/diff/60001/cc/surfaces/scoped_surf... cc/surfaces/scoped_surface_reference_base.cc:16: DCHECK(!factory_) << "Each leaf subclass must call Destroy in its destructor"; On 2017/01/10 16:46:30, danakj (slow) wrote: > Why do subclasses have to call it? The data "SurfaceReference" or "SurfaceSequence" lives in the derived class. We need to call "Destroy" before we destroy the data objects in the derived class, thus Destroy must be called in the derived class. I agree though that callbacks can accomplish what we want though without this subtlety. https://codereview.chromium.org/2616403003/diff/60001/cc/surfaces/sequence_su... File cc/surfaces/sequence_surface_reference_factory.cc (right): https://codereview.chromium.org/2616403003/diff/60001/cc/surfaces/sequence_su... cc/surfaces/sequence_surface_reference_factory.cc:18: return base::MakeUnique<ScopedSurfaceSequence>(make_scoped_refptr(this), seq); On 2017/01/10 16:46:30, danakj (slow) wrote: > Do the references have to control the lifetime of the factory? Under what > conditions is the factory destroyed before the references? The references are held on the compositor thread whereas the factory lives on the main thread so during tear down the factory may be destroyed before the reference IIRC.
https://codereview.chromium.org/2616403003/diff/60001/cc/surfaces/sequence_su... File cc/surfaces/sequence_surface_reference_factory.cc (right): https://codereview.chromium.org/2616403003/diff/60001/cc/surfaces/sequence_su... cc/surfaces/sequence_surface_reference_factory.cc:18: return base::MakeUnique<ScopedSurfaceSequence>(make_scoped_refptr(this), seq); On 2017/01/10 18:14:01, Fady Samuel wrote: > On 2017/01/10 16:46:30, danakj (slow) wrote: > > Do the references have to control the lifetime of the factory? Under what > > conditions is the factory destroyed before the references? > > The references are held on the compositor thread whereas the factory lives on > the main thread so during tear down the factory may be destroyed before the > reference IIRC. What if they are posted to the main thread via https://cs.chromium.org/chromium/src/cc/trees/blocking_task_runner.h?q=blocki... BlockingTaskRunner, and we had a BlockingTaskRunner::CapturePostTasks around the blocking call to Stop() the compositor thread?
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL I replaced the scoped reference classes with closures.
Net negative lines of code! I love it! Please update the description! LGTM
Description was changed from ========== Refactoring SurfaceReferenceBase, Introducing ScopedSurfaceReference This CL does the following renames: SurfaceReferenceBase -> ScopedSurfaceReferenceBase SequenceSurfaceReference -> ScopedSurfaceSequence and introduces ScopedSurfaceReference, which is a SurfaceReference that automatically removes itself when it goes out of scope. TBR=sadrul@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Replacing SurfaceReferenceBase and SequenceSurfaceReference with Closures The sole purpose of SurfaceReferenceBase and SequenceSurfaceReference was to be able to return the reference later on, but the same goal can be achieved using closures with much less complexity. From now on, SurfaceReferenceFactory::CreateReference returns a closure that returns the reference once its called. TBR=sadrul@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was unchecked by commit-bot@chromium.org
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...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Sorry, a few off-topic question/comments while reading the code also. https://codereview.chromium.org/2616403003/diff/140001/cc/layers/surface_laye... File cc/layers/surface_layer.cc (right): https://codereview.chromium.org/2616403003/diff/140001/cc/layers/surface_laye... cc/layers/surface_layer.cc:112: std::move(reference_returner_), base::ThreadTaskRunnerHandle::Get()); Just noticed, but this code should not use ThreadTaskRunnerHandle, it should be getting the task runner from LayerTreeHost. https://codereview.chromium.org/2616403003/diff/140001/cc/surfaces/sequence_s... File cc/surfaces/sequence_surface_reference_factory.cc (right): https://codereview.chromium.org/2616403003/diff/140001/cc/surfaces/sequence_s... cc/surfaces/sequence_surface_reference_factory.cc:19: return base::Bind(&SequenceSurfaceReferenceFactory::SatisfySequence, this, This still binds the lifetime of |this| to the callback, what did you think of whay I posted earlier about BlockingTaskRunner? https://codereview.chromium.org/2616403003/diff/140001/cc/surfaces/sequence_s... File cc/surfaces/sequence_surface_reference_factory.h (right): https://codereview.chromium.org/2616403003/diff/140001/cc/surfaces/sequence_s... cc/surfaces/sequence_surface_reference_factory.h:20: base::Closure CreateReference(SurfaceReferenceOwner* owner, // SurfaceReferenceFactory implementation. https://codereview.chromium.org/2616403003/diff/140001/cc/surfaces/sequence_s... cc/surfaces/sequence_surface_reference_factory.h:27: virtual void RequireSequence(const SurfaceId& surface_id, What does it mean to be private and abstract? The point of being abstract is for a subclass to override it, implying protected at least..? https://codereview.chromium.org/2616403003/diff/140001/cc/surfaces/surface_re... File cc/surfaces/surface_reference_factory.h (right): https://codereview.chromium.org/2616403003/diff/140001/cc/surfaces/surface_re... cc/surfaces/surface_reference_factory.h:21: // a closure that can be called to remove the reference. "must be called"? It can only be called a single time also right? https://codereview.chromium.org/2616403003/diff/140001/cc/surfaces/surface_re... cc/surfaces/surface_reference_factory.h:25: SurfaceReferenceFactory() = default; why does this class have a constructor?
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
https://codereview.chromium.org/2616403003/diff/140001/cc/layers/surface_laye... File cc/layers/surface_layer.cc (right): https://codereview.chromium.org/2616403003/diff/140001/cc/layers/surface_laye... cc/layers/surface_layer.cc:112: std::move(reference_returner_), base::ThreadTaskRunnerHandle::Get()); On 2017/01/10 21:12:06, danakj (slow) wrote: > Just noticed, but this code should not use ThreadTaskRunnerHandle, it should be > getting the task runner from LayerTreeHost. LayerTreeHostImpl::task_runner_provider()->MainThreadTaskRunner() looks like the right task runner. https://codereview.chromium.org/2616403003/diff/140001/cc/surfaces/sequence_s... File cc/surfaces/sequence_surface_reference_factory.h (right): https://codereview.chromium.org/2616403003/diff/140001/cc/surfaces/sequence_s... cc/surfaces/sequence_surface_reference_factory.h:27: virtual void RequireSequence(const SurfaceId& surface_id, On 2017/01/10 21:12:06, danakj (slow) wrote: > What does it mean to be private and abstract? The point of being abstract is for > a subclass to override it, implying protected at least..? +1 https://codereview.chromium.org/2616403003/diff/140001/cc/surfaces/surface_re... File cc/surfaces/surface_reference_factory.h (right): https://codereview.chromium.org/2616403003/diff/140001/cc/surfaces/surface_re... cc/surfaces/surface_reference_factory.h:21: // a closure that can be called to remove the reference. On 2017/01/10 21:12:06, danakj (slow) wrote: > "must be called"? It can only be called a single time also right? Yes it can only be called a single time.
The CQ bit was checked by samans@chromium.org to run a CQ dry run
https://codereview.chromium.org/2616403003/diff/140001/cc/layers/surface_laye... File cc/layers/surface_layer.cc (right): https://codereview.chromium.org/2616403003/diff/140001/cc/layers/surface_laye... cc/layers/surface_layer.cc:112: std::move(reference_returner_), base::ThreadTaskRunnerHandle::Get()); On 2017/01/10 21:12:06, danakj (slow) wrote: > Just noticed, but this code should not use ThreadTaskRunnerHandle, it should be > getting the task runner from LayerTreeHost. Done. https://codereview.chromium.org/2616403003/diff/140001/cc/surfaces/sequence_s... File cc/surfaces/sequence_surface_reference_factory.h (right): https://codereview.chromium.org/2616403003/diff/140001/cc/surfaces/sequence_s... cc/surfaces/sequence_surface_reference_factory.h:20: base::Closure CreateReference(SurfaceReferenceOwner* owner, On 2017/01/10 21:12:06, danakj (slow) wrote: > // SurfaceReferenceFactory implementation. Done. https://codereview.chromium.org/2616403003/diff/140001/cc/surfaces/surface_re... File cc/surfaces/surface_reference_factory.h (right): https://codereview.chromium.org/2616403003/diff/140001/cc/surfaces/surface_re... cc/surfaces/surface_reference_factory.h:21: // a closure that can be called to remove the reference. On 2017/01/10 21:12:06, danakj (slow) wrote: > "must be called"? It can only be called a single time also right? Done. https://codereview.chromium.org/2616403003/diff/140001/cc/surfaces/surface_re... cc/surfaces/surface_reference_factory.h:25: SurfaceReferenceFactory() = default; On 2017/01/10 21:12:06, danakj (slow) wrote: > why does this class have a constructor? "call to implicitly-deleted default constructor of 'cc::SequenceSurfaceReferenceFactory'" I guess because of DISALLOW_COPY_AND_ASSIGN?
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2616403003/diff/140001/cc/surfaces/surface_re... File cc/surfaces/surface_reference_factory.h (right): https://codereview.chromium.org/2616403003/diff/140001/cc/surfaces/surface_re... cc/surfaces/surface_reference_factory.h:25: SurfaceReferenceFactory() = default; On 2017/01/10 21:56:14, Saman Sami wrote: > On 2017/01/10 21:12:06, danakj (slow) wrote: > > why does this class have a constructor? > > "call to implicitly-deleted default constructor of > 'cc::SequenceSurfaceReferenceFactory'" I guess because of > DISALLOW_COPY_AND_ASSIGN? Oh, there is no need for DISALLOW_COPY_AND_ASSIGN for an interface IMO. I'd drop both and keep the smallest LOC possible for the interface - just the virtual methods. It should be the subclass that is refcounted (tho better to not use at all) also, not the interface. There are some cases where you have to refcount the interface because of multiple inheritance problems but I don't think this is one. Maybe we can improve this separately. https://codereview.chromium.org/2616403003/diff/160001/cc/layers/surface_laye... File cc/layers/surface_layer.cc (right): https://codereview.chromium.org/2616403003/diff/160001/cc/layers/surface_laye... cc/layers/surface_layer.cc:113: layer_tree_host()->GetTaskRunnerProvider()->MainThreadTaskRunner()); One question: If the layer is not in a tree at the time SetSurfaceInfo/SetLayerTreeHost is called, there's no tree to get a task runner from. But then also there's no current reference to remove. Should this early out?
https://codereview.chromium.org/2616403003/diff/160001/cc/layers/surface_laye... File cc/layers/surface_layer.cc (right): https://codereview.chromium.org/2616403003/diff/160001/cc/layers/surface_laye... cc/layers/surface_layer.cc:113: layer_tree_host()->GetTaskRunnerProvider()->MainThreadTaskRunner()); On 2017/01/11 15:25:28, danakj (slow) wrote: > One question: If the layer is not in a tree at the time > SetSurfaceInfo/SetLayerTreeHost is called, there's no tree to get a task runner > from. But then also there's no current reference to remove. Should this early > out? Of course, the line below would crash also. I guess there are no users of SetSurfaceInfo when SurfaceLayer is not in a tree right now. We should add an early out and unit test this scenario. This patch LGTM tho.
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL I created the unit test. https://codereview.chromium.org/2616403003/diff/140001/cc/surfaces/surface_re... File cc/surfaces/surface_reference_factory.h (right): https://codereview.chromium.org/2616403003/diff/140001/cc/surfaces/surface_re... cc/surfaces/surface_reference_factory.h:25: SurfaceReferenceFactory() = default; On 2017/01/11 15:25:28, danakj (slow) wrote: > On 2017/01/10 21:56:14, Saman Sami wrote: > > On 2017/01/10 21:12:06, danakj (slow) wrote: > > > why does this class have a constructor? > > > > "call to implicitly-deleted default constructor of > > 'cc::SequenceSurfaceReferenceFactory'" I guess because of > > DISALLOW_COPY_AND_ASSIGN? > > Oh, there is no need for DISALLOW_COPY_AND_ASSIGN for an interface IMO. I'd drop > both and keep the smallest LOC possible for the interface - just the virtual > methods. It should be the subclass that is refcounted (tho better to not use at > all) also, not the interface. There are some cases where you have to refcount > the interface because of multiple inheritance problems but I don't think this is > one. Maybe we can improve this separately. But we have scoped_refptr<SurfaceReferenceFactory>. Doesn't that mean it should be ref counted? https://codereview.chromium.org/2616403003/diff/160001/cc/layers/surface_laye... File cc/layers/surface_layer.cc (right): https://codereview.chromium.org/2616403003/diff/160001/cc/layers/surface_laye... cc/layers/surface_layer.cc:113: layer_tree_host()->GetTaskRunnerProvider()->MainThreadTaskRunner()); On 2017/01/11 15:40:38, danakj (slow) wrote: > On 2017/01/11 15:25:28, danakj (slow) wrote: > > One question: If the layer is not in a tree at the time > > SetSurfaceInfo/SetLayerTreeHost is called, there's no tree to get a task > runner > > from. But then also there's no current reference to remove. Should this early > > out? > > Of course, the line below would crash also. I guess there are no users of > SetSurfaceInfo when SurfaceLayer is not in a tree right now. We should add an > early out and unit test this scenario. > > This patch LGTM tho. The reference would have never existed if there was no layer tree host. We check that there actually exists a reference at the very beginning.
On 2017/01/11 16:19:16, Saman Sami wrote: > PTAL > > I created the unit test. > > https://codereview.chromium.org/2616403003/diff/140001/cc/surfaces/surface_re... > File cc/surfaces/surface_reference_factory.h (right): > > https://codereview.chromium.org/2616403003/diff/140001/cc/surfaces/surface_re... > cc/surfaces/surface_reference_factory.h:25: SurfaceReferenceFactory() = default; > On 2017/01/11 15:25:28, danakj (slow) wrote: > > On 2017/01/10 21:56:14, Saman Sami wrote: > > > On 2017/01/10 21:12:06, danakj (slow) wrote: > > > > why does this class have a constructor? > > > > > > "call to implicitly-deleted default constructor of > > > 'cc::SequenceSurfaceReferenceFactory'" I guess because of > > > DISALLOW_COPY_AND_ASSIGN? > > > > Oh, there is no need for DISALLOW_COPY_AND_ASSIGN for an interface IMO. I'd > drop > > both and keep the smallest LOC possible for the interface - just the virtual > > methods. It should be the subclass that is refcounted (tho better to not use > at > > all) also, not the interface. There are some cases where you have to refcount > > the interface because of multiple inheritance problems but I don't think this > is > > one. Maybe we can improve this separately. > > But we have scoped_refptr<SurfaceReferenceFactory>. Doesn't that mean it should > be ref counted? Oh I see, normally we'd have scoped_refptr of the subclass I guess. In that case you're right. > https://codereview.chromium.org/2616403003/diff/160001/cc/layers/surface_laye... > File cc/layers/surface_layer.cc (right): > > https://codereview.chromium.org/2616403003/diff/160001/cc/layers/surface_laye... > cc/layers/surface_layer.cc:113: > layer_tree_host()->GetTaskRunnerProvider()->MainThreadTaskRunner()); > On 2017/01/11 15:40:38, danakj (slow) wrote: > > On 2017/01/11 15:25:28, danakj (slow) wrote: > > > One question: If the layer is not in a tree at the time > > > SetSurfaceInfo/SetLayerTreeHost is called, there's no tree to get a task > > runner > > > from. But then also there's no current reference to remove. Should this > early > > > out? > > > > Of course, the line below would crash also. I guess there are no users of > > SetSurfaceInfo when SurfaceLayer is not in a tree right now. We should add an > > early out and unit test this scenario. > > > > This patch LGTM tho. > > The reference would have never existed if there was no layer tree host. We check > that there actually exists a reference at the very beginning. Agree!
Thanks for the test LGTM https://codereview.chromium.org/2616403003/diff/180001/base/callback_unittest.cc File base/callback_unittest.cc (right): https://codereview.chromium.org/2616403003/diff/180001/base/callback_unittest... base/callback_unittest.cc:119: // Moving should reset the callback comments get periods
The CQ bit was checked by samans@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jbauman@chromium.org, fsamuel@chromium.org, danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2616403003/#ps200001 (title: "dot")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for cc/layers/surface_layer.cc:
While running git apply --index -p1;
error: patch failed: cc/layers/surface_layer.cc:67
error: cc/layers/surface_layer.cc: patch does not apply
Patch: cc/layers/surface_layer.cc
Index: cc/layers/surface_layer.cc
diff --git a/cc/layers/surface_layer.cc b/cc/layers/surface_layer.cc
index
a849589c08e06194597335fdcbcd03162a267bdd..6b9f514e6e8897a6bbd7c94eb6340888502e13c8
100644
--- a/cc/layers/surface_layer.cc
+++ b/cc/layers/surface_layer.cc
@@ -8,22 +8,22 @@
#include "base/macros.h"
#include "base/memory/ptr_util.h"
-#include "base/threading/thread_task_runner_handle.h"
#include "base/trace_event/trace_event.h"
#include "cc/layers/surface_layer_impl.h"
#include "cc/output/swap_promise.h"
#include "cc/surfaces/surface_sequence_generator.h"
#include "cc/trees/layer_tree_host.h"
#include "cc/trees/swap_promise_manager.h"
+#include "cc/trees/task_runner_provider.h"
namespace cc {
class SatisfySwapPromise : public SwapPromise {
public:
SatisfySwapPromise(
- std::unique_ptr<SurfaceReferenceBase> surface_ref,
+ base::Closure reference_returner,
scoped_refptr<base::SingleThreadTaskRunner> main_task_runner)
- : surface_ref_(std::move(surface_ref)),
+ : reference_returner_(reference_returner),
main_task_runner_(std::move(main_task_runner)) {}
~SatisfySwapPromise() override {}
@@ -34,17 +34,17 @@ class SatisfySwapPromise : public SwapPromise {
void WillSwap(CompositorFrameMetadata* metadata) override {}
void DidSwap() override {
- main_task_runner_->DeleteSoon(FROM_HERE, surface_ref_.release());
+ main_task_runner_->PostTask(FROM_HERE, reference_returner_);
}
DidNotSwapAction DidNotSwap(DidNotSwapReason reason) override {
- main_task_runner_->DeleteSoon(FROM_HERE, surface_ref_.release());
+ main_task_runner_->PostTask(FROM_HERE, reference_returner_);
return DidNotSwapAction::BREAK_PROMISE;
}
int64_t TraceId() const override { return 0; }
- std::unique_ptr<SurfaceReferenceBase> surface_ref_;
+ base::Closure reference_returner_;
scoped_refptr<base::SingleThreadTaskRunner> main_task_runner_;
DISALLOW_COPY_AND_ASSIGN(SatisfySwapPromise);
@@ -67,7 +67,7 @@ void SurfaceLayer::SetSurfaceInfo(const SurfaceInfo&
surface_info,
RemoveCurrentReference();
surface_info_ = surface_info;
if (layer_tree_host()) {
- current_ref_ =
+ reference_returner_ =
ref_factory_->CreateReference(layer_tree_host(), surface_info_.id());
}
stretch_content_to_fill_bounds_ = stretch_content_to_fill_bounds;
@@ -92,7 +92,7 @@ void SurfaceLayer::SetLayerTreeHost(LayerTreeHost* host) {
RemoveCurrentReference();
Layer::SetLayerTreeHost(host);
if (layer_tree_host()) {
- current_ref_ =
+ reference_returner_ =
ref_factory_->CreateReference(layer_tree_host(), surface_info_.id());
}
}
@@ -106,10 +106,11 @@ void SurfaceLayer::PushPropertiesTo(LayerImpl* layer) {
}
void SurfaceLayer::RemoveCurrentReference() {
- if (!current_ref_)
+ if (!reference_returner_)
return;
auto swap_promise = base::MakeUnique<SatisfySwapPromise>(
- std::move(current_ref_), base::ThreadTaskRunnerHandle::Get());
+ std::move(reference_returner_),
+ layer_tree_host()->GetTaskRunnerProvider()->MainThreadTaskRunner());
layer_tree_host()->GetSwapPromiseManager()->QueueSwapPromise(
std::move(swap_promise));
}
The CQ bit was checked by samans@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, fsamuel@chromium.org, sadrul@chromium.org, jbauman@chromium.org Link to the patchset: https://codereview.chromium.org/2616403003/#ps160002 (title: "c")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160002, "attempt_start_ts": 1484158181171880,
"parent_rev": "e7419b3ea146e6aa9ba26ec3fae255754f47cafb", "commit_rev":
"6c267ca6a4bc8f207d58a682a4c98f0b9b697ad4"}
Message was sent while issue was closed.
Description was changed from ========== Replacing SurfaceReferenceBase and SequenceSurfaceReference with Closures The sole purpose of SurfaceReferenceBase and SequenceSurfaceReference was to be able to return the reference later on, but the same goal can be achieved using closures with much less complexity. From now on, SurfaceReferenceFactory::CreateReference returns a closure that returns the reference once its called. TBR=sadrul@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Replacing SurfaceReferenceBase and SequenceSurfaceReference with Closures The sole purpose of SurfaceReferenceBase and SequenceSurfaceReference was to be able to return the reference later on, but the same goal can be achieved using closures with much less complexity. From now on, SurfaceReferenceFactory::CreateReference returns a closure that returns the reference once its called. TBR=sadrul@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2616403003 Cr-Commit-Position: refs/heads/master@{#442984} Committed: https://chromium.googlesource.com/chromium/src/+/6c267ca6a4bc8f207d58a682a4c9... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:160002) as https://chromium.googlesource.com/chromium/src/+/6c267ca6a4bc8f207d58a682a4c9... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
