Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1417963011:
Added serialization to protobufs for property trees. (Closed)
DescriptionAdd serialization to protobufs for property trees.
Add serialization tests.
Update existing tests to validate serialization.
BUG=554319
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/54f287ace5c9092a2ed3a8884d13d0087a83eaf0
Cr-Commit-Position: refs/heads/master@{#361574}
Patch Set 1 #Patch Set 2 : gn fix #
Total comments: 7
Patch Set 3 : Address comments. #
Total comments: 53
Patch Set 4 : Add remaining tests. #Patch Set 5 : template fix. #Patch Set 6 : fix for reals. #
Total comments: 4
Patch Set 7 : Address comments. #
Total comments: 8
Patch Set 8 : Address comments. #Patch Set 9 : Rebase. #Patch Set 10 : Update PropertyTree::operator== signature #
Total comments: 7
Patch Set 11 : Rebase and address comments. #
Messages
Total messages: 35 (9 generated)
Description was changed from ========== Added serialization to protobufs for property trees. BUG=554319 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Added serialization to protobufs for property trees. BUG=554319 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
khushalsagar@chromium.org changed reviewers: + dtrainor@chromium.org, nyquist@chromium.org
TODO: Add tests.
https://chromiumcodereview.appspot.com/1417963011/diff/20001/cc/proto/propert... File cc/proto/property_tree.proto (right): https://chromiumcodereview.appspot.com/1417963011/diff/20001/cc/proto/propert... cc/proto/property_tree.proto:129: message PropertyTree { Would it make more sense for this to look something like: message PropertyTree { enum PropertyType { Transform = 1; Clip = 2; Effect = 3; } optional PropertyType type = 1; repeated cc.proto.TreeNode nodes = 2; optional bool needs_update = 3; // Set based on the PropertyType optional cc.proto.TransformTreeProperties = 1000; } Then for TreeNode, you could skip the type and assume it's pulled from the PropertyTree and just pull the correct node_data. wdyt? https://chromiumcodereview.appspot.com/1417963011/diff/20001/cc/trees/propert... File cc/trees/property_tree.cc (right): https://chromiumcodereview.appspot.com/1417963011/diff/20001/cc/trees/propert... cc/trees/property_tree.cc:912: PropertyTree::ToProtobuf(proto->mutable_property_tree()); I can't find this. Not added yet?
https://chromiumcodereview.appspot.com/1417963011/diff/20001/cc/proto/propert... File cc/proto/property_tree.proto (right): https://chromiumcodereview.appspot.com/1417963011/diff/20001/cc/proto/propert... cc/proto/property_tree.proto:129: message PropertyTree { On 2015/11/11 16:43:16, David Trainor wrote: > Would it make more sense for this to look something like: > > message PropertyTree { > enum PropertyType { > Transform = 1; > Clip = 2; > Effect = 3; > } > > optional PropertyType type = 1; > repeated cc.proto.TreeNode nodes = 2; > optional bool needs_update = 3; > > // Set based on the PropertyType > optional cc.proto.TransformTreeProperties = 1000; > } > > Then for TreeNode, you could skip the type and assume it's pulled from the > PropertyTree and just pull the correct node_data. > > wdyt? > I was trying to structure the property tree protos, the same way the classes are structured. So someone modifying a class knows exactly which proto they need to modify. Actually for tree nodes, we can skip the type right now as well. Added a comment where it is being used. https://chromiumcodereview.appspot.com/1417963011/diff/20001/cc/trees/propert... File cc/trees/property_tree.cc (right): https://chromiumcodereview.appspot.com/1417963011/diff/20001/cc/trees/propert... cc/trees/property_tree.cc:104: void PropertyTree<T>::ToProtobuf(proto::PropertyTree* proto) const { Serialization for the base PropertyTree. https://chromiumcodereview.appspot.com/1417963011/diff/20001/cc/trees/propert... cc/trees/property_tree.cc:181: DCHECK(!proto->has_property_type()); We don't really need the property type here. I added it for the DCHECK. But may be that's an overkill? We can probably just check that the proto has only the expected data field set here. https://chromiumcodereview.appspot.com/1417963011/diff/20001/cc/trees/propert... cc/trees/property_tree.cc:912: PropertyTree::ToProtobuf(proto->mutable_property_tree()); On 2015/11/11 16:43:16, David Trainor wrote: > I can't find this. Not added yet? Added a comment for where it is.
https://chromiumcodereview.appspot.com/1417963011/diff/20001/cc/proto/propert... File cc/proto/property_tree.proto (right): https://chromiumcodereview.appspot.com/1417963011/diff/20001/cc/proto/propert... cc/proto/property_tree.proto:129: message PropertyTree { On 2015/11/11 19:14:13, Khushal wrote: > On 2015/11/11 16:43:16, David Trainor wrote: > > Would it make more sense for this to look something like: > > > > message PropertyTree { > > enum PropertyType { > > Transform = 1; > > Clip = 2; > > Effect = 3; > > } > > > > optional PropertyType type = 1; > > repeated cc.proto.TreeNode nodes = 2; > > optional bool needs_update = 3; > > > > // Set based on the PropertyType > > optional cc.proto.TransformTreeProperties = 1000; > > } > > > > Then for TreeNode, you could skip the type and assume it's pulled from the > > PropertyTree and just pull the correct node_data. > > > > wdyt? > > > > I was trying to structure the property tree protos, the same way the classes are > structured. So someone modifying a class knows exactly which proto they need to > modify. > > Actually for tree nodes, we can skip the type right now as well. Added a comment > where it is being used. I re-structured the proto to have the type in the PropertyTree itself. This keeps it consistent with the setup for Layer protos. https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... File cc/trees/property_tree_unittest.cc (right): https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... cc/trees/property_tree_unittest.cc:52: class PropertyTreeTest : public testing::Test { Would it be a good idea to use something like this to run the same property tree tests after serializing an object. This validates that the serialization doesn't change the expected behaviour of the class.
https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/proto/propert... File cc/proto/property_tree.proto (right): https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/proto/propert... cc/proto/property_tree.proto:16: message TranformNodeData { Put comments above each of these types pointing to the class and file they are related to. https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/proto/propert... cc/proto/property_tree.proto:17: optional cc.proto.Transform pre_local = 1; I know I added this, but maybe remove cc.proto if it's not needed? I forget what the three of us discussed yesterday. https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/proto/propert... cc/proto/property_tree.proto:103: optional TransformTreeData transform_tree_data = 100; 1000 for these kind of subclass types? at least that seems to be what we're doing. https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/proto/propert... cc/proto/property_tree.proto:107: optional bool source_to_parent_updates_allowed = 100; start at 1 https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/proto/propert... cc/proto/property_tree.proto:124: optional bool needs_rebuild = 100; just make this 4. it's just a continuation of base properties. https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... File cc/trees/property_tree.cc (right): https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... cc/trees/property_tree.cc:21: void CheckIsValidTreeNodeProto( Can we just DCHECK(proto.has_***_node_data()) instead of calling this? https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... cc/trees/property_tree.cc:105: for (size_t i = 0; i < nodes_.size(); i++) for (const auto& node : nodes_) ... Maybe remove const if we start having this be a bit smarter about sending deltas in the future :). https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... cc/trees/property_tree.cc:123: nodes_[i].FromProtobuf(proto.nodes(i)); Maybe just nodes_.back().FromProtobuf(proto.nodes(i))? https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... cc/trees/property_tree.cc:246: proto::TranformNodeData data = proto.transform_node_data(); const proto::TransformNodeData& data = ...;? Same for others? https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... cc/trees/property_tree.cc:321: transform_id == other.transform_id && target_id == other.target_id && one per line looks a lot cleaner imo. does the presubmit styleguide prefer this? https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... cc/trees/property_tree.cc:358: clip = ProtoToRectF(data.clip()); const & https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... cc/trees/property_tree.cc:382: bool EffectNodeData::operator==(const EffectNodeData& other) const { Need one for TransformNodeData? https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... cc/trees/property_tree.cc:386: transform_id == other.transform_id && clip_id == other.clip_id; new line imo https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... cc/trees/property_tree.cc:402: proto::EffectNodeData data = proto.effect_node_data(); const & https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... cc/trees/property_tree.cc:933: for (int i : nodes_affected_by_inner_viewport_bounds_delta_) would it be cleaner to use auto here and below? https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... cc/trees/property_tree.cc:945: proto::TransformTreeData data = proto.transform_tree_data(); const & https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... cc/trees/property_tree.cc:1042: // TODO(khushalsagar): Add support for sending diffs when serializaing Add a bug number. https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... cc/trees/property_tree.cc:1047: proto->set_needs_rebuild(needs_rebuild); Do we actually need this? I guess it's fine to send. Maybe we need it for when we move PropertyTrees between LayerImpl trees? https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... cc/trees/property_tree.cc:1050: // TODO(khushalsagar): Consider using the sequence number to decide if Use same bug number as above imo :) https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... File cc/trees/property_tree.h (right): https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... cc/trees/property_tree.h:458: static PropertyTrees CreateFromProtobuf(const proto::PropertyTrees& proto); do we want to just return a PropertyTrees object here? What's the overhead of doing this? I don't know enough about whether or not the compiler can be smart in these cases. https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... File cc/trees/property_tree_unittest.cc (right): https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... cc/trees/property_tree_unittest.cc:12: namespace cc { Maybe anonymous namespace in here for everything? https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... cc/trees/property_tree_unittest.cc:14: TEST(PropertyTreeSerializationTest, ClipNodeDataSerialization) { Add transform one. https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... cc/trees/property_tree_unittest.cc:79: #define DIRECT_PROPERTY_TREE_TEST_F(TEST_FIXTURE_NAME) \ Good idea IMO :). Maybe #undef these after the test code just to be clean? https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... cc/trees/property_tree_unittest.cc:80: TEST_F(TEST_FIXTURE_NAME, RunDirect) { RunTest(false); } Always use PropertyTreeTest as the fixture name. Then build the test name with something like TEST_FIXTURE_NAME##Direct or ##Serialized. I forget the macro format though. https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... cc/trees/property_tree_unittest.cc:122: PropertyTreeTestComputeTransformRoot); Can we just call this ComputeTransformRoot? That might look a little cleaner.
https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... File cc/trees/property_tree_unittest.cc (right): https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... cc/trees/property_tree_unittest.cc:80: TEST_F(TEST_FIXTURE_NAME, RunDirect) { RunTest(false); } On 2015/11/12 22:46:11, David Trainor wrote: > Always use PropertyTreeTest as the fixture name. Then build the test name with > something like TEST_FIXTURE_NAME##Direct or ##Serialized. I forget the macro > format though. Just a note from our offline discussion, it looks like this follows the layer_tree_test existing macro format. Carry on! https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... cc/trees/property_tree_unittest.cc:122: PropertyTreeTestComputeTransformRoot); On 2015/11/12 22:46:11, David Trainor wrote: > Can we just call this ComputeTransformRoot? That might look a little cleaner. Same as above, ignore.
https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/proto/propert... File cc/proto/property_tree.proto (right): https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/proto/propert... cc/proto/property_tree.proto:16: message TranformNodeData { On 2015/11/12 22:46:10, David Trainor wrote: > Put comments above each of these types pointing to the class and file they are > related to. Done. https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/proto/propert... cc/proto/property_tree.proto:17: optional cc.proto.Transform pre_local = 1; On 2015/11/12 22:46:10, David Trainor wrote: > I know I added this, but maybe remove cc.proto if it's not needed? I forget > what the three of us discussed yesterday. We decided to remove them since they are not needed. Done. https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/proto/propert... cc/proto/property_tree.proto:103: optional TransformTreeData transform_tree_data = 100; On 2015/11/12 22:46:10, David Trainor wrote: > 1000 for these kind of subclass types? at least that seems to be what we're > doing. Done. https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/proto/propert... cc/proto/property_tree.proto:107: optional bool source_to_parent_updates_allowed = 100; On 2015/11/12 22:46:10, David Trainor wrote: > start at 1 Done. https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/proto/propert... cc/proto/property_tree.proto:124: optional bool needs_rebuild = 100; On 2015/11/12 22:46:10, David Trainor wrote: > just make this 4. it's just a continuation of base properties. Done. https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... File cc/trees/property_tree.cc (right): https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... cc/trees/property_tree.cc:21: void CheckIsValidTreeNodeProto( On 2015/11/12 22:46:11, David Trainor wrote: > Can we just DCHECK(proto.has_***_node_data()) instead of calling this? The function is to make sure that the proto has only one of the *NodeData set. I guess that's probably unnecesssary. Removed it. https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... cc/trees/property_tree.cc:105: for (size_t i = 0; i < nodes_.size(); i++) On 2015/11/12 22:46:11, David Trainor wrote: > for (const auto& node : nodes_) > ... > > Maybe remove const if we start having this be a bit smarter about sending deltas > in the future :). Done. Let's let the const stay for now. That's a TODO to get to. :) https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... cc/trees/property_tree.cc:123: nodes_[i].FromProtobuf(proto.nodes(i)); On 2015/11/12 22:46:10, David Trainor wrote: > Maybe just nodes_.back().FromProtobuf(proto.nodes(i))? Done. https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... cc/trees/property_tree.cc:246: proto::TranformNodeData data = proto.transform_node_data(); On 2015/11/12 22:46:11, David Trainor wrote: > const proto::TransformNodeData& data = ...;? Same for others? Done. https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... cc/trees/property_tree.cc:321: transform_id == other.transform_id && target_id == other.target_id && On 2015/11/12 22:46:11, David Trainor wrote: > one per line looks a lot cleaner imo. does the presubmit styleguide prefer > this? The presubmit asks me to run git cl format for cc. It prefers it this way. :/ https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... cc/trees/property_tree.cc:358: clip = ProtoToRectF(data.clip()); On 2015/11/12 22:46:11, David Trainor wrote: > const & Done. https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... cc/trees/property_tree.cc:382: bool EffectNodeData::operator==(const EffectNodeData& other) const { On 2015/11/12 22:46:10, David Trainor wrote: > Need one for TransformNodeData? Done. https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... cc/trees/property_tree.cc:386: transform_id == other.transform_id && clip_id == other.clip_id; On 2015/11/12 22:46:10, David Trainor wrote: > new line imo presubmit complains. :( https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... cc/trees/property_tree.cc:402: proto::EffectNodeData data = proto.effect_node_data(); On 2015/11/12 22:46:11, David Trainor wrote: > const & Done. https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... cc/trees/property_tree.cc:933: for (int i : nodes_affected_by_inner_viewport_bounds_delta_) On 2015/11/12 22:46:10, David Trainor wrote: > would it be cleaner to use auto here and below? Done. https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... cc/trees/property_tree.cc:945: proto::TransformTreeData data = proto.transform_tree_data(); On 2015/11/12 22:46:11, David Trainor wrote: > const & Done. https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... cc/trees/property_tree.cc:1042: // TODO(khushalsagar): Add support for sending diffs when serializaing On 2015/11/12 22:46:11, David Trainor wrote: > Add a bug number. Done. https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... cc/trees/property_tree.cc:1047: proto->set_needs_rebuild(needs_rebuild); On 2015/11/12 22:46:11, David Trainor wrote: > Do we actually need this? I guess it's fine to send. Maybe we need it for when > we move PropertyTrees between LayerImpl trees? Not really. Once the tree has been re-built when updating layers, this flag is reset, so when we send the property trees during a commit, this would probably always be false. The impl side changes it on its copy of the tree and then re-builds it when the pending and active tree are being synchronized. Though I'm not sure if this is being done right now. I added it because I wasn't sure if we should do the direct serialization right now or consider what happens during the commit flow. May be when we optimize this later, this will be sent for the first commit and for each subsequent one, we'll see that the field hasn't changed so we'll never re-send it. I'm not sure what would be better. Wdyt? https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... cc/trees/property_tree.cc:1050: // TODO(khushalsagar): Consider using the sequence number to decide if On 2015/11/12 22:46:10, David Trainor wrote: > Use same bug number as above imo :) Done. https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... File cc/trees/property_tree.h (right): https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... cc/trees/property_tree.h:458: static PropertyTrees CreateFromProtobuf(const proto::PropertyTrees& proto); On 2015/11/12 22:46:11, David Trainor wrote: > do we want to just return a PropertyTrees object here? What's the overhead of > doing this? I don't know enough about whether or not the compiler can be smart > in these cases. Yeah, just returning property trees is bad. I added it as a method to property trees. This way when we de-serialize the layer tree host for a commit, we can just de-serialize into the property trees in the LTH. https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... File cc/trees/property_tree_unittest.cc (right): https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... cc/trees/property_tree_unittest.cc:12: namespace cc { On 2015/11/12 22:46:11, David Trainor wrote: > Maybe anonymous namespace in here for everything? Done. https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... cc/trees/property_tree_unittest.cc:14: TEST(PropertyTreeSerializationTest, ClipNodeDataSerialization) { On 2015/11/12 22:46:11, David Trainor wrote: > Add transform one. Done. https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... cc/trees/property_tree_unittest.cc:79: #define DIRECT_PROPERTY_TREE_TEST_F(TEST_FIXTURE_NAME) \ On 2015/11/12 22:46:11, David Trainor wrote: > Good idea IMO :). > > Maybe #undef these after the test code just to be clean? Done. https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... cc/trees/property_tree_unittest.cc:80: TEST_F(TEST_FIXTURE_NAME, RunDirect) { RunTest(false); } On 2015/11/12 22:49:33, David Trainor wrote: > On 2015/11/12 22:46:11, David Trainor wrote: > > Always use PropertyTreeTest as the fixture name. Then build the test name > with > > something like TEST_FIXTURE_NAME##Direct or ##Serialized. I forget the macro > > format though. > > Just a note from our offline discussion, it looks like this follows the > layer_tree_test existing macro format. Carry on! Done.
general structure lgtm. https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... File cc/trees/property_tree.cc (right): https://chromiumcodereview.appspot.com/1417963011/diff/40001/cc/trees/propert... cc/trees/property_tree.cc:321: transform_id == other.transform_id && target_id == other.target_id && On 2015/11/13 10:33:13, Khushal wrote: > On 2015/11/12 22:46:11, David Trainor wrote: > > one per line looks a lot cleaner imo. does the presubmit styleguide prefer > > this? > > The presubmit asks me to run git cl format for cc. It prefers it this way. :/ K no worries then!
Description was changed from ========== Added serialization to protobufs for property trees. BUG=554319 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Add serialization to protobufs for property trees. Add serialization tests. Update existing tests to validate serialization. BUG=554319 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
khushalsagar@chromium.org changed reviewers: + vmpstr@chromium.org
lgtm https://codereview.chromium.org/1417963011/diff/100001/cc/proto/property_tree... File cc/proto/property_tree.proto (right): https://codereview.chromium.org/1417963011/diff/100001/cc/proto/property_tree... cc/proto/property_tree.proto:17: // cc/trees/property_tree.h Optional nit: How about //cc/trees/property_tree.h ? https://codereview.chromium.org/1417963011/diff/100001/cc/trees/property_tree.cc File cc/trees/property_tree.cc (right): https://codereview.chromium.org/1417963011/diff/100001/cc/trees/property_tree... cc/trees/property_tree.cc:1009: i++) { Nit: ++i across this change
khushalsagar@chromium.org changed reviewers: + vollick@chromium.org
https://codereview.chromium.org/1417963011/diff/100001/cc/proto/property_tree... File cc/proto/property_tree.proto (right): https://codereview.chromium.org/1417963011/diff/100001/cc/proto/property_tree... cc/proto/property_tree.proto:17: // cc/trees/property_tree.h On 2015/11/16 19:48:56, nyquist wrote: > Optional nit: How about //cc/trees/property_tree.h ? It kinda looks cleaner like this. :) https://codereview.chromium.org/1417963011/diff/100001/cc/trees/property_tree.cc File cc/trees/property_tree.cc (right): https://codereview.chromium.org/1417963011/diff/100001/cc/trees/property_tree... cc/trees/property_tree.cc:1009: i++) { On 2015/11/16 19:48:56, nyquist wrote: > Nit: ++i across this change Done.
vollick@, just a note from our discussion earlier. We want to add presubmit checks to make sure that the property_tree.h and proto file is edited at the same time. For now we could add this as a warning, since any changes to this file which don't change the data members of the classes shouldn't need to update the proto file. Would you like me to add the pre-submit change as a part of this patch?
https://codereview.chromium.org/1417963011/diff/120001/cc/proto/property_tree... File cc/proto/property_tree.proto (right): https://codereview.chromium.org/1417963011/diff/120001/cc/proto/property_tree... cc/proto/property_tree.proto:134: [packed = true]; What's packed? https://codereview.chromium.org/1417963011/diff/120001/cc/trees/property_tree.cc File cc/trees/property_tree.cc (right): https://codereview.chromium.org/1417963011/diff/120001/cc/trees/property_tree... cc/trees/property_tree.cc:151: return pre_local == other.pre_local && local == other.local && O_O I almost wonder if this approach might be more readable/maintainable if (a != b) return false; if (c != d) return false; But then it will of course be much longer. I don't know. https://codereview.chromium.org/1417963011/diff/120001/cc/trees/property_tree.h File cc/trees/property_tree.h (right): https://codereview.chromium.org/1417963011/diff/120001/cc/trees/property_tree... cc/trees/property_tree.h:28: // Each class declared here has a corresponding proto defined in Can you add // IMPORTANT! https://codereview.chromium.org/1417963011/diff/120001/cc/trees/property_tree... cc/trees/property_tree.h:42: bool operator==(const TreeNode& other) const; I'd prefer const TreeNode<T>& other (ie explicit <T>)
On 2015/11/19 23:24:51, vmpstr wrote: > https://codereview.chromium.org/1417963011/diff/120001/cc/proto/property_tree... > File cc/proto/property_tree.proto (right): > > https://codereview.chromium.org/1417963011/diff/120001/cc/proto/property_tree... > cc/proto/property_tree.proto:134: [packed = true]; > What's packed? > > https://codereview.chromium.org/1417963011/diff/120001/cc/trees/property_tree.cc > File cc/trees/property_tree.cc (right): > > https://codereview.chromium.org/1417963011/diff/120001/cc/trees/property_tree... > cc/trees/property_tree.cc:151: return pre_local == other.pre_local && local == > other.local && > O_O > > I almost wonder if this approach might be more readable/maintainable > > if (a != b) > return false; > > if (c != d) > return false; > > But then it will of course be much longer. I don't know. > > https://codereview.chromium.org/1417963011/diff/120001/cc/trees/property_tree.h > File cc/trees/property_tree.h (right): > > https://codereview.chromium.org/1417963011/diff/120001/cc/trees/property_tree... > cc/trees/property_tree.h:28: // Each class declared here has a corresponding > proto defined in > Can you add > > // IMPORTANT! > > https://codereview.chromium.org/1417963011/diff/120001/cc/trees/property_tree... > cc/trees/property_tree.h:42: bool operator==(const TreeNode& other) const; > I'd prefer const TreeNode<T>& other (ie explicit <T>) Chatted a bit with vmpstr about this offline and have a hopefully simple, low-road solution. Rather than a complex presubmit script that has to parse the headers to see if a data member has changed, we could demarcate the beginning and end of the data in a header. Perhaps something like // Begin data <some boring text about why this line can't be changed> // End data <same> If the presubmit notices a change between these demarcations but no corresponding .proto change, it could bark. wdyt?
On 2015/11/20 01:20:42, vollick wrote: > Chatted a bit with vmpstr about this offline and have a hopefully simple, > low-road solution. > > Rather than a complex presubmit script that has to parse the headers to see if a > data member has changed, we could demarcate the beginning and end of the data in > a header. Perhaps something like > > // Begin data <some boring text about why this line can't be changed> > // End data <same> > > If the presubmit notices a change between these demarcations but no > corresponding .proto change, it could bark. > > wdyt? This sounds like a reasonable and maintainable idea. I'd vote for doing this as a follow up CL though and just keep an extra eye on the serialization code until the script has been submitted.
This sounds good to me. But I'd like it if we could do it in a follow-up patch too. https://codereview.chromium.org/1417963011/diff/120001/cc/proto/property_tree... File cc/proto/property_tree.proto (right): https://codereview.chromium.org/1417963011/diff/120001/cc/proto/property_tree... cc/proto/property_tree.proto:134: [packed = true]; On 2015/11/19 23:24:51, vmpstr wrote: > What's packed? It's a data efficient way to write the repeated fields. https://developers.google.com/protocol-buffers/docs/encoding?hl=en#packed-rep... https://codereview.chromium.org/1417963011/diff/120001/cc/trees/property_tree.cc File cc/trees/property_tree.cc (right): https://codereview.chromium.org/1417963011/diff/120001/cc/trees/property_tree... cc/trees/property_tree.cc:151: return pre_local == other.pre_local && local == other.local && On 2015/11/19 23:24:51, vmpstr wrote: > O_O > > I almost wonder if this approach might be more readable/maintainable > > if (a != b) > return false; > > if (c != d) > return false; > > But then it will of course be much longer. I don't know. I saw that's how we do this for other classes in cc so kept it consistent. With the one above it would be suuuper long. https://codereview.chromium.org/1417963011/diff/120001/cc/trees/property_tree.h File cc/trees/property_tree.h (right): https://codereview.chromium.org/1417963011/diff/120001/cc/trees/property_tree... cc/trees/property_tree.h:28: // Each class declared here has a corresponding proto defined in On 2015/11/19 23:24:51, vmpstr wrote: > Can you add > > // IMPORTANT! Done. :D https://codereview.chromium.org/1417963011/diff/120001/cc/trees/property_tree... cc/trees/property_tree.h:42: bool operator==(const TreeNode& other) const; On 2015/11/19 23:24:51, vmpstr wrote: > I'd prefer const TreeNode<T>& other (ie explicit <T>) Done.
On 2015/11/20 01:23:45, nyquist wrote: > On 2015/11/20 01:20:42, vollick wrote: > > Chatted a bit with vmpstr about this offline and have a hopefully simple, > > low-road solution. > > > > Rather than a complex presubmit script that has to parse the headers to see if > a > > data member has changed, we could demarcate the beginning and end of the data > in > > a header. Perhaps something like > > > > // Begin data <some boring text about why this line can't be changed> > > // End data <same> > > > > If the presubmit notices a change between these demarcations but no > > corresponding .proto change, it could bark. > > > > wdyt? > > This sounds like a reasonable and maintainable idea. > I'd vote for doing this as a follow up CL though and just keep an extra eye on > the serialization code until the script has been submitted. I'm totally fine with that being a follow up patch. This lgtm % vmpstr.
On 2015/11/20 02:03:52, vollick wrote: > On 2015/11/20 01:23:45, nyquist wrote: > > On 2015/11/20 01:20:42, vollick wrote: > > > Chatted a bit with vmpstr about this offline and have a hopefully simple, > > > low-road solution. > > > > > > Rather than a complex presubmit script that has to parse the headers to see > if > > a > > > data member has changed, we could demarcate the beginning and end of the > data > > in > > > a header. Perhaps something like > > > > > > // Begin data <some boring text about why this line can't be changed> > > > // End data <same> > > > > > > If the presubmit notices a change between these demarcations but no > > > corresponding .proto change, it could bark. > > > > > > wdyt? > > > > This sounds like a reasonable and maintainable idea. > > I'd vote for doing this as a follow up CL though and just keep an extra eye on > > the serialization code until the script has been submitted. > > I'm totally fine with that being a follow up patch. > > This lgtm % vmpstr. Thanks Ian. I added a bug for it so we can track which other files need that check too.
+vmpstr, friendly ping. :)
lgtm with a few nits https://codereview.chromium.org/1417963011/diff/180001/cc/proto/gfx_conversio... File cc/proto/gfx_conversions_unittest.cc (right): https://codereview.chromium.org/1417963011/diff/180001/cc/proto/gfx_conversio... cc/proto/gfx_conversions_unittest.cc:181: const gfx::Vector2dF vector(5.1f, 10.2f); Can you add a few more random things? Maybe some negatives too https://codereview.chromium.org/1417963011/diff/180001/cc/proto/gfx_conversio... cc/proto/gfx_conversions_unittest.cc:194: const gfx::ScrollOffset scroll_offset(5.1f, 10.2f); Same here. https://codereview.chromium.org/1417963011/diff/180001/cc/trees/property_tree.cc File cc/trees/property_tree.cc (right): https://codereview.chromium.org/1417963011/diff/180001/cc/trees/property_tree... cc/trees/property_tree.cc:1059: return PropertyTree::operator==(other); Do you need this? If you don't define ClipTree::operator== would it do exactly this?
Thanks Vlad! https://codereview.chromium.org/1417963011/diff/180001/cc/proto/gfx_conversio... File cc/proto/gfx_conversions_unittest.cc (right): https://codereview.chromium.org/1417963011/diff/180001/cc/proto/gfx_conversio... cc/proto/gfx_conversions_unittest.cc:181: const gfx::Vector2dF vector(5.1f, 10.2f); On 2015/11/24 19:32:33, vmpstr wrote: > Can you add a few more random things? Maybe some negatives too Done. https://codereview.chromium.org/1417963011/diff/180001/cc/proto/gfx_conversio... cc/proto/gfx_conversions_unittest.cc:194: const gfx::ScrollOffset scroll_offset(5.1f, 10.2f); On 2015/11/24 19:32:33, vmpstr wrote: > Same here. Done. https://codereview.chromium.org/1417963011/diff/180001/cc/trees/property_tree.cc File cc/trees/property_tree.cc (right): https://codereview.chromium.org/1417963011/diff/180001/cc/trees/property_tree... cc/trees/property_tree.cc:1059: return PropertyTree::operator==(other); On 2015/11/24 19:32:33, vmpstr wrote: > Do you need this? If you don't define ClipTree::operator== would it do exactly > this? We don't need it right now, but I figured its safer to add it so if any new data members are added to EffectTree or ClipTree, it would be easier to remember to update these methods.
The CQ bit was checked by khushalsagar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtrainor@chromium.org, nyquist@chromium.org, vollick@chromium.org, vmpstr@chromium.org Link to the patchset: https://codereview.chromium.org/1417963011/#ps200001 (title: "Rebase and address comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1417963011/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1417963011/200001
https://codereview.chromium.org/1417963011/diff/180001/cc/trees/property_tree.cc File cc/trees/property_tree.cc (right): https://codereview.chromium.org/1417963011/diff/180001/cc/trees/property_tree... cc/trees/property_tree.cc:1059: return PropertyTree::operator==(other); On 2015/11/24 22:22:40, Khushal wrote: > On 2015/11/24 19:32:33, vmpstr wrote: > > Do you need this? If you don't define ClipTree::operator== would it do exactly > > this? > > We don't need it right now, but I figured its safer to add it so if any new data > members are added to EffectTree or ClipTree, it would be easier to remember to > update these methods. Makes sense, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by khushalsagar@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1417963011/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1417963011/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/54f287ace5c9092a2ed3a8884d13d0087a83eaf0 Cr-Commit-Position: refs/heads/master@{#361574} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
