Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1959933002:
Add FormatOriginForSecurityDisplay which takes a const url::Origin& origin (Closed)
DescriptionAdd FormatOriginForSecurityDisplay which takes a const url::Origin& origin
There is a FormatUrlForSecurityDisplay which takes a const GURL&.
It is better to add a FormatOriginForSecurityDisplay which takes a
const url::Origin&.
BUG=610147
Committed: https://crrev.com/cb63cac018227ec356e3d58e3c78e26761e76f26
Cr-Commit-Position: refs/heads/master@{#393410}
Patch Set 1 : added FormatUrlForSecurityDisplay which takes a const url::Origin #
Total comments: 4
Patch Set 2 : added test code #
Total comments: 7
Patch Set 3 : address brettw@'s comments #
Messages
Total messages: 21 (8 generated)
juncai@chromium.org changed reviewers: + palmer@chromium.org
Please take a look. This is the first try, and I want to make sure that I got the basic idea correct. If so, I will add test code at elide_url_unittest.cc and add more comments.
https://codereview.chromium.org/1959933002/diff/1/components/url_formatter/el... File components/url_formatter/elide_url.cc (right): https://codereview.chromium.org/1959933002/diff/1/components/url_formatter/el... components/url_formatter/elide_url.cc:130: bool ShouldShowScheme(const url::Origin& origin, Rather than (essentially) duplicate this code, how about instead refactoring |ShouldShowScheme|: bool ShouldShowScheme(const std::string& scheme, const url_formatter::SchemeDisplay scheme_display) and then updating the call sites for both GURLs and Origins? https://codereview.chromium.org/1959933002/diff/1/components/url_formatter/el... File components/url_formatter/elide_url.h (right): https://codereview.chromium.org/1959933002/diff/1/components/url_formatter/el... components/url_formatter/elide_url.h:85: base::string16 FormatUrlForSecurityDisplay( Naming it |FormatOriginForSecurityDisplay| might be more clear. Up to you, though.
https://codereview.chromium.org/1959933002/diff/1/components/url_formatter/el... File components/url_formatter/elide_url.cc (right): https://codereview.chromium.org/1959933002/diff/1/components/url_formatter/el... components/url_formatter/elide_url.cc:130: bool ShouldShowScheme(const url::Origin& origin, On 2016/05/09 19:40:48, palmer wrote: > Rather than (essentially) duplicate this code, how about instead refactoring > |ShouldShowScheme|: > > bool ShouldShowScheme(const std::string& scheme, > const url_formatter::SchemeDisplay scheme_display) > > and then updating the call sites for both GURLs and Origins? Done. https://codereview.chromium.org/1959933002/diff/1/components/url_formatter/el... File components/url_formatter/elide_url.h (right): https://codereview.chromium.org/1959933002/diff/1/components/url_formatter/el... components/url_formatter/elide_url.h:85: base::string16 FormatUrlForSecurityDisplay( On 2016/05/09 19:40:48, palmer wrote: > Naming it |FormatOriginForSecurityDisplay| might be more clear. Up to you, > though. Done.
lgtm
The CQ bit was checked by juncai@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1959933002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1959933002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
juncai@chromium.org changed reviewers: + brettw@chromium.org
brettw@chromium.org: Please take a look at //components/url_formatter/elide_url_unittest.cc
lgtm https://codereview.chromium.org/1959933002/diff/20001/components/url_formatte... File components/url_formatter/elide_url.cc (right): https://codereview.chromium.org/1959933002/diff/20001/components/url_formatte... components/url_formatter/elide_url.cc:359: const std::string& scheme = origin.scheme(); While we're changing this, can you change these to be base::StringPiece scheme = origin.scheme_piece(); base::StringPiece host = origin.host_piece(); and change ShouldShowScheme to take a StringPiece argument? https://codereview.chromium.org/1959933002/diff/20001/components/url_formatte... components/url_formatter/elide_url.cc:371: result += colon + base::UTF8ToUTF16(origin.port()); Can you also change this to port_piece()? https://codereview.chromium.org/1959933002/diff/20001/components/url_formatte... components/url_formatter/elide_url.cc:379: const std::string& scheme = origin.scheme(); Same, StringPiece, scheme_piece(), and host_piece()
https://codereview.chromium.org/1959933002/diff/20001/components/url_formatte... File components/url_formatter/elide_url.cc (right): https://codereview.chromium.org/1959933002/diff/20001/components/url_formatte... components/url_formatter/elide_url.cc:359: const std::string& scheme = origin.scheme(); On 2016/05/12 16:47:24, brettw wrote: > While we're changing this, can you change these to be > base::StringPiece scheme = origin.scheme_piece(); > base::StringPiece host = origin.host_piece(); > and change ShouldShowScheme to take a StringPiece argument? Done. https://codereview.chromium.org/1959933002/diff/20001/components/url_formatte... components/url_formatter/elide_url.cc:371: result += colon + base::UTF8ToUTF16(origin.port()); On 2016/05/12 16:47:24, brettw wrote: > Can you also change this to port_piece()? Done. https://codereview.chromium.org/1959933002/diff/20001/components/url_formatte... components/url_formatter/elide_url.cc:379: const std::string& scheme = origin.scheme(); On 2016/05/12 16:47:24, brettw wrote: > Same, StringPiece, scheme_piece(), and host_piece() url::Origin doesn't have scheme_piece(), and host_piece() member functions. So I here just use: base::StringPiece scheme = origin.scheme(); base::StringPiece host = origin.host(); Do you think it is ok?
Still LGTM https://codereview.chromium.org/1959933002/diff/20001/components/url_formatte... File components/url_formatter/elide_url.cc (right): https://codereview.chromium.org/1959933002/diff/20001/components/url_formatte... components/url_formatter/elide_url.cc:379: const std::string& scheme = origin.scheme(); On 2016/05/12 17:29:06, juncai wrote: > On 2016/05/12 16:47:24, brettw wrote: > > Same, StringPiece, scheme_piece(), and host_piece() > > url::Origin doesn't have scheme_piece(), and host_piece() member functions. So I > here just use: > base::StringPiece scheme = origin.scheme(); > base::StringPiece host = origin.host(); > > Do you think it is ok? Oh I see, these are const refs. This part is fine then.
The CQ bit was checked by juncai@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org Link to the patchset: https://codereview.chromium.org/1959933002/#ps40001 (title: "address brettw@'s comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1959933002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1959933002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Add FormatUrlForSecurityDisplay which takes a const url::Origin& origin There is a FormatUrlForSecurityDisplay which takes a const GURL&. It is better to add a FormatUrlForSecurityDisplay which takes a const url::Origin&. BUG=610147 ========== to ========== Add FormatUrlForSecurityDisplay which takes a const url::Origin& origin There is a FormatUrlForSecurityDisplay which takes a const GURL&. It is better to add a FormatUrlForSecurityDisplay which takes a const url::Origin&. BUG=610147 Committed: https://crrev.com/cb63cac018227ec356e3d58e3c78e26761e76f26 Cr-Commit-Position: refs/heads/master@{#393410} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/cb63cac018227ec356e3d58e3c78e26761e76f26 Cr-Commit-Position: refs/heads/master@{#393410}
Message was sent while issue was closed.
Description was changed from ========== Add FormatUrlForSecurityDisplay which takes a const url::Origin& origin There is a FormatUrlForSecurityDisplay which takes a const GURL&. It is better to add a FormatUrlForSecurityDisplay which takes a const url::Origin&. BUG=610147 Committed: https://crrev.com/cb63cac018227ec356e3d58e3c78e26761e76f26 Cr-Commit-Position: refs/heads/master@{#393410} ========== to ========== Add FormatOriginForSecurityDisplay which takes a const url::Origin& origin There is a FormatUrlForSecurityDisplay which takes a const GURL&. It is better to add a FormatOriginForSecurityDisplay which takes a const url::Origin&. BUG=610147 Committed: https://crrev.com/cb63cac018227ec356e3d58e3c78e26761e76f26 Cr-Commit-Position: refs/heads/master@{#393410} ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
