Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2179643002:
Fix infobar painting issues at fractional scales (Closed)
|
Created:
4 years, 5 months ago by Evan Stade Modified:
4 years, 4 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. |
DescriptionFix infobar painting issues at fractional scales
This calculates the paths for the infobar background (which paints the arrow) with the scale factor taken into account. It fixes blurriness in the arrow as well as a gap above the infobar (you can see both issues in a screenshot in the linked bug report).
It should have no impact on whole number DSFs (1x, 2x, etc)
BUG=627601
Committed: https://crrev.com/21e70f121e72154c9891b061e9724ef30e1aecb9
Cr-Commit-Position: refs/heads/master@{#408498}
Patch Set 1 #Patch Set 2 : comment #
Total comments: 12
Patch Set 3 : reason for floor #Patch Set 4 : bsep #
Total comments: 1
Patch Set 5 : fiddle #
Total comments: 10
Patch Set 6 : pkasting review #
Total comments: 8
Patch Set 7 : pkasting review 2 #
Total comments: 2
Patch Set 8 : pkasting final review #
Messages
Total messages: 30 (13 generated)
Description was changed from ========== WIP - fix infobar painting issues at 1.5x scale BUG= ========== to ========== Fix infobar painting issues at 1.5x scale This calculates the paths for the infobar background (which paints the arrow) with the scale factor taken into account. It fixes blurriness in the arrow as well as a gap above the infobar (you can see both issues in a screenshot in the linked bug report). It should have no impact on whole number DSFs (1x, 2x, etc) This does not fix the gap for all fractional scale factors, e.g. 1.75. I investigated that a bit, but I believe the issue there is a problem of sizing/positioning views in dip, so would require a significant amount of extra work to fix. For now, just fixing 1.5x is a good start as it is a more common scale factor. BUG=627601 ==========
estade@chromium.org changed reviewers: + bsep@chromium.org, pkasting@chromium.org
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
It might be good to put some screenshots on the bug to help explain the pathing question I have below. Maybe these can also clarify how this changes things in 1.5x and 1.75x, as I'm curious whether this improves 1.75x (just not enough), or doesn't help it at all. Interestingly, this bug overlaps with bug 629242, which I was about to start working on (like, in a couple of minutes). Good timing. I guess I will mostly worry about the infobar content positioning portion of that bug (unless you want to tackle that too). Generally the approach here seems fine so I think the last question below is my only major concern. https://codereview.chromium.org/2179643002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/infobars/infobar_background.cc (right): https://codereview.chromium.org/2179643002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/infobars/infobar_background.cc:78: // The fill for the arrow Nit: This comment doesn't seem to add much. I'd probably just nuke it. After doing so, the declaration lines can be combined again as they were before. https://codereview.chromium.org/2179643002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/infobars/infobar_background.cc:84: // Always scale to a whole number to make sure strokes are sharp. Nit: Can you extend this comment to explain why floor() is a better choice than round() (or, I guess, ceil())? I'm not sure why, myself. https://codereview.chromium.org/2179643002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/infobars/infobar_background.cc:97: stroke_path.rLineTo(-1, 0); I don't understand how this line works. Doesn't this convert this: /\ / \ To this?: _ X / \ That seems wrong. Why wouldn't leaving this rLineTo() call out be better?
Basically looks good. Ditto Peter's comments. Does this work for 1.25? https://codereview.chromium.org/2179643002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/infobars/infobar_background.cc (right): https://codereview.chromium.org/2179643002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/infobars/infobar_background.cc:84: // Always scale to a whole number to make sure strokes are sharp. On 2016/07/25 22:21:21, Peter Kasting wrote: > Nit: Can you extend this comment to explain why floor() is a better choice than > round() (or, I guess, ceil())? I'm not sure why, myself. Which method to use to snap to pixels is somewhat arbitrary I think. My instinct would be to use round but if it looks fine I don't think it's important to change. https://codereview.chromium.org/2179643002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/infobars/infobar_background.cc:93: stroke_path.moveTo(scale(arrow_x) - scale(infobar->arrow_half_width()), reuse arrow_half_width https://codereview.chromium.org/2179643002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/infobars/infobar_background.cc:114: stroke.setStrokeWidth(SkIntToScalar(kSeparatorThicknessPx)); I think the default stroke width is 1px already (while you're cleaning up anyway).
https://codereview.chromium.org/2179643002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/infobars/infobar_background.cc (right): https://codereview.chromium.org/2179643002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/infobars/infobar_background.cc:84: // Always scale to a whole number to make sure strokes are sharp. On 2016/07/25 22:35:31, Bret Sepulveda wrote: > On 2016/07/25 22:21:21, Peter Kasting wrote: > > Nit: Can you extend this comment to explain why floor() is a better choice > than > > round() (or, I guess, ceil())? I'm not sure why, myself. > > Which method to use to snap to pixels is somewhat arbitrary I think. My instinct > would be to use round but if it looks fine I don't think it's important to > change. To be clear, I'm fine with any of them (whatever works best), but such subtle differences usually matter and are thus worth explaining, and that goes double anywhere that doesn't use round(), since that'd probably be the default choice for most people and thus using something else is another indicator that the distinction is important.
https://codereview.chromium.org/2179643002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/infobars/infobar_background.cc (right): https://codereview.chromium.org/2179643002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/infobars/infobar_background.cc:78: // The fill for the arrow On 2016/07/25 22:21:21, Peter Kasting wrote: > Nit: This comment doesn't seem to add much. I'd probably just nuke it. After > doing so, the declaration lines can be combined again as they were before. Done. https://codereview.chromium.org/2179643002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/infobars/infobar_background.cc:84: // Always scale to a whole number to make sure strokes are sharp. On 2016/07/25 22:21:21, Peter Kasting wrote: > Nit: Can you extend this comment to explain why floor() is a better choice than > round() (or, I guess, ceil())? I'm not sure why, myself. good question, I didn't give it much thought beyond that was how to fix 1.5x. It didn't fix 1.75x though so it's clearly not always right (round and ceil both don't work for 1.5x). I thought about it more and I concluded that based on rounding of other numbers (view coordinates, toolbar dimensions, etc.) floor would sometimes be right and sometimes make the fill region 1px too high, such as in the 1.75x case. Trying to figure out if we should be using ceil instead in cases like that seems very difficult, so I still think floor is the right call. To compensate for the 1px of overlap we'll get in some cases, like 1.75x, I made this re-draw the top separator. (I also noticed that my original patchset didn't work well for the second infobar if there are multiple infobars visible, at 1.5x, and this fixes that situation.) https://codereview.chromium.org/2179643002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/infobars/infobar_background.cc:97: stroke_path.rLineTo(-1, 0); On 2016/07/25 22:21:21, Peter Kasting wrote: > I don't understand how this line works. Doesn't this convert this: > > /\ > / \ > > To this?: > > _ > X > / \ > > That seems wrong. Why wouldn't leaving this rLineTo() call out be better? Your ASCII art is accurate, except the way it's rendered on screen looks more like: ██ █ █ vs █ █ █ The latter is preferable.
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Description was changed from ========== Fix infobar painting issues at 1.5x scale This calculates the paths for the infobar background (which paints the arrow) with the scale factor taken into account. It fixes blurriness in the arrow as well as a gap above the infobar (you can see both issues in a screenshot in the linked bug report). It should have no impact on whole number DSFs (1x, 2x, etc) This does not fix the gap for all fractional scale factors, e.g. 1.75. I investigated that a bit, but I believe the issue there is a problem of sizing/positioning views in dip, so would require a significant amount of extra work to fix. For now, just fixing 1.5x is a good start as it is a more common scale factor. BUG=627601 ========== to ========== Fix infobar painting issues at fractional scales This calculates the paths for the infobar background (which paints the arrow) with the scale factor taken into account. It fixes blurriness in the arrow as well as a gap above the infobar (you can see both issues in a screenshot in the linked bug report). It should have no impact on whole number DSFs (1x, 2x, etc) BUG=627601 ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
> Does this work for 1.25? it does now. It works for all tested DSFs: 1, 1.25, 1.5, 1.75, 2 https://codereview.chromium.org/2179643002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/infobars/infobar_background.cc (right): https://codereview.chromium.org/2179643002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/infobars/infobar_background.cc:93: stroke_path.moveTo(scale(arrow_x) - scale(infobar->arrow_half_width()), On 2016/07/25 22:35:31, Bret Sepulveda wrote: > reuse arrow_half_width Done. https://codereview.chromium.org/2179643002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/infobars/infobar_background.cc:114: stroke.setStrokeWidth(SkIntToScalar(kSeparatorThicknessPx)); On 2016/07/25 22:35:31, Bret Sepulveda wrote: > I think the default stroke width is 1px already (while you're cleaning up > anyway). the default is 0 (hairline)
lgtm
https://codereview.chromium.org/2179643002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/infobars/infobar_background.cc (right): https://codereview.chromium.org/2179643002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/infobars/infobar_background.cc:97: // is sharper without this adjustment. This is actually getting at the reason why I believe adding the rLineTo(-1, 0) helps: the path isn't on pixel centers. The problem is that your adjustments here don't fix that problem completely. You only ever adjust the Y coordinate to be on a pixel center, and even then not all the time (only for the horizontal stroke). Also, I'm not sure that (arrow_height - SK_ScalarHalf) is the Y coord we want: it seems like for both X and Y you'd want to add SK_ScalarHalf to the start coordinates, not subtract it. I'd have to code the whole thing up and check it locally to be sure, which I'll do if you can't get something working. You want something like: stroke_path.moveTo(0, arrow_height + SK_ScalarHalf); SkScalar scaled_arrow_x = scale(arrow_x) + SK_ScalarHalf; stroke_path.rLineTo(scaled_arrow_x - arrow_half_width, 0); stroke_path.rLineTo(arrow_half_width, -arrow_height); stroke_path.rLineTo(arrow_half_width, arrow_height); stroke_path.rLineTo(scale(infobar->width()) - (scaled_arrow_x + arrow_half_width), 0); I believe this should give you the sharp point you want. May require some adjustment to not be off-by-one on the arrow tip X or Y coordinates or the divider Y coordinate.
On 2016/07/26 00:02:34, Peter Kasting wrote: > https://codereview.chromium.org/2179643002/diff/60001/chrome/browser/ui/views... > File chrome/browser/ui/views/infobars/infobar_background.cc (right): > > https://codereview.chromium.org/2179643002/diff/60001/chrome/browser/ui/views... > chrome/browser/ui/views/infobars/infobar_background.cc:97: // is sharper without > this adjustment. > This is actually getting at the reason why I believe adding the rLineTo(-1, 0) > helps: the path isn't on pixel centers. > > The problem is that your adjustments here don't fix that problem completely. > You only ever adjust the Y coordinate to be on a pixel center, and even then not > all the time (only for the horizontal stroke). Also, I'm not sure that > (arrow_height - SK_ScalarHalf) is the Y coord we want: it seems like for both X > and Y you'd want to add SK_ScalarHalf to the start coordinates, not subtract it. > I'd have to code the whole thing up and check it locally to be sure, which I'll > do if you can't get something working. > > You want something like: > > stroke_path.moveTo(0, arrow_height + SK_ScalarHalf); > SkScalar scaled_arrow_x = scale(arrow_x) + SK_ScalarHalf; > stroke_path.rLineTo(scaled_arrow_x - arrow_half_width, 0); > stroke_path.rLineTo(arrow_half_width, -arrow_height); > stroke_path.rLineTo(arrow_half_width, arrow_height); > stroke_path.rLineTo(scale(infobar->width()) - (scaled_arrow_x + > arrow_half_width), 0); > > I believe this should give you the sharp point you want. May require some > adjustment to not be off-by-one on the arrow tip X or Y coordinates or the > divider Y coordinate. Updated. You're right, this is much cleaner. In fiddling with this I also noticed that we were drawing the arrow 1px too big --- if arrow_height is 11 we're drawing a line that offsets y by 11, but with the start and end points both taken into account this is actually a 12px high line. This isn't just an inconsequential nit, it actually creates a noticeable visual issue at 1.5x.
https://codereview.chromium.org/2179643002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/infobars/infobar_background.cc (right): https://codereview.chromium.org/2179643002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/infobars/infobar_background.cc:94: // length n + 1. Is this because n is measured between pixel centers, and we get an extra half pixel on each end from the stroke going out to the pixel edge? If so, wouldn't subtracting one from the half_width value below end up subtracting one extra pixel, since where the strokes meet at the tip of the /\, they meet at a pixel center, and so we just have the half-pixel overshoots at the outsides of each? Actually, now that I say this I'm even more confused, since those pixel centers will be the endpoints for the horizontal lines on either side... so it seems like there's no overshoot at all? My brain hurts. I feel like I understand the height offset if we're counting from "the top edge of the stroke at the tip to the bottom edge of the horizontal stroke" and we want that distance to equal the arrow_height, but I'm not clear on the half_width adjustments. https://codereview.chromium.org/2179643002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/infobars/infobar_background.cc:97: // Compensate for the zero-indexing of coordinates. Nit: Might be clearer to say "Offset by a half pixel in each direction so the path goes through pixel centers." https://codereview.chromium.org/2179643002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/infobars/infobar_background.cc:98: stroke_path.moveTo(SK_ScalarHalf, arrow_height - 1 + SK_ScalarHalf); Nit: Reuse r_arrow_height https://codereview.chromium.org/2179643002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/infobars/infobar_background.cc:103: stroke_path.rLineTo(scale(infobar->width()), 0); Couldn't this overdraw the vertical client edge, at least on Windows? Seems like it'd be safest to just subtract 1, then you wouldn't need the comment either. https://codereview.chromium.org/2179643002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/infobars/infobar_background.cc:109: fill_path.addRect(0, arrow_height, scale(infobar->width()), Nit: Maybe stuff scale(infobar->width()) into a temp above so you can reuse it both places it's referenced?
https://codereview.chromium.org/2179643002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/infobars/infobar_background.cc (right): https://codereview.chromium.org/2179643002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/infobars/infobar_background.cc:94: // length n + 1. On 2016/07/26 22:41:01, Peter Kasting wrote: > Is this because n is measured between pixel centers, and we get an extra half > pixel on each end from the stroke going out to the pixel edge? If so, wouldn't > subtracting one from the half_width value below end up subtracting one extra > pixel, since where the strokes meet at the tip of the /\, they meet at a pixel > center, and so we just have the half-pixel overshoots at the outsides of each? > > Actually, now that I say this I'm even more confused, since those pixel centers > will be the endpoints for the horizontal lines on either side... so it seems > like there's no overshoot at all? My brain hurts. I feel like I understand the > height offset if we're counting from "the top edge of the stroke at the tip to > the bottom edge of the horizontal stroke" and we want that distance to equal the > arrow_height, but I'm not clear on the half_width adjustments. arrow_half_width is perhaps a misnomer because it's 11dp, but the arrow in total is 21dp wide. We can't really make the arrow an even width if we want a sharp point and symmetry. In any case, arrow_half_width and arrow_height have to match exactly for the short arrow if we want sharp lines and no AA blurring. https://codereview.chromium.org/2179643002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/infobars/infobar_background.cc:97: // Compensate for the zero-indexing of coordinates. On 2016/07/26 22:41:01, Peter Kasting wrote: > Nit: Might be clearer to say "Offset by a half pixel in each direction so the > path goes through pixel centers." ah, this comment was intended to explain the arrow_height - 1 https://codereview.chromium.org/2179643002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/infobars/infobar_background.cc:98: stroke_path.moveTo(SK_ScalarHalf, arrow_height - 1 + SK_ScalarHalf); On 2016/07/26 22:41:01, Peter Kasting wrote: > Nit: Reuse r_arrow_height the reason I didn't do that is because according to my logic, this |- 1| is there for a different reason, which I attempted to explain via the comment on L97. https://codereview.chromium.org/2179643002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/infobars/infobar_background.cc:103: stroke_path.rLineTo(scale(infobar->width()), 0); On 2016/07/26 22:41:01, Peter Kasting wrote: > Couldn't this overdraw the vertical client edge, at least on Windows? Seems > like it'd be safest to just subtract 1, then you wouldn't need the comment > either. I don't believe so? The layer we're painting onto should be clipped. We can't just subtract 1 because this is a relative line, what we'd need to do is something like: SkPoint pt; stroke_path.GetLastPt(&pt); stroke_path.rLineTo(infobar->width() - pt.fX, 0); https://codereview.chromium.org/2179643002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/infobars/infobar_background.cc:109: fill_path.addRect(0, arrow_height, scale(infobar->width()), On 2016/07/26 22:41:01, Peter Kasting wrote: > Nit: Maybe stuff scale(infobar->width()) into a temp above so you can reuse it > both places it's referenced? kinda meh, but done.
LGTM https://codereview.chromium.org/2179643002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/find_bar_view.cc (right): https://codereview.chromium.org/2179643002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/find_bar_view.cc:94: const int kDefaultCharWidthMd = 30; This will go away on resync https://codereview.chromium.org/2179643002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/infobars/infobar_background.cc (right): https://codereview.chromium.org/2179643002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/infobars/infobar_background.cc:99: stroke_path.moveTo(SK_ScalarHalf, arrow_height - 1 + SK_ScalarHalf); I'm still a bit confused by the "zero-indexing" comment since it implies that the arrow_height value was originally computed as one-indexed, or something. It's hard for me to understand, reading the comment, how the two computations of arrow_height - 1 have separate meanings. I guess you'd be referring to the same logic as "relative movement of n = line of n + 1", but that still implies to me that the two computations here have the same motivation. What about: // We want the arrow tip to be at the top of the bounds, so we need to // start |r_arrow_height| below. Add SK_ScalarHalf to both coordinates to // place the path on pixel centers. stroke_path.moveTo(SK_ScalarHalf, r_arrow_height + SK_ScalarHalf); To me this makes it clearer that the Y coordinate here must be the same distance downwards that r_arrow_height wants to go upwards, as well as why the 0.5s are added in. https://codereview.chromium.org/2179643002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/infobars/infobar_background.cc:104: stroke_path.rLineTo(infobar_width, 0); I've had a lot of problems where views code on Linux clips the canvas and views code on Windows doesn't, so things can draw outside their own bounds on Windows but not Linux. That makes me nervous about this sort of code because I always am unsure whether the canvas has been explicitly clipped for Windows too at the point I'm reading. So what about: stroke_path.lineTo(infobar_width - SK_ScalarHalf, arrow_height - 1 + SK_ScalarHalf); Since this would be exactly correct it wouldn't need the comment line above. You could even pull the Y portion out to a temp |arrow_bottom_y| or something and reuse it both places it's used. https://codereview.chromium.org/2179643002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/infobars/infobar_background.cc:129: SkScalar w = scale(view_size_px.width()); Wasn't this already scaled by the ScaleSize() call? I think you just want to use SkFloatToScalar.
https://codereview.chromium.org/2179643002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/find_bar_view.cc (right): https://codereview.chromium.org/2179643002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/find_bar_view.cc:94: const int kDefaultCharWidthMd = 30; On 2016/07/28 19:41:30, Peter Kasting wrote: > This will go away on resync oops. Got my branches crossed. https://codereview.chromium.org/2179643002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/infobars/infobar_background.cc (right): https://codereview.chromium.org/2179643002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/infobars/infobar_background.cc:99: stroke_path.moveTo(SK_ScalarHalf, arrow_height - 1 + SK_ScalarHalf); On 2016/07/28 19:41:31, Peter Kasting wrote: > I'm still a bit confused by the "zero-indexing" comment since it implies that > the arrow_height value was originally computed as one-indexed, or something. > It's hard for me to understand, reading the comment, how the two computations of > arrow_height - 1 have separate meanings. I guess you'd be referring to the same > logic as "relative movement of n = line of n + 1", but that still implies to me > that the two computations here have the same motivation. > > What about: > > // We want the arrow tip to be at the top of the bounds, so we need to > // start |r_arrow_height| below. Add SK_ScalarHalf to both coordinates to > // place the path on pixel centers. > stroke_path.moveTo(SK_ScalarHalf, r_arrow_height + SK_ScalarHalf); > > To me this makes it clearer that the Y coordinate here must be the same distance > downwards that r_arrow_height wants to go upwards, as well as why the 0.5s are > added in. I copied your comment but if we're reusing r_arrow_height, the first part of the comment seems a little redundant, so I removed that and just left the bit about halves. https://codereview.chromium.org/2179643002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/infobars/infobar_background.cc:104: stroke_path.rLineTo(infobar_width, 0); On 2016/07/28 19:41:30, Peter Kasting wrote: > I've had a lot of problems where views code on Linux clips the canvas and views > code on Windows doesn't, so things can draw outside their own bounds on Windows > but not Linux. That makes me nervous about this sort of code because I always > am unsure whether the canvas has been explicitly clipped for Windows too at the > point I'm reading. > > So what about: > > stroke_path.lineTo(infobar_width - SK_ScalarHalf, > arrow_height - 1 + SK_ScalarHalf); > > Since this would be exactly correct it wouldn't need the comment line above. > > You could even pull the Y portion out to a temp |arrow_bottom_y| or something > and reuse it both places it's used. I kinda dislike switching between absolute and relative coordinates and I don't want to have to deal with SK_ScalarHalf more than once. So how do you like the new code? https://codereview.chromium.org/2179643002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/infobars/infobar_background.cc:129: SkScalar w = scale(view_size_px.width()); On 2016/07/28 19:41:31, Peter Kasting wrote: > Wasn't this already scaled by the ScaleSize() call? I think you just want to > use SkFloatToScalar. yea good call.
Cool, I like your idea. Thumbs up! https://codereview.chromium.org/2179643002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/infobars/infobar_background.cc (right): https://codereview.chromium.org/2179643002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/infobars/infobar_background.cc:95: // length n + 1. Nit: "...once the path is placed on pixel centers and stroked"? https://codereview.chromium.org/2179643002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/infobars/infobar_background.cc:103: stroke_path.lineTo(infobar_width, r_arrow_height); Nit: You want "infobar_width - 1" here.
The CQ bit was checked by estade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bsep@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2179643002/#ps140001 (title: "pkasting final review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fix infobar painting issues at fractional scales This calculates the paths for the infobar background (which paints the arrow) with the scale factor taken into account. It fixes blurriness in the arrow as well as a gap above the infobar (you can see both issues in a screenshot in the linked bug report). It should have no impact on whole number DSFs (1x, 2x, etc) BUG=627601 ========== to ========== Fix infobar painting issues at fractional scales This calculates the paths for the infobar background (which paints the arrow) with the scale factor taken into account. It fixes blurriness in the arrow as well as a gap above the infobar (you can see both issues in a screenshot in the linked bug report). It should have no impact on whole number DSFs (1x, 2x, etc) BUG=627601 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Fix infobar painting issues at fractional scales This calculates the paths for the infobar background (which paints the arrow) with the scale factor taken into account. It fixes blurriness in the arrow as well as a gap above the infobar (you can see both issues in a screenshot in the linked bug report). It should have no impact on whole number DSFs (1x, 2x, etc) BUG=627601 ========== to ========== Fix infobar painting issues at fractional scales This calculates the paths for the infobar background (which paints the arrow) with the scale factor taken into account. It fixes blurriness in the arrow as well as a gap above the infobar (you can see both issues in a screenshot in the linked bug report). It should have no impact on whole number DSFs (1x, 2x, etc) BUG=627601 Committed: https://crrev.com/21e70f121e72154c9891b061e9724ef30e1aecb9 Cr-Commit-Position: refs/heads/master@{#408498} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/21e70f121e72154c9891b061e9724ef30e1aecb9 Cr-Commit-Position: refs/heads/master@{#408498} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
