Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2485473003:
Remove SurfaceFactory::Create and SurfaceFactory::Destroy (Closed)
|
Created:
4 years, 1 month ago by Saman Sami Modified:
4 years ago CC:
anandc+watch-blimp_chromium.org, bgoldman+watch-blimp_chromium.org, cc-bugs_chromium.org, chromium-reviews, creis+watch_chromium.org, danakj+watch_chromium.org, darin-cc_chromium.org, dtrainor+watch-blimp_chromium.org, gcasto+watch-blimp_chromium.org, jam, jbauman+watch_chromium.org, kalyank, khushalsagar+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, lethalantidote+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, miu+watch_chromium.org, nasko+codewatch_chromium.org, nyquist+watch-blimp_chromium.org, perumaal+watch-blimp_chromium.org, piman+watch_chromium.org, rjkroege, scf+watch-blimp_chromium.org, shaktisahu+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org, steimel+watch-blimp_chromium.org, xjz+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove SurfaceFactory::Create and SurfaceFactory::Destroy
It is no longer possible to explicitly construct and destroy surfaces. A
SurfaceFactory instance now handles only one surface at a time. Once the
local frame id passed to SubmitCompositorFrame changes, the factory gets
rid of the old surface and creates a new one.
BUG=658607
Committed: https://crrev.com/b902fd366ba91757dbee99d5f821f00d2de1d181
Cr-Commit-Position: refs/heads/master@{#432312}
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Committed: https://crrev.com/46b4f407905420c8997e5c886baef4bbf5248b79
Cr-Commit-Position: refs/heads/master@{#434724}
Patch Set 1 #Patch Set 2 : Android compile error + fixed content/ #Patch Set 3 : Android compile error #Patch Set 4 : updated #Patch Set 5 : up #Patch Set 6 : rebase #Patch Set 7 : up #Patch Set 8 : rebase #Patch Set 9 : up #
Total comments: 16
Patch Set 10 : Up #Patch Set 11 : rebase #Patch Set 12 : up #Patch Set 13 : Fixed render widget #Patch Set 14 : fixed exo #Patch Set 15 : rebase #
Total comments: 4
Patch Set 16 : fixed optional #Patch Set 17 : Fixed destructors #Patch Set 18 : up #Patch Set 19 : up #
Total comments: 23
Patch Set 20 : rename #Patch Set 21 : rebase #
Total comments: 3
Patch Set 22 : Fixed compile error #Patch Set 23 : rebase + client #Patch Set 24 : android compile error #Patch Set 25 : remove optional #Patch Set 26 : Fixed cc_unittests #Patch Set 27 : surface factory test clean up #
Total comments: 7
Patch Set 28 : SetPreviousFrame lint #
Total comments: 20
Patch Set 29 : Get rid of map Remove RequestCopyOfSurface first arg #Patch Set 30 : compiler error + evictframe #Patch Set 31 : evict frame #Patch Set 32 : blimp compile error #Patch Set 33 : nit #Patch Set 34 : more evict #
Total comments: 32
Patch Set 35 : up #Patch Set 36 : dcheck #Patch Set 37 : perftest #Patch Set 38 : doc #Patch Set 39 : doc #
Total comments: 16
Patch Set 40 : up #
Total comments: 1
Patch Set 41 : rebase #Patch Set 42 : compile error #Patch Set 43 : test #Patch Set 44 : test #Patch Set 45 : fixed windows compile error #
Total comments: 2
Patch Set 46 : up #Messages
Total messages: 258 (205 generated)
Description was changed from ========== Remove SurfaceFactory::Create and SurfaceFactory::Destroy BUG=658607 ========== to ========== Remove SurfaceFactory::Create and SurfaceFactory::Destroy BUG=658607 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Remove SurfaceFactory::Create and SurfaceFactory::Destroy BUG=658607 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Remove SurfaceFactory::Create and SurfaceFactory::Destroy BUG=658607 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_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...
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...)
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 unchecked by commit-bot@chromium.org
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...)
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 unchecked by commit-bot@chromium.org
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...)
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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
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 unchecked by commit-bot@chromium.org
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...)
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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_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...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
fsamuel@chromium.org changed reviewers: + fsamuel@chromium.org
https://codereview.chromium.org/2485473003/diff/160001/cc/surfaces/surface_fa... File cc/surfaces/surface_factory.cc (right): https://codereview.chromium.org/2485473003/diff/160001/cc/surfaces/surface_fa... cc/surfaces/surface_factory.cc:124: if (client_) Is this necessary? https://codereview.chromium.org/2485473003/diff/160001/cc/surfaces/surface_ma... File cc/surfaces/surface_manager_ref_unittest.cc (right): https://codereview.chromium.org/2485473003/diff/160001/cc/surfaces/surface_ma... cc/surfaces/surface_manager_ref_unittest.cc:34: SurfaceId CreateSurface(const FrameSinkId& frame_sink_id, This method doesn't really make sense. https://codereview.chromium.org/2485473003/diff/160001/cc/surfaces/surface_ma... cc/surfaces/surface_manager_ref_unittest.cc:43: void DestroySurface(const SurfaceId& surface_id) { This method doesn't really make sense. https://codereview.chromium.org/2485473003/diff/160001/cc/surfaces/surface_re... File cc/surfaces/surface_resource_holder.cc (right): https://codereview.chromium.org/2485473003/diff/160001/cc/surfaces/surface_re... cc/surfaces/surface_resource_holder.cc:74: if (client_) Is this necessary? https://codereview.chromium.org/2485473003/diff/160001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2485473003/diff/160001/components/exo/surface... components/exo/surface.cc:478: factory_owner_->surface_factory_->SubmitCompositorFrame( Is this necessary? https://codereview.chromium.org/2485473003/diff/160001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_child_frame.cc (right): https://codereview.chromium.org/2485473003/diff/160001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_child_frame.cc:405: surface_factory_->SubmitCompositorFrame(local_frame_id_, Is this necessary? Just add the destruction dependency AFTER SubmitCompositorFrame below. https://codereview.chromium.org/2485473003/diff/160001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_guest.cc (right): https://codereview.chromium.org/2485473003/diff/160001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_guest.cc:311: surface_factory_->SubmitCompositorFrame(local_frame_id_, I don't think this approach is correct. We shouldn't be submitting empty compositorframes unless absolutely necessary. Move the destruction dependency code after SubmitCompositorFrame. https://codereview.chromium.org/2485473003/diff/160001/content/browser/render... File content/browser/renderer_host/delegated_frame_host.cc (right): https://codereview.chromium.org/2485473003/diff/160001/content/browser/render... content/browser/renderer_host/delegated_frame_host.cc:482: surface_factory_->SubmitCompositorFrame( Remove this?
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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) blimp_linux_dbg on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/blimp_linux_dbg...)
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 unchecked by commit-bot@chromium.org
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...)
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 unchecked by commit-bot@chromium.org
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_...)
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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
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 unchecked by samans@chromium.org
Patchset #17 (id:320001) has been deleted
Patchset #16 (id:300001) has been deleted
Patchset #16 (id:340001) has been deleted
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 unchecked by commit-bot@chromium.org
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...)
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...
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...
Patchset #20 (id:440001) has been deleted
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 unchecked by commit-bot@chromium.org
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...)
Patchset #20 (id:460001) has been deleted
Description was changed from ========== Remove SurfaceFactory::Create and SurfaceFactory::Destroy BUG=658607 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Remove SurfaceFactory::Create and SurfaceFactory::Destroy It is no longer possible to explicitly construct and destroy surfaces. A SurfaceFactory instance now handles only one surface at a time. Once the local frame id passed to SubmitCompositorFrame changes, it gets rid of the old surface and creates a new one. BUG=658607 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
Description was changed from ========== Remove SurfaceFactory::Create and SurfaceFactory::Destroy BUG=658607 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Remove SurfaceFactory::Create and SurfaceFactory::Destroy It is no longer possible to explicitly construct and destroy surfaces. A SurfaceFactory instance now handles only one surface at a time. Once the local frame id passed to SubmitCompositorFrame changes, it gets rid of the old surface and creates a new one. BUG=658607 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
Description was changed from ========== Remove SurfaceFactory::Create and SurfaceFactory::Destroy It is no longer possible to explicitly construct and destroy surfaces. A SurfaceFactory instance now handles only one surface at a time. Once the local frame id passed to SubmitCompositorFrame changes, it gets rid of the old surface and creates a new one. BUG=658607 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Remove SurfaceFactory::Create and SurfaceFactory::Destroy It is no longer possible to explicitly construct and destroy surfaces. A SurfaceFactory instance now handles only one surface at a time. Once the local frame id passed to SubmitCompositorFrame changes, the factory gets rid of the old surface and creates a new one. BUG=658607 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
samans@chromium.org changed reviewers: + boliu@chromium.org, danakj@chromium.org
samans@chromium.org changed reviewers: + dtrainor@chromium.org
samans@chromium.org changed reviewers: + piman@chromium.org, sky@chromium.org
samans@chromium.org changed reviewers: - sky@chromium.org
samans@chromium.org changed reviewers: + sky@chromium.org
dtrainor@ for blimp/ danakj@ for cc/ piman@ for content/ boliu@ for android_webview/ and content/renderer/android/ sky@ for services/ and ui/
blimp/ lgtm
https://codereview.chromium.org/2485473003/diff/280001/cc/surfaces/surface_fa... File cc/surfaces/surface_factory.h (right): https://codereview.chromium.org/2485473003/diff/280001/cc/surfaces/surface_fa... cc/surfaces/surface_factory.h:93: base::Optional<LocalFrameId> last_local_frame_id_; why is this optional? If last_local_frame_id_ isn't valid then you haven't received one. https://codereview.chromium.org/2485473003/diff/280001/cc/surfaces/surface_re... File cc/surfaces/surface_resource_holder.cc (right): https://codereview.chromium.org/2485473003/diff/280001/cc/surfaces/surface_re... cc/surfaces/surface_resource_holder.cc:74: if (client_) Why add if (client_)? https://codereview.chromium.org/2485473003/diff/420001/cc/surfaces/direct_com... File cc/surfaces/direct_compositor_frame_sink.cc (right): https://codereview.chromium.org/2485473003/diff/420001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.cc:141: if (client_) nit: is this check necessary? https://codereview.chromium.org/2485473003/diff/420001/cc/surfaces/surface_fa... File cc/surfaces/surface_factory.cc (right): https://codereview.chromium.org/2485473003/diff/420001/cc/surfaces/surface_fa... cc/surfaces/surface_factory.cc:124: if (client_) Is this check necessary? https://codereview.chromium.org/2485473003/diff/420001/cc/surfaces/surface_fa... File cc/surfaces/surface_factory.h (right): https://codereview.chromium.org/2485473003/diff/420001/cc/surfaces/surface_fa... cc/surfaces/surface_factory.h:91: base::Optional<LocalFrameId> last_local_frame_id_; Why is this optional? I don't think this needs to be a base::Optional. https://codereview.chromium.org/2485473003/diff/420001/cc/surfaces/surface_fa... File cc/surfaces/surface_factory_unittest.cc (right): https://codereview.chromium.org/2485473003/diff/420001/cc/surfaces/surface_fa... cc/surfaces/surface_factory_unittest.cc:480: factory2->SubmitCompositorFrame(LocalFrameId(0, 0), CompositorFrame(), If staraz@ lands his patch first then this will not work. LocalFrameId will be invalid and I think this test will fail. https://codereview.chromium.org/2485473003/diff/420001/cc/surfaces/surface_re... File cc/surfaces/surface_resource_holder.cc (right): https://codereview.chromium.org/2485473003/diff/420001/cc/surfaces/surface_re... cc/surfaces/surface_resource_holder.cc:74: if (client_) Is this check necessary? https://codereview.chromium.org/2485473003/diff/420001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_child_frame.cc (right): https://codereview.chromium.org/2485473003/diff/420001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_child_frame.cc:401: bool was_null = false; rename to allocated_new_local_frame_id https://codereview.chromium.org/2485473003/diff/420001/content/browser/render... File content/browser/renderer_host/delegated_frame_host.cc (right): https://codereview.chromium.org/2485473003/diff/420001/content/browser/render... content/browser/renderer_host/delegated_frame_host.cc:479: bool new_surface = false; allocated_new_local_frame_id
LGTM
https://codereview.chromium.org/2485473003/diff/420001/cc/surfaces/direct_com... File cc/surfaces/direct_compositor_frame_sink.cc (right): https://codereview.chromium.org/2485473003/diff/420001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.cc:141: if (client_) Should we remove ourselves somehow in DetachFromClient instead? https://codereview.chromium.org/2485473003/diff/420001/cc/surfaces/surface_fa... File cc/surfaces/surface_factory.cc (left): https://codereview.chromium.org/2485473003/diff/420001/cc/surfaces/surface_fa... cc/surfaces/surface_factory.cc:29: if (!surface_map_.empty()) { Can we enforce this still? https://codereview.chromium.org/2485473003/diff/420001/cc/surfaces/surface_fa... File cc/surfaces/surface_factory.cc (right): https://codereview.chromium.org/2485473003/diff/420001/cc/surfaces/surface_fa... cc/surfaces/surface_factory.cc:106: last_local_frame_id_ = local_frame_id; instead of last_ maybe this is current_ ? last sounds like previous and seems misleading here https://codereview.chromium.org/2485473003/diff/420001/cc/surfaces/surface_fa... File cc/surfaces/surface_factory.h (right): https://codereview.chromium.org/2485473003/diff/420001/cc/surfaces/surface_fa... cc/surfaces/surface_factory.h:91: base::Optional<LocalFrameId> last_local_frame_id_; On 2016/11/10 23:33:37, Fady Samuel wrote: > Why is this optional? I don't think this needs to be a base::Optional. Same, just have an empty/default id? https://codereview.chromium.org/2485473003/diff/420001/cc/surfaces/surface_fa... File cc/surfaces/surface_factory_unittest.cc (right): https://codereview.chromium.org/2485473003/diff/420001/cc/surfaces/surface_fa... cc/surfaces/surface_factory_unittest.cc:480: factory2->SubmitCompositorFrame(LocalFrameId(0, 0), CompositorFrame(), On 2016/11/10 23:33:37, Fady Samuel wrote: > If staraz@ lands his patch first then this will not work. LocalFrameId will be > invalid and I think this test will fail. Also if you use 0s to signify nothing instead of Optional. Non-zeros here?
https://codereview.chromium.org/2485473003/diff/420001/cc/surfaces/surface_fa... File cc/surfaces/surface_factory.cc (left): https://codereview.chromium.org/2485473003/diff/420001/cc/surfaces/surface_fa... cc/surfaces/surface_factory.cc:29: if (!surface_map_.empty()) { On 2016/11/10 23:57:58, danakj wrote: > Can we enforce this still? Why? What does that solve? This CL eliminates explicit Create/Destroy in order to better match the CompositorFrameSink API. The last surface will be destroyed here.
https://codereview.chromium.org/2485473003/diff/420001/cc/surfaces/surface_fa... File cc/surfaces/surface_factory.cc (left): https://codereview.chromium.org/2485473003/diff/420001/cc/surfaces/surface_fa... cc/surfaces/surface_factory.cc:29: if (!surface_map_.empty()) { On 2016/11/10 23:57:58, danakj wrote: > Can we enforce this still? Why? What does that solve? This CL eliminates explicit Create/Destroy in order to better match the CompositorFrameSink API. The last surface will be destroyed here.
https://codereview.chromium.org/2485473003/diff/420001/cc/surfaces/surface_fa... File cc/surfaces/surface_factory.cc (left): https://codereview.chromium.org/2485473003/diff/420001/cc/surfaces/surface_fa... cc/surfaces/surface_factory.cc:29: if (!surface_map_.empty()) { On 2016/11/11 00:03:01, Fady Samuel wrote: > On 2016/11/10 23:57:58, danakj wrote: > > Can we enforce this still? > > Why? What does that solve? This CL eliminates explicit Create/Destroy in order > to better match the CompositorFrameSink API. The last surface will be destroyed > here. Cuz this is why there's all the null checks unless I'm misunderstanding.
On 2016/11/11 00:10:47, danakj wrote: > https://codereview.chromium.org/2485473003/diff/420001/cc/surfaces/surface_fa... > File cc/surfaces/surface_factory.cc (left): > > https://codereview.chromium.org/2485473003/diff/420001/cc/surfaces/surface_fa... > cc/surfaces/surface_factory.cc:29: if (!surface_map_.empty()) { > On 2016/11/11 00:03:01, Fady Samuel wrote: > > On 2016/11/10 23:57:58, danakj wrote: > > > Can we enforce this still? > > > > Why? What does that solve? This CL eliminates explicit Create/Destroy in order > > to better match the CompositorFrameSink API. The last surface will be > destroyed > > here. > > Cuz this is why there's all the null checks unless I'm misunderstanding. I just chatted with Saman. I understand now. The problem is We UnregisterSurfaceFactoryClient before we get rid of SurfaceFactory. I think SurfaceFactory can be Reset prior to Unregister.
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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_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...
jbauman@chromium.org changed reviewers: + jbauman@chromium.org
https://codereview.chromium.org/2485473003/diff/500001/content/browser/render... File content/browser/renderer_host/delegated_frame_host.cc (right): https://codereview.chromium.org/2485473003/diff/500001/content/browser/render... content/browser/renderer_host/delegated_frame_host.cc:596: if (local_frame_id_.is_valid()) { If you remove the destroy, in what sense is the frame evicted?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_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...
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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
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 https://codereview.chromium.org/2485473003/diff/160001/cc/surfaces/surface_fa... File cc/surfaces/surface_factory.cc (right): https://codereview.chromium.org/2485473003/diff/160001/cc/surfaces/surface_fa... cc/surfaces/surface_factory.cc:124: if (client_) On 2016/11/08 22:23:59, Fady Samuel wrote: > Is this necessary? Removed https://codereview.chromium.org/2485473003/diff/160001/cc/surfaces/surface_ma... File cc/surfaces/surface_manager_ref_unittest.cc (right): https://codereview.chromium.org/2485473003/diff/160001/cc/surfaces/surface_ma... cc/surfaces/surface_manager_ref_unittest.cc:34: SurfaceId CreateSurface(const FrameSinkId& frame_sink_id, On 2016/11/08 22:23:59, Fady Samuel wrote: > This method doesn't really make sense. Since the code never sends a frame, I have to do this. https://codereview.chromium.org/2485473003/diff/160001/cc/surfaces/surface_ma... cc/surfaces/surface_manager_ref_unittest.cc:43: void DestroySurface(const SurfaceId& surface_id) { On 2016/11/08 22:23:59, Fady Samuel wrote: > This method doesn't really make sense. Same https://codereview.chromium.org/2485473003/diff/160001/cc/surfaces/surface_re... File cc/surfaces/surface_resource_holder.cc (right): https://codereview.chromium.org/2485473003/diff/160001/cc/surfaces/surface_re... cc/surfaces/surface_resource_holder.cc:74: if (client_) On 2016/11/08 22:23:59, Fady Samuel wrote: > Is this necessary? Removed https://codereview.chromium.org/2485473003/diff/160001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2485473003/diff/160001/components/exo/surface... components/exo/surface.cc:478: factory_owner_->surface_factory_->SubmitCompositorFrame( On 2016/11/08 22:23:59, Fady Samuel wrote: > Is this necessary? Removed https://codereview.chromium.org/2485473003/diff/160001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_child_frame.cc (right): https://codereview.chromium.org/2485473003/diff/160001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_child_frame.cc:405: surface_factory_->SubmitCompositorFrame(local_frame_id_, On 2016/11/08 22:23:59, Fady Samuel wrote: > Is this necessary? Just add the destruction dependency AFTER > SubmitCompositorFrame below. Done. https://codereview.chromium.org/2485473003/diff/160001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_guest.cc (right): https://codereview.chromium.org/2485473003/diff/160001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_guest.cc:311: surface_factory_->SubmitCompositorFrame(local_frame_id_, On 2016/11/08 22:23:59, Fady Samuel wrote: > I don't think this approach is correct. We shouldn't be submitting empty > compositorframes unless absolutely necessary. Move the destruction dependency > code after SubmitCompositorFrame. Done. https://codereview.chromium.org/2485473003/diff/160001/content/browser/render... File content/browser/renderer_host/delegated_frame_host.cc (right): https://codereview.chromium.org/2485473003/diff/160001/content/browser/render... content/browser/renderer_host/delegated_frame_host.cc:482: surface_factory_->SubmitCompositorFrame( On 2016/11/08 22:23:59, Fady Samuel wrote: > Remove this? Done. https://codereview.chromium.org/2485473003/diff/280001/cc/surfaces/surface_fa... File cc/surfaces/surface_factory.h (right): https://codereview.chromium.org/2485473003/diff/280001/cc/surfaces/surface_fa... cc/surfaces/surface_factory.h:93: base::Optional<LocalFrameId> last_local_frame_id_; On 2016/11/10 23:33:36, Fady Samuel wrote: > why is this optional? If last_local_frame_id_ isn't valid then you haven't > received one. Done. https://codereview.chromium.org/2485473003/diff/280001/cc/surfaces/surface_re... File cc/surfaces/surface_resource_holder.cc (right): https://codereview.chromium.org/2485473003/diff/280001/cc/surfaces/surface_re... cc/surfaces/surface_resource_holder.cc:74: if (client_) On 2016/11/10 23:33:36, Fady Samuel wrote: > Why add if (client_)? Removed https://codereview.chromium.org/2485473003/diff/420001/cc/surfaces/direct_com... File cc/surfaces/direct_compositor_frame_sink.cc (right): https://codereview.chromium.org/2485473003/diff/420001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.cc:141: if (client_) On 2016/11/10 23:57:58, danakj wrote: > Should we remove ourselves somehow in DetachFromClient instead? Done. https://codereview.chromium.org/2485473003/diff/420001/cc/surfaces/surface_fa... File cc/surfaces/surface_factory.cc (left): https://codereview.chromium.org/2485473003/diff/420001/cc/surfaces/surface_fa... cc/surfaces/surface_factory.cc:29: if (!surface_map_.empty()) { On 2016/11/11 00:10:47, danakj wrote: > On 2016/11/11 00:03:01, Fady Samuel wrote: > > On 2016/11/10 23:57:58, danakj wrote: > > > Can we enforce this still? > > > > Why? What does that solve? This CL eliminates explicit Create/Destroy in order > > to better match the CompositorFrameSink API. The last surface will be > destroyed > > here. > > Cuz this is why there's all the null checks unless I'm misunderstanding. I got rid of the null checks. I still don't have this error message in my last patch. https://codereview.chromium.org/2485473003/diff/420001/cc/surfaces/surface_fa... File cc/surfaces/surface_factory.cc (right): https://codereview.chromium.org/2485473003/diff/420001/cc/surfaces/surface_fa... cc/surfaces/surface_factory.cc:106: last_local_frame_id_ = local_frame_id; On 2016/11/10 23:57:58, danakj wrote: > instead of last_ maybe this is current_ ? last sounds like previous and seems > misleading here Done. https://codereview.chromium.org/2485473003/diff/420001/cc/surfaces/surface_fa... cc/surfaces/surface_factory.cc:124: if (client_) On 2016/11/10 23:33:37, Fady Samuel wrote: > Is this check necessary? Removed https://codereview.chromium.org/2485473003/diff/420001/cc/surfaces/surface_fa... File cc/surfaces/surface_factory.h (right): https://codereview.chromium.org/2485473003/diff/420001/cc/surfaces/surface_fa... cc/surfaces/surface_factory.h:91: base::Optional<LocalFrameId> last_local_frame_id_; On 2016/11/10 23:57:58, danakj wrote: > On 2016/11/10 23:33:37, Fady Samuel wrote: > > Why is this optional? I don't think this needs to be a base::Optional. > > Same, just have an empty/default id? Done. https://codereview.chromium.org/2485473003/diff/420001/cc/surfaces/surface_fa... File cc/surfaces/surface_factory_unittest.cc (right): https://codereview.chromium.org/2485473003/diff/420001/cc/surfaces/surface_fa... cc/surfaces/surface_factory_unittest.cc:480: factory2->SubmitCompositorFrame(LocalFrameId(0, 0), CompositorFrame(), On 2016/11/10 23:33:37, Fady Samuel wrote: > If staraz@ lands his patch first then this will not work. LocalFrameId will be > invalid and I think this test will fail. Done. https://codereview.chromium.org/2485473003/diff/420001/cc/surfaces/surface_re... File cc/surfaces/surface_resource_holder.cc (right): https://codereview.chromium.org/2485473003/diff/420001/cc/surfaces/surface_re... cc/surfaces/surface_resource_holder.cc:74: if (client_) On 2016/11/10 23:33:37, Fady Samuel wrote: > Is this check necessary? Removed https://codereview.chromium.org/2485473003/diff/420001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_child_frame.cc (right): https://codereview.chromium.org/2485473003/diff/420001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_child_frame.cc:401: bool was_null = false; On 2016/11/10 23:33:37, Fady Samuel wrote: > rename to allocated_new_local_frame_id Done. https://codereview.chromium.org/2485473003/diff/420001/content/browser/render... File content/browser/renderer_host/delegated_frame_host.cc (right): https://codereview.chromium.org/2485473003/diff/420001/content/browser/render... content/browser/renderer_host/delegated_frame_host.cc:479: bool new_surface = false; On 2016/11/10 23:33:37, Fady Samuel wrote: > allocated_new_local_frame_id Done. https://codereview.chromium.org/2485473003/diff/500001/content/browser/render... File content/browser/renderer_host/delegated_frame_host.cc (right): https://codereview.chromium.org/2485473003/diff/500001/content/browser/render... content/browser/renderer_host/delegated_frame_host.cc:596: if (local_frame_id_.is_valid()) { On 2016/11/11 03:23:20, jbauman wrote: > If you remove the destroy, in what sense is the frame evicted? Does it matter whether or not we get rid of the frame immediately? The tests seem to be doing fine. I can call Reset on surface factory if we need to. (Or maybe we should just change the name of this method?)
samans@chromium.org changed reviewers: + reveman@chromium.org
components/exo lgtm
On 2016/11/11 17:49:59, Saman Sami wrote: > https://codereview.chromium.org/2485473003/diff/500001/content/browser/render... > File content/browser/renderer_host/delegated_frame_host.cc (right): > > https://codereview.chromium.org/2485473003/diff/500001/content/browser/render... > content/browser/renderer_host/delegated_frame_host.cc:596: if > (local_frame_id_.is_valid()) { > On 2016/11/11 03:23:20, jbauman wrote: > > If you remove the destroy, in what sense is the frame evicted? > > Does it matter whether or not we get rid of the frame immediately? The tests > seem to be doing fine. I can call Reset on surface factory if we need to. (Or > maybe we should just change the name of this method?) If we don't get rid of the frame then evicted tabs will still hold onto their resources for a long time, which defeats the entire purpose of this mechanism. Calling Reset won't work as that invalidates weak pointers to the surface factory and prevents the renderer from being informed when the resources should be destroyed.
https://codereview.chromium.org/2485473003/diff/500001/content/browser/render... File content/browser/renderer_host/delegated_frame_host.cc (right): https://codereview.chromium.org/2485473003/diff/500001/content/browser/render... content/browser/renderer_host/delegated_frame_host.cc:596: if (local_frame_id_.is_valid()) { On 2016/11/11 17:49:59, Saman Sami wrote: > On 2016/11/11 03:23:20, jbauman wrote: > > If you remove the destroy, in what sense is the frame evicted? > > Does it matter whether or not we get rid of the frame immediately? The tests > seem to be doing fine. I can call Reset on surface factory if we need to. (Or > maybe we should just change the name of this method?) Maybe SurfaceFactory (which we eventually want to merge with "CompositorFrameSinkSupport"/GpuCompositorFrameSInk) can have a public EvictFrame() API that evicts the current frame if there is one.
https://codereview.chromium.org/2485473003/diff/620001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2485473003/diff/620001/components/exo/surface... components/exo/surface.cc:250: factory_owner_->surface_factory_->Reset(); This will prevent the resources from being returned at the right time. https://codereview.chromium.org/2485473003/diff/620001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_child_frame.cc (right): https://codereview.chromium.org/2485473003/diff/620001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_child_frame.cc:728: void RenderWidgetHostViewChildFrame::ClearCompositorSurfaceIfNecessary() { Same problem here (in the case where the guest is detached).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2485473003/diff/620001/cc/surfaces/surface_fa... File cc/surfaces/surface_factory.cc (right): https://codereview.chromium.org/2485473003/diff/620001/cc/surfaces/surface_fa... cc/surfaces/surface_factory.cc:32: void SurfaceFactory::DestroyAll() { Rename this to EvictFrame and make public. Btw, it just occurs to me that we probably don't need a surface_map_. https://codereview.chromium.org/2485473003/diff/620001/cc/surfaces/surface_fa... cc/surfaces/surface_factory.cc:107: local_frame_id != current_local_frame_id_) Add braces. Call SetPreviousFrameSurface here before destroy. https://codereview.chromium.org/2485473003/diff/620001/cc/surfaces/surface_fa... File cc/surfaces/surface_factory.h (right): https://codereview.chromium.org/2485473003/diff/620001/cc/surfaces/surface_fa... cc/surfaces/surface_factory.h:55: void SetPreviousFrameSurface(const LocalFrameId& new_id, Make this private.
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/2485473003/diff/640001/android_webview/browse... File android_webview/browser/surfaces_instance.cc (right): https://codereview.chromium.org/2485473003/diff/640001/android_webview/browse... android_webview/browser/surfaces_instance.cc:88: surface_factory_.reset(); surface_factory_->EvictFrame(); https://codereview.chromium.org/2485473003/diff/640001/cc/surfaces/direct_com... File cc/surfaces/direct_compositor_frame_sink.cc (right): https://codereview.chromium.org/2485473003/diff/640001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.cc:85: factory_.Reset(); factory_.EvictFrame(); https://codereview.chromium.org/2485473003/diff/640001/cc/surfaces/surface_fa... File cc/surfaces/surface_factory.h (left): https://codereview.chromium.org/2485473003/diff/640001/cc/surfaces/surface_fa... cc/surfaces/surface_factory.h:95: FrameSinkId frame_sink_id_; nit make const. https://codereview.chromium.org/2485473003/diff/640001/cc/surfaces/surface_fa... File cc/surfaces/surface_factory.h (right): https://codereview.chromium.org/2485473003/diff/640001/cc/surfaces/surface_fa... cc/surfaces/surface_factory.h:91: void Create(const LocalFrameId& local_frame_id); nit: style, we usually put methods above class variables. https://codereview.chromium.org/2485473003/diff/640001/cc/surfaces/surface_fa... cc/surfaces/surface_factory.h:93: void DestroyAll(); Rename DestroyAll() => EvictFrame() and make public. https://codereview.chromium.org/2485473003/diff/640001/cc/surfaces/surface_fa... cc/surfaces/surface_factory.h:97: void SetPreviousFrameSurface(const LocalFrameId& new_id, Try to make method order in the cc file match the order in the header file. https://codereview.chromium.org/2485473003/diff/640001/cc/surfaces/surface_fa... File cc/surfaces/surface_factory_unittest.cc (right): https://codereview.chromium.org/2485473003/diff/640001/cc/surfaces/surface_fa... cc/surfaces/surface_factory_unittest.cc:480: factory2->Reset(); EvictFrame() https://codereview.chromium.org/2485473003/diff/640001/cc/surfaces/surface_fa... cc/surfaces/surface_factory_unittest.cc:496: factory2->Reset(); EvictFrame() https://codereview.chromium.org/2485473003/diff/640001/cc/surfaces/surface_fa... cc/surfaces/surface_factory_unittest.cc:514: factory_->Reset(); EvictFrame() https://codereview.chromium.org/2485473003/diff/640001/cc/surfaces/surface_fa... cc/surfaces/surface_factory_unittest.cc:545: factory2->Reset(); EvictFrame() https://codereview.chromium.org/2485473003/diff/640001/cc/surfaces/surface_fa... cc/surfaces/surface_factory_unittest.cc:555: factory_->Reset(); EvictFrame() https://codereview.chromium.org/2485473003/diff/640001/cc/surfaces/surface_fa... cc/surfaces/surface_factory_unittest.cc:624: factory_->Reset(); EvictFrame() https://codereview.chromium.org/2485473003/diff/640001/cc/test/test_composito... File cc/test/test_compositor_frame_sink.cc (right): https://codereview.chromium.org/2485473003/diff/640001/cc/test/test_composito... cc/test/test_compositor_frame_sink.cc:114: surface_factory_->Reset(); EvictFrame() https://codereview.chromium.org/2485473003/diff/640001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2485473003/diff/640001/components/exo/surface... components/exo/surface.cc:250: factory_owner_->surface_factory_->Reset(); EvictFrame() https://codereview.chromium.org/2485473003/diff/640001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_child_frame.cc (left): https://codereview.chromium.org/2485473003/diff/640001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_child_frame.cc:730: surface_factory_->Destroy(local_frame_id_); surface_factory_->EvictFrame(); https://codereview.chromium.org/2485473003/diff/640001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_child_frame.cc (right): https://codereview.chromium.org/2485473003/diff/640001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_child_frame.cc:72: surface_factory_.reset(); surface_factory_->EvictFrame(); https://codereview.chromium.org/2485473003/diff/640001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_guest.cc (right): https://codereview.chromium.org/2485473003/diff/640001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_guest.cc:303: bool was_null = false; nit: allocated_new_local_frame_id https://codereview.chromium.org/2485473003/diff/640001/content/browser/render... File content/browser/renderer_host/delegated_frame_host.cc (right): https://codereview.chromium.org/2485473003/diff/640001/content/browser/render... content/browser/renderer_host/delegated_frame_host.cc:830: surface_factory_->Reset(); EvictFrame() https://codereview.chromium.org/2485473003/diff/640001/services/ui/ws/gpu_com... File services/ui/ws/gpu_compositor_frame_sink.cc (right): https://codereview.chromium.org/2485473003/diff/640001/services/ui/ws/gpu_com... services/ui/ws/gpu_compositor_frame_sink.cc:55: surface_factory_.Reset(); EvictFrame()
https://codereview.chromium.org/2485473003/diff/640001/cc/surfaces/surface_fa... File cc/surfaces/surface_factory.h (right): https://codereview.chromium.org/2485473003/diff/640001/cc/surfaces/surface_fa... cc/surfaces/surface_factory.h:102: OwningSurfaceMap surface_map_; Replace this with: std::unique_ptr<Surface> active_surface_;
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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...
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 All reviewers' suggestions should be applied. https://codereview.chromium.org/2485473003/diff/620001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2485473003/diff/620001/components/exo/surface... components/exo/surface.cc:250: factory_owner_->surface_factory_->Reset(); On 2016/11/11 18:16:47, jbauman wrote: > This will prevent the resources from being returned at the right time. Replaced with EvictFrame https://codereview.chromium.org/2485473003/diff/620001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_child_frame.cc (right): https://codereview.chromium.org/2485473003/diff/620001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_child_frame.cc:728: void RenderWidgetHostViewChildFrame::ClearCompositorSurfaceIfNecessary() { On 2016/11/11 18:16:47, jbauman wrote: > Same problem here (in the case where the guest is detached). Added EvictFrame
Thank you Saman for pushing this through this far! This is a hard patch to write and get right, and at this point lgtm.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2485473003/diff/750001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_child_frame.cc (right): https://codereview.chromium.org/2485473003/diff/750001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_child_frame.cc:407: cc::SurfaceFactory::DrawCallback ack_callback = any reason why this is moved to the up? I don't see any data dependency between and "if (allocated_new_local_frame_id)" block https://codereview.chromium.org/2485473003/diff/750001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_guest.cc (right): https://codereview.chromium.org/2485473003/diff/750001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_guest.cc:309: cc::SurfaceFactory::DrawCallback ack_callback = base::Bind( ditto https://codereview.chromium.org/2485473003/diff/750001/content/browser/render... File content/browser/renderer_host/delegated_frame_host.cc (right): https://codereview.chromium.org/2485473003/diff/750001/content/browser/render... content/browser/renderer_host/delegated_frame_host.cc:506: if (allocated_new_local_frame_id) { same question https://codereview.chromium.org/2485473003/diff/750001/content/renderer/andro... File content/renderer/android/synchronous_compositor_frame_sink.cc (right): https://codereview.chromium.org/2485473003/diff/750001/content/renderer/andro... content/renderer/android/synchronous_compositor_frame_sink.cc:126: new cc::SurfaceFactory(kFrameSinkId, surface_manager_.get(), this)), is re-using the same kFrameSinkId for two different surface factories safe?
https://codereview.chromium.org/2485473003/diff/750001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_child_frame.cc (right): https://codereview.chromium.org/2485473003/diff/750001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_child_frame.cc:407: cc::SurfaceFactory::DrawCallback ack_callback = On 2016/11/11 23:50:44, boliu wrote: > any reason why this is moved to the up? I don't see any data dependency between > and "if (allocated_new_local_frame_id)" block Surfaces are now created lazily when CompositorFrames are submitted. Thus, we must submit a CompositorFrame before we can access it via GetSurfaceForId. It's probably worth adding a comment here about this. https://codereview.chromium.org/2485473003/diff/750001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_guest.cc (right): https://codereview.chromium.org/2485473003/diff/750001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_guest.cc:309: cc::SurfaceFactory::DrawCallback ack_callback = base::Bind( On 2016/11/11 23:50:44, boliu wrote: > ditto Surfaces are now created lazily when CompositorFrames are submitted. Thus, we must submit a CompositorFrame before we can access it via GetSurfaceForId. It's probably worth adding a comment here about this. https://codereview.chromium.org/2485473003/diff/750001/content/browser/render... File content/browser/renderer_host/delegated_frame_host.cc (right): https://codereview.chromium.org/2485473003/diff/750001/content/browser/render... content/browser/renderer_host/delegated_frame_host.cc:506: if (allocated_new_local_frame_id) { On 2016/11/11 23:50:45, boliu wrote: > same question Surfaces are now created lazily when CompositorFrames are submitted. Thus, we must submit a CompositorFrame before we can access it via GetSurfaceForId. It's probably worth adding a comment here about this. https://codereview.chromium.org/2485473003/diff/750001/content/renderer/andro... File content/renderer/android/synchronous_compositor_frame_sink.cc (right): https://codereview.chromium.org/2485473003/diff/750001/content/renderer/andro... content/renderer/android/synchronous_compositor_frame_sink.cc:126: new cc::SurfaceFactory(kFrameSinkId, surface_manager_.get(), this)), On 2016/11/11 23:50:45, boliu wrote: > is re-using the same kFrameSinkId for two different surface factories safe? Good catch! No we shouldn't reuse the same FrameSinkId.
https://codereview.chromium.org/2485473003/diff/750001/cc/surfaces/surface_fa... File cc/surfaces/surface_factory.cc (right): https://codereview.chromium.org/2485473003/diff/750001/cc/surfaces/surface_fa... cc/surfaces/surface_factory.cc:78: surface->SetPreviousFrameSurface(current_surface_.get()); I think doing this implicitly makes sense, but I'm not 100% sure that we don't expect full damage in some places on e.g. resize. Before this change, damage was implicitly being discarded on a surface change, because of no previous surface. So we should definitely watch for issues on resize on platforms that support partial swaps, such as (I believe) Windows, Chrome OS and possibly Mac. IIRC it's disabled/not implemented on Linux and Android If there are issues, we should probably EvictFrame in places where we used to recreate a surface and did not do SetPreviousFrameSurface. https://codereview.chromium.org/2485473003/diff/750001/cc/surfaces/surface_fa... File cc/surfaces/surface_factory.h (right): https://codereview.chromium.org/2485473003/diff/750001/cc/surfaces/surface_fa... cc/surfaces/surface_factory.h:48: void EvictFrame(); EvictSurface maybe? Trying to get consistent naming... This is a SurfaceFactory, it creates Surfaces that you can submit CompostorFrames to. This function destroys the Surface - as opposed to, say, submit an empty frame to it. https://codereview.chromium.org/2485473003/diff/750001/cc/surfaces/surface_fa... cc/surfaces/surface_factory.h:54: // A frame can only be submitted to a surface created by this factory, Documentation should be updated.
https://codereview.chromium.org/2485473003/diff/750001/cc/surfaces/surface_ag... File cc/surfaces/surface_aggregator_perftest.cc (left): https://codereview.chromium.org/2485473003/diff/750001/cc/surfaces/surface_ag... cc/surfaces/surface_aggregator_perftest.cc:102: factory_.Create(LocalFrameId(num_surfaces + 1, kArbitraryToken)); nit: I think this would read a bit clearer if the array was child_factories and excluded this last one here, and you made another SurfaceFactory here as the root_factory. https://codereview.chromium.org/2485473003/diff/750001/cc/surfaces/surface_ag... File cc/surfaces/surface_aggregator_unittest.cc (right): https://codereview.chromium.org/2485473003/diff/750001/cc/surfaces/surface_ag... cc/surfaces/surface_aggregator_unittest.cc:1352: // SetPreviousFrameSurface. You deleted this function, so what is this testing now? https://codereview.chromium.org/2485473003/diff/750001/cc/surfaces/surface_ag... cc/surfaces/surface_aggregator_unittest.cc:1412: // Frame from SetPreviousFrameSurface was aggregated last, so damage rect This comment and expectation is referring to a function that doesn't exist anymore. https://codereview.chromium.org/2485473003/diff/750001/cc/surfaces/surface_fa... File cc/surfaces/surface_factory.cc (right): https://codereview.chromium.org/2485473003/diff/750001/cc/surfaces/surface_fa... cc/surfaces/surface_factory.cc:29: EvictFrame(); Is there a reason to support having a surface here, or could we dcheck that current_surface_ was evicted already? (similar to the old weaker log error) https://codereview.chromium.org/2485473003/diff/750001/cc/surfaces/surface_fa... cc/surfaces/surface_factory.cc:78: surface->SetPreviousFrameSurface(current_surface_.get()); On 2016/11/12 00:19:10, piman wrote: > I think doing this implicitly makes sense, but I'm not 100% sure that we don't > expect full damage in some places on e.g. resize. Before this change, damage was > implicitly being discarded on a surface change, because of no previous surface. > So we should definitely watch for issues on resize on platforms that support > partial swaps, such as (I believe) Windows, Chrome OS and possibly Mac. IIRC > it's disabled/not implemented on Linux and Android Should on linux with --disable-gpu > If there are issues, we should probably EvictFrame in places where we used to > recreate a surface and did not do SetPreviousFrameSurface. https://codereview.chromium.org/2485473003/diff/750001/cc/surfaces/surface_fa... cc/surfaces/surface_factory.cc:122: if (manager_) I'm confused by the if (manager_) checks sometimes but not others. When can the manager_ be null? https://codereview.chromium.org/2485473003/diff/750001/cc/surfaces/surface_fa... File cc/surfaces/surface_factory.h (right): https://codereview.chromium.org/2485473003/diff/750001/cc/surfaces/surface_fa... cc/surfaces/surface_factory.h:48: void EvictFrame(); Please add a comment to explain what this does and when it should be used https://codereview.chromium.org/2485473003/diff/750001/cc/surfaces/surface_fa... cc/surfaces/surface_factory.h:50: // Destroys and disown all surfaces, and reset all resource references. This does the meaning of "all surfaces" change now? https://codereview.chromium.org/2485473003/diff/750001/cc/surfaces/surface_fa... File cc/surfaces/surface_factory_unittest.cc (right): https://codereview.chromium.org/2485473003/diff/750001/cc/surfaces/surface_fa... cc/surfaces/surface_factory_unittest.cc:626: nit: random whitespace? https://codereview.chromium.org/2485473003/diff/750001/cc/surfaces/surface_ma... File cc/surfaces/surface_manager_ref_unittest.cc (right): https://codereview.chromium.org/2485473003/diff/750001/cc/surfaces/surface_ma... cc/surfaces/surface_manager_ref_unittest.cc:27: class EmptySurfaceFactoryClient : public SurfaceFactoryClient { Stub is the word for this, rather than Empty
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...
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 All suggestions are applied. Some tests might fail, but that'll be only because of the added DCHECK and I can fix them quickly. https://codereview.chromium.org/2485473003/diff/750001/cc/surfaces/surface_ag... File cc/surfaces/surface_aggregator_perftest.cc (left): https://codereview.chromium.org/2485473003/diff/750001/cc/surfaces/surface_ag... cc/surfaces/surface_aggregator_perftest.cc:102: factory_.Create(LocalFrameId(num_surfaces + 1, kArbitraryToken)); On 2016/11/12 00:27:21, danakj wrote: > nit: I think this would read a bit clearer if the array was child_factories and > excluded this last one here, and you made another SurfaceFactory here as the > root_factory. Done. https://codereview.chromium.org/2485473003/diff/750001/cc/surfaces/surface_ag... File cc/surfaces/surface_aggregator_unittest.cc (right): https://codereview.chromium.org/2485473003/diff/750001/cc/surfaces/surface_ag... cc/surfaces/surface_aggregator_unittest.cc:1352: // SetPreviousFrameSurface. On 2016/11/12 00:27:21, danakj wrote: > You deleted this function, so what is this testing now? It can still test whether SubmitCompositorFrame correctly sets the previous frame. https://codereview.chromium.org/2485473003/diff/750001/cc/surfaces/surface_ag... cc/surfaces/surface_aggregator_unittest.cc:1412: // Frame from SetPreviousFrameSurface was aggregated last, so damage rect On 2016/11/12 00:27:21, danakj wrote: > This comment and expectation is referring to a function that doesn't exist > anymore. Done. https://codereview.chromium.org/2485473003/diff/750001/cc/surfaces/surface_fa... File cc/surfaces/surface_factory.cc (right): https://codereview.chromium.org/2485473003/diff/750001/cc/surfaces/surface_fa... cc/surfaces/surface_factory.cc:122: if (manager_) On 2016/11/12 00:27:21, danakj wrote: > I'm confused by the if (manager_) checks sometimes but not others. When can the > manager_ be null? I have no idea. I didn't write this. We can get rid of it in a future CL if possible. https://codereview.chromium.org/2485473003/diff/750001/cc/surfaces/surface_fa... File cc/surfaces/surface_factory.h (right): https://codereview.chromium.org/2485473003/diff/750001/cc/surfaces/surface_fa... cc/surfaces/surface_factory.h:50: // Destroys and disown all surfaces, and reset all resource references. This On 2016/11/12 00:27:21, danakj wrote: > does the meaning of "all surfaces" change now? Done. https://codereview.chromium.org/2485473003/diff/750001/cc/surfaces/surface_fa... cc/surfaces/surface_factory.h:54: // A frame can only be submitted to a surface created by this factory, On 2016/11/12 00:19:10, piman wrote: > Documentation should be updated. Done. https://codereview.chromium.org/2485473003/diff/750001/cc/surfaces/surface_fa... File cc/surfaces/surface_factory_unittest.cc (right): https://codereview.chromium.org/2485473003/diff/750001/cc/surfaces/surface_fa... cc/surfaces/surface_factory_unittest.cc:626: On 2016/11/12 00:27:21, danakj wrote: > nit: random whitespace? Done. https://codereview.chromium.org/2485473003/diff/750001/cc/surfaces/surface_ma... File cc/surfaces/surface_manager_ref_unittest.cc (right): https://codereview.chromium.org/2485473003/diff/750001/cc/surfaces/surface_ma... cc/surfaces/surface_manager_ref_unittest.cc:27: class EmptySurfaceFactoryClient : public SurfaceFactoryClient { On 2016/11/12 00:27:22, danakj wrote: > Stub is the word for this, rather than Empty Done. https://codereview.chromium.org/2485473003/diff/750001/content/renderer/andro... File content/renderer/android/synchronous_compositor_frame_sink.cc (right): https://codereview.chromium.org/2485473003/diff/750001/content/renderer/andro... content/renderer/android/synchronous_compositor_frame_sink.cc:126: new cc::SurfaceFactory(kFrameSinkId, surface_manager_.get(), this)), On 2016/11/11 23:50:45, boliu wrote: > is re-using the same kFrameSinkId for two different surface factories safe? Done.
Please wrap your CL description at 72 chars: http://chris.beams.io/posts/git-commit/ LGTM % piman's concerns with damage https://codereview.chromium.org/2485473003/diff/750001/cc/surfaces/surface_ag... File cc/surfaces/surface_aggregator_unittest.cc (right): https://codereview.chromium.org/2485473003/diff/750001/cc/surfaces/surface_ag... cc/surfaces/surface_aggregator_unittest.cc:1352: // SetPreviousFrameSurface. On 2016/11/14 23:39:01, Saman Sami wrote: > On 2016/11/12 00:27:21, danakj wrote: > > You deleted this function, so what is this testing now? > > It can still test whether SubmitCompositorFrame correctly sets the previous > frame. Thanks, was looking for the comment to be updated :) https://codereview.chromium.org/2485473003/diff/750001/cc/surfaces/surface_fa... File cc/surfaces/surface_factory.cc (right): https://codereview.chromium.org/2485473003/diff/750001/cc/surfaces/surface_fa... cc/surfaces/surface_factory.cc:122: if (manager_) On 2016/11/14 23:39:01, Saman Sami wrote: > On 2016/11/12 00:27:21, danakj wrote: > > I'm confused by the if (manager_) checks sometimes but not others. When can > the > > manager_ be null? > > I have no idea. I didn't write this. We can get rid of it in a future CL if > possible. I would greatly appreciate that we do, unneeded null checks frustrate me :) https://codereview.chromium.org/2485473003/diff/840001/cc/surfaces/surface_ag... File cc/surfaces/surface_aggregator_perftest.cc (right): https://codereview.chromium.org/2485473003/diff/840001/cc/surfaces/surface_ag... cc/surfaces/surface_aggregator_perftest.cc:50: std::unique_ptr<SurfaceFactory> child_factories[num_surfaces + 1]; child_factories[0] is never used anymore. drop the +1 here and change all the loops to go from 0 instead of 1, and references to |i| to |1+i|? https://codereview.chromium.org/2485473003/diff/840001/cc/surfaces/surface_ag... File cc/surfaces/surface_aggregator_unittest.cc (right): https://codereview.chromium.org/2485473003/diff/840001/cc/surfaces/surface_ag... cc/surfaces/surface_aggregator_unittest.cc:80: factory_.EvictSurface(); nit: should do local destruction/shutdown work before calling parent class usually https://codereview.chromium.org/2485473003/diff/840001/cc/surfaces/surface_ag... cc/surfaces/surface_aggregator_unittest.cc:119: child_factory_.EvictSurface(); nitto https://codereview.chromium.org/2485473003/diff/840001/cc/surfaces/surface_fa... File cc/surfaces/surface_factory.cc (right): https://codereview.chromium.org/2485473003/diff/840001/cc/surfaces/surface_fa... cc/surfaces/surface_factory.cc:33: DCHECK(!current_surface_) << "Please call EvictSurface before destruction"; A++ polite code https://codereview.chromium.org/2485473003/diff/840001/cc/surfaces/surface_fa... File cc/surfaces/surface_factory.h (right): https://codereview.chromium.org/2485473003/diff/840001/cc/surfaces/surface_fa... cc/surfaces/surface_factory.h:54: // the old surface will be dealt with) nit: period https://codereview.chromium.org/2485473003/diff/840001/cc/surfaces/surface_fa... File cc/surfaces/surface_factory_unittest.cc (left): https://codereview.chromium.org/2485473003/diff/840001/cc/surfaces/surface_fa... cc/surfaces/surface_factory_unittest.cc:493: factory_->DestroyAll(); Should this test exist still for EvictSurface? https://codereview.chromium.org/2485473003/diff/840001/cc/surfaces/surface_un... File cc/surfaces/surface_unittest.cc (right): https://codereview.chromium.org/2485473003/diff/840001/cc/surfaces/surface_un... cc/surfaces/surface_unittest.cc:43: { nit: these {} are pointless but distract from what is happening by introducing a block. maybe remove them?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2485473003/diff/840001/cc/surfaces/surface_fa... File cc/surfaces/surface_factory.cc (right): https://codereview.chromium.org/2485473003/diff/840001/cc/surfaces/surface_fa... cc/surfaces/surface_factory.cc:67: if (!surface->HasFrame()) { nit: This check seems redundant at this point. If we just created the Surface then we know it doesn't have a Frame. if (create_new_surface) { surface = Create(local_frame_id); ... manager_->SurfaceCreated(surface->surface_id(), ...); }
Description was changed from ========== Remove SurfaceFactory::Create and SurfaceFactory::Destroy It is no longer possible to explicitly construct and destroy surfaces. A SurfaceFactory instance now handles only one surface at a time. Once the local frame id passed to SubmitCompositorFrame changes, the factory gets rid of the old surface and creates a new one. BUG=658607 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Remove SurfaceFactory::Create and SurfaceFactory::Destroy It is no longer possible to explicitly construct and destroy surfaces. A SurfaceFactory instance now handles only one surface at a time. Once the local frame id passed to SubmitCompositorFrame changes, the factory gets rid of the old surface and creates a new one. BUG=658607 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
Patchset #40 (id:860001) has been deleted
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 unchecked by commit-bot@chromium.org
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...)
PTAL In addition to fixing the last set of problems, I tested resizing chrome on linux with --disable-gpu and did not notice any glitches. https://codereview.chromium.org/2485473003/diff/840001/cc/surfaces/surface_ag... File cc/surfaces/surface_aggregator_perftest.cc (right): https://codereview.chromium.org/2485473003/diff/840001/cc/surfaces/surface_ag... cc/surfaces/surface_aggregator_perftest.cc:50: std::unique_ptr<SurfaceFactory> child_factories[num_surfaces + 1]; On 2016/11/15 00:43:41, danakj wrote: > child_factories[0] is never used anymore. drop the +1 here and change all the > loops to go from 0 instead of 1, and references to |i| to |1+i|? Done. https://codereview.chromium.org/2485473003/diff/840001/cc/surfaces/surface_ag... File cc/surfaces/surface_aggregator_unittest.cc (right): https://codereview.chromium.org/2485473003/diff/840001/cc/surfaces/surface_ag... cc/surfaces/surface_aggregator_unittest.cc:80: factory_.EvictSurface(); On 2016/11/15 00:43:41, danakj wrote: > nit: should do local destruction/shutdown work before calling parent class > usually Done. https://codereview.chromium.org/2485473003/diff/840001/cc/surfaces/surface_ag... cc/surfaces/surface_aggregator_unittest.cc:119: child_factory_.EvictSurface(); On 2016/11/15 00:43:41, danakj wrote: > nitto Done. https://codereview.chromium.org/2485473003/diff/840001/cc/surfaces/surface_fa... File cc/surfaces/surface_factory.cc (right): https://codereview.chromium.org/2485473003/diff/840001/cc/surfaces/surface_fa... cc/surfaces/surface_factory.cc:33: DCHECK(!current_surface_) << "Please call EvictSurface before destruction"; On 2016/11/15 00:43:41, danakj wrote: > A++ polite code lol https://codereview.chromium.org/2485473003/diff/840001/cc/surfaces/surface_fa... cc/surfaces/surface_factory.cc:67: if (!surface->HasFrame()) { On 2016/11/15 12:46:51, Fady Samuel wrote: > nit: This check seems redundant at this point. If we just created the Surface > then we know it doesn't have a Frame. > > if (create_new_surface) { > surface = Create(local_frame_id); > ... > manager_->SurfaceCreated(surface->surface_id(), ...); > } Done. https://codereview.chromium.org/2485473003/diff/840001/cc/surfaces/surface_fa... File cc/surfaces/surface_factory.h (right): https://codereview.chromium.org/2485473003/diff/840001/cc/surfaces/surface_fa... cc/surfaces/surface_factory.h:54: // the old surface will be dealt with) On 2016/11/15 00:43:41, danakj wrote: > nit: period Done. https://codereview.chromium.org/2485473003/diff/840001/cc/surfaces/surface_fa... File cc/surfaces/surface_factory_unittest.cc (left): https://codereview.chromium.org/2485473003/diff/840001/cc/surfaces/surface_fa... cc/surfaces/surface_factory_unittest.cc:493: factory_->DestroyAll(); On 2016/11/15 00:43:41, danakj wrote: > Should this test exist still for EvictSurface? Done. https://codereview.chromium.org/2485473003/diff/840001/cc/surfaces/surface_un... File cc/surfaces/surface_unittest.cc (right): https://codereview.chromium.org/2485473003/diff/840001/cc/surfaces/surface_un... cc/surfaces/surface_unittest.cc:43: { On 2016/11/15 00:43:41, danakj wrote: > nit: these {} are pointless but distract from what is happening by introducing a > block. maybe remove them? Done.
lgtm
lgtm
The CQ bit was checked by samans@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtrainor@chromium.org, sky@chromium.org, reveman@chromium.org, fsamuel@chromium.org, danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2485473003/#ps880001 (title: "up")
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
On 2016/11/15 23:36:26, commit-bot: I haz the power wrote: > 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...) stop spamming the trybot.. after it fails twice on the exact same thing, probably should look into why: crbug.com/665595
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...
Message was sent while issue was closed.
Description was changed from ========== Remove SurfaceFactory::Create and SurfaceFactory::Destroy It is no longer possible to explicitly construct and destroy surfaces. A SurfaceFactory instance now handles only one surface at a time. Once the local frame id passed to SubmitCompositorFrame changes, the factory gets rid of the old surface and creates a new one. BUG=658607 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Remove SurfaceFactory::Create and SurfaceFactory::Destroy It is no longer possible to explicitly construct and destroy surfaces. A SurfaceFactory instance now handles only one surface at a time. Once the local frame id passed to SubmitCompositorFrame changes, the factory gets rid of the old surface and creates a new one. BUG=658607 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
Message was sent while issue was closed.
Committed patchset #40 (id:880001)
Message was sent while issue was closed.
Description was changed from ========== Remove SurfaceFactory::Create and SurfaceFactory::Destroy It is no longer possible to explicitly construct and destroy surfaces. A SurfaceFactory instance now handles only one surface at a time. Once the local frame id passed to SubmitCompositorFrame changes, the factory gets rid of the old surface and creates a new one. BUG=658607 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Remove SurfaceFactory::Create and SurfaceFactory::Destroy It is no longer possible to explicitly construct and destroy surfaces. A SurfaceFactory instance now handles only one surface at a time. Once the local frame id passed to SubmitCompositorFrame changes, the factory gets rid of the old surface and creates a new one. BUG=658607 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Committed: https://crrev.com/b902fd366ba91757dbee99d5f821f00d2de1d181 Cr-Commit-Position: refs/heads/master@{#432312} ==========
Message was sent while issue was closed.
Patchset 40 (id:??) landed as https://crrev.com/b902fd366ba91757dbee99d5f821f00d2de1d181 Cr-Commit-Position: refs/heads/master@{#432312}
Message was sent while issue was closed.
A revert of this CL (patchset #40 id:880001) has been created in https://codereview.chromium.org/2506883002/ by suzyh@chromium.org. The reason for reverting is: Seems to have broken windows compile: https://build.chromium.org/p/chromium/builders/Win/builds/49156.
Message was sent while issue was closed.
https://codereview.chromium.org/2485473003/diff/880001/cc/surfaces/surface_ag... File cc/surfaces/surface_aggregator_perftest.cc (right): https://codereview.chromium.org/2485473003/diff/880001/cc/surfaces/surface_ag... cc/surfaces/surface_aggregator_perftest.cc:50: std::unique_ptr<SurfaceFactory> child_factories[num_surfaces]; Dynamically sized arrays on the stack is not a standard C++ feature IIRC.
Message was sent while issue was closed.
Description was changed from ========== Remove SurfaceFactory::Create and SurfaceFactory::Destroy It is no longer possible to explicitly construct and destroy surfaces. A SurfaceFactory instance now handles only one surface at a time. Once the local frame id passed to SubmitCompositorFrame changes, the factory gets rid of the old surface and creates a new one. BUG=658607 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Committed: https://crrev.com/b902fd366ba91757dbee99d5f821f00d2de1d181 Cr-Commit-Position: refs/heads/master@{#432312} ========== to ========== Remove SurfaceFactory::Create and SurfaceFactory::Destroy It is no longer possible to explicitly construct and destroy surfaces. A SurfaceFactory instance now handles only one surface at a time. Once the local frame id passed to SubmitCompositorFrame changes, the factory gets rid of the old surface and creates a new one. BUG=658607 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Committed: https://crrev.com/b902fd366ba91757dbee99d5f821f00d2de1d181 Cr-Commit-Position: refs/heads/master@{#432312} ==========
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 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...)
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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)
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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)
Description was changed from ========== Remove SurfaceFactory::Create and SurfaceFactory::Destroy It is no longer possible to explicitly construct and destroy surfaces. A SurfaceFactory instance now handles only one surface at a time. Once the local frame id passed to SubmitCompositorFrame changes, the factory gets rid of the old surface and creates a new one. BUG=658607 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Committed: https://crrev.com/b902fd366ba91757dbee99d5f821f00d2de1d181 Cr-Commit-Position: refs/heads/master@{#432312} ========== to ========== Remove SurfaceFactory::Create and SurfaceFactory::Destroy It is no longer possible to explicitly construct and destroy surfaces. A SurfaceFactory instance now handles only one surface at a time. Once the local frame id passed to SubmitCompositorFrame changes, the factory gets rid of the old surface and creates a new one. BUG=658607 Committed: https://crrev.com/b902fd366ba91757dbee99d5f821f00d2de1d181 Cr-Commit-Position: refs/heads/master@{#432312} ==========
Description was changed from ========== Remove SurfaceFactory::Create and SurfaceFactory::Destroy It is no longer possible to explicitly construct and destroy surfaces. A SurfaceFactory instance now handles only one surface at a time. Once the local frame id passed to SubmitCompositorFrame changes, the factory gets rid of the old surface and creates a new one. BUG=658607 Committed: https://crrev.com/b902fd366ba91757dbee99d5f821f00d2de1d181 Cr-Commit-Position: refs/heads/master@{#432312} ========== to ========== Remove SurfaceFactory::Create and SurfaceFactory::Destroy It is no longer possible to explicitly construct and destroy surfaces. A SurfaceFactory instance now handles only one surface at a time. Once the local frame id passed to SubmitCompositorFrame changes, the factory gets rid of the old surface and creates a new one. BUG=658607 Committed: https://crrev.com/b902fd366ba91757dbee99d5f821f00d2de1d181 Cr-Commit-Position: refs/heads/master@{#432312} CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Remove SurfaceFactory::Create and SurfaceFactory::Destroy It is no longer possible to explicitly construct and destroy surfaces. A SurfaceFactory instance now handles only one surface at a time. Once the local frame id passed to SubmitCompositorFrame changes, the factory gets rid of the old surface and creates a new one. BUG=658607 Committed: https://crrev.com/b902fd366ba91757dbee99d5f821f00d2de1d181 Cr-Commit-Position: refs/heads/master@{#432312} CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Remove SurfaceFactory::Create and SurfaceFactory::Destroy It is no longer possible to explicitly construct and destroy surfaces. A SurfaceFactory instance now handles only one surface at a time. Once the local frame id passed to SubmitCompositorFrame changes, the factory gets rid of the old surface and creates a new one. BUG=658607 Committed: https://crrev.com/b902fd366ba91757dbee99d5f821f00d2de1d181 Cr-Commit-Position: refs/heads/master@{#432312} CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== Remove SurfaceFactory::Create and SurfaceFactory::Destroy It is no longer possible to explicitly construct and destroy surfaces. A SurfaceFactory instance now handles only one surface at a time. Once the local frame id passed to SubmitCompositorFrame changes, the factory gets rid of the old surface and creates a new one. BUG=658607 Committed: https://crrev.com/b902fd366ba91757dbee99d5f821f00d2de1d181 Cr-Commit-Position: refs/heads/master@{#432312} CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Remove SurfaceFactory::Create and SurfaceFactory::Destroy It is no longer possible to explicitly construct and destroy surfaces. A SurfaceFactory instance now handles only one surface at a time. Once the local frame id passed to SubmitCompositorFrame changes, the factory gets rid of the old surface and creates a new one. BUG=658607 Committed: https://crrev.com/b902fd366ba91757dbee99d5f821f00d2de1d181 Cr-Commit-Position: refs/heads/master@{#432312} ==========
The CQ bit was checked by samans@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtrainor@chromium.org, reveman@chromium.org, sky@chromium.org, boliu@chromium.org, piman@chromium.org, danakj@chromium.org, fsamuel@chromium.org Link to the patchset: https://codereview.chromium.org/2485473003/#ps980001 (title: "fixed windows compile error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2485473003/diff/980001/content/browser/render... File content/browser/renderer_host/delegated_frame_host.cc (right): https://codereview.chromium.org/2485473003/diff/980001/content/browser/render... content/browser/renderer_host/delegated_frame_host.cc:481: surface_factory_->SubmitCompositorFrame( Please don't land this. This is not the correct fix in my opinion because this adds unnecessary overhead.
The CQ bit was unchecked by fsamuel@chromium.org
https://codereview.chromium.org/2485473003/diff/980001/cc/surfaces/surface_fa... File cc/surfaces/surface_factory.cc (right): https://codereview.chromium.org/2485473003/diff/980001/cc/surfaces/surface_fa... cc/surfaces/surface_factory.cc:41: current_surface_.reset(); nit: this is unnecessary because you moved current_surface_ above.
Description was changed from ========== Remove SurfaceFactory::Create and SurfaceFactory::Destroy It is no longer possible to explicitly construct and destroy surfaces. A SurfaceFactory instance now handles only one surface at a time. Once the local frame id passed to SubmitCompositorFrame changes, the factory gets rid of the old surface and creates a new one. BUG=658607 Committed: https://crrev.com/b902fd366ba91757dbee99d5f821f00d2de1d181 Cr-Commit-Position: refs/heads/master@{#432312} ========== to ========== Remove SurfaceFactory::Create and SurfaceFactory::Destroy It is no longer possible to explicitly construct and destroy surfaces. A SurfaceFactory instance now handles only one surface at a time. Once the local frame id passed to SubmitCompositorFrame changes, the factory gets rid of the old surface and creates a new one. BUG=658607 Committed: https://crrev.com/b902fd366ba91757dbee99d5f821f00d2de1d181 Cr-Commit-Position: refs/heads/master@{#432312} CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Remove SurfaceFactory::Create and SurfaceFactory::Destroy It is no longer possible to explicitly construct and destroy surfaces. A SurfaceFactory instance now handles only one surface at a time. Once the local frame id passed to SubmitCompositorFrame changes, the factory gets rid of the old surface and creates a new one. BUG=658607 Committed: https://crrev.com/b902fd366ba91757dbee99d5f821f00d2de1d181 Cr-Commit-Position: refs/heads/master@{#432312} CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Remove SurfaceFactory::Create and SurfaceFactory::Destroy It is no longer possible to explicitly construct and destroy surfaces. A SurfaceFactory instance now handles only one surface at a time. Once the local frame id passed to SubmitCompositorFrame changes, the factory gets rid of the old surface and creates a new one. BUG=658607 Committed: https://crrev.com/b902fd366ba91757dbee99d5f821f00d2de1d181 Cr-Commit-Position: refs/heads/master@{#432312} 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by samans@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtrainor@chromium.org, reveman@chromium.org, sky@chromium.org, boliu@chromium.org, piman@chromium.org, danakj@chromium.org, fsamuel@chromium.org Link to the patchset: https://codereview.chromium.org/2485473003/#ps1000001 (title: "up")
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_TIMED_OUT, no build URL)
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...
CQ is committing da patch.
Bot data: {"patchset_id": 1000001, "attempt_start_ts": 1480368204989190,
"parent_rev": "94a774b15837c6686dd29d4861d0918129e8c188", "commit_rev":
"98d3aa0e32cfb8dfbcd39a7ca19abbe8aca91171"}
Message was sent while issue was closed.
Committed patchset #46 (id:1000001)
Message was sent while issue was closed.
Description was changed from ========== Remove SurfaceFactory::Create and SurfaceFactory::Destroy It is no longer possible to explicitly construct and destroy surfaces. A SurfaceFactory instance now handles only one surface at a time. Once the local frame id passed to SubmitCompositorFrame changes, the factory gets rid of the old surface and creates a new one. BUG=658607 Committed: https://crrev.com/b902fd366ba91757dbee99d5f821f00d2de1d181 Cr-Commit-Position: refs/heads/master@{#432312} CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Remove SurfaceFactory::Create and SurfaceFactory::Destroy It is no longer possible to explicitly construct and destroy surfaces. A SurfaceFactory instance now handles only one surface at a time. Once the local frame id passed to SubmitCompositorFrame changes, the factory gets rid of the old surface and creates a new one. BUG=658607 Committed: https://crrev.com/b902fd366ba91757dbee99d5f821f00d2de1d181 Cr-Commit-Position: refs/heads/master@{#432312} CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Committed: https://crrev.com/46b4f407905420c8997e5c886baef4bbf5248b79 Cr-Commit-Position: refs/heads/master@{#434724} ==========
Message was sent while issue was closed.
Patchset 46 (id:??) landed as https://crrev.com/46b4f407905420c8997e5c886baef4bbf5248b79 Cr-Commit-Position: refs/heads/master@{#434724} |
