Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1368363003:
Programmatically render the omnibox border (Closed)
DescriptionProgrammatically render the omnibox border
For Material Design switch from using an asset for rendering the omnibox border to programmatically rendering it.
It is now a one pixel line regardless of screen scaling.
TEST=manual testing on 100% & 200% devices. Confirmed the rendering matched the designed asset.
BUG=527863
Committed: https://crrev.com/701b557ba7141b55636ae7572160cb8fa7581267
Cr-Commit-Position: refs/heads/master@{#351370}
Patch Set 1 : #
Total comments: 9
Patch Set 2 : #
Total comments: 2
Patch Set 3 : #
Total comments: 6
Patch Set 4 : Scale Fix #Patch Set 5 : Corner Rounding #
Total comments: 4
Patch Set 6 : #
Depends on Patchset: Messages
Total messages: 28 (9 generated)
Patchset #1 (id:1) has been deleted
jonross@chromium.org changed reviewers: + tdanderson@chromium.org
Hi Terry, Mind taking a first look?
https://codereview.chromium.org/1368363003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/1368363003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:1279: if (ui::MaterialDesignController::IsModeMaterial()) { If this pattern begins to be used a lot we may want to consider creating a border/painter to encompass it. For example the Find popup uses assets for its bubble border still.
Please see some comments below. It would also be a good idea to attach screenshots (for both 1x and 2x) on the bug. https://codereview.chromium.org/1368363003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/1368363003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:540: return min_size; With your changes it looks like we can hit this and be returning a size with a width of 0. Is that going to be problematic? https://codereview.chromium.org/1368363003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:1286: paint.setColor(SkColorSetARGBInline(0x40, 0x0, 0x0, 0x0)); Is this color, the corner radius value, etc, likely to be referenced anywhere else? If so, consider pulling the constants out into a common location. https://codereview.chromium.org/1368363003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:1290: const float kOffset = .5f; nit: 0.5f https://codereview.chromium.org/1368363003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:1300: views::Painter::PaintPainterAt(recorder.canvas(), border_painter_.get(), perhaps include a comment above |border_painter_| in the .h to state that it is not used in MD mode.
https://codereview.chromium.org/1368363003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/1368363003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:540: return min_size; On 2015/09/28 15:12:14, tdanderson wrote: > With your changes it looks like we can hit this and be returning a size with a > width of 0. Is that going to be problematic? Nope. We actually refuse to layout when not initialized. That is there so we provide a Size to calls done before initialization. https://codereview.chromium.org/1368363003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:1286: paint.setColor(SkColorSetARGBInline(0x40, 0x0, 0x0, 0x0)); On 2015/09/28 15:12:14, tdanderson wrote: > Is this color, the corner radius value, etc, likely to be referenced anywhere > else? If so, consider pulling the constants out into a common location. Currently they are not. However if this becomes common I would want this rendering pulled out into a common border/painter class. https://codereview.chromium.org/1368363003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:1290: const float kOffset = .5f; On 2015/09/28 15:12:14, tdanderson wrote: > nit: 0.5f Done. https://codereview.chromium.org/1368363003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:1300: views::Painter::PaintPainterAt(recorder.canvas(), border_painter_.get(), On 2015/09/28 15:12:14, tdanderson wrote: > perhaps include a comment above |border_painter_| in the .h to state that it is > not used in MD mode. Done.
lgtm https://codereview.chromium.org/1368363003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_view.h (right): https://codereview.chromium.org/1368363003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.h:405: // Object used to paint the border when not being used in Material Design. nit: how about something like "Object used to paint the border. Not used for material design." ?
https://codereview.chromium.org/1368363003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_view.h (right): https://codereview.chromium.org/1368363003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.h:405: // Object used to paint the border when not being used in Material Design. On 2015/09/28 15:52:12, tdanderson wrote: > nit: how about something like "Object used to paint the border. Not used for > material design." ? Done.
Hey Peter, Could you review this change? Thanks, Jon
jonross@chromium.org changed reviewers: + pkasting@chromium.org
Hey Peter, Could you review this change? Thanks, Jon
LGTM https://codereview.chromium.org/1368363003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/1368363003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:1286: paint.setColor(SkColorSetARGBInline(0x40, 0x0, 0x0, 0x0)); Don't explicitly call the inline version, just use SkColorSetARGB(). Nit: Use 0x00 for these color values. https://codereview.chromium.org/1368363003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:1287: paint.setStrokeWidth(0); Use stroke width 1 rather than 0. https://codereview.chromium.org/1368363003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:1295: const SkScalar kCornerRadius = SkDoubleToScalar(2.5f); Not sure this is right. This will give the corners a 2.5 px radius in every zoom level, meaning they'll look proportionately twice as big in 1x as 2x. Is that really what you want, or should you be computing this value here as n * scale for some n?
Patchset #4 (id:80001) has been deleted
https://codereview.chromium.org/1368363003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/1368363003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:1286: paint.setColor(SkColorSetARGBInline(0x40, 0x0, 0x0, 0x0)); On 2015/09/28 23:01:10, Peter Kasting wrote: > Don't explicitly call the inline version, just use SkColorSetARGB(). > > Nit: Use 0x00 for these color values. Done. https://codereview.chromium.org/1368363003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:1287: paint.setStrokeWidth(0); On 2015/09/28 23:01:10, Peter Kasting wrote: > Use stroke width 1 rather than 0. Done. https://codereview.chromium.org/1368363003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:1295: const SkScalar kCornerRadius = SkDoubleToScalar(2.5f); On 2015/09/28 23:01:10, Peter Kasting wrote: > Not sure this is right. This will give the corners a 2.5 px radius in every > zoom level, meaning they'll look proportionately twice as big in 1x as 2x. Is > that really what you want, or should you be computing this value here as n * > scale for some n? Changing the scale of the sk_canvas keeps these corners at the desired radius. I'll attach screenshots to the issue. But I compared 100% to 200% and they corners are the same.
The CQ bit was checked by jonross@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdanderson@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/1368363003/#ps100001 (title: "Scale Fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1368363003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1368363003/100001
I just took a look at your screenshots and they exhibit the problem I described: the corners are more rounded in 1x than 2x. Looking at Sebastien's mocks, this looks wrong in 2x. I think you need to multiply the corner radius by the scale factor.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
On 2015/09/29 17:33:09, Peter Kasting wrote: > I just took a look at your screenshots and they exhibit the problem I described: > the corners are more rounded in 1x than 2x. Looking at Sebastien's mocks, this > looks wrong in 2x. > > I think you need to multiply the corner radius by the scale factor. Sorry I misinterpreted your concern. I thought you were concerned that the corners would be different between 100% vs 200% at the pixel level. Not that they should be to account for how rounded they appear. Updated issue with new 200% screenshot, also double checked the 200% omnibox_border asset.
Cool, looks better. LGTM https://codereview.chromium.org/1368363003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/1368363003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_view.cc:1271: recorder.canvas()->DrawFocusRect(omnibox_view_->bounds()); Nit: recorder.canvas() is used a lot in this function, may want a local gfx::Canvas* to refer to it https://codereview.chromium.org/1368363003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_view.cc:1293: border_rect_f.Scale(display_scale); Wow, how did it work at all in 2x without this?
https://codereview.chromium.org/1368363003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/1368363003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_view.cc:1271: recorder.canvas()->DrawFocusRect(omnibox_view_->bounds()); On 2015/09/29 18:12:37, Peter Kasting wrote: > Nit: recorder.canvas() is used a lot in this function, may want a local > gfx::Canvas* to refer to it Done. https://codereview.chromium.org/1368363003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_view.cc:1293: border_rect_f.Scale(display_scale); On 2015/09/29 18:12:37, Peter Kasting wrote: > Wow, how did it work at all in 2x without this? I goofed. I had it at one point. Accidentally removed it in cleanup. But had task switched and forgot that the md-flag was off on samus when i came back. :(
The CQ bit was checked by jonross@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdanderson@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/1368363003/#ps140001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1368363003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1368363003/140001
Message was sent while issue was closed.
Committed patchset #6 (id:140001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/701b557ba7141b55636ae7572160cb8fa7581267 Cr-Commit-Position: refs/heads/master@{#351370} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
