Chromium Code Reviews
Help | Chromium Project | Sign in
(13)

Issue 2641003002: Show scheme in black and content in gray for data: protocol urls (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months ago by elawrence
Modified:
2 weeks, 5 days ago
CC:
chromium-reviews, jdonnelly+watch_chromium.org, tfarina, mac-reviews_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Show scheme in black and content in gray for data: protocol urls Data URIs can be used for spoofing the user because attacker controlled text appears near the front of the string, and thus a fake origin can be placed the URI such that the user believes it represents the true origin of the page. To mitigate this, we will render the data scheme in black and the rest of the data URI string in the deemphasized text color. BUG=533705 Review-Url: https://codereview.chromium.org/2641003002 Cr-Commit-Position: refs/heads/master@{#454699} Committed: https://chromium.googlesource.com/chromium/src/+/486bb18560662755558c628e39ed8b3a3de272a2

Patch Set 1 #

Patch Set 2 : Fix off-by-one #

Total comments: 3

Patch Set 3 : Address review feedback #

Total comments: 2

Patch Set 4 : Hoist deemphasis logic #

Total comments: 4

Patch Set 5 : Hoist yet more #

Patch Set 6 : Fixup DEPS #

Patch Set 7 : Rebase #

Patch Set 8 : Update iOS #

Total comments: 66

Patch Set 9 : Address reviewer feedback #

Patch Set 10 : Fix omnibox_edit_unittest #

Patch Set 11 : Only call UpdateSchemeStyle when a scheme is present. #

Total comments: 10

Patch Set 12 : Rebase #

Patch Set 13 : Address Nits #

Total comments: 2

Patch Set 14 : Move misplaced #include to mm files #

Unified diffs Side-by-side diffs Delta from patch set Stats (+384 lines, -167 lines) Patch
M chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +12 lines, -2 lines 0 comments Download
M chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm View 1 2 3 4 5 6 7 8 9 10 11 12 13 4 chunks +57 lines, -62 lines 0 comments Download
M chrome/browser/ui/views/omnibox/omnibox_view_views.h View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/ui/views/omnibox/omnibox_view_views.cc View 1 2 3 4 5 6 7 8 9 10 3 chunks +19 lines, -35 lines 0 comments Download
M chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 6 chunks +120 lines, -3 lines 0 comments Download
M components/omnibox/browser/BUILD.gn View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M components/omnibox/browser/DEPS View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M components/omnibox/browser/omnibox_edit_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +2 lines, -0 lines 0 comments Download
M components/omnibox/browser/omnibox_view.h View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +23 lines, -0 lines 0 comments Download
M components/omnibox/browser/omnibox_view.cc View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +61 lines, -0 lines 0 comments Download
M ios/chrome/browser/ui/omnibox/omnibox_view_ios.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 4 chunks +13 lines, -1 line 0 comments Download
M ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm View 1 2 3 4 5 6 7 8 9 10 11 12 13 5 chunks +67 lines, -64 lines 0 comments Download
M ui/gfx/range/BUILD.gn View 1 2 3 4 5 6 7 1 chunk +6 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 83 (50 generated)
Peter Kasting
Was this intended to be ready for review and just not sent?
1 month, 4 weeks ago (2017-01-23 09:29:15 UTC) #1
elawrence
On 2017/01/23 09:29:15, Peter Kasting wrote: > Was this intended to be ready for review ...
1 month, 4 weeks ago (2017-01-23 14:06:57 UTC) #2
elawrence
Please take a look? Design decided we should use the standard level of deemphasis. Mobile ...
1 month, 2 weeks ago (2017-02-03 22:37:55 UTC) #4
Peter Kasting
Commented only on views, but the Mac comments are similar. Since we seem to be ...
1 month, 2 weeks ago (2017-02-06 22:42:28 UTC) #5
elawrence
> Since we seem to be duplicating code on views and Mac, consider hoisting much ...
4 weeks, 1 day ago (2017-02-21 22:29:09 UTC) #11
Peter Kasting
On 2017/02/06 22:42:28, Peter Kasting (backlogged) wrote: > Since we seem to be duplicating code ...
4 weeks ago (2017-02-23 01:30:43 UTC) #12
elawrence
> Did you look into this suggestion? I'm really hoping to avoid the duplication > ...
3 weeks, 6 days ago (2017-02-23 18:41:42 UTC) #17
Peter Kasting
On 2017/02/23 18:41:42, elawrence wrote: > I suppose I could refactor EmphasizeURLComponents into a function ...
3 weeks, 6 days ago (2017-02-24 01:53:40 UTC) #19
elawrence
> https://codereview.chromium.org/2641003002/diff/100001/components/omnibox/browser/omnibox_view.cc#newcode195 > components/omnibox/browser/omnibox_view.cc:195: // complicated dependencies. > Urgh. > Can we not have extensions/common/ ...
3 weeks, 5 days ago (2017-02-24 19:35:58 UTC) #20
Peter Kasting
(If possible, please reply to review comments inline with the commenting tool rather than via ...
3 weeks, 5 days ago (2017-02-24 20:34:57 UTC) #21
elawrence
pkasting@chromium.org: I've hoisted all of the scheme analysis logic to the OmniboxView and left the ...
3 weeks, 1 day ago (2017-02-28 22:46:53 UTC) #43
Eugene But
rohitrao@, who owns omnibox would be a better reviewer. I can comment on this CL ...
3 weeks, 1 day ago (2017-02-28 23:04:38 UTC) #45
Peter Kasting
Cool, I think this is almost ready to go. Most of the remaining comments are ...
3 weeks, 1 day ago (2017-03-01 02:39:11 UTC) #48
Eugene But
https://codereview.chromium.org/2641003002/diff/370001/ios/chrome/browser/ui/omnibox/omnibox_view_ios.h File ios/chrome/browser/ui/omnibox/omnibox_view_ios.h (right): https://codereview.chromium.org/2641003002/diff/370001/ios/chrome/browser/ui/omnibox/omnibox_view_ios.h#newcode144 ios/chrome/browser/ui/omnibox/omnibox_view_ios.h:144: // Update colors in |attributing_display_string_|. On 2017/03/01 02:39:11, Peter ...
3 weeks, 1 day ago (2017-03-01 06:08:08 UTC) #49
Robert Sesek
https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h (right): https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h#newcode242 chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h:242: NSMutableAttributedString* attributing_display_string_; Who owns this? Either document as "Weak" ...
3 weeks ago (2017-03-01 18:29:45 UTC) #50
elawrence
Thanks for having a look! https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h (right): https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h#newcode187 chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h:187: // Update colors in ...
3 weeks ago (2017-03-01 21:49:02 UTC) #51
Eugene But
https://codereview.chromium.org/2641003002/diff/370001/ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm File ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm (right): https://codereview.chromium.org/2641003002/diff/370001/ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm#newcode660 ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm:660: (security_level == security_state::HTTP_SHOW_WARNING)) On 2017/03/01 21:49:02, elawrence wrote: > ...
3 weeks ago (2017-03-01 22:31:19 UTC) #53
Peter Kasting
https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc File chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc (right): https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc#newcode275 chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc:275: // as a hostname. On 2017/03/01 21:49:01, elawrence wrote: ...
3 weeks ago (2017-03-01 23:18:47 UTC) #55
elawrence
https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc File chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc (right): https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc#newcode275 chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc:275: // as a hostname. On 2017/03/01 23:18:47, Peter Kasting ...
3 weeks ago (2017-03-01 23:50:53 UTC) #56
elawrence
rdevlin.cronin@chromium.org: Please approve addition of "+extensions/common/constants.h" to the DEPS so that I can use kExtensionScheme? ...
2 weeks, 6 days ago (2017-03-02 19:08:01 UTC) #57
elawrence
rdevlin.cronin@chromium.org: Please approve addition of "+extensions/common/constants.h" to the DEPS so that I can use kExtensionScheme?
2 weeks, 6 days ago (2017-03-02 19:08:14 UTC) #59
Devlin
On 2017/03/02 19:08:14, elawrence wrote: > mailto:rdevlin.cronin@chromium.org: Please approve addition of > "+extensions/common/constants.h" to the ...
2 weeks, 6 days ago (2017-03-02 19:32:05 UTC) #62
elawrence
> I'm fine with this, but it's probably more a question for omnibox owners, aka ...
2 weeks, 6 days ago (2017-03-02 19:41:57 UTC) #63
Devlin
On 2017/03/02 19:41:57, elawrence wrote: > > I'm fine with this, but it's probably more ...
2 weeks, 6 days ago (2017-03-02 19:59:53 UTC) #64
Peter Kasting
LGTM on this. I'm hoping to find a quick and easy way of avoiding exposing ...
2 weeks, 6 days ago (2017-03-03 02:31:13 UTC) #67
Peter Kasting
Update: I have a working way of doing this "correctly"; it requires a couple precursor ...
2 weeks, 6 days ago (2017-03-03 09:17:14 UTC) #68
Peter Kasting
On 2017/03/03 09:17:14, Peter Kasting wrote: > Update: I have a working way of doing ...
2 weeks, 6 days ago (2017-03-03 09:18:41 UTC) #69
elawrence
Thanks for your feedback so far! rsesek@chromium.org: Please review the tiny change to ui/gfx/range/BUILD.gn. rohitrao@chromium.org: ...
2 weeks, 6 days ago (2017-03-03 16:07:42 UTC) #71
Robert Sesek
cocoa/ and ui/gfx/range/ LGTM
2 weeks, 5 days ago (2017-03-03 16:29:17 UTC) #72
rohitrao
lgtm https://codereview.chromium.org/2641003002/diff/510001/ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm File ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm (right): https://codereview.chromium.org/2641003002/diff/510001/ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm#newcode645 ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm:645: : NSMakeRange(0, [attributing_display_string_ length]); If the range is ...
2 weeks, 5 days ago (2017-03-03 21:53:51 UTC) #77
elawrence
Thanks for the reviews, all! https://codereview.chromium.org/2641003002/diff/510001/ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm File ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm (right): https://codereview.chromium.org/2641003002/diff/510001/ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm#newcode645 ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm:645: : NSMakeRange(0, [attributing_display_string_ length]); ...
2 weeks, 5 days ago (2017-03-03 22:01:59 UTC) #78
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2641003002/530001
2 weeks, 5 days ago (2017-03-03 22:02:40 UTC) #80
commit-bot: I haz the power
2 weeks, 5 days ago (2017-03-03 23:04:29 UTC) #83
Message was sent while issue was closed.
Committed patchset #14 (id:530001) as
https://chromium.googlesource.com/chromium/src/+/486bb18560662755558c628e39ed...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld d1a128a62