Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1826653002:
Draw infobar arrow border with width of 1px on any scale factor. (Closed)
|
Created:
4 years, 9 months ago by Evan Stade Modified:
4 years, 8 months ago Reviewers:
Peter Kasting CC:
chromium-reviews, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDraw infobar arrow border with width of 1px on any scale factor.
BUG=597069
Committed: https://crrev.com/1d9418afc9ddb2254d7c9316ea3043e3e6ff1ffd
Cr-Commit-Position: refs/heads/master@{#384953}
Patch Set 1 #Patch Set 2 : also deal with bottom separator #
Total comments: 6
Patch Set 3 : unscale, md/non-md #
Total comments: 3
Patch Set 4 : fix comment typo #
Total comments: 6
Patch Set 5 : move pixel adjustment to bg painter #Patch Set 6 : move path calc #
Total comments: 20
Patch Set 7 : pkasting review #Patch Set 8 : fix pre-md bug #Patch Set 9 : git commit #
Total comments: 9
Patch Set 10 : pkasting review #Patch Set 11 : format #
Messages
Total messages: 32 (6 generated)
estade@chromium.org changed reviewers: + pkasting@chromium.org
hold on, don't review yet
Be careful using hairlines. Last time I checked Skia had interesting bugs with them. Usually better to unscale and then draw at width 1.
The latest patch seems to work in all configurations (and uses hairlines). Screenshots posted to bug. Do you remember what bugs you ran into before?
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/1826653002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1826653002/20001
https://codereview.chromium.org/1826653002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/infobars/infobar_background.cc (right): https://codereview.chromium.org/1826653002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/infobars/infobar_background.cc:45: paint.setStrokeCap(SkPaint::kRound_Cap); Why do we need to set the cap for a hairline? https://codereview.chromium.org/1826653002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/infobars/infobar_view.cc (right): https://codereview.chromium.org/1826653002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/infobars/infobar_view.cc:200: stroke_path_.rLineTo(arrow_fill_half_width, arrow_fill_height); I don't think this code is technically correct in non-1x scales, because e.g. the half-pixel offset will get scaled to one pixel and we'll wind up with a path whose coordinate is no longer in the pixel center. I think what we need to do here is the same sort of thing I do for the tab outlines: we need to make a path that assumes it will be drawn unscaled, then at the drawing site we need to unscale and draw the path. As a side effect this will also mean we can avoid hairlines since we'll always be drawing unscaled and can just use a 1 px thickness. https://codereview.chromium.org/1826653002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/infobars/infobar_view.cc:204: // edge of the arrow than on the left. Now that I know more about Skia than when I wrote this, the right way to compute the fill is probably the way we do it for the MD tabstrip: start with the same path as the stroke, but then use Op() from SkPathOps.h to subtract the stroked stroke path away. This would avoid the need for an epsilon like this.
On 2016/03/22 23:39:19, Peter Kasting wrote: > https://codereview.chromium.org/1826653002/diff/20001/chrome/browser/ui/views... > File chrome/browser/ui/views/infobars/infobar_background.cc (right): > > https://codereview.chromium.org/1826653002/diff/20001/chrome/browser/ui/views... > chrome/browser/ui/views/infobars/infobar_background.cc:45: > paint.setStrokeCap(SkPaint::kRound_Cap); > Why do we need to set the cap for a hairline? > > https://codereview.chromium.org/1826653002/diff/20001/chrome/browser/ui/views... > File chrome/browser/ui/views/infobars/infobar_view.cc (right): > > https://codereview.chromium.org/1826653002/diff/20001/chrome/browser/ui/views... > chrome/browser/ui/views/infobars/infobar_view.cc:200: > stroke_path_.rLineTo(arrow_fill_half_width, arrow_fill_height); > I don't think this code is technically correct in non-1x scales, because e.g. > the half-pixel offset will get scaled to one pixel and we'll wind up with a path > whose coordinate is no longer in the pixel center. > > I think what we need to do here is the same sort of thing I do for the tab > outlines: we need to make a path that assumes it will be drawn unscaled, then at > the drawing site we need to unscale and draw the path. > > As a side effect this will also mean we can avoid hairlines since we'll always > be drawing unscaled and can just use a 1 px thickness. > > https://codereview.chromium.org/1826653002/diff/20001/chrome/browser/ui/views... > chrome/browser/ui/views/infobars/infobar_view.cc:204: // edge of the arrow than > on the left. > Now that I know more about Skia than when I wrote this, the right way to compute > the fill is probably the way we do it for the MD tabstrip: start with the same > path as the stroke, but then use Op() from SkPathOps.h to subtract the stroked > stroke path away. This would avoid the need for an epsilon like this. It seems like you have more expertise in this area than me. Would you like to take over this bug?
On 2016/03/22 23:48:50, Evan Stade wrote: > It seems like you have more expertise in this area than me. Would you like to > take over this bug? I can. I won't get to it for a while but I know how to fix it once I do.
On 2016/03/23 00:19:43, Peter Kasting wrote: > On 2016/03/22 23:48:50, Evan Stade wrote: > > It seems like you have more expertise in this area than me. Would you like to > > take over this bug? > > I can. I won't get to it for a while but I know how to fix it once I do. If you won't have time for a larger overhaul soon, perhaps this approach could be useful in the meantime. The screenshots on the bug look like an improvement to me.
In principle, I'm not opposed to at least improving things. In practice, I worry that my concerns about the half-pixel offsets (see another such case below) might mean that for certain scale factors and positions, we're going to draw lines in slightly incorrect places. https://codereview.chromium.org/1826653002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/infobars/infobar_background.cc (right): https://codereview.chromium.org/1826653002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/infobars/infobar_background.cc:42: paint.setShader(nullptr); Nit: Rather than reset this, let's just use two different SkPaints for the stroke and the fill. https://codereview.chromium.org/1826653002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/infobars/infobar_background.cc:60: SkScalar bottom = SkIntToScalar(view->height()) - SK_ScalarHalf; This seems wrong at >1x. It really wants to move up by 0.5 unscaled pixels. (similar to the issue with defining the stroke path.) Since antialiasing is off, we won't get a partial line across 2 vertical pixels; instead we'll get a line that might be off by 1 pixel depending on how the non-antialiased drawing path works. (At >2x this is presumably even more likely to be incorrect.)
https://codereview.chromium.org/1826653002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/infobars/infobar_background.cc (right): https://codereview.chromium.org/1826653002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/infobars/infobar_background.cc:60: SkScalar bottom = SkIntToScalar(view->height()) - SK_ScalarHalf; On 2016/03/23 00:28:17, Peter Kasting wrote: > This seems wrong at >1x. It really wants to move up by 0.5 unscaled pixels. > (similar to the issue with defining the stroke path.) Since antialiasing is > off, we won't get a partial line across 2 vertical pixels; instead we'll get a > line that might be off by 1 pixel depending on how the non-antialiased drawing > path works. (At >2x this is presumably even more likely to be incorrect.) You're right, it does have bugs at >2x scales. I'll look into this more.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
updated, PTAL. https://codereview.chromium.org/1826653002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/infobars/infobar_background.cc (right): https://codereview.chromium.org/1826653002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/infobars/infobar_background.cc:30: if (ui::MaterialDesignController::IsModeMaterial()) { I found it easier to not break pre-MD while simplifying the MD path (e.g. getting rid of the gradient) if the code was not shared. https://codereview.chromium.org/1826653002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/infobars/infobar_background.cc:43: // The paths provided by |infobar| are in dp. Manually scale for px. instead of changing the meaning/value of stroke_path/fill_path, I just scaled them here. https://codereview.chromium.org/1826653002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/infobars/infobar_background.cc:69: SkPoint gradient_points[2]; only whitespace changes were made to this path.
https://codereview.chromium.org/1826653002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/infobars/infobar_background.cc (right): https://codereview.chromium.org/1826653002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/infobars/infobar_background.cc:30: if (ui::MaterialDesignController::IsModeMaterial()) { For simplicity, can we just draw a 1 px border for non-MD as well? It seems like the non-MD codepath is complex and different enough that that would be a nice win, and I don't think the change would feel particularly bad or "out of place" with the old UI. IIRC I made some similar toolbar/tabstrip changes where we now use 1 px borders for some of that stuff even pre-MD... https://codereview.chromium.org/1826653002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/infobars/infobar_view.cc (right): https://codereview.chromium.org/1826653002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/infobars/infobar_view.cc:197: SkIntToScalar(arrow_x) + SK_ScalarHalf - arrow_fill_half_width, This 0.5 adjustment is still going to be incorrect at larger scale factors. I think if you construct the path here in px coordinates instead of dp, you'll avoid this issue and avoid reintroducing the similar problem that led to your most recent set of changes. Plus the code might actually be slightly simpler than constructing the path here, then scaling it and shifting it post-scale. This would also open the door to making the other change I suggested earlier, which would be to pretty much replace the fill_path_ computation code below with a call to Op(). (But I'm not saying you have to do that, just that this would be a necessary step along the way to getting there.)
https://codereview.chromium.org/1826653002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/infobars/infobar_background.cc (right): https://codereview.chromium.org/1826653002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/infobars/infobar_background.cc:30: if (ui::MaterialDesignController::IsModeMaterial()) { On 2016/03/25 05:07:46, Peter Kasting wrote: > For simplicity, can we just draw a 1 px border for non-MD as well? It seems > like the non-MD codepath is complex and different enough that that would be a > nice win, and I don't think the change would feel particularly bad or "out of > place" with the old UI. IIRC I made some similar toolbar/tabstrip changes where > we now use 1 px borders for some of that stuff even pre-MD... There are several differences: - stroke width, as you have noted. - use of gradient - color of stroke (blending in an alpha from the gradient colors) So any sharing would have to either - change things in non-MD, or - still use some conditional codepaths, or - simultaneously handle both UIs > the non-MD codepath is complex and different enough that that would be a > nice win The non-MD codepath already exists and is tested (in that it's been in use on stable for many releases) and should go away soon anyway. I'd rather not touch it. https://codereview.chromium.org/1826653002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/infobars/infobar_view.cc (right): https://codereview.chromium.org/1826653002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/infobars/infobar_view.cc:197: SkIntToScalar(arrow_x) + SK_ScalarHalf - arrow_fill_half_width, On 2016/03/25 05:07:46, Peter Kasting wrote: > This 0.5 adjustment is still going to be incorrect at larger scale factors. Yes, at 2x the point is 2px wide, but I don't think it looks bad: https://screenshot.googleplex.com/VPof11VhcDZ.png With higher pixel densities it should be hard to even notice the difference. > > I think if you construct the path here in px coordinates instead of dp, you'll > avoid this issue and avoid reintroducing the similar problem that led to your > most recent set of changes. Plus the code might actually be slightly simpler > than constructing the path here, then scaling it and shifting it post-scale. > > This would also open the door to making the other change I suggested earlier, > which would be to pretty much replace the fill_path_ computation code below with > a call to Op(). (But I'm not saying you have to do that, just that this would > be a necessary step along the way to getting there.) There's no way to get the DSF in Layout(). You can do something gross like GetDisplayNearestWindow(view).device_scale_factor(); but there's no guarantee the display won't change between layout and paint (e.g. the user could drag the browser window between monitors and the DSF would change but Layout might not be called again). It seems better to keep this calculation in DP and make pixel adjustments at paint time in InfobarBackground. See my latest patchset, which produces https://screenshot.googleplex.com/9fNhzntPOyj.png (I believe these screenshots demonstrate correctness at all shown scale factors)
https://codereview.chromium.org/1826653002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/infobars/infobar_background.cc (right): https://codereview.chromium.org/1826653002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/infobars/infobar_background.cc:30: if (ui::MaterialDesignController::IsModeMaterial()) { On 2016/03/28 21:44:36, Evan Stade wrote: > On 2016/03/25 05:07:46, Peter Kasting wrote: > > For simplicity, can we just draw a 1 px border for non-MD as well? It seems > > like the non-MD codepath is complex and different enough that that would be a > > nice win, and I don't think the change would feel particularly bad or "out of > > place" with the old UI. IIRC I made some similar toolbar/tabstrip changes > where > > we now use 1 px borders for some of that stuff even pre-MD... > > There are several differences: > - stroke width, as you have noted. > - use of gradient > - color of stroke (blending in an alpha from the gradient colors) > > So any sharing would have to either > - change things in non-MD, or > - still use some conditional codepaths, or > - simultaneously handle both UIs Right, this would be an appearance change for non-MD. That was intentional on my part (the differences don't seem like they would look bad outside the MD context). > > the non-MD codepath is complex and different enough that that would be a > > nice win > > The non-MD codepath already exists and is tested (in that it's been in use on > stable for many releases) and should go away soon anyway. I'd rather not touch > it. Fair enough. I'm OK with leaving it if that turns out to be simpler. https://codereview.chromium.org/1826653002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/infobars/infobar_view.cc (right): https://codereview.chromium.org/1826653002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/infobars/infobar_view.cc:197: SkIntToScalar(arrow_x) + SK_ScalarHalf - arrow_fill_half_width, On 2016/03/28 21:44:36, Evan Stade wrote: > On 2016/03/25 05:07:46, Peter Kasting wrote: > > This 0.5 adjustment is still going to be incorrect at larger scale factors. > > Yes, at 2x the point is 2px wide, but I don't think it looks bad: > https://screenshot.googleplex.com/VPof11VhcDZ.png > With higher pixel densities it should be hard to even notice the difference. At odd integral scale factors, the tip will look correct; the sides will have slightly different angles, but that will be hard to spot. At even integral scale factors, the above is true, plus the tip will basically be split evenly across 2 px, which isn't great, but is not a huge deal. But I'm kinda worried about e.g. 140% and other non-integral scale factors, which AFAIK are common on Windows. > > I think if you construct the path here in px coordinates instead of dp, you'll > > avoid this issue and avoid reintroducing the similar problem that led to your > > most recent set of changes. Plus the code might actually be slightly simpler > > than constructing the path here, then scaling it and shifting it post-scale. > > > > This would also open the door to making the other change I suggested earlier, > > which would be to pretty much replace the fill_path_ computation code below > with > > a call to Op(). (But I'm not saying you have to do that, just that this would > > be a necessary step along the way to getting there.) > > There's no way to get the DSF in Layout(). You can do something gross like > GetDisplayNearestWindow(view).device_scale_factor(); but there's no guarantee > the display won't change between layout and paint (e.g. the user could drag the > browser window between monitors and the DSF would change but Layout might not be > called again). It seems better to keep this calculation in DP and make pixel > adjustments at paint time in InfobarBackground. See my latest patchset, which > produces https://screenshot.googleplex.com/9fNhzntPOyj.png (I believe these > screenshots demonstrate correctness at all shown scale factors) In other places, the way we do this when we have no canvas is by querying GetWidget()->GetCompositor()->device_scale_factor(). However, I think your concerns about layout have merit. Looking more closely at this code, it looks like there's no real reason we have to compute these paths in Layout() to begin with; no code in InfoBarView uses them, they're just created for use in InfoBarBackground::Paint(). So what if we just rip all this code out of here and move the path calculation entirely to that function? This would presumably be a bit simpler overall and would give us maximum flexibility in how to construct the paths. I think these paths are simple enough that we don't need to cache them anyway.
So I moved the path calculation to InfoBarBackground, as you suggested. However, something as seemingly simple as moving the code without even changing it broke non-MD in subtle ways (the tip of the arrow moved for some reason). I spent a good while trying to debug this without much luck (long enough to become frustrated and feel like I wasn't going to stumble on a solution in a reasonable amount of time). So I kept the same path calculation and painting code for non-MD, and just moved/simplified for MD. Along the way I also found an MD bug with the arrow when there's no bar. A couple constants had to be updated because of changes in padding in the omnibox.
On 2016/03/29 19:41:33, Evan Stade wrote: > So I moved the path calculation to InfoBarBackground, as you suggested. However, > something as seemingly simple as moving the code without even changing it broke > non-MD in subtle ways (the tip of the arrow moved for some reason). I spent a > good while trying to debug this without much luck (long enough to become > frustrated and feel like I wasn't going to stumble on a solution in a reasonable > amount of time). What was the tip movement? Maybe this is one where, as you suggested early on, I should take over, as I've been working on the arrow tip placement lately anyway.
On 2016/03/29 20:44:21, Peter Kasting wrote: > On 2016/03/29 19:41:33, Evan Stade wrote: > > So I moved the path calculation to InfoBarBackground, as you suggested. > However, > > something as seemingly simple as moving the code without even changing it > broke > > non-MD in subtle ways (the tip of the arrow moved for some reason). I spent a > > good while trying to debug this without much luck (long enough to become > > frustrated and feel like I wasn't going to stumble on a solution in a > reasonable > > amount of time). > > What was the tip movement? Maybe this is one where, as you suggested early on, > I should take over, as I've been working on the arrow tip placement lately > anyway. The tip was one pixel low, which made the angle a little less than 45 degrees in the no-bar case. The current code works though and the new (MD) code is a good deal simpler than what exists for non-MD, so I think it's ok to have divergent code paths for now.
https://codereview.chromium.org/1826653002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/infobars/infobar_background.cc (right): https://codereview.chromium.org/1826653002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/infobars/infobar_background.cc:34: InfoBarView* infobar = static_cast<InfoBarView*>(view); Nit: Google style guide suggests declaring these "as close to the first use as possible". https://codereview.chromium.org/1826653002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/infobars/infobar_background.cc:38: delegate ? delegate->GetInfoBarSeparatorColor() : SK_ColorBLACK; When will the delegate be null during painting? I can't think why that could be. (2 places) https://codereview.chromium.org/1826653002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/infobars/infobar_background.cc:83: SkIntToScalar(std::max(infobar->arrow_height(), 0)); Can arrow_height() actually return a negative value? https://codereview.chromium.org/1826653002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/infobars/infobar_background.cc:97: 0.0, SkIntToScalar(infobar->arrow_height()), Nit: Unless it fails to compile, I'd use 0 instead of 0.0 here, since technically 0.0 is a double and might require truncation, while 0 should be promotable to either float or double. https://codereview.chromium.org/1826653002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/infobars/infobar_background.cc:100: InfoBarContainerDelegate::kSeparatorLineHeight)); Two issues: (1) In general, I don't think we want to use kSeparatorLineHeight in MD. That value is in dp, and we now want to force 1 px separators. I think we should hardcode 1 px here, as we've done in e.g. the tabstrip, toolbar top and bottom, etc. and mark kSeparatorLineHeight as to-be-removed when non-MD is gone. (2) Even if we wanted to keep this, it's being used inconsistently in this function: you're using it unscaled (so dp) here, and scaled (so px) below. You'd have to adjust, e.g. by scaling before adding this rect. However, maybe we can just remove this subtraction entirely, fill to the bottom of the rect, and let the separator paint atop the fill region, thus avoiding the issue. https://codereview.chromium.org/1826653002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/infobars/infobar_background.cc:117: fill_path.offset(SK_ScalarHalf, SK_ScalarHalf); Nit: This deserves an explanation https://codereview.chromium.org/1826653002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/infobars/infobar_background.cc:128: // Top separator (with arrow shape). Doesn't this only draw the arrow shape, i.e. there's no actual "top separator" being drawn here, and instead we rely on either the toolbar bottom or the previous infobar bottom to be the "separator"? https://codereview.chromium.org/1826653002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/infobars/infobar_background.cc:132: stroke.setAntiAlias(false); Nit: I think this whole bottom section can just become a call to BrowserView::Paint1pxHorizontalLine(canvas, separator_color, view->bounds(), true);, as long as you let the scale scoper go out of scope first. https://codereview.chromium.org/1826653002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/infobars/infobar_view.cc (right): https://codereview.chromium.org/1826653002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/infobars/infobar_view.cc:183: // width is changed, which affects both paths. Nit: Might want removal TODO here also https://codereview.chromium.org/1826653002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/infobars/infobar_view.h (right): https://codereview.chromium.org/1826653002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/infobars/infobar_view.h:143: // above. Nit: Might want removal TODO here also
https://codereview.chromium.org/1826653002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/infobars/infobar_background.cc (right): https://codereview.chromium.org/1826653002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/infobars/infobar_background.cc:34: InfoBarView* infobar = static_cast<InfoBarView*>(view); On 2016/03/30 00:20:26, Peter Kasting wrote: > Nit: Google style guide suggests declaring these "as close to the first use as > possible". Done. https://codereview.chromium.org/1826653002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/infobars/infobar_background.cc:38: delegate ? delegate->GetInfoBarSeparatorColor() : SK_ColorBLACK; On 2016/03/30 00:20:26, Peter Kasting wrote: > When will the delegate be null during painting? I can't think why that could > be. (2 places) I don't know why it would be null. Removed. https://codereview.chromium.org/1826653002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/infobars/infobar_background.cc:83: SkIntToScalar(std::max(infobar->arrow_height(), 0)); On 2016/03/30 00:20:26, Peter Kasting wrote: > Can arrow_height() actually return a negative value? I looked into this and can't find anywhere that sets it to a negative value. Fixed. https://codereview.chromium.org/1826653002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/infobars/infobar_background.cc:97: 0.0, SkIntToScalar(infobar->arrow_height()), On 2016/03/30 00:20:26, Peter Kasting wrote: > Nit: Unless it fails to compile, I'd use 0 instead of 0.0 here, since > technically 0.0 is a double and might require truncation, while 0 should be > promotable to either float or double. Done. https://codereview.chromium.org/1826653002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/infobars/infobar_background.cc:100: InfoBarContainerDelegate::kSeparatorLineHeight)); On 2016/03/30 00:20:26, Peter Kasting wrote: > Two issues: > > (1) In general, I don't think we want to use kSeparatorLineHeight in MD. That > value is in dp, and we now want to force 1 px separators. I think we should > hardcode 1 px here, as we've done in e.g. the tabstrip, toolbar top and bottom, > etc. and mark kSeparatorLineHeight as to-be-removed when non-MD is gone. > > (2) Even if we wanted to keep this, it's being used inconsistently in this > function: you're using it unscaled (so dp) here, and scaled (so px) below. > You'd have to adjust, e.g. by scaling before adding this rect. However, maybe > we can just remove this subtraction entirely, fill to the bottom of the rect, > and let the separator paint atop the fill region, thus avoiding the issue. ok, good point. Removed usage of this constant in MD. https://codereview.chromium.org/1826653002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/infobars/infobar_background.cc:117: fill_path.offset(SK_ScalarHalf, SK_ScalarHalf); On 2016/03/30 00:20:26, Peter Kasting wrote: > Nit: This deserves an explanation Done. https://codereview.chromium.org/1826653002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/infobars/infobar_background.cc:128: // Top separator (with arrow shape). On 2016/03/30 00:20:26, Peter Kasting wrote: > Doesn't this only draw the arrow shape, i.e. there's no actual "top separator" > being drawn here, and instead we rely on either the toolbar bottom or the > previous infobar bottom to be the "separator"? reworded https://codereview.chromium.org/1826653002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/infobars/infobar_background.cc:132: stroke.setAntiAlias(false); On 2016/03/30 00:20:26, Peter Kasting wrote: > Nit: I think this whole bottom section can just become a call to > BrowserView::Paint1pxHorizontalLine(canvas, separator_color, view->bounds(), > true);, as long as you let the scale scoper go out of scope first. I don't think this is a huge win. We go from 4 lines to 2 lines (still have to call canvas->Restore(), or worse, indent a large block of code above for a new scope). It seems a little roundabout to me to un-unscale then unscale again, and the call to Paint1PxHorizontalLine seems a bit harder to understand (the "true" at the end is particularly mysterious until you go look up what it is). https://codereview.chromium.org/1826653002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/infobars/infobar_view.cc (right): https://codereview.chromium.org/1826653002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/infobars/infobar_view.cc:183: // width is changed, which affects both paths. On 2016/03/30 00:20:26, Peter Kasting wrote: > Nit: Might want removal TODO here also Done. https://codereview.chromium.org/1826653002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/infobars/infobar_view.h (right): https://codereview.chromium.org/1826653002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/infobars/infobar_view.h:143: // above. On 2016/03/30 00:20:26, Peter Kasting wrote: > Nit: Might want removal TODO here also moved the todo here https://codereview.chromium.org/1826653002/diff/160001/chrome/browser/ui/info... File chrome/browser/ui/infobar_container_delegate.cc (right): https://codereview.chromium.org/1826653002/diff/160001/chrome/browser/ui/info... chrome/browser/ui/infobar_container_delegate.cc:69: top_arrow_target_height_ = std::min(height, kMaximumArrowTargetHeight); I found a pre-existing bug where the arrow is unsharp in non-MD when there are two arrows and no visible bookmark bar. This fixes that bug. I think the separator offset on L128 is sufficient to address the issue the comment was talking about.
LGTM. I commend you for sticking with this. https://codereview.chromium.org/1826653002/diff/160001/chrome/browser/ui/info... File chrome/browser/ui/infobar_container_delegate.cc (right): https://codereview.chromium.org/1826653002/diff/160001/chrome/browser/ui/info... chrome/browser/ui/infobar_container_delegate.cc:69: top_arrow_target_height_ = std::min(height, kMaximumArrowTargetHeight); On 2016/04/02 02:21:42, Evan Stade wrote: > I found a pre-existing bug where the arrow is unsharp in non-MD when there are > two arrows and no visible bookmark bar. This fixes that bug. I think the > separator offset on L128 is sufficient to address the issue the comment was > talking about. Make sure that the arrow tip gets drawn at the exact same y coordinate before this change as after (1 dp adjustments to this might look bad in certain scenarios). https://codereview.chromium.org/1826653002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/infobars/infobar_background.cc (right): https://codereview.chromium.org/1826653002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/infobars/infobar_background.cc:109: // The paths provided by |infobar| are in dp. Manually scale for px. Nit: I think this comment is slightly out of date, since we no longer get the paths from the infobar. https://codereview.chromium.org/1826653002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/infobars/infobar_background.cc:113: // Skia considers the middle of the pixel to be a fractional value (e.g. 0.5). Nit: This comment is confusing to me since it sounds like the value might be other than 0.5 sometimes, and also since it's not really clear why Skia's perception of pixel centers matters. How about this: In order to affect exactly the pixels we want, the fill and stroke paths need to go through the pixel centers instead of along the pixel edges/corners. Skia considers integral coordinates to be the edges between pixels, so offset by 0.5 to get to the centers. https://codereview.chromium.org/1826653002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/infobars/infobar_background.cc:125: // Top separator (just the arrow shape). Nit: Maybe fill this out more: ...(just the arrow shape; the rest of the separator line has already been drawn by the toolbar or previous infobar).
https://codereview.chromium.org/1826653002/diff/160001/chrome/browser/ui/info... File chrome/browser/ui/infobar_container_delegate.cc (right): https://codereview.chromium.org/1826653002/diff/160001/chrome/browser/ui/info... chrome/browser/ui/infobar_container_delegate.cc:69: top_arrow_target_height_ = std::min(height, kMaximumArrowTargetHeight); On 2016/04/02 02:42:11, Peter Kasting wrote: > On 2016/04/02 02:21:42, Evan Stade wrote: > > I found a pre-existing bug where the arrow is unsharp in non-MD when there are > > two arrows and no visible bookmark bar. This fixes that bug. I think the > > separator offset on L128 is sufficient to address the issue the comment was > > talking about. > > Make sure that the arrow tip gets drawn at the exact same y coordinate before > this change as after (1 dp adjustments to this might look bad in certain > scenarios). yea, in the single infobar case, it copes up one pixel higher. I'll leave this line alone (and let the multi-infobar case continue to be slightly buggy in non-md). https://codereview.chromium.org/1826653002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/infobars/infobar_background.cc (right): https://codereview.chromium.org/1826653002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/infobars/infobar_background.cc:109: // The paths provided by |infobar| are in dp. Manually scale for px. On 2016/04/02 02:42:11, Peter Kasting wrote: > Nit: I think this comment is slightly out of date, since we no longer get the > paths from the infobar. Done. https://codereview.chromium.org/1826653002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/infobars/infobar_background.cc:113: // Skia considers the middle of the pixel to be a fractional value (e.g. 0.5). On 2016/04/02 02:42:11, Peter Kasting wrote: > Nit: This comment is confusing to me since it sounds like the value might be > other than 0.5 sometimes, and also since it's not really clear why Skia's > perception of pixel centers matters. How about this: > > In order to affect exactly the pixels we want, the fill and stroke paths need to > go through the pixel centers instead of along the pixel edges/corners. Skia > considers integral coordinates to be the edges between pixels, so offset by 0.5 > to get to the centers. Done. https://codereview.chromium.org/1826653002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/infobars/infobar_background.cc:125: // Top separator (just the arrow shape). On 2016/04/02 02:42:11, Peter Kasting wrote: > Nit: Maybe fill this out more: > > ...(just the arrow shape; the rest of the separator line has already been drawn > by the toolbar or previous infobar). Done.
The CQ bit was checked by estade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/1826653002/#ps200001 (title: "format")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1826653002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1826653002/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Draw infobar arrow border with width of 1px on any scale factor. BUG=597069 ========== to ========== Draw infobar arrow border with width of 1px on any scale factor. BUG=597069 Committed: https://crrev.com/1d9418afc9ddb2254d7c9316ea3043e3e6ff1ffd Cr-Commit-Position: refs/heads/master@{#384953} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/1d9418afc9ddb2254d7c9316ea3043e3e6ff1ffd Cr-Commit-Position: refs/heads/master@{#384953} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
