Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
950083003:
device/bluetooth: Implemented UriBeacon decoder. (Closed)
DescriptionImplemented UriBeacon decoder.
Specifications of UriBeacon:
https://github.com/google/uribeacon/tree/master/specification
Here's the description of the encoding of UriBeacon:
https://github.com/google/uribeacon/blob/master/specification/AdvertisingMode.md
Committed: https://crrev.com/96fbd9e457bdfdf78422af9f661e524e97134dab
Cr-Commit-Position: refs/heads/master@{#322187}
Patch Set 1 #
Total comments: 28
Patch Set 2 : #
Total comments: 10
Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #
Total comments: 1
Patch Set 7 : #Patch Set 8 : #Patch Set 9 : #
Messages
Total messages: 55 (24 generated)
dvh@chromium.org changed reviewers: + armansito@chromium.org
Hi Arman, Please take a look. Thanks!
A few comments, I would also stick "device/bluetooth: " as the commit message title prefix. https://codereview.chromium.org/950083003/diff/1/device/bluetooth/uribeacon/u... File device/bluetooth/uribeacon/uri_encoder.h (right): https://codereview.chromium.org/950083003/diff/1/device/bluetooth/uribeacon/u... device/bluetooth/uribeacon/uri_encoder.h:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. nit: Drop the '(c)' bit. https://codereview.chromium.org/950083003/diff/1/device/bluetooth/uribeacon/u... device/bluetooth/uribeacon/uri_encoder.h:15: // specifications. I would probably add a comment-block at the top of this file that describes how to use these utility functions (similar to src/base/ headers), to demonstrate what kind of input-output data is expected. https://codereview.chromium.org/950083003/diff/1/device/bluetooth/uribeacon/u... device/bluetooth/uribeacon/uri_encoder.h:16: void EncodeUriBeaconUri(const base::StringPiece& input, std::string* output); I guess string -> string works as the general data type but it might be more suitable to represent the encoded format as std::vector<uint8_t> instead, which makes it more explicit that this is a series of bytes. It would also be more consistent with other places in device/bluetooth. https://codereview.chromium.org/950083003/diff/1/device/bluetooth/uribeacon/u... File device/bluetooth/uribeacon/uri_encoder_unittest.cc (right): https://codereview.chromium.org/950083003/diff/1/device/bluetooth/uribeacon/u... device/bluetooth/uribeacon/uri_encoder_unittest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. nit: s/(c) 2012/2015/ https://codereview.chromium.org/950083003/diff/1/device/bluetooth/uribeacon/u... device/bluetooth/uribeacon/uri_encoder_unittest.cc:44: 8}; Probably run 'git cl format' at least once :)
I was trying to be consistent with src/base/base64.h and provided very similar APIs. Should I switch anyway to std::vector<uint8_t>? https://codereview.chromium.org/950083003/diff/1/device/bluetooth/uribeacon/u... File device/bluetooth/uribeacon/uri_encoder.h (right): https://codereview.chromium.org/950083003/diff/1/device/bluetooth/uribeacon/u... device/bluetooth/uribeacon/uri_encoder.h:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. On 2015/02/24 23:55:08, armansito wrote: > nit: Drop the '(c)' bit. Done. https://codereview.chromium.org/950083003/diff/1/device/bluetooth/uribeacon/u... File device/bluetooth/uribeacon/uri_encoder_unittest.cc (right): https://codereview.chromium.org/950083003/diff/1/device/bluetooth/uribeacon/u... device/bluetooth/uribeacon/uri_encoder_unittest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2015/02/24 23:55:08, armansito wrote: > nit: s/(c) 2012/2015/ Done. https://codereview.chromium.org/950083003/diff/1/device/bluetooth/uribeacon/u... device/bluetooth/uribeacon/uri_encoder_unittest.cc:44: 8}; On 2015/02/24 23:55:08, armansito wrote: > Probably run 'git cl format' at least once :) It already went through clang-format with setting of Chromium. That was the rendering. Any suggestion about how to format that in a proper way?
> I was trying to be consistent with src/base/base64.h and > provided very similar APIs. Should I switch anyway to > std::vector<uint8_t>? Those look more like string encoding functions so the types makes sense there. I would use a vector<uint8_t> here. It's generally preferred to be consistent with the immediately enclosing codebase. https://codereview.chromium.org/950083003/diff/1/device/bluetooth/uribeacon/u... File device/bluetooth/uribeacon/uri_encoder.cc (right): https://codereview.chromium.org/950083003/diff/1/device/bluetooth/uribeacon/u... device/bluetooth/uribeacon/uri_encoder.cc:49: void device::EncodeUriBeaconUri(const StringPiece& input, std::string* output) { It's generally difficult to understand what the code below is doing. It would be nice to break this down into separate helpers for encoding the prefix and the suffix, finding the prefix and suffix of a URI, and so on. https://codereview.chromium.org/950083003/diff/1/device/bluetooth/uribeacon/u... device/bluetooth/uribeacon/uri_encoder.cc:54: int foundLength = -1; Variable names need to be all lowercase and using underscores instead of camel-case. Here and everywhere else. (See style guide: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Variable_Names) https://codereview.chromium.org/950083003/diff/1/device/bluetooth/uribeacon/u... device/bluetooth/uribeacon/uri_encoder.cc:61: sizeof(prefix_expansions_list) / sizeof(prefix_expansions_list[0]); Use arraysize(x) from base/ (i.e. arraysize(prefix_expansions_list)). https://codereview.chromium.org/950083003/diff/1/device/bluetooth/uribeacon/u... device/bluetooth/uribeacon/uri_encoder.cc:84: } You should generally make sure that the native C++ encoding for characters on all platforms matches the encoding that you're targeting for URI beacon. https://codereview.chromium.org/950083003/diff/1/device/bluetooth/uribeacon/u... File device/bluetooth/uribeacon/uri_encoder_unittest.cc (right): https://codereview.chromium.org/950083003/diff/1/device/bluetooth/uribeacon/u... device/bluetooth/uribeacon/uri_encoder_unittest.cc:27: const char encodedUri[] = {0, nit: This should be |encoded_uri|, according Google C++ style. https://codereview.chromium.org/950083003/diff/1/device/bluetooth/uribeacon/u... device/bluetooth/uribeacon/uri_encoder_unittest.cc:44: 8}; On 2015/02/25 23:52:13, dvh wrote: > On 2015/02/24 23:55:08, armansito wrote: > > Probably run 'git cl format' at least once :) > > It already went through clang-format with setting of Chromium. > That was the rendering. Any suggestion about how to format that in a proper way? Oh well, this is one of those cases when git cl format ends up producing a worse out put. It looks pretty bad to have all of these single-character lines that are indented way too far. It's up to your best judgement at this point, if it were up to me I would have formatted it like this: const char encoded_uri[] = { 0x0, 'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 0x8 }; https://codereview.chromium.org/950083003/diff/1/device/bluetooth/uribeacon/u... device/bluetooth/uribeacon/uri_encoder_unittest.cc:59: const char encodedUri[] = {3, '1', '2', '3', 0, '1', '2', '3'}; nit: |encoded_uri| https://codereview.chromium.org/950083003/diff/1/device/bluetooth/uribeacon/u... device/bluetooth/uribeacon/uri_encoder_unittest.cc:74: const char encodedUri[] = {0, 'u', 'r', 'i', 'b', 'e', 'a', 'c', 'o', 'n', 8}; nit: |encoded_uri| https://codereview.chromium.org/950083003/diff/1/device/bluetooth/uribeacon/u... device/bluetooth/uribeacon/uri_encoder_unittest.cc:89: const char encodedUri[] = {2, 'w', 'e', 'b', '.', 'm', 'i', 't', 2}; nit: |encoded_uri|
All fixed. Please take a look. Thanks! https://codereview.chromium.org/950083003/diff/1/device/bluetooth/uribeacon/u... File device/bluetooth/uribeacon/uri_encoder.cc (right): https://codereview.chromium.org/950083003/diff/1/device/bluetooth/uribeacon/u... device/bluetooth/uribeacon/uri_encoder.cc:49: void device::EncodeUriBeaconUri(const StringPiece& input, std::string* output) { On 2015/02/26 00:39:15, armansito wrote: > It's generally difficult to understand what the code below is doing. It would be > nice to break this down into separate helpers for encoding the prefix and the > suffix, finding the prefix and suffix of a URI, and so on. Done. https://codereview.chromium.org/950083003/diff/1/device/bluetooth/uribeacon/u... device/bluetooth/uribeacon/uri_encoder.cc:54: int foundLength = -1; On 2015/02/26 00:39:15, armansito wrote: > Variable names need to be all lowercase and using underscores instead of > camel-case. Here and everywhere else. (See style guide: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Variable_Names) Done. https://codereview.chromium.org/950083003/diff/1/device/bluetooth/uribeacon/u... device/bluetooth/uribeacon/uri_encoder.cc:61: sizeof(prefix_expansions_list) / sizeof(prefix_expansions_list[0]); On 2015/02/26 00:39:15, armansito wrote: > Use arraysize(x) from base/ (i.e. arraysize(prefix_expansions_list)). Done. https://codereview.chromium.org/950083003/diff/1/device/bluetooth/uribeacon/u... device/bluetooth/uribeacon/uri_encoder.cc:84: } On 2015/02/26 00:39:15, armansito wrote: > You should generally make sure that the native C++ encoding for characters on > all platforms matches the encoding that you're targeting for URI beacon. Done. https://codereview.chromium.org/950083003/diff/1/device/bluetooth/uribeacon/u... File device/bluetooth/uribeacon/uri_encoder.h (right): https://codereview.chromium.org/950083003/diff/1/device/bluetooth/uribeacon/u... device/bluetooth/uribeacon/uri_encoder.h:15: // specifications. On 2015/02/24 23:55:07, armansito wrote: > I would probably add a comment-block at the top of this file that describes how > to use these utility functions (similar to src/base/ headers), to demonstrate > what kind of input-output data is expected. Done. https://codereview.chromium.org/950083003/diff/1/device/bluetooth/uribeacon/u... device/bluetooth/uribeacon/uri_encoder.h:16: void EncodeUriBeaconUri(const base::StringPiece& input, std::string* output); On 2015/02/24 23:55:08, armansito wrote: > I guess string -> string works as the general data type but it might be more > suitable to represent the encoded format as std::vector<uint8_t> instead, which > makes it more explicit that this is a series of bytes. It would also be more > consistent with other places in device/bluetooth. Done. https://codereview.chromium.org/950083003/diff/1/device/bluetooth/uribeacon/u... File device/bluetooth/uribeacon/uri_encoder_unittest.cc (right): https://codereview.chromium.org/950083003/diff/1/device/bluetooth/uribeacon/u... device/bluetooth/uribeacon/uri_encoder_unittest.cc:27: const char encodedUri[] = {0, On 2015/02/26 00:39:16, armansito wrote: > nit: This should be |encoded_uri|, according Google C++ style. Done. https://codereview.chromium.org/950083003/diff/1/device/bluetooth/uribeacon/u... device/bluetooth/uribeacon/uri_encoder_unittest.cc:44: 8}; On 2015/02/26 00:39:15, armansito wrote: > On 2015/02/25 23:52:13, dvh wrote: > > On 2015/02/24 23:55:08, armansito wrote: > > > Probably run 'git cl format' at least once :) > > > > It already went through clang-format with setting of Chromium. > > That was the rendering. Any suggestion about how to format that in a proper > way? > > Oh well, this is one of those cases when git cl format ends up producing a worse > out put. It looks pretty bad to have all of these single-character lines that > are indented way too far. It's up to your best judgement at this point, if it > were up to me I would have formatted it like this: > > const char encoded_uri[] = { > 0x0, 'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', > 'o', 'p', 0x8 > }; Done. https://codereview.chromium.org/950083003/diff/1/device/bluetooth/uribeacon/u... device/bluetooth/uribeacon/uri_encoder_unittest.cc:59: const char encodedUri[] = {3, '1', '2', '3', 0, '1', '2', '3'}; On 2015/02/26 00:39:16, armansito wrote: > nit: |encoded_uri| Done. https://codereview.chromium.org/950083003/diff/1/device/bluetooth/uribeacon/u... device/bluetooth/uribeacon/uri_encoder_unittest.cc:74: const char encodedUri[] = {0, 'u', 'r', 'i', 'b', 'e', 'a', 'c', 'o', 'n', 8}; On 2015/02/26 00:39:16, armansito wrote: > nit: |encoded_uri| Done. https://codereview.chromium.org/950083003/diff/1/device/bluetooth/uribeacon/u... device/bluetooth/uribeacon/uri_encoder_unittest.cc:89: const char encodedUri[] = {2, 'w', 'e', 'b', '.', 'm', 'i', 't', 2}; On 2015/02/26 00:39:15, armansito wrote: > nit: |encoded_uri| Done.
lgtm with nits https://codereview.chromium.org/950083003/diff/20001/device/bluetooth/uribeac... File device/bluetooth/uribeacon/uri_encoder.cc (right): https://codereview.chromium.org/950083003/diff/20001/device/bluetooth/uribeac... device/bluetooth/uribeacon/uri_encoder.cc:66: } nit: no need for curly braces in single-line if body. Here and elsewhere. https://codereview.chromium.org/950083003/diff/20001/device/bluetooth/uribeac... device/bluetooth/uribeacon/uri_encoder.cc:105: exp = LookupPrefixExpansionByValue(input, i); nit: ditto https://codereview.chromium.org/950083003/diff/20001/device/bluetooth/uribeac... device/bluetooth/uribeacon/uri_encoder.cc:106: } else { nit: ditto https://codereview.chromium.org/950083003/diff/20001/device/bluetooth/uribeac... device/bluetooth/uribeacon/uri_encoder.cc:124: if (i == 0) { nit: ditto https://codereview.chromium.org/950083003/diff/20001/device/bluetooth/uribeac... device/bluetooth/uribeacon/uri_encoder.cc:129: if (exp == NULL) { nit: ditto
fixed. https://codereview.chromium.org/950083003/diff/20001/device/bluetooth/uribeac... File device/bluetooth/uribeacon/uri_encoder.cc (right): https://codereview.chromium.org/950083003/diff/20001/device/bluetooth/uribeac... device/bluetooth/uribeacon/uri_encoder.cc:66: } On 2015/03/06 22:00:30, armansito wrote: > nit: no need for curly braces in single-line if body. Here and elsewhere. Done. https://codereview.chromium.org/950083003/diff/20001/device/bluetooth/uribeac... device/bluetooth/uribeacon/uri_encoder.cc:105: exp = LookupPrefixExpansionByValue(input, i); On 2015/03/06 22:00:30, armansito wrote: > nit: ditto Done. https://codereview.chromium.org/950083003/diff/20001/device/bluetooth/uribeac... device/bluetooth/uribeacon/uri_encoder.cc:106: } else { On 2015/03/06 22:00:30, armansito wrote: > nit: ditto Done. https://codereview.chromium.org/950083003/diff/20001/device/bluetooth/uribeac... device/bluetooth/uribeacon/uri_encoder.cc:124: if (i == 0) { On 2015/03/06 22:00:30, armansito wrote: > nit: ditto Done. https://codereview.chromium.org/950083003/diff/20001/device/bluetooth/uribeac... device/bluetooth/uribeacon/uri_encoder.cc:129: if (exp == NULL) { On 2015/03/06 22:00:30, armansito wrote: > nit: ditto Done.
lgtm
The CQ bit was checked by dvh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/950083003/40001
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by dvh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from armansito@chromium.org Link to the patchset: https://codereview.chromium.org/950083003/#ps60001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/950083003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dvh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from armansito@chromium.org Link to the patchset: https://codereview.chromium.org/950083003/#ps80001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/950083003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dvh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from armansito@chromium.org Link to the patchset: https://codereview.chromium.org/950083003/#ps100001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/950083003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) Timed out jobs are not retried to avoid causing additional load on the builders with large pending queues.
armansito@, do you have an idea about what's going on? I have this issues on Linux build: ../../device/bluetooth/uribeacon/uri_encoder_unittest.cc:19: error: undefined reference to 'device::EncodeUriBeaconUri(std::string const&, std::__debug::vector<unsigned char, std::allocator<unsigned char> >&)' ../../device/bluetooth/uribeacon/uri_encoder_unittest.cc:22: error: undefined reference to 'device::DecodeUriBeaconUri(std::__debug::vector<unsigned char, std::allocator<unsigned char> > const&, std::string&)' device_unittests fails to link. It's correctly linking against libdevice_bluetooth but it looks like libdevice_bluetooth does not include uri_encoder.cc
https://codereview.chromium.org/950083003/diff/100001/device/BUILD.gn File device/BUILD.gn (left): https://codereview.chromium.org/950083003/diff/100001/device/BUILD.gn#oldcode25 device/BUILD.gn:25: "bluetooth/bluetooth_task_manager_win_unittest.cc", You probably need to add "//device/bluetooth/uribeacon" to deps under "device_unittests".
The CQ bit was checked by dvh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from armansito@chromium.org Link to the patchset: https://codereview.chromium.org/950083003/#ps120001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/950083003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
You should really test building this using GN.
On 2015/03/12 00:31:33, armansito wrote: > You should really test building this using GN. I was starting to look at some documentation of GN to understand how it was working.
The CQ bit was checked by dvh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/950083003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dvh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from armansito@chromium.org Link to the patchset: https://codereview.chromium.org/950083003/#ps140001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/950083003/140001
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by dvh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from armansito@chromium.org Link to the patchset: https://codereview.chromium.org/950083003/#ps160001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/950083003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by dvh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/950083003/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/96fbd9e457bdfdf78422af9f661e524e97134dab Cr-Commit-Position: refs/heads/master@{#322187} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
