Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2564483003:
Default share to Share Target: Partial impl. of Web Share for Desktop. (Closed)
|
Created:
4 years ago by constantina Modified:
3 years, 11 months ago CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org, dcheng Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDefault share to Share Target: Partial impl. of Web Share for Desktop.
Currently only shares to a demo Share Target App hosted at
https://wicg.github.io/web-share-target/demos/sharetarget.html;
follow-up CL will have more share options.
BUG=668389
Review-Url: https://codereview.chromium.org/2564483003
Cr-Commit-Position: refs/heads/master@{#443510}
Committed: https://chromium.googlesource.com/chromium/src/+/2cfa55e24997bd05c573b163a630f1dac13cd184
Patch Set 1 #Patch Set 2 : Replace Twitter target web app with Web Share Target web app #Patch Set 3 : New tab opens next to the current tab, and is brought to foreground #Patch Set 4 : New algorithm replaces all instances of placeholders, no matter the order, and maintains the order #Patch Set 5 : Added test cases #Patch Set 6 : Updated Share Target app address #Patch Set 7 : Fixed my nits #Patch Set 8 : Refactored test, so there is a test helper subclass of ShareServiceImpl #Patch Set 9 : Check the target_url opened by Share, in test #
Total comments: 51
Patch Set 10 : Corrected all minor feedback, as well as expanding on tests. #Patch Set 11 : Added a few more tests. #Patch Set 12 : Allow ReplacePlaceholders to return an error. #Patch Set 13 : Reimplemented ReplacePlaceholders algorithm. #
Total comments: 37
Patch Set 14 : Fixed, as per feedback. #Patch Set 15 : Updated documentation #
Total comments: 75
Patch Set 16 : Fixed, according to feedback. #Patch Set 17 : Reimplemented algorithm, and changed error cases. Test cases changed as appropriate. #
Total comments: 49
Patch Set 18 : Updated includes. #
Total comments: 26
Patch Set 19 : Fixed, according to feedback, excluding reimplementation. #Patch Set 20 : Inverted if statements when looking for delimiters. #Patch Set 21 : Remove % from opening delimiter #
Total comments: 22
Patch Set 22 : Refactored algorithm, fixed according to feedback. #Patch Set 23 : Use StringPiece.substr() #
Total comments: 6
Patch Set 24 : Fixed, according to feedback. #
Total comments: 12
Patch Set 25 : Fixed, according to feedback. #
Total comments: 2
Patch Set 26 : Add TODO. #Patch Set 27 : Rebase #Patch Set 28 : Prevent code running on android. #Patch Set 29 : Fix typo #Patch Set 30 #
Dependent Patchsets: Messages
Total messages: 55 (22 generated)
Description was changed from ========== Implemented Web Share for Desktop. Currently only shares to Twitter; follow-up CL will have more share options. BUG=668389 ========== to ========== Implemented Web Share for Desktop. Currently only shares to Twitter; follow-up CL will have more share options. BUG=668389 ==========
Description was changed from ========== Implemented Web Share for Desktop. Currently only shares to Twitter; follow-up CL will have more share options. BUG=668389 ========== to ========== Implemented Web Share for Desktop. Currently only shares to a demo Share Target App hosted at https://cpyro.github.io/web-share-target-1/demos/sharetarget.html; follow-up CL will have more share options. BUG=668389 ==========
Description was changed from ========== Implemented Web Share for Desktop. Currently only shares to a demo Share Target App hosted at https://cpyro.github.io/web-share-target-1/demos/sharetarget.html; follow-up CL will have more share options. BUG=668389 ========== to ========== Implemented Web Share for Desktop. Currently only shares to a demo Share Target App hosted at https://wicg.github.io/web-share-target/demos/sharetarget.html; follow-up CL will have more share options. BUG=668389 ==========
constantina@google.com changed reviewers: + mgiuca@chromium.org, sammc@chromium.org
PTAL
A few comments for you :) I suggest you start working on the tests first (TDD for the win); then that will tell you exactly what behaviour needs to be changed and you will know when you change the algorithm whether you are breaking things and when it's good. https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:26: std::string ShareServiceImpl::replacePlaceholders( As Sam suggested, you should just search (in one pass) for all "%{" sequences and mark them (regardless of what they contain). This allows you to properly deal with non-standard fields (which should become empty, for future compatibility) and avoid multiple passes over the string. https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:36: std::string title_placeholder = "%{title}"; Nit: kTitlePlaceholder, kTextPlaceholder and kUrlPlaceholder. (Even when local variables, still follow this convention.) Also you should not have const std::strings; instead the convention is: const char kTitlePlaceholder[] = "%{title}"; Rationale is that this can be allocated into static memory rather than being dynamically created every time the function runs. https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:50: for (std::string placeholder : placeholders) { const std::string& or const auto& https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:65: for (std::pair<int, std::string> placeholder : placeholder_positions) { For the second pass, you can avoid doing replacements on the string (in reverse) by simply building a brand new string from scratch, copying over the non-replaced segments of the old string into the new one. https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:92: std::string url_base = "https://wicg.github.io/web-share-target/"; const char kUrlBase[] https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:102: // TODO(constantina): Check for errors. You should do this in this CL. If you don't have any errors, this logic should be simplified so it doesn't check at all. https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.h (right): https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. CL Description: 1. The title is misleading since this doesn't properly implement WS (since the user can't choose). 2. Wrap to 72 chars. https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:24: void Share(const std::string& title, Add a comment: // ShareService overrides: (By convention, all overrides from a certain base class should have a similar comment at the top of the block.) https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:26: const GURL& share_url, Why not just |url|? https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:29: virtual void openTargetURL(GURL target_url); nit: Names should start with a capital letter. This should be protected (because it is only used by the subclass). Document what it does, and mention that it is virtual for testing. https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:32: FRIEND_TEST_ALL_PREFIXES(ShareServiceImplUnittest, ReplacePlaceholders); 👍 https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl_unittest.cc (right): https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:17: class ShareServiceImplTestHelper : public ShareServiceImpl { "TestHelper" implies that it is a collection of helper methods. Rather, this *is* the Share Service (but subclassed with extra test magic). How about: ShareServiceTestImpl or ShareServiceImplForTest? https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:30: void openTargetURL(GURL target_url) override { const GURL& Get into the habit of always using "const X&" in place of X whenever X is a non-primitive value parameter. It means one less copy of the data. https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:30: void openTargetURL(GURL target_url) override { Nit: Names should start with a capital letter. https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:34: std::string last_used_target_url_; Even in a test, you shouldn't have public fields. Make this private and provide a getter. https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:70: std::string target_url = nit: name this expected_url. https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:83: share_service_->Share("title", "text", url, callback); Make title and text constants too. In fact, these are kind of the same in all tests; why not make them global consts and just reuse them in both tests (which would make the title "My title" and "My text"). https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:91: const std::string title = "My title"; As above, use const char kTitle[] = "My title", etc. The static allocation thing doesn't really matter for tests but still good practice. https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:95: // One title placeholder Nit: Full stops at the end. https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:135: } Some other things to test: - Badly nested placeholders: "%{title", "%{title{", "%{title %{url}}", "%{}". - Unexpected placeholders: "%{badplaceholder}". These should just be substituted with "", for future compatibility. - Format string with no placeholder: "example.com". - Empty format string: "". - Format string with percent escapes in it: "%20%{title}" (to make sure you don't crap out when you see a '%' that isn't followed by a '{'). And testing some other substitution strings: - Containing percent-escapes: title="My%20title"; should expand to "My%2520title". - Containing "%{": title="My %{text}" substituted into "%{title}%{text}" should become "My%20%25{text}My%20text", and not get double-substituted. (This would create an illegal URL but that's not your concern.) - Containing other characters. It might be worth having a string that simply has all the ASCII characters, and note exactly which ones need to get escaped: "\0\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f !\"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~" (Maybe split control and non-control characters out into separate tests.) - Containing Unicode characters (Chinese, etc). That needs to work. https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:139: TEST_F(ShareServiceImplUnittest, ReplacePlaceholdersInQuery) { I think this could be merged into the previous test. It isn't really necessary to test both with a query and with no other letters (since there is no special logic for queries). Just put in the query syntax into the previous tests and you won't need this one.
As mentioned on https://codereview.chromium.org/2545323002/, please include a *random* reviewer from //ipc/SECURITY_OWNERS, since the implementation should go through security review as well. Thanks!
https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:36: std::string title_placeholder = "%{title}"; On 2016/12/21 07:13:49, Matt Giuca (OOO til Jan 5) wrote: > Nit: kTitlePlaceholder, kTextPlaceholder and kUrlPlaceholder. > > (Even when local variables, still follow this convention.) > > Also you should not have const std::strings; instead the convention is: > > const char kTitlePlaceholder[] = "%{title}"; > > Rationale is that this can be allocated into static memory rather than being > dynamically created every time the function runs. Better yet, use constexpr char kTitlePlaceholder[]. https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:49: std::vector<std::pair<int, std::string>> placeholder_positions; Prefer base::StringPiece when you can refer to part of an existing string. https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.h (right): https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:38: static std::string replacePlaceholders(const std::string url_template, ReplacePlaceholders base::StringPiece
Description was changed from ========== Implemented Web Share for Desktop. Currently only shares to a demo Share Target App hosted at https://wicg.github.io/web-share-target/demos/sharetarget.html; follow-up CL will have more share options. BUG=668389 ========== to ========== Default share to Share Target: Partial impl. of Web Share for Desktop. Currently only shares to a demo Share Target App hosted at https://wicg.github.io/web-share-target/demos/sharetarget.html; follow-up CL will have more share options. BUG=668389 ==========
https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:26: std::string ShareServiceImpl::replacePlaceholders( On 2016/12/21 07:13:49, Matt Giuca (OOO til Jan 5) wrote: > As Sam suggested, you should just search (in one pass) for all "%{" sequences > and mark them (regardless of what they contain). This allows you to properly > deal with non-standard fields (which should become empty, for future > compatibility) and avoid multiple passes over the string. Done. https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:36: std::string title_placeholder = "%{title}"; On 2016/12/21 07:13:49, Matt Giuca (OOO til Jan 5) wrote: > Nit: kTitlePlaceholder, kTextPlaceholder and kUrlPlaceholder. > > (Even when local variables, still follow this convention.) > > Also you should not have const std::strings; instead the convention is: > > const char kTitlePlaceholder[] = "%{title}"; > > Rationale is that this can be allocated into static memory rather than being > dynamically created every time the function runs. Done. https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:36: std::string title_placeholder = "%{title}"; On 2016/12/22 07:03:50, Sam McNally wrote: > On 2016/12/21 07:13:49, Matt Giuca (OOO til Jan 5) wrote: > > Nit: kTitlePlaceholder, kTextPlaceholder and kUrlPlaceholder. > > > > (Even when local variables, still follow this convention.) > > > > Also you should not have const std::strings; instead the convention is: > > > > const char kTitlePlaceholder[] = "%{title}"; > > > > Rationale is that this can be allocated into static memory rather than being > > dynamically created every time the function runs. > > Better yet, use constexpr char kTitlePlaceholder[]. Done. https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:49: std::vector<std::pair<int, std::string>> placeholder_positions; On 2016/12/22 07:03:50, Sam McNally wrote: > Prefer base::StringPiece when you can refer to part of an existing string. Done. Also changed |placeholders|. https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:50: for (std::string placeholder : placeholders) { On 2016/12/21 07:13:49, Matt Giuca (OOO til Jan 5) wrote: > const std::string& > > or const auto& Done. https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:65: for (std::pair<int, std::string> placeholder : placeholder_positions) { On 2016/12/21 07:13:49, Matt Giuca (OOO til Jan 5) wrote: > For the second pass, you can avoid doing replacements on the string (in reverse) > by simply building a brand new string from scratch, copying over the > non-replaced segments of the old string into the new one. Done. https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:92: std::string url_base = "https://wicg.github.io/web-share-target/"; On 2016/12/21 07:13:49, Matt Giuca (OOO til Jan 5) wrote: > const char kUrlBase[] Done. https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:102: // TODO(constantina): Check for errors. On 2016/12/21 07:13:49, Matt Giuca (OOO til Jan 5) wrote: > You should do this in this CL. If you don't have any errors, this logic should > be simplified so it doesn't check at all. Done. https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.h (right): https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/12/21 07:13:49, Matt Giuca (OOO til Jan 5) wrote: > CL Description: > > 1. The title is misleading since this doesn't properly implement WS (since the > user can't choose). > 2. Wrap to 72 chars. Done. https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:24: void Share(const std::string& title, On 2016/12/21 07:13:49, Matt Giuca (OOO til Jan 5) wrote: > Add a comment: > // ShareService overrides: > > (By convention, all overrides from a certain base class should have a similar > comment at the top of the block.) Done. The following example I found had the full namespace, so i did that: https://cs.chromium.org/chromium/src/services/ui/ws/gpu_host.cc?l=55&dr=C https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:26: const GURL& share_url, On 2016/12/21 07:13:49, Matt Giuca (OOO til Jan 5) wrote: > Why not just |url|? The implementation references lots of different types of urls: url_template, url_template_filled, url_base, target_url. I suppose those are unambiguously named, and the only url left is the one being shared, so I can change it to just be url. I think shared_url is even better than share_url, but not sure about past tense conventions. What do you think? https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:29: virtual void openTargetURL(GURL target_url); On 2016/12/21 07:13:49, Matt Giuca (OOO til Jan 5) wrote: > nit: Names should start with a capital letter. > > This should be protected (because it is only used by the subclass). Document > what it does, and mention that it is virtual for testing. Done. https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:32: FRIEND_TEST_ALL_PREFIXES(ShareServiceImplUnittest, ReplacePlaceholders); On 2016/12/21 07:13:49, Matt Giuca (OOO til Jan 5) wrote: > 👍 👍 https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:38: static std::string replacePlaceholders(const std::string url_template, On 2016/12/22 07:03:50, Sam McNally wrote: > ReplacePlaceholders > base::StringPiece Changed Capitalisation. Did you want me to change url_template, or the returned value to base::StringPiece? Or both? All the parameters? https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl_unittest.cc (right): https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:17: class ShareServiceImplTestHelper : public ShareServiceImpl { On 2016/12/21 07:13:49, Matt Giuca (OOO til Jan 5) wrote: > "TestHelper" implies that it is a collection of helper methods. Rather, this > *is* the Share Service (but subclassed with extra test magic). > > How about: ShareServiceTestImpl or ShareServiceImplForTest? Done. https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:30: void openTargetURL(GURL target_url) override { On 2016/12/21 07:13:50, Matt Giuca (OOO til Jan 5) wrote: > Nit: Names should start with a capital letter. Done. https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:30: void openTargetURL(GURL target_url) override { On 2016/12/21 07:13:49, Matt Giuca (OOO til Jan 5) wrote: > const GURL& > > Get into the habit of always using "const X&" in place of X whenever X is a > non-primitive value parameter. It means one less copy of the data. Done. https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:34: std::string last_used_target_url_; On 2016/12/21 07:13:49, Matt Giuca (OOO til Jan 5) wrote: > Even in a test, you shouldn't have public fields. Make this private and provide > a getter. Done. https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:70: std::string target_url = On 2016/12/21 07:13:50, Matt Giuca (OOO til Jan 5) wrote: > nit: name this expected_url. Done. https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:83: share_service_->Share("title", "text", url, callback); On 2016/12/21 07:13:49, Matt Giuca (OOO til Jan 5) wrote: > Make title and text constants too. > > In fact, these are kind of the same in all tests; why not make them global > consts and just reuse them in both tests (which would make the title "My title" > and "My text"). Done. https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:91: const std::string title = "My title"; On 2016/12/21 07:13:50, Matt Giuca (OOO til Jan 5) wrote: > As above, use const char kTitle[] = "My title", etc. > > The static allocation thing doesn't really matter for tests but still good > practice. Done. https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:95: // One title placeholder On 2016/12/21 07:13:50, Matt Giuca (OOO til Jan 5) wrote: > Nit: Full stops at the end. Done. https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:135: } On 2016/12/21 07:13:49, Matt Giuca (OOO til Jan 5) wrote: > Some other things to test: > - Badly nested placeholders: "%{title", "%{title{", "%{title %{url}}", "%{}". > - Unexpected placeholders: "%{badplaceholder}". These should just be substituted > with "", for future compatibility. > - Format string with no placeholder: "example.com". > - Empty format string: "". > - Format string with percent escapes in it: "%20%{title}" (to make sure you > don't crap out when you see a '%' that isn't followed by a '{'). > > And testing some other substitution strings: > - Containing percent-escapes: title="My%20title"; should expand to > "My%2520title". > - Containing "%{": title="My %{text}" substituted into "%{title}%{text}" should > become "My%20%25{text}My%20text", and not get double-substituted. (This would > create an illegal URL but that's not your concern.) > - Containing other characters. It might be worth having a string that simply has > all the ASCII characters, and note exactly which ones need to get escaped: > > "\0\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f > !\"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~" > (Maybe split control and non-control characters out into separate tests.) > - Containing Unicode characters (Chinese, etc). That needs to work. Done. https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:139: TEST_F(ShareServiceImplUnittest, ReplacePlaceholdersInQuery) { On 2016/12/21 07:13:49, Matt Giuca (OOO til Jan 5) wrote: > I think this could be merged into the previous test. It isn't really necessary > to test both with a query and with no other letters (since there is no special > logic for queries). > > Just put in the query syntax into the previous tests and you won't need this > one. Not quite sure what you meant, so I took the last test in this function and included in the previous. Deleted the rest.
constantina@google.com changed reviewers: + kenrb@chromium.org
+kenrb for ipc security
https://codereview.chromium.org/2564483003/diff/240001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2564483003/diff/240001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:51: std::map<base::StringPiece, base::StringPiece> placeholder_to_data; You could use an initializer list to initialise this: ... placeholder_to_data = {{kTitlePlaceholder, title_escaped}, ...}; https://codereview.chromium.org/2564483003/diff/240001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:59: std::vector<int> placeholder_open_indices; Consider building a std::vector<base::StringPiece> of string segments to glue together. https://codereview.chromium.org/2564483003/diff/240001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:61: bool lastSawOpen = false; last_saw_open https://codereview.chromium.org/2564483003/diff/240001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:62: for (unsigned long i = 0; i < url_template.size(); ++i) { size_t https://codereview.chromium.org/2564483003/diff/240001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:88: std::string url_template_filled = ""; The default constructor is fine. It's probably worth using reserve() though. https://codereview.chromium.org/2564483003/diff/240001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:89: int start_index_to_copy = 0; size_t https://codereview.chromium.org/2564483003/diff/240001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:100: if (placeholder_to_data.count(placeholder)) { Use placeholder_to_data.find(). This avoids a second lookup with operator[]. https://codereview.chromium.org/2564483003/diff/240001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:101: url_template_filled += placeholder_to_data[placeholder].as_string(); Use placeholder_to_data[placeholder].AppendToString(&url_template_filled) That should avoid an unnecessary copy. https://codereview.chromium.org/2564483003/diff/240001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.h (right): https://codereview.chromium.org/2564483003/diff/240001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:33: virtual void OpenTargetURL(const GURL& target_url); This doesn't need to be protected to override it. https://codereview.chromium.org/2564483003/diff/240001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:37: FRIEND_TEST_ALL_PREFIXES(ShareServiceImplUnittest, Remove. https://codereview.chromium.org/2564483003/diff/240001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:47: static std::string ReplacePlaceholders(const std::string url_template, Take base::StringPiece unless you need pass it to something that requires a std::string; in that case take a const std::string&. https://codereview.chromium.org/2564483003/diff/240001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:51: int* error); It's more common for the error code/bool to be returned and the value produced written to an out parameter. If there's no information beyond success or failure, returning base::Optional<std::string> works too. https://codereview.chromium.org/2564483003/diff/240001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl_unittest.cc (right): https://codereview.chromium.org/2564483003/diff/240001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:16: namespace { https://codereview.chromium.org/2564483003/diff/240001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:17: const char kTitle[] = "My title"; constexpr https://codereview.chromium.org/2564483003/diff/240001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:33: std::string GetLastUsedTargetURL() { return last_used_target_url_; } const std::string& (or const GURL&) https://codereview.chromium.org/2564483003/diff/240001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:38: protected: protected before private. https://codereview.chromium.org/2564483003/diff/240001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:58: void DidShare(const std::string expected_target_url, & https://codereview.chromium.org/2564483003/diff/240001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:175: // TODO(constantina): come back to this and think about returned value Longer term I expect we'd reject malformed templates ahead of time.
https://codereview.chromium.org/2564483003/diff/240001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2564483003/diff/240001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:51: std::map<base::StringPiece, base::StringPiece> placeholder_to_data; On 2017/01/04 09:15:46, Sam McNally wrote: > You could use an initializer list to initialise this: > ... placeholder_to_data = {{kTitlePlaceholder, title_escaped}, ...}; I will keep it like this, in case we expand the API to allow more fields in the shared data. I have however changed the previous 2 blocks to use initializer lists. https://codereview.chromium.org/2564483003/diff/240001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:59: std::vector<int> placeholder_open_indices; On 2017/01/04 09:15:46, Sam McNally wrote: > Consider building a std::vector<base::StringPiece> of string segments to glue > together. Acknowledged. https://codereview.chromium.org/2564483003/diff/240001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:61: bool lastSawOpen = false; On 2017/01/04 09:15:46, Sam McNally wrote: > last_saw_open Done. https://codereview.chromium.org/2564483003/diff/240001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:62: for (unsigned long i = 0; i < url_template.size(); ++i) { On 2017/01/04 09:15:46, Sam McNally wrote: > size_t Done. https://codereview.chromium.org/2564483003/diff/240001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:88: std::string url_template_filled = ""; On 2017/01/04 09:15:46, Sam McNally wrote: > The default constructor is fine. It's probably worth using reserve() though. Done. https://codereview.chromium.org/2564483003/diff/240001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:89: int start_index_to_copy = 0; On 2017/01/04 09:15:46, Sam McNally wrote: > size_t Done. https://codereview.chromium.org/2564483003/diff/240001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:100: if (placeholder_to_data.count(placeholder)) { On 2017/01/04 09:15:46, Sam McNally wrote: > Use placeholder_to_data.find(). This avoids a second lookup with operator[]. Done. https://codereview.chromium.org/2564483003/diff/240001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:101: url_template_filled += placeholder_to_data[placeholder].as_string(); On 2017/01/04 09:15:46, Sam McNally wrote: > Use placeholder_to_data[placeholder].AppendToString(&url_template_filled) > > That should avoid an unnecessary copy. Done. https://codereview.chromium.org/2564483003/diff/240001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.h (right): https://codereview.chromium.org/2564483003/diff/240001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:33: virtual void OpenTargetURL(const GURL& target_url); On 2017/01/04 09:15:46, Sam McNally wrote: > This doesn't need to be protected to override it. Matt said to make it protected, but private makes more sense to me. Changed. https://codereview.chromium.org/2564483003/diff/240001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:37: FRIEND_TEST_ALL_PREFIXES(ShareServiceImplUnittest, On 2017/01/04 09:15:46, Sam McNally wrote: > Remove. Done. https://codereview.chromium.org/2564483003/diff/240001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:47: static std::string ReplacePlaceholders(const std::string url_template, On 2017/01/04 09:15:46, Sam McNally wrote: > Take base::StringPiece unless you need pass it to something that requires a > std::string; in that case take a const std::string&. Done. https://codereview.chromium.org/2564483003/diff/240001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:51: int* error); On 2017/01/04 09:15:46, Sam McNally wrote: > It's more common for the error code/bool to be returned and the value produced > written to an out parameter. If there's no information beyond success or > failure, returning base::Optional<std::string> works too. Done. https://codereview.chromium.org/2564483003/diff/240001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl_unittest.cc (right): https://codereview.chromium.org/2564483003/diff/240001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:16: On 2017/01/04 09:15:46, Sam McNally wrote: > namespace { Done. https://codereview.chromium.org/2564483003/diff/240001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:17: const char kTitle[] = "My title"; On 2017/01/04 09:15:46, Sam McNally wrote: > constexpr Done. https://codereview.chromium.org/2564483003/diff/240001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:33: std::string GetLastUsedTargetURL() { return last_used_target_url_; } On 2017/01/04 09:15:46, Sam McNally wrote: > const std::string& (or const GURL&) Done. https://codereview.chromium.org/2564483003/diff/240001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:38: protected: On 2017/01/04 09:15:47, Sam McNally wrote: > protected before private. Changed this to private. https://codereview.chromium.org/2564483003/diff/240001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:58: void DidShare(const std::string expected_target_url, On 2017/01/04 09:15:47, Sam McNally wrote: > & Done. https://codereview.chromium.org/2564483003/diff/240001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:175: // TODO(constantina): come back to this and think about returned value On 2017/01/04 09:15:46, Sam McNally wrote: > Longer term I expect we'd reject malformed templates ahead of time. Ah yes, this makes sense. I have recorded this, and will keep it in mind for the future.
The CQ bit was checked by constantina@google.com 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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:36: std::string title_placeholder = "%{title}"; On 2016/12/22 07:03:50, Sam McNally wrote: > On 2016/12/21 07:13:49, Matt Giuca (OOO til Jan 5) wrote: > > Nit: kTitlePlaceholder, kTextPlaceholder and kUrlPlaceholder. > > > > (Even when local variables, still follow this convention.) > > > > Also you should not have const std::strings; instead the convention is: > > > > const char kTitlePlaceholder[] = "%{title}"; > > > > Rationale is that this can be allocated into static memory rather than being > > dynamically created every time the function runs. > > Better yet, use constexpr char kTitlePlaceholder[]. Sam: Is your advice just to always use constexpr in place of const if the RHS is a compile-time constant? https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.h (right): https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:26: const GURL& share_url, On 2017/01/04 07:01:45, constantina wrote: > On 2016/12/21 07:13:49, Matt Giuca (OOO til Jan 5) wrote: > > Why not just |url|? > > The implementation references lots of different types of urls: url_template, > url_template_filled, url_base, target_url. I suppose those are unambiguously > named, and the only url left is the one being shared, so I can change it to just > be url. Right, makes sense. But you should not change the names of parameters when overriding a method (that is valid, but confusing). Feel free to disambiguate this in all of your internal methods here (like replacePlaceholders) but don't change this name here (or in the .cc file for this method). > I think shared_url is even better than share_url, but not sure about > past tense conventions. What do you think? I agree shared_url is better. https://codereview.chromium.org/2564483003/diff/240001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.h (right): https://codereview.chromium.org/2564483003/diff/240001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:33: virtual void OpenTargetURL(const GURL& target_url); On 2017/01/05 00:34:38, constantina wrote: > On 2017/01/04 09:15:46, Sam McNally wrote: > > This doesn't need to be protected to override it. > > Matt said to make it protected, but private makes more sense to me. Changed. Acknowledged. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:36: constexpr char kTitlePlaceholder[] = "%{title}"; It would be much nicer I think if we could avoid having the "%{" and "}" here (and in the map). Can we just make it so these constants are "title" rather than "%{title}"? https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:47: for (size_t i = 0; i < placeholders.size(); ++i) { I think this would read much clearer if it was just explicit rather than building all these data structures first: placeholder_to_data[kTitlePlaceholder] = net::EscapeQueryParamValue(title, false); placeholder_to_data[kTextPlaceholder] = net::EscapeQueryParamValue(text, false); placeholder_to_data[kUrlPlaceholder] = net::EscapeQueryParamValue(share_url.spec(), false); Then should be able to delete all the stuff above. Failing that, I would rather those vectors be a vector of pairs rather than two separate vectors. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:53: // open, return an error code. I think we could change this rule to *only* allow identifier characters (ASCII letters, numbers and maybe '-' and/or '_') inside these braces. There's no future in which we want to allow all kinds of random characters in there. That might simplify this algorithm a bit. Instead of explicitly erroring if we see a "%{", we error if we see any non-identifier character at all (essentially |last_saw_open| changes to a different mode where we are only looking for identifiers or '}'). https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:54: std::vector<int> placeholder_open_indices; Can we make this a std::vector<std::pair<int, int>> instead of two separate vectors? I'm not really a fan of having two vectors that *must* be the same length and making an assumption about that. It would make the code below a bit neater. Failing that, add a DCHECK below when you loop over these vectors to check that they are the same length. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:58: if (url_template[i] == '%' && i < url_template.size() && i + 1 < url_template.size() ^^^ https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:61: *url_template_filled = url_template; Why populate |url_template_filled| at all on error? Conventionally you just leave outputs unchanged if there is an error. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:62: return 1; Please comment each error case why it is an error. e.g.: // Nested template parameters are not allowed. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:67: if (!last_saw_open) { We could just not make this an error. It isn't necessarily bad to have a '}' character outside of a template parameter sequence. It would simplify the algorithm and your description of it if we just don't make this an error. On the flip side, '}' is an illegal character in URL syntax, so it should never appear outside of the template parameters. However, it isn't our job to police illegal characters (or else we should error on '<' and '>' too, for example). So I'd say just remove this rule. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:77: return 1; Comment why this is an error. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:80: // Reserve memory for filled template, assuming all placeholders are replaced This function is getting pretty big. Maybe break off a separate function to calculate the size of the allocation, and maybe break some other things out into functions. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:82: int placeholder_char_count = 0; I would suggest not to worry about this. You don't need to allocate the exact amount of memory required. Subtracting the typically ~20 bytes of placeholder characters isn't really worth this complexity. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:87: size_t largest_share_datum = 0; Comment here to explain why you're doing this. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:98: // Look at each placeholder, and check if it is valid. What does "valid" mean? You mean one of the recognised strings? Maybe choose a different word. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:142: error_occured |= error; No point having error_occurred if it only gets set once (this |= pattern might be useful if you have a number of things that can go wrong but for now it is unnecessary). If you change the result of ReplacePlaceholders to bool (as I suggested above) this will be easier -- just declare and set error_occurred in this one line. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.h (right): https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:37: // Writes to |url_template_filled|, a copy of |url_template with all instances nit: "|url_template" needs a close '|'. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:38: // of "%{title}", "%{text}", and "%{url}" (quotes for clarity) replaced with Drop "(quotes for clarity)". https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:40: // Replaces instances of "%{other}" where other is any string besides title, Quote "other", "title", "text" and "url". Also maybe replace "other" with "X". https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:41: // text, and url, with and empty string, for backwards compatibility. with *an* empty string. For *forwards* compatibility (this is done so we don't break if we add a new one in the future, not to support a legacy thing that's been removed). https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:46: static int ReplacePlaceholders(const std::string& url_template, Can this return a bool, not an int? https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:47: const std::string& title, What happened to StringPiece? https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl_unittest.cc (right): https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:85: const GURL url(kUrlSpec); Can this be constexpr? (Not sure... I'm just picking up Sam's suggestions; this is new to me.) https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:100: TEST_F(ShareServiceImplUnittest, ReplacePlaceholders) { Excellent test, very thorough. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:101: const GURL url(kUrlSpec); As above: constexpr? https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:179: // TODO(constantina): come back to this and think about returned value Do this before submitting. (Note: If you have a TODO that you want to do before submitting, as opposed to one you intend to land into the code for awhile, you should include the text "DO NOT SUBMIT" which blocks the CQ.) https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:180: // Badly nested placeholders. Add a test for a placeholder with other characters, e.g., "%{abc ? def}". In my opinion, this should be an error (see above). https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:254: // Unicode chars Nit: "." https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:256: error = ShareServiceImpl::ReplacePlaceholders(url_template, "☻", kText, url, "\xe2\x98\xbb" instead of "☻". Also test a Latin-1 character [i.e., a character in range U+0080 to U+00FF], like U+00E9 ("\xc3\xa9") which should encode to %C3%A9. Also test an Astral character [i.e., a character in range U+10000 to U+10FFFF], like U+1F4A9 ("\xf0\x9f\x92\xa9") which should encode to "%F0%9F%92%A9". (Yes this is the poop emoji.) I like to test these three categories since they can exhibit weird edge cases.
https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:36: std::string title_placeholder = "%{title}"; On 2017/01/05 03:09:24, Matt Giuca wrote: > On 2016/12/22 07:03:50, Sam McNally wrote: > > On 2016/12/21 07:13:49, Matt Giuca (OOO til Jan 5) wrote: > > > Nit: kTitlePlaceholder, kTextPlaceholder and kUrlPlaceholder. > > > > > > (Even when local variables, still follow this convention.) > > > > > > Also you should not have const std::strings; instead the convention is: > > > > > > const char kTitlePlaceholder[] = "%{title}"; > > > > > > Rationale is that this can be allocated into static memory rather than being > > > dynamically created every time the function runs. > > > > Better yet, use constexpr char kTitlePlaceholder[]. > > Sam: Is your advice just to always use constexpr in place of const if the RHS is > a compile-time constant? Yes, if constexpr is supported for the type. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:54: std::vector<int> placeholder_open_indices; On 2017/01/05 03:09:25, Matt Giuca wrote: > Can we make this a std::vector<std::pair<int, int>> instead of two separate > vectors? > > I'm not really a fan of having two vectors that *must* be the same length and > making an assumption about that. It would make the code below a bit neater. > > Failing that, add a DCHECK below when you loop over these vectors to check that > they are the same length. std::vector<base::StringPiece> is a nice way to model string segments within a larger string. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:61: *url_template_filled = url_template; Don't change |*url_template_filled| on failure. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:80: // Reserve memory for filled template, assuming all placeholders are replaced Use the real sizes. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:93: *url_template_filled = ""; ->clear(); https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:109: base::StringPiece placeholder = url_template.substr( This is dangling unless |url_template| is a StringPiece. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:136: std::string url_template = constexpr char kUrlTemplate[] = https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:145: OpenTargetURL(target_url); Why open the URL if substitution fails? https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl_unittest.cc (right): https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:85: const GURL url(kUrlSpec); On 2017/01/05 03:09:26, Matt Giuca wrote: > Can this be constexpr? (Not sure... I'm just picking up Sam's suggestions; this > is new to me.) No. An object can only be constexpr if the class has a constexpr constructor. Containers generally don't as they need to allocate storage. E.g., base::TimeDelta does so you can make constants like constexpr base::TimeDelta kOneSecond = base::TimeDelta::FromSeconds(1);
https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:54: std::vector<int> placeholder_open_indices; On 2017/01/05 04:08:37, Sam McNally wrote: > On 2017/01/05 03:09:25, Matt Giuca wrote: > > Can we make this a std::vector<std::pair<int, int>> instead of two separate > > vectors? > > > > I'm not really a fan of having two vectors that *must* be the same length and > > making an assumption about that. It would make the code below a bit neater. > > > > Failing that, add a DCHECK below when you loop over these vectors to check > that > > they are the same length. > > std::vector<base::StringPiece> is a nice way to model string segments within a > larger string. I agree, that sounds good, as long as you can figure out which indices of the original string those pieces point to. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:80: // Reserve memory for filled template, assuming all placeholders are replaced On 2017/01/05 04:08:37, Sam McNally wrote: > Use the real sizes. Do you mean actually calculate the final size before building the string (rather than just doing the max as is done here)? It seems like more work than it's worth?
Please change the title of the CL to match the first line of the description.
https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:80: // Reserve memory for filled template, assuming all placeholders are replaced On 2017/01/05 05:00:13, Matt Giuca wrote: > On 2017/01/05 04:08:37, Sam McNally wrote: > > Use the real sizes. > > Do you mean actually calculate the final size before building the string (rather > than just doing the max as is done here)? It seems like more work than it's > worth? Yes. The mapping from placeholder to value can be done in the initial pass through the template URL.
Patchset #16 (id:300001) has been deleted
https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:36: constexpr char kTitlePlaceholder[] = "%{title}"; On 2017/01/05 03:09:25, Matt Giuca wrote: > It would be much nicer I think if we could avoid having the "%{" and "}" here > (and in the map). Can we just make it so these constants are "title" rather than > "%{title}"? Done. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:47: for (size_t i = 0; i < placeholders.size(); ++i) { On 2017/01/05 03:09:25, Matt Giuca wrote: > I think this would read much clearer if it was just explicit rather than > building all these data structures first: > > placeholder_to_data[kTitlePlaceholder] = net::EscapeQueryParamValue(title, > false); > placeholder_to_data[kTextPlaceholder] = net::EscapeQueryParamValue(text, false); > placeholder_to_data[kUrlPlaceholder] = > net::EscapeQueryParamValue(share_url.spec(), false); > > Then should be able to delete all the stuff above. > > Failing that, I would rather those vectors be a vector of pairs rather than two > separate vectors. Done. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:53: // open, return an error code. On 2017/01/05 03:09:25, Matt Giuca wrote: > I think we could change this rule to *only* allow identifier characters (ASCII > letters, numbers and maybe '-' and/or '_') inside these braces. There's no > future in which we want to allow all kinds of random characters in there. > > That might simplify this algorithm a bit. Instead of explicitly erroring if we > see a "%{", we error if we see any non-identifier character at all (essentially > |last_saw_open| changes to a different mode where we are only looking for > identifiers or '}'). Done. Not sure if this is the way you wanted it implemented. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:54: std::vector<int> placeholder_open_indices; On 2017/01/05 05:00:13, Matt Giuca wrote: > On 2017/01/05 04:08:37, Sam McNally wrote: > > On 2017/01/05 03:09:25, Matt Giuca wrote: > > > Can we make this a std::vector<std::pair<int, int>> instead of two separate > > > vectors? > > > > > > I'm not really a fan of having two vectors that *must* be the same length > and > > > making an assumption about that. It would make the code below a bit neater. > > > > > > Failing that, add a DCHECK below when you loop over these vectors to check > > that > > > they are the same length. > > > > std::vector<base::StringPiece> is a nice way to model string segments within a > > larger string. > > I agree, that sounds good, as long as you can figure out which indices of the > original string those pieces point to. Reimplemented algo to allow for this. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:58: if (url_template[i] == '%' && i < url_template.size() && On 2017/01/05 03:09:25, Matt Giuca wrote: > i + 1 < url_template.size() > ^^^ Done. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:61: *url_template_filled = url_template; On 2017/01/05 03:09:25, Matt Giuca wrote: > Why populate |url_template_filled| at all on error? Conventionally you just > leave outputs unchanged if there is an error. Done. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:62: return 1; On 2017/01/05 03:09:25, Matt Giuca wrote: > Please comment each error case why it is an error. > > e.g.: > // Nested template parameters are not allowed. Done. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:67: if (!last_saw_open) { On 2017/01/05 03:09:25, Matt Giuca wrote: > We could just not make this an error. It isn't necessarily bad to have a '}' > character outside of a template parameter sequence. It would simplify the > algorithm and your description of it if we just don't make this an error. > > On the flip side, '}' is an illegal character in URL syntax, so it should never > appear outside of the template parameters. However, it isn't our job to police > illegal characters (or else we should error on '<' and '>' too, for example). So > I'd say just remove this rule. Done. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:77: return 1; On 2017/01/05 03:09:25, Matt Giuca wrote: > Comment why this is an error. Done. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:80: // Reserve memory for filled template, assuming all placeholders are replaced On 2017/01/05 05:41:49, Sam McNally wrote: > On 2017/01/05 05:00:13, Matt Giuca wrote: > > On 2017/01/05 04:08:37, Sam McNally wrote: > > > Use the real sizes. > > > > Do you mean actually calculate the final size before building the string > (rather > > than just doing the max as is done here)? It seems like more work than it's > > worth? > > Yes. The mapping from placeholder to value can be done in the initial pass > through the template URL. Incorporated as part of the new implementation. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:82: int placeholder_char_count = 0; On 2017/01/05 03:09:25, Matt Giuca wrote: > I would suggest not to worry about this. You don't need to allocate the exact > amount of memory required. Subtracting the typically ~20 bytes of placeholder > characters isn't really worth this complexity. New implementation means this is easily accounted for, with no added code complexity. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:87: size_t largest_share_datum = 0; On 2017/01/05 03:09:25, Matt Giuca wrote: > Comment here to explain why you're doing this. Gone with new impl. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:93: *url_template_filled = ""; On 2017/01/05 04:08:37, Sam McNally wrote: > ->clear(); Done. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:98: // Look at each placeholder, and check if it is valid. On 2017/01/05 03:09:25, Matt Giuca wrote: > What does "valid" mean? You mean one of the recognised strings? Maybe choose a > different word. Gone. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:109: base::StringPiece placeholder = url_template.substr( On 2017/01/05 04:08:37, Sam McNally wrote: > This is dangling unless |url_template| is a StringPiece. Gone. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:136: std::string url_template = On 2017/01/05 04:08:37, Sam McNally wrote: > constexpr char kUrlTemplate[] = Done. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:142: error_occured |= error; On 2017/01/05 03:09:25, Matt Giuca wrote: > No point having error_occurred if it only gets set once (this |= pattern might > be useful if you have a number of things that can go wrong but for now it is > unnecessary). > > If you change the result of ReplacePlaceholders to bool (as I suggested above) > this will be easier -- just declare and set error_occurred in this one line. Done. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:145: OpenTargetURL(target_url); On 2017/01/05 04:08:37, Sam McNally wrote: > Why open the URL if substitution fails? Done. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.h (right): https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:37: // Writes to |url_template_filled|, a copy of |url_template with all instances On 2017/01/05 03:09:25, Matt Giuca wrote: > nit: "|url_template" needs a close '|'. Done. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:38: // of "%{title}", "%{text}", and "%{url}" (quotes for clarity) replaced with On 2017/01/05 03:09:25, Matt Giuca wrote: > Drop "(quotes for clarity)". Done. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:40: // Replaces instances of "%{other}" where other is any string besides title, On 2017/01/05 03:09:25, Matt Giuca wrote: > Quote "other", "title", "text" and "url". > > Also maybe replace "other" with "X". Done. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:41: // text, and url, with and empty string, for backwards compatibility. On 2017/01/05 03:09:26, Matt Giuca wrote: > with *an* empty string. > > For *forwards* compatibility (this is done so we don't break if we add a new one > in the future, not to support a legacy thing that's been removed). Done. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:46: static int ReplacePlaceholders(const std::string& url_template, On 2017/01/05 03:09:25, Matt Giuca wrote: > Can this return a bool, not an int? Done. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:47: const std::string& title, On 2017/01/05 03:09:26, Matt Giuca wrote: > What happened to StringPiece? Sam said to use std::string& if it gets passed directly to something that needs a string, which it does. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl_unittest.cc (right): https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:85: const GURL url(kUrlSpec); On 2017/01/05 04:08:37, Sam McNally wrote: > On 2017/01/05 03:09:26, Matt Giuca wrote: > > Can this be constexpr? (Not sure... I'm just picking up Sam's suggestions; > this > > is new to me.) > > No. An object can only be constexpr if the class has a constexpr constructor. > Containers generally don't as they need to allocate storage. E.g., > base::TimeDelta does so you can make constants like > constexpr base::TimeDelta kOneSecond = base::TimeDelta::FromSeconds(1); Acknowledged. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:100: TEST_F(ShareServiceImplUnittest, ReplacePlaceholders) { On 2017/01/05 03:09:26, Matt Giuca wrote: > Excellent test, very thorough. :) https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:101: const GURL url(kUrlSpec); On 2017/01/05 03:09:26, Matt Giuca wrote: > As above: constexpr? Acknowledged. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:179: // TODO(constantina): come back to this and think about returned value On 2017/01/05 03:09:26, Matt Giuca wrote: > Do this before submitting. > > (Note: If you have a TODO that you want to do before submitting, as opposed to > one you intend to land into the code for awhile, you should include the text "DO > NOT SUBMIT" which blocks the CQ.) Done. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:180: // Badly nested placeholders. On 2017/01/05 03:09:26, Matt Giuca wrote: > Add a test for a placeholder with other characters, e.g., "%{abc ? def}". > > In my opinion, this should be an error (see above). Done. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:254: // Unicode chars On 2017/01/05 03:09:26, Matt Giuca wrote: > Nit: "." Done. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:256: error = ShareServiceImpl::ReplacePlaceholders(url_template, "☻", kText, url, On 2017/01/05 03:09:26, Matt Giuca wrote: > "\xe2\x98\xbb" instead of "☻". > > Also test a Latin-1 character [i.e., a character in range U+0080 to U+00FF], > like U+00E9 ("\xc3\xa9") which should encode to %C3%A9. > > Also test an Astral character [i.e., a character in range U+10000 to U+10FFFF], > like U+1F4A9 ("\xf0\x9f\x92\xa9") which should encode to "%F0%9F%92%A9". (Yes > this is the poop emoji.) > > I like to test these three categories since they can exhibit weird edge cases. Done.
The CQ bit was checked by constantina@google.com 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...
https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.h (right): https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:47: const std::string& title, On 2017/01/09 02:12:43, constantina wrote: > On 2017/01/05 03:09:26, Matt Giuca wrote: > > What happened to StringPiece? > > Sam said to use std::string& if it gets passed directly to something that needs > a string, which it does. It doesn't anymore. https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:26: bool ShareServiceImpl::SplitTemplate( Make this a function in the anonymous namespace. https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:27: const std::string& url_template, StringPiece https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:28: std::vector<base::StringPiece>& split_template) { Use pointers for out parameters. https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:32: std::vector<int> delimeter_indices; std::vector<size_t> delimiter_indices; https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:35: if (url_template[i] == '%' && i + 1 < url_template.size() && How about url_template.substr(i, 2) == "%{"? https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:39: return true; When returning a bool, true usually means success. https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:54: url_template[i] != '_' && url_template[i] != '_') { We really like underscores? https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:68: split_template.push_back( (assuming |url_template| is a StringPiece) push_back(url_template.substr(start_index_to_copy, delimeter_indices[i] - start_index_to_copy)) https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:77: // Remove %{ and } from placeholders in |split_template|. I wonder if it might be simpler to store a before and after position for each delimiter instead. https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:79: base::StringPiece placeholder = split_template[i]; base::StringPiece& https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:80: split_template[i] = placeholder = placeholder.substr(2, placeholder.size() - 3) https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:83: return 0; Don't implicitly convert to bool. https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:107: return error; Be consistent with whether single line ifs have braces. https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:109: std::string empty_string = ""; Remove. https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:113: std::map<base::StringPiece, std::string>::iterator it = auto https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:116: split_template[i] = base::StringPiece(it->second); You shouldn't need an explicit cast here. https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:118: split_template[i] = empty_string; .clear(); https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:124: int filled_size = 0; size_t https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:131: for (const base::StringPiece piece : split_template) { const auto& piece https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:132: *url_template_filled += piece.as_string(); piece.AppendToString(url_template_filled); as_string() creates a copy. https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:160: GURL target_url(kUrlBase + url_template_filled); We should probably return an error if !target_url.is_valid(). https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl_unittest.cc (right): https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:109: EXPECT_EQ(false, error); EXPECT_FALSE https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:183: EXPECT_EQ(true, error); EXPECT_TRUE https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:199: Test for "%{"?
https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:136: std::string url_template = On 2017/01/09 02:12:42, constantina wrote: > On 2017/01/05 04:08:37, Sam McNally wrote: > > constexpr char kUrlTemplate[] = > > Done. You still need to rename to kUrlTemplate. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.h (right): https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:47: const std::string& title, On 2017/01/09 02:12:43, constantina wrote: > On 2017/01/05 03:09:26, Matt Giuca wrote: > > What happened to StringPiece? > > Sam said to use std::string& if it gets passed directly to something that needs > a string, which it does. None of these do though. url_template gets inspected by SplitTemplate, which never stores it in a string, so it could take a StringPiece and never copy the string. title and text get passed to net::EscapeQueryParamValue which takes a StringPiece. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl_unittest.cc (right): https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:256: error = ShareServiceImpl::ReplacePlaceholders(url_template, "☻", kText, url, On 2017/01/09 02:12:43, constantina wrote: > On 2017/01/05 03:09:26, Matt Giuca wrote: > > "\xe2\x98\xbb" instead of "☻". > > > > Also test a Latin-1 character [i.e., a character in range U+0080 to U+00FF], > > like U+00E9 ("\xc3\xa9") which should encode to %C3%A9. > > > > Also test an Astral character [i.e., a character in range U+10000 to > U+10FFFF], > > like U+1F4A9 ("\xf0\x9f\x92\xa9") which should encode to "%F0%9F%92%A9". (Yes > > this is the poop emoji.) > > > > I like to test these three categories since they can exhibit weird edge cases. > > Done. Just to make it clear, add a comment explaining which character is being tested: // U+263B // U+00E9 // U+1F4A9 https://codereview.chromium.org/2564483003/diff/360001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2564483003/diff/360001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:32: // open, return an error code. Drop "code" (since we're just returning a bool). https://codereview.chromium.org/2564483003/diff/360001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:33: std::vector<int> delimeter_indices; Spelling: delimiter, not delimeter. https://codereview.chromium.org/2564483003/diff/360001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:39: if (last_saw_open) { Note: You don't *have* to include the braces if you have a single line if statement with a single line body. You can. It's up to your personal preference, just letting you know in case you thought this was a rule. (My preference is to avoid it; you may ignore this if your preference differs, but be consistent.) https://codereview.chromium.org/2564483003/diff/360001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:54: if (!isalnum(url_template[i]) && url_template[i] != '_' && You repeated url_template[i] != '_' twice... did you mean '-' on the second one? I don't think you can use isalnum here: it's defined as "Checks if the given character is an alphanumeric character as classified *by the current C locale*" which means it will match different sets of characters depending on the user's system locale -- certainly not something we want to depend on here! I think you should just specify your own function for checking whether a character is an identifier character. https://codereview.chromium.org/2564483003/diff/360001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:70: base::StringPiece(&url_template[0] + start_index_to_copy, Replace "&url_template[0]" with "url_template.data()". https://codereview.chromium.org/2564483003/diff/360001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:78: // Remove %{ and } from placeholders in |split_template|. ..."from placeholders (odd-numbered elements)"... https://codereview.chromium.org/2564483003/diff/360001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:84: return 0; false https://codereview.chromium.org/2564483003/diff/360001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:123: // Precalculate size of |url_filled_template|. From here to the end of the function can be very neatly summarized as "join a std::vector<base::StringPiece> into a single std::string", and therefore is ripe for breaking off into a separate function. (In fact there is already a JoinString that does this in base/strings/string_util.h, but it doesn't take a vector of StringPiece so I think you are OK to just write your own local one here.) https://codereview.chromium.org/2564483003/diff/360001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:135: return 0; false. https://codereview.chromium.org/2564483003/diff/360001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.h (right): https://codereview.chromium.org/2564483003/diff/360001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:39: // between %{ and } in the |url_template|. Thus, if } was immediately followed nit: Add quotes around the literal bits to make it easier to read ("%{" and "}", etc). (You have done this elsewhere, it seems.) https://codereview.chromium.org/2564483003/diff/360001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:40: // by %{, an empty string is stored. Document the return conditions. https://codereview.chromium.org/2564483003/diff/360001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:41: static bool SplitTemplate(const std::string& url_template, Maybe a quick example to clarify: Input: "abc%{def}%{ghi}" Output: {"abc", "def", "", "ghi", ""}. Also is this always guaranteed to have an odd number of output strings? (i.e. if there is nothing before the first template, or nothing after the last template, will it generate an empty string?) If not, I think it should. Document this. https://codereview.chromium.org/2564483003/diff/360001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:41: static bool SplitTemplate(const std::string& url_template, This doesn't have to be a static method; it can just be in an unnamed namespace in the .cc file (without "static"). See https://google.github.io/styleguide/cppguide.html#Unnamed_Namespaces_and_Stat... (I was going to say the same thing about ReplacePlaceholders but I realised it needs to be called by the test.)
https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:77: // Remove %{ and } from placeholders in |split_template|. On 2017/01/09 03:15:21, Sam McNally wrote: > I wonder if it might be simpler to store a before and after position for each > delimiter instead. What do you mean exactly? Like separate index vars for the open and close? Connie and I just had a chat about reworking this algorithm. I suggested something a bit along these lines (separate code for finding the end delimiter to the start) but nothing we thought of significantly improved the complexity and I think I'm happy to leave it as-is. If you disagree, can you be more specific about what you want?
https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:136: std::string url_template = On 2017/01/09 03:56:54, Matt Giuca wrote: > On 2017/01/09 02:12:42, constantina wrote: > > On 2017/01/05 04:08:37, Sam McNally wrote: > > > constexpr char kUrlTemplate[] = > > > > Done. > > You still need to rename to kUrlTemplate. Done. https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.h (right): https://codereview.chromium.org/2564483003/diff/280001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:47: const std::string& title, On 2017/01/09 03:56:54, Matt Giuca wrote: > On 2017/01/09 02:12:43, constantina wrote: > > On 2017/01/05 03:09:26, Matt Giuca wrote: > > > What happened to StringPiece? > > > > Sam said to use std::string& if it gets passed directly to something that > needs > > a string, which it does. > > None of these do though. > > url_template gets inspected by SplitTemplate, which never stores it in a string, > so it could take a StringPiece and never copy the string. > > title and text get passed to net::EscapeQueryParamValue which takes a > StringPiece. Done. https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:26: bool ShareServiceImpl::SplitTemplate( On 2017/01/09 03:15:21, Sam McNally wrote: > Make this a function in the anonymous namespace. Done. https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:27: const std::string& url_template, On 2017/01/09 03:15:21, Sam McNally wrote: > StringPiece Done. https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:28: std::vector<base::StringPiece>& split_template) { On 2017/01/09 03:15:21, Sam McNally wrote: > Use pointers for out parameters. Done. https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:32: std::vector<int> delimeter_indices; On 2017/01/09 03:15:21, Sam McNally wrote: > std::vector<size_t> delimiter_indices; Done. https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:35: if (url_template[i] == '%' && i + 1 < url_template.size() && On 2017/01/09 03:15:21, Sam McNally wrote: > How about url_template.substr(i, 2) == "%{"? Done. https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:39: return true; On 2017/01/09 03:15:21, Sam McNally wrote: > When returning a bool, true usually means success. Swapped true and false. https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:54: url_template[i] != '_' && url_template[i] != '_') { On 2017/01/09 03:15:21, Sam McNally wrote: > We really like underscores? Fixed. We also love hyphens. https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:68: split_template.push_back( On 2017/01/09 03:15:21, Sam McNally wrote: > (assuming |url_template| is a StringPiece) > > push_back(url_template.substr(start_index_to_copy, delimeter_indices[i] - > start_index_to_copy)) Done. https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:77: // Remove %{ and } from placeholders in |split_template|. On 2017/01/09 04:50:14, Matt Giuca wrote: > On 2017/01/09 03:15:21, Sam McNally wrote: > > I wonder if it might be simpler to store a before and after position for each > > delimiter instead. > > What do you mean exactly? Like separate index vars for the open and close? > > Connie and I just had a chat about reworking this algorithm. I suggested > something a bit along these lines (separate code for finding the end delimiter > to the start) but nothing we thought of significantly improved the complexity > and I think I'm happy to leave it as-is. > > If you disagree, can you be more specific about what you want? As discussed in person, we will store the StringPieces, and do the replacements, in the first for loop simultaneously, instead of storing indices. Will do this in the next patch. https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:79: base::StringPiece placeholder = split_template[i]; On 2017/01/09 03:15:21, Sam McNally wrote: > base::StringPiece& Done. https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:80: split_template[i] = On 2017/01/09 03:15:21, Sam McNally wrote: > placeholder = placeholder.substr(2, placeholder.size() - 3) Done. https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:83: return 0; On 2017/01/09 03:15:21, Sam McNally wrote: > Don't implicitly convert to bool. Done. https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:107: return error; On 2017/01/09 03:15:21, Sam McNally wrote: > Be consistent with whether single line ifs have braces. Done. https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:109: std::string empty_string = ""; On 2017/01/09 03:15:21, Sam McNally wrote: > Remove. Done. https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:113: std::map<base::StringPiece, std::string>::iterator it = On 2017/01/09 03:15:21, Sam McNally wrote: > auto Done. https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:116: split_template[i] = base::StringPiece(it->second); On 2017/01/09 03:15:21, Sam McNally wrote: > You shouldn't need an explicit cast here. Done. https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:118: split_template[i] = empty_string; On 2017/01/09 03:15:21, Sam McNally wrote: > .clear(); Done. https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:124: int filled_size = 0; On 2017/01/09 03:15:21, Sam McNally wrote: > size_t Done. https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:131: for (const base::StringPiece piece : split_template) { On 2017/01/09 03:15:21, Sam McNally wrote: > const auto& piece Done. https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:132: *url_template_filled += piece.as_string(); On 2017/01/09 03:15:21, Sam McNally wrote: > piece.AppendToString(url_template_filled); > > as_string() creates a copy. Done. https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:160: GURL target_url(kUrlBase + url_template_filled); On 2017/01/09 03:15:21, Sam McNally wrote: > We should probably return an error if !target_url.is_valid(). Done. https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl_unittest.cc (right): https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:109: EXPECT_EQ(false, error); On 2017/01/09 03:15:22, Sam McNally wrote: > EXPECT_FALSE Done. https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:183: EXPECT_EQ(true, error); On 2017/01/09 03:15:22, Sam McNally wrote: > EXPECT_TRUE Done. https://codereview.chromium.org/2564483003/diff/340001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:199: On 2017/01/09 03:15:22, Sam McNally wrote: > Test for "%{"? Done. https://codereview.chromium.org/2564483003/diff/360001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2564483003/diff/360001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:32: // open, return an error code. On 2017/01/09 03:56:54, Matt Giuca wrote: > Drop "code" (since we're just returning a bool). Done. https://codereview.chromium.org/2564483003/diff/360001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:33: std::vector<int> delimeter_indices; On 2017/01/09 03:56:54, Matt Giuca wrote: > Spelling: delimiter, not delimeter. Done. https://codereview.chromium.org/2564483003/diff/360001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:39: if (last_saw_open) { On 2017/01/09 03:56:54, Matt Giuca wrote: > Note: You don't *have* to include the braces if you have a single line if > statement with a single line body. You can. > > It's up to your personal preference, just letting you know in case you thought > this was a rule. (My preference is to avoid it; you may ignore this if your > preference differs, but be consistent.) Done. https://codereview.chromium.org/2564483003/diff/360001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:54: if (!isalnum(url_template[i]) && url_template[i] != '_' && On 2017/01/09 03:56:54, Matt Giuca wrote: > You repeated url_template[i] != '_' twice... did you mean '-' on the second one? > > I don't think you can use isalnum here: it's defined as "Checks if the given > character is an alphanumeric character as classified *by the current C locale*" > which means it will match different sets of characters depending on the user's > system locale -- certainly not something we want to depend on here! > > I think you should just specify your own function for checking whether a > character is an identifier character. Done. https://codereview.chromium.org/2564483003/diff/360001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:70: base::StringPiece(&url_template[0] + start_index_to_copy, On 2017/01/09 03:56:54, Matt Giuca wrote: > Replace "&url_template[0]" with "url_template.data()". Done. https://codereview.chromium.org/2564483003/diff/360001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:78: // Remove %{ and } from placeholders in |split_template|. On 2017/01/09 03:56:54, Matt Giuca wrote: > ..."from placeholders (odd-numbered elements)"... Done. Used odd-indexed https://codereview.chromium.org/2564483003/diff/360001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:84: return 0; On 2017/01/09 03:56:54, Matt Giuca wrote: > false Done. https://codereview.chromium.org/2564483003/diff/360001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:123: // Precalculate size of |url_filled_template|. On 2017/01/09 03:56:54, Matt Giuca wrote: > From here to the end of the function can be very neatly summarized as "join a > std::vector<base::StringPiece> into a single std::string", and therefore is ripe > for breaking off into a separate function. > > (In fact there is already a JoinString that does this in > base/strings/string_util.h, but it doesn't take a vector of StringPiece so I > think you are OK to just write your own local one here.) Done. https://codereview.chromium.org/2564483003/diff/360001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:135: return 0; On 2017/01/09 03:56:54, Matt Giuca wrote: > false. Done. https://codereview.chromium.org/2564483003/diff/360001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.h (right): https://codereview.chromium.org/2564483003/diff/360001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:39: // between %{ and } in the |url_template|. Thus, if } was immediately followed On 2017/01/09 03:56:54, Matt Giuca wrote: > nit: Add quotes around the literal bits to make it easier to read ("%{" and "}", > etc). (You have done this elsewhere, it seems.) Done. https://codereview.chromium.org/2564483003/diff/360001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:40: // by %{, an empty string is stored. On 2017/01/09 03:56:54, Matt Giuca wrote: > Document the return conditions. Done. https://codereview.chromium.org/2564483003/diff/360001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:41: static bool SplitTemplate(const std::string& url_template, On 2017/01/09 03:56:54, Matt Giuca wrote: > This doesn't have to be a static method; it can just be in an unnamed namespace > in the .cc file (without "static"). > > See > https://google.github.io/styleguide/cppguide.html#Unnamed_Namespaces_and_Stat... > > (I was going to say the same thing about ReplacePlaceholders but I realised it > needs to be called by the test.) Done. https://codereview.chromium.org/2564483003/diff/360001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:41: static bool SplitTemplate(const std::string& url_template, On 2017/01/09 03:56:54, Matt Giuca wrote: > Maybe a quick example to clarify: > > Input: "abc%{def}%{ghi}" > Output: {"abc", "def", "", "ghi", ""}. > > Also is this always guaranteed to have an odd number of output strings? (i.e. if > there is nothing before the first template, or nothing after the last template, > will it generate an empty string?) If not, I think it should. Document this. Done.
https://codereview.chromium.org/2564483003/diff/420001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2564483003/diff/420001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:21: return ((c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || return base::IsAsciiAlpha(c) || base::IsAsciiDigit(c); (from base/strings/string_util.h) https://codereview.chromium.org/2564483003/diff/420001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:37: bool SplitTemplate(const base::StringPiece& url_template, Take StringPiece by value. https://codereview.chromium.org/2564483003/diff/420001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:71: int start_index_to_copy = 0; size_t https://codereview.chromium.org/2564483003/diff/420001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:72: for (size_t i = 0; i < delimiter_indices.size(); ++i) { for (auto delimiter_index : delimiter_indices) { https://codereview.chromium.org/2564483003/diff/420001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:91: std::string joined_pieces; Declare this closer to where it's used. https://codereview.chromium.org/2564483003/diff/420001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:94: for (const base::StringPiece piece : pieces) { const auto& https://codereview.chromium.org/2564483003/diff/420001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:131: bool succeeded = SplitTemplate(url_template, &split_template); if (!SplitTemplate(...)) { return false; } https://codereview.chromium.org/2564483003/diff/420001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:161: const char kUrlBase[] = "https://wicg.github.io/web-share-target/"; constexpr https://codereview.chromium.org/2564483003/diff/420001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:166: bool succeeded = ReplacePlaceholders(kUrlTemplate, title, text, share_url, if (!ReplacePlaceholders(...)) { https://codereview.chromium.org/2564483003/diff/420001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.h (right): https://codereview.chromium.org/2564483003/diff/420001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:37: // static bool SplitTemplate(const std::string& url_template, Remove. https://codereview.chromium.org/2564483003/diff/420001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:48: static bool ReplacePlaceholders(const std::string& url_template, StringPiece.
https://codereview.chromium.org/2564483003/diff/420001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2564483003/diff/420001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:21: return ((c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || On 2017/01/10 06:53:50, Sam McNally wrote: > return base::IsAsciiAlpha(c) || base::IsAsciiDigit(c); > > (from base/strings/string_util.h) Done. https://codereview.chromium.org/2564483003/diff/420001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:37: bool SplitTemplate(const base::StringPiece& url_template, On 2017/01/10 06:53:50, Sam McNally wrote: > Take StringPiece by value. func is gone https://codereview.chromium.org/2564483003/diff/420001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:71: int start_index_to_copy = 0; On 2017/01/10 06:53:50, Sam McNally wrote: > size_t Gone. https://codereview.chromium.org/2564483003/diff/420001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:72: for (size_t i = 0; i < delimiter_indices.size(); ++i) { On 2017/01/10 06:53:50, Sam McNally wrote: > for (auto delimiter_index : delimiter_indices) { Gone. https://codereview.chromium.org/2564483003/diff/420001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:91: std::string joined_pieces; On 2017/01/10 06:53:50, Sam McNally wrote: > Declare this closer to where it's used. Done. https://codereview.chromium.org/2564483003/diff/420001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:94: for (const base::StringPiece piece : pieces) { On 2017/01/10 06:53:50, Sam McNally wrote: > const auto& Done. https://codereview.chromium.org/2564483003/diff/420001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:131: bool succeeded = SplitTemplate(url_template, &split_template); On 2017/01/10 06:53:50, Sam McNally wrote: > if (!SplitTemplate(...)) { > return false; > } Gone. https://codereview.chromium.org/2564483003/diff/420001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:161: const char kUrlBase[] = "https://wicg.github.io/web-share-target/"; On 2017/01/10 06:53:50, Sam McNally wrote: > constexpr Done. https://codereview.chromium.org/2564483003/diff/420001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:166: bool succeeded = ReplacePlaceholders(kUrlTemplate, title, text, share_url, On 2017/01/10 06:53:50, Sam McNally wrote: > if (!ReplacePlaceholders(...)) { Done. https://codereview.chromium.org/2564483003/diff/420001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.h (right): https://codereview.chromium.org/2564483003/diff/420001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:37: // static bool SplitTemplate(const std::string& url_template, On 2017/01/10 06:53:50, Sam McNally wrote: > Remove. Done. https://codereview.chromium.org/2564483003/diff/420001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.h:48: static bool ReplacePlaceholders(const std::string& url_template, On 2017/01/10 06:53:50, Sam McNally wrote: > StringPiece. Done.
lgtm https://codereview.chromium.org/2564483003/diff/460001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2564483003/diff/460001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:21: bool IsIndentifier(char c) { Indentifier? https://codereview.chromium.org/2564483003/diff/460001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:67: int start_index_to_copy = 0; size_t https://codereview.chromium.org/2564483003/diff/460001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:138: callback.Run(base::Optional<std::string>()); base::nullopt
Looks good, I have a few nits mostly which are documentation. https://codereview.chromium.org/2564483003/diff/480001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2564483003/diff/480001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:20: namespace { nit: Blank line after this, and before the }. https://codereview.chromium.org/2564483003/diff/480001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:21: bool IsIdentifier(char c) { // Determines whether a character is allowed in a URL template placeholder. (It's obvious from the one line of code *what* this function does, but not obvious what it means, so it's helpful to document that. Otherwise, e.g., someone might later decide to de-duplicate it with some other "IsIdentifier" logic, but we need our implementation to match a very particular set of characters which will be mandated by the spec.) https://codereview.chromium.org/2564483003/diff/480001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:75: split_template.push_back(it->second); // Replace the placeholder text with the parameter value. https://codereview.chromium.org/2564483003/diff/480001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:84: } else if (!last_saw_open) { Just else (no need for inverted condition). https://codereview.chromium.org/2564483003/diff/480001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:118: constexpr char kUrlBase[] = "https://wicg.github.io/web-share-target/"; Add a TODO(constantina) to replace this hard-coded URL with one from a user-chosen site's manifest. This looks pretty weird without explanation. https://codereview.chromium.org/2564483003/diff/480001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl_unittest.cc (right): https://codereview.chromium.org/2564483003/diff/480001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:279: // Unicode chars. I think you missed my previous comment: Just to make it clear, add a comment explaining which character is being tested: // U+263B // U+00E9 // U+1F4A9
https://codereview.chromium.org/2564483003/diff/460001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2564483003/diff/460001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:21: bool IsIndentifier(char c) { On 2017/01/12 04:56:12, Sam McNally wrote: > Indentifier? *Identifier. Fixed. https://codereview.chromium.org/2564483003/diff/460001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:67: int start_index_to_copy = 0; On 2017/01/12 04:56:12, Sam McNally wrote: > size_t Done. https://codereview.chromium.org/2564483003/diff/460001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:138: callback.Run(base::Optional<std::string>()); On 2017/01/12 04:56:12, Sam McNally wrote: > base::nullopt Done. https://codereview.chromium.org/2564483003/diff/480001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2564483003/diff/480001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:20: namespace { On 2017/01/12 05:51:38, Matt Giuca wrote: > nit: Blank line after this, and before the }. Done. https://codereview.chromium.org/2564483003/diff/480001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:21: bool IsIdentifier(char c) { On 2017/01/12 05:51:38, Matt Giuca wrote: > // Determines whether a character is allowed in a URL template placeholder. > > (It's obvious from the one line of code *what* this function does, but not > obvious what it means, so it's helpful to document that. Otherwise, e.g., > someone might later decide to de-duplicate it with some other "IsIdentifier" > logic, but we need our implementation to match a very particular set of > characters which will be mandated by the spec.) Done. https://codereview.chromium.org/2564483003/diff/480001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:75: split_template.push_back(it->second); On 2017/01/12 05:51:38, Matt Giuca wrote: > // Replace the placeholder text with the parameter value. Done. https://codereview.chromium.org/2564483003/diff/480001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:84: } else if (!last_saw_open) { On 2017/01/12 05:51:38, Matt Giuca wrote: > Just else (no need for inverted condition). Done. https://codereview.chromium.org/2564483003/diff/480001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:118: constexpr char kUrlBase[] = "https://wicg.github.io/web-share-target/"; On 2017/01/12 05:51:38, Matt Giuca wrote: > Add a TODO(constantina) to replace this hard-coded URL with one from a > user-chosen site's manifest. > > This looks pretty weird without explanation. Done. https://codereview.chromium.org/2564483003/diff/480001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl_unittest.cc (right): https://codereview.chromium.org/2564483003/diff/480001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl_unittest.cc:279: // Unicode chars. On 2017/01/12 05:51:38, Matt Giuca wrote: > I think you missed my previous comment: > > Just to make it clear, add a comment explaining which character is being tested: > > // U+263B > // U+00E9 > // U+1F4A9 Done.
lgtm!
:) kenrb, could you take a look for IPC security?
mojo lgtm, with an unrelated nit. Thanks for requesting security review for changes to the mojo interface. It isn't strictly required based on the ownership rules but it helps us monitor for risk in the IPC system. https://codereview.chromium.org/2564483003/diff/500001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2564483003/diff/500001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:28: std::string JoinString(const std::vector<base::StringPiece>& pieces) { Is there a reason you are implementing this rather than just calling base::JoinString? If there is a reason why the function in base isn't what you need, it seems to warrant a comment explaining why.
https://codereview.chromium.org/2564483003/diff/500001/chrome/browser/webshar... File chrome/browser/webshare/share_service_impl.cc (right): https://codereview.chromium.org/2564483003/diff/500001/chrome/browser/webshar... chrome/browser/webshare/share_service_impl.cc:28: std::string JoinString(const std::vector<base::StringPiece>& pieces) { On 2017/01/12 14:53:37, kenrb wrote: > Is there a reason you are implementing this rather than just calling > base::JoinString? If there is a reason why the function in base isn't what you > need, it seems to warrant a comment explaining why. It is simply because base::JoinString doesn't take a vector of StringPieces, only a vector of Strings. I added a TODO to implement this in base.
The CQ bit was checked by constantina@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from sammc@chromium.org, kenrb@chromium.org, mgiuca@chromium.org Link to the patchset: https://codereview.chromium.org/2564483003/#ps540001 (title: "Rebase")
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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by constantina@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org, sammc@chromium.org, mgiuca@chromium.org Link to the patchset: https://codereview.chromium.org/2564483003/#ps560001 (title: "Prevent code running on android.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by constantina@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org, sammc@chromium.org, mgiuca@chromium.org Link to the patchset: https://codereview.chromium.org/2564483003/#ps600001 (title: "don't define browser in android again")
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": 600001, "attempt_start_ts": 1484286960627570,
"parent_rev": "883980129c6938e83dca43abb9f1693d2ed77274", "commit_rev":
"2cfa55e24997bd05c573b163a630f1dac13cd184"}
Message was sent while issue was closed.
Description was changed from ========== Default share to Share Target: Partial impl. of Web Share for Desktop. Currently only shares to a demo Share Target App hosted at https://wicg.github.io/web-share-target/demos/sharetarget.html; follow-up CL will have more share options. BUG=668389 ========== to ========== Default share to Share Target: Partial impl. of Web Share for Desktop. Currently only shares to a demo Share Target App hosted at https://wicg.github.io/web-share-target/demos/sharetarget.html; follow-up CL will have more share options. BUG=668389 Review-Url: https://codereview.chromium.org/2564483003 Cr-Commit-Position: refs/heads/master@{#443510} Committed: https://chromium.googlesource.com/chromium/src/+/2cfa55e24997bd05c573b163a630... ==========
Message was sent while issue was closed.
Committed patchset #30 (id:600001) as https://chromium.googlesource.com/chromium/src/+/2cfa55e24997bd05c573b163a630...
Message was sent while issue was closed.
High five! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
