Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2240043004:
Tracing V2: Fully-functional plugin. (Closed)
DescriptionTracing V2: Fully-functional plugin.
Added wrapper namespace for unit tests to avoid name collisions
with official protobuf. Tiny varint support in ProtoZeroMessage.
Added tests to check conformance with the official protobuf.
BUG=608721
Committed: https://crrev.com/e4f1cbdbcbac1130f412990beffef70ad96609ef
Cr-Commit-Position: refs/heads/master@{#414413}
Patch Set 1 #Patch Set 2 : tests #Patch Set 3 : nit #Patch Set 4 : fix use after free in tests #
Total comments: 6
Patch Set 5 : nit #
Total comments: 4
Patch Set 6 : simplify #Patch Set 7 : nit #
Total comments: 4
Patch Set 8 : style #Patch Set 9 : rebase #
Messages
Total messages: 51 (42 generated)
The CQ bit was checked by kraynov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by kraynov@chromium.org
The CQ bit was checked by kraynov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Tracing V2: Fully-functional plugin. Added wrapper namespace for unit tests to avoid name collisons with official protobuf. Tiny varint support in ProtoZeroMessage. All the stubs are uncommented. BUG=608721 ========== to ========== Tracing V2: Fully-functional plugin. Added wrapper namespace for unit tests to avoid name collisions with official protobuf. Tiny varint support in ProtoZeroMessage. Added tests to check conformance with the official protobuf. If message handles are used, non-repeated field can't be appended twice. BUG=608721 ==========
kraynov@chromium.org changed reviewers: + alph@chromium.org, primiano@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
PTAL
The CQ bit was checked by kraynov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by kraynov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2240043004/diff/60001/components/tracing/core... File components/tracing/core/proto_zero_message.h (right): https://codereview.chromium.org/2240043004/diff/60001/components/tracing/core... components/tracing/core/proto_zero_message.h:100: *pos = static_cast<uint8_t>(value); nit: *pos++ = https://codereview.chromium.org/2240043004/diff/60001/components/tracing/core... File components/tracing/core/proto_zero_message_handle.h (right): https://codereview.chromium.org/2240043004/diff/60001/components/tracing/core... components/tracing/core/proto_zero_message_handle.h:62: std::set<uint32_t> sealed_fields_; nit: unordered_set https://codereview.chromium.org/2240043004/diff/60001/components/tracing/test... File components/tracing/test/fake_scattered_buffer.cc (right): https://codereview.chromium.org/2240043004/diff/60001/components/tracing/test... components/tracing/test/fake_scattered_buffer.cc:37: if (chunk_index >= chunks_.size()) { nit: looks redundant provided the ASSERT above.
The CQ bit was checked by kraynov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
fixed https://codereview.chromium.org/2240043004/diff/60001/components/tracing/core... File components/tracing/core/proto_zero_message.h (right): https://codereview.chromium.org/2240043004/diff/60001/components/tracing/core... components/tracing/core/proto_zero_message.h:100: *pos = static_cast<uint8_t>(value); On 2016/08/22 22:48:35, alph wrote: > nit: *pos++ = Done. https://codereview.chromium.org/2240043004/diff/60001/components/tracing/core... File components/tracing/core/proto_zero_message_handle.h (right): https://codereview.chromium.org/2240043004/diff/60001/components/tracing/core... components/tracing/core/proto_zero_message_handle.h:62: std::set<uint32_t> sealed_fields_; On 2016/08/22 22:48:35, alph wrote: > nit: unordered_set Done. https://codereview.chromium.org/2240043004/diff/60001/components/tracing/test... File components/tracing/test/fake_scattered_buffer.cc (right): https://codereview.chromium.org/2240043004/diff/60001/components/tracing/test... components/tracing/test/fake_scattered_buffer.cc:37: if (chunk_index >= chunks_.size()) { On 2016/08/22 22:48:35, alph wrote: > nit: looks redundant provided the ASSERT above. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Please remove the sealed field logic. The benefit is IMHO minimal but the implementation is a hack. https://codereview.chromium.org/2240043004/diff/80001/components/tracing/core... File components/tracing/core/proto_zero_message.h (right): https://codereview.chromium.org/2240043004/diff/80001/components/tracing/core... components/tracing/core/proto_zero_message.h:26: #if DCHECK_IS_ON() is this causing an actual compilation error? If not please remove this #if and always fwd declare. https://codereview.chromium.org/2240043004/diff/80001/components/tracing/core... components/tracing/core/proto_zero_message.h:90: // It could be looking weird not to optimize writing field_id, but it's Honestly I feel this comment is just too apologetic. Just remove it, caused me more time to figure out what you really meant ;-) https://codereview.chromium.org/2240043004/diff/80001/components/tracing/core... components/tracing/core/proto_zero_message.h:142: // Prevent multiple appends for non-repeated fields. honestly I feel this duplicated field detection is a bit too much complexity for the benefit given https://codereview.chromium.org/2240043004/diff/80001/components/tracing/core... File components/tracing/core/proto_zero_message_handle.h (right): https://codereview.chromium.org/2240043004/diff/80001/components/tracing/core... components/tracing/core/proto_zero_message_handle.h:41: inline void SealField(uint32_t field_id) { wait why sealfield is a property of the Handle? The handle is supposed to be just a transparent adaptor. Please don't add anything to that. THe idea is that if the handle become a bottleneck we can just short-circuit to ProtoZeroMessage. Also, really, the fact that the sealed fields is here feels a hack to me. Also note how this handle has move constructors.
The CQ bit was checked by kraynov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with some final comments, please address them before landing. https://codereview.chromium.org/2240043004/diff/120001/components/tracing/tes... File components/tracing/test/fake_scattered_buffer.cc (right): https://codereview.chromium.org/2240043004/diff/120001/components/tracing/tes... components/tracing/test/fake_scattered_buffer.cc:43: EXPECT_LE(start + length, chunks_.size() * chunk_size_); at this point can you rewrite this in terms of GetBytes + hex serialization? There feels to be quite some duplication. Fine to remove the <OUT OF BOUND> if you have the assert_le. https://codereview.chromium.org/2240043004/diff/120001/components/tracing/tes... File components/tracing/test/proto_zero_generation_unittest.cc (right): https://codereview.chromium.org/2240043004/diff/120001/components/tracing/tes... components/tracing/test/proto_zero_generation_unittest.cc:19: using namespace pbzero::foo::bar; probably a bit more readable if you wrap this into another namespace, e.g. pbtest = pbzero::foo:bar pbgold = foo:bar https://codereview.chromium.org/2240043004/diff/120001/components/tracing/tes... components/tracing/test/proto_zero_generation_unittest.cc:46: void CreateMessage(T** message_ptr) { This signature is a bit odd. Why don't you return a T* here, instead of getting a ptr-to-ptr argument? https://codereview.chromium.org/2240043004/diff/120001/components/tracing/tes... components/tracing/test/proto_zero_generation_unittest.cc:100: pbgold::EveryField gold; maybe s/gold/gold_msg/ to make it clear this is an "official" message?
The CQ bit was checked by kraynov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
excellent, thanks a lot, LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by kraynov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by kraynov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Tracing V2: Fully-functional plugin. Added wrapper namespace for unit tests to avoid name collisions with official protobuf. Tiny varint support in ProtoZeroMessage. Added tests to check conformance with the official protobuf. If message handles are used, non-repeated field can't be appended twice. BUG=608721 ========== to ========== Tracing V2: Fully-functional plugin. Added wrapper namespace for unit tests to avoid name collisions with official protobuf. Tiny varint support in ProtoZeroMessage. Added tests to check conformance with the official protobuf. BUG=608721 ==========
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 kraynov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alph@chromium.org, primiano@chromium.org Link to the patchset: https://codereview.chromium.org/2240043004/#ps160001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Tracing V2: Fully-functional plugin. Added wrapper namespace for unit tests to avoid name collisions with official protobuf. Tiny varint support in ProtoZeroMessage. Added tests to check conformance with the official protobuf. BUG=608721 ========== to ========== Tracing V2: Fully-functional plugin. Added wrapper namespace for unit tests to avoid name collisions with official protobuf. Tiny varint support in ProtoZeroMessage. Added tests to check conformance with the official protobuf. BUG=608721 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Tracing V2: Fully-functional plugin. Added wrapper namespace for unit tests to avoid name collisions with official protobuf. Tiny varint support in ProtoZeroMessage. Added tests to check conformance with the official protobuf. BUG=608721 ========== to ========== Tracing V2: Fully-functional plugin. Added wrapper namespace for unit tests to avoid name collisions with official protobuf. Tiny varint support in ProtoZeroMessage. Added tests to check conformance with the official protobuf. BUG=608721 Committed: https://crrev.com/e4f1cbdbcbac1130f412990beffef70ad96609ef Cr-Commit-Position: refs/heads/master@{#414413} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/e4f1cbdbcbac1130f412990beffef70ad96609ef Cr-Commit-Position: refs/heads/master@{#414413} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
