Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1394763003:
Update LocationBarView Background (Closed)
DescriptionUpdate LocationBarView Background
Update the rendering of the background of LocationBarView in order to match the programmatically rendering border. This fixes the 1 pixel gap between the border and the background. It also gets the corners to match up.
TEST=manual testing on 100 and 200% devices
BUG=527863
Committed: https://crrev.com/e026ad2bbf8946970784b810a771a5d31a6903e3
Cr-Commit-Position: refs/heads/master@{#357571}
Patch Set 1 : #
Total comments: 5
Patch Set 2 : Don't round popup #
Total comments: 1
Patch Set 3 : Subtractive Path Painting #
Total comments: 6
Patch Set 4 : Refactor Painting into a Background class #
Total comments: 27
Patch Set 5 : Move new background class into cbuvl #Patch Set 6 : #
Total comments: 5
Patch Set 7 : #
Total comments: 10
Patch Set 8 : Update nomenclature #
Depends on Patchset: Messages
Total messages: 40 (8 generated)
Patchset #1 (id:1) has been deleted
jonross@chromium.org changed reviewers: + pkasting@chromium.org, tdanderson@chromium.org
PTAL
https://codereview.chromium.org/1394763003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/1394763003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:106: void DrawScaledRoundRect(gfx::Canvas* canvas, Nit: I'd add a comment about what this does, in particular how the round rect relates to |bounds|. https://codereview.chromium.org/1394763003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:1322: SkColorSetARGB(0x40, 0x00, 0x00, 0x00), border_rect); I don't think this is right for popup windows, where we want a rectangular border rather than a round one. Also, won't this draw the stroke atop the fill? I'm not sure that's what we want; do we want the fill inside the stroke instead? If so that's doable, I can show you the technique to do it.
https://codereview.chromium.org/1394763003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/1394763003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:1322: SkColorSetARGB(0x40, 0x00, 0x00, 0x00), border_rect); On 2015/10/08 21:33:06, Peter Kasting wrote: > I don't think this is right for popup windows, where we want a rectangular > border rather than a round one. > > Also, won't this draw the stroke atop the fill? I'm not sure that's what we > want; do we want the fill inside the stroke instead? If so that's doable, I can > show you the technique to do it. I looked at the spec, to get the desired alpha blend we will not want to draw the stroke atop the fill, but will want the fill inset.
https://codereview.chromium.org/1394763003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/1394763003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:106: void DrawScaledRoundRect(gfx::Canvas* canvas, On 2015/10/08 21:33:06, Peter Kasting wrote: > Nit: I'd add a comment about what this does, in particular how the round rect > relates to |bounds|. Done. https://codereview.chromium.org/1394763003/diff/2/chrome/browser/ui/views/loc... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/1394763003/diff/2/chrome/browser/ui/views/loc... chrome/browser/ui/views/location_bar/location_bar_view.cc:1330: SkColorSetARGB(0x4D, 0x00, 0x00, 0x00), border_rect, This fixes an incorrect color sampling of the original border asset.
https://codereview.chromium.org/1394763003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/1394763003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:1322: SkColorSetARGB(0x40, 0x00, 0x00, 0x00), border_rect); On 2015/10/09 15:14:17, jonross wrote: > On 2015/10/08 21:33:06, Peter Kasting wrote: > > I don't think this is right for popup windows, where we want a rectangular > > border rather than a round one. > > > > Also, won't this draw the stroke atop the fill? I'm not sure that's what we > > want; do we want the fill inside the stroke instead? If so that's doable, I > can > > show you the technique to do it. > > I looked at the spec, to get the desired alpha blend we will not want to draw > the stroke atop the fill, but will want the fill inset. That's what I thought. There are several ways to do this, but the easiest is probably something like this: (1) Get the path for the round rect stroke. (2) Create an SkPaint for stroking. (3) Use the stroke paint to convert the path to a corresponding fill path for the stroke, i.e. a fill that, if drawn, would look exactly like a stroke. (4) Subtract this fill from your main path. Now the main path goes up to, but not over, the stroke. (5) Draw both fill paths with a fill-style SkPaint. Remember to turn AA on and set the right colors for each. For (1), use SkPath::addRoundRect(). For (2), use SkPaint::setStyle(kStroke_Style); and setStrokeWidth();. For (3), use SkPaint::getFillPath(); you want paint.getFillPath(path, &stroke); where |stroke| is a new SkPath. For (4), use the Op() function in SkPathOps.h; you want Op(path, stroke, kDifference_SkPathOp, &path);.
Patchset #3 (id:50001) has been deleted
On 2015/10/09 16:03:32, Peter Kasting wrote: > https://codereview.chromium.org/1394763003/diff/20001/chrome/browser/ui/views... > File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): > > https://codereview.chromium.org/1394763003/diff/20001/chrome/browser/ui/views... > chrome/browser/ui/views/location_bar/location_bar_view.cc:1322: > SkColorSetARGB(0x40, 0x00, 0x00, 0x00), border_rect); > On 2015/10/09 15:14:17, jonross wrote: > > On 2015/10/08 21:33:06, Peter Kasting wrote: > > > I don't think this is right for popup windows, where we want a rectangular > > > border rather than a round one. > > > > > > Also, won't this draw the stroke atop the fill? I'm not sure that's what we > > > want; do we want the fill inside the stroke instead? If so that's doable, I > > can > > > show you the technique to do it. > > > > I looked at the spec, to get the desired alpha blend we will not want to draw > > the stroke atop the fill, but will want the fill inset. > > That's what I thought. > > There are several ways to do this, but the easiest is probably something like > this: > > (1) Get the path for the round rect stroke. > (2) Create an SkPaint for stroking. > (3) Use the stroke paint to convert the path to a corresponding fill path for > the stroke, i.e. a fill that, if drawn, would look exactly like a stroke. > (4) Subtract this fill from your main path. Now the main path goes up to, but > not over, the stroke. > (5) Draw both fill paths with a fill-style SkPaint. Remember to turn AA on and > set the right colors for each. > > For (1), use SkPath::addRoundRect(). > For (2), use SkPaint::setStyle(kStroke_Style); and setStrokeWidth();. > For (3), use SkPaint::getFillPath(); you want paint.getFillPath(path, &stroke); > where |stroke| is a new SkPath. > For (4), use the Op() function in SkPathOps.h; you want Op(path, stroke, > kDifference_SkPathOp, &path);. Yup that works. Blended colors match the specs.
estade@chromium.org changed reviewers: + estade@chromium.org
https://codereview.chromium.org/1394763003/diff/70001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/1394763003/diff/70001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:120: rect->Inset(insets); you can just do rect->Inset(kOffset, kOffset)
yea, this patch definitely seems to heavily overlap with programmatically rendering the chip, so it's probably worth refactoring into a shared file (or at least waiting for this one to land before working on the chip). https://codereview.chromium.org/1394763003/diff/70001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/1394763003/diff/70001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:1334: // the omnibox background, so we can't just blindly fill our entire bounds. I don't think this is still true for material mode. The edge thickness is 0. Hence you could just pass the entire bounds. This makes me think it would be fine to implement a round rect version of views::Background and not pass bounds at all (passing color in the constructor).
So, I'm a little unclear -- after Evan's comments, should I still look at this? Wait? Also see Sebastien's comments in the email thread that sound like the border isn't being calculated right for 2x mode, maybe address that first?
On 2015/10/10 00:20:43, Peter Kasting wrote: > So, I'm a little unclear -- after Evan's comments, should I still look at this? > Wait? Also see Sebastien's comments in the email thread that sound like the > border isn't being calculated right for 2x mode, maybe address that first? By this comment > (or at least waiting for this one to land before working on the chip) I meant that it would be reasonable to go ahead with this patch, then tackle the chip rendering later, at which point we'd figure out the most sensible way to share the code.
https://codereview.chromium.org/1394763003/diff/70001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/1394763003/diff/70001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:1375: !is_popup_mode_); In the MD world, I don't think we actually need to do this in PaintChildren(); we can do it in Paint(). This simplifies things because you can draw the background and border in the same function. This will also allow you to fix what looks like an inconsistency here where the popup mode border is drawn atop the background, but the normal border is drawn outside it.
On 2015/10/12 23:45:44, Peter Kasting wrote: > https://codereview.chromium.org/1394763003/diff/70001/chrome/browser/ui/views... > File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): > > https://codereview.chromium.org/1394763003/diff/70001/chrome/browser/ui/views... > chrome/browser/ui/views/location_bar/location_bar_view.cc:1375: > !is_popup_mode_); > In the MD world, I don't think we actually need to do this in PaintChildren(); > we can do it in Paint(). This simplifies things because you can draw the > background and border in the same function. > > This will also allow you to fix what looks like an inconsistency here where the > popup mode border is drawn atop the background, but the normal border is drawn > outside it. Since this heavily overlaps with Evan's upcoming work, I will pull this out into a new Background class. This will let it be reused, and also address the difference in Paint vs PaintChildren ordering. There I will address the 2x mode request for an extra 1 pixel inset.
Also take note of https://codereview.chromium.org/1393013007/ where I'm slightly simplifying the code to unscale the canvas.
Separated the painting into a new Background class. I've rebased to include the Canvas->SaveAndUnscale. The updated painting takes into account the comments from the previous revision. https://codereview.chromium.org/1394763003/diff/70001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/1394763003/diff/70001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:120: rect->Inset(insets); On 2015/10/09 20:00:24, Evan Stade wrote: > you can just do rect->Inset(kOffset, kOffset) Done. https://codereview.chromium.org/1394763003/diff/70001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:1334: // the omnibox background, so we can't just blindly fill our entire bounds. On 2015/10/09 20:34:38, Evan Stade wrote: > I don't think this is still true for material mode. The edge thickness is 0. > Hence you could just pass the entire bounds. This makes me think it would be > fine to implement a round rect version of views::Background and not pass bounds > at all (passing color in the constructor). Done. https://codereview.chromium.org/1394763003/diff/70001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:1375: !is_popup_mode_); On 2015/10/12 23:45:44, Peter Kasting wrote: > In the MD world, I don't think we actually need to do this in PaintChildren(); > we can do it in Paint(). This simplifies things because you can draw the > background and border in the same function. > > This will also allow you to fix what looks like an inconsistency here where the > popup mode border is drawn atop the background, but the normal border is drawn > outside it. Done.
https://codereview.chromium.org/1394763003/diff/90001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/1394763003/diff/90001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:164: if (ui::MaterialDesignController::IsModeMaterial()) { Nit: Blank line above. https://codereview.chromium.org/1394763003/diff/90001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:1258: // SolidScaledBackground is used in Material Design Nit: Comments should have periods. Maybe this: if (ui::MaterialDesignController::IsModeMaterial()) return; // The background and border are painted by our Background. https://codereview.chromium.org/1394763003/diff/90001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:1294: // SolidScaledBackground used for Material Design See above https://codereview.chromium.org/1394763003/diff/90001/ui/views/background.cc File ui/views/background.cc (right): https://codereview.chromium.org/1394763003/diff/90001/ui/views/background.cc#... ui/views/background.cc:43: explicit SolidScaledBackground(SkColor background, SkColor border, bool round) Nit: No explicit (only used for 1-arg constructors) I would name the third arg (and member) "round_corners" or "round_rect" rather than "round", as "round" sounds like a mathematical operation. https://codereview.chromium.org/1394763003/diff/90001/ui/views/background.cc#... ui/views/background.cc:49: gfx::RectF border_rect_f(view->GetContentsBounds()); Nit: Declare |border_rect_f| and |kOffset| right above the conditional that uses them. https://codereview.chromium.org/1394763003/diff/90001/ui/views/background.cc#... ui/views/background.cc:51: const float kOffset = scale > 1.0f ? 1.5f : 0.5f; I don't think this gets the right result for scales other than 1x and 2x, e.g. 1.5x. My understanding from email was that the problem is that Sebastien wants the omnibox sized based on scaling the interior up and outdenting the border, rather than based on scaling the outer size up and indenting. To implement that you'd need to do one of the following: (1) Inset the |border_rect_f| by 1 px on each side before scaling, then scale, then outset by 0.5 px. (2) Do something like what you're doing here, but compute |kOffset| as "scale - 0.5f" (and name it |inset| or something since it's not a pure constant). https://codereview.chromium.org/1394763003/diff/90001/ui/views/background.cc#... ui/views/background.cc:55: stroke_paint.setStyle(SkPaint::Style::kStroke_Style); Nit: No need for "Style::" https://codereview.chromium.org/1394763003/diff/90001/ui/views/background.cc#... ui/views/background.cc:72: border_rect_f.Inset(-kOffset, kOffset); I don't think this is right for non-1x. It seems like in this case you want the x inset to always be -0.5, I think. Probably the two will look equivalent assuming the rect goes to the window bounds, but that would be technically more correct. https://codereview.chromium.org/1394763003/diff/90001/ui/views/background.cc#... ui/views/background.cc:82: canvas->sk_canvas()->drawPath(path, stroke_paint); Nit: I think it might be slightly more performant to do fill_paint.setColor(border_color_); canvas->sk_canvas()->drawPath(stroke_path, fill_paint); This allows you to further simplify things to just use one paint: SkPaint paint; paint.setAntiAlias(true); paint.setStyle(SkPaint::kStroke_Style); paint.setStrokeWidth(1); SkPath stroke_path; paint.getFillPath(path, &stroke_path); SkPath fill_path; Op(path, stroke_path, kDifference_SkPathOp, &fill_path); paint.setStyle(SkPaint::kFill_style); paint.setColor(get_color()); canvas->sk_canvas()->drawPath(fill_path, paint); paint.setColor(border_color_); canvas->sk_canvas()->drawPath(stroke_path, paint); https://codereview.chromium.org/1394763003/diff/90001/ui/views/background.cc#... ui/views/background.cc:90: // If true the rect will be a rounded rect. Nit: Maybe "If true the border and interior corners will be rounded." https://codereview.chromium.org/1394763003/diff/90001/ui/views/background.h File ui/views/background.h (right): https://codereview.chromium.org/1394763003/diff/90001/ui/views/background.h#n... ui/views/background.h:61: static Background* CreateSolidScaledBackground(SkColor background, Why put this here? Are you going to be using it outside the location bar? If not, I would define your new background class in the .cc file that uses it.
cool, lgtm aside from resolving pkasting's concerns https://codereview.chromium.org/1394763003/diff/90001/ui/views/background.cc File ui/views/background.cc (right): https://codereview.chromium.org/1394763003/diff/90001/ui/views/background.cc#... ui/views/background.cc:90: // If true the rect will be a rounded rect. On 2015/10/19 21:09:44, Peter Kasting wrote: > Nit: Maybe "If true the border and interior corners will be rounded." I don't really care what this comment says because it's clear to me either way but I think it's usually "roundrect" not "rounded rect". https://codereview.chromium.org/1394763003/diff/90001/ui/views/background.h File ui/views/background.h (right): https://codereview.chromium.org/1394763003/diff/90001/ui/views/background.h#n... ui/views/background.h:61: static Background* CreateSolidScaledBackground(SkColor background, On 2015/10/19 21:09:44, Peter Kasting wrote: > Why put this here? Are you going to be using it outside the location bar? If > not, I would define your new background class in the .cc file that uses it. The plan is to use it for chips as well (e.g. ev_bubble_view), but I agree it could live in c/b/u/v/location_bar/ (probably its own file).
https://codereview.chromium.org/1394763003/diff/90001/ui/views/background.h File ui/views/background.h (right): https://codereview.chromium.org/1394763003/diff/90001/ui/views/background.h#n... ui/views/background.h:61: static Background* CreateSolidScaledBackground(SkColor background, On 2015/10/19 23:21:30, Evan Stade wrote: > On 2015/10/19 21:09:44, Peter Kasting wrote: > > Why put this here? Are you going to be using it outside the location bar? If > > not, I would define your new background class in the .cc file that uses it. > > The plan is to use it for chips as well (e.g. ev_bubble_view), but I agree it > could live in c/b/u/v/location_bar/ (probably its own file). Ah, helpful. Yeah, I would probably avoid touching ui/ unless you're adding stuff that will be used outside of chrome/. I would probably also give thought to a different name here, e.g. something around "BackgroundWith1PxBorder" or some other phrase that emphasizes the "background and border" nature of this.
https://codereview.chromium.org/1394763003/diff/90001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/1394763003/diff/90001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:164: if (ui::MaterialDesignController::IsModeMaterial()) { On 2015/10/19 21:09:44, Peter Kasting wrote: > Nit: Blank line above. Done. https://codereview.chromium.org/1394763003/diff/90001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:1258: // SolidScaledBackground is used in Material Design On 2015/10/19 21:09:44, Peter Kasting wrote: > Nit: Comments should have periods. > > Maybe this: > > if (ui::MaterialDesignController::IsModeMaterial()) > return; // The background and border are painted by our Background. Done. https://codereview.chromium.org/1394763003/diff/90001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:1294: // SolidScaledBackground used for Material Design On 2015/10/19 21:09:44, Peter Kasting wrote: > See above Done. https://codereview.chromium.org/1394763003/diff/90001/ui/views/background.cc File ui/views/background.cc (right): https://codereview.chromium.org/1394763003/diff/90001/ui/views/background.cc#... ui/views/background.cc:43: explicit SolidScaledBackground(SkColor background, SkColor border, bool round) On 2015/10/19 21:09:44, Peter Kasting wrote: > Nit: No explicit (only used for 1-arg constructors) > > I would name the third arg (and member) "round_corners" or "round_rect" rather > than "round", as "round" sounds like a mathematical operation. Done. https://codereview.chromium.org/1394763003/diff/90001/ui/views/background.cc#... ui/views/background.cc:49: gfx::RectF border_rect_f(view->GetContentsBounds()); On 2015/10/19 21:09:44, Peter Kasting wrote: > Nit: Declare |border_rect_f| and |kOffset| right above the conditional that uses > them. Done. https://codereview.chromium.org/1394763003/diff/90001/ui/views/background.cc#... ui/views/background.cc:51: const float kOffset = scale > 1.0f ? 1.5f : 0.5f; On 2015/10/19 21:09:44, Peter Kasting wrote: > I don't think this gets the right result for scales other than 1x and 2x, e.g. > 1.5x. My understanding from email was that the problem is that Sebastien wants > the omnibox sized based on scaling the interior up and outdenting the border, > rather than based on scaling the outer size up and indenting. To implement that > you'd need to do one of the following: > > (1) Inset the |border_rect_f| by 1 px on each side before scaling, then scale, > then outset by 0.5 px. > (2) Do something like what you're doing here, but compute |kOffset| as "scale - > 0.5f" (and name it |inset| or something since it's not a pure constant). I spoke with him, he wants the 200% inset by one pixel. So that the outermost pixel of the view is transparent. https://codereview.chromium.org/1394763003/diff/90001/ui/views/background.cc#... ui/views/background.cc:55: stroke_paint.setStyle(SkPaint::Style::kStroke_Style); On 2015/10/19 21:09:44, Peter Kasting wrote: > Nit: No need for "Style::" Done. https://codereview.chromium.org/1394763003/diff/90001/ui/views/background.cc#... ui/views/background.cc:72: border_rect_f.Inset(-kOffset, kOffset); On 2015/10/19 21:09:44, Peter Kasting wrote: > I don't think this is right for non-1x. It seems like in this case you want the > x inset to always be -0.5, I think. Probably the two will look equivalent > assuming the rect goes to the window bounds, but that would be technically more > correct. Done. https://codereview.chromium.org/1394763003/diff/90001/ui/views/background.cc#... ui/views/background.cc:82: canvas->sk_canvas()->drawPath(path, stroke_paint); On 2015/10/19 21:09:44, Peter Kasting wrote: > Nit: I think it might be slightly more performant to do > > fill_paint.setColor(border_color_); > canvas->sk_canvas()->drawPath(stroke_path, fill_paint); > > This allows you to further simplify things to just use one paint: > > SkPaint paint; > paint.setAntiAlias(true); > paint.setStyle(SkPaint::kStroke_Style); > paint.setStrokeWidth(1); > SkPath stroke_path; > paint.getFillPath(path, &stroke_path); > > SkPath fill_path; > Op(path, stroke_path, kDifference_SkPathOp, &fill_path); > paint.setStyle(SkPaint::kFill_style); > paint.setColor(get_color()); > canvas->sk_canvas()->drawPath(fill_path, paint); > > paint.setColor(border_color_); > canvas->sk_canvas()->drawPath(stroke_path, paint); Done. https://codereview.chromium.org/1394763003/diff/90001/ui/views/background.cc#... ui/views/background.cc:90: // If true the rect will be a rounded rect. On 2015/10/19 21:09:44, Peter Kasting wrote: > Nit: Maybe "If true the border and interior corners will be rounded." Done. https://codereview.chromium.org/1394763003/diff/90001/ui/views/background.h File ui/views/background.h (right): https://codereview.chromium.org/1394763003/diff/90001/ui/views/background.h#n... ui/views/background.h:61: static Background* CreateSolidScaledBackground(SkColor background, On 2015/10/19 21:09:44, Peter Kasting wrote: > Why put this here? Are you going to be using it outside the location bar? If > not, I would define your new background class in the .cc file that uses it. Evan was going to use this for the omnibox chips
https://codereview.chromium.org/1394763003/diff/90001/ui/views/background.cc File ui/views/background.cc (right): https://codereview.chromium.org/1394763003/diff/90001/ui/views/background.cc#... ui/views/background.cc:51: const float kOffset = scale > 1.0f ? 1.5f : 0.5f; On 2015/10/20 18:54:54, jonross wrote: > On 2015/10/19 21:09:44, Peter Kasting wrote: > > I don't think this gets the right result for scales other than 1x and 2x, e.g. > > 1.5x. My understanding from email was that the problem is that Sebastien > wants > > the omnibox sized based on scaling the interior up and outdenting the border, > > rather than based on scaling the outer size up and indenting. To implement > that > > you'd need to do one of the following: > > > > (1) Inset the |border_rect_f| by 1 px on each side before scaling, then scale, > > then outset by 0.5 px. > > (2) Do something like what you're doing here, but compute |kOffset| as "scale > - > > 0.5f" (and name it |inset| or something since it's not a pure constant). > > I spoke with him, he wants the 200% inset by one pixel. So that the outermost > pixel of the view is transparent. Yes, and my suggestions should accomplish that, but they will also work in modes like 140% and 150%, whereas this code will break badly outside 1x and 2x.
https://codereview.chromium.org/1394763003/diff/90001/ui/views/background.cc File ui/views/background.cc (right): https://codereview.chromium.org/1394763003/diff/90001/ui/views/background.cc#... ui/views/background.cc:51: const float kOffset = scale > 1.0f ? 1.5f : 0.5f; On 2015/10/20 23:04:05, Peter Kasting wrote: > On 2015/10/20 18:54:54, jonross wrote: > > On 2015/10/19 21:09:44, Peter Kasting wrote: > > > I don't think this gets the right result for scales other than 1x and 2x, > e.g. > > > 1.5x. My understanding from email was that the problem is that Sebastien > > wants > > > the omnibox sized based on scaling the interior up and outdenting the > border, > > > rather than based on scaling the outer size up and indenting. To implement > > that > > > you'd need to do one of the following: > > > > > > (1) Inset the |border_rect_f| by 1 px on each side before scaling, then > scale, > > > then outset by 0.5 px. > > > (2) Do something like what you're doing here, but compute |kOffset| as > "scale > > - > > > 0.5f" (and name it |inset| or something since it's not a pure constant). > > > > I spoke with him, he wants the 200% inset by one pixel. So that the outermost > > pixel of the view is transparent. > > Yes, and my suggestions should accomplish that, but they will also work in modes > like 140% and 150%, whereas this code will break badly outside 1x and 2x. Done.
https://codereview.chromium.org/1394763003/diff/130001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/background_with_1_px_border.cc (right): https://codereview.chromium.org/1394763003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/background_with_1_px_border.cc:30: border_rect_f.Inset(kPostScaleOffset, kPostScaleOffset); If evan is going to use this for the omnibox chips, is this the math that they want to use? It's not clear to me whether this is specifically applicable to anything other than the omnibox itself. https://codereview.chromium.org/1394763003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/background_with_1_px_border.cc:35: border_rect_f.Inset(0.0f, kPreScaleOffset); It's non-obvious to a reader why you're not indenting the sides here. And to someone who knows, it's a bit odd that we tied this to a "round corners" bit, as this doesn't really have to do with rounding the corners per se, but more with whether we're in popup mode. Furthermore, in popup mode, we really only want to do the no-inset thing in the case where the location bar horizontal edge thickness is zero, we don't want to do it all the time. So as-is, this code doesn't seem like it does the right thing.
https://codereview.chromium.org/1394763003/diff/130001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/background_with_1_px_border.cc (right): https://codereview.chromium.org/1394763003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/background_with_1_px_border.cc:30: border_rect_f.Inset(kPostScaleOffset, kPostScaleOffset); On 2015/10/21 16:55:57, Peter Kasting wrote: > If evan is going to use this for the omnibox chips, is this the math that they > want to use? It's not clear to me whether this is specifically applicable to > anything other than the omnibox itself. For my part, I'm happy to leave questions like this unanswered until I do actually implement chips, at which point I can refactor whatever's necessary.
https://codereview.chromium.org/1394763003/diff/130001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/background_with_1_px_border.cc (right): https://codereview.chromium.org/1394763003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/background_with_1_px_border.cc:35: border_rect_f.Inset(0.0f, kPreScaleOffset); On 2015/10/21 16:55:57, Peter Kasting wrote: > It's non-obvious to a reader why you're not indenting the sides here. And to > someone who knows, it's a bit odd that we tied this to a "round corners" bit, as > this doesn't really have to do with rounding the corners per se, but more with > whether we're in popup mode. Furthermore, in popup mode, we really only want to > do the no-inset thing in the case where the location bar horizontal edge > thickness is zero, we don't want to do it all the time. So as-is, this code > doesn't seem like it does the right thing. Then it may be more appropriate to bring this out into it's own class to better differentiate the two. And update LocationBarView to provide it with the needed information for 0 thickness horizontal edges.
https://codereview.chromium.org/1394763003/diff/130001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/background_with_1_px_border.cc (right): https://codereview.chromium.org/1394763003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/background_with_1_px_border.cc:35: border_rect_f.Inset(0.0f, kPreScaleOffset); On 2015/10/21 17:34:50, jonross wrote: > On 2015/10/21 16:55:57, Peter Kasting wrote: > > It's non-obvious to a reader why you're not indenting the sides here. And to > > someone who knows, it's a bit odd that we tied this to a "round corners" bit, > as > > this doesn't really have to do with rounding the corners per se, but more with > > whether we're in popup mode. Furthermore, in popup mode, we really only want > to > > do the no-inset thing in the case where the location bar horizontal edge > > thickness is zero, we don't want to do it all the time. So as-is, this code > > doesn't seem like it does the right thing. > > Then it may be more appropriate to bring this out into it's own class to better > differentiate the two. And update LocationBarView to provide it with the needed > information for 0 thickness horizontal edges. In LocationBarView, we use GetHorizontalEdgeThickness() for this. You could find a way to use that, or another function like it, a la: border_rect_f.Inset(owner.GetEdgeInsets()); border_rect_f.Scale(scale); border_rect_f.Inset(-0.5, -0.5); There are other solutions, too -- you could provide the necessary info on construction, although even if you say "use 0 edge width for maximized horizontal edges", the border still needs a way to know when it's maximized.
On 2015/10/21 18:05:36, Peter Kasting wrote: > https://codereview.chromium.org/1394763003/diff/130001/chrome/browser/ui/view... > File chrome/browser/ui/views/location_bar/background_with_1_px_border.cc > (right): > > https://codereview.chromium.org/1394763003/diff/130001/chrome/browser/ui/view... > chrome/browser/ui/views/location_bar/background_with_1_px_border.cc:35: > border_rect_f.Inset(0.0f, kPreScaleOffset); > On 2015/10/21 17:34:50, jonross wrote: > > On 2015/10/21 16:55:57, Peter Kasting wrote: > > > It's non-obvious to a reader why you're not indenting the sides here. And > to > > > someone who knows, it's a bit odd that we tied this to a "round corners" > bit, > > as > > > this doesn't really have to do with rounding the corners per se, but more > with > > > whether we're in popup mode. Furthermore, in popup mode, we really only > want > > to > > > do the no-inset thing in the case where the location bar horizontal edge > > > thickness is zero, we don't want to do it all the time. So as-is, this code > > > doesn't seem like it does the right thing. > > > > Then it may be more appropriate to bring this out into it's own class to > better > > differentiate the two. And update LocationBarView to provide it with the > needed > > information for 0 thickness horizontal edges. > > In LocationBarView, we use GetHorizontalEdgeThickness() for this. You could > find a way to use that, or another function like it, a la: > > border_rect_f.Inset(owner.GetEdgeInsets()); > border_rect_f.Scale(scale); > border_rect_f.Inset(-0.5, -0.5); > > There are other solutions, too -- you could provide the necessary info on > construction, although even if you say "use 0 edge width for maximized > horizontal edges", the border still needs a way to know when it's maximized. Updated. The maximized state for popup mode can be detected at render time. Now that the border renders underneath the child views I updated the layout to make use of the border thickness, slightly tweaked previous layout params.
LGTM https://codereview.chromium.org/1394763003/diff/150001/chrome/browser/ui/view... File chrome/browser/ui/views/layout_constants.h (right): https://codereview.chromium.org/1394763003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/layout_constants.h:28: LOCATION_BAR_EDGE_THICKNESS, Nit: Use BORDER rather than EDGE since semantically, the "edge" is one side or the other of the border. https://codereview.chromium.org/1394763003/diff/150001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/background_with_1_px_border.cc (right): https://codereview.chromium.org/1394763003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/background_with_1_px_border.cc:23: const float scale = canvas->SaveAndUnscale(); Note: You'll need to update this to compile against Dana's latest changes. (Use a ScopedCanvas, then call UndoDeviceScaleFactor().) https://codereview.chromium.org/1394763003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/background_with_1_px_border.cc:24: const float kPreScaleOffset = 1.0f; Nit: Since you added LOCATION_BAR_EDGE_THICKNESS, this should probably use that instead of hardcoding the same value. https://codereview.chromium.org/1394763003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/background_with_1_px_border.cc:36: if (view->GetWidget()->IsMaximized()) Please add comments describing why we're doing this, e.g. by copying them over from the location bar code. I'm still a bit concerned that this behavior is more "specific to the popup location bar" than "specific to rounding corners". To me, it would be clearer to make the class arg/member be "is popup mode" or "is the popup mode location bar" rather than "round corners"; that matches how the location bar thinks about this, and it makes it clearer that the distinction is more than just rounding corners, it's about generally doing the border how popups want it. That will also make it obvious that if we use this for other things than the location bar (e.g. the bubbles inside it), and we for some reason decided to not round their corners, that it wouldn't be appropriate to just flip the flag on this class. https://codereview.chromium.org/1394763003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/background_with_1_px_border.cc:39: border_rect_f.Inset(kPreScaleOffset, kPreScaleOffset); Nit: Simpler: border_rect_f.Inset(view->GetWidget()->IsMaximized() ? 0 : kPreScaleOffset, kPreScaleOffset);
I updated the background/border class to refer to popup mode instead of rounded corners. So that it's more explicit. I'll land this unless you have any other concerns. https://codereview.chromium.org/1394763003/diff/150001/chrome/browser/ui/view... File chrome/browser/ui/views/layout_constants.h (right): https://codereview.chromium.org/1394763003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/layout_constants.h:28: LOCATION_BAR_EDGE_THICKNESS, On 2015/10/30 21:07:14, Peter Kasting wrote: > Nit: Use BORDER rather than EDGE since semantically, the "edge" is one side or > the other of the border. Done. https://codereview.chromium.org/1394763003/diff/150001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/background_with_1_px_border.cc (right): https://codereview.chromium.org/1394763003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/background_with_1_px_border.cc:23: const float scale = canvas->SaveAndUnscale(); On 2015/10/30 21:07:14, Peter Kasting wrote: > Note: You'll need to update this to compile against Dana's latest changes. (Use > a ScopedCanvas, then call UndoDeviceScaleFactor().) Done. https://codereview.chromium.org/1394763003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/background_with_1_px_border.cc:24: const float kPreScaleOffset = 1.0f; On 2015/10/30 21:07:14, Peter Kasting wrote: > Nit: Since you added LOCATION_BAR_EDGE_THICKNESS, this should probably use that > instead of hardcoding the same value. Done. https://codereview.chromium.org/1394763003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/background_with_1_px_border.cc:36: if (view->GetWidget()->IsMaximized()) On 2015/10/30 21:07:14, Peter Kasting wrote: > Please add comments describing why we're doing this, e.g. by copying them over > from the location bar code. > > I'm still a bit concerned that this behavior is more "specific to the popup > location bar" than "specific to rounding corners". To me, it would be clearer > to make the class arg/member be "is popup mode" or "is the popup mode location > bar" rather than "round corners"; that matches how the location bar thinks about > this, and it makes it clearer that the distinction is more than just rounding > corners, it's about generally doing the border how popups want it. > > That will also make it obvious that if we use this for other things than the > location bar (e.g. the bubbles inside it), and we for some reason decided to not > round their corners, that it wouldn't be appropriate to just flip the flag on > this class. Switched over the naming to make it more consistent with the implementation. It now tracks is_popup_mode_ https://codereview.chromium.org/1394763003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/background_with_1_px_border.cc:39: border_rect_f.Inset(kPreScaleOffset, kPreScaleOffset); On 2015/10/30 21:07:14, Peter Kasting wrote: > Nit: Simpler: > > border_rect_f.Inset(view->GetWidget()->IsMaximized() ? 0 : kPreScaleOffset, > kPreScaleOffset); Done.
The CQ bit was checked by jonross@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from estade@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/1394763003/#ps110002 (title: "Update nomenclature")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1394763003/110002 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1394763003/110002
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by jonross@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1394763003/110002 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1394763003/110002
Message was sent while issue was closed.
Committed patchset #8 (id:110002)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/e026ad2bbf8946970784b810a771a5d31a6903e3 Cr-Commit-Position: refs/heads/master@{#357571} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
