Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1519293002:
Add support for (de)serializing LayerTreeHost. (Closed)
DescriptionAdd support for (de)serializing LayerTreeHost.
As part of the cc commit flow, we need to be able to serialize
the LayerTreeHost. Not all state is necessary to serialize to
be able to do a commit on the client side, so only some members
are included in the proto.
BUG=561210
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/4f5e8afdadf36c01a01467abb441be357db6d142
Cr-Commit-Position: refs/heads/master@{#369450}
Patch Set 1 #Patch Set 2 : Added framework for (de)serializing #
Total comments: 14
Patch Set 3 : merged upstream branches #Patch Set 4 : merged master #Patch Set 5 : Added full serialization #Patch Set 6 : moved layerselectionbound fix to other CL #Patch Set 7 : Added missing .proto entry for GYP #Patch Set 8 : Ready for review #
Total comments: 28
Patch Set 9 : Addressed comments from vmpstr #
Total comments: 2
Patch Set 10 : Addressed comment from vmpstr #Patch Set 11 : Moved to become anonymous function #Patch Set 12 : Remove unreachable code, and use early return #
Depends on Patchset: Messages
Total messages: 42 (18 generated)
Description was changed from ========== Add support for (de)serializing LayerTreeHost. BUG=561210 ========== to ========== Add support for (de)serializing LayerTreeHost. BUG=561210 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Description was changed from ========== Add support for (de)serializing LayerTreeHost. BUG=561210 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Add support for (de)serializing LayerTreeHost. As part of the cc commit flow, we need to be able to serialize the LayerTreeHost. Not all state is necessary to serialize to be able to do a commit on the client side, so only some members are included in the proto. BUG=561210 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
vmpstr@chromium.org changed reviewers: + vmpstr@chromium.org
https://codereview.chromium.org/1519293002/diff/20001/cc/proto/layer_tree_hos... File cc/proto/layer_tree_host.proto (right): https://codereview.chromium.org/1519293002/diff/20001/cc/proto/layer_tree_hos... cc/proto/layer_tree_host.proto:36: // LayerTreeHostClient* client_; These are created on the client side independently right? https://codereview.chromium.org/1519293002/diff/20001/cc/proto/layer_tree_hos... cc/proto/layer_tree_host.proto:45: // scoped_ptr<OutputSurface> new_output_surface_; The server shouldn't have these, right? https://codereview.chromium.org/1519293002/diff/20001/cc/proto/layer_tree_hos... cc/proto/layer_tree_host.proto:51: // LayerNode What are these? https://codereview.chromium.org/1519293002/diff/20001/cc/proto/layer_tree_hos... cc/proto/layer_tree_host.proto:72: // bool visible_; I'd want to verify if this is possible to switch from the server side https://codereview.chromium.org/1519293002/diff/20001/cc/proto/layer_tree_hos... cc/proto/layer_tree_host.proto:80: optional bool gpu_rasterization_histogram_recorded = 23; This is a bit awkward to serialize. Either the client shouldn't care or the server shouldn't have the right value. https://codereview.chromium.org/1519293002/diff/20001/cc/proto/layer_tree_hos... cc/proto/layer_tree_host.proto:85: // skip animations for now This looks iffy. Why do we skip animations? https://codereview.chromium.org/1519293002/diff/20001/cc/proto/layer_tree_hos... cc/proto/layer_tree_host.proto:111: // promise to ignore it for now. :P what is this used for?
The CQ bit was checked by nyquist@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1519293002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1519293002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by nyquist@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1519293002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1519293002/120001
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 nyquist@chromium.org to run a CQ dry run
vmpstr: PTAL https://codereview.chromium.org/1519293002/diff/20001/cc/proto/layer_tree_hos... File cc/proto/layer_tree_host.proto (right): https://codereview.chromium.org/1519293002/diff/20001/cc/proto/layer_tree_hos... cc/proto/layer_tree_host.proto:36: // LayerTreeHostClient* client_; On 2015/12/18 19:15:13, vmpstr wrote: > These are created on the client side independently right? Yes. https://codereview.chromium.org/1519293002/diff/20001/cc/proto/layer_tree_hos... cc/proto/layer_tree_host.proto:45: // scoped_ptr<OutputSurface> new_output_surface_; On 2015/12/18 19:15:13, vmpstr wrote: > The server shouldn't have these, right? Yeah, the output surface should only be on the client. https://codereview.chromium.org/1519293002/diff/20001/cc/proto/layer_tree_hos... cc/proto/layer_tree_host.proto:51: // LayerNode On 2015/12/18 19:15:13, vmpstr wrote: > What are these? Just comments about what the proto message names are. https://codereview.chromium.org/1519293002/diff/20001/cc/proto/layer_tree_hos... cc/proto/layer_tree_host.proto:72: // bool visible_; On 2015/12/18 19:15:13, vmpstr wrote: > I'd want to verify if this is possible to switch from the server side This will be sent separately from the client to the engine, but will basically be controlled by the client. https://codereview.chromium.org/1519293002/diff/20001/cc/proto/layer_tree_hos... cc/proto/layer_tree_host.proto:80: optional bool gpu_rasterization_histogram_recorded = 23; On 2015/12/18 19:15:13, vmpstr wrote: > This is a bit awkward to serialize. Either the client shouldn't care or the > server shouldn't have the right value. Agreed. I'll remove it. https://codereview.chromium.org/1519293002/diff/20001/cc/proto/layer_tree_hos... cc/proto/layer_tree_host.proto:85: // skip animations for now On 2015/12/18 19:15:13, vmpstr wrote: > This looks iffy. Why do we skip animations? We don't know how to serialize animations yet. :-( https://codereview.chromium.org/1519293002/diff/20001/cc/proto/layer_tree_hos... cc/proto/layer_tree_host.proto:111: // promise to ignore it for now. On 2015/12/18 19:15:13, vmpstr wrote: > :P what is this used for? perf measurements apparently.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1519293002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1519293002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1519293002/diff/140001/cc/proto/layer_tree_ho... File cc/proto/layer_tree_host.proto (right): https://codereview.chromium.org/1519293002/diff/140001/cc/proto/layer_tree_ho... cc/proto/layer_tree_host.proto:19: message LayerTreeHost { Can you add a comment here for whether all of these are serialized or just some and why? https://codereview.chromium.org/1519293002/diff/140001/cc/proto/layer_tree_ho... cc/proto/layer_tree_host.proto:40: optional uint32 background_color = 21; /* SkColor */ should we add an SkColor proto message or do you think knowing that it's an uint32 is good enough? https://codereview.chromium.org/1519293002/diff/140001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host.cc (right): https://codereview.chromium.org/1519293002/diff/140001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host.cc:1290: // outside of the LayerTreeHost serialization. SwapPromise are currently not Can you make all of the different things point form so it's easier to read? https://codereview.chromium.org/1519293002/diff/140001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host.cc:1295: // TODO(nyquist): Figure out how to support animations. crbug? https://codereview.chromium.org/1519293002/diff/140001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host.cc:1339: LayerSelectionToProtobuf(selection_, proto->mutable_selection()); I'm a little bit sad that some things are out of line FooToProtobuf functions and some are foo.ToProtobuf functions. Can you file a general bug to take a look at this and possibly unify the convention? https://codereview.chromium.org/1519293002/diff/140001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host.cc:1359: } blank line after this please https://codereview.chromium.org/1519293002/diff/140001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host.cc:1360: layer_id_map_.clear(); // Populate layer_id_map_ with the new layers. https://codereview.chromium.org/1519293002/diff/140001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host.cc:1369: hud_layer_->SetLayerTreeHost(nullptr); blank line after this please https://codereview.chromium.org/1519293002/diff/140001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host.cc:1370: if (proto.hud_layer_id() == Layer::INVALID_ID) { Should we check if it's the same layer id to avoid work? https://codereview.chromium.org/1519293002/diff/140001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host.cc:1399: if (overscroll_elasticity_layer_) Same comment for all these layers as for the hud layer. Also can you move either the hud layer thing here or all of the layers up? Just to group things up better, or is there some dependency that isn't obvious? https://codereview.chromium.org/1519293002/diff/140001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host.h (right): https://codereview.chromium.org/1519293002/diff/140001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host.h:374: // protobuf message. Not all members are serialized as they are not helpful Put a similar note (Not all members..) in the proto file as well, please. https://codereview.chromium.org/1519293002/diff/140001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_unittest_serialization.cc (right): https://codereview.chromium.org/1519293002/diff/140001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_unittest_serialization.cc:28: : fake_client_src_(FakeLayerTreeHostClient::DIRECT_3D), just client_src_ and client_dst_ is fine https://codereview.chromium.org/1519293002/diff/140001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_unittest_serialization.cc:71: if (layer_tree_host_src_->hud_layer_) { group up ifs with other ifs, please https://codereview.chromium.org/1519293002/diff/140001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_unittest_serialization.cc:251: TestTaskGraphRunner task_graph_runner_src_; nit: private: or protected:
vmpstr: PTAL https://codereview.chromium.org/1519293002/diff/140001/cc/proto/layer_tree_ho... File cc/proto/layer_tree_host.proto (right): https://codereview.chromium.org/1519293002/diff/140001/cc/proto/layer_tree_ho... cc/proto/layer_tree_host.proto:19: message LayerTreeHost { On 2016/01/11 22:06:29, vmpstr wrote: > Can you add a comment here for whether all of these are serialized or just some > and why? Done. https://codereview.chromium.org/1519293002/diff/140001/cc/proto/layer_tree_ho... cc/proto/layer_tree_host.proto:40: optional uint32 background_color = 21; /* SkColor */ On 2016/01/11 22:06:29, vmpstr wrote: > should we add an SkColor proto message or do you think knowing that it's an > uint32 is good enough? It is defined as uint32_t, so I'd like to keep it as that for now. If you feel strongly that we should wrap the uint32_t in something else, I could do that as a follow-up. FWIW, we've used SkColor like this in previous patches. https://codereview.chromium.org/1519293002/diff/140001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host.cc (right): https://codereview.chromium.org/1519293002/diff/140001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host.cc:1290: // outside of the LayerTreeHost serialization. SwapPromise are currently not On 2016/01/11 22:06:29, vmpstr wrote: > Can you make all of the different things point form so it's easier to read? Done. https://codereview.chromium.org/1519293002/diff/140001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host.cc:1295: // TODO(nyquist): Figure out how to support animations. On 2016/01/11 22:06:30, vmpstr wrote: > crbug? OK with reusing the one for animationcontroller? https://code.google.com/p/chromium/issues/detail?id=570376 https://codereview.chromium.org/1519293002/diff/140001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host.cc:1339: LayerSelectionToProtobuf(selection_, proto->mutable_selection()); On 2016/01/11 22:06:30, vmpstr wrote: > I'm a little bit sad that some things are out of line FooToProtobuf functions > and some are foo.ToProtobuf functions. Can you file a general bug to take a look > at this and possibly unify the convention? Agreed. See crbug.com/548432 which is tracking that effort. https://codereview.chromium.org/1519293002/diff/140001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host.cc:1359: } On 2016/01/11 22:06:30, vmpstr wrote: > blank line after this please Done. https://codereview.chromium.org/1519293002/diff/140001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host.cc:1360: layer_id_map_.clear(); On 2016/01/11 22:06:30, vmpstr wrote: > // Populate layer_id_map_ with the new layers. Done. https://codereview.chromium.org/1519293002/diff/140001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host.cc:1369: hud_layer_->SetLayerTreeHost(nullptr); On 2016/01/11 22:06:30, vmpstr wrote: > blank line after this please Done. https://codereview.chromium.org/1519293002/diff/140001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host.cc:1370: if (proto.hud_layer_id() == Layer::INVALID_ID) { On 2016/01/11 22:06:30, vmpstr wrote: > Should we check if it's the same layer id to avoid work? Absolutely! Otherwise this apparently has a bug when we try to run the whole system, as SetLayerTreeHost might delete it from the map. https://codereview.chromium.org/1519293002/diff/140001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host.cc:1399: if (overscroll_elasticity_layer_) On 2016/01/11 22:06:30, vmpstr wrote: > Same comment for all these layers as for the hud layer. Also can you move either > the hud layer thing here or all of the layers up? Just to group things up > better, or is there some dependency that isn't obvious? Done. https://codereview.chromium.org/1519293002/diff/140001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host.h (right): https://codereview.chromium.org/1519293002/diff/140001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host.h:374: // protobuf message. Not all members are serialized as they are not helpful On 2016/01/11 22:06:30, vmpstr wrote: > Put a similar note (Not all members..) in the proto file as well, please. Done. https://codereview.chromium.org/1519293002/diff/140001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_unittest_serialization.cc (right): https://codereview.chromium.org/1519293002/diff/140001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_unittest_serialization.cc:28: : fake_client_src_(FakeLayerTreeHostClient::DIRECT_3D), On 2016/01/11 22:06:30, vmpstr wrote: > just client_src_ and client_dst_ is fine Done. https://codereview.chromium.org/1519293002/diff/140001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_unittest_serialization.cc:71: if (layer_tree_host_src_->hud_layer_) { On 2016/01/11 22:06:30, vmpstr wrote: > group up ifs with other ifs, please Done. https://codereview.chromium.org/1519293002/diff/140001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_unittest_serialization.cc:251: TestTaskGraphRunner task_graph_runner_src_; On 2016/01/11 22:06:30, vmpstr wrote: > nit: private: or protected: Done.
The CQ bit was checked by nyquist@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1519293002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1519293002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Other than a comment below looks good. https://codereview.chromium.org/1519293002/diff/160001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host.cc (right): https://codereview.chromium.org/1519293002/diff/160001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host.cc:1397: if (proto.hud_layer_id() == Layer::INVALID_ID) { What do you think of a function Layer* UpdateAndGetLayer(Layer* current_layer, int layer_id) { if (layer_id == Layer::INVALID_ID) { if (current_layer) current_layer->SetLayerTreeHost(nullptr) return nullptr; } else { auto layer_it = layer_id_map_.find(layer_id); DCHECK(layer_it != layer_id_map_.end()); if (current_layer && current_layer != layer_it->second) current_layer->SetLayerTreeHost(nullptr); return layer_it->second; } NOTREACHED(); return nullptr; } Then all of these become something along the lines of hud_layer_ = static_cast<HeadsUpDisplayLayer*>( UpdateAndGetLayer(hud_layer_, proto.hud_layer_id()); overscroll_elasticity_layer_ = UpdateAndGetLayer( overscroll_elasticity_layer_, proto.overscroll_elasticity_layer_id()); page_scale_layer_ = UpdateAndGetLayer( page_scale_layer_, proto.page_scale_layer_id()); etc?
vmpstr: PTAL https://codereview.chromium.org/1519293002/diff/160001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host.cc (right): https://codereview.chromium.org/1519293002/diff/160001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host.cc:1397: if (proto.hud_layer_id() == Layer::INVALID_ID) { On 2016/01/13 23:38:14, vmpstr wrote: > What do you think of a function > > Layer* UpdateAndGetLayer(Layer* current_layer, int layer_id) { > if (layer_id == Layer::INVALID_ID) { > if (current_layer) > current_layer->SetLayerTreeHost(nullptr) > return nullptr; > } else { > auto layer_it = layer_id_map_.find(layer_id); > DCHECK(layer_it != layer_id_map_.end()); > if (current_layer && current_layer != layer_it->second) > current_layer->SetLayerTreeHost(nullptr); > return layer_it->second; > } > NOTREACHED(); > return nullptr; > } > > Then all of these become something along the lines of > > hud_layer_ = static_cast<HeadsUpDisplayLayer*>( > UpdateAndGetLayer(hud_layer_, proto.hud_layer_id()); > overscroll_elasticity_layer_ = UpdateAndGetLayer( > overscroll_elasticity_layer_, proto.overscroll_elasticity_layer_id()); > page_scale_layer_ = UpdateAndGetLayer( > page_scale_layer_, proto.page_scale_layer_id()); > > etc? Done. I first added it as a private function, since it needed access to layer_id_map_. I then tried keeping it as an anonymous function, but the sad part is that I had to specify the type of the LayerIdMap manually. I don't feel strongly either way. Care to give guidance?
The CQ bit was checked by nyquist@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1519293002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1519293002/200001
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
The CQ bit was checked by nyquist@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org Link to the patchset: https://codereview.chromium.org/1519293002/#ps220001 (title: "Remove unreachable code, and use early return")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1519293002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1519293002/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Add support for (de)serializing LayerTreeHost. As part of the cc commit flow, we need to be able to serialize the LayerTreeHost. Not all state is necessary to serialize to be able to do a commit on the client side, so only some members are included in the proto. BUG=561210 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Add support for (de)serializing LayerTreeHost. As part of the cc commit flow, we need to be able to serialize the LayerTreeHost. Not all state is necessary to serialize to be able to do a commit on the client side, so only some members are included in the proto. BUG=561210 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Committed: https://crrev.com/4f5e8afdadf36c01a01467abb441be357db6d142 Cr-Commit-Position: refs/heads/master@{#369450} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/4f5e8afdadf36c01a01467abb441be357db6d142 Cr-Commit-Position: refs/heads/master@{#369450}
Message was sent while issue was closed.
hans@chromium.org changed reviewers: + hans@chromium.org
Message was sent while issue was closed.
This is causing failures under UBSan Vptr: https://build.chromium.org/p/chromium.fyi/builders/ClangToTLinuxUBSanVptr%20t...
Message was sent while issue was closed.
aizatsky@chromium.org changed reviewers: + aizatsky@chromium.org
Message was sent while issue was closed.
Looks like this change breaks CFI build: https://build.chromium.org/p/chromium.fyi/builders/CFI%20Linux/builds/3948/st...
Message was sent while issue was closed.
On 2016/01/15 01:19:32, aizatsky wrote: > Looks like this change breaks CFI build: > > https://build.chromium.org/p/chromium.fyi/builders/CFI%20Linux/builds/3948/st... OK. Thanks! I'll have a look.
Message was sent while issue was closed.
On 2016/01/15 01:22:39, nyquist wrote: > On 2016/01/15 01:19:32, aizatsky wrote: > > Looks like this change breaks CFI build: > > > > > https://build.chromium.org/p/chromium.fyi/builders/CFI%20Linux/builds/3948/st... > > OK. Thanks! I'll have a look. I've filed https://crbug.com/577972 with more detailed CFI report. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
