Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2540783004:
Add SurfaceManager option to use references exclusively. (Closed)
DescriptionAdd SurfaceManager option to use references exclusively.
SurfaceManager should be able to walk from root and delete all destroyed
and unreachable surfaces. This doesn't work in conjunction with surface
sequence dependencies (like regular chrome) but works fine for mustash
which only uses surface refrences. Add an optional parameter to
SurfaceManager. This parameter would be removed when everything uses
surface references.
BUG=659227
Committed: https://crrev.com/a886f220c05a5d12d65ccf97bfc86ec1985dc93e
Cr-Commit-Position: refs/heads/master@{#435970}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Fixes. #
Total comments: 3
Patch Set 3 : Add .push(). #
Messages
Total messages: 26 (15 generated)
Description was changed from ========== Add SurfaceManager option to use references exclusively. SurfaceManager should be able to walk from root and delete all destroyed and unreachable surfaces. This doesn't work in conjunction with surface sequence dependencies (like regular chrome) but works fine for mustash which only uses surface refrences. Add an optional parameter to SurfaceManager. This parameter would be removed when everything uses surface references. BUG=659227 ========== to ========== Add SurfaceManager option to use references exclusively. SurfaceManager should be able to walk from root and delete all destroyed and unreachable surfaces. This doesn't work in conjunction with surface sequence dependencies (like regular chrome) but works fine for mustash which only uses surface refrences. Add an optional parameter to SurfaceManager. This parameter would be removed when everything uses surface references. BUG=659227 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
kylechar@chromium.org changed reviewers: + fsamuel@chromium.org
Very cool! lgtm
kylechar@chromium.org changed reviewers: + enne@chromium.org
Thanks! +enne for cc/surfaces/*
vmpstr@chromium.org changed reviewers: + vmpstr@chromium.org
https://codereview.chromium.org/2540783004/diff/1/cc/surfaces/surface_manager.cc File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2540783004/diff/1/cc/surfaces/surface_manager... cc/surfaces/surface_manager.cc:34: LocalFrameId(1u, base::UnguessableToken::Create())) { Why 1u change? https://codereview.chromium.org/2540783004/diff/1/cc/surfaces/surface_manager... cc/surfaces/surface_manager.cc:165: std::queue<SurfaceId> surface_queue({root_surface_id_}); surface_queue{root_surface_id_}? Can you also verify in the styleguide that brace/initializer list things like that are allowed? (afaik it is, but doesn't hurt to check) https://codereview.chromium.org/2540783004/diff/1/cc/surfaces/surface_manager.h File cc/surfaces/surface_manager.h (right): https://codereview.chromium.org/2540783004/diff/1/cc/surfaces/surface_manager... cc/surfaces/surface_manager.h:34: explicit SurfaceManager(bool use_references = false); minor nit: we'll either have to put comments every time we put an explicit parameter (as I commented where you use it), or you could add an enum like USE_REFERENCES, DONT_USE_REFERENCES or something, then the ctor calls will be self documenting. https://codereview.chromium.org/2540783004/diff/1/services/ui/surfaces/displa... File services/ui/surfaces/display_compositor.cc (right): https://codereview.chromium.org/2540783004/diff/1/services/ui/surfaces/displa... services/ui/surfaces/display_compositor.cc:25: : manager_(true), manager_(true /* use_references */)
kylechar@chromium.org changed reviewers: - enne@chromium.org
https://codereview.chromium.org/2540783004/diff/1/cc/surfaces/surface_manager.cc File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2540783004/diff/1/cc/surfaces/surface_manager... cc/surfaces/surface_manager.cc:34: LocalFrameId(1u, base::UnguessableToken::Create())) { On 2016/12/01 21:04:02, vmpstr wrote: > Why 1u change? The LocalFrameId will be a valid value (local_frame.is_valid()) this way. Realistically, it doesn't make a difference but seems nicer for consistency. https://codereview.chromium.org/2540783004/diff/1/cc/surfaces/surface_manager... cc/surfaces/surface_manager.cc:165: std::queue<SurfaceId> surface_queue({root_surface_id_}); On 2016/12/01 21:04:02, vmpstr wrote: > surface_queue{root_surface_id_}? > > Can you also verify in the styleguide that brace/initializer list things like > that are allowed? (afaik it is, but doesn't hurt to check) std::initializer_list are allowed now and I can't see anything against using it this way in the style guide. https://codereview.chromium.org/2540783004/diff/1/cc/surfaces/surface_manager.h File cc/surfaces/surface_manager.h (right): https://codereview.chromium.org/2540783004/diff/1/cc/surfaces/surface_manager... cc/surfaces/surface_manager.h:34: explicit SurfaceManager(bool use_references = false); On 2016/12/01 21:04:02, vmpstr wrote: > minor nit: we'll either have to put comments every time we put an explicit > parameter (as I commented where you use it), or you could add an enum like > USE_REFERENCES, DONT_USE_REFERENCES or something, then the ctor calls will be > self documenting. Done. https://codereview.chromium.org/2540783004/diff/1/services/ui/surfaces/displa... File services/ui/surfaces/display_compositor.cc (right): https://codereview.chromium.org/2540783004/diff/1/services/ui/surfaces/displa... services/ui/surfaces/display_compositor.cc:25: : manager_(true), On 2016/12/01 21:04:02, vmpstr wrote: > manager_(true /* use_references */) Changed to enum.
lgtm https://codereview.chromium.org/2540783004/diff/20001/cc/surfaces/surface_man... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2540783004/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:165: std::queue<SurfaceId> surface_queue({root_surface_id_}); nit: still don't need brackets tho :)
https://codereview.chromium.org/2540783004/diff/20001/cc/surfaces/surface_man... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2540783004/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:165: std::queue<SurfaceId> surface_queue({root_surface_id_}); On 2016/12/01 23:44:06, vmpstr wrote: > nit: still don't need brackets tho :) Do you mean the following std::queue<SurfaceId> surface_queue{root_surface_id_}; I actually can't get that to compile (it works fine for std::vector). Looking closer std::queue has different constructors than other contains. I've added a separate push for the root_surface_id_ just to be safe.
Description was changed from ========== Add SurfaceManager option to use references exclusively. SurfaceManager should be able to walk from root and delete all destroyed and unreachable surfaces. This doesn't work in conjunction with surface sequence dependencies (like regular chrome) but works fine for mustash which only uses surface refrences. Add an optional parameter to SurfaceManager. This parameter would be removed when everything uses surface references. BUG=659227 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Add SurfaceManager option to use references exclusively. SurfaceManager should be able to walk from root and delete all destroyed and unreachable surfaces. This doesn't work in conjunction with surface sequence dependencies (like regular chrome) but works fine for mustash which only uses surface refrences. Add an optional parameter to SurfaceManager. This parameter would be removed when everything uses surface references. BUG=659227 ==========
The CQ bit was checked by kylechar@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 kylechar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fsamuel@chromium.org, vmpstr@chromium.org Link to the patchset: https://codereview.chromium.org/2540783004/#ps40001 (title: "Add .push().")
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": 40001, "attempt_start_ts": 1480700857665390,
"parent_rev": "e8945319ded00d5a1c9857d2310c43d76cfc93c3", "commit_rev":
"4f8be4bf352541b64387ea48d5a498f36f75b128"}
Message was sent while issue was closed.
Description was changed from ========== Add SurfaceManager option to use references exclusively. SurfaceManager should be able to walk from root and delete all destroyed and unreachable surfaces. This doesn't work in conjunction with surface sequence dependencies (like regular chrome) but works fine for mustash which only uses surface refrences. Add an optional parameter to SurfaceManager. This parameter would be removed when everything uses surface references. BUG=659227 ========== to ========== Add SurfaceManager option to use references exclusively. SurfaceManager should be able to walk from root and delete all destroyed and unreachable surfaces. This doesn't work in conjunction with surface sequence dependencies (like regular chrome) but works fine for mustash which only uses surface refrences. Add an optional parameter to SurfaceManager. This parameter would be removed when everything uses surface references. BUG=659227 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Add SurfaceManager option to use references exclusively. SurfaceManager should be able to walk from root and delete all destroyed and unreachable surfaces. This doesn't work in conjunction with surface sequence dependencies (like regular chrome) but works fine for mustash which only uses surface refrences. Add an optional parameter to SurfaceManager. This parameter would be removed when everything uses surface references. BUG=659227 ========== to ========== Add SurfaceManager option to use references exclusively. SurfaceManager should be able to walk from root and delete all destroyed and unreachable surfaces. This doesn't work in conjunction with surface sequence dependencies (like regular chrome) but works fine for mustash which only uses surface refrences. Add an optional parameter to SurfaceManager. This parameter would be removed when everything uses surface references. BUG=659227 Committed: https://crrev.com/a886f220c05a5d12d65ccf97bfc86ec1985dc93e Cr-Commit-Position: refs/heads/master@{#435970} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/a886f220c05a5d12d65ccf97bfc86ec1985dc93e Cr-Commit-Position: refs/heads/master@{#435970}
Message was sent while issue was closed.
https://codereview.chromium.org/2540783004/diff/20001/cc/surfaces/surface_man... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2540783004/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:165: std::queue<SurfaceId> surface_queue({root_surface_id_}); On 2016/12/02 16:23:31, kylechar wrote: > On 2016/12/01 23:44:06, vmpstr wrote: > > nit: still don't need brackets tho :) > > Do you mean the following > std::queue<SurfaceId> surface_queue{root_surface_id_}; > > I actually can't get that to compile (it works fine for std::vector). Looking > closer std::queue has different constructors than other contains. > > I've added a separate push for the root_surface_id_ just to be safe. Hmm weird. Thanks for making this safer then :P |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
