Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1398443008:
Add support for (de)serializing cc::Layer hierarchy. (Closed)
DescriptionAdd support for (de)serializing cc::Layer hierarchy.
This CL adds a protocol buffer for serializing the Layer hierarchy,
including the mask and replica layers. This will be used whenever the
hierarchy has changed and needs to be sent to the client.
Given that the protocol still is in flux, and for good measure, all
fields in the protocol buffer are marked as optional, even the
currently required fields.
When a hierarchy is serialized, the whole hierarchy is serialized from
the root node, instead of just the changed subtree.
Most of the logic lives in the Layer class, but the possibility to
change the root of the hierarchy is extracted out to a
LayerProtoConverter. It also has functionality to build up a map of
the current tree, which will also be used during property
deserialization.
The next CL will deal with serializing the properties:
https://codereview.chromium.org/1423523002/
BUG=538710
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/353b7d5d108750e6af2e254bc7e20ee0043b1d99
Cr-Commit-Position: refs/heads/master@{#358424}
Patch Set 1 #Patch Set 2 : Added hierarchical serialization and deserialization (no properties) #Patch Set 3 : Add missing serialization of mask and replica layer properties #Patch Set 4 : Cleaned up quite a bit, still missing one unittest #Patch Set 5 : Added last tests and fixed CC_EXPORT #Patch Set 6 : Fixed CC_EXPORT #
Total comments: 35
Patch Set 7 : Addressed most comments, except merging new functionality #Patch Set 8 : Moved functionality outside of Layer to LayerProtoConverter. Updated test-comments to be multiline. #
Total comments: 13
Patch Set 9 : Addressed comments from vmpstr #
Total comments: 4
Patch Set 10 : Addressed comments from vmpstr@ again #Patch Set 11 : git merge origin/master #Patch Set 12 : Fix GYP build in //cc #
Total comments: 6
Patch Set 13 : Merged origin/master #Patch Set 14 : Remove include of layer.pb.h #Patch Set 15 : Added descriptive comment #Patch Set 16 : Reorderded TODO #
Dependent Patchsets: Messages
Total messages: 66 (27 generated)
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/1398443008/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1398443008/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...) cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from ========== Add support for (de)serializing cc::Layer. BUG=538710 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Add support for (de)serializing cc::Layer. BUG=538710 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
nyquist@chromium.org changed reviewers: + dtrainor@chromium.org, vmpstr@chromium.org
The CQ bit was checked by nyquist@chromium.org to run a CQ dry run
Description was changed from ========== Add support for (de)serializing cc::Layer. This CL adds a protocol buffer for serializing the Layer hierarchy, including the mask and replica layers. This will be used whenever the hierarchy has changed and needs to be sent to the client. Given that the protocol still is in flux, and for good measure, all fields in the protocol buffer are marked as optional, even the currently required fields. When a hierarchy is serialized, the whole hierarchy is serialized from the root node, instead of just the changed subtree. Most of the logic lives in the Layer class, but the possibility to change the root of the hierarchy is extracted out to a LayerDeserializer. It also has functionality to build up a map of the current tree, which will also be used during property deserialization. The next CL will deal with serializing the properties: https://codereview.chromium.org/1423523002/ BUG=538710 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Add support for (de)serializing cc::Layer hierarchy. This CL adds a protocol buffer for serializing the Layer hierarchy, including the mask and replica layers. This will be used whenever the hierarchy has changed and needs to be sent to the client. Given that the protocol still is in flux, and for good measure, all fields in the protocol buffer are marked as optional, even the currently required fields. When a hierarchy is serialized, the whole hierarchy is serialized from the root node, instead of just the changed subtree. Most of the logic lives in the Layer class, but the possibility to change the root of the hierarchy is extracted out to a LayerDeserializer. It also has functionality to build up a map of the current tree, which will also be used during property deserialization. The next CL will deal with serializing the properties: https://codereview.chromium.org/1423523002/ BUG=538710 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1398443008/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1398443008/80001
vmpstr: PTAL as cc/ OWNERS dtrainor: PTAL for blimp
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
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/1398443008/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1398443008/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Do you think it's possible to combine LayerSerializer/LayerDeserializer/LayerProtoFactory into one class that deals with Layer <-> proto conversions? LayerProtoConverter maybe? Wdyt? https://codereview.chromium.org/1398443008/diff/100001/cc/layers/layer.cc File cc/layers/layer.cc (right): https://codereview.chromium.org/1398443008/diff/100001/cc/layers/layer.cc#new... cc/layers/layer.cc:1350: void Layer::ToLayerNodeProto(proto::LayerNode* proto) { Can this function be const? https://codereview.chromium.org/1398443008/diff/100001/cc/layers/layer.cc#new... cc/layers/layer.cc:1354: if (parent_) { nit: braces optional. https://codereview.chromium.org/1398443008/diff/100001/cc/layers/layer.cc#new... cc/layers/layer.cc:1357: proto->clear_children(); DCHECK that children size is 0 instead? Or are there situations where there may be children? What are those situations? https://codereview.chromium.org/1398443008/diff/100001/cc/layers/layer.cc#new... cc/layers/layer.cc:1358: for (auto child : children_) { What's the type of child? if it's a raw pointer, then you should say auto*, if it's a ref pointer then you should do const auto& child. https://codereview.chromium.org/1398443008/diff/100001/cc/layers/layer.cc#new... cc/layers/layer.cc:1370: if (proto.has_id()) Are there cases where this isn't set? If not, then DCHECK(proto.has_id()); instead https://codereview.chromium.org/1398443008/diff/100001/cc/layers/layer.cc#new... cc/layers/layer.cc:1374: RemoveAllChildren(); I assume this is needed because we're using layer_map, right? That is, the layers that we get might already have children. https://codereview.chromium.org/1398443008/diff/100001/cc/layers/layer.cc#new... cc/layers/layer.cc:1377: if (!child_proto.has_type()) { When would this happen? https://codereview.chromium.org/1398443008/diff/100001/cc/layers/layer_deseri... File cc/layers/layer_deserializer.cc (right): https://codereview.chromium.org/1398443008/diff/100001/cc/layers/layer_deseri... cc/layers/layer_deserializer.cc:27: (root_node.has_id() && root_node.id() != (uint32)existing_root->id())) { no c-style casts, is the type of the id in the proto different from the type in the class? Why is it different? https://codereview.chromium.org/1398443008/diff/100001/cc/layers/layer_deseri... cc/layers/layer_deserializer.cc:38: void LayerDeserializer::RecursivelyFindAllLayers(scoped_refptr<Layer> layer, Can you use LayerTreeHostComon::CallFunctionForSubtree instead? https://codereview.chromium.org/1398443008/diff/100001/cc/layers/layer_deseri... File cc/layers/layer_deserializer.h (right): https://codereview.chromium.org/1398443008/diff/100001/cc/layers/layer_deseri... cc/layers/layer_deserializer.h:37: static void RecursivelyFindAllLayers(const scoped_refptr<Layer> layer, const scoped_refptr<Layer>& layer? https://codereview.chromium.org/1398443008/diff/100001/cc/layers/layer_deseri... File cc/layers/layer_deserializer_unittest.cc (right): https://codereview.chromium.org/1398443008/diff/100001/cc/layers/layer_deseri... cc/layers/layer_deserializer_unittest.cc:15: class LayerDeserializerTest : public testing::Test { Do you need this? Can you just do TEST(LayerDeserializerTest, TestKeepingRoot) instead of TEST_F? https://codereview.chromium.org/1398443008/diff/100001/cc/layers/layer_deseri... cc/layers/layer_deserializer_unittest.cc:36: child_a_node.set_id(442); Same ids for a and b? https://codereview.chromium.org/1398443008/diff/100001/cc/layers/layer_deseri... cc/layers/layer_deserializer_unittest.cc:54: EXPECT_EQ(old_root->id(), new_root->id()); Can you verify the full structure here? https://codereview.chromium.org/1398443008/diff/100001/cc/layers/layer_proto_... File cc/layers/layer_proto_factory.cc (right): https://codereview.chromium.org/1398443008/diff/100001/cc/layers/layer_proto_... cc/layers/layer_proto_factory.cc:17: if (proto.has_id()) { Can you DCHECK that it has id? https://codereview.chromium.org/1398443008/diff/100001/cc/layers/layer_proto_... cc/layers/layer_proto_factory.cc:22: if (!proto.has_type()) When can this happen? I don't think we handle nullptr in the rest of the code https://codereview.chromium.org/1398443008/diff/100001/cc/layers/layer_unitte... File cc/layers/layer_unittest.cc (right): https://codereview.chromium.org/1398443008/diff/100001/cc/layers/layer_unitte... cc/layers/layer_unittest.cc:1380: ASSERT_EQ(2UL, layer_dest_root->children().size()); nit: we just usually lower case u, like 2u https://codereview.chromium.org/1398443008/diff/100001/cc/proto/layer.proto File cc/proto/layer.proto (right): https://codereview.chromium.org/1398443008/diff/100001/cc/proto/layer.proto#n... cc/proto/layer.proto:21: // required I made a few comments throughout the code, but I think you should just DCHECK fields that are required by the protocol
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/1398443008/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1398443008/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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/1398443008/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1398443008/140001
vmpstr: PTAL. Patch set 7 addresses all the code-specific comments you had. Patch set 8 moves all the new code into the suggested common class. I split them in two patch sets so it would be easy for you to see what I had addressed and how. https://codereview.chromium.org/1398443008/diff/100001/cc/layers/layer.cc File cc/layers/layer.cc (right): https://codereview.chromium.org/1398443008/diff/100001/cc/layers/layer.cc#new... cc/layers/layer.cc:1350: void Layer::ToLayerNodeProto(proto::LayerNode* proto) { On 2015/10/21 22:34:14, vmpstr wrote: > Can this function be const? Done. https://codereview.chromium.org/1398443008/diff/100001/cc/layers/layer.cc#new... cc/layers/layer.cc:1354: if (parent_) { On 2015/10/21 22:34:14, vmpstr wrote: > nit: braces optional. Done. https://codereview.chromium.org/1398443008/diff/100001/cc/layers/layer.cc#new... cc/layers/layer.cc:1357: proto->clear_children(); On 2015/10/21 22:34:13, vmpstr wrote: > DCHECK that children size is 0 instead? Or are there situations where there may > be children? What are those situations? Done. https://codereview.chromium.org/1398443008/diff/100001/cc/layers/layer.cc#new... cc/layers/layer.cc:1358: for (auto child : children_) { On 2015/10/21 22:34:14, vmpstr wrote: > What's the type of child? if it's a raw pointer, then you should say auto*, if > it's a ref pointer then you should do const auto& child. Done. https://codereview.chromium.org/1398443008/diff/100001/cc/layers/layer.cc#new... cc/layers/layer.cc:1370: if (proto.has_id()) On 2015/10/21 22:34:13, vmpstr wrote: > Are there cases where this isn't set? If not, then DCHECK(proto.has_id()); > instead Done. https://codereview.chromium.org/1398443008/diff/100001/cc/layers/layer.cc#new... cc/layers/layer.cc:1374: RemoveAllChildren(); Yeah, the issue is if you remove all children from a Layer, we will send no children through the proto, which means that we will not add any new ones. But, we also don't delete the old ones... Since the proto structure contains the whole hierarchy, we can safely first delete all children, and then add the ones we got. We could of course check if the children are the same as last time, but I figured that'd complicate things. The test "...RemoveSubtree..." tests this specific line in fact, since it ensures that the number of children are correct. Added a comment to clarify this in the code as well. https://codereview.chromium.org/1398443008/diff/100001/cc/layers/layer.cc#new... cc/layers/layer.cc:1377: if (!child_proto.has_type()) { On 2015/10/21 22:34:13, vmpstr wrote: > When would this happen? DCHECK here as well now, so done. https://codereview.chromium.org/1398443008/diff/100001/cc/layers/layer_deseri... File cc/layers/layer_deserializer.cc (right): https://codereview.chromium.org/1398443008/diff/100001/cc/layers/layer_deseri... cc/layers/layer_deserializer.cc:27: (root_node.has_id() && root_node.id() != (uint32)existing_root->id())) { On 2015/10/21 22:34:14, vmpstr wrote: > no c-style casts, is the type of the id in the proto different from the type in > the class? Why is it different? My bad. Fixed proto. https://codereview.chromium.org/1398443008/diff/100001/cc/layers/layer_deseri... cc/layers/layer_deserializer.cc:38: void LayerDeserializer::RecursivelyFindAllLayers(scoped_refptr<Layer> layer, On 2015/10/21 22:34:14, vmpstr wrote: > Can you use LayerTreeHostComon::CallFunctionForSubtree instead? Awesome! Thanks! https://codereview.chromium.org/1398443008/diff/100001/cc/layers/layer_deseri... File cc/layers/layer_deserializer.h (right): https://codereview.chromium.org/1398443008/diff/100001/cc/layers/layer_deseri... cc/layers/layer_deserializer.h:37: static void RecursivelyFindAllLayers(const scoped_refptr<Layer> layer, On 2015/10/21 22:34:14, vmpstr wrote: > const scoped_refptr<Layer>& layer? Done. https://codereview.chromium.org/1398443008/diff/100001/cc/layers/layer_deseri... File cc/layers/layer_deserializer_unittest.cc (right): https://codereview.chromium.org/1398443008/diff/100001/cc/layers/layer_deseri... cc/layers/layer_deserializer_unittest.cc:15: class LayerDeserializerTest : public testing::Test { On 2015/10/21 22:34:14, vmpstr wrote: > Do you need this? Can you just do TEST(LayerDeserializerTest, TestKeepingRoot) > instead of TEST_F? Done. https://codereview.chromium.org/1398443008/diff/100001/cc/layers/layer_deseri... cc/layers/layer_deserializer_unittest.cc:36: child_a_node.set_id(442); On 2015/10/21 22:34:14, vmpstr wrote: > Same ids for a and b? Doh! Fixed. https://codereview.chromium.org/1398443008/diff/100001/cc/layers/layer_deseri... cc/layers/layer_deserializer_unittest.cc:54: EXPECT_EQ(old_root->id(), new_root->id()); On 2015/10/21 22:34:14, vmpstr wrote: > Can you verify the full structure here? Done here and below. Also updated how the structure is set up a little bit. Same structure, different code. https://codereview.chromium.org/1398443008/diff/100001/cc/layers/layer_proto_... File cc/layers/layer_proto_factory.cc (right): https://codereview.chromium.org/1398443008/diff/100001/cc/layers/layer_proto_... cc/layers/layer_proto_factory.cc:17: if (proto.has_id()) { On 2015/10/21 22:34:14, vmpstr wrote: > Can you DCHECK that it has id? Done. https://codereview.chromium.org/1398443008/diff/100001/cc/layers/layer_proto_... cc/layers/layer_proto_factory.cc:22: if (!proto.has_type()) On 2015/10/21 22:34:14, vmpstr wrote: > When can this happen? I don't think we handle nullptr in the rest of the code Done. https://codereview.chromium.org/1398443008/diff/100001/cc/layers/layer_unitte... File cc/layers/layer_unittest.cc (right): https://codereview.chromium.org/1398443008/diff/100001/cc/layers/layer_unitte... cc/layers/layer_unittest.cc:1380: ASSERT_EQ(2UL, layer_dest_root->children().size()); On 2015/10/21 22:34:14, vmpstr wrote: > nit: we just usually lower case u, like 2u Done. https://codereview.chromium.org/1398443008/diff/100001/cc/proto/layer.proto File cc/proto/layer.proto (right): https://codereview.chromium.org/1398443008/diff/100001/cc/proto/layer.proto#n... cc/proto/layer.proto:21: // required On 2015/10/21 22:34:14, vmpstr wrote: > I made a few comments throughout the code, but I think you should just DCHECK > fields that are required by the protocol I've implemented this. To explain my reasoning: We are receiving this over a network channel, where things can go bad over the wire, or a server that is misbehaving, etc. I figured I'd code defensively and ensure we don't use bad data. However, I realize that we should instead do some kind of validation of the data at a higher level possibly instead, and have a way of recovering from it (asking server to re-send or re-initialize, etc.) instead of littering the code with if-statements and ending up with badly constructed objects.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/1398443008/diff/100001/cc/proto/layer.proto File cc/proto/layer.proto (right): https://codereview.chromium.org/1398443008/diff/100001/cc/proto/layer.proto#n... cc/proto/layer.proto:21: // required On 2015/10/23 00:12:31, nyquist (OOO - back 11-4) wrote: > On 2015/10/21 22:34:14, vmpstr wrote: > > I made a few comments throughout the code, but I think you should just DCHECK > > fields that are required by the protocol > > I've implemented this. > > To explain my reasoning: We are receiving this over a network channel, where > things can go bad over the wire, or a server that is misbehaving, etc. I figured > I'd code defensively and ensure we don't use bad data. However, I realize that > we should instead do some kind of validation of the data at a higher level > possibly instead, and have a way of recovering from it (asking server to re-send > or re-initialize, etc.) instead of littering the code with if-statements and > ending up with badly constructed objects. Ya that's totally understandable. However, at this point, I'd prefer to dcheck everything instead of handle error cases. The reason for that is that bugs that we introduce now could cause these error condition and go unnoticed for a long time, because we handled them. If we DCHECK, we can catch the bugs now. In the future, when DCHECKs fail incorrectly or if the protocol changes, then maybe we can add error handling. This is partly the reason why I asked (here or in some other patch) on what the story is for server evolving separately from the client. That's an important thing to get right. https://codereview.chromium.org/1398443008/diff/140001/cc/layers/layer.cc File cc/layers/layer.cc (right): https://codereview.chromium.org/1398443008/diff/140001/cc/layers/layer.cc#new... cc/layers/layer.cc:1352: proto->set_type(proto::Base); I assume that layer overrides would do something like void PictureLayer::ToLayerNodeProto(proto::LayerNode* proto) const { Layer::ToLayerNodeProto(proto); proto->set_some_picture_specific_thing(); } If that's the case, then setting the type here is awkward, since the derived class has to set it to? Or is the plan to just override it? Alternatively, if ids, children, and types are the only things that we want to record into LayerNodes then maybe ToLayerNodeProto doesn't have to be virtual, and instead have a virtual GetTypeForProtoSerialization() or something that just returns proto::Base or specific one for each layer? I'm not sure that's the best either. Anyway, what's the plan? :) https://codereview.chromium.org/1398443008/diff/140001/cc/layers/layer.h File cc/layers/layer.h (right): https://codereview.chromium.org/1398443008/diff/140001/cc/layers/layer.h#newc... cc/layers/layer.h:371: // Recursively iterate over the given LayerNode proto and read the structure nit: can you put a blank line before this one? https://codereview.chromium.org/1398443008/diff/140001/cc/layers/layer_proto_... File cc/layers/layer_proto_converter.cc (right): https://codereview.chromium.org/1398443008/diff/140001/cc/layers/layer_proto_... cc/layers/layer_proto_converter.cc:63: case proto::Base: Do other cases exist already? If there's only Base for now, I'd prefer you drop the default case, so that when new enums are added it won't compile until the author updates this switch. https://codereview.chromium.org/1398443008/diff/140001/cc/layers/layer_proto_... File cc/layers/layer_proto_converter.h (right): https://codereview.chromium.org/1398443008/diff/140001/cc/layers/layer_proto_... cc/layers/layer_proto_converter.h:43: virtual ~LayerProtoConverter(); Doesn't need to be virtual. https://codereview.chromium.org/1398443008/diff/140001/cc/layers/layer_proto_... cc/layers/layer_proto_converter.h:45: typedef base::hash_map<int, scoped_refptr<Layer>> LayerIdMap; new code should be doing this: using LayerIdMap = base::hash_map<int, scoped_refptr<Layer>>; https://codereview.chromium.org/1398443008/diff/140001/cc/layers/layer_proto_... cc/layers/layer_proto_converter.h:51: DISALLOW_COPY_AND_ASSIGN(LayerProtoConverter); I don't think you need this if the constructor is private :)
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/1398443008/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1398443008/160001
vmpstr: PTAL https://codereview.chromium.org/1398443008/diff/140001/cc/layers/layer.cc File cc/layers/layer.cc (right): https://codereview.chromium.org/1398443008/diff/140001/cc/layers/layer.cc#new... cc/layers/layer.cc:1352: proto->set_type(proto::Base); On 2015/10/23 21:26:10, vmpstr wrote: > I assume that layer overrides would do something like > > void PictureLayer::ToLayerNodeProto(proto::LayerNode* proto) const { > Layer::ToLayerNodeProto(proto); > proto->set_some_picture_specific_thing(); > } > > If that's the case, then setting the type here is awkward, since the derived > class has to set it to? Or is the plan to just override it? > > Alternatively, if ids, children, and types are the only things that we want to > record into LayerNodes then maybe ToLayerNodeProto doesn't have to be virtual, > and instead have a virtual GetTypeForProtoSerialization() or something that just > returns proto::Base or specific one for each layer? I'm not sure that's the best > either. > > Anyway, what's the plan? :) Done. From what I can understand, it should be enough for the hierarchy parts to be a generic thing except for type. Changing it to that for now until my understanding changes. Now this is non-virtual, but with a virtual GetType...() https://codereview.chromium.org/1398443008/diff/140001/cc/layers/layer.h File cc/layers/layer.h (right): https://codereview.chromium.org/1398443008/diff/140001/cc/layers/layer.h#newc... cc/layers/layer.h:371: // Recursively iterate over the given LayerNode proto and read the structure On 2015/10/23 21:26:10, vmpstr wrote: > nit: can you put a blank line before this one? Done. https://codereview.chromium.org/1398443008/diff/140001/cc/layers/layer_proto_... File cc/layers/layer_proto_converter.cc (right): https://codereview.chromium.org/1398443008/diff/140001/cc/layers/layer_proto_... cc/layers/layer_proto_converter.cc:63: case proto::Base: On 2015/10/23 21:26:10, vmpstr wrote: > Do other cases exist already? If there's only Base for now, I'd prefer you drop > the default case, so that when new enums are added it won't compile until the > author updates this switch. Done. For now it's only base (until the TODO is implemented). Unsure about the style you prefer for this though. Added a NOTREACHED() below the switch for now and returning nullptr to make it compile. Is that OK? Happy to change to whatever format you want! https://codereview.chromium.org/1398443008/diff/140001/cc/layers/layer_proto_... File cc/layers/layer_proto_converter.h (right): https://codereview.chromium.org/1398443008/diff/140001/cc/layers/layer_proto_... cc/layers/layer_proto_converter.h:43: virtual ~LayerProtoConverter(); On 2015/10/23 21:26:10, vmpstr wrote: > Doesn't need to be virtual. Done. https://codereview.chromium.org/1398443008/diff/140001/cc/layers/layer_proto_... cc/layers/layer_proto_converter.h:45: typedef base::hash_map<int, scoped_refptr<Layer>> LayerIdMap; On 2015/10/23 21:26:10, vmpstr wrote: > new code should be doing this: > > using LayerIdMap = base::hash_map<int, scoped_refptr<Layer>>; Done. https://codereview.chromium.org/1398443008/diff/140001/cc/layers/layer_proto_... cc/layers/layer_proto_converter.h:51: DISALLOW_COPY_AND_ASSIGN(LayerProtoConverter); On 2015/10/23 21:26:10, vmpstr wrote: > I don't think you need this if the constructor is private :) Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
CC'ing wez@ and kmarshall@ for FYI.
So my understanding is that this is just the structural serialization (no actual data aside from just the layer types and child/parent relationships). Is that correct? https://codereview.chromium.org/1398443008/diff/140001/cc/layers/layer_proto_... File cc/layers/layer_proto_converter.cc (right): https://codereview.chromium.org/1398443008/diff/140001/cc/layers/layer_proto_... cc/layers/layer_proto_converter.cc:63: case proto::Base: On 2015/10/26 03:14:29, nyquist (OOO - back 11-4) wrote: > On 2015/10/23 21:26:10, vmpstr wrote: > > Do other cases exist already? If there's only Base for now, I'd prefer you > drop > > the default case, so that when new enums are added it won't compile until the > > author updates this switch. > > Done. > > For now it's only base (until the TODO is implemented). Unsure about the style > you prefer for this though. Added a NOTREACHED() below the switch for now and > returning nullptr to make it compile. Is that OK? Happy to change to whatever > format you want! Yep, that's perfect. https://codereview.chromium.org/1398443008/diff/160001/cc/layers/layer.h File cc/layers/layer.h (right): https://codereview.chromium.org/1398443008/diff/160001/cc/layers/layer.h#newc... cc/layers/layer.h:372: void ToLayerNodeProto(proto::LayerNode* proto) const; I meant like // comment // comment void function // comment // comment void function :) https://codereview.chromium.org/1398443008/diff/160001/cc/layers/layer.h#newc... cc/layers/layer.h:377: virtual void FromLayerNodeProto(const proto::LayerNode& proto, Can this also be non virtual?
vmpstr: PTAL https://codereview.chromium.org/1398443008/diff/160001/cc/layers/layer.h File cc/layers/layer.h (right): https://codereview.chromium.org/1398443008/diff/160001/cc/layers/layer.h#newc... cc/layers/layer.h:372: void ToLayerNodeProto(proto::LayerNode* proto) const; On 2015/10/26 17:51:45, vmpstr wrote: > I meant like > > // comment > // comment > void function > > // comment > // comment > void function > > :) Wow... I don't even... Sorry... :-/ https://codereview.chromium.org/1398443008/diff/160001/cc/layers/layer.h#newc... cc/layers/layer.h:377: virtual void FromLayerNodeProto(const proto::LayerNode& proto, On 2015/10/26 17:51:45, vmpstr wrote: > Can this also be non virtual? Doh... Yeah! 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/1398443008/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1398443008/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) cast_shell_android on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...) cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
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/1398443008/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1398443008/220001
Description was changed from ========== Add support for (de)serializing cc::Layer hierarchy. This CL adds a protocol buffer for serializing the Layer hierarchy, including the mask and replica layers. This will be used whenever the hierarchy has changed and needs to be sent to the client. Given that the protocol still is in flux, and for good measure, all fields in the protocol buffer are marked as optional, even the currently required fields. When a hierarchy is serialized, the whole hierarchy is serialized from the root node, instead of just the changed subtree. Most of the logic lives in the Layer class, but the possibility to change the root of the hierarchy is extracted out to a LayerDeserializer. It also has functionality to build up a map of the current tree, which will also be used during property deserialization. The next CL will deal with serializing the properties: https://codereview.chromium.org/1423523002/ BUG=538710 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Add support for (de)serializing cc::Layer hierarchy. This CL adds a protocol buffer for serializing the Layer hierarchy, including the mask and replica layers. This will be used whenever the hierarchy has changed and needs to be sent to the client. Given that the protocol still is in flux, and for good measure, all fields in the protocol buffer are marked as optional, even the currently required fields. When a hierarchy is serialized, the whole hierarchy is serialized from the root node, instead of just the changed subtree. Most of the logic lives in the Layer class, but the possibility to change the root of the hierarchy is extracted out to a LayerProtoConverter. It also has functionality to build up a map of the current tree, which will also be used during property deserialization. The next CL will deal with serializing the properties: https://codereview.chromium.org/1423523002/ BUG=538710 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) cast_shell_android on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...) cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) 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...) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
wez@chromium.org changed reviewers: + wez@chromium.org
https://codereview.chromium.org/1398443008/diff/220001/cc/layers/layer.h File cc/layers/layer.h (right): https://codereview.chromium.org/1398443008/diff/220001/cc/layers/layer.h#newc... cc/layers/layer.h:365: virtual proto::LayerType GetTypeForProtoSerialization() const; IIUC this was previously implicit in the behaviours overridden by Layer specializations? https://codereview.chromium.org/1398443008/diff/220001/cc/layers/layer.h#newc... cc/layers/layer.h:371: void ToLayerNodeProto(proto::LayerNode* proto) const; Is it necessary for these to live on the actual Layer base - they don't seem to touch anything private to the layer except for setting the mask_layer_ and replica_layer_ to nullptr directly? These methods are also non-virtual, so Layer specializations cannot override them; what happens if a specialization has additional state to handle?
https://codereview.chromium.org/1398443008/diff/220001/cc/layers/layer.h File cc/layers/layer.h (right): https://codereview.chromium.org/1398443008/diff/220001/cc/layers/layer.h#newc... cc/layers/layer.h:365: virtual proto::LayerType GetTypeForProtoSerialization() const; On 2015/10/27 03:07:06, Wez wrote: > IIUC this was previously implicit in the behaviours overridden by Layer > specializations? Yeah, it could have been done like that. However, vmpstr@ suggested that I'd keep the main method non-virtual and only make the layer type accessor to be virtual. https://codereview.chromium.org/1398443008/diff/220001/cc/layers/layer.h#newc... cc/layers/layer.h:371: void ToLayerNodeProto(proto::LayerNode* proto) const; On 2015/10/27 03:07:06, Wez wrote: > Is it necessary for these to live on the actual Layer base - they don't seem to > touch anything private to the layer except for setting the mask_layer_ and > replica_layer_ to nullptr directly? AFAIK there is no way to set the Layer id outside of the class (only in constructor), so it needs to live somewhere with write-access to that member. > These methods are also non-virtual, so Layer specializations cannot override > them; what happens if a specialization has additional state to handle? If that becomes the case, we can change this. As of now, this is how the hierarchy is organized.
lgtm, assuming wez@ is ok with it. https://codereview.chromium.org/1398443008/diff/220001/cc/layers/layer.h File cc/layers/layer.h (right): https://codereview.chromium.org/1398443008/diff/220001/cc/layers/layer.h#newc... cc/layers/layer.h:371: void ToLayerNodeProto(proto::LayerNode* proto) const; On 2015/10/28 16:03:44, nyquist (OOO - back 11-4) wrote: > On 2015/10/27 03:07:06, Wez wrote: > > Is it necessary for these to live on the actual Layer base - they don't seem > to > > touch anything private to the layer except for setting the mask_layer_ and > > replica_layer_ to nullptr directly? > > AFAIK there is no way to set the Layer id outside of the class (only in > constructor), so it needs to live somewhere with write-access to that member. > > > These methods are also non-virtual, so Layer specializations cannot override > > them; what happens if a specialization has additional state to handle? > > If that becomes the case, we can change this. As of now, this is how the > hierarchy is organized. Yeah, my suggestions only really apply if this is only used for hierarchical information of the tree. If these functions are also meant to serialize the actual contents of layers, then it needs a different approach. However, I'm under the impression that this will be done in a second pass (ie structure and contents are two different passes).
https://codereview.chromium.org/1398443008/diff/220001/cc/layers/layer.h File cc/layers/layer.h (right): https://codereview.chromium.org/1398443008/diff/220001/cc/layers/layer.h#newc... cc/layers/layer.h:371: void ToLayerNodeProto(proto::LayerNode* proto) const; On 2015/10/28 at 18:20:49, vmpstr wrote: > On 2015/10/28 16:03:44, nyquist (OOO - back 11-4) wrote: > > On 2015/10/27 03:07:06, Wez wrote: > > > Is it necessary for these to live on the actual Layer base - they don't seem > > to > > > touch anything private to the layer except for setting the mask_layer_ and > > > replica_layer_ to nullptr directly? > > > > AFAIK there is no way to set the Layer id outside of the class (only in > > constructor), so it needs to live somewhere with write-access to that member. It's not clear that we want to be setting the Layer-id, though - Layer-ids are allocated globally within the process, so if we allow marshalled data received from outside to create them with specific Ids, we risk clashes with any locally-allocated Layers. That will bite us if we e.g. process serialized Layer tree data from multiple sources. Could we use the LayerIdMap to decouple the Ids allocated by the sender from those we're managing at the receiver, i.e. allow the receiver to allocate arbitrary Layer Ids locally, and translate all Layer Ids in serialized data into those local Ids via the LayerIdMap? We could make LayerProtoConverter a stateful class and maintain the mapping there, for the various (de)serializers to use, for example. > > > These methods are also non-virtual, so Layer specializations cannot override > > > them; what happens if a specialization has additional state to handle? > > > > If that becomes the case, we can change this. As of now, this is how the > > hierarchy is organized. > > Yeah, my suggestions only really apply if this is only used for hierarchical information of the tree. If these functions are also meant to serialize the actual contents of layers, then it needs a different approach. However, I'm under the impression that this will be done in a second pass (ie structure and contents are two different passes).
On Wed, Oct 28, 2015 at 4:59 PM, <wez@chromium.org> wrote: > > https://codereview.chromium.org/1398443008/diff/220001/cc/layers/layer.h > File cc/layers/layer.h (right): > > > https://codereview.chromium.org/1398443008/diff/220001/cc/layers/layer.h#newc... > cc/layers/layer.h:371: void ToLayerNodeProto(proto::LayerNode* proto) > const; > On 2015/10/28 at 18:20:49, vmpstr wrote: > >> On 2015/10/28 16:03:44, nyquist (OOO - back 11-4) wrote: >> > On 2015/10/27 03:07:06, Wez wrote: >> > > Is it necessary for these to live on the actual Layer base - they >> > don't seem > >> > to >> > > touch anything private to the layer except for setting the >> > mask_layer_ and > >> > > replica_layer_ to nullptr directly? >> > >> > AFAIK there is no way to set the Layer id outside of the class (only >> > in > >> > constructor), so it needs to live somewhere with write-access to >> > that member. > > It's not clear that we want to be setting the Layer-id, though - > Layer-ids are allocated globally within the process, so if we allow > marshalled data received from outside to create them with specific Ids, > we risk clashes with any locally-allocated Layers. That will bite us if > we e.g. process serialized Layer tree data from multiple sources. > In what sort of situation would we do that? > > Could we use the LayerIdMap to decouple the Ids allocated by the sender > from those we're managing at the receiver, i.e. allow the receiver to > allocate arbitrary Layer Ids locally, and translate all Layer Ids in > serialized data into those local Ids via the LayerIdMap? We could make > LayerProtoConverter a stateful class and maintain the mapping there, for > the various (de)serializers to use, for example. > > > > > These methods are also non-virtual, so Layer specializations >> > cannot override > >> > > them; what happens if a specialization has additional state to >> > handle? > >> > >> > If that becomes the case, we can change this. As of now, this is how >> > the > >> > hierarchy is organized. >> > > Yeah, my suggestions only really apply if this is only used for >> > hierarchical information of the tree. If these functions are also meant > to serialize the actual contents of layers, then it needs a different > approach. However, I'm under the impression that this will be done in a > second pass (ie structure and contents are two different passes). > > https://codereview.chromium.org/1398443008/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/10/29 17:56:13, danakj wrote: > On Wed, Oct 28, 2015 at 4:59 PM, <mailto:wez@chromium.org> wrote: > > > > > https://codereview.chromium.org/1398443008/diff/220001/cc/layers/layer.h > > File cc/layers/layer.h (right): > > > > > > > https://codereview.chromium.org/1398443008/diff/220001/cc/layers/layer.h#newc... > > cc/layers/layer.h:371: void ToLayerNodeProto(proto::LayerNode* proto) > > const; > > On 2015/10/28 at 18:20:49, vmpstr wrote: > > > >> On 2015/10/28 16:03:44, nyquist (OOO - back 11-4) wrote: > >> > On 2015/10/27 03:07:06, Wez wrote: > >> > > Is it necessary for these to live on the actual Layer base - they > >> > > don't seem > > > >> > to > >> > > touch anything private to the layer except for setting the > >> > > mask_layer_ and > > > >> > > replica_layer_ to nullptr directly? > >> > > >> > AFAIK there is no way to set the Layer id outside of the class (only > >> > > in > > > >> > constructor), so it needs to live somewhere with write-access to > >> > > that member. > > > > It's not clear that we want to be setting the Layer-id, though - > > Layer-ids are allocated globally within the process, so if we allow > > marshalled data received from outside to create them with specific Ids, > > we risk clashes with any locally-allocated Layers. That will bite us if > > we e.g. process serialized Layer tree data from multiple sources. > > That's a good point, but I think we'll be okay. Because of where we're splitting this, the client and server compositor components have a 1:1 mapping. There's state going back and forth that might prohibit us from easily connecting two servers to one client. We'll create another client compositor instance if we need to show another tab or when we spin up a new renderer for the current tab. We'll have to do *some* resolution to figure out the destination of messages (process/render widget on server, compositor instance on client), but I think that should be handled at a higher level than these compositor messages. If we do want to composite local layers with the server source (maybe for some kind of compositor-driven UI?), I would imagine we'd do it somewhat like Clank and use the uber-comp delegated rendering layers (output of one compositor basically gets fed into a layer on another compositor). Although admittedly I haven't looked into this much. > > In what sort of situation would we do that? > > > > > > Could we use the LayerIdMap to decouple the Ids allocated by the sender > > from those we're managing at the receiver, i.e. allow the receiver to > > allocate arbitrary Layer Ids locally, and translate all Layer Ids in > > serialized data into those local Ids via the LayerIdMap? We could make > > LayerProtoConverter a stateful class and maintain the mapping there, for > > the various (de)serializers to use, for example. > > > > > > > > These methods are also non-virtual, so Layer specializations > >> > > cannot override > > > >> > > them; what happens if a specialization has additional state to > >> > > handle? > > > >> > > >> > If that becomes the case, we can change this. As of now, this is how > >> > > the > > > >> > hierarchy is organized. > >> > > > > Yeah, my suggestions only really apply if this is only used for > >> > > hierarchical information of the tree. If these functions are also meant > > to serialize the actual contents of layers, then it needs a different > > approach. However, I'm under the impression that this will be done in a > > second pass (ie structure and contents are two different passes). > > > > https://codereview.chromium.org/1398443008/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
On Sat, Oct 31, 2015 at 9:42 AM, <dtrainor@chromium.org> wrote: > On 2015/10/29 17:56:13, danakj wrote: > >> On Wed, Oct 28, 2015 at 4:59 PM, <mailto:wez@chromium.org> wrote: >> > > > >> > >> https://codereview.chromium.org/1398443008/diff/220001/cc/layers/layer.h >> > File cc/layers/layer.h (right): >> > >> > >> > >> > > > https://codereview.chromium.org/1398443008/diff/220001/cc/layers/layer.h#newc... > >> > cc/layers/layer.h:371: void ToLayerNodeProto(proto::LayerNode* proto) >> > const; >> > On 2015/10/28 at 18:20:49, vmpstr wrote: >> > >> >> On 2015/10/28 16:03:44, nyquist (OOO - back 11-4) wrote: >> >> > On 2015/10/27 03:07:06, Wez wrote: >> >> > > Is it necessary for these to live on the actual Layer base - they >> >> >> > don't seem >> > >> >> > to >> >> > > touch anything private to the layer except for setting the >> >> >> > mask_layer_ and >> > >> >> > > replica_layer_ to nullptr directly? >> >> > >> >> > AFAIK there is no way to set the Layer id outside of the class (only >> >> >> > in >> > >> >> > constructor), so it needs to live somewhere with write-access to >> >> >> > that member. >> > >> > It's not clear that we want to be setting the Layer-id, though - >> > Layer-ids are allocated globally within the process, so if we allow >> > marshalled data received from outside to create them with specific Ids, >> > we risk clashes with any locally-allocated Layers. That will bite us if >> > we e.g. process serialized Layer tree data from multiple sources. >> > >> > > That's a good point, but I think we'll be okay. Because of where we're > splitting this, the client and server compositor components have a 1:1 > mapping. > There's state going back and forth that might prohibit us from easily > connecting > two servers to one client. We'll create another client compositor > instance if > we need to show another tab or when we spin up a new renderer for the > current > tab. > > We'll have to do *some* resolution to figure out the destination of > messages > (process/render widget on server, compositor instance on client), but I > think > that should be handled at a higher level than these compositor messages. > > If we do want to composite local layers with the server source (maybe for > some > kind of compositor-driven UI?), I would imagine we'd do it somewhat like > Clank > and use the uber-comp delegated rendering layers (output of one compositor > basically gets fed into a layer on another compositor). Although > admittedly I > haven't looked into this much. FYI the delegated layers are deprecated, and replaced by surface layers and the surface-related code in cc/surfaces and in content/browser/compositor and friends. "Delegated rendering" should be deleted soon (except the cc::DelegatingRenderer class, it's used for surfaces too). > > > > In what sort of situation would we do that? >> > > > > >> > Could we use the LayerIdMap to decouple the Ids allocated by the sender >> > from those we're managing at the receiver, i.e. allow the receiver to >> > allocate arbitrary Layer Ids locally, and translate all Layer Ids in >> > serialized data into those local Ids via the LayerIdMap? We could make >> > LayerProtoConverter a stateful class and maintain the mapping there, for >> > the various (de)serializers to use, for example. >> > >> > >> > > > These methods are also non-virtual, so Layer specializations >> >> >> > cannot override >> > >> >> > > them; what happens if a specialization has additional state to >> >> >> > handle? >> > >> >> > >> >> > If that becomes the case, we can change this. As of now, this is how >> >> >> > the >> > >> >> > hierarchy is organized. >> >> >> > >> > Yeah, my suggestions only really apply if this is only used for >> >> >> > hierarchical information of the tree. If these functions are also meant >> > to serialize the actual contents of layers, then it needs a different >> > approach. However, I'm under the impression that this will be done in a >> > second pass (ie structure and contents are two different passes). >> > >> > https://codereview.chromium.org/1398443008/ >> > >> > > To unsubscribe from this group and stop receiving emails from it, send an >> > email > >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > > https://codereview.chromium.org/1398443008/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
wez@chromium.org changed reviewers: - wez@chromium.org
Deferring to David's judgement on the Id issue!
The CQ bit was checked by nyquist@chromium.org to run a CQ dry run
lgtm
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/1398443008/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1398443008/300001
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
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/1398443008/#ps300001 (title: "Reorderded TODO")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1398443008/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1398443008/300001
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/353b7d5d108750e6af2e254bc7e20ee0043b1d99 Cr-Commit-Position: refs/heads/master@{#358424} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
