Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(861)

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

Created:
3 years, 11 months ago by elawrence
Modified:
3 years, 9 months 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

Messages

Total messages: 83 (50 generated)
Peter Kasting
Was this intended to be ready for review and just not sent?
3 years, 11 months 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 ...
3 years, 11 months 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 ...
3 years, 10 months 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 ...
3 years, 10 months ago (2017-02-06 22:42:28 UTC) #5
elawrence
> Since we seem to be duplicating code on views and Mac, consider hoisting much ...
3 years, 10 months 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 ...
3 years, 10 months 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 years, 10 months 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 years, 10 months 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 years, 10 months 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 years, 10 months 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 years, 9 months ago (2017-02-28 22:46:53 UTC) #43
Eugene But (OOO till 7-30)
rohitrao@, who owns omnibox would be a better reviewer. I can comment on this CL ...
3 years, 9 months 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 years, 9 months ago (2017-03-01 02:39:11 UTC) #48
Eugene But (OOO till 7-30)
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 years, 9 months 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 years, 9 months 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 years, 9 months ago (2017-03-01 21:49:02 UTC) #51
Eugene But (OOO till 7-30)
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 years, 9 months 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 years, 9 months 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 years, 9 months 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? ...
3 years, 9 months 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?
3 years, 9 months 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 ...
3 years, 9 months 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 ...
3 years, 9 months 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 ...
3 years, 9 months 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 ...
3 years, 9 months 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 ...
3 years, 9 months 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 ...
3 years, 9 months 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: ...
3 years, 9 months ago (2017-03-03 16:07:42 UTC) #71
Robert Sesek
cocoa/ and ui/gfx/range/ LGTM
3 years, 9 months ago (2017-03-03 16:29:17 UTC) #72
rohitrao (ping after 24h)
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 ...
3 years, 9 months 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]); ...
3 years, 9 months 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
3 years, 9 months ago (2017-03-03 22:02:40 UTC) #80
commit-bot: I haz the power
3 years, 9 months 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...

Powered by Google App Engine
This is Rietveld 408576698