|
|
Created:
3 years, 10 months ago by Evan Stade Modified:
3 years, 10 months ago CC:
chromium-reviews, tfarina Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix appearance of task manager border at fractional scale factors.
BUG=688507
Review-Url: https://codereview.chromium.org/2694933002
Cr-Commit-Position: refs/heads/master@{#452215}
Committed: https://chromium.googlesource.com/chromium/src/+/194bb15b9740165e8ca0ec4a4ee898887876946a
Patch Set 1 #Patch Set 2 : more better #Patch Set 3 : inline #
Total comments: 3
Patch Set 4 : handle snaptopixelbounds case #
Total comments: 2
Patch Set 5 : update tests #Patch Set 6 : more self documenting? #
Total comments: 4
Patch Set 7 : tests #
Total comments: 6
Messages
Total messages: 56 (24 generated)
estade@chromium.org changed reviewers: + pkasting@chromium.org
slightly worried this will break something somewhere, but I poked around and couldn't find that place. Also, if it does break something, it should only be at a fractional scale.
The CQ bit was checked by estade@chromium.org to run a CQ dry run
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...
LGTM https://codereview.chromium.org/2694933002/diff/40001/ui/views/border.cc File ui/views/border.cc (right): https://codereview.chromium.org/2694933002/diff/40001/ui/views/border.cc#newc... ui/views/border.cc:52: gfx::RectF bounds(view.GetLocalBounds()); Nit: Prefer = to () for this (see https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Variab... )
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
estade@chromium.org changed reviewers: + msw@chromium.org
+msw for owners. I'll fix the (weird) border tests tomorrow. https://codereview.chromium.org/2694933002/diff/40001/ui/views/border.cc File ui/views/border.cc (right): https://codereview.chromium.org/2694933002/diff/40001/ui/views/border.cc#newc... ui/views/border.cc:52: gfx::RectF bounds(view.GetLocalBounds()); On 2017/02/17 02:40:15, Peter Kasting wrote: > Nit: Prefer = to () for this (see > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Variab... > ) For rectf from rect you have to use the constructor don't you?
https://codereview.chromium.org/2694933002/diff/40001/ui/views/border.cc File ui/views/border.cc (right): https://codereview.chromium.org/2694933002/diff/40001/ui/views/border.cc#newc... ui/views/border.cc:52: gfx::RectF bounds(view.GetLocalBounds()); On 2017/02/17 06:19:55, Evan Stade wrote: > On 2017/02/17 02:40:15, Peter Kasting wrote: > > Nit: Prefer = to () for this (see > > > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Variab... > > ) > > For rectf from rect you have to use the constructor don't you? Oh, I didn't even see that you were converting integral->floating here. Yeah, that constructor is explicit.
Mike: regarding the now failing border unit tests, do you have an idea how I could update them to something that's useful? In their current form they seem like change-detectors to me. I'm really not sure what kind of regression that could possibly catch. I could update them to make sure we get exactly one onDrawPaint but I don't think that would be a useful test. I vote for just deleting them.
msw@chromium.org changed reviewers: + mgiuca@chromium.org, sadrul@chromium.org
On 2017/02/17 20:42:23, Evan Stade wrote: > Mike: regarding the now failing border unit tests, do you have an idea how I > could update them to something that's useful? In their current form they seem > like change-detectors to me. I'm really not sure what kind of regression that > could possibly catch. I could update them to make sure we get exactly one > onDrawPaint but I don't think that would be a useful test. I vote for just > deleting them. mgiuca@ added them fairly recently, and sadrul@ approved in https://codereview.chromium.org/1517463003 . That CL has substantial discussion about the utility of the tests. I've added them as reviewers here to assess your suggestion, or maybe you could look there and be swayed toward keeping them? I don't have a strong opinion.
ericrk@chromium.org changed reviewers: + ericrk@chromium.org
Glad to see this is being added in! I was trying this change out to see if it would address the clipped-border issue we've seen in crbug.com/685867, caused by fractional scale factors. While this change does ensure that a border is rendered with hard edges, it seems to still result in uneven widths for the resulting border. Wanted to get your thoughts: Note that dimensions below are in the form (left, top, right, bottom): Consider a 202x63 Pixel window with a 1 DIP border and a 1.25x scale factor. 1) When calculating the DIP size for the window's view, we ensure that we don't under-fill the window by rounding up - see WindowTreeHost::UpdateRootWindowSizeInPixels - this means that the view bounds in DIPs is (0, 0, 162, 51). 2) Now, in SolidSidedBorder::Paint, we calculate RectF scaled pixel bounds for the window, arriving at (0, 0, 202.5, 63.75) 3) continuing, the (1, 1, 1, 1) insets are scaled and floored to (1, 1, 1, 1) 4) continuing, the inner bounds of the border end up being (1, 1, 201.5, 62.75) 5) finally, when applied to the window's pixel bounds (0, 0, 202, 63), this leaves us with actual pixel border widths of: (1, 1, 0.5, 0.25). When the border gets filled, rounding ends up making it so the bottom and right portions of the border are not visible (completely clipped). In order to ensure that we get an even border in pixels on all sides of the window, it seems like we want to round down (floor) in step (2), so that the pixel value we're insetting from matches the actual pixel bounds of the window? Let me know if this makes sense?
On 2017/02/17 22:00:45, ericrk wrote: > Glad to see this is being added in! > > I was trying this change out to see if it would address the clipped-border issue > we've seen in crbug.com/685867, caused by fractional scale factors. > > While this change does ensure that a border is rendered with hard edges, it > seems to still result in uneven widths for the resulting border. Wanted to get > your thoughts: > > Note that dimensions below are in the form (left, top, right, bottom): > > Consider a 202x63 Pixel window with a 1 DIP border and a 1.25x scale factor. > 1) When calculating the DIP size for the window's view, we ensure that we don't > under-fill the window by rounding up - see > WindowTreeHost::UpdateRootWindowSizeInPixels - this means that the view bounds > in DIPs is (0, 0, 162, 51). > 2) Now, in SolidSidedBorder::Paint, we calculate RectF scaled pixel bounds for > the window, arriving at (0, 0, 202.5, 63.75) > 3) continuing, the (1, 1, 1, 1) insets are scaled and floored to (1, 1, 1, 1) > 4) continuing, the inner bounds of the border end up being (1, 1, 201.5, 62.75) > 5) finally, when applied to the window's pixel bounds (0, 0, 202, 63), this > leaves us with actual pixel border widths of: (1, 1, 0.5, 0.25). > > When the border gets filled, rounding ends up making it so the bottom and right > portions of the border are not visible (completely clipped). > > In order to ensure that we get an even border in pixels on all sides of the > window, it seems like we want to round down (floor) in step (2), so that the > pixel value we're insetting from matches the actual pixel bounds of the window? > > Let me know if this makes sense? I think it makes sense to clamp the scaled-to-pixel bounds to integral values in this code, assuming the origin of the view is itself pixel-aligned (i.e. the canvas is not translated a fractional number of pixels). I don't know whether that assumption always holds. It's less clear to me that flooring is always correct. It's correct in your case, because the original construction of DIP values from pixel ones ceiled. But is that always going to be true, e.g. for borders on views that are not "the full content of a widget" but just some small child within the widget?
On 2017/02/17 22:00:45, ericrk wrote: > Glad to see this is being added in! > > I was trying this change out to see if it would address the clipped-border issue > we've seen in crbug.com/685867, caused by fractional scale factors. > > While this change does ensure that a border is rendered with hard edges, it > seems to still result in uneven widths for the resulting border. Wanted to get > your thoughts: > > Note that dimensions below are in the form (left, top, right, bottom): > > Consider a 202x63 Pixel window with a 1 DIP border and a 1.25x scale factor. > 1) When calculating the DIP size for the window's view, we ensure that we don't > under-fill the window by rounding up - see > WindowTreeHost::UpdateRootWindowSizeInPixels - this means that the view bounds > in DIPs is (0, 0, 162, 51). > 2) Now, in SolidSidedBorder::Paint, we calculate RectF scaled pixel bounds for > the window, arriving at (0, 0, 202.5, 63.75) > 3) continuing, the (1, 1, 1, 1) insets are scaled and floored to (1, 1, 1, 1) > 4) continuing, the inner bounds of the border end up being (1, 1, 201.5, 62.75) > 5) finally, when applied to the window's pixel bounds (0, 0, 202, 63), this > leaves us with actual pixel border widths of: (1, 1, 0.5, 0.25). > > When the border gets filled, rounding ends up making it so the bottom and right > portions of the border are not visible (completely clipped). > > In order to ensure that we get an even border in pixels on all sides of the > window, it seems like we want to round down (floor) in step (2), so that the > pixel value we're insetting from matches the actual pixel bounds of the window? > > Let me know if this makes sense? I think the difference here is whether we're snapping to pixel boundaries. Indeed this patch only works if we are NOT snapping to pixel boundaries. I think we can check for this just by consulting view.layer(). The updated patch works for both the task manager and the hypothetical situation where the task manager's scroll area (tab_table_parent_) is set to use a layer.
https://codereview.chromium.org/2694933002/diff/60001/ui/views/border.cc File ui/views/border.cc (right): https://codereview.chromium.org/2694933002/diff/60001/ui/views/border.cc#newc... ui/views/border.cc:60: bounds = gfx::RectF(gfx::ToEnclosingRect(bounds)); Why ToEnclosingRect() and not ToEnclosedRect()? The latter seems more correct to me in the example Eric gave.
On 2017/02/18 00:08:28, Peter Kasting wrote: > https://codereview.chromium.org/2694933002/diff/60001/ui/views/border.cc > File ui/views/border.cc (right): > > https://codereview.chromium.org/2694933002/diff/60001/ui/views/border.cc#newc... > ui/views/border.cc:60: bounds = gfx::RectF(gfx::ToEnclosingRect(bounds)); > Why ToEnclosingRect() and not ToEnclosedRect()? The latter seems more correct > to me in the example Eric gave. I don't think Eric's example is correct. In step 5, the window bounds should be: (0, 0, 203, 64). At least, that's how I understand the pixel-boundary snapping to work, and this code works in both the layered-view and layerless-view cases.
On 2017/02/18 00:19:20, Evan Stade wrote: > On 2017/02/18 00:08:28, Peter Kasting wrote: > > https://codereview.chromium.org/2694933002/diff/60001/ui/views/border.cc > > File ui/views/border.cc (right): > > > > > https://codereview.chromium.org/2694933002/diff/60001/ui/views/border.cc#newc... > > ui/views/border.cc:60: bounds = gfx::RectF(gfx::ToEnclosingRect(bounds)); > > Why ToEnclosingRect() and not ToEnclosedRect()? The latter seems more correct > > to me in the example Eric gave. > > I don't think Eric's example is correct. In step 5, the window bounds should be: > (0, 0, 203, 64). At least, that's how I understand the pixel-boundary snapping > to work, and this code works in both the layered-view and layerless-view cases. AFAIK Eric is correct and the window's pixel bounds are smaller than its DIP bounds in this case; see discussion on https://codereview.chromium.org/2694183005/ for more gory details.
https://codereview.chromium.org/2694933002/diff/60001/ui/views/border.cc File ui/views/border.cc (right): https://codereview.chromium.org/2694933002/diff/60001/ui/views/border.cc#newc... ui/views/border.cc:60: bounds = gfx::RectF(gfx::ToEnclosingRect(bounds)); On 2017/02/18 00:08:28, Peter Kasting wrote: > Why ToEnclosingRect() and not ToEnclosedRect()? The latter seems more correct > to me in the example Eric gave. Agreed. Interestingly, it appears that the view.layer() check doesn't fully cover the case I'm dealing with (the autofill popup). In the autofill popup case, the view does not seem to have a layer. Need to understand things a bit better, but in the autofill case, the AutofillPopupBaseView creates a new Widget and acts as a WidgetDelegateView. The widget itself is pixel sized, and the view appears to draw to the widget without requiring a layer.
On 2017/02/18 00:25:48, Peter Kasting wrote: > On 2017/02/18 00:19:20, Evan Stade wrote: > > On 2017/02/18 00:08:28, Peter Kasting wrote: > > > https://codereview.chromium.org/2694933002/diff/60001/ui/views/border.cc > > > File ui/views/border.cc (right): > > > > > > > > > https://codereview.chromium.org/2694933002/diff/60001/ui/views/border.cc#newc... > > > ui/views/border.cc:60: bounds = gfx::RectF(gfx::ToEnclosingRect(bounds)); > > > Why ToEnclosingRect() and not ToEnclosedRect()? The latter seems more > correct > > > to me in the example Eric gave. > > > > I don't think Eric's example is correct. In step 5, the window bounds should > be: > > (0, 0, 203, 64). At least, that's how I understand the pixel-boundary snapping > > to work, and this code works in both the layered-view and layerless-view > cases. > > AFAIK Eric is correct and the window's pixel bounds are smaller than its DIP > bounds in this case; see discussion on > https://codereview.chromium.org/2694183005/ for more gory details. from what I can tell, the case I'm hitting seems to be a case where the view is directly rendered to a widget (with no layer created), see my code comment. I wonder if our rounding behaves differently in that case than in the layer case? If you want to try this out, just visit any site with an autofill popup (https://rsolomakhin.github.io/autofill/), at 1.25x scale factor, the borders on popups end up disappearing.
On 2017/02/18 00:43:56, ericrk wrote: > On 2017/02/18 00:25:48, Peter Kasting wrote: > > On 2017/02/18 00:19:20, Evan Stade wrote: > > > On 2017/02/18 00:08:28, Peter Kasting wrote: > > > > https://codereview.chromium.org/2694933002/diff/60001/ui/views/border.cc > > > > File ui/views/border.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2694933002/diff/60001/ui/views/border.cc#newc... > > > > ui/views/border.cc:60: bounds = gfx::RectF(gfx::ToEnclosingRect(bounds)); > > > > Why ToEnclosingRect() and not ToEnclosedRect()? The latter seems more > > correct > > > > to me in the example Eric gave. > > > > > > I don't think Eric's example is correct. In step 5, the window bounds should > > be: > > > (0, 0, 203, 64). At least, that's how I understand the pixel-boundary > snapping > > > to work, and this code works in both the layered-view and layerless-view > > cases. > > > > AFAIK Eric is correct and the window's pixel bounds are smaller than its DIP > > bounds in this case; see discussion on > > https://codereview.chromium.org/2694183005/ for more gory details. > > from what I can tell, the case I'm hitting seems to be a case where the view is > directly rendered to a widget (with no layer created), see my code comment. I > wonder if our rounding behaves differently in that case than in the layer case? > > If you want to try this out, just visit any site with an autofill popup > (https://rsolomakhin.github.io/autofill/), at 1.25x scale factor, the borders > on popups end up disappearing. I believe this patch to be correct conceptually and in practice. It sounds like a bug that windows are truncating their contents (ie smaller pixel size than dip*dsf). If that's what we're doing then it's perfectly correct that autofill borders are missing. I'd think windows should round up and then certain views within the window may need to snap to pixel bounds.
On 2017/02/18 00:43:56, ericrk wrote: > On 2017/02/18 00:25:48, Peter Kasting wrote: > > On 2017/02/18 00:19:20, Evan Stade wrote: > > > On 2017/02/18 00:08:28, Peter Kasting wrote: > > > > https://codereview.chromium.org/2694933002/diff/60001/ui/views/border.cc > > > > File ui/views/border.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2694933002/diff/60001/ui/views/border.cc#newc... > > > > ui/views/border.cc:60: bounds = gfx::RectF(gfx::ToEnclosingRect(bounds)); > > > > Why ToEnclosingRect() and not ToEnclosedRect()? The latter seems more > > correct > > > > to me in the example Eric gave. > > > > > > I don't think Eric's example is correct. In step 5, the window bounds should > > be: > > > (0, 0, 203, 64). At least, that's how I understand the pixel-boundary > snapping > > > to work, and this code works in both the layered-view and layerless-view > > cases. > > > > AFAIK Eric is correct and the window's pixel bounds are smaller than its DIP > > bounds in this case; see discussion on > > https://codereview.chromium.org/2694183005/ for more gory details. > > from what I can tell, the case I'm hitting seems to be a case where the view is > directly rendered to a widget (with no layer created), see my code comment. I > wonder if our rounding behaves differently in that case than in the layer case? > > If you want to try this out, just visit any site with an autofill popup > (https://rsolomakhin.github.io/autofill/), at 1.25x scale factor, the borders > on popups end up disappearing. I believe this patch to be correct conceptually and in practice. It sounds like a bug that windows are truncating their contents (ie smaller pixel size than dip*dsf). If that's what we're doing then it's perfectly correct that autofill borders are missing. I'd think windows should round up and then certain views within the window may need to snap to pixel bounds.
On 2017/02/18 01:32:30, Evan Stade wrote: > On 2017/02/18 00:43:56, ericrk wrote: > > On 2017/02/18 00:25:48, Peter Kasting wrote: > > > On 2017/02/18 00:19:20, Evan Stade wrote: > > > > On 2017/02/18 00:08:28, Peter Kasting wrote: > > > > > https://codereview.chromium.org/2694933002/diff/60001/ui/views/border.cc > > > > > File ui/views/border.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2694933002/diff/60001/ui/views/border.cc#newc... > > > > > ui/views/border.cc:60: bounds = > gfx::RectF(gfx::ToEnclosingRect(bounds)); > > > > > Why ToEnclosingRect() and not ToEnclosedRect()? The latter seems more > > > correct > > > > > to me in the example Eric gave. > > > > > > > > I don't think Eric's example is correct. In step 5, the window bounds > should > > > be: > > > > (0, 0, 203, 64). At least, that's how I understand the pixel-boundary > > snapping > > > > to work, and this code works in both the layered-view and layerless-view > > > cases. > > > > > > AFAIK Eric is correct and the window's pixel bounds are smaller than its DIP > > > bounds in this case; see discussion on > > > https://codereview.chromium.org/2694183005/ for more gory details. > > > > from what I can tell, the case I'm hitting seems to be a case where the view > is > > directly rendered to a widget (with no layer created), see my code comment. I > > wonder if our rounding behaves differently in that case than in the layer > case? > > > > If you want to try this out, just visit any site with an autofill popup > > (https://rsolomakhin.github.io/autofill/), at 1.25x scale factor, the borders > > on popups end up disappearing. > > I believe this patch to be correct conceptually and in practice. I'm willing to buy that, but I asked because I honestly don't have any instinctive feel for why Enclosing vs. Enclosed is more correct, and we know in practice it doesn't produce good-looking results for autofill. Is there somewhere else it will produce bad-looking results for if we switch to Enclosed? More generally, can you articulate better why conceptually using the enclosing rect is the right thing for this code to do? > It sounds like > a bug that windows are truncating their contents (ie smaller pixel size than > dip*dsf). It's what we're doing, but it's not a bug for now. At fractional DSFs, we always have the risk that windows will be larger (in px) than the desired number of DIPs or smaller, which will lead to underpainting or clipping, respectively. There's no way around this without doing the DIP-to-px conversion, which should fix it for real. Until then, we pick which bad consequence we want, and we picked clipping. (We used to pick underpainting, and people decided that was worse.) But as a result, this means we know that if a DIP window size is converted to a px window size, it should use the enclosed rect rather than the enclosing rect. This is part of why conceptually it makes sense to me to do that here too. > If that's what we're doing then it's perfectly correct that autofill > borders are missing. I almost wrote that earlier, but I want to make sure we're really doing the right thing here before we Abandon All Hope. > I'd think windows should round up and then certain views > within the window may need to snap to pixel bounds. Should the top-level content view in a widget always paint to a layer (to get pixel-snapping), then? Would that address this?
On 2017/02/17 21:00:30, msw wrote: > On 2017/02/17 20:42:23, Evan Stade wrote: > > Mike: regarding the now failing border unit tests, do you have an idea how I > > could update them to something that's useful? In their current form they seem > > like change-detectors to me. I'm really not sure what kind of regression that > > could possibly catch. I could update them to make sure we get exactly one > > onDrawPaint but I don't think that would be a useful test. I vote for just > > deleting them. > > mgiuca@ added them fairly recently, and sadrul@ approved in > https://codereview.chromium.org/1517463003 . That CL has substantial discussion > about the utility of the tests. I've added them as reviewers here to assess your > suggestion, or maybe you could look there and be swayed toward keeping them? I > don't have a strong opinion. Hi, It looks like you've run into the fact that these tests expect four rectangles to be drawn and you've changed the code to draw using a clipping area. Is there a reason you have to use that technique (and can't just change the size of the four rectangles that get drawn)? Otherwise, I think you have to update the MockCanvas in the test to capture whatever low-level draw commands are sent to the SkCanvas and then check that they match your expectations. It's unfortunate that our tests here care about the specific draw commands that are used, but that is simply the nature of graphics testing (unless you want to go into pixel testing, and then suffer having to potentially maintain different tests on different platforms and rebaseline every so often as the Blink folks are constantly having to do -- I know from a recent sheriffing stint!) These tests aren't *just* testing the specific approach we use -- this code does some non-trivial calculations and it is definitely worth hard-coding the expected outputs of those calculations in tests and making sure they don't regress (or updating them if we decide we need different answers as we have here). To answer your question, "I'm really not sure what kind of regression that could possibly catch.": https://crbug.com/565801. I inadvertently created this bug when I changed border (which had absolutely no tests), then felt compelled to add tests to fix any such calculation issues in Border in the future. What followed was literally 6 months of arguing about whether the tests were useful or merely "change detectors" and rewriting them several times. Please don't delete them.
On 2017/02/19 23:38:54, Matt Giuca wrote: > Hi, > > It looks like you've run into the fact that these tests expect four rectangles > to be drawn and you've changed the code to draw using a clipping area. Is there > a reason you have to use that technique (and can't just change the size of the > four rectangles that get drawn)? Because drawing four rectangles seems like a really roundabout way to accomplish the goal, but more importantly because it made the code for getting the corners right at fractional scales really complicated. > Otherwise, I think you have to update the > MockCanvas in the test to capture whatever low-level draw commands are sent to > the SkCanvas and then check that they match your expectations. The low level draw command is a single drawPaint. I've updated the test. > What followed was literally 6 months of arguing about whether the tests were useful or merely "change detectors" and rewriting them several times. I'll leave the subject alone then :)
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...
On 2017/02/18 01:40:03, Peter Kasting wrote: > > I believe this patch to be correct conceptually and in practice. > > I'm willing to buy that, but I asked because I honestly don't have any > instinctive feel for why Enclosing vs. Enclosed is more correct, and we know in > practice it doesn't produce good-looking results for autofill. Is there > somewhere else it will produce bad-looking results for if we switch to Enclosed? The hypothetical situation where we give a layer to the task manager's tab_table_parent_ exhibits bad results (sometimes double-width borders) with Enclosed. In the real world, that view doesn't have a layer so the enclosing/enclosed decision is moot. > More generally, can you articulate better why conceptually using the enclosing > rect is the right thing for this code to do? I've updated the code to use ConvertRectToPixel from ui/compositor/dip_util, which is the same thing but hopefully more enlightening to the next person who reads this code. The comment in that function is: // Use ToEnclosingRect() to ensure we paint all the possible pixels // touched. ToEnclosingRect() floors the origin, and ceils the max // coordinate. To do otherwise (such as flooring the size) potentially // results in rounding down and not drawing all the pixels that are // touched. > Should the top-level content view in a widget always paint to a layer (to get > pixel-snapping), then? Would that address this? I have a feeling that something along these lines could potentially work.
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...
On 2017/02/21 19:56:58, Evan Stade wrote: > On 2017/02/18 01:40:03, Peter Kasting wrote: > > Should the top-level content view in a widget always paint to a layer (to get > > pixel-snapping), then? Would that address this? > > I have a feeling that something along these lines could potentially work. Along those lines, something I mentioned to Eric last week was that adding a layer was kludgy and sometimes leads to unwanted consequences re: clipping, z-order, etc. We do use it sometimes though: https://cs.chromium.org/chromium/src/ui/views/controls/button/md_text_button.... (aside: I'm now wondering if we can avoid adding this layer by doing something similar to what's in this CL) I haven't investigated the feasibility but it might be nice to modify views::View painting/clipping so that we could enable pixel alignment for the canvas without adding a layer.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
On 2017/02/21 20:06:41, Evan Stade wrote: > On 2017/02/21 19:56:58, Evan Stade wrote: > > On 2017/02/18 01:40:03, Peter Kasting wrote: > > > Should the top-level content view in a widget always paint to a layer (to > get > > > pixel-snapping), then? Would that address this? > > > > I have a feeling that something along these lines could potentially work. > > Along those lines, something I mentioned to Eric last week was that adding a > layer was kludgy and sometimes leads to unwanted consequences re: clipping, > z-order, etc. We do use it sometimes though: > https://cs.chromium.org/chromium/src/ui/views/controls/button/md_text_button.... > (aside: I'm now wondering if we can avoid adding this layer by doing something > similar to what's in this CL) > > I haven't investigated the feasibility but it might be nice to modify > views::View painting/clipping so that we could enable pixel alignment for the > canvas without adding a layer. FYI - Tried changing the popup to paint to a layer, but with this CL applied, the border is still clipped. In order to un-clip the border, I need to also change WindowTreeHost to round-down (underfill) when calculating view DIPs from widget pixels. With that done, the promotion to a layer does result in good looking borders. However the window tree host change re-introduces the black borders on the main app window. Maybe we could update the main window to fix this - need to try this out.
On 2017/02/21 22:04:51, ericrk wrote: > On 2017/02/21 20:06:41, Evan Stade wrote: > > On 2017/02/21 19:56:58, Evan Stade wrote: > > > On 2017/02/18 01:40:03, Peter Kasting wrote: > > > > Should the top-level content view in a widget always paint to a layer (to > > get > > > > pixel-snapping), then? Would that address this? > > > > > > I have a feeling that something along these lines could potentially work. > > > > Along those lines, something I mentioned to Eric last week was that adding a > > layer was kludgy and sometimes leads to unwanted consequences re: clipping, > > z-order, etc. We do use it sometimes though: > > > https://cs.chromium.org/chromium/src/ui/views/controls/button/md_text_button.... > > (aside: I'm now wondering if we can avoid adding this layer by doing something > > similar to what's in this CL) > > > > I haven't investigated the feasibility but it might be nice to modify > > views::View painting/clipping so that we could enable pixel alignment for the > > canvas without adding a layer. > > FYI - Tried changing the popup to paint to a layer, but with this CL applied, > the border is still clipped. In order to un-clip the border, I need to also > change WindowTreeHost to round-down (underfill) when calculating view DIPs from > widget pixels. With that done, the promotion to a layer does result in good > looking borders. However the window tree host change re-introduces the black > borders on the main app window. Maybe we could update the main window to fix > this - need to try this out. OK, after digging a bit more, the reason this code isn't working for my case is because we have two different Pixel > DIP rounding techniques in use in the code base: - WindowTreeHost and ScreenToDIPRectInWindow convert Pixels > DIPs using an enclosing rect operation. - ui::ConvertRectToDIP, converts using an enclosed rect. While we have this difference, it's going to be hard to make a drawing utility (like border) behave consistently in all cases, whether or not we draw to a layer / etc... I think we can land this change as-is, and then decide whether we want to update the rounding behavior of these utilities independently. I've confirmed that if we ensure that our various DIP/Pixel conversions are consistent and round-trippable (one uses enclosing rect, the other enclosed), the code as written here works great. LGTM
https://codereview.chromium.org/2694933002/diff/100001/ui/views/border_unitte... File ui/views/border_unittest.cc (right): https://codereview.chromium.org/2694933002/diff/100001/ui/views/border_unitte... ui/views/border_unittest.cc:88: void onDrawPaint(const SkPaint& paint) override { ++draw_paint_call_count_; } Can we record the SkPaint object into a set like draw_rect_calls_, instead of just counting? Then we can check that it was actually painted correctly. https://codereview.chromium.org/2694933002/diff/100001/ui/views/border_unitte... ui/views/border_unittest.cc:180: EXPECT_EQ(1, sk_canvas_->draw_paint_call_count()); Expect that the size of the draw_paint_calls set is 1, and verify that the single paint has color SK_ColorBLUE.
https://codereview.chromium.org/2694933002/diff/100001/ui/views/border_unitte... File ui/views/border_unittest.cc (right): https://codereview.chromium.org/2694933002/diff/100001/ui/views/border_unitte... ui/views/border_unittest.cc:88: void onDrawPaint(const SkPaint& paint) override { ++draw_paint_call_count_; } On 2017/02/21 23:18:51, Matt Giuca wrote: > Can we record the SkPaint object into a set like draw_rect_calls_, instead of > just counting? > > Then we can check that it was actually painted correctly. done https://codereview.chromium.org/2694933002/diff/100001/ui/views/border_unitte... ui/views/border_unittest.cc:180: EXPECT_EQ(1, sk_canvas_->draw_paint_call_count()); On 2017/02/21 23:18:51, Matt Giuca wrote: > Expect that the size of the draw_paint_calls set is 1, and verify that the > single paint has color SK_ColorBLUE. Done.
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...
lgtm (with a few queries). Thanks for dealing with the tests. https://codereview.chromium.org/2694933002/diff/120001/ui/views/border_unitte... File ui/views/border_unittest.cc (right): https://codereview.chromium.org/2694933002/diff/120001/ui/views/border_unitte... ui/views/border_unittest.cc:105: // Stores the onDrawPaint calls in chronological order. I suppose the reason for this being a vector and not a set is that (unlike the rects we had previously), if we did get more than one draw here we *would* care about the order? https://codereview.chromium.org/2694933002/diff/120001/ui/views/border_unitte... ui/views/border_unittest.cc:178: const SkColor kBorderColor = SK_ColorMAGENTA; Any reason to change this to magenta? (Perhaps just to have different tests drawing different colours to test that it isn't just hard-coded to BLUE?)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
estade@chromium.org changed reviewers: + sky@chromium.org
+sky as msw may be ooo https://codereview.chromium.org/2694933002/diff/120001/ui/views/border_unitte... File ui/views/border_unittest.cc (right): https://codereview.chromium.org/2694933002/diff/120001/ui/views/border_unitte... ui/views/border_unittest.cc:105: // Stores the onDrawPaint calls in chronological order. On 2017/02/22 03:29:09, Matt Giuca wrote: > I suppose the reason for this being a vector and not a set is that (unlike the > rects we had previously), if we did get more than one draw here we *would* care > about the order? It's hard to hypothesize about what we would want if there were more than one draw call. A set seemed overkill because then I'd have to think up and implement some way of sorting SkPaints which is something we don't actually need at this point. https://codereview.chromium.org/2694933002/diff/120001/ui/views/border_unitte... ui/views/border_unittest.cc:178: const SkColor kBorderColor = SK_ColorMAGENTA; On 2017/02/22 03:29:09, Matt Giuca wrote: > Any reason to change this to magenta? > > (Perhaps just to have different tests drawing different colours to test that it > isn't just hard-coded to BLUE?) no real reason. I guess I thought it was less likely to be used in any real life scenario.
LGTM
LGTM https://codereview.chromium.org/2694933002/diff/120001/ui/views/border_unitte... File ui/views/border_unittest.cc (right): https://codereview.chromium.org/2694933002/diff/120001/ui/views/border_unitte... ui/views/border_unittest.cc:178: const SkColor kBorderColor = SK_ColorMAGENTA; On 2017/02/22 16:59:23, Evan Stade wrote: > On 2017/02/22 03:29:09, Matt Giuca wrote: > > Any reason to change this to magenta? > > > > (Perhaps just to have different tests drawing different colours to test that > it > > isn't just hard-coded to BLUE?) > > no real reason. I guess I thought it was less likely to be used in any real life > scenario. Choose border color using random number generator? :)
The CQ bit was checked by estade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ericrk@chromium.org Link to the patchset: https://codereview.chromium.org/2694933002/#ps120001 (title: "tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 120001, "attempt_start_ts": 1487793887336010, "parent_rev": "9ac53b8c2ecfd5facddbdc82bfb4c404f34b6d72", "commit_rev": "194bb15b9740165e8ca0ec4a4ee898887876946a"}
Message was sent while issue was closed.
Description was changed from ========== Fix appearance of task manager border at fractional scale factors. BUG=688507 ========== to ========== Fix appearance of task manager border at fractional scale factors. BUG=688507 Review-Url: https://codereview.chromium.org/2694933002 Cr-Commit-Position: refs/heads/master@{#452215} Committed: https://chromium.googlesource.com/chromium/src/+/194bb15b9740165e8ca0ec4a4ee8... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/194bb15b9740165e8ca0ec4a4ee8...
Message was sent while issue was closed.
https://codereview.chromium.org/2694933002/diff/120001/ui/views/border_unitte... File ui/views/border_unittest.cc (right): https://codereview.chromium.org/2694933002/diff/120001/ui/views/border_unitte... ui/views/border_unittest.cc:178: const SkColor kBorderColor = SK_ColorMAGENTA; On 2017/02/22 19:07:12, Peter Kasting (backlogged) wrote: > On 2017/02/22 16:59:23, Evan Stade wrote: > > On 2017/02/22 03:29:09, Matt Giuca wrote: > > > Any reason to change this to magenta? > > > > > > (Perhaps just to have different tests drawing different colours to test that > > it > > > isn't just hard-coded to BLUE?) > > > > no real reason. I guess I thought it was less likely to be used in any real > life > > scenario. > > Choose border color using random number generator? :) And choose the expected border color with a 98% chance to be the same as the set one. A little puzzle for the sheriffs :) |