Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2247563002:
Change status bubble rendering at hidpi and when there is no client edge (Closed)
|
Created:
4 years, 4 months ago by Bret Modified:
4 years, 3 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. |
DescriptionChange status bubble rendering at hidpi and when there is no client edge
The recently removed client edge on Windows 10 made the status bubble
overlap the window edge. This patch clips out the bubble border when the
bubble is docked, and leaves it unchanged when it's floating. Also made
the border always 1 pixel even at hidpi to make it look more consistent
with the material UI.
BUG=636479
Committed: https://crrev.com/e5a8ba6722632879cfa09ef0e7f8d7f4a99c5d4e
Cr-Commit-Position: refs/heads/master@{#415216}
Patch Set 1 #
Total comments: 16
Patch Set 2 : refactored based on comments #
Total comments: 4
Patch Set 3 : fix clang compile #Patch Set 4 : adjust layout #Patch Set 5 : remove tmp file #
Total comments: 14
Patch Set 6 : nits #Patch Set 7 : correct inset #
Total comments: 10
Patch Set 8 : comments and nits #
Total comments: 2
Messages
Total messages: 33 (18 generated)
Description was changed from ========== Change status bubble rendering at hidpi and when there is no client edge The recently removed client edge on Windows 10 made the status bubble overlap the window edge. This patch clips out the bubble border when the bubble is docked, and leaves it unchanged when it's floating. Also made the border always 1 pixel even at hidpi to make it look more consistent with the material UI. BUG=636479 ========== to ========== Change status bubble rendering at hidpi and when there is no client edge The recently removed client edge on Windows 10 made the status bubble overlap the window edge. This patch clips out the bubble border when the bubble is docked, and leaves it unchanged when it's floating. Also made the border always 1 pixel even at hidpi to make it look more consistent with the material UI. BUG=636479 ==========
bsep@chromium.org changed reviewers: + pkasting@chromium.org
There are still some layout issues at hidpi but they're caused by the status bubble being its own window similar to the omnibox popup. I'm punting on those for now. This at least makes 1x look great.
https://codereview.chromium.org/2247563002/diff/1/chrome/browser/ui/views/sta... File chrome/browser/ui/views/status_bubble_views.cc (right): https://codereview.chromium.org/2247563002/diff/1/chrome/browser/ui/views/sta... chrome/browser/ui/views/status_bubble_views.cc:375: gfx::Rect popup_bounds = popup_->GetWindowBoundsInScreen(); Nit: I would avoid GetWindowBoundsInScreen() here if you can, because it does a lossy conversion to DIPs. Plus you only care about the width and height anyway, not the origin, so it seems like just using size() or some similar thing (popup_->size()? I dunno) ought to work. This is actually true of all three places in this file that use GetWindowBoundsInScreen(). I don't know why we're using that at all. https://codereview.chromium.org/2247563002/diff/1/chrome/browser/ui/views/sta... chrome/browser/ui/views/status_bubble_views.cc:379: const int radius = std::round(kBubbleCornerRadius * scale); Seems like we should just use a float, not round. https://codereview.chromium.org/2247563002/diff/1/chrome/browser/ui/views/sta... chrome/browser/ui/views/status_bubble_views.cc:414: const int height = std::round(popup_bounds.height() * scale); Similarly, I think we should use floats here and RectFs below instead of rounding. https://codereview.chromium.org/2247563002/diff/1/chrome/browser/ui/views/sta... chrome/browser/ui/views/status_bubble_views.cc:416: if (!browser_view_->HasClientEdge()) { This function's return value isn't going to change over the life of the status bubble, so rather than passing in |browser_view_| so it can call this, pass in this bool directly. (This sort of pattern of passing in the direct state values where possible is generally encouraged because it makes designs more testable, since tests don't have to create an entire fake object to pass in.) https://codereview.chromium.org/2247563002/diff/1/chrome/browser/ui/views/sta... chrome/browser/ui/views/status_bubble_views.cc:430: rrect.setRectRadii(RectToSkRect(bubble_rect), (const SkVector*)rad); The reason you're having layout issues at higher DPI factors is because you're painting the "shadow" flush inside the outside edge of the bubble's bounds. But you're painting a 1 px shadow, while the outdent of those bounds (set in Reposition() below) is 1 dip. A good clue that there are problems here is that in the function here you use kShadowThickness as a px value, but later in the file it's used as a dip value. This means there's a unit mismatch. Instead, you need to paint the 1 px shadow flush outside the "inside" of this 1 dip outdent. This is basically what BackgroundWith1PxBorder::Paint() does, although I don't _think_ you want the floor() call it has in there (but I'm not totally sure; testing in e.g. 1.5x would probably make it clear). (Hopefully we don't get bitten by how the window itself is positioned using DIPs, resulting in one of those scenarios where at higher scales, the status bubble is offset versus the main window depending on the screen position of the main window.) https://codereview.chromium.org/2247563002/diff/1/chrome/browser/ui/views/sta... chrome/browser/ui/views/status_bubble_views.cc:432: SkPaint shadow_paint; Nit: No need to use a different SkPaint here than below. Just declare one, and change the color of it in between. https://codereview.chromium.org/2247563002/diff/1/chrome/browser/ui/views/sta... chrome/browser/ui/views/status_bubble_views.cc:439: rrect.setRectRadii(RectToSkRect(bubble_rect), (const SkVector*)rad); This is another place that's incorrect, and should be doing what BackgroundWith1PxBorder::Paint() does. Instead of drawing rrects with the same radii inside each other, we should compute the fill region by subtracting the stroke's fill path from the fill's. Drawing equal-radius rects inside each other like we do here means the outer curvature of the inner rrect won't meet up exactly with the inner curvature of the outer rrect. Computing the fill path based on the stroke path, by contrast, means they'll fit each other exactly. (This gets more noticeable as you scale up; running on a 1x screen with --force-device-scale-factor=4 makes it fairly obvious.) https://codereview.chromium.org/2247563002/diff/1/chrome/browser/ui/views/sta... chrome/browser/ui/views/status_bubble_views.cc:463: const gfx::FontList font_list; Nit: Can we just pass FontList() in the call below?
https://codereview.chromium.org/2247563002/diff/1/chrome/browser/ui/views/sta... File chrome/browser/ui/views/status_bubble_views.cc (right): https://codereview.chromium.org/2247563002/diff/1/chrome/browser/ui/views/sta... chrome/browser/ui/views/status_bubble_views.cc:375: gfx::Rect popup_bounds = popup_->GetWindowBoundsInScreen(); On 2016/08/13 06:13:07, Peter Kasting wrote: > Nit: I would avoid GetWindowBoundsInScreen() here if you can, because it does a > lossy conversion to DIPs. Plus you only care about the width and height anyway, > not the origin, so it seems like just using size() or some similar thing > (popup_->size()? I dunno) ought to work. > > This is actually true of all three places in this file that use > GetWindowBoundsInScreen(). I don't know why we're using that at all. I played around with this and I think GetWindowBoundsInScreen is actually correct. There's no Widget::Size or otherwise simpler getter, and the size_ field from StatusBubbleViews doesn't respond to changes (e.g. when it expands to fit a large URL). I agree that it's confusing (especially because it raises the question of what size_ is used for) but untangling it is out of scope for this patch. https://codereview.chromium.org/2247563002/diff/1/chrome/browser/ui/views/sta... chrome/browser/ui/views/status_bubble_views.cc:379: const int radius = std::round(kBubbleCornerRadius * scale); On 2016/08/13 06:13:07, Peter Kasting wrote: > Seems like we should just use a float, not round. Done. https://codereview.chromium.org/2247563002/diff/1/chrome/browser/ui/views/sta... chrome/browser/ui/views/status_bubble_views.cc:414: const int height = std::round(popup_bounds.height() * scale); On 2016/08/13 06:13:07, Peter Kasting wrote: > Similarly, I think we should use floats here and RectFs below instead of > rounding. Done. https://codereview.chromium.org/2247563002/diff/1/chrome/browser/ui/views/sta... chrome/browser/ui/views/status_bubble_views.cc:416: if (!browser_view_->HasClientEdge()) { On 2016/08/13 06:13:07, Peter Kasting wrote: > This function's return value isn't going to change over the life of the status > bubble, so rather than passing in |browser_view_| so it can call this, pass in > this bool directly. > > (This sort of pattern of passing in the direct state values where possible is > generally encouraged because it makes designs more testable, since tests don't > have to create an entire fake object to pass in.) Ok, I was thinking the value could change over the life of the status bubble, due to changing the frame by installing a theme for example. But you seem to be saying this object would get destroyed in that case, so I changed the plumbing. In the same spirit I removed ThemeProvider and just passed in the colors directly. https://codereview.chromium.org/2247563002/diff/1/chrome/browser/ui/views/sta... chrome/browser/ui/views/status_bubble_views.cc:430: rrect.setRectRadii(RectToSkRect(bubble_rect), (const SkVector*)rad); On 2016/08/13 06:13:07, Peter Kasting wrote: > The reason you're having layout issues at higher DPI factors is because you're > painting the "shadow" flush inside the outside edge of the bubble's bounds. But > you're painting a 1 px shadow, while the outdent of those bounds (set in > Reposition() below) is 1 dip. A good clue that there are problems here is that > in the function here you use kShadowThickness as a px value, but later in the > file it's used as a dip value. This means there's a unit mismatch. > > Instead, you need to paint the 1 px shadow flush outside the "inside" of this 1 > dip outdent. This is basically what BackgroundWith1PxBorder::Paint() does, > although I don't _think_ you want the floor() call it has in there (but I'm not > totally sure; testing in e.g. 1.5x would probably make it clear). (Hopefully we > don't get bitten by how the window itself is positioned using DIPs, resulting in > one of those scenarios where at higher scales, the status bubble is offset > versus the main window depending on the screen position of the main window.) I changed the painting to work the same way as BackgroundWith1PxBorder which makes the roundrect look better, but the layout issues I was referring to is exactly what you're saying in your parenthetical. I managed to find an easy hack that makes it look okay at factional dpi. It still hangs off the edge of the window a little bit, but I won't try to fix that now. The code ended up a bit duplicated with BackgroundWith1PxBorder but I'm not sure there's a good way to refactor it. I kept kShadowThickness even though the shadow thickness is now 1 pixel instead of 1 dip because there's no good way to actually lay the bubble out in pixels, so I think it just has to live on as a misnomer for a while. I thought about changing its name but I can't think of anything else that makes sense. https://codereview.chromium.org/2247563002/diff/1/chrome/browser/ui/views/sta... chrome/browser/ui/views/status_bubble_views.cc:432: SkPaint shadow_paint; On 2016/08/13 06:13:07, Peter Kasting wrote: > Nit: No need to use a different SkPaint here than below. Just declare one, and > change the color of it in between. Done. https://codereview.chromium.org/2247563002/diff/1/chrome/browser/ui/views/sta... chrome/browser/ui/views/status_bubble_views.cc:439: rrect.setRectRadii(RectToSkRect(bubble_rect), (const SkVector*)rad); On 2016/08/13 06:13:07, Peter Kasting wrote: > This is another place that's incorrect, and should be doing what > BackgroundWith1PxBorder::Paint() does. Instead of drawing rrects with the same > radii inside each other, we should compute the fill region by subtracting the > stroke's fill path from the fill's. Drawing equal-radius rects inside each > other like we do here means the outer curvature of the inner rrect won't meet up > exactly with the inner curvature of the outer rrect. Computing the fill path > based on the stroke path, by contrast, means they'll fit each other exactly. > (This gets more noticeable as you scale up; running on a 1x screen with > --force-device-scale-factor=4 makes it fairly obvious.) Done. See above comment. https://codereview.chromium.org/2247563002/diff/1/chrome/browser/ui/views/sta... chrome/browser/ui/views/status_bubble_views.cc:463: const gfx::FontList font_list; On 2016/08/13 06:13:07, Peter Kasting wrote: > Nit: Can we just pass FontList() in the call below? Done.
The CQ bit was checked by bsep@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by bsep@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.
https://codereview.chromium.org/2247563002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/status_bubble_views.cc (right): https://codereview.chromium.org/2247563002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/status_bubble_views.cc:431: const int scale_error_adjustment = scale - std::floor(scale) > 0 ? 3 : 0; Wat. 3 px seems entirely magic. I would expect that number to be governed by the scale factor you're at, not clamped to 3. And what is actually clipping you here? Why is this not a problem for the old code as well? https://codereview.chromium.org/2247563002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/status_bubble_views.cc:433: bubble_rect.Inset(0.5, 0.5); You're still overlapping by 1 DIP in Reposition() and then painting in the outermost px here. This is still going to result in incorrect positioning at DSF > 1. You need to point in the innermost px of that outer DIP, not the outermost. Something akin to: // The left and bottom edges are outdented by kShadowThickness DIPs in // Reposition(). We want to paint on the inner edge of that outdent. We floor // the scaled outdent to snap the border coordinates to the pixel grid. float aligned_scale = std::floor(kShadowThickness * scale); bubble_rect.Inset(aligned_scale - 0.5, 0.5, 0.5, aligned_scale - 0.5);
The CQ bit was checked by bsep@chromium.org to run a CQ dry run
https://codereview.chromium.org/2247563002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/status_bubble_views.cc (right): https://codereview.chromium.org/2247563002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/status_bubble_views.cc:431: const int scale_error_adjustment = scale - std::floor(scale) > 0 ? 3 : 0; On 2016/08/20 00:42:42, Peter Kasting wrote: > Wat. > > 3 px seems entirely magic. I would expect that number to be governed by the > scale factor you're at, not clamped to 3. > > And what is actually clipping you here? Why is this not a problem for the old > code as well? It is a problem for the old code, I just got a bit frustrated with it before. I investigated this more thoroughly and part of the problem was that we weren't snapping the width and height to pixels, so sometimes the border line would be aliased and it looks similar to clipping (I fixed that). But the real problem was that GetWindowBoundsInScreen is using ScreenWin::DIPToScreenRect which uses ScaleToEnclosingRect to coerce to integers, so it's going to be an overestimate on the width most of the time. That means if we draw to that width we'll go outside the layer bounds and get clipped. So I changed the plumbing to just use the original size_ in this file (it wasn't as problematic as I thought earlier). https://codereview.chromium.org/2247563002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/status_bubble_views.cc:433: bubble_rect.Inset(0.5, 0.5); On 2016/08/20 00:42:42, Peter Kasting wrote: > You're still overlapping by 1 DIP in Reposition() and then painting in the > outermost px here. This is still going to result in incorrect positioning at > DSF > 1. You need to point in the innermost px of that outer DIP, not the > outermost. > > Something akin to: > > // The left and bottom edges are outdented by kShadowThickness DIPs in > // Reposition(). We want to paint on the inner edge of that outdent. We > floor > // the scaled outdent to snap the border coordinates to the pixel grid. > float aligned_scale = std::floor(kShadowThickness * scale); > bubble_rect.Inset(aligned_scale - 0.5, 0.5, 0.5, aligned_scale - 0.5); Ok I misunderstood what you meant before. The relative position is annoyingly dependent on the absolute position so it's not really possible to be pixel perfect without fixing that. I added a big comment, hopefully it's clear enough.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
My feeling reading this CL is it still seems wildly complicated and I don't understand the complications or see how they address the potential positioning errors here. I also don't really understand why we're having positioning errors at all when the position of the status bubble is based on the position of the outer window +/- some integral number of DIPs. https://codereview.chromium.org/2247563002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/status_bubble_views.cc (right): https://codereview.chromium.org/2247563002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/status_bubble_views.cc:425: const float height = std::round(popup_size_.height() * scale); Why are we rounding if we're storing in floats? If rounding is correct, then don't we want ints? And is rounding really what we want? I'd think we'd want to floor these, if leaving them as the original values is wrong. https://codereview.chromium.org/2247563002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/status_bubble_views.cc:435: // precisely enough to guarantee exactly what we want. Having the bubble "pull I think you meant to divide this first sentence into two, or add conjunctions or something. https://codereview.chromium.org/2247563002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/status_bubble_views.cc:438: // window. Plus the possible positioning imprecision increases at higher dsfs. Are you saying that sometimes (for example) the bubble is offset to the right, relative to where we think it should be, by more than 1 px, so if we draw on what seems like the inner pixel of the client edge, that will be at least 2 px inside that edge, and there will be at least a 1 px gap? And that's an argument for insetting less than we otherwise would? How much less? What would you be doing if this wasn't an issue? Because it seems like the calculation I think would be correct is _less_ than the values you're computing below. I really want to see screenshots of the different positioning errors and how they look with the code you have here as well as the code I proposed. https://codereview.chromium.org/2247563002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/status_bubble_views.cc:441: const int subdip_layout_inset = scale > 1.25 ? (scale > 2 ? 2 : 1) : 0; It clearly isn't right to just use "2" for all scale factors larger than 2. Scale factor 10, for example, is not going to want "2". I realize that we don't _have_ many larger scale factors, but the point is that the code is less comprehensible this way than if you use a formula that computes the right offset based directly on the scale, instead of using a bunch of conditionals. https://codereview.chromium.org/2247563002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/status_bubble_views.cc:443: // FIXME: Since the dip error is dependent on platform-specific code we need Which platform-specific code? The DIP-vs.px positioning problems are common to all views targets. It doesn't seem like an ifdef here is right. https://codereview.chromium.org/2247563002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/status_bubble_views.cc:466: // inset it at all causes it to pull away from the frame. I don't understand why this is different than the left edge. Both were overlapped by the same value in Reposition(), so it seems like both should be handled the same way. https://codereview.chromium.org/2247563002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/status_bubble_views.cc:482: // Get the fill path by subtracting the shadow so they algin neatly. Nit: align
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Yes, this patch got pretty complicated, sorry! I ended up basically rewriting the entire way its drawn. The thing you might be missing about the positioning error is that the status bubble is actually its own hwnd, so it's not explicitly positioned relative to the window. This is the same problem we have with the omnibox popup by the way. If you open it up at 3x dsf you'll notice it's WAY off of where it should be. https://codereview.chromium.org/2247563002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/status_bubble_views.cc (right): https://codereview.chromium.org/2247563002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/status_bubble_views.cc:425: const float height = std::round(popup_size_.height() * scale); On 2016/08/24 22:53:24, Peter Kasting wrote: > Why are we rounding if we're storing in floats? If rounding is correct, then > don't we want ints? And is rounding really what we want? I'd think we'd want > to floor these, if leaving them as the original values is wrong. The types are just an oversight. I didn't think there was a reason to floor so I just rounded by default. It doesn't seem to cause any problems. I can floor instead if you think it's important. https://codereview.chromium.org/2247563002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/status_bubble_views.cc:435: // precisely enough to guarantee exactly what we want. Having the bubble "pull On 2016/08/24 22:53:24, Peter Kasting wrote: > I think you meant to divide this first sentence into two, or add conjunctions or > something. Done. https://codereview.chromium.org/2247563002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/status_bubble_views.cc:438: // window. Plus the possible positioning imprecision increases at higher dsfs. On 2016/08/24 22:53:24, Peter Kasting wrote: > Are you saying that sometimes (for example) the bubble is offset to the right, > relative to where we think it should be, by more than 1 px, so if we draw on > what seems like the inner pixel of the client edge, that will be at least 2 px > inside that edge, and there will be at least a 1 px gap? And that's an argument > for insetting less than we otherwise would? How much less? What would you be > doing if this wasn't an issue? > > Because it seems like the calculation I think would be correct is _less_ than > the values you're computing below. > > I really want to see screenshots of the different positioning errors and how > they look with the code you have here as well as the code I proposed. To your first question: you have the basic idea right. It depends on the absolute screen coordinates. It's hard for me to tell exactly what the possible error is, at 1.5x it can be 1-2 pixels, for example. But at 3x it's in the range of 2-4 pixels. It seems to always error to the left, so that's why I can add a couple pixels and not have it "pull away" from the frame. I'm guessing it's converting back and forth from dips somewhere. For everything <3x this at least keeps it inside the visible bounds of the window. I don't know what I'd be doing if this wasn't an issue because I can't play with it. Probably something like you suggested. I can send you some screenshots. https://codereview.chromium.org/2247563002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/status_bubble_views.cc:441: const int subdip_layout_inset = scale > 1.25 ? (scale > 2 ? 2 : 1) : 0; On 2016/08/24 22:53:24, Peter Kasting wrote: > It clearly isn't right to just use "2" for all scale factors larger than 2. > Scale factor 10, for example, is not going to want "2". > > I realize that we don't _have_ many larger scale factors, but the point is that > the code is less comprehensible this way than if you use a formula that computes > the right offset based directly on the scale, instead of using a bunch of > conditionals. I'm not claiming this isn't a hack, basically, but I'm not sure what else to do. You're right that it's probably not correct for very large dsfs but I'm not going to test everything to see what looks good. This is ok for 1, 1.25, 1.5, 2, 2.25, and 2.5, which are our most common scale factors. And like I said above it always errors to the left, so if this number is too low then it at least doesn't look worse than doing nothing. I could also remove this and come back to fixing the root positioning problem later. We need to fix this and the omnibox popup anyway. https://codereview.chromium.org/2247563002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/status_bubble_views.cc:443: // FIXME: Since the dip error is dependent on platform-specific code we need On 2016/08/24 22:53:24, Peter Kasting wrote: > Which platform-specific code? The DIP-vs.px positioning problems are common to > all views targets. It doesn't seem like an ifdef here is right. ScreenWin::DIPToScreenRect is what I was thinking of. This function was calling GetWindowRect before I refactored it too. So I don't trust that the positioning error is going to work the same way (or even exist) on other platforms. https://codereview.chromium.org/2247563002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/status_bubble_views.cc:466: // inset it at all causes it to pull away from the frame. On 2016/08/24 22:53:24, Peter Kasting wrote: > I don't understand why this is different than the left edge. Both were > overlapped by the same value in Reposition(), so it seems like both should be > handled the same way. I'm not sure either, tbh. It might have something to do with the shenanigans we do with the non-client area in the glass frame. But applying the correction to the bottom edge looks really bad in opaque, so I turned it off. https://codereview.chromium.org/2247563002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/status_bubble_views.cc:482: // Get the fill path by subtracting the shadow so they algin neatly. On 2016/08/24 22:53:24, Peter Kasting wrote: > Nit: align Done.
Updated with the code we talked about over email. PTAL
LGTM https://codereview.chromium.org/2247563002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/status_bubble_views.cc (right): https://codereview.chromium.org/2247563002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/status_bubble_views.cc:430: // the window as the scale increases. Nit: I think if you move this down lower it can be put right on the block it actually describes. See lower comment for suggestion. https://codereview.chromium.org/2247563002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/status_bubble_views.cc:431: const int subdip_layout_inset = std::floor(kShadowThickness * scale); Nit: I would call this |shadow_thickness_px| at this point, since that's basically how it's computed, and I think that reads more clearly below as well. I assume in testing 1.5x you found that the floor() call was indeed necessary to snap our drawing to the pixel grid. If so I'd leave a comment like "The floor() call here ensures we stay aligned to the pixel grid at non-integral scale factors." You might also want to explain why floor(), and not round() or ceil(), is correct. https://codereview.chromium.org/2247563002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/status_bubble_views.cc:436: // border when the bubble is floating so it looks complete. Nit: Maybe this would be slightly less awkward? // When there's no client edge, parts of the shadow can stick outside the // window. Clip these out when the bubble is docked. When the bubble is // floating, we preserve the full shadow so the bubble looks complete. https://codereview.chromium.org/2247563002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/status_bubble_views.cc:447: style_ != STYLE_STANDARD_RIGHT ? subdip_layout_inset : 1; Nit: Prefer "style == STANDARD_RIGHT ? 1 : var" so the "else" portion of the expression doesn't mentally read like a double-negative. https://codereview.chromium.org/2247563002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/status_bubble_views.cc:449: style_ == STYLE_STANDARD_RIGHT ? subdip_layout_inset : 1; Nit: I was confused for a bit why we were insetting by 1 in these expressions, then I realized it was to counteract the "- 0.5" below. Might be clearer to do something like this. This includes a possible rephrase of the comment moved down from above. 12345678901234567890123456789012345678901234567890123456789012345678901234567890 // Reposition() extends the bounds so they overlap the client edge (or window // exterior, when there is no client edge -- though this may have been clipped // out above) by 1 DIP. We want to draw a 1 px shadow at all scale factors, // on the innermost pixel of that overlap. So we need to inset the extended // edges by one DIP, less 1 px. const int inset = shadow_thickness_px - 1; bubble_rect.Inset((style == STANDARD_RIGHT) ? 0 : inset, 0, (style == STANDARD_RIGHT) ? inset : 0, inset); // Now |bubble_rect| encloses the desired path and is aligned to the pixel // edges. Place the path on pixel centers. bubble_rect.Inset(0.5, 0.5); SkPath path; ...
https://codereview.chromium.org/2247563002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/status_bubble_views.cc (right): https://codereview.chromium.org/2247563002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/status_bubble_views.cc:430: // the window as the scale increases. On 2016/08/27 04:07:31, Peter Kasting wrote: > Nit: I think if you move this down lower it can be put right on the block it > actually describes. See lower comment for suggestion. Done. https://codereview.chromium.org/2247563002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/status_bubble_views.cc:431: const int subdip_layout_inset = std::floor(kShadowThickness * scale); On 2016/08/27 04:07:31, Peter Kasting wrote: > Nit: I would call this |shadow_thickness_px| at this point, since that's > basically how it's computed, and I think that reads more clearly below as well. > > I assume in testing 1.5x you found that the floor() call was indeed necessary to > snap our drawing to the pixel grid. If so I'd leave a comment like "The floor() > call here ensures we stay aligned to the pixel grid at non-integral scale > factors." You might also want to explain why floor(), and not round() or > ceil(), is correct. Done. https://codereview.chromium.org/2247563002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/status_bubble_views.cc:436: // border when the bubble is floating so it looks complete. On 2016/08/27 04:07:31, Peter Kasting wrote: > Nit: Maybe this would be slightly less awkward? > > // When there's no client edge, parts of the shadow can stick outside the > // window. Clip these out when the bubble is docked. When the bubble is > // floating, we preserve the full shadow so the bubble looks complete. Done; edited the suggestion. https://codereview.chromium.org/2247563002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/status_bubble_views.cc:447: style_ != STYLE_STANDARD_RIGHT ? subdip_layout_inset : 1; On 2016/08/27 04:07:31, Peter Kasting wrote: > Nit: Prefer "style == STANDARD_RIGHT ? 1 : var" so the "else" portion of the > expression doesn't mentally read like a double-negative. Done. https://codereview.chromium.org/2247563002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/status_bubble_views.cc:449: style_ == STYLE_STANDARD_RIGHT ? subdip_layout_inset : 1; On 2016/08/27 04:07:31, Peter Kasting wrote: > Nit: I was confused for a bit why we were insetting by 1 in these expressions, > then I realized it was to counteract the "- 0.5" below. > > Might be clearer to do something like this. This includes a possible rephrase > of the comment moved down from above. > > 12345678901234567890123456789012345678901234567890123456789012345678901234567890 > // Reposition() extends the bounds so they overlap the client edge (or window > // exterior, when there is no client edge -- though this may have been clipped > // out above) by 1 DIP. We want to draw a 1 px shadow at all scale factors, > // on the innermost pixel of that overlap. So we need to inset the extended > // edges by one DIP, less 1 px. > const int inset = shadow_thickness_px - 1; > bubble_rect.Inset((style == STANDARD_RIGHT) ? 0 : inset, 0, > (style == STANDARD_RIGHT) ? inset : 0, inset); > > // Now |bubble_rect| encloses the desired path and is aligned to the pixel > // edges. Place the path on pixel centers. > bubble_rect.Inset(0.5, 0.5); > SkPath path; > ... Done; edited the suggested comment.
The CQ bit was checked by bsep@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/2247563002/#ps140001 (title: "comments and nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2247563002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/status_bubble_views.cc (right): https://codereview.chromium.org/2247563002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/status_bubble_views.cc:425: const int height = std::round(popup_size_.height() * scale); Oh, I missed that we were still rounding here. This doesn't cause problems with clipping off a pixel if we round up, does it?
https://codereview.chromium.org/2247563002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/status_bubble_views.cc (right): https://codereview.chromium.org/2247563002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/status_bubble_views.cc:425: const int height = std::round(popup_size_.height() * scale); On 2016/08/30 00:41:58, Peter Kasting wrote: > Oh, I missed that we were still rounding here. > > This doesn't cause problems with clipping off a pixel if we round up, does it? We inset to the inner pixel when aligning to pixel centers so it shouldn't cause any clipping. I haven't seen any issues with it the whole time I've been testing.
Message was sent while issue was closed.
Description was changed from ========== Change status bubble rendering at hidpi and when there is no client edge The recently removed client edge on Windows 10 made the status bubble overlap the window edge. This patch clips out the bubble border when the bubble is docked, and leaves it unchanged when it's floating. Also made the border always 1 pixel even at hidpi to make it look more consistent with the material UI. BUG=636479 ========== to ========== Change status bubble rendering at hidpi and when there is no client edge The recently removed client edge on Windows 10 made the status bubble overlap the window edge. This patch clips out the bubble border when the bubble is docked, and leaves it unchanged when it's floating. Also made the border always 1 pixel even at hidpi to make it look more consistent with the material UI. BUG=636479 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Change status bubble rendering at hidpi and when there is no client edge The recently removed client edge on Windows 10 made the status bubble overlap the window edge. This patch clips out the bubble border when the bubble is docked, and leaves it unchanged when it's floating. Also made the border always 1 pixel even at hidpi to make it look more consistent with the material UI. BUG=636479 ========== to ========== Change status bubble rendering at hidpi and when there is no client edge The recently removed client edge on Windows 10 made the status bubble overlap the window edge. This patch clips out the bubble border when the bubble is docked, and leaves it unchanged when it's floating. Also made the border always 1 pixel even at hidpi to make it look more consistent with the material UI. BUG=636479 Committed: https://crrev.com/e5a8ba6722632879cfa09ef0e7f8d7f4a99c5d4e Cr-Commit-Position: refs/heads/master@{#415216} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/e5a8ba6722632879cfa09ef0e7f8d7f4a99c5d4e Cr-Commit-Position: refs/heads/master@{#415216} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
