Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2641003002:
Show scheme in black and content in gray for data: protocol urls (Closed)
DescriptionShow 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 #Messages
Total messages: 83 (50 generated)
Was this intended to be ready for review and just not sent?
On 2017/01/23 09:29:15, Peter Kasting wrote: > Was this intended to be ready for review and just not sent? There was some discussion about the proper level of graying, and we also need the change for mobile platforms. I expect to get this change out for review this week.
elawrence@chromium.org changed reviewers: + pkasting@chromium.org
Please take a look? Design decided we should use the standard level of deemphasis. Mobile changes are a bit more complicated and I'll tackle those in a followup CL. thanks!
Commented only on views, but the Mac comments are similar. Since we seem to be duplicating code on views and Mac, consider hoisting much of the logic below (e.g. the enum and conditions) to a method in the cross-platform model, so the view is only responsible for the actual coloring, not the logic behind what to emphasize. This should probably have a unittest to verify that we've actually applied color ranges to the correct parts of some sample input URLs. https://codereview.chromium.org/2641003002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://codereview.chromium.org/2641003002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:592: // Is the scheme one that might be abused for spoofing purposes? ALl this code feels a little piecemeal to me; the comments explain very locally what individual temps mean, but there's not much holistic picture of what we're trying to do, so it feels a bit error-prone for future modification. I tried rewriting with better comments and (possibly) structure by using a local enum. See what you think: // When possible, de-emphasize parts of the URL to draw attention to // whatever the best proxy is for the "identity" of the URL. enum Deemphasize { EVERYTHING, ALL_BUT_SCHEME, ALL_BUT_HOST, NOTHING, } deemphasize = NOTHING; if (text_is_url) { // Extension IDs are not human-readable, so de-emphasize everything to draw // attention to the human-readable name in the location icon text. if (url_scheme == base::UTF8ToUTF16(extensions::kExtensionScheme)) deemphasize = EVERYTHING; // Data URLs are rarely human-readable and can be used for spoofing, so draw // attention to the scheme to emphasize "this is just a bunch of data". else if (url_scheme == base::UTF8ToUTF16(url::kDataScheme)) deemphasize = ALL_BUT_SCHEME; // For normal URLs, the host is the best proxy for "identity". else if (host.is_nonempty()) deemphasize = ALL_BUT_HOST; } const SkColor color = location_bar_view_->GetColor(LocationBarView::TEXT); SetColor(deemphasize == NOTHING ? text : location_bar_view_->GetColor(DEEMPHASIZED_TEXT)); if (deemphasize == ALL_BUT_SCHEME) ApplyColor(color, gfx::Range(scheme.begin, scheme.end())); else if (deemphasize == ALL_BUT_HOST) ApplyColor(color, gfx::Range(host.begin, host.end())); There are still problems in this function -- SetDirectionalityMode() doesn't belong in a function called EmphasizeURLComponents() (we should probably rename the function), the comments in the blocks above this are rather misplaced (e.g. by talking about "ask the model" in the block after the one that asks the model), etc. -- but those don't have to be fixed in this CL. https://codereview.chromium.org/2641003002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:593: bool spoofy_scheme = url_scheme == base::UTF8ToUTF16(url::kDataScheme); Should we just go ahead and include blob: in this? https://codereview.chromium.org/2641003002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:601: Nit: I wouldn't add this
The CQ bit was checked by elawrence@chromium.org 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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Patchset #3 (id:40001) has been deleted
> Since we seem to be duplicating code on views and Mac, consider hoisting much of > the logic below (e.g. the enum and conditions) to a method in the cross-platform > model, so the view is only responsible for the actual coloring, not the logic > behind what to emphasize. > I tried rewriting with better comments and (possibly) structure by using a local > enum. See what you think: Done. I agree this looks a lot better. > This should probably have a unittest to verify that we've actually applied color > ranges to the correct parts of some sample input URLs. Is the idea that I'd find a way to grab the RenderText() and then walk its colors() breaks to check that coloring was as expected for each component? I took a quick look but I didn't see any tests for Views that examined colors in the omnibox. > Should we just go ahead and include blob: in this? Handling blob is tracked by Issue 682866; the plan there is to do something more elaborate (as the blob's data contains an origin). > chrome/browser/ui/views/omnibox/omnibox_view_views.cc:601: > Nit: I wouldn't add this Fixed.
On 2017/02/06 22:42:28, Peter Kasting (backlogged) wrote: > Since we seem to be duplicating code on views and Mac, consider hoisting much of > the logic below (e.g. the enum and conditions) to a method in the cross-platform > model, so the view is only responsible for the actual coloring, not the logic > behind what to emphasize. Did you look into this suggestion? I'm really hoping to avoid the duplication in the current code. On 2017/02/21 22:29:09, elawrence wrote: > > This should probably have a unittest to verify that we've actually applied > color > > ranges to the correct parts of some sample input URLs. > > Is the idea that I'd find a way to grab the RenderText() and then walk its > colors() breaks to check that coloring was as expected for each component? I > took a quick look but I didn't see any tests for Views that examined colors in > the omnibox. I think so? I'm not sure how best to plumb this. We could expose RenderText::colors() publicly or add a FRIEND_TEST. We could expose this through TextField somehow, e.g. with an accessor next to ApplyColor(). We could have some test-only method on the omnibox view to ask which parts of the URL are "emphasized". We could somehow mock out the TextField/RenderText ApplyColor() implementation and see what the view asks to color, instead of exposing the real state. I like the last idea best in terms of architectural cleanliness, but I've written very few mocks and don't actually know how to do it. https://codereview.chromium.org/2641003002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right): https://codereview.chromium.org/2641003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:603: if (deemphasize == ALL_BUT_SCHEME) Nit: {} https://codereview.chromium.org/2641003002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://codereview.chromium.org/2641003002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:615: const SkColor color = location_bar_view_->GetColor(LocationBarView::TEXT); Nit: Wonder if this would be clearer as |emphasized_color| or something? I'm just thinking maybe "color" is vague when we're working with two different colors.
The CQ bit was checked by elawrence@chromium.org 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: This issue passed the CQ dry run.
> Did you look into this suggestion? I'm really hoping to avoid the duplication
> in the current code.
I've uploaded an update which hoists the emphasis logic to the model.
> chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:603: if (deemphasize ==
> ALL_BUT_SCHEME)
> Nit: {}
Done.
> Nit: Wonder if this would be clearer as |emphasized_color| or something? I'm
> just thinking maybe "color" is vague when we're working with two different
> colors.
Agreed. Done.
> > > This should probably have a unittest to verify that we've actually applied
> > color ranges to the correct parts of some sample input URLs.
> I think so? I'm not sure how best to plumb this. We could expose
> RenderText::colors() publicly or add a FRIEND_TEST. We could expose this
> through TextField somehow, e.g. with an accessor next to ApplyColor(). We
could
> have some test-only method on the omnibox view to ask which parts of the URL
are
> "emphasized". We could somehow mock out the TextField/RenderText ApplyColor()
> implementation and see what the view asks to color, instead of exposing the
real
> state.
>
> I like the last idea best in terms of architectural cleanliness, but I've
> written very few mocks and don't actually know how to do it.
Writing a unit test for EmphasizeURLComponents() looks tricky. The existing
Omnibox_View_Views_unittests don't assign a LocationBarView*, which is needed by
EmphasizeURLComponents() to GetColor and GetSecureTextColor. (The first line of
EmphasizeURLComponents bails if the location_bar_view isn't available.) To
create a LocationBarView that isn't super-crashy, I need a Browser object, which
doesn't seem to be in use elsewhere in the OmniboxViewViews unittests.
I suppose I could refactor EmphasizeURLComponents into a function that wraps a
new more testable function (which takes color values as inputs). Or perhaps I
could learn how to create a mock LocationBarView?
Patchset #4 (id:80001) has been deleted
On 2017/02/23 18:41:42, elawrence wrote: > I suppose I could refactor EmphasizeURLComponents into a function that wraps a > new more testable function (which takes color values as inputs). Or perhaps I > could learn how to create a mock LocationBarView? I would have OmniboxView::GetDeemphasis() not just return an enum value, but actually set the relevant colors by calling a virtual method to do so (with a signature something like SetRangeEmphasis(gfx::Range, bool emphasize)); this would avoid the need to expose the "where should emphasis be" enum in the public header (as it becomes local to that function) and allow the test to subclass OmniboxViewViews and override this method to record what ranges were called, which the test body can then compare to the expected range list. This avoids the need to mock the LocationBarView and hoists a little more common code into the model function, and dovetails with one of my review suggestions below to move the EmphasizeURLComponents() call into this method as well. https://codereview.chromium.org/2641003002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://codereview.chromium.org/2641003002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:587: text(), ChromeAutocompleteSchemeClassifier(profile_), &scheme, &host); Can we hoist this block and |url_scheme| to GetDeemphasis as well? This will mean replacing its current args with outparams for |scheme| and |host| (unless you follow my suggestion on testability, in which case this basically becomes a 0-arg method that returns void). https://codereview.chromium.org/2641003002/diff/100001/components/omnibox/bro... File components/omnibox/browser/omnibox_view.cc (right): https://codereview.chromium.org/2641003002/diff/100001/components/omnibox/bro... components/omnibox/browser/omnibox_view.cc:192: OmniboxView::DEEMPHASIZE_COMPONENTS deemphasize = NOTHING; Nit: Can remove this variable, change assignments below to direct returns, remove "else"s, and "return NOTHING;" at the end of the function. This also allows putting blank lines between each return and the next block, and in general avoiding comments in between "if" and "else", which would be a nice-to-have. https://codereview.chromium.org/2641003002/diff/100001/components/omnibox/bro... components/omnibox/browser/omnibox_view.cc:195: // complicated dependencies. Urgh. Can we not have extensions/common/ in components/omnibox/DEPS? If not, I would consider having the caller pass this in to avoid this. https://codereview.chromium.org/2641003002/diff/100001/components/omnibox/bro... File components/omnibox/browser/omnibox_view.h (right): https://codereview.chromium.org/2641003002/diff/100001/components/omnibox/bro... components/omnibox/browser/omnibox_view.h:264: enum DEEMPHASIZE_COMPONENTS { This should be CamelCase.
> https://codereview.chromium.org/2641003002/diff/100001/components/omnibox/bro... > components/omnibox/browser/omnibox_view.cc:195: // complicated dependencies. > Urgh. > Can we not have extensions/common/ in components/omnibox/DEPS? > If not, I would consider having the caller pass this in to avoid this. I wasn't able to find good guidance on how to determine which DEPS modifications are acceptable and which they're not. It seemed a bit cumbersome to modify DEPS to bring in a single string constant which is very unlikely to change; the redeclaration pattern that I followed is used in numerous other places in the codebase for the same reason. > I would have OmniboxView::GetDeemphasis() not just return an enum value, but > actually set the relevant colors by calling a virtual method to do so (with a > signature something like SetRangeEmphasis(gfx::Range, bool emphasize)); this > would avoid the need to expose the "where should emphasis be" enum in the public > header (as it becomes local to that function) and allow the test to subclass > OmniboxViewViews and override this method to record what ranges were called, > which the test body can then compare to the expected range list. I've tried this in Patchset 5, but it looks like making this work correctly will be difficult. On the Views side, the EmphasizeURLComponents function directly modifies the styling of the text. Changing the code so the analysis occurs in OmniboxView and the styling occurs via new virtual method calls to OmniboxViewViews is straightforward. However, on the Mac side, the URL analysis code runs inside ApplyTextAttributes which updates a NSMutableAttributedString with all of the styling that should be updated, and then after all of those updates are done the style is actually applied via [field_ setAttributedStringValue:attributedString]. This makes it a bit cumbersome to implement the new virtual method calls in OmniboxViewMac because we'd need to have some sort of temporary member variable that holds the styles across these method calls (because I don't think you'd be happy if I used any kind of raw pointer to smuggle a reference to this state through the base to the virtual methods).
(If possible, please reply to review comments inline with the commenting tool rather than via email/"send message", so discussion threads stay together... that makes it easier to track things down later) On 2017/02/24 19:35:58, elawrence wrote: > > > https://codereview.chromium.org/2641003002/diff/100001/components/omnibox/bro... > > components/omnibox/browser/omnibox_view.cc:195: // complicated dependencies. > > Urgh. > > Can we not have extensions/common/ in components/omnibox/DEPS? > > If not, I would consider having the caller pass this in to avoid this. > > I wasn't able to find good guidance on how to determine which DEPS modifications > are acceptable and which they're not. It seemed a bit cumbersome to modify DEPS > to bring in a single string constant which is very unlikely to change; the > redeclaration pattern that I followed is used in numerous other places in the > codebase for the same reason. Yeah, I object to it everywhere in the codebase :). If we really didn't believe this would ever change, and it was fine to hardcode it repeatedly, we should hardcode it everywhere, and not imply that we have a universal constant. Which seems clearly bad. While modifying DEPS is cumbersome, it's sort of the price we pay for having components. This code has a dependency on a concept specified by extensions/, so its DEPS file should really say so. In general, I ask componentization folks like blundell when I have questions about what can depend on what. If we can't depend on extensions/ here, then we should probably find a place these sorts of constants can live that everything _can_ depend on, or else add some sort of getter plumbing so components can ask the embedder about this. These sorts of larger fixes would clearly be outside the scope of this patch, of course. > > I would have OmniboxView::GetDeemphasis() not just return an enum value, but > > actually set the relevant colors by calling a virtual method to do so (with a > > signature something like SetRangeEmphasis(gfx::Range, bool emphasize)); this > > would avoid the need to expose the "where should emphasis be" enum in the > public > > header (as it becomes local to that function) and allow the test to subclass > > OmniboxViewViews and override this method to record what ranges were called, > > which the test body can then compare to the expected range list. > > I've tried this in Patchset 5, but it looks like making this work correctly will > be difficult. > > On the Views side, the EmphasizeURLComponents function directly modifies the > styling of the text. Changing the code so the analysis occurs in OmniboxView and > the styling occurs via new virtual method calls to OmniboxViewViews is > straightforward. > > However, on the Mac side, the URL analysis code runs inside ApplyTextAttributes > which updates a NSMutableAttributedString with all of the styling that should be > updated, and then after all of those updates are done the style is actually > applied via [field_ setAttributedStringValue:attributedString]. This makes it a > bit cumbersome to implement the new virtual method calls in OmniboxViewMac > because we'd need to have some sort of temporary member variable that holds the > styles across these method calls (because I don't think you'd be happy if I used > any kind of raw pointer to smuggle a reference to this state through the base to > the virtual methods). I wonder if the Mac APIs have an easy way to just apply an incremental attribute change, which could be done separately of the existing batching. If not, I'm not all that opposed to either the temporary member idea (which is probably the route I'd go), or the "pass some state along" route (which I might do by #ifdefing an additional argument to the function on Mac only, so it could be passed as a properly-typed pointer instead of a meaningless void* that views would set to nullptr).
Patchset #8 (id:180001) has been deleted
Patchset #8 (id:200001) has been deleted
The CQ bit was checked by elawrence@chromium.org 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: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #8 (id:220001) has been deleted
The CQ bit was checked by elawrence@chromium.org 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: This issue passed the CQ dry run.
Patchset #8 (id:240001) has been deleted
Patchset #9 (id:280001) has been deleted
Patchset #8 (id:260001) has been deleted
Patchset #11 (id:350001) has been deleted
Patchset #10 (id:340001) has been deleted
Patchset #9 (id:320001) has been deleted
Patchset #8 (id:300001) has been deleted
The CQ bit was checked by elawrence@chromium.org 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...
elawrence@chromium.org changed reviewers: + eugenebut@chromium.org, rsesek@chromium.org
pkasting@chromium.org: I've hoisted all of the scheme analysis logic to the OmniboxView and left the per-platform subclasses to actually implement the coloring. I've added unit tests for data and other schemes. Please take another look? rsesek@chromium.org: Please review the tiny change to ui/gfx/range/BUILD.gn. eugenebut@chromium.org: Please review changes in ios/chrome/browser/ui/omnibox/* Thanks!
eugenebut@chromium.org changed reviewers: + rohitrao@chromium.org
rohitrao@, who owns omnibox would be a better reviewer. I can comment on this CL only from Style Guide perspective. https://codereview.chromium.org/2641003002/diff/370001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/omnibox/omnibox_view_ios.h (right): https://codereview.chromium.org/2641003002/diff/370001/ios/chrome/browser/ui/... ios/chrome/browser/ui/omnibox/omnibox_view_ios.h:43: static UIColor* GetSecureTextColor( Could you please add comments https://codereview.chromium.org/2641003002/diff/370001/ios/chrome/browser/ui/... ios/chrome/browser/ui/omnibox/omnibox_view_ios.h:45: bool in_dark_mode); Do you want to add enum instead of this bool? Otherwise the code will be quite uninformative at call site: GetSecureTextColor(security_level, true) https://codereview.chromium.org/2641003002/diff/370001/ios/chrome/browser/ui/... ios/chrome/browser/ui/omnibox/omnibox_view_ios.h:144: // Update colors in |attributing_display_string_|. s/Update/Updates https://codereview.chromium.org/2641003002/diff/370001/ios/chrome/browser/ui/... ios/chrome/browser/ui/omnibox/omnibox_view_ios.h:144: // Update colors in |attributing_display_string_|. Do you want to explain what this method does from the interface perspective, rather than documenting specific implementation details? Same comment for the next method https://codereview.chromium.org/2641003002/diff/370001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm (right): https://codereview.chromium.org/2641003002/diff/370001/ios/chrome/browser/ui/... ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm:660: (security_level == security_state::HTTP_SHOW_WARNING)) nit: Do you want to add braces, because if statement has multiple lines?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Cool, I think this is almost ready to go. Most of the remaining comments are nits. The question about the test of the non-URL case is probably the most important one. https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h (right): https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h:187: // Update colors in |attributing_display_string_|. Nit: Descriptive, not imperative (e.g. "Updates"); see http://google.github.io/styleguide/cppguide.html#Function_Comments . (2 places) https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right): https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:600: attributing_display_string_ = attributedString; Nit: Consider using base::AutoReset here for safety against future complexity: base::AutoReset<NSMutableAttributedString*> resetter( &attributing_display_string_, attributedString); ApplyEmphasis(display_text, ChromeAutocompleteSchemeClassifier(profile_)); https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:580: const bool strike = (security_level_ == security_state::DANGEROUS); Nit: () unnecessary https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc (right): https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc:40: NULL, Nit: While here: nullptr https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc:70: gfx::Range GetSchemeRange() { return scheme_range_; } Nit: Next three can be const. I suggest naming these like inlined accessors as well, e.g. "scheme_range()", and omitting the blank lines between them. https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc:246: // Ensure that everything is emphasized for unknown scheme heirarchical URLs. Nit: hierarchical https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc:265: // Ensure that Origin is emphasized for URL-like text. Nit: Origin -> the origin https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc:275: // as a hostname. That doesn't sound right. CurrentTextIsURL() ought to be false for this, leading us to emphasize the whole string (rather than a range that includes the whole string), I'd think. https://codereview.chromium.org/2641003002/diff/370001/components/omnibox/bro... File components/omnibox/browser/omnibox_view.cc (right): https://codereview.chromium.org/2641003002/diff/370001/components/omnibox/bro... components/omnibox/browser/omnibox_view.cc:212: const base::string16 url_scheme = Nit: Move inside the next conditional https://codereview.chromium.org/2641003002/diff/370001/components/omnibox/bro... components/omnibox/browser/omnibox_view.cc:252: if (model()->user_input_in_progress() || !model_->CurrentTextIsURL() || I think we can nuke the CurrentTextIsURL() check and everything but the first line of the above comment; it was only applicable for Search Term Replacement, which is dead. https://codereview.chromium.org/2641003002/diff/370001/components/omnibox/bro... components/omnibox/browser/omnibox_view.cc:253: !scheme.is_nonempty()) { Nit: {} unnecessary https://codereview.chromium.org/2641003002/diff/370001/components/omnibox/bro... components/omnibox/browser/omnibox_view.cc:256: UpdateSchemeEmphasis(scheme_range); Do we always have to call this method? It seems like it should only be necessary if the range is nonempty, as long as in the views implementation we clear the diagonal strike state before calling ApplyEmphasis() (which I think is reasonable). Then we could avoid checking range validity in the callers, and just do this here: if (!model()->user_input_in_progress() && scheme_range.IsValid()) UpdateSchemeEmphasis(scheme_range); https://codereview.chromium.org/2641003002/diff/370001/components/omnibox/bro... File components/omnibox/browser/omnibox_view.h (right): https://codereview.chromium.org/2641003002/diff/370001/components/omnibox/bro... components/omnibox/browser/omnibox_view.h:264: // range to indicate the base level of emphasis for the text. Nit: Maybe this would be more descriptive while being briefer: Marks part (or, if |range| is invalid, all) of the current text as emphasized or de-emphasized, by changing its color. https://codereview.chromium.org/2641003002/diff/370001/components/omnibox/bro... components/omnibox/browser/omnibox_view.h:265: virtual void SetEmphasis(bool emphasize, gfx::Range); I'd make these two functions pure virtual. All subclasses should have to think about how to implement these, and if there are any today that want to do nothing, I'd do that in those subclasses directly. This will also help clarify that these are implemented by subclasses, which you were trying to indicate in the function comment (and I've removed in my proposed comment). Nit: Name the second param https://codereview.chromium.org/2641003002/diff/370001/components/omnibox/bro... components/omnibox/browser/omnibox_view.h:269: virtual void UpdateSchemeEmphasis(gfx::Range range); Nit: I might call this UpdateSchemeStyle() or something, since it's not solely about "emphasis". You could comment it: Sets the color and strikethrough state for |range|, which represents the current scheme, based on the current security state. Schemes are displayed in different ways for different security levels. https://codereview.chromium.org/2641003002/diff/370001/components/omnibox/bro... components/omnibox/browser/omnibox_view.h:271: // Deemphasize parts of the URL to draw attention to whatever best represents Nit: Descriptive, not imperative https://codereview.chromium.org/2641003002/diff/370001/components/omnibox/bro... components/omnibox/browser/omnibox_view.h:273: void ApplyEmphasis(const base::string16& display_text, Nit: I wonder if this title is too specific now that this calls UpdateSchemeEmphasis() (which I propose renaming above). Maybe it should be named more like UpdateTextStyle() and commented: Parses |display_text|, then sets the display emphasis and scheme style appropriately. If the text is a query string, there is no scheme, and everything is emphasized equally, whereas for URLs the scheme is styled based on the current security state, and parts of the URL are de-emphasized to draw attention to whatever best represents the "identity" of the current URL. See comments in the code for more. https://codereview.chromium.org/2641003002/diff/370001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/omnibox/omnibox_view_ios.h (right): https://codereview.chromium.org/2641003002/diff/370001/ios/chrome/browser/ui/... ios/chrome/browser/ui/omnibox/omnibox_view_ios.h:144: // Update colors in |attributing_display_string_|. On 2017/02/28 23:04:38, Eugene But wrote: > Do you want to explain what this method does from the interface perspective, > rather than documenting specific implementation details? Wouldn't one expect to find such comments on the base class declaration? That's the pattern in most of Chrome. https://codereview.chromium.org/2641003002/diff/370001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm (right): https://codereview.chromium.org/2641003002/diff/370001/ios/chrome/browser/ui/... ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm:663: DCHECK(security_level != security_state::SECURITY_WARNING); Nit: DCHECK_NE(security_state::SECURITY_WARNING, security_level) (2 places) https://codereview.chromium.org/2641003002/diff/370001/ios/chrome/browser/ui/... ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm:689: attributing_display_string_ = as; Nit: See Mac comment
https://codereview.chromium.org/2641003002/diff/370001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/omnibox/omnibox_view_ios.h (right): https://codereview.chromium.org/2641003002/diff/370001/ios/chrome/browser/ui/... ios/chrome/browser/ui/omnibox/omnibox_view_ios.h:144: // Update colors in |attributing_display_string_|. On 2017/03/01 02:39:11, Peter Kasting wrote: > On 2017/02/28 23:04:38, Eugene But wrote: > > Do you want to explain what this method does from the interface perspective, > > rather than documenting specific implementation details? > > Wouldn't one expect to find such comments on the base class declaration? That's > the pattern in most of Chrome. Oh, right. I missed that these are override methods. In this case maybe it would be better to drop these comments at all?
https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h (right): https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h:242: NSMutableAttributedString* attributing_display_string_; Who owns this? Either document as "Weak" or put in a scoped_nsobject. https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right): https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:188: do_coalesced_range_update_(false) { attributing_display_string_ is uninitialized. https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:584: NSMutableAttributedString* attributedString) { nit: Since you're here, this should be attributed_string https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:600: attributing_display_string_ = attributedString; Perhaps DCHECK that attributing_display_string_ is nil first? https://codereview.chromium.org/2641003002/diff/370001/components/omnibox/bro... File components/omnibox/browser/omnibox_view.h (right): https://codereview.chromium.org/2641003002/diff/370001/components/omnibox/bro... components/omnibox/browser/omnibox_view.h:265: virtual void SetEmphasis(bool emphasize, gfx::Range); The gfx::Range argument here and on line 269 could be passed as const&.
Thanks for having a look! https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h (right): https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h:187: // Update colors in |attributing_display_string_|. On 2017/03/01 02:39:10, Peter Kasting wrote: > Nit: Descriptive, not imperative (e.g. "Updates"); see > http://google.github.io/styleguide/cppguide.html#Function_Comments . (2 places) Removed https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h:242: NSMutableAttributedString* attributing_display_string_; On 2017/03/01 18:29:45, Robert Sesek wrote: > Who owns this? Either document as "Weak" or put in a scoped_nsobject. Documented as weak. https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right): https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:188: do_coalesced_range_update_(false) { On 2017/03/01 18:29:45, Robert Sesek wrote: > attributing_display_string_ is uninitialized. Done. https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:584: NSMutableAttributedString* attributedString) { On 2017/03/01 18:29:45, Robert Sesek wrote: > nit: Since you're here, this should be attributed_string Done. https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:600: attributing_display_string_ = attributedString; On 2017/03/01 02:39:10, Peter Kasting wrote: > Nit: Consider using base::AutoReset here for safety against future complexity: > > base::AutoReset<NSMutableAttributedString*> resetter( > &attributing_display_string_, attributedString); > ApplyEmphasis(display_text, ChromeAutocompleteSchemeClassifier(profile_)); Done. https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:600: attributing_display_string_ = attributedString; On 2017/03/01 18:29:45, Robert Sesek wrote: > Perhaps DCHECK that attributing_display_string_ is nil first? Done. https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:580: const bool strike = (security_level_ == security_state::DANGEROUS); On 2017/03/01 02:39:10, Peter Kasting wrote: > Nit: () unnecessary Done. https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc (right): https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc:40: NULL, On 2017/03/01 02:39:11, Peter Kasting wrote: > Nit: While here: nullptr Done. https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc:70: gfx::Range GetSchemeRange() { return scheme_range_; } On 2017/03/01 02:39:11, Peter Kasting wrote: > Nit: Next three can be const. > > I suggest naming these like inlined accessors as well, e.g. "scheme_range()", > and omitting the blank lines between them. Done. https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc:246: // Ensure that everything is emphasized for unknown scheme heirarchical URLs. On 2017/03/01 02:39:11, Peter Kasting wrote: > Nit: hierarchical Done. https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc:265: // Ensure that Origin is emphasized for URL-like text. On 2017/03/01 02:39:10, Peter Kasting wrote: > Nit: Origin -> the origin Done. https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc:275: // as a hostname. On 2017/03/01 02:39:10, Peter Kasting wrote: > That doesn't sound right. CurrentTextIsURL() ought to be false for this, > leading us to emphasize the whole string (rather than a range that includes the > whole string), I'd think. The problem is that OmniboxEditModel::CurrentTextIsURL() returns true if (!user_input_in_progress_). CurrentTextIsURL() isn't virtual, and even if I expose tweak the TestingOmniboxEditController so that calling SetInputInProgress doesn't crash, CurrentTextIsURL ends up crashing inside a call to TemplateURLService::Load from KeywordProvider::GetTemplateURLService. For now, I've just removed this test. https://codereview.chromium.org/2641003002/diff/370001/components/omnibox/bro... File components/omnibox/browser/omnibox_view.cc (right): https://codereview.chromium.org/2641003002/diff/370001/components/omnibox/bro... components/omnibox/browser/omnibox_view.cc:212: const base::string16 url_scheme = On 2017/03/01 02:39:11, Peter Kasting wrote: > Nit: Move inside the next conditional Done. https://codereview.chromium.org/2641003002/diff/370001/components/omnibox/bro... components/omnibox/browser/omnibox_view.cc:252: if (model()->user_input_in_progress() || !model_->CurrentTextIsURL() || On 2017/03/01 02:39:11, Peter Kasting wrote: > I think we can nuke the CurrentTextIsURL() check and everything but the first > line of the above comment; it was only applicable for Search Term Replacement, > which is dead. Interesting. Updated. https://codereview.chromium.org/2641003002/diff/370001/components/omnibox/bro... components/omnibox/browser/omnibox_view.cc:253: !scheme.is_nonempty()) { On 2017/03/01 02:39:11, Peter Kasting wrote: > Nit: {} unnecessary Technically yes, although the style guide seems to allow it and Eugene asked for {} elsewhere where the conditional test was multiple lines. Now that it's not, I guess I'll drop them. https://codereview.chromium.org/2641003002/diff/370001/components/omnibox/bro... components/omnibox/browser/omnibox_view.cc:256: UpdateSchemeEmphasis(scheme_range); On 2017/03/01 02:39:11, Peter Kasting wrote: > Do we always have to call this method? It seems like it should only be > necessary if the range is nonempty, as long as in the views implementation we > clear the diagonal strike state before calling ApplyEmphasis() (which I think is > reasonable). > > Then we could avoid checking range validity in the callers, and just do this > here: > > if (!model()->user_input_in_progress() && scheme_range.IsValid()) > UpdateSchemeEmphasis(scheme_range); I'm inclined to leave this as it is, because it makes for a more clear behavioral contract for subclasses-- we'll always call both virtual methods, even when the scheme isn't present. As you noted, we could just change this and update ViewViews to clear the strike state before asking for emphasis, but we'd have to revise this if a subclass ever needed to change some state when the scheme was removed. Any strong objections? https://codereview.chromium.org/2641003002/diff/370001/components/omnibox/bro... File components/omnibox/browser/omnibox_view.h (right): https://codereview.chromium.org/2641003002/diff/370001/components/omnibox/bro... components/omnibox/browser/omnibox_view.h:264: // range to indicate the base level of emphasis for the text. On 2017/03/01 02:39:11, Peter Kasting wrote: > Nit: Maybe this would be more descriptive while being briefer: > > Marks part (or, if |range| is invalid, all) of the current text as emphasized or > de-emphasized, by changing its color. Done. https://codereview.chromium.org/2641003002/diff/370001/components/omnibox/bro... components/omnibox/browser/omnibox_view.h:265: virtual void SetEmphasis(bool emphasize, gfx::Range); On 2017/03/01 18:29:45, Robert Sesek wrote: > The gfx::Range argument here and on line 269 could be passed as const&. Done. https://codereview.chromium.org/2641003002/diff/370001/components/omnibox/bro... components/omnibox/browser/omnibox_view.h:265: virtual void SetEmphasis(bool emphasize, gfx::Range); On 2017/03/01 02:39:11, Peter Kasting wrote: > I'd make these two functions pure virtual. All subclasses should have to think > about how to implement these, and if there are any today that want to do > nothing, I'd do that in those subclasses directly. > > This will also help clarify that these are implemented by subclasses, which you > were trying to indicate in the function comment (and I've removed in my proposed > comment). > > Nit: Name the second param Done. https://codereview.chromium.org/2641003002/diff/370001/components/omnibox/bro... components/omnibox/browser/omnibox_view.h:269: virtual void UpdateSchemeEmphasis(gfx::Range range); On 2017/03/01 02:39:11, Peter Kasting wrote: > Nit: I might call this UpdateSchemeStyle() or something, since it's not solely > about "emphasis". You could comment it: > > Sets the color and strikethrough state for |range|, which represents the current > scheme, based on the current security state. Schemes are displayed in different > ways for different security levels. Done. https://codereview.chromium.org/2641003002/diff/370001/components/omnibox/bro... components/omnibox/browser/omnibox_view.h:271: // Deemphasize parts of the URL to draw attention to whatever best represents On 2017/03/01 02:39:11, Peter Kasting wrote: > Nit: Descriptive, not imperative Done. https://codereview.chromium.org/2641003002/diff/370001/components/omnibox/bro... components/omnibox/browser/omnibox_view.h:273: void ApplyEmphasis(const base::string16& display_text, On 2017/03/01 02:39:11, Peter Kasting wrote: > Nit: I wonder if this title is too specific now that this calls > UpdateSchemeEmphasis() (which I propose renaming above). Maybe it should be > named more like UpdateTextStyle() and commented: > > Parses |display_text|, then sets the display emphasis and scheme style > appropriately. If the text is a query string, there is no scheme, and > everything is emphasized equally, whereas for URLs the scheme is styled based on > the current security state, and parts of the URL are de-emphasized to draw > attention to whatever best represents the "identity" of the current URL. See > comments in the code for more. Done. https://codereview.chromium.org/2641003002/diff/370001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/omnibox/omnibox_view_ios.h (right): https://codereview.chromium.org/2641003002/diff/370001/ios/chrome/browser/ui/... ios/chrome/browser/ui/omnibox/omnibox_view_ios.h:43: static UIColor* GetSecureTextColor( On 2017/02/28 23:04:38, Eugene But wrote: > Could you please add comments Done. https://codereview.chromium.org/2641003002/diff/370001/ios/chrome/browser/ui/... ios/chrome/browser/ui/omnibox/omnibox_view_ios.h:45: bool in_dark_mode); On 2017/02/28 23:04:38, Eugene But wrote: > Do you want to add enum instead of this bool? Otherwise the code will be quite > uninformative at call site: GetSecureTextColor(security_level, true) I'm inclined to leave this as it is to match Mac's code. Readers who want to see what this method is doing can easily do so, and it's unlikely that anyone will ever be passing a bare boolean literal instead of e.g. [field_ incognito], which is what the only caller of this function does. https://codereview.chromium.org/2641003002/diff/370001/ios/chrome/browser/ui/... ios/chrome/browser/ui/omnibox/omnibox_view_ios.h:144: // Update colors in |attributing_display_string_|. On 2017/03/01 06:08:08, Eugene But wrote: > On 2017/03/01 02:39:11, Peter Kasting wrote: > > On 2017/02/28 23:04:38, Eugene But wrote: > > > Do you want to explain what this method does from the interface perspective, > > > rather than documenting specific implementation details? > > > > Wouldn't one expect to find such comments on the base class declaration? > That's > > the pattern in most of Chrome. > Oh, right. I missed that these are override methods. In this case maybe it would > be better to drop these comments at all? Removed. https://codereview.chromium.org/2641003002/diff/370001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm (right): https://codereview.chromium.org/2641003002/diff/370001/ios/chrome/browser/ui/... ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm:660: (security_level == security_state::HTTP_SHOW_WARNING)) On 2017/02/28 23:04:38, Eugene But wrote: > nit: Do you want to add braces, because if statement has multiple lines? Ok. I didn't see a style guide opinion on this one, and Peter seems to request the opposite elsewhere. https://codereview.chromium.org/2641003002/diff/370001/ios/chrome/browser/ui/... ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm:663: DCHECK(security_level != security_state::SECURITY_WARNING); On 2017/03/01 02:39:11, Peter Kasting wrote: > Nit: DCHECK_NE(security_state::SECURITY_WARNING, security_level) (2 places) Done. https://codereview.chromium.org/2641003002/diff/370001/ios/chrome/browser/ui/... ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm:689: attributing_display_string_ = as; On 2017/03/01 02:39:11, Peter Kasting wrote: > Nit: See Mac comment Done.
Patchset #9 (id:390001) has been deleted
https://codereview.chromium.org/2641003002/diff/370001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm (right): https://codereview.chromium.org/2641003002/diff/370001/ios/chrome/browser/ui/... 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: > On 2017/02/28 23:04:38, Eugene But wrote: > > nit: Do you want to add braces, because if statement has multiple lines? > > Ok. I didn't see a style guide opinion on this one, and Peter seems to request > the opposite elsewhere. C++ Style Guide is quite vague on this: "In general, curly braces are not required for single-line statements, but they are allowed if you like them; conditional or loop statements with complex conditions or statements may be more readable with curly braces. Some groups require that an if must always have an accompanying brace.". I read this as "prefer curly braces for multiline statement", but I see how other people can interpret this differently.
Patchset #10 (id:430001) has been deleted
https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc (right): https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc:275: // as a hostname. On 2017/03/01 21:49:01, elawrence wrote: > On 2017/03/01 02:39:10, Peter Kasting wrote: > > That doesn't sound right. CurrentTextIsURL() ought to be false for this, > > leading us to emphasize the whole string (rather than a range that includes > the > > whole string), I'd think. > > The problem is that OmniboxEditModel::CurrentTextIsURL() returns true if > (!user_input_in_progress_). CurrentTextIsURL() isn't virtual, and even if I > expose tweak the TestingOmniboxEditController so that calling SetInputInProgress > doesn't crash, CurrentTextIsURL ends up crashing inside a call to > TemplateURLService::Load from KeywordProvider::GetTemplateURLService. > > For now, I've just removed this test. That's because you're calling SetText() in all of these, which is really a low-level Textfield function rather than an OmniboxView one. I would attempt to use omnibox_view()->SetUserText() instead, which is the intended API to change the text in this control. That's a suggestion not just for this test, but all the ones above. This may still cause problems if this test doesn't instantiate enough objects (as mocks/fakes) to avoid crashing today. My hope is that this would be fairly easily solved, especially since the test base is already instantiating a TestingProfile and such. https://codereview.chromium.org/2641003002/diff/370001/components/omnibox/bro... File components/omnibox/browser/omnibox_view.cc (right): https://codereview.chromium.org/2641003002/diff/370001/components/omnibox/bro... components/omnibox/browser/omnibox_view.cc:256: UpdateSchemeEmphasis(scheme_range); On 2017/03/01 21:49:01, elawrence wrote: > On 2017/03/01 02:39:11, Peter Kasting wrote: > > Do we always have to call this method? It seems like it should only be > > necessary if the range is nonempty, as long as in the views implementation we > > clear the diagonal strike state before calling ApplyEmphasis() (which I think > is > > reasonable). > > > > Then we could avoid checking range validity in the callers, and just do this > > here: > > > > if (!model()->user_input_in_progress() && scheme_range.IsValid()) > > UpdateSchemeEmphasis(scheme_range); > > I'm inclined to leave this as it is, because it makes for a more clear > behavioral contract for subclasses-- we'll always call both virtual methods, > even when the scheme isn't present. As you noted, we could just change this and > update ViewViews to clear the strike state before asking for emphasis, but we'd > have to revise this if a subclass ever needed to change some state when the > scheme was removed. Any strong objections? It really depends how one thinks about these methods. Are they intended to set state to specific values, or are they intended to be generic always-called hook points that subclasses can use to do any twiddling they may or may not need to do? I made my suggestion because I think the former is clearer (call methods to have them do a specific act, rather than to provide a hook at a specific control flow point). I think you're suggesting the latter. Either route is arguably reasonable. I would say, consider in the abstract which route you think is easiest to name and document the functions to reflect: "what does this do" versus "when is this called"; and which will lead to the fewest bugs down the road. Then go whichever route those considerations lead you to. https://codereview.chromium.org/2641003002/diff/370001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm (right): https://codereview.chromium.org/2641003002/diff/370001/ios/chrome/browser/ui/... ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm:660: (security_level == security_state::HTTP_SHOW_WARNING)) On 2017/03/01 22:31:19, Eugene But wrote: > On 2017/03/01 21:49:02, elawrence wrote: > > On 2017/02/28 23:04:38, Eugene But wrote: > > > nit: Do you want to add braces, because if statement has multiple lines? > > > > Ok. I didn't see a style guide opinion on this one, and Peter seems to request > > the opposite elsewhere. > C++ Style Guide is quite vague on this: "In general, curly braces are not > required for single-line statements, but they are allowed if you like them; > conditional or loop statements with complex conditions or statements may be more > readable with curly braces. Some groups require that an if must always have an > accompanying brace.". > > I read this as "prefer curly braces for multiline statement", but I see how > other people can interpret this differently. Read "statement" as specifically "loop body" in the style guide text (the part above the body is the "condition") and this gets clearer.
https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc (right): https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc:275: // as a hostname. On 2017/03/01 23:18:47, Peter Kasting wrote: > On 2017/03/01 21:49:01, elawrence wrote: > > On 2017/03/01 02:39:10, Peter Kasting wrote: > > > That doesn't sound right. CurrentTextIsURL() ought to be false for this, > > > leading us to emphasize the whole string (rather than a range that includes > > the > > > whole string), I'd think. > > > > The problem is that OmniboxEditModel::CurrentTextIsURL() returns true if > > (!user_input_in_progress_). CurrentTextIsURL() isn't virtual, and even if I > > expose tweak the TestingOmniboxEditController so that calling > SetInputInProgress > > doesn't crash, CurrentTextIsURL ends up crashing inside a call to > > TemplateURLService::Load from KeywordProvider::GetTemplateURLService. > > > > For now, I've just removed this test. > > That's because you're calling SetText() in all of these, which is really a > low-level Textfield function rather than an OmniboxView one. I would attempt to > use omnibox_view()->SetUserText() instead, which is the intended API to change > the text in this control. That's a suggestion not just for this test, but all > the ones above. > > This may still cause problems if this test doesn't instantiate enough objects > (as mocks/fakes) to avoid crashing today. My hope is that this would be fairly > easily solved, especially since the test base is already instantiating a > TestingProfile and such. Yes, SetUserText() code path that leads to the described crashes. I didn't spend a lot of time looking into how to make the TemplateURLService call not crash, but it doesn't seem like the OmniBoxEditController is well set up for unittesting today, and it's not clear that a security fix should block on an broader rearchitecture.
rdevlin.cronin@chromium.org: Please approve addition of "+extensions/common/constants.h" to the DEPS so that I can use kExtensionScheme? https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc (right): https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc:275: // as a hostname. On 2017/03/01 23:18:47, Peter Kasting wrote: > On 2017/03/01 21:49:01, elawrence wrote: > > On 2017/03/01 02:39:10, Peter Kasting wrote: > > > That doesn't sound right. CurrentTextIsURL() ought to be false for this, > > > leading us to emphasize the whole string (rather than a range that includes > > the > > > whole string), I'd think. > > > > The problem is that OmniboxEditModel::CurrentTextIsURL() returns true if > > (!user_input_in_progress_). CurrentTextIsURL() isn't virtual, and even if I > > expose tweak the TestingOmniboxEditController so that calling > SetInputInProgress > > doesn't crash, CurrentTextIsURL ends up crashing inside a call to > > TemplateURLService::Load from KeywordProvider::GetTemplateURLService. > > > > For now, I've just removed this test. > > That's because you're calling SetText() in all of these, which is really a > low-level Textfield function rather than an OmniboxView one. I would attempt to > use omnibox_view()->SetUserText() instead, which is the intended API to change > the text in this control. That's a suggestion not just for this test, but all > the ones above. > > This may still cause problems if this test doesn't instantiate enough objects > (as mocks/fakes) to avoid crashing today. My hope is that this would be fairly > easily solved, especially since the test base is already instantiating a > TestingProfile and such. Exposing CurrentTextIsURL as a virtual function on the View and checking that is the only reasonable path I've found for unblocking the unit test. https://codereview.chromium.org/2641003002/diff/370001/components/omnibox/bro... File components/omnibox/browser/omnibox_view.cc (right): https://codereview.chromium.org/2641003002/diff/370001/components/omnibox/bro... components/omnibox/browser/omnibox_view.cc:256: UpdateSchemeEmphasis(scheme_range); On 2017/03/01 23:18:47, Peter Kasting wrote: > On 2017/03/01 21:49:01, elawrence wrote: > > On 2017/03/01 02:39:11, Peter Kasting wrote: > > > Do we always have to call this method? It seems like it should only be > > > necessary if the range is nonempty, as long as in the views implementation > we > > > clear the diagonal strike state before calling ApplyEmphasis() (which I > think > > is > > > reasonable). > > > > > > Then we could avoid checking range validity in the callers, and just do this > > > here: > > > > > > if (!model()->user_input_in_progress() && scheme_range.IsValid()) > > > UpdateSchemeEmphasis(scheme_range); > > > > I'm inclined to leave this as it is, because it makes for a more clear > > behavioral contract for subclasses-- we'll always call both virtual methods, > > even when the scheme isn't present. As you noted, we could just change this > and > > update ViewViews to clear the strike state before asking for emphasis, but > we'd > > have to revise this if a subclass ever needed to change some state when the > > scheme was removed. Any strong objections? > > It really depends how one thinks about these methods. Are they intended to set > state to specific values, or are they intended to be generic always-called hook > points that subclasses can use to do any twiddling they may or may not need to > do? > > I made my suggestion because I think the former is clearer (call methods to have > them do a specific act, rather than to provide a hook at a specific control flow > point). I think you're suggesting the latter. Either route is arguably > reasonable. > > I would say, consider in the abstract which route you think is easiest to name > and document the functions to reflect: "what does this do" versus "when is this > called"; and which will lead to the fewest bugs down the road. Then go > whichever route those considerations lead you to. Done.
elawrence@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
rdevlin.cronin@chromium.org: Please approve addition of "+extensions/common/constants.h" to the DEPS so that I can use kExtensionScheme?
The CQ bit was checked by elawrence@chromium.org 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...
On 2017/03/02 19:08:14, elawrence wrote: > mailto:rdevlin.cronin@chromium.org: Please approve addition of > "+extensions/common/constants.h" to the DEPS so that I can use kExtensionScheme? I'm fine with this, but it's probably more a question for omnibox owners, aka Peter. :)
> I'm fine with this, but it's probably more a question for omnibox owners, aka
> Peter. :)
Devlin: As I understand things, DEPS updates require explicit LGTM from the
*target* path owner.
** Presubmit Messages **
You need LGTM from owners of depends-on paths in DEPS that were modified in this
CL:
'+extensions/common/constants.h',
On 2017/03/02 19:41:57, elawrence wrote: > > I'm fine with this, but it's probably more a question for omnibox owners, aka > > Peter. :) > > Devlin: As I understand things, DEPS updates require explicit LGTM from the > *target* path owner. > > ** Presubmit Messages ** > You need LGTM from owners of depends-on paths in DEPS that were modified in this > CL: > '+extensions/common/constants.h', Oh, interesting. LGTM as long as omnibox folks don't mind loosely depending on extensions.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM on this. I'm hoping to find a quick and easy way of avoiding exposing CurrentTextIsURL() as a virtual function, but I don't want to block you on that in case it takes me longer than "tonight". On 2017/03/02 19:59:53, Devlin wrote: > On 2017/03/02 19:41:57, elawrence wrote: > > > I'm fine with this, but it's probably more a question for omnibox owners, > aka > > > Peter. :) > > > > Devlin: As I understand things, DEPS updates require explicit LGTM from the > > *target* path owner. > > > > ** Presubmit Messages ** > > You need LGTM from owners of depends-on paths in DEPS that were modified in > this > > CL: > > '+extensions/common/constants.h', > > Oh, interesting. > > LGTM as long as omnibox folks don't mind loosely depending on extensions. I don't mind. I would also be OK with avoiding this dependency by plumbing through some generic API in OmniboxClient that asks what to emphasize in the given URL, so the explicit extension-checking part could be moved Chrome-side. I'm not sure that's necessary, so going the simpler route seemed fine. https://codereview.chromium.org/2641003002/diff/470001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h (right): https://codereview.chromium.org/2641003002/diff/470001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h:190: void SetEmphasis(bool emphasize, const gfx::Range& range) override; Nit: Consider an "// OmniboxView:" header above these two to emphasize that they're overrides, and where they came from https://codereview.chromium.org/2641003002/diff/470001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc (right): https://codereview.chromium.org/2641003002/diff/470001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc:97: // Range of the last scheme logged by |UpdateSchemeStyle()|. Nit: No || around function names (3 places) https://codereview.chromium.org/2641003002/diff/470001/components/omnibox/bro... File components/omnibox/browser/omnibox_view.cc (right): https://codereview.chromium.org/2641003002/diff/470001/components/omnibox/bro... components/omnibox/browser/omnibox_view.cc:235: DCHECK_NE(gfx::Range::InvalidRange(), scheme_range); Nit: DCHECK(scheme_range.IsValid()) ? https://codereview.chromium.org/2641003002/diff/470001/components/omnibox/bro... File components/omnibox/browser/omnibox_view.h (right): https://codereview.chromium.org/2641003002/diff/470001/components/omnibox/bro... components/omnibox/browser/omnibox_view.h:276: // Parses |display_text|, then invokes |SetEmphasis| and |UpdateSchemeStyle| Nit: |SetEmphasis| -> SetEmphasis(), and similar
Update: I have a working way of doing this "correctly"; it requires a couple precursor changes to land, which I'll get up for review tomorrow. It exposed that, for example, the current test for "partial URLs" doesn't really make sense, because we would never display such a URL while not editing, and when editing, such a string would not be marked with the "hostname" emphasized, because it would not be parsed as a URL. On the whole, it's a lot more work to set up than I expected, but it seems worth doing to hopefully avoid unexpectedly breaking these tests in the future, because their current behavior implicitly depends on things like whether the model thinks input is in progress, but doesn't make that as clear in the test. Or at least that's my self-justification... https://codereview.chromium.org/2641003002/diff/470001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc (right): https://codereview.chromium.org/2641003002/diff/470001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc:147: void SetAndEmphasizeText(const std::string& newText) { Nit: newText -> new_text
On 2017/03/03 09:17:14, Peter Kasting wrote: > Update: I have a working way of doing this "correctly"; it requires a couple > precursor changes to land, which I'll get up for review tomorrow. Oh, and if you're hoping to merge this CL to some previous branch, then I would suggest landing this CL in advance of my changes and having me patch as a followon (that won't be merged), since otherwise you'll have to pull back my precursor/cleanup changes as well.
elawrence@chromium.org changed reviewers: - eugenebut@chromium.org
Thanks for your feedback so far! rsesek@chromium.org: Please review the tiny change to ui/gfx/range/BUILD.gn. rohitrao@chromium.org: Please review changes in ios/chrome/browser/ui/omnibox/* > If you're hoping to merge this CL to some previous branch, > then I would suggest landing this CL in advance of my > changes Yes, this needs to go to M57. It's probably too late for M56 at this point. https://codereview.chromium.org/2641003002/diff/470001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h (right): https://codereview.chromium.org/2641003002/diff/470001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h:190: void SetEmphasis(bool emphasize, const gfx::Range& range) override; On 2017/03/03 02:31:08, Peter Kasting wrote: > Nit: Consider an "// OmniboxView:" header above these two to emphasize that > they're overrides, and where they came from Done. https://codereview.chromium.org/2641003002/diff/470001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc (right): https://codereview.chromium.org/2641003002/diff/470001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc:97: // Range of the last scheme logged by |UpdateSchemeStyle()|. On 2017/03/03 02:31:08, Peter Kasting wrote: > Nit: No || around function names (3 places) Done. https://codereview.chromium.org/2641003002/diff/470001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc:147: void SetAndEmphasizeText(const std::string& newText) { On 2017/03/03 09:17:14, Peter Kasting wrote: > Nit: newText -> new_text Done. https://codereview.chromium.org/2641003002/diff/470001/components/omnibox/bro... File components/omnibox/browser/omnibox_view.cc (right): https://codereview.chromium.org/2641003002/diff/470001/components/omnibox/bro... components/omnibox/browser/omnibox_view.cc:235: DCHECK_NE(gfx::Range::InvalidRange(), scheme_range); On 2017/03/03 02:31:08, Peter Kasting wrote: > Nit: DCHECK(scheme_range.IsValid()) ? Done. https://codereview.chromium.org/2641003002/diff/470001/components/omnibox/bro... File components/omnibox/browser/omnibox_view.h (right): https://codereview.chromium.org/2641003002/diff/470001/components/omnibox/bro... components/omnibox/browser/omnibox_view.h:276: // Parses |display_text|, then invokes |SetEmphasis| and |UpdateSchemeStyle| On 2017/03/03 02:31:10, Peter Kasting wrote: > Nit: |SetEmphasis| -> SetEmphasis(), and similar Done.
cocoa/ and ui/gfx/range/ LGTM
The CQ bit was checked by elawrence@chromium.org 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: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2641003002/diff/510001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm (right): https://codereview.chromium.org/2641003002/diff/510001/ios/chrome/browser/ui/... ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm:645: : NSMakeRange(0, [attributing_display_string_ length]); If the range is invalid, we emphasize everything instead of emphasizing nothing? Is this the behavior on other platforms too?
Thanks for the reviews, all! https://codereview.chromium.org/2641003002/diff/510001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm (right): https://codereview.chromium.org/2641003002/diff/510001/ios/chrome/browser/ui/... ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm:645: : NSMakeRange(0, [attributing_display_string_ length]); On 2017/03/03 21:53:51, rohitrao wrote: > If the range is invalid, we emphasize everything instead of emphasizing nothing? > Is this the behavior on other platforms too? Yes, if the range is invalid, we apply the color (black when |emphasize| is true, or grey when it's false) to the entire string. Yes, this matches other platforms, although the exact mechanism differs. On Views::View, SetColor() is used to set the color for the full text, while ApplyColor(range) is used to set the color only for a partial range of the text. In practice, I believe SetColor and ApplyColor(0,length) works out to be the same thing, but that's how this code worked before I got there.
The CQ bit was checked by elawrence@chromium.org
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": 530001, "attempt_start_ts": 1488578543760390,
"parent_rev": "16fe9a094fcfeacefe25e768918379cc2419d715", "commit_rev":
"486bb18560662755558c628e39ed8b3a3de272a2"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/486bb18560662755558c628e39ed... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:530001) as https://chromium.googlesource.com/chromium/src/+/486bb18560662755558c628e39ed... |
