Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2096103003:
Move weave packet to common location b/t generator and receiver (Closed)
DescriptionMove weave packet to common location b/t generator and receiver
BUG=617238
Committed: https://crrev.com/714da71c4b3b8a763ed61b00ee7f8efa83cc0347
Cr-Commit-Position: refs/heads/master@{#402355}
Patch Set 1 #Patch Set 2 : added files back #
Total comments: 16
Patch Set 3 : moved common enums to a namespace #
Total comments: 5
Patch Set 4 : addressed minor style issue #
Total comments: 4
Patch Set 5 : changed type of max_packet_size to uint16 instead of uint32 #
Total comments: 2
Patch Set 6 : renamed weave_packet to weave_defines #
Messages
Total messages: 24 (8 generated)
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Description was changed from ========== Move weave packet to common location b/t generator and receiver BUG= ========== to ========== Move weave packet to common location b/t generator and receiver BUG=617238 ==========
jingxuy@google.com changed reviewers: + khorimoto@chromium.org, msarda@chromium.org, sacomoto@chromium.org, tengs@chromium.org
https://codereview.chromium.org/2096103003/diff/60001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet.h (right): https://codereview.chromium.org/2096103003/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet.h:14: // Generates the messages sent using the uWeave protocol. This comment needs updating. https://codereview.chromium.org/2096103003/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet.h:15: class BluetoothLowEnergyWeavePacket { Maybe name this something like WeaveDefines. This name makes it sound like this class is the actual packet, when there is a typedef Packet in the class itself. https://codereview.chromium.org/2096103003/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet.h:16: public: You should also make a private constructor, so this class can not be initialized.
https://codereview.chromium.org/2096103003/diff/60001/components/proximity_au... File components/proximity_auth.gypi (right): https://codereview.chromium.org/2096103003/diff/60001/components/proximity_au... components/proximity_auth.gypi:36: "proximity_auth/ble/remote_attribute.h", What changed here? https://codereview.chromium.org/2096103003/diff/60001/components/proximity_au... File components/proximity_auth/ble/BUILD.gn (right): https://codereview.chromium.org/2096103003/diff/60001/components/proximity_au... components/proximity_auth/ble/BUILD.gn:17: "bluetooth_low_energy_weave_client_connection.cc", Please don't add these until the other CL is landed. Same for the test. https://codereview.chromium.org/2096103003/diff/60001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet.h (right): https://codereview.chromium.org/2096103003/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet.h:15: class BluetoothLowEnergyWeavePacket { On 2016/06/27 20:47:30, Tim Song wrote: > Maybe name this something like WeaveDefines. This name makes it sound like this > class is the actual packet, when there is a typedef Packet in the class itself. +1, this should not even be a class at all. Just add an extra namespace layer instead of creating a class. https://codereview.chromium.org/2096103003/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet.h:16: public: On 2016/06/27 20:47:30, Tim Song wrote: > You should also make a private constructor, so this class can not be > initialized. It won't need a constructor at all once you make it not a class. https://codereview.chromium.org/2096103003/diff/60001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc (right): https://codereview.chromium.org/2096103003/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:19: namespace proximity_auth { Why is this moved? Same in the other unit test.
https://codereview.chromium.org/2096103003/diff/60001/components/proximity_au... File components/proximity_auth.gypi (right): https://codereview.chromium.org/2096103003/diff/60001/components/proximity_au... components/proximity_auth.gypi:36: "proximity_auth/ble/remote_attribute.h", On 2016/06/27 20:51:11, Kyle Horimoto wrote: > What changed here? Nothing, I can't shake this file from the git though https://codereview.chromium.org/2096103003/diff/60001/components/proximity_au... File components/proximity_auth/ble/BUILD.gn (right): https://codereview.chromium.org/2096103003/diff/60001/components/proximity_au... components/proximity_auth/ble/BUILD.gn:17: "bluetooth_low_energy_weave_client_connection.cc", On 2016/06/27 20:51:11, Kyle Horimoto wrote: > Please don't add these until the other CL is landed. Same for the test. yah, sorry just ignore them. I'll make sure I'll remove them before submit. It's just hard to switch between BUILD files when I'm working on one branch. https://codereview.chromium.org/2096103003/diff/60001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet.h (right): https://codereview.chromium.org/2096103003/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet.h:14: // Generates the messages sent using the uWeave protocol. On 2016/06/27 20:47:30, Tim Song wrote: > This comment needs updating. Done. https://codereview.chromium.org/2096103003/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet.h:15: class BluetoothLowEnergyWeavePacket { On 2016/06/27 20:51:11, Kyle Horimoto wrote: > On 2016/06/27 20:47:30, Tim Song wrote: > > Maybe name this something like WeaveDefines. This name makes it sound like > this > > class is the actual packet, when there is a typedef Packet in the class > itself. > > +1, this should not even be a class at all. Just add an extra namespace layer > instead of creating a class. Done. https://codereview.chromium.org/2096103003/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet.h:16: public: On 2016/06/27 20:51:11, Kyle Horimoto wrote: > On 2016/06/27 20:47:30, Tim Song wrote: > > You should also make a private constructor, so this class can not be > > initialized. > > It won't need a constructor at all once you make it not a class. Done. https://codereview.chromium.org/2096103003/diff/60001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc (right): https://codereview.chromium.org/2096103003/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:19: namespace proximity_auth { On 2016/06/27 20:51:12, Kyle Horimoto wrote: > Why is this moved? Same in the other unit test. I saw that this is how the connection class does it currently. I think as long as the namespace is anonymous, it achieves the same purpose whether it's inside a namespace or within the global namespace. But if it's declared within proxmity_auth, it has the benefit of accessing functions/types from that namespace. I moved it here for the benefit of the typedefs will not need to go through proximity_auth again.
https://codereview.chromium.org/2096103003/diff/60001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc (right): https://codereview.chromium.org/2096103003/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:19: namespace proximity_auth { On 2016/06/27 22:21:33, jingxuy wrote: > On 2016/06/27 20:51:12, Kyle Horimoto wrote: > > Why is this moved? Same in the other unit test. > > I saw that this is how the connection class does it currently. I think as long > as the namespace is anonymous, it achieves the same purpose whether it's inside > a namespace or within the global namespace. But if it's declared within > proxmity_auth, it has the benefit of accessing functions/types from that > namespace. I moved it here for the benefit of the typedefs will not need to go > through proximity_auth again. Ah, good idea. Thanks for the explanation. https://codereview.chromium.org/2096103003/diff/80001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet.h (right): https://codereview.chromium.org/2096103003/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet.h:37: const uint16_t kDefaultMaxPacketSize = 20; Hmm, does this work? I thought you had to declare these values in your .h and assign them to value in a .cc file, though I'm not 100% sure. https://codereview.chromium.org/2096103003/diff/80001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h (right): https://codereview.chromium.org/2096103003/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:19: namespace weave { nit: Newline before this. Same in other file. https://codereview.chromium.org/2096103003/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:71: } // namespace proximity_auth nit: Newline before this. Same in other file.
https://codereview.chromium.org/2096103003/diff/60001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc (right): https://codereview.chromium.org/2096103003/diff/60001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc:19: namespace proximity_auth { On 2016/06/27 22:25:40, Kyle Horimoto wrote: > On 2016/06/27 22:21:33, jingxuy wrote: > > On 2016/06/27 20:51:12, Kyle Horimoto wrote: > > > Why is this moved? Same in the other unit test. > > > > I saw that this is how the connection class does it currently. I think as long > > as the namespace is anonymous, it achieves the same purpose whether it's > inside > > a namespace or within the global namespace. But if it's declared within > > proxmity_auth, it has the benefit of accessing functions/types from that > > namespace. I moved it here for the benefit of the typedefs will not need to go > > through proximity_auth again. > > Ah, good idea. Thanks for the explanation. np https://codereview.chromium.org/2096103003/diff/80001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet.h (right): https://codereview.chromium.org/2096103003/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet.h:37: const uint16_t kDefaultMaxPacketSize = 20; On 2016/06/27 22:25:41, Kyle Horimoto wrote: > Hmm, does this work? I thought you had to declare these values in your .h and > assign them to value in a .cc file, though I'm not 100% sure. it's passing the test so I presume so? https://codereview.chromium.org/2096103003/diff/80001/components/proximity_au... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h (right): https://codereview.chromium.org/2096103003/diff/80001/components/proximity_au... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:71: } // namespace proximity_auth On 2016/06/27 22:25:41, Kyle Horimoto wrote: > nit: Newline before this. Same in other file. Done.
lgtm https://codereview.chromium.org/2096103003/diff/100001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h (right): https://codereview.chromium.org/2096103003/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:64: uint16_t max_packet_size_; If you're changing this, you also have to change SetMaxPacketSize() to take a uint16_t. But you should do this in another CL as this one is unrelated.
https://codereview.chromium.org/2096103003/diff/100001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h (right): https://codereview.chromium.org/2096103003/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:64: uint16_t max_packet_size_; On 2016/06/27 22:54:17, Kyle Horimoto wrote: > If you're changing this, you also have to change SetMaxPacketSize() to take a > uint16_t. But you should do this in another CL as this one is unrelated. good point, I overlooked it. I might have to do it in this CL as the constants I am using is uint16_t and if this is not matched, it might break on a windows machine. I think it broke earlier because of a uint16 to uint8 mismatch though that one is implicitly casting from uint16 to uint8, so truncating. I am not sure whether implicit elogation is allowed on window.
https://codereview.chromium.org/2096103003/diff/100001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h (right): https://codereview.chromium.org/2096103003/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:64: uint16_t max_packet_size_; On 2016/06/27 23:06:28, jingxuy wrote: > On 2016/06/27 22:54:17, Kyle Horimoto wrote: > > If you're changing this, you also have to change SetMaxPacketSize() to take a > > uint16_t. But you should do this in another CL as this one is unrelated. > > good point, I overlooked it. I might have to do it in this CL as the constants I > am using is uint16_t and if this is not matched, it might break on a windows > machine. I think it broke earlier because of a uint16 to uint8 mismatch though > that one is implicitly casting from uint16 to uint8, so truncating. I am not > sure whether implicit elogation is allowed on window. Alright, go ahead and do it this CL then.
https://codereview.chromium.org/2096103003/diff/100001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h (right): https://codereview.chromium.org/2096103003/diff/100001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h:64: uint16_t max_packet_size_; On 2016/06/27 23:09:14, Kyle Horimoto wrote: > On 2016/06/27 23:06:28, jingxuy wrote: > > On 2016/06/27 22:54:17, Kyle Horimoto wrote: > > > If you're changing this, you also have to change SetMaxPacketSize() to take > a > > > uint16_t. But you should do this in another CL as this one is unrelated. > > > > good point, I overlooked it. I might have to do it in this CL as the constants > I > > am using is uint16_t and if this is not matched, it might break on a windows > > machine. I think it broke earlier because of a uint16 to uint8 mismatch though > > that one is implicitly casting from uint16 to uint8, so truncating. I am not > > sure whether implicit elogation is allowed on window. > > Alright, go ahead and do it this CL then. Done.
https://codereview.chromium.org/2096103003/diff/120001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet.h (right): https://codereview.chromium.org/2096103003/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet.h:5: #ifndef COMPONENTS_PROXIMITY_AUTH_BLE_BLUETOOTH_LOW_ENERGY_WEAVE_PACKET_H_ Also rename this file to something like weave_defines.h
https://codereview.chromium.org/2096103003/diff/120001/components/proximity_a... File components/proximity_auth/ble/bluetooth_low_energy_weave_packet.h (right): https://codereview.chromium.org/2096103003/diff/120001/components/proximity_a... components/proximity_auth/ble/bluetooth_low_energy_weave_packet.h:5: #ifndef COMPONENTS_PROXIMITY_AUTH_BLE_BLUETOOTH_LOW_ENERGY_WEAVE_PACKET_H_ On 2016/06/27 23:16:10, Tim Song wrote: > Also rename this file to something like weave_defines.h Done.
LGTM
The CQ bit was checked by jingxuy@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from khorimoto@chromium.org Link to the patchset: https://codereview.chromium.org/2096103003/#ps140001 (title: "renamed weave_packet to weave_defines")
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 ========== Move weave packet to common location b/t generator and receiver BUG=617238 ========== to ========== Move weave packet to common location b/t generator and receiver BUG=617238 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Move weave packet to common location b/t generator and receiver BUG=617238 ========== to ========== Move weave packet to common location b/t generator and receiver BUG=617238 Committed: https://crrev.com/714da71c4b3b8a763ed61b00ee7f8efa83cc0347 Cr-Commit-Position: refs/heads/master@{#402355} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/714da71c4b3b8a763ed61b00ee7f8efa83cc0347 Cr-Commit-Position: refs/heads/master@{#402355} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
