Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1998493002:
Update omnibox chips in MD (Closed)
|
Created:
4 years, 7 months ago by Evan Stade Modified:
4 years, 7 months ago CC:
chromium-reviews, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpdate omnibox chips in MD
BUG=612983
Committed: https://crrev.com/397c9b9f1eb15e4bf7dff5d0d10d9727479f6f4a
Cr-Commit-Position: refs/heads/master@{#396176}
Patch Set 1 #Patch Set 2 : cleaned up #Patch Set 3 : ready #
Total comments: 1
Patch Set 4 : self review #Patch Set 5 : self review 2 #
Total comments: 27
Patch Set 6 : calculations #Patch Set 7 : fix keyword hint view #Patch Set 8 : no but really fix tab key #Patch Set 9 : rebase #Patch Set 10 : rebase on top of icon change; fix incognito colors #Patch Set 11 : slight refactor #
Total comments: 13
Patch Set 12 : rejigger #Patch Set 13 : remove extra include #
Total comments: 13
Patch Set 14 : remove unused #Patch Set 15 : . #Messages
Total messages: 50 (16 generated)
Description was changed from ========== WIP - updated omnibox chips in MD BUG=612983 ========== to ========== Update omnibox chips in MD BUG=612983 ==========
estade@chromium.org changed reviewers: + pkasting@chromium.org
https://codereview.chromium.org/1998493002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.h (left): https://codereview.chromium.org/1998493002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:132: int builtin_trailing_padding_; I believe this is no longer necessary given that the new icon set should all have the same internal padding. I can also remove gfx::GetVisibleMargins https://codereview.chromium.org/1998493002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/1998493002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:226: ui::MaterialDesignController::IsModeMaterial() ? 7 : 0; these constants are basically trial and error. The measurements in the specs were not possible to copy directly because of various other padding (such as the effective padding around text, which also varies with the actual characters being displayed). https://codereview.chromium.org/1998493002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.h (left): https://codereview.chromium.org/1998493002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:32: class IconLabelBubbleView : public views::InkDropHostView { Bubble is no longer the best terminology, but I'll defer changing that till MD is the D. https://codereview.chromium.org/1998493002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:132: int builtin_trailing_padding_; I believe this is no longer necessary given that the new icon set should all have the same internal padding.
https://codereview.chromium.org/1998493002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.h (left): https://codereview.chromium.org/1998493002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:32: class IconLabelBubbleView : public views::InkDropHostView { On 2016/05/19 21:20:45, Evan Stade wrote: > Bubble is no longer the best terminology, but I'll defer changing that till MD > is the D. this was supposed to be "...is the only D"
On 2016/05/19 21:24:49, Evan Stade wrote: > https://codereview.chromium.org/1998493002/diff/80001/chrome/browser/ui/views... > File chrome/browser/ui/views/location_bar/icon_label_bubble_view.h (left): > > https://codereview.chromium.org/1998493002/diff/80001/chrome/browser/ui/views... > chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:32: class > IconLabelBubbleView : public views::InkDropHostView { > On 2016/05/19 21:20:45, Evan Stade wrote: > > Bubble is no longer the best terminology, but I'll defer changing that till MD > > is the D. > > this was supposed to be "...is the only D" Another note -- incognito colors don't look amazingly good and I've pinged Max about that since they weren't really spec'd (at least not that I saw).
https://codereview.chromium.org/1998493002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (left): https://codereview.chromium.org/1998493002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:104: &builtin_trailing_padding_); If this call is removed, GetVisibleMargins() can be removed entirely; this was the only non-test caller. https://codereview.chromium.org/1998493002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/1998493002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:226: ui::MaterialDesignController::IsModeMaterial() ? 7 : 0; On 2016/05/19 21:20:45, Evan Stade wrote: > these constants are basically trial and error. The measurements in the specs > were not possible to copy directly because of various other padding (such as the > effective padding around text, which also varies with the actual characters > being displayed). The mock is 8 dp between text and divider line, right? I would probably make this value 8 if so; I would imagine some characters have a fully- or mostly-transparent pixel to their right and some don't, and we probably want to ensure "at least 8 px". Although, how does this space get apportioned across (pre-divider space, divider, post-divider space)? We want that total to be 17 dp (depending on the answer to the divider scaling question below), right? I guess I'm not sure how we get away with only 7 here. https://codereview.chromium.org/1998493002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:296: const int kPostSeparatorSpacing = 4; This is calculated so that with the additional spacing between items, it's 8 dp, right? Maybe this value should be "8 - (whatever constant we use for spacing between items)" then. https://codereview.chromium.org/1998493002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:298: const int kSeparatorHeight = 16; I wonder if this should be calculated based on the actual font metrics. It looked in the mock like this was the same height up and down as the normal ascent (i.e. no internal leading) and descent. Perhaps we should be taking the max distance either of those goes from the centerline and making that the "radius" of this divider? This would matter for e.g. Hindi or something where the substituted font may have very different metrics than the normal UI font. Or maybe we just want 16 px no matter what. https://codereview.chromium.org/1998493002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:306: separator_color); Will this draw a 2 px-thick line on scale factor 2? I'm wondering whether we want that, or we want a 1 px line at all scale factors. https://codereview.chromium.org/1998493002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/keyword_hint_view.cc (right): https://codereview.chromium.org/1998493002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/keyword_hint_view.cc:43: // views::Label Nit: Trailing colon https://codereview.chromium.org/1998493002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/1998493002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:586: GetEditLeadingInternalSpace()); Nit: I'd parenthesize this last subexpr just to avoid and potential confusion/precedence issues with ?: https://codereview.chromium.org/1998493002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/selected_keyword_view.cc (right): https://codereview.chromium.org/1998493002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/selected_keyword_view.cc:97: int keyword_text_ids = ui::MaterialDesignController::IsModeMaterial() Nit: "ids" here looks to me more like "IDs" than "IDS" -- I might just call this "id". That matches other code where we e.g. use "id" instead of "idr" in graphic resource variable names.
https://codereview.chromium.org/1998493002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (left): https://codereview.chromium.org/1998493002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:104: &builtin_trailing_padding_); On 2016/05/20 20:54:11, Peter Kasting wrote: > If this call is removed, GetVisibleMargins() can be removed entirely; this was > the only non-test caller. yes, as noted above. The only reason I haven't done it yet is that I didn't want to go through with the hassle until this removal was approved by you. https://codereview.chromium.org/1998493002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/1998493002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:226: ui::MaterialDesignController::IsModeMaterial() ? 7 : 0; On 2016/05/20 20:54:11, Peter Kasting wrote: > On 2016/05/19 21:20:45, Evan Stade wrote: > > these constants are basically trial and error. The measurements in the specs > > were not possible to copy directly because of various other padding (such as > the > > effective padding around text, which also varies with the actual characters > > being displayed). > > The mock is 8 dp between text and divider line, right? I would probably make > this value 8 if so; I would imagine some characters have a fully- or > mostly-transparent pixel to their right and some don't, and we probably want to > ensure "at least 8 px". > > Although, how does this space get apportioned across (pre-divider space, > divider, post-divider space)? We want that total to be 17 dp (depending on the > answer to the divider scaling question below), right? I guess I'm not sure how > we get away with only 7 here. The way the mocks measure the padding abuts the readlines too closely to the text. 7 gets us closer to the effective appearance represented by the mocks. Thus we really only want to get to 15. We are still padding by ICON_LABEL_VIEW_TRAILING_PADDING, i.e. 6, so we're adding 13 in total. 2 more come from the spacing between the "bubble" and omnibox view. That makes 15 total. It would be a little simpler if I could move that 2 into ICON_LABEL_VIEW_TRAILING_PADDING, but I can't because that would affect the padding before the icon as well. https://codereview.chromium.org/1998493002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:296: const int kPostSeparatorSpacing = 4; On 2016/05/20 20:54:11, Peter Kasting wrote: > This is calculated so that with the additional spacing between items, it's 8 dp, > right? > > Maybe this value should be "8 - (whatever constant we use for spacing between > items)" then. well, not quite. The mocks seem to assume less internal padding inside the text fields / labels than reality. So this would need to be ~7 - 1 - some constant. That constant is 2, the magic number from LocationBarView::Layout. The reason I didn't want to share this number and use math is because in the course of creating this CL I found it much more confusing to read calculations and treacherous to change values that are used in multiple places, rather than just having a few hardcoded constants (especially given that the mathy approach generally doesn't reduce the number of hardcoded values). https://codereview.chromium.org/1998493002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:298: const int kSeparatorHeight = 16; On 2016/05/20 20:54:11, Peter Kasting wrote: > I wonder if this should be calculated based on the actual font metrics. It > looked in the mock like this was the same height up and down as the normal > ascent (i.e. no internal leading) and descent. Perhaps we should be taking the > max distance either of those goes from the centerline and making that the > "radius" of this divider? > > This would matter for e.g. Hindi or something where the substituted font may > have very different metrics than the normal UI font. > > Or maybe we just want 16 px no matter what. Not sure what you want me to do at the moment. Shall I file a bug to follow up on this? https://codereview.chromium.org/1998493002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:306: separator_color); On 2016/05/20 20:54:11, Peter Kasting wrote: > Will this draw a 2 px-thick line on scale factor 2? I'm wondering whether we > want that, or we want a 1 px line at all scale factors. I think we want 1dp, i.e. 2px on scale factor 2. The 200% mocks do have 2px width. I'll ping Max. https://codereview.chromium.org/1998493002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/keyword_hint_view.cc (right): https://codereview.chromium.org/1998493002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/keyword_hint_view.cc:43: // views::Label On 2016/05/20 20:54:11, Peter Kasting wrote: > Nit: Trailing colon Done. https://codereview.chromium.org/1998493002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/1998493002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:586: GetEditLeadingInternalSpace()); On 2016/05/20 20:54:11, Peter Kasting wrote: > Nit: I'd parenthesize this last subexpr just to avoid and potential > confusion/precedence issues with ?: Done. https://codereview.chromium.org/1998493002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/selected_keyword_view.cc (right): https://codereview.chromium.org/1998493002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/selected_keyword_view.cc:97: int keyword_text_ids = ui::MaterialDesignController::IsModeMaterial() On 2016/05/20 20:54:11, Peter Kasting wrote: > Nit: "ids" here looks to me more like "IDs" than "IDS" -- I might just call this > "id". That matches other code where we e.g. use "id" instead of "idr" in > graphic resource variable names. Done.
https://codereview.chromium.org/1998493002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/1998493002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:226: ui::MaterialDesignController::IsModeMaterial() ? 7 : 0; On 2016/05/20 22:33:53, Evan Stade wrote: > On 2016/05/20 20:54:11, Peter Kasting wrote: > > On 2016/05/19 21:20:45, Evan Stade wrote: > > > these constants are basically trial and error. The measurements in the specs > > > were not possible to copy directly because of various other padding (such as > > the > > > effective padding around text, which also varies with the actual characters > > > being displayed). > > > > The mock is 8 dp between text and divider line, right? I would probably make > > this value 8 if so; I would imagine some characters have a fully- or > > mostly-transparent pixel to their right and some don't, and we probably want > to > > ensure "at least 8 px". > > > > Although, how does this space get apportioned across (pre-divider space, > > divider, post-divider space)? We want that total to be 17 dp (depending on > the > > answer to the divider scaling question below), right? I guess I'm not sure > how > > we get away with only 7 here. > > The way the mocks measure the padding abuts the readlines too closely to the > text. 7 gets us closer to the effective appearance represented by the mocks. > Thus we really only want to get to 15. > > We are still padding by ICON_LABEL_VIEW_TRAILING_PADDING, i.e. 6, so we're > adding 13 in total. 2 more come from the spacing between the "bubble" and > omnibox view. That makes 15 total. > > It would be a little simpler if I could move that 2 into > ICON_LABEL_VIEW_TRAILING_PADDING, but I can't because that would affect the > padding before the icon as well. Is that because that same constant is used as the initial padding? Can we just split that into two separate constants, and then move the 2 as you suggest? https://codereview.chromium.org/1998493002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:296: const int kPostSeparatorSpacing = 4; On 2016/05/20 22:33:53, Evan Stade wrote: > On 2016/05/20 20:54:11, Peter Kasting wrote: > > This is calculated so that with the additional spacing between items, it's 8 > dp, > > right? > > > > Maybe this value should be "8 - (whatever constant we use for spacing between > > items)" then. > > well, not quite. The mocks seem to assume less internal padding inside the text > fields / labels than reality. So this would need to be ~7 - 1 - some constant. > That constant is 2, the magic number from LocationBarView::Layout. The reason I > didn't want to share this number and use math is because in the course of > creating this CL I found it much more confusing to read calculations and > treacherous to change values that are used in multiple places, rather than just > having a few hardcoded constants (especially given that the mathy approach > generally doesn't reduce the number of hardcoded values). Totally agree that calculations can be confusing and hard to read. I still kind of like them, and the reason is because they make explicit in the code the implicit dependencies between the values of particular variable and the places affected by them. I ran into a lot of frame and location bar bugs during the MD work that had happened because we changed the constants that obviously affected one location and didn't realize this actually meant we needed to change some constants in a different class. Not a few of these had to do with the old version of these chips -- e.g. over time people had put in fixes for the EV bubble that actually broke some stuff about keyword search, and then the keyword search bubble worked around that, but no one had ever fixed the blocked popup bubble which had the same problem, etc. So if this implicitly depends on that other value I'd kind of rather make it explicitly depend on it even if it's ugly. Another route could be to change how the parent of these objects (the LocationBarView) lays them out to not put any external padding around them and instead expect the code here to do everything. That would let you just use a single constant here. The question mark would be (1) how feasible that is and (2) whether there's really still an implicit dependency, where if in the future we change the padding between most items, we really want to change all the padding values here too. https://codereview.chromium.org/1998493002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:298: const int kSeparatorHeight = 16; On 2016/05/20 22:33:53, Evan Stade wrote: > On 2016/05/20 20:54:11, Peter Kasting wrote: > > I wonder if this should be calculated based on the actual font metrics. It > > looked in the mock like this was the same height up and down as the normal > > ascent (i.e. no internal leading) and descent. Perhaps we should be taking > the > > max distance either of those goes from the centerline and making that the > > "radius" of this divider? > > > > This would matter for e.g. Hindi or something where the substituted font may > > have very different metrics than the normal UI font. > > > > Or maybe we just want 16 px no matter what. > > Not sure what you want me to do at the moment. Shall I file a bug to follow up > on this? I'd just ask Max, and assume until he answers that hardcoding 16 is fine.
estade@chromium.org changed reviewers: + sky@chromium.org
I also changed the coloration of the separator based on discussions with Max. Removing GetVisibleMargins is still TODO. +sky for ui/gfx https://codereview.chromium.org/1998493002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/1998493002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:226: ui::MaterialDesignController::IsModeMaterial() ? 7 : 0; On 2016/05/20 22:44:56, Peter Kasting wrote: > On 2016/05/20 22:33:53, Evan Stade wrote: > > On 2016/05/20 20:54:11, Peter Kasting wrote: > > > On 2016/05/19 21:20:45, Evan Stade wrote: > > > > these constants are basically trial and error. The measurements in the > specs > > > > were not possible to copy directly because of various other padding (such > as > > > the > > > > effective padding around text, which also varies with the actual > characters > > > > being displayed). > > > > > > The mock is 8 dp between text and divider line, right? I would probably > make > > > this value 8 if so; I would imagine some characters have a fully- or > > > mostly-transparent pixel to their right and some don't, and we probably want > > to > > > ensure "at least 8 px". > > > > > > Although, how does this space get apportioned across (pre-divider space, > > > divider, post-divider space)? We want that total to be 17 dp (depending on > > the > > > answer to the divider scaling question below), right? I guess I'm not sure > > how > > > we get away with only 7 here. > > > > The way the mocks measure the padding abuts the readlines too closely to the > > text. 7 gets us closer to the effective appearance represented by the mocks. > > Thus we really only want to get to 15. > > > > We are still padding by ICON_LABEL_VIEW_TRAILING_PADDING, i.e. 6, so we're > > adding 13 in total. 2 more come from the spacing between the "bubble" and > > omnibox view. That makes 15 total. > > > > It would be a little simpler if I could move that 2 into > > ICON_LABEL_VIEW_TRAILING_PADDING, but I can't because that would affect the > > padding before the icon as well. > > Is that because that same constant is used as the initial padding? Can we just > split that into two separate constants, and then move the 2 as you suggest? OK, I tried that, but it messed up the blocked popup chip spacing (that chip/bubble being followed by the bookmark star). So I left the 2 in place but made it more mathy. https://codereview.chromium.org/1998493002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:296: const int kPostSeparatorSpacing = 4; On 2016/05/20 22:44:56, Peter Kasting wrote: > On 2016/05/20 22:33:53, Evan Stade wrote: > > On 2016/05/20 20:54:11, Peter Kasting wrote: > > > This is calculated so that with the additional spacing between items, it's 8 > > dp, > > > right? > > > > > > Maybe this value should be "8 - (whatever constant we use for spacing > between > > > items)" then. > > > > well, not quite. The mocks seem to assume less internal padding inside the > text > > fields / labels than reality. So this would need to be ~7 - 1 - some constant. > > That constant is 2, the magic number from LocationBarView::Layout. The reason > I > > didn't want to share this number and use math is because in the course of > > creating this CL I found it much more confusing to read calculations and > > treacherous to change values that are used in multiple places, rather than > just > > having a few hardcoded constants (especially given that the mathy approach > > generally doesn't reduce the number of hardcoded values). > > Totally agree that calculations can be confusing and hard to read. > > I still kind of like them, and the reason is because they make explicit in the > code the implicit dependencies between the values of particular variable and the > places affected by them. I ran into a lot of frame and location bar bugs during > the MD work that had happened because we changed the constants that obviously > affected one location and didn't realize this actually meant we needed to change > some constants in a different class. Not a few of these had to do with the old > version of these chips -- e.g. over time people had put in fixes for the EV > bubble that actually broke some stuff about keyword search, and then the keyword > search bubble worked around that, but no one had ever fixed the blocked popup > bubble which had the same problem, etc. > > So if this implicitly depends on that other value I'd kind of rather make it > explicitly depend on it even if it's ugly. > > Another route could be to change how the parent of these objects (the > LocationBarView) lays them out to not put any external padding around them and > instead expect the code here to do everything. That would let you just use a > single constant here. The question mark would be (1) how feasible that is and > (2) whether there's really still an implicit dependency, where if in the future > we change the padding between most items, we really want to change all the > padding values here too. made this mathy, ptal? https://codereview.chromium.org/1998493002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:298: const int kSeparatorHeight = 16; On 2016/05/20 22:44:57, Peter Kasting wrote: > On 2016/05/20 22:33:53, Evan Stade wrote: > > On 2016/05/20 20:54:11, Peter Kasting wrote: > > > I wonder if this should be calculated based on the actual font metrics. It > > > looked in the mock like this was the same height up and down as the normal > > > ascent (i.e. no internal leading) and descent. Perhaps we should be taking > > the > > > max distance either of those goes from the centerline and making that the > > > "radius" of this divider? > > > > > > This would matter for e.g. Hindi or something where the substituted font may > > > have very different metrics than the normal UI font. > > > > > > Or maybe we just want 16 px no matter what. > > > > Not sure what you want me to do at the moment. Shall I file a bug to follow up > > on this? > > I'd just ask Max, and assume until he answers that hardcoding 16 is fine. he seems to want exactly 16 https://codereview.chromium.org/1998493002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:306: separator_color); On 2016/05/20 22:33:53, Evan Stade wrote: > On 2016/05/20 20:54:11, Peter Kasting wrote: > > Will this draw a 2 px-thick line on scale factor 2? I'm wondering whether we > > want that, or we want a 1 px line at all scale factors. > > I think we want 1dp, i.e. 2px on scale factor 2. The 200% mocks do have 2px > width. I'll ping Max. ok, 1px it is
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1998493002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1998493002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
LGTM
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1998493002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1998493002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1998493002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1998493002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1998493002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/1998493002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:305: views::Textfield::kTextPadding; Is this calculation correct for the blocked popup case, which isn't followed by a location bar? I would have expected to maybe need a different value there. https://codereview.chromium.org/1998493002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/keyword_hint_view.cc (right): https://codereview.chromium.org/1998493002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/keyword_hint_view.cc:85: inverted ? SK_ColorWHITE : SkColorSetA(text_color, 0x13); I assume this was explicit direction from Max? Kinda curious what using the latter value on dark backgrounds would have looked like, but I imagine you guys already went through that. https://codereview.chromium.org/1998493002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/1998493002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_view.cc:363: return GetColor(EV_BUBBLE_TEXT_AND_BORDER); Nit: Shorter, and avoids else after return: if (security_level != security_state::SecurityStateModel::SECURITY_ERROR) return md ? gfx::kGoogleGreen700 : GetColor(EV_BUBBLE_TEXT_AND_BORDER); text_color = md ? gfx::kGoogleRed700 : SkColorSetRGB(162, 0, 0); https://codereview.chromium.org/1998493002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_view.cc:576: GetEditLeadingInternalSpace())); Won't not factoring in GetEditLeadingInternalSpace() in MD mean the distance to the edit is off by 1 in RTL vs. LTR? https://codereview.chromium.org/1998493002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/selected_keyword_view.cc (right): https://codereview.chromium.org/1998493002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/selected_keyword_view.cc:61: ? ui::NativeTheme::kColorId_TextfieldDefaultColor Was this the final outcome of your discussions with Max? Seems like it might be nice to have some kind of blue in this color -- did you guys consider using GetReadableColor()?
https://codereview.chromium.org/1998493002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/1998493002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:305: views::Textfield::kTextPadding; On 2016/05/25 00:03:27, Peter Kasting wrote: > Is this calculation correct for the blocked popup case, which isn't followed by > a location bar? I would have expected to maybe need a different value there. I found a different way of doing this that is perhaps more intuitively appealing. https://codereview.chromium.org/1998493002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/keyword_hint_view.cc (right): https://codereview.chromium.org/1998493002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/keyword_hint_view.cc:85: inverted ? SK_ColorWHITE : SkColorSetA(text_color, 0x13); On 2016/05/25 00:03:27, Peter Kasting wrote: > I assume this was explicit direction from Max? Kinda curious what using the > latter value on dark backgrounds would have looked like, but I imagine you guys > already went through that. No, this didn't change. This is pre-existing code, just moved from where it used to reside in IconLabelBubbleView. https://codereview.chromium.org/1998493002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/1998493002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_view.cc:363: return GetColor(EV_BUBBLE_TEXT_AND_BORDER); On 2016/05/25 00:03:27, Peter Kasting wrote: > Nit: Shorter, and avoids else after return: > > if (security_level != security_state::SecurityStateModel::SECURITY_ERROR) > return md ? gfx::kGoogleGreen700 : GetColor(EV_BUBBLE_TEXT_AND_BORDER); > text_color = md ? gfx::kGoogleRed700 : SkColorSetRGB(162, 0, 0); good suggestion. Since we're switching on md so much I decided to rearrange so md and non-md are different branches. I think this is clearer and makes it easier to remove pre-MD code when the time comes. https://codereview.chromium.org/1998493002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_view.cc:576: GetEditLeadingInternalSpace())); On 2016/05/25 00:03:27, Peter Kasting wrote: > Won't not factoring in GetEditLeadingInternalSpace() in MD mean the distance to > the edit is off by 1 in RTL vs. LTR? changed RTL is tricky to get precisely correct because it seems that the padding depends on the directionality and alignment. LTR text that is right aligned (such as a URL) has different padding than RTL text that is right aligned (like a typed search query). The current code is not perfect (spacing around the separator is a couple pixels extra) but it looks pretty good, at least as good as pre-MD. https://codereview.chromium.org/1998493002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/selected_keyword_view.cc (right): https://codereview.chromium.org/1998493002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/selected_keyword_view.cc:61: ? ui::NativeTheme::kColorId_TextfieldDefaultColor On 2016/05/25 00:03:27, Peter Kasting wrote: > Was this the final outcome of your discussions with Max? yes. I've pinged Max again to confirm. > Seems like it might be > nice to have some kind of blue in this color -- did you guys consider using > GetReadableColor()? How do you feel about the lack of green in the incognito ev chip? I don't think we'd need to rely on GetReadableColor, we could just hand pick a good shade of blue. But I kind of like the white because it has the best contrast.
https://codereview.chromium.org/1998493002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/keyword_hint_view.cc (right): https://codereview.chromium.org/1998493002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/keyword_hint_view.cc:85: inverted ? SK_ColorWHITE : SkColorSetA(text_color, 0x13); On 2016/05/25 20:03:44, Evan Stade wrote: > On 2016/05/25 00:03:27, Peter Kasting wrote: > > I assume this was explicit direction from Max? Kinda curious what using the > > latter value on dark backgrounds would have looked like, but I imagine you > guys > > already went through that. > > No, this didn't change. This is pre-existing code, just moved from where it used > to reside in IconLabelBubbleView. Ah. Well, it's fine to leave this way or change, either way. Might want to let Max or Sebastien know about this in case they prefer one or the other. https://codereview.chromium.org/1998493002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/selected_keyword_view.cc (right): https://codereview.chromium.org/1998493002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/selected_keyword_view.cc:61: ? ui::NativeTheme::kColorId_TextfieldDefaultColor On 2016/05/25 20:03:44, Evan Stade wrote: > On 2016/05/25 00:03:27, Peter Kasting wrote: > > Seems like it might be > > nice to have some kind of blue in this color -- did you guys consider using > > GetReadableColor()? > > How do you feel about the lack of green in the incognito ev chip? I don't think > we'd need to rely on GetReadableColor, we could just hand pick a good shade of > blue. But I kind of like the white because it has the best contrast. The security guys may feel more strongly than I do. I think we could use white if the other options are bad, but it would be nice to have green for this if it's feasible. The nice thing about GetReadableColor() is that it ought to transparently handle it if we make the omnibox responsive to the system color scheme, but I'd be OK with hardcoding things too. https://codereview.chromium.org/1998493002/diff/240001/chrome/browser/ui/layo... File chrome/browser/ui/layout_constants.h (right): https://codereview.chromium.org/1998493002/diff/240001/chrome/browser/ui/layo... chrome/browser/ui/layout_constants.h:26: LOCATION_BAR_PRE_EDIT_SPACING, Seems like this constant is no longer used. https://codereview.chromium.org/1998493002/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/1998493002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:316: gfx::RectF bounds(GetLocalBounds()); Can using float-based bounds here result in drawing the separator in between two pixels (and thus antialiased across both) instead of snapped to the grid in some cases? https://codereview.chromium.org/1998493002/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/1998493002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_view.cc:365: text_color = gfx::kGoogleGreen700; This is a behavior change -- the old code returned this directly, the new calls GetReadableColor() first. Is that OK? https://codereview.chromium.org/1998493002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_view.cc:370: text_color = GetColor(EV_BUBBLE_TEXT_AND_BORDER); Same behavior change.
https://codereview.chromium.org/1998493002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/selected_keyword_view.cc (right): https://codereview.chromium.org/1998493002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/selected_keyword_view.cc:61: ? ui::NativeTheme::kColorId_TextfieldDefaultColor On 2016/05/25 20:23:02, Peter Kasting wrote: > On 2016/05/25 20:03:44, Evan Stade wrote: > > On 2016/05/25 00:03:27, Peter Kasting wrote: > > > Seems like it might be > > > nice to have some kind of blue in this color -- did you guys consider using > > > GetReadableColor()? > > > > How do you feel about the lack of green in the incognito ev chip? I don't > think > > we'd need to rely on GetReadableColor, we could just hand pick a good shade of > > blue. But I kind of like the white because it has the best contrast. > > The security guys may feel more strongly than I do. I think we could use white > if the other options are bad, but it would be nice to have green for this if > it's feasible. > > The nice thing about GetReadableColor() is that it ought to transparently handle > it if we make the omnibox responsive to the system color scheme, but I'd be OK > with hardcoding things too. Right, and the bad thing about GetReadableColor is that it isn't GetGoodLookingAndReadableColor :) https://codereview.chromium.org/1998493002/diff/240001/chrome/browser/ui/layo... File chrome/browser/ui/layout_constants.h (right): https://codereview.chromium.org/1998493002/diff/240001/chrome/browser/ui/layo... chrome/browser/ui/layout_constants.h:26: LOCATION_BAR_PRE_EDIT_SPACING, On 2016/05/25 20:23:03, Peter Kasting wrote: > Seems like this constant is no longer used. Done. https://codereview.chromium.org/1998493002/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/1998493002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:316: gfx::RectF bounds(GetLocalBounds()); On 2016/05/25 20:23:03, Peter Kasting wrote: > Can using float-based bounds here result in drawing the separator in between two > pixels (and thus antialiased across both) instead of snapped to the grid in some > cases? no because there's no anti aliasing, also it's hairline width. https://codereview.chromium.org/1998493002/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/1998493002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_view.cc:365: text_color = gfx::kGoogleGreen700; On 2016/05/25 20:23:03, Peter Kasting wrote: > This is a behavior change -- the old code returned this directly, the new calls > GetReadableColor() first. Is that OK? It's a behavior change relative to the previous patchset, not to the existing code (right?). The behavior change between the existing code and the previous patchset was the mistake. I don't actually think GetReadableColor ever does anything here (in MD) because we're returning directly in the inverted case.
LGTM https://codereview.chromium.org/1998493002/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/1998493002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:316: gfx::RectF bounds(GetLocalBounds()); On 2016/05/25 21:03:26, Evan Stade wrote: > On 2016/05/25 20:23:03, Peter Kasting wrote: > > Can using float-based bounds here result in drawing the separator in between > two > > pixels (and thus antialiased across both) instead of snapped to the grid in > some > > cases? > > no because there's no anti aliasing, also it's hairline width. Since there's no code here that explicitly disables AA, and I'm not sure which side a line that's directly on the pixel divider will be snapped to, does it make sense to explicitly compute integer drawing coords anyway? I would find that clearer (and it would avoid the need to add overrides to Canvas). Basically, I think all that would be needed would be to call ToEnclosingRect() or ToEnclosedRect() (depending on which snapping direction you wanted) on |scaled_bounds| right before the DrawLine() call. https://codereview.chromium.org/1998493002/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/1998493002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_view.cc:365: text_color = gfx::kGoogleGreen700; On 2016/05/25 21:03:26, Evan Stade wrote: > On 2016/05/25 20:23:03, Peter Kasting wrote: > > This is a behavior change -- the old code returned this directly, the new > calls > > GetReadableColor() first. Is that OK? > > It's a behavior change relative to the previous patchset, not to the existing > code (right?). The behavior change between the existing code and the previous > patchset was the mistake. Ah, you're right.
https://codereview.chromium.org/1998493002/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/1998493002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:316: gfx::RectF bounds(GetLocalBounds()); On 2016/05/25 21:24:17, Peter Kasting wrote: > On 2016/05/25 21:03:26, Evan Stade wrote: > > On 2016/05/25 20:23:03, Peter Kasting wrote: > > > Can using float-based bounds here result in drawing the separator in between > > two > > > pixels (and thus antialiased across both) instead of snapped to the grid in > > some > > > cases? > > > > no because there's no anti aliasing, also it's hairline width. > > Since there's no code here that explicitly disables AA, and I'm not sure which > side a line that's directly on the pixel divider will be snapped to, does it > make sense to explicitly compute integer drawing coords anyway? I would find > that clearer (and it would avoid the need to add overrides to Canvas). > > Basically, I think all that would be needed would be to call ToEnclosingRect() > or ToEnclosedRect() (depending on which snapping direction you wanted) on > |scaled_bounds| right before the DrawLine() call. Don't you mean explicitly compute non-integer coordinates? Because whole numbers are between the pixels whereas X.5 is the middle of the (X+1)th pixel from the left. There are many places where we rely on the snapping behavior already (everywhere that uses the integer version of DrawLine, for example). I think it would make more sense to make gfx::Canvas::DrawLine explicitly disable anti aliasing, except since the default is not to AA, that also feels wrong.
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1998493002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1998493002/260001
https://codereview.chromium.org/1998493002/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/1998493002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:316: gfx::RectF bounds(GetLocalBounds()); On 2016/05/25 21:43:08, Evan Stade wrote: > On 2016/05/25 21:24:17, Peter Kasting wrote: > > On 2016/05/25 21:03:26, Evan Stade wrote: > > > On 2016/05/25 20:23:03, Peter Kasting wrote: > > > > Can using float-based bounds here result in drawing the separator in > between > > > two > > > > pixels (and thus antialiased across both) instead of snapped to the grid > in > > > some > > > > cases? > > > > > > no because there's no anti aliasing, also it's hairline width. > > > > Since there's no code here that explicitly disables AA, and I'm not sure which > > side a line that's directly on the pixel divider will be snapped to, does it > > make sense to explicitly compute integer drawing coords anyway? I would find > > that clearer (and it would avoid the need to add overrides to Canvas). > > > > Basically, I think all that would be needed would be to call ToEnclosingRect() > > or ToEnclosedRect() (depending on which snapping direction you wanted) on > > |scaled_bounds| right before the DrawLine() call. > > Don't you mean explicitly compute non-integer coordinates? Because whole numbers > are between the pixels whereas X.5 is the middle of the (X+1)th pixel from the > left. Does that apply to Canvas::DrawLine()? I thought that, given the integer coordinates that currently takes, it handles conversion to the appropriate pixel grid, whereas if you're constructing e.g. an SkPath you need to worry about this yourself. That said, in most places I use the Canvas functions, I draw rects (of 1 px width) instead of lines, in part because the way the coordinates work is clearer. I guess what I'm really saying in all this is, I'm not worried about your code having bugs, and trying to prevent those bugs. I'm worried that, when I read your code, it's doing something different than everywhere else (calling DrawLine() with floating-point coordinates), it looks like sometimes the coordinates might be at 0.5 and sometimes at 0.0, and it's not obvious that both of those distinct cases are intended and how they'll be handled. Whereas if you use e.g. ToEnclosingRect(), I know exactly what the code intends without having to investigate how Skia handles fractional coordinates or snaps to the pixel grid.
https://codereview.chromium.org/1998493002/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/1998493002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:316: gfx::RectF bounds(GetLocalBounds()); On 2016/05/25 21:54:12, Peter Kasting wrote: > On 2016/05/25 21:43:08, Evan Stade wrote: > > On 2016/05/25 21:24:17, Peter Kasting wrote: > > > On 2016/05/25 21:03:26, Evan Stade wrote: > > > > On 2016/05/25 20:23:03, Peter Kasting wrote: > > > > > Can using float-based bounds here result in drawing the separator in > > between > > > > two > > > > > pixels (and thus antialiased across both) instead of snapped to the grid > > in > > > > some > > > > > cases? > > > > > > > > no because there's no anti aliasing, also it's hairline width. > > > > > > Since there's no code here that explicitly disables AA, and I'm not sure > which > > > side a line that's directly on the pixel divider will be snapped to, does it > > > make sense to explicitly compute integer drawing coords anyway? I would > find > > > that clearer (and it would avoid the need to add overrides to Canvas). > > > > > > Basically, I think all that would be needed would be to call > ToEnclosingRect() > > > or ToEnclosedRect() (depending on which snapping direction you wanted) on > > > |scaled_bounds| right before the DrawLine() call. > > > > Don't you mean explicitly compute non-integer coordinates? Because whole > numbers > > are between the pixels whereas X.5 is the middle of the (X+1)th pixel from the > > left. > > Does that apply to Canvas::DrawLine()? I thought that, given the integer > coordinates that currently takes, it handles conversion to the appropriate pixel > grid, whereas if you're constructing e.g. an SkPath you need to worry about this > yourself. I don't think there's any explicit handling, DrawLine just calls SkCanvas::drawLine and wraps the integers with SkIntToScalar. (Also, I was wrong about hairline, it explicitly sets stroke width to 1.) > > That said, in most places I use the Canvas functions, I draw rects (of 1 px > width) instead of lines, in part because the way the coordinates work is > clearer. > > I guess what I'm really saying in all this is, I'm not worried about your code > having bugs, and trying to prevent those bugs. I'm worried that, when I read > your code, it's doing something different than everywhere else (calling > DrawLine() with floating-point coordinates), it looks like sometimes the > coordinates might be at 0.5 and sometimes at 0.0, and it's not obvious that both > of those distinct cases are intended and how they'll be handled. Whereas if you > use e.g. ToEnclosingRect(), I know exactly what the code intends without having > to investigate how Skia handles fractional coordinates or snaps to the pixel > grid. so I just tried this and ToEnclosedRect does the right thing in LTR and the wrong thing in RTL. ToEnclosingRect does the wrong thing in LTR.
https://codereview.chromium.org/1998493002/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/1998493002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:316: gfx::RectF bounds(GetLocalBounds()); On 2016/05/25 22:32:43, Evan Stade wrote: > On 2016/05/25 21:54:12, Peter Kasting wrote: > > I guess what I'm really saying in all this is, I'm not worried about your code > > having bugs, and trying to prevent those bugs. I'm worried that, when I read > > your code, it's doing something different than everywhere else (calling > > DrawLine() with floating-point coordinates), it looks like sometimes the > > coordinates might be at 0.5 and sometimes at 0.0, and it's not obvious that > both > > of those distinct cases are intended and how they'll be handled. Whereas if > you > > use e.g. ToEnclosingRect(), I know exactly what the code intends without > having > > to investigate how Skia handles fractional coordinates or snaps to the pixel > > grid. > > so I just tried this and ToEnclosedRect does the right thing in LTR and the > wrong thing in RTL. ToEnclosingRect does the wrong thing in LTR. I'm kind of lost by this point as to what snapping is desirable in which modes. Do you get the right answer if you just replace all the floats with ints in this code (like you originally had it)? Is there any way at all of writing this so the reader can see precisely how we allocate extra space in which directionalities/scale factors?
> Do you get the right answer if you just replace all the floats with ints in this > code (like you originally had it)? no, that's why I changed it :) > Is there any way at all of writing this so > the reader can see precisely how we allocate extra space in which > directionalities/scale factors? do you prefer the latest patchset? It does the same thing.
Dry run: None
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1998493002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1998493002/280001
LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Dry run: None
The CQ bit was checked by estade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org Link to the patchset: https://codereview.chromium.org/1998493002/#ps280001 (title: ".")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1998493002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1998493002/280001
Message was sent while issue was closed.
Description was changed from ========== Update omnibox chips in MD BUG=612983 ========== to ========== Update omnibox chips in MD BUG=612983 ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== Update omnibox chips in MD BUG=612983 ========== to ========== Update omnibox chips in MD BUG=612983 Committed: https://crrev.com/397c9b9f1eb15e4bf7dff5d0d10d9727479f6f4a Cr-Commit-Position: refs/heads/master@{#396176} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/397c9b9f1eb15e4bf7dff5d0d10d9727479f6f4a Cr-Commit-Position: refs/heads/master@{#396176} |
