Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2731273004:
Add Platform Independent Eddystone Encoder (Closed)
DescriptionAdd Platform Independent Eddystone Encoder
This change creates an eddystone encoder in cpp, which can be used
across platforms for future physical web projects. Right now,
specifically, it will be used for Physical Web Sharing. Here are the
eddystone specs:
https://github.com/google/eddystone/blob/master/protocol-specification.md
BUG=685856
Review-Url: https://codereview.chromium.org/2731273004
Cr-Commit-Position: refs/heads/master@{#457164}
Committed: https://chromium.googlesource.com/chromium/src/+/cbdb60c32817557d172f233b1fbf3f9111fc46ec
Patch Set 1 #
Total comments: 22
Patch Set 2 : Matts Nits 1 #
Total comments: 20
Patch Set 3 : Matts Nits Part 2 #
Total comments: 2
Patch Set 4 : Matts nits #
Total comments: 10
Patch Set 5 : Olivier Nitts #Patch Set 6 : Rebasing With Master #
Total comments: 6
Patch Set 7 : Agrieves nits #Patch Set 8 : Fixing Casting Problems #Patch Set 9 : Fixing more Casting #
Messages
Total messages: 50 (23 generated)
Description was changed from ========== Add Platform Independent Eddystone Encoder This change creates an eddystone encoder in cpp, which can be used across platforms for future physical web projects. Right now, specifically, it will be used for Physical Web Sharing. Here are the eddystone specs: https://github.com/google/eddystone/blob/master/protocol-specification.md BUG=685856 ========== to ========== Add Platform Independent Eddystone Encoder This change creates an eddystone encoder in cpp, which can be used across platforms for future physical web projects. Right now, specifically, it will be used for Physical Web Sharing. Here are the eddystone specs: https://github.com/google/eddystone/blob/master/protocol-specification.md BUG=685856 ==========
iankc@google.com changed reviewers: + cco3@chromium.org, mattreynolds@chromium.org
Here is the platform independent eddystone encoder.
https://codereview.chromium.org/2731273004/diff/1/components/physical_web/edd... File components/physical_web/eddystone/BUILD.gn (right): https://codereview.chromium.org/2731273004/diff/1/components/physical_web/edd... components/physical_web/eddystone/BUILD.gn:12: "//base", //base might not be required, can you try building without it? https://codereview.chromium.org/2731273004/diff/1/components/physical_web/edd... File components/physical_web/eddystone/eddystone_encoder.cc (right): https://codereview.chromium.org/2731273004/diff/1/components/physical_web/edd... components/physical_web/eddystone/eddystone_encoder.cc:24: std::vector<uint8_t> EncodeUrl(std::string c_url) { Use a const reference for the input parameter to avoid a copy: const std::string& c_url https://codereview.chromium.org/2731273004/diff/1/components/physical_web/edd... components/physical_web/eddystone/eddystone_encoder.cc:25: std::vector<uint8_t> encodedUrl; Use snake_case for common variables https://codereview.chromium.org/2731273004/diff/1/components/physical_web/edd... components/physical_web/eddystone/eddystone_encoder.cc:28: encodedUrl.push_back(-1); Returning errors by setting invalid values in the returned array is strange and makes extra work for callers. Can we change it to make it easier to check for errors? Maybe something like: int EncodeUrl(const std::string& url, std::vector<uint8_t>* encoded_url) (In Google C++ style out params are always pointers) https://codereview.chromium.org/2731273004/diff/1/components/physical_web/edd... components/physical_web/eddystone/eddystone_encoder.cc:48: size_t prefixLengths[kNumPrefixes] = {}; Can you make this static? Then we can compute it once on the first call and avoid recomputing it for future calls. Also, the initializer is unnecessary since you're setting the contents below. https://codereview.chromium.org/2731273004/diff/1/components/physical_web/edd... File components/physical_web/eddystone/eddystone_encoder.h (right): https://codereview.chromium.org/2731273004/diff/1/components/physical_web/edd... components/physical_web/eddystone/eddystone_encoder.h:5: #ifndef CHROME_BROWSER_ANDROID_EDDYSTONE_ENCODER_H_ Change to COMPONENTS_PHYSICAL_WEB_EDDYSTONE_EDDYSTONE_ENCODER_H_ https://codereview.chromium.org/2731273004/diff/1/components/physical_web/edd... components/physical_web/eddystone/eddystone_encoder.h:12: std::vector<uint8_t> EncodeUrl(std::string c_url); Add a comment describing the method https://codereview.chromium.org/2731273004/diff/1/components/physical_web/edd... File components/physical_web/eddystone/eddystone_encoder_unittest.cc (right): https://codereview.chromium.org/2731273004/diff/1/components/physical_web/edd... components/physical_web/eddystone/eddystone_encoder_unittest.cc:24: bool ByteVectorCmp(vector<uint8_t> a, vector<uint8_t> b); Use const references to avoid copies: bool ByteVectorCmp(const std::vector<uint8_t>& a, const std::vector<uint8_t>& b) https://codereview.chromium.org/2731273004/diff/1/components/physical_web/edd... components/physical_web/eddystone/eddystone_encoder_unittest.cc:28: size_t aSize = a.size(); snake_case https://codereview.chromium.org/2731273004/diff/1/components/physical_web/edd... components/physical_web/eddystone/eddystone_encoder_unittest.cc:280: // decode(encode(".com/aURL")) == decode(encode("http://aURL")) without url Should we check "http://aURL" here too? https://codereview.chromium.org/2731273004/diff/1/components/physical_web/edd... components/physical_web/eddystone/eddystone_encoder_unittest.cc:303: 0x72, 0x65, 0x66}; // "ref" This is perfect, thanks!
Fixed Matts Comments. Please Review again. https://codereview.chromium.org/2731273004/diff/1/components/physical_web/edd... File components/physical_web/eddystone/BUILD.gn (right): https://codereview.chromium.org/2731273004/diff/1/components/physical_web/edd... components/physical_web/eddystone/BUILD.gn:12: "//base", On 2017/03/08 19:14:46, mattreynolds wrote: > //base might not be required, can you try building without it? Done. https://codereview.chromium.org/2731273004/diff/1/components/physical_web/edd... File components/physical_web/eddystone/eddystone_encoder.cc (right): https://codereview.chromium.org/2731273004/diff/1/components/physical_web/edd... components/physical_web/eddystone/eddystone_encoder.cc:24: std::vector<uint8_t> EncodeUrl(std::string c_url) { On 2017/03/08 19:14:46, mattreynolds wrote: > Use a const reference for the input parameter to avoid a copy: > > const std::string& c_url Done. https://codereview.chromium.org/2731273004/diff/1/components/physical_web/edd... components/physical_web/eddystone/eddystone_encoder.cc:25: std::vector<uint8_t> encodedUrl; On 2017/03/08 19:14:46, mattreynolds wrote: > Use snake_case for common variables Done. https://codereview.chromium.org/2731273004/diff/1/components/physical_web/edd... components/physical_web/eddystone/eddystone_encoder.cc:28: encodedUrl.push_back(-1); On 2017/03/08 19:14:46, mattreynolds wrote: > Returning errors by setting invalid values in the returned array is strange and > makes extra work for callers. Can we change it to make it easier to check for > errors? Maybe something like: > > int EncodeUrl(const std::string& url, std::vector<uint8_t>* encoded_url) > > (In Google C++ style out params are always pointers) Done. https://codereview.chromium.org/2731273004/diff/1/components/physical_web/edd... components/physical_web/eddystone/eddystone_encoder.cc:48: size_t prefixLengths[kNumPrefixes] = {}; On 2017/03/08 19:14:46, mattreynolds wrote: > Can you make this static? Then we can compute it once on the first call and > avoid recomputing it for future calls. > > Also, the initializer is unnecessary since you're setting the contents below. Done. https://codereview.chromium.org/2731273004/diff/1/components/physical_web/edd... File components/physical_web/eddystone/eddystone_encoder.h (right): https://codereview.chromium.org/2731273004/diff/1/components/physical_web/edd... components/physical_web/eddystone/eddystone_encoder.h:5: #ifndef CHROME_BROWSER_ANDROID_EDDYSTONE_ENCODER_H_ On 2017/03/08 19:14:46, mattreynolds wrote: > Change to COMPONENTS_PHYSICAL_WEB_EDDYSTONE_EDDYSTONE_ENCODER_H_ Done. https://codereview.chromium.org/2731273004/diff/1/components/physical_web/edd... components/physical_web/eddystone/eddystone_encoder.h:12: std::vector<uint8_t> EncodeUrl(std::string c_url); On 2017/03/08 19:14:47, mattreynolds wrote: > Add a comment describing the method Done. https://codereview.chromium.org/2731273004/diff/1/components/physical_web/edd... File components/physical_web/eddystone/eddystone_encoder_unittest.cc (right): https://codereview.chromium.org/2731273004/diff/1/components/physical_web/edd... components/physical_web/eddystone/eddystone_encoder_unittest.cc:24: bool ByteVectorCmp(vector<uint8_t> a, vector<uint8_t> b); On 2017/03/08 19:14:47, mattreynolds wrote: > Use const references to avoid copies: > > bool ByteVectorCmp(const std::vector<uint8_t>& a, const std::vector<uint8_t>& b) Done. https://codereview.chromium.org/2731273004/diff/1/components/physical_web/edd... components/physical_web/eddystone/eddystone_encoder_unittest.cc:28: size_t aSize = a.size(); On 2017/03/08 19:14:47, mattreynolds wrote: > snake_case Done. https://codereview.chromium.org/2731273004/diff/1/components/physical_web/edd... components/physical_web/eddystone/eddystone_encoder_unittest.cc:280: // decode(encode(".com/aURL")) == decode(encode("http://aURL")) without url On 2017/03/08 19:14:47, mattreynolds wrote: > Should we check "http://aURL" here too? Done. https://codereview.chromium.org/2731273004/diff/1/components/physical_web/edd... components/physical_web/eddystone/eddystone_encoder_unittest.cc:303: 0x72, 0x65, 0x66}; // "ref" On 2017/03/08 19:14:47, mattreynolds wrote: > This is perfect, thanks! Acknowledged.
https://codereview.chromium.org/2731273004/diff/20001/components/physical_web... File components/physical_web/eddystone/eddystone_encoder.cc (right): https://codereview.chromium.org/2731273004/diff/20001/components/physical_web... components/physical_web/eddystone/eddystone_encoder.cc:24: int EncodeUrl(const std::string& url, std::vector<uint8_t>* encoded_url) { Add a check for encoded_url == NULL. https://codereview.chromium.org/2731273004/diff/20001/components/physical_web... components/physical_web/eddystone/eddystone_encoder.cc:26: * Sanatize url. Sanitize URL. https://codereview.chromium.org/2731273004/diff/20001/components/physical_web... components/physical_web/eddystone/eddystone_encoder.cc:29: return -1; Define these error codes as constants in eddystone_encoder.h. Or, consider combining all errors into one failure code (we could just return a boolean true for success). It's not clear to me why clients will need to distinguish these errors, so I suspect we won't lose much by combining them. https://codereview.chromium.org/2731273004/diff/20001/components/physical_web... components/physical_web/eddystone/eddystone_encoder.cc:46: static bool prefix_lengths_loaded = false; How about "lengths_initialized"? (we aren't really loading anything) https://codereview.chromium.org/2731273004/diff/20001/components/physical_web... components/physical_web/eddystone/eddystone_encoder.cc:55: static bool suffix_lengths_loaded = false; You can use the same bool for both array initializations. https://codereview.chromium.org/2731273004/diff/20001/components/physical_web... File components/physical_web/eddystone/eddystone_encoder.h (right): https://codereview.chromium.org/2731273004/diff/20001/components/physical_web... components/physical_web/eddystone/eddystone_encoder.h:13: * EncodeUrl takes a URL in the form of a std::string and Would it make sense to take a GURL as the input param? https://codereview.chromium.org/2731273004/diff/20001/components/physical_web... components/physical_web/eddystone/eddystone_encoder.h:22: * https://github.com/google/eddystone/blob/master/protocol-specification.md Nice comment! A few things to clean up: * Use all-caps for initialisms like URL and HTTP * Capitalize Eddystone * Use the error code names once you've added them https://codereview.chromium.org/2731273004/diff/20001/components/physical_web... File components/physical_web/eddystone/eddystone_encoder_unittest.cc (right): https://codereview.chromium.org/2731273004/diff/20001/components/physical_web... components/physical_web/eddystone/eddystone_encoder_unittest.cc:24: bool ByteVectorCmp(const vector<uint8_t>& a, const vector<uint8_t>& b); Let's rename this to ByteVectorEquals. Cmp suggests it returns -1/0/1 like strcmp, but it's actually returning a bool like std::equals. https://codereview.chromium.org/2731273004/diff/20001/components/physical_web... components/physical_web/eddystone/eddystone_encoder_unittest.cc:29: size_t a_size = a.size(); I guess this isn't needed anymore. https://codereview.chromium.org/2731273004/diff/20001/components/physical_web... components/physical_web/eddystone/eddystone_encoder_unittest.cc:37: TEST_F(EddystoneEncoderTest, ByteVectorCmp) { I think this test isn't really necessary because the implementation is simple enough to be correct by inspection. But, it's good that you wrote this to make sure nothing broke when you changed the implementation.
https://codereview.chromium.org/2731273004/diff/20001/components/physical_web... File components/physical_web/eddystone/eddystone_encoder.cc (right): https://codereview.chromium.org/2731273004/diff/20001/components/physical_web... components/physical_web/eddystone/eddystone_encoder.cc:24: int EncodeUrl(const std::string& url, std::vector<uint8_t>* encoded_url) { On 2017/03/09 22:11:30, mattreynolds wrote: > Add a check for encoded_url == NULL. Done. https://codereview.chromium.org/2731273004/diff/20001/components/physical_web... components/physical_web/eddystone/eddystone_encoder.cc:26: * Sanatize url. On 2017/03/09 22:11:31, mattreynolds wrote: > Sanitize URL. Done. https://codereview.chromium.org/2731273004/diff/20001/components/physical_web... components/physical_web/eddystone/eddystone_encoder.cc:29: return -1; On 2017/03/09 22:11:31, mattreynolds wrote: > Define these error codes as constants in eddystone_encoder.h. Or, consider > combining all errors into one failure code (we could just return a boolean true > for success). It's not clear to me why clients will need to distinguish these > errors, so I suspect we won't lose much by combining them. Done. https://codereview.chromium.org/2731273004/diff/20001/components/physical_web... components/physical_web/eddystone/eddystone_encoder.cc:46: static bool prefix_lengths_loaded = false; On 2017/03/09 22:11:30, mattreynolds wrote: > How about "lengths_initialized"? (we aren't really loading anything) Done. https://codereview.chromium.org/2731273004/diff/20001/components/physical_web... components/physical_web/eddystone/eddystone_encoder.cc:55: static bool suffix_lengths_loaded = false; On 2017/03/09 22:11:30, mattreynolds wrote: > You can use the same bool for both array initializations. Done. https://codereview.chromium.org/2731273004/diff/20001/components/physical_web... File components/physical_web/eddystone/eddystone_encoder.h (right): https://codereview.chromium.org/2731273004/diff/20001/components/physical_web... components/physical_web/eddystone/eddystone_encoder.h:13: * EncodeUrl takes a URL in the form of a std::string and On 2017/03/09 22:11:31, mattreynolds wrote: > Would it make sense to take a GURL as the input param? Acknowledged. https://codereview.chromium.org/2731273004/diff/20001/components/physical_web... components/physical_web/eddystone/eddystone_encoder.h:22: * https://github.com/google/eddystone/blob/master/protocol-specification.md On 2017/03/09 22:11:31, mattreynolds wrote: > Nice comment! A few things to clean up: > > * Use all-caps for initialisms like URL and HTTP > * Capitalize Eddystone > * Use the error code names once you've added them Done. https://codereview.chromium.org/2731273004/diff/20001/components/physical_web... File components/physical_web/eddystone/eddystone_encoder_unittest.cc (right): https://codereview.chromium.org/2731273004/diff/20001/components/physical_web... components/physical_web/eddystone/eddystone_encoder_unittest.cc:24: bool ByteVectorCmp(const vector<uint8_t>& a, const vector<uint8_t>& b); On 2017/03/09 22:11:31, mattreynolds wrote: > Let's rename this to ByteVectorEquals. Cmp suggests it returns -1/0/1 like > strcmp, but it's actually returning a bool like std::equals. Done. https://codereview.chromium.org/2731273004/diff/20001/components/physical_web... components/physical_web/eddystone/eddystone_encoder_unittest.cc:29: size_t a_size = a.size(); On 2017/03/09 22:11:31, mattreynolds wrote: > I guess this isn't needed anymore. Done. https://codereview.chromium.org/2731273004/diff/20001/components/physical_web... components/physical_web/eddystone/eddystone_encoder_unittest.cc:37: TEST_F(EddystoneEncoderTest, ByteVectorCmp) { On 2017/03/09 22:11:31, mattreynolds wrote: > I think this test isn't really necessary because the implementation is simple > enough to be correct by inspection. But, it's good that you wrote this to make > sure nothing broke when you changed the implementation. Acknowledged.
lgtm https://codereview.chromium.org/2731273004/diff/40001/components/physical_web... File components/physical_web/eddystone/eddystone_encoder.h (right): https://codereview.chromium.org/2731273004/diff/40001/components/physical_web... components/physical_web/eddystone/eddystone_encoder.h:13: const int kInvalidURL = -2; In C++, use "Url" when including an initialism in a CamelCase name. E.g. HttpUrlRequest instead of HTTPURLRequest. (Chrome isn't very consistent on this since the style decision was made recently, but new code should abide by the new style.) More info in this thread: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/k8-ztIrWaUY/UIOe3...
https://codereview.chromium.org/2731273004/diff/40001/components/physical_web... File components/physical_web/eddystone/eddystone_encoder.h (right): https://codereview.chromium.org/2731273004/diff/40001/components/physical_web... components/physical_web/eddystone/eddystone_encoder.h:13: const int kInvalidURL = -2; On 2017/03/09 23:39:48, mattreynolds wrote: > In C++, use "Url" when including an initialism in a CamelCase name. E.g. > HttpUrlRequest instead of HTTPURLRequest. (Chrome isn't very consistent on this > since the style decision was made recently, but new code should abide by the new > style.) > > More info in this thread: > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/k8-ztIrWaUY/UIOe3... Done.
iankc@google.com changed reviewers: + olivierrobin@chromium.org
Hey Olivier, I'm an eng res on the Physical Web team working on a Physical Web Sharing feature on clank. We need an eddystone encoder for it and we thought we should just put it into the components directory so it could be shared across platforms. Let us know what you think. Thanks!
LGTM, just nits. https://codereview.chromium.org/2731273004/diff/40002/components/physical_web... File components/physical_web/eddystone/BUILD.gn (right): https://codereview.chromium.org/2731273004/diff/40002/components/physical_web... components/physical_web/eddystone/BUILD.gn:11: public_deps = [ Why public? https://codereview.chromium.org/2731273004/diff/40002/components/physical_web... File components/physical_web/eddystone/eddystone_encoder.cc (right): https://codereview.chromium.org/2731273004/diff/40002/components/physical_web... components/physical_web/eddystone/eddystone_encoder.cc:29: GURL gurl(url); nit: move to line 34 https://codereview.chromium.org/2731273004/diff/40002/components/physical_web... components/physical_web/eddystone/eddystone_encoder.cc:45: static size_t prefix_lengths[kNumPrefixes]; nit: use only one if(!lengths_loaded) { ... ... lengths_loaded = true; } https://codereview.chromium.org/2731273004/diff/40002/components/physical_web... File components/physical_web/eddystone/eddystone_encoder.h (right): https://codereview.chromium.org/2731273004/diff/40002/components/physical_web... components/physical_web/eddystone/eddystone_encoder.h:24: * kInvalidUrl If the URL parameter is not a valid Url. s/Url/URL for consistency https://codereview.chromium.org/2731273004/diff/40002/components/physical_web... components/physical_web/eddystone/eddystone_encoder.h:25: * kInvalidFormat If the URL parameter is not a standard HTTPS or HTTP URL. or is IP URL.
The CQ bit was checked by iankc@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mattreynolds@chromium.org, olivierrobin@chromium.org Link to the patchset: https://codereview.chromium.org/2731273004/#ps70001 (title: "Olivier Nitts")
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 iankc@google.com
Thanks Olivier! https://codereview.chromium.org/2731273004/diff/40002/components/physical_web... File components/physical_web/eddystone/BUILD.gn (right): https://codereview.chromium.org/2731273004/diff/40002/components/physical_web... components/physical_web/eddystone/BUILD.gn:11: public_deps = [ On 2017/03/13 11:57:55, Olivier Robin wrote: > Why public? Done. https://codereview.chromium.org/2731273004/diff/40002/components/physical_web... File components/physical_web/eddystone/eddystone_encoder.cc (right): https://codereview.chromium.org/2731273004/diff/40002/components/physical_web... components/physical_web/eddystone/eddystone_encoder.cc:29: GURL gurl(url); On 2017/03/13 11:57:55, Olivier Robin wrote: > nit: move to line 34 Done. https://codereview.chromium.org/2731273004/diff/40002/components/physical_web... components/physical_web/eddystone/eddystone_encoder.cc:45: static size_t prefix_lengths[kNumPrefixes]; On 2017/03/13 11:57:55, Olivier Robin wrote: > nit: use only one > if(!lengths_loaded) { > ... > ... > lengths_loaded = true; > } Done. https://codereview.chromium.org/2731273004/diff/40002/components/physical_web... File components/physical_web/eddystone/eddystone_encoder.h (right): https://codereview.chromium.org/2731273004/diff/40002/components/physical_web... components/physical_web/eddystone/eddystone_encoder.h:24: * kInvalidUrl If the URL parameter is not a valid Url. On 2017/03/13 11:57:56, Olivier Robin wrote: > s/Url/URL for consistency Done. https://codereview.chromium.org/2731273004/diff/40002/components/physical_web... components/physical_web/eddystone/eddystone_encoder.h:25: * kInvalidFormat If the URL parameter is not a standard HTTPS or HTTP URL. On 2017/03/13 11:57:55, Olivier Robin wrote: > or is IP URL. Acknowledged.
Thanks Olivier!
The CQ bit was checked by iankc@google.com
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by iankc@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mattreynolds@chromium.org, olivierrobin@chromium.org Link to the patchset: https://codereview.chromium.org/2731273004/#ps90001 (title: "Rebasing With Master")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
iankc@google.com changed reviewers: + agrieve@chromium.org
Hi agrieve, This is a CL to add an URL to Eddystone URL encoder in the Java Native Libraries. It's gotten the LGTM from owners of all files except for components/BUILD. Let us know what you think. Thanks!
BUILD file lgtm, but left some bonus suggestions on the code. https://codereview.chromium.org/2731273004/diff/90001/components/physical_web... File components/physical_web/eddystone/eddystone_encoder.cc (right): https://codereview.chromium.org/2731273004/diff/90001/components/physical_web... components/physical_web/eddystone/eddystone_encoder.cc:29: if (encoded_url == NULL) nit: Should be using nullptr rather than NULL for new code. As an aside though, is this check useful? Would an assert be more appropriate here (e.g. would encoded_url == NULL imply a programming error?) https://codereview.chromium.org/2731273004/diff/90001/components/physical_web... components/physical_web/eddystone/eddystone_encoder.cc:49: if (!lengths_loaded) { Did you measure that this was necessary? The strings here are extremely small, so I'd guess this caching is just using up memory unnecessarily. https://codereview.chromium.org/2731273004/diff/90001/components/physical_web... components/physical_web/eddystone/eddystone_encoder.cc:88: encoded_url->push_back(*raw_url_position++); nit: this would be more readable to put on two lines (one for the dereference, one for the increment)
The CQ bit was checked by iankc@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mattreynolds@chromium.org, agrieve@chromium.org, olivierrobin@chromium.org Link to the patchset: https://codereview.chromium.org/2731273004/#ps110001 (title: "Agrieves nits")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
iankc@google.com changed reviewers: + blundell@chromium.org
https://codereview.chromium.org/2731273004/diff/90001/components/physical_web... File components/physical_web/eddystone/eddystone_encoder.cc (right): https://codereview.chromium.org/2731273004/diff/90001/components/physical_web... components/physical_web/eddystone/eddystone_encoder.cc:29: if (encoded_url == NULL) On 2017/03/14 00:30:15, agrieve wrote: > nit: Should be using nullptr rather than NULL for new code. > > As an aside though, is this check useful? Would an assert be more appropriate > here (e.g. would encoded_url == NULL imply a programming error?) Not necessarily. We're writing this for a particular use case now, where it would imply a program error. However, we're trying to make this extensible and we can envision use cases in the future where this would be legal. https://codereview.chromium.org/2731273004/diff/90001/components/physical_web... components/physical_web/eddystone/eddystone_encoder.cc:49: if (!lengths_loaded) { On 2017/03/14 00:30:15, agrieve wrote: > Did you measure that this was necessary? The strings here are extremely small, > so I'd guess this caching is just using up memory unnecessarily. Done. https://codereview.chromium.org/2731273004/diff/90001/components/physical_web... components/physical_web/eddystone/eddystone_encoder.cc:88: encoded_url->push_back(*raw_url_position++); On 2017/03/14 00:30:15, agrieve wrote: > nit: this would be more readable to put on two lines (one for the dereference, > one for the increment) Done.
Hello Colin, This is a CL to make an eddystone encoder for a Physical Web Sharing Feature. It's in components because it could potentially be shared by Android and iOS. We need an LGTM from an owner of the components/build.gn file. Let us know what you think, thanks!
//components/BUILD.gn lgtm
The CQ bit was checked by iankc@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mattreynolds@chromium.org, agrieve@chromium.org, olivierrobin@chromium.org Link to the patchset: https://codereview.chromium.org/2731273004/#ps130001 (title: "Fixing Casting Problems")
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
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by iankc@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mattreynolds@chromium.org, agrieve@chromium.org, olivierrobin@chromium.org, blundell@chromium.org Link to the patchset: https://codereview.chromium.org/2731273004/#ps150001 (title: "Fixing more Casting")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 150001, "attempt_start_ts": 1489600436224760,
"parent_rev": "f2fc623943d585acb44512ebafb626acd7b3f217", "commit_rev":
"cbdb60c32817557d172f233b1fbf3f9111fc46ec"}
Message was sent while issue was closed.
Description was changed from ========== Add Platform Independent Eddystone Encoder This change creates an eddystone encoder in cpp, which can be used across platforms for future physical web projects. Right now, specifically, it will be used for Physical Web Sharing. Here are the eddystone specs: https://github.com/google/eddystone/blob/master/protocol-specification.md BUG=685856 ========== to ========== Add Platform Independent Eddystone Encoder This change creates an eddystone encoder in cpp, which can be used across platforms for future physical web projects. Right now, specifically, it will be used for Physical Web Sharing. Here are the eddystone specs: https://github.com/google/eddystone/blob/master/protocol-specification.md BUG=685856 Review-Url: https://codereview.chromium.org/2731273004 Cr-Commit-Position: refs/heads/master@{#457164} Committed: https://chromium.googlesource.com/chromium/src/+/cbdb60c32817557d172f233b1fbf... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:150001) as https://chromium.googlesource.com/chromium/src/+/cbdb60c32817557d172f233b1fbf... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
