Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1909453004:
[Mac][Material Design] Adjust EV chip to match Material Design spec. (Closed)
Description[Mac][Material Design] Adjust EV chip to match Material Design spec.
Tweak the EV chip to match the spec, per sgabriel@.
R=avi@chromium.org
BUG=596594
Committed: https://crrev.com/41af3ca181bd1abeda8ef5264a646da9d58e6821
Cr-Commit-Position: refs/heads/master@{#390116}
Patch Set 1 #
Total comments: 31
Patch Set 2 : Fix problems. #
Total comments: 8
Patch Set 3 : Fix nits. #
Messages
Total messages: 19 (6 generated)
PTAL
Description was changed from ========== [Mac][Material Design] Adjust EV chip to match Material Design spec. Tweak the EV chip to match the spec, per sgabriel@. R=avi@chromium.org BUG=596594 ========== to ========== [Mac][Material Design] Adjust EV chip to match Material Design spec. Tweak the EV chip to match the spec, per sgabriel@. R=avi@chromium.org BUG=596594 ==========
shrike@chromium.org changed reviewers: + tapted@chromium.org - avi@chromium.org
PTAL
note the cl description still has R=avi in it :) https://codereview.chromium.org/1909453004/diff/1/chrome/browser/ui/cocoa/loc... File chrome/browser/ui/cocoa/location_bar/bubble_decoration.h (right): https://codereview.chromium.org/1909453004/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/bubble_decoration.h:30: void SetFont(NSFont* font); Does anything rely on LocationBarDecoration::GetFont()? (i.e. is there a risk adding SetFont, but not overriding GetFont()). But then I did some `git grep`ing and it looks like nothing (yet) actually overrides LocationBarDecoration::GetFont(). (I guess it should probably be non-virtual, and be called GetDefaultFont() or something) https://codereview.chromium.org/1909453004/diff/1/chrome/browser/ui/cocoa/loc... File chrome/browser/ui/cocoa/location_bar/bubble_decoration.mm (right): https://codereview.chromium.org/1909453004/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/bubble_decoration.mm:35: : baseline_offset_(0) { nit: put on the line above? - pretty sure that's what `git cl format` would choose https://codereview.chromium.org/1909453004/diff/1/chrome/browser/ui/cocoa/loc... File chrome/browser/ui/cocoa/location_bar/ev_bubble_decoration.h (right): https://codereview.chromium.org/1909453004/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/ev_bubble_decoration.h:37: void DrawWithBackgroundInFrame(NSRect background_frame, nit: move dec into the other LocationBarDecoration overrides - (we usually follow the order from the parent class too, so it could go between `GetWidthForSpace` and `IsDraggable`, but some of the later stuff is already out of order) Then I'd put the comment into the .cc https://codereview.chromium.org/1909453004/diff/1/chrome/browser/ui/cocoa/loc... File chrome/browser/ui/cocoa/location_bar/ev_bubble_decoration.mm (right): https://codereview.chromium.org/1909453004/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/ev_bubble_decoration.mm:64: SetFont([NSFont systemFontOfSize:11]); `const int kMaterialFontSize = 11;` in the anon namespace? Or I think this is also [NSFont smallSystemFontSize]. There's also ResourceBundle::GetFontWithDelta(-2).GetNativeFont(), but that's probably not worth it since we don't need a gfx::Font at any point https://codereview.chromium.org/1909453004/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/ev_bubble_decoration.mm:96: CGFloat lineWidth = [control_view cr_lineWidth]; nit: since this isn't between @implementation/@end, we use hacker_style: line_width, in_dark_mode https://codereview.chromium.org/1909453004/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/ev_bubble_decoration.mm:104: xRadius:2 `const CFGloat kMaterialBorderRadius = 2;` in anon namespace https://codereview.chromium.org/1909453004/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/ev_bubble_decoration.mm:111: [[NSColor colorWithCalibratedRed:46 / 255. This will use the wrong colorspace (assuming the value #2E8732 was given in srgb, which is what css is). The NSColor documentation now even says "Where possible, it is preferable to specify the colorspace explicitly using the colorWithSRGBRed:green:blue:alpha: or colorWithGenericGamma22White:alpha: method.", and now we've ditched 10.6, we can actually use colorWithSRGBRed: \o/. But actually the typical way we do this is: const SkColor kMaterialBackgroundColor = SkColorSetARGB(0x0d, 0x2e, 0x87, 0x32); .. [skia::SkColorToSRGBNSColor(kMaterialBackgroundColor) set]; (using stuff in skia/ext/skia_utils_mac.h) https://codereview.chromium.org/1909453004/diff/1/chrome/browser/ui/cocoa/loc... File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right): https://codereview.chromium.org/1909453004/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:557: void LocationBarViewMac::UpdateLocationIcon() { Should this be UpdateMaterialLocationIcon? I had to poke around a bit to convince myself that the non-material EV chip's icon wasn't going to change too :) https://codereview.chromium.org/1909453004/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:562: int iconSize = 16; these should all be hacker_style const int kDefaultVectorLocationIconSize = 16; const int kEVBubbleVectorLocationIconSize = 12; int icon_size = kDefaultLocationIconSize; etc (inDarkMode should be hacker_style too.. `const bool kInDarkMode` is another option, but the style guide is a bit vague about whether things declared `const` that are not *compile time* const, should be kFooSyle or just remain hacker_style) https://codereview.chromium.org/1909453004/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:563: if (ShouldShowEVBubble()) { Hm. could this even move into ev_bubble_decoration.mm? It looks like ev_bubble_decoration_ is either visible or not, but it always has the same "valid" icon -- there shouldn't ever be a need to call SetImage on it after construction.
https://codereview.chromium.org/1909453004/diff/1/chrome/browser/ui/cocoa/loc... File chrome/browser/ui/cocoa/location_bar/ev_bubble_decoration.h (right): https://codereview.chromium.org/1909453004/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/ev_bubble_decoration.h:37: void DrawWithBackgroundInFrame(NSRect background_frame, On 2016/04/22 00:41:52, tapted wrote: > nit: move dec into the other LocationBarDecoration overrides - (we usually > follow the order from the parent class too, so it could go between > `GetWidthForSpace` and `IsDraggable`, but some of the later stuff is already out > of order) > > Then I'd put the comment into the .cc Unfortunately I'm not following what you mean - can you re-explain what you're asking for? And "dec" stands for "declaration?" https://codereview.chromium.org/1909453004/diff/1/chrome/browser/ui/cocoa/loc... File chrome/browser/ui/cocoa/location_bar/ev_bubble_decoration.mm (right): https://codereview.chromium.org/1909453004/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/ev_bubble_decoration.mm:111: [[NSColor colorWithCalibratedRed:46 / 255. On 2016/04/22 00:41:52, tapted wrote: > This will use the wrong colorspace (assuming the value #2E8732 was given in > srgb, which is what css is). The NSColor documentation now even says "Where > possible, it is preferable to specify the colorspace explicitly using the > colorWithSRGBRed:green:blue:alpha: or colorWithGenericGamma22White:alpha: > method.", and now we've ditched 10.6, we can actually use colorWithSRGBRed: \o/. > But actually the typical way we do this is: > > const SkColor kMaterialBackgroundColor = SkColorSetARGB(0x0d, 0x2e, 0x87, 0x32); > .. > [skia::SkColorToSRGBNSColor(kMaterialBackgroundColor) set]; > > > (using stuff in skia/ext/skia_utils_mac.h) The comment you quote is only in reference to colorWithRed:green:blue:alpha:, not colorWithCalibratedRed:.... At this point I think it's better to leave it as is, and switch to SkColorToSRGBNSColor() in this and all other places in a different cl. I didn't run across this function before (all the other places I've seen use calibratedColor). https://codereview.chromium.org/1909453004/diff/1/chrome/browser/ui/cocoa/loc... File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right): https://codereview.chromium.org/1909453004/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:562: int iconSize = 16; On 2016/04/22 00:41:52, tapted wrote: > these should all be hacker_style > > const int kDefaultVectorLocationIconSize = 16; > const int kEVBubbleVectorLocationIconSize = 12; > > int icon_size = kDefaultLocationIconSize; etc > > > (inDarkMode should be hacker_style too.. `const bool kInDarkMode` is another > option, but the style guide is a bit vague about whether things declared `const` > that are not *compile time* const, should be kFooSyle or just remain > hacker_style) By "hacker_style" you mean with underbars? This is an ObjectiveC file, and the convention is camel case? I;m not sure what part of the style guide you are thinking of. https://codereview.chromium.org/1909453004/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:563: if (ShouldShowEVBubble()) { On 2016/04/22 00:41:52, tapted wrote: > Hm. could this even move into ev_bubble_decoration.mm? It looks like > ev_bubble_decoration_ is either visible or not, but it always has the same > "valid" icon -- there shouldn't ever be a need to call SetImage on it after > construction. In general the icon does not change but that's not always true (if, for example, you have an Incognito window that has it start of in dark mode and then you change the theme to a "light" custom theme).
https://codereview.chromium.org/1909453004/diff/1/chrome/browser/ui/cocoa/loc... File chrome/browser/ui/cocoa/location_bar/ev_bubble_decoration.h (right): https://codereview.chromium.org/1909453004/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/ev_bubble_decoration.h:37: void DrawWithBackgroundInFrame(NSRect background_frame, On 2016/04/22 05:46:30, shrike wrote: > On 2016/04/22 00:41:52, tapted wrote: > > nit: move dec into the other LocationBarDecoration overrides - (we usually > > follow the order from the parent class too, so it could go between > > `GetWidthForSpace` and `IsDraggable`, but some of the later stuff is already > out > > of order) > > > > Then I'd put the comment into the .cc > > Unfortunately I'm not following what you mean - can you re-explain what you're > asking for? And "dec" stands for "declaration?" yup dec is declaration. Since this method is an override of a method on LocationBarDecoration it should be grouped together with the other overrides on LocationBarDecoration which is just below; under "// Implement |LocationBarDecoration|. And when doing that, it's generally good to follow the same declaration order that the parent class uses -- e.g. it makes it easy to see which methods a subclass *doesn't* override by comparing the list. https://codereview.chromium.org/1909453004/diff/1/chrome/browser/ui/cocoa/loc... File chrome/browser/ui/cocoa/location_bar/ev_bubble_decoration.mm (right): https://codereview.chromium.org/1909453004/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/ev_bubble_decoration.mm:111: [[NSColor colorWithCalibratedRed:46 / 255. On 2016/04/22 05:46:30, shrike wrote: > On 2016/04/22 00:41:52, tapted wrote: > > This will use the wrong colorspace (assuming the value #2E8732 was given in > > srgb, which is what css is). The NSColor documentation now even says "Where > > possible, it is preferable to specify the colorspace explicitly using the > > colorWithSRGBRed:green:blue:alpha: or colorWithGenericGamma22White:alpha: > > method.", and now we've ditched 10.6, we can actually use colorWithSRGBRed: > \o/. > > But actually the typical way we do this is: > > > > const SkColor kMaterialBackgroundColor = SkColorSetARGB(0x0d, 0x2e, 0x87, > 0x32); > > .. > > [skia::SkColorToSRGBNSColor(kMaterialBackgroundColor) set]; > > > > > > (using stuff in skia/ext/skia_utils_mac.h) > > The comment you quote is only in reference to colorWithRed:green:blue:alpha:, > not colorWithCalibratedRed:.... > > At this point I think it's better to leave it as is, and switch to > SkColorToSRGBNSColor() in this and all other places in a different cl. I didn't > run across this function before (all the other places I've seen use > calibratedColor). > If it's left as is the color will be wrong, see e.g. http://crbug.com/257275 and http://crbug.com/254361. This is because the colorspace for "calibrated" is also badly specified. (e.g. before 10.4 it was "device", now it is "the `generic` colorspace that OSX stopped using for system UI in 10.6"). sRGB on the other hand, is well-defined and widely used. The problem with switching to SkColorToSRGBNSColor _afterwards_ is that the color will actually change, and we don't have access to designer specs that say what the color "should" be. But in this case, we have a HTML color value from a designer, so we should assume they want sRGB for it. (and, certainly, all toolkit-views UI is rendered into canvases assuming sRGB) https://codereview.chromium.org/1909453004/diff/1/chrome/browser/ui/cocoa/loc... File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right): https://codereview.chromium.org/1909453004/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:562: int iconSize = 16; On 2016/04/22 05:46:30, shrike wrote: > On 2016/04/22 00:41:52, tapted wrote: > > these should all be hacker_style > > > > const int kDefaultVectorLocationIconSize = 16; > > const int kEVBubbleVectorLocationIconSize = 12; > > > > int icon_size = kDefaultLocationIconSize; etc > > > > > > (inDarkMode should be hacker_style too.. `const bool kInDarkMode` is another > > option, but the style guide is a bit vague about whether things declared > `const` > > that are not *compile time* const, should be kFooSyle or just remain > > hacker_style) > > By "hacker_style" you mean with underbars? This is an ObjectiveC file, and the > convention is camel case? I;m not sure what part of the style guide you are > thinking of. yup - underbars. And the naming scheme for Objective-C isn't based on the file extension, but on whether it's in an Objective C class, which is easiest to apply as "any new declarations between @{imlementation,interface,etc.}/@end. E.g. `web_contents` appears in this file quite a bit, and all the methods take parameters in hacker_style. go/cppguide?cl=head#Constant_Names says "Variables declared constexpr or const, **and whose value is fixed for the duration of the program**, are named with a leading "k" followed by mixed case." (i.e. I interpret that as meaning if something is declared `const` but initialized by a (non-constexpr) method call, then it should not have kNamingStyle) https://codereview.chromium.org/1909453004/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:563: if (ShouldShowEVBubble()) { On 2016/04/22 05:46:30, shrike wrote: > On 2016/04/22 00:41:52, tapted wrote: > > Hm. could this even move into ev_bubble_decoration.mm? It looks like > > ev_bubble_decoration_ is either visible or not, but it always has the same > > "valid" icon -- there shouldn't ever be a need to call SetImage on it after > > construction. > > In general the icon does not change but that's not always true (if, for example, > you have an Incognito window that has it start of in dark mode and then you > change the theme to a "light" custom theme). wouldn't that need handling for `vectorIconColor` on line 565 to take into account in_dark_mode
https://codereview.chromium.org/1909453004/diff/1/chrome/browser/ui/cocoa/loc... File chrome/browser/ui/cocoa/location_bar/ev_bubble_decoration.h (right): https://codereview.chromium.org/1909453004/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/ev_bubble_decoration.h:37: void DrawWithBackgroundInFrame(NSRect background_frame, On 2016/04/22 06:09:48, tapted wrote: > On 2016/04/22 05:46:30, shrike wrote: > > On 2016/04/22 00:41:52, tapted wrote: > > > nit: move dec into the other LocationBarDecoration overrides - (we usually > > > follow the order from the parent class too, so it could go between > > > `GetWidthForSpace` and `IsDraggable`, but some of the later stuff is already > > out > > > of order) > > > > > > Then I'd put the comment into the .cc > > > > Unfortunately I'm not following what you mean - can you re-explain what you're > > asking for? And "dec" stands for "declaration?" > > yup dec is declaration. Since this method is an override of a method on > LocationBarDecoration it should be grouped together with the other overrides on > LocationBarDecoration which is just below; under "// Implement > |LocationBarDecoration|. > > And when doing that, it's generally good to follow the same declaration order > that the parent class uses -- e.g. it makes it easy to see which methods a > subclass *doesn't* override by comparing the list. Got it. https://codereview.chromium.org/1909453004/diff/1/chrome/browser/ui/cocoa/loc... File chrome/browser/ui/cocoa/location_bar/ev_bubble_decoration.mm (right): https://codereview.chromium.org/1909453004/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/ev_bubble_decoration.mm:111: [[NSColor colorWithCalibratedRed:46 / 255. On 2016/04/22 06:09:48, tapted wrote: > On 2016/04/22 05:46:30, shrike wrote: > > On 2016/04/22 00:41:52, tapted wrote: > > > This will use the wrong colorspace (assuming the value #2E8732 was given in > > > srgb, which is what css is). The NSColor documentation now even says "Where > > > possible, it is preferable to specify the colorspace explicitly using the > > > colorWithSRGBRed:green:blue:alpha: or colorWithGenericGamma22White:alpha: > > > method.", and now we've ditched 10.6, we can actually use colorWithSRGBRed: > > \o/. > > > But actually the typical way we do this is: > > > > > > const SkColor kMaterialBackgroundColor = SkColorSetARGB(0x0d, 0x2e, 0x87, > > 0x32); > > > .. > > > [skia::SkColorToSRGBNSColor(kMaterialBackgroundColor) set]; > > > > > > > > > (using stuff in skia/ext/skia_utils_mac.h) > > > > The comment you quote is only in reference to colorWithRed:green:blue:alpha:, > > not colorWithCalibratedRed:.... > > > > At this point I think it's better to leave it as is, and switch to > > SkColorToSRGBNSColor() in this and all other places in a different cl. I > didn't > > run across this function before (all the other places I've seen use > > calibratedColor). > > > > If it's left as is the color will be wrong, see e.g. http://crbug.com/257275 and > http://crbug.com/254361. This is because the colorspace for "calibrated" is also > badly specified. (e.g. before 10.4 it was "device", now it is "the `generic` > colorspace that OSX stopped using for system UI in 10.6"). sRGB on the other > hand, is well-defined and widely used. > > The problem with switching to SkColorToSRGBNSColor _afterwards_ is that the > color will actually change, and we don't have access to designer specs that say > what the color "should" be. But in this case, we have a HTML color value from a > designer, so we should assume they want sRGB for it. > > (and, certainly, all toolkit-views UI is rendered into canvases assuming sRGB) Yes, you are correct the color will be wrong. But what I am saying is that colors are already wrong in many other places because I used the calibratedColor method. At least using calibratedColor here makes the colors consistently wrong instead of one right plus the rest wrong. I'm not sure what the issue is with switching over to SkColorToSRGBNSColor at a later date. Even if I use SkColorToSRGBNSColor here I should make that change everywhere else anyway (which will be switching over at a later date). I do have specs today - they don't specify a color space but they are saved in sRGB. I can also double-check with the designer. https://codereview.chromium.org/1909453004/diff/1/chrome/browser/ui/cocoa/loc... File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right): https://codereview.chromium.org/1909453004/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:562: int iconSize = 16; On 2016/04/22 06:09:48, tapted wrote: > On 2016/04/22 05:46:30, shrike wrote: > > On 2016/04/22 00:41:52, tapted wrote: > > > these should all be hacker_style > > > > > > const int kDefaultVectorLocationIconSize = 16; > > > const int kEVBubbleVectorLocationIconSize = 12; > > > > > > int icon_size = kDefaultLocationIconSize; etc > > > > > > > > > (inDarkMode should be hacker_style too.. `const bool kInDarkMode` is another > > > option, but the style guide is a bit vague about whether things declared > > `const` > > > that are not *compile time* const, should be kFooSyle or just remain > > > hacker_style) > > > > By "hacker_style" you mean with underbars? This is an ObjectiveC file, and the > > convention is camel case? I;m not sure what part of the style guide you are > > thinking of. > > yup - underbars. And the naming scheme for Objective-C isn't based on the file > extension, but on whether it's in an Objective C class, which is easiest to > apply as "any new declarations between @{imlementation,interface,etc.}/@end. > E.g. `web_contents` appears in this file quite a bit, and all the methods take > parameters in hacker_style. > > go/cppguide?cl=head#Constant_Names says "Variables declared constexpr or const, > **and whose value is fixed for the duration of the program**, are named with a > leading "k" followed by mixed case." > > (i.e. I interpret that as meaning if something is declared `const` but > initialized by a (non-constexpr) method call, then it should not have > kNamingStyle) Ah, OK, I see where I was confused. Thanks. https://codereview.chromium.org/1909453004/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:563: if (ShouldShowEVBubble()) { On 2016/04/22 06:09:48, tapted wrote: > On 2016/04/22 05:46:30, shrike wrote: > > On 2016/04/22 00:41:52, tapted wrote: > > > Hm. could this even move into ev_bubble_decoration.mm? It looks like > > > ev_bubble_decoration_ is either visible or not, but it always has the same > > > "valid" icon -- there shouldn't ever be a need to call SetImage on it after > > > construction. > > > > In general the icon does not change but that's not always true (if, for > example, > > you have an Incognito window that has it start of in dark mode and then you > > change the theme to a "light" custom theme). > > wouldn't that need handling for `vectorIconColor` on line 565 to take into > account in_dark_mode The vector icon code is a little screwy (if you ask me). It always takes a color, but sometimes the color is ignored. In the case of the lock, when you're in dark mode the color is hard-coded within the icon to be white.
some follow-ups (sorry for the delay - Monday was a public holiday, so lots to catch up on). https://codereview.chromium.org/1909453004/diff/1/chrome/browser/ui/cocoa/loc... File chrome/browser/ui/cocoa/location_bar/ev_bubble_decoration.mm (right): https://codereview.chromium.org/1909453004/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/ev_bubble_decoration.mm:111: [[NSColor colorWithCalibratedRed:46 / 255. On 2016/04/22 06:24:15, shrike wrote: > On 2016/04/22 06:09:48, tapted wrote: > > On 2016/04/22 05:46:30, shrike wrote: > > > On 2016/04/22 00:41:52, tapted wrote: > > > > This will use the wrong colorspace (assuming the value #2E8732 was given > in > > > > srgb, which is what css is). The NSColor documentation now even says > "Where > > > > possible, it is preferable to specify the colorspace explicitly using the > > > > colorWithSRGBRed:green:blue:alpha: or colorWithGenericGamma22White:alpha: > > > > method.", and now we've ditched 10.6, we can actually use > colorWithSRGBRed: > > > \o/. > > > > But actually the typical way we do this is: > > > > > > > > const SkColor kMaterialBackgroundColor = SkColorSetARGB(0x0d, 0x2e, 0x87, > > > 0x32); > > > > .. > > > > [skia::SkColorToSRGBNSColor(kMaterialBackgroundColor) set]; > > > > > > > > > > > > (using stuff in skia/ext/skia_utils_mac.h) > > > > > > The comment you quote is only in reference to > colorWithRed:green:blue:alpha:, > > > not colorWithCalibratedRed:.... > > > > > > At this point I think it's better to leave it as is, and switch to > > > SkColorToSRGBNSColor() in this and all other places in a different cl. I > > didn't > > > run across this function before (all the other places I've seen use > > > calibratedColor). > > > > > > > If it's left as is the color will be wrong, see e.g. http://crbug.com/257275 > and > > http://crbug.com/254361. This is because the colorspace for "calibrated" is > also > > badly specified. (e.g. before 10.4 it was "device", now it is "the `generic` > > colorspace that OSX stopped using for system UI in 10.6"). sRGB on the other > > hand, is well-defined and widely used. > > > > The problem with switching to SkColorToSRGBNSColor _afterwards_ is that the > > color will actually change, and we don't have access to designer specs that > say > > what the color "should" be. But in this case, we have a HTML color value from > a > > designer, so we should assume they want sRGB for it. > > > > (and, certainly, all toolkit-views UI is rendered into canvases assuming sRGB) > > Yes, you are correct the color will be wrong. But what I am saying is that > colors are already wrong in many other places because I used the calibratedColor > method. At least using calibratedColor here makes the colors consistently wrong > instead of one right plus the rest wrong. > > I'm not sure what the issue is with switching over to SkColorToSRGBNSColor at a > later date. Even if I use SkColorToSRGBNSColor here I should make that change > everywhere else anyway (which will be switching over at a later date). I do have > specs today - they don't specify a color space but they are saved in sRGB. I can > also double-check with the designer. Ah, yep - definitely go for consistency with the existing UI :). With regard to switching later - switching for a subset of the UI might be OK. But my concern with updating calls mechanically has always been that we don't know whether a designer had looked at the colors as they appear in the browser and thought 'this should be darker' and so updated the spec to be darker. I guess it should be subtle enough to not really worry though. It's also possible some icon assets are colored to match a particular background. But that, too, should be subtle and less of a concern as we move to vector assets (and since they use Skia, they're likely to be *actually* in sRGB already when drawn). https://codereview.chromium.org/1909453004/diff/1/chrome/browser/ui/cocoa/loc... File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right): https://codereview.chromium.org/1909453004/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:563: if (ShouldShowEVBubble()) { On 2016/04/22 06:24:15, shrike wrote: > On 2016/04/22 06:09:48, tapted wrote: > > On 2016/04/22 05:46:30, shrike wrote: > > > On 2016/04/22 00:41:52, tapted wrote: > > > > Hm. could this even move into ev_bubble_decoration.mm? It looks like > > > > ev_bubble_decoration_ is either visible or not, but it always has the same > > > > "valid" icon -- there shouldn't ever be a need to call SetImage on it > after > > > > construction. > > > > > > In general the icon does not change but that's not always true (if, for > > example, > > > you have an Incognito window that has it start of in dark mode and then you > > > change the theme to a "light" custom theme). > > > > wouldn't that need handling for `vectorIconColor` on line 565 to take into > > account in_dark_mode > > The vector icon code is a little screwy (if you ask me). It always takes a > color, but sometimes the color is ignored. In the case of the lock, when you're > in dark mode the color is hard-coded within the icon to be white. Acknowledged.
PTAL https://codereview.chromium.org/1909453004/diff/1/chrome/browser/ui/cocoa/loc... File chrome/browser/ui/cocoa/location_bar/bubble_decoration.h (right): https://codereview.chromium.org/1909453004/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/bubble_decoration.h:30: void SetFont(NSFont* font); On 2016/04/22 00:41:52, tapted wrote: > Does anything rely on LocationBarDecoration::GetFont()? (i.e. is there a risk > adding SetFont, but not overriding GetFont()). But then I did some `git grep`ing > and it looks like nothing (yet) actually overrides > LocationBarDecoration::GetFont(). (I guess it should probably be non-virtual, > and be called GetDefaultFont() or something) I like the simplicity of GetFont() in LocationBarDecoration vs. a GetDefaultFont. Things are only getting messy because I'm adding a SetFont() to BubbleDecoration but as a result if you call GetFont() on a BubbleDecoration you may not get the correct answer. I think the best way to fix it is to just override GetFont() in BubbleDecoration and have it return the correct result. https://codereview.chromium.org/1909453004/diff/1/chrome/browser/ui/cocoa/loc... File chrome/browser/ui/cocoa/location_bar/bubble_decoration.mm (right): https://codereview.chromium.org/1909453004/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/bubble_decoration.mm:35: : baseline_offset_(0) { On 2016/04/22 00:41:52, tapted wrote: > nit: put on the line above? - pretty sure that's what `git cl format` would > choose Done. https://codereview.chromium.org/1909453004/diff/1/chrome/browser/ui/cocoa/loc... File chrome/browser/ui/cocoa/location_bar/ev_bubble_decoration.mm (right): https://codereview.chromium.org/1909453004/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/ev_bubble_decoration.mm:64: SetFont([NSFont systemFontOfSize:11]); On 2016/04/22 00:41:52, tapted wrote: > `const int kMaterialFontSize = 11;` in the anon namespace? Or I think this is > also [NSFont smallSystemFontSize]. > > There's also ResourceBundle::GetFontWithDelta(-2).GetNativeFont(), but that's > probably not worth it since we don't need a gfx::Font at any point I think kMaterialFontSize is best - the spec calls for 11pt, and I don't think there's a contract that says smallSystemFontSize does and will always return 11 https://codereview.chromium.org/1909453004/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/ev_bubble_decoration.mm:96: CGFloat lineWidth = [control_view cr_lineWidth]; On 2016/04/22 00:41:52, tapted wrote: > nit: since this isn't between @implementation/@end, we use hacker_style: > line_width, in_dark_mode Done. https://codereview.chromium.org/1909453004/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/ev_bubble_decoration.mm:104: xRadius:2 On 2016/04/22 00:41:52, tapted wrote: > `const CFGloat kMaterialBorderRadius = 2;` in anon namespace Done. https://codereview.chromium.org/1909453004/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/ev_bubble_decoration.mm:111: [[NSColor colorWithCalibratedRed:46 / 255. On 2016/04/25 21:48:10, tapted wrote: > On 2016/04/22 06:24:15, shrike wrote: > > On 2016/04/22 06:09:48, tapted wrote: > > > On 2016/04/22 05:46:30, shrike wrote: > > > > On 2016/04/22 00:41:52, tapted wrote: > > > > > This will use the wrong colorspace (assuming the value #2E8732 was given > > in > > > > > srgb, which is what css is). The NSColor documentation now even says > > "Where > > > > > possible, it is preferable to specify the colorspace explicitly using > the > > > > > colorWithSRGBRed:green:blue:alpha: or > colorWithGenericGamma22White:alpha: > > > > > method.", and now we've ditched 10.6, we can actually use > > colorWithSRGBRed: > > > > \o/. > > > > > But actually the typical way we do this is: > > > > > > > > > > const SkColor kMaterialBackgroundColor = SkColorSetARGB(0x0d, 0x2e, > 0x87, > > > > 0x32); > > > > > .. > > > > > [skia::SkColorToSRGBNSColor(kMaterialBackgroundColor) set]; > > > > > > > > > > > > > > > (using stuff in skia/ext/skia_utils_mac.h) > > > > > > > > The comment you quote is only in reference to > > colorWithRed:green:blue:alpha:, > > > > not colorWithCalibratedRed:.... > > > > > > > > At this point I think it's better to leave it as is, and switch to > > > > SkColorToSRGBNSColor() in this and all other places in a different cl. I > > > didn't > > > > run across this function before (all the other places I've seen use > > > > calibratedColor). > > > > > > > > > > If it's left as is the color will be wrong, see e.g. http://crbug.com/257275 > > and > > > http://crbug.com/254361. This is because the colorspace for "calibrated" is > > also > > > badly specified. (e.g. before 10.4 it was "device", now it is "the `generic` > > > colorspace that OSX stopped using for system UI in 10.6"). sRGB on the other > > > hand, is well-defined and widely used. > > > > > > The problem with switching to SkColorToSRGBNSColor _afterwards_ is that the > > > color will actually change, and we don't have access to designer specs that > > say > > > what the color "should" be. But in this case, we have a HTML color value > from > > a > > > designer, so we should assume they want sRGB for it. > > > > > > (and, certainly, all toolkit-views UI is rendered into canvases assuming > sRGB) > > > > Yes, you are correct the color will be wrong. But what I am saying is that > > colors are already wrong in many other places because I used the > calibratedColor > > method. At least using calibratedColor here makes the colors consistently > wrong > > instead of one right plus the rest wrong. > > > > I'm not sure what the issue is with switching over to SkColorToSRGBNSColor at > a > > later date. Even if I use SkColorToSRGBNSColor here I should make that change > > everywhere else anyway (which will be switching over at a later date). I do > have > > specs today - they don't specify a color space but they are saved in sRGB. I > can > > also double-check with the designer. > > Ah, yep - definitely go for consistency with the existing UI :). > > With regard to switching later - switching for a subset of the UI might be OK. > But my concern with updating calls mechanically has always been that we don't > know whether a designer had looked at the colors as they appear in the browser > and thought 'this should be darker' and so updated the spec to be darker. I > guess it should be subtle enough to not really worry though. It's also possible > some icon assets are colored to match a particular background. But that, too, > should be subtle and less of a concern as we move to vector assets (and since > they use Skia, they're likely to be *actually* in sRGB already when drawn). In Cocoa I call NSImageFromImageSkia() to convert the skia vector-derived image to an NSImage, and that returns an NSImage with NSBitmapImageReps in the generic RGB color space. There's a variant that takes a color space so that would be a way to ensure it's sRGB. I think it's fine to use an sRGB color here so I will do that. I plan to make a color correction pass at a later date to fix all the others (and the vector image cases).
lgtm % nits https://codereview.chromium.org/1909453004/diff/1/chrome/browser/ui/cocoa/loc... File chrome/browser/ui/cocoa/location_bar/ev_bubble_decoration.mm (right): https://codereview.chromium.org/1909453004/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/ev_bubble_decoration.mm:64: SetFont([NSFont systemFontOfSize:11]); On 2016/04/26 18:03:40, shrike wrote: > On 2016/04/22 00:41:52, tapted wrote: > > `const int kMaterialFontSize = 11;` in the anon namespace? Or I think this is > > also [NSFont smallSystemFontSize]. > > > > There's also ResourceBundle::GetFontWithDelta(-2).GetNativeFont(), but that's > > probably not worth it since we don't need a gfx::Font at any point > > I think kMaterialFontSize is best - the spec calls for 11pt, and I don't think > there's a contract that says smallSystemFontSize does and will always return 11 hehe - specs tend not to consider that users can "globally" rescale their fonts - at least on Windows. On Windows, Chrome obeys that by doing everything relative to "The font size used for a system ::MessageBox()", and the UI adjusts. So font pt-sizes in specs are usually mapped to deltas. ResourceBundle encapsulates this craziness. Let's just hope Apple don't decide to bring this font-scaling concept to OSX any time soon... (the help centre article "Yosemite: Make it easier to see what's on the screen." https://support.apple.com/kb/PH18831 is surprisingly quaint for an OS that goes to such lengths to look nice). https://codereview.chromium.org/1909453004/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/location_bar/bubble_decoration.h (right): https://codereview.chromium.org/1909453004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/bubble_decoration.h:30: NSFont* GetFont() const override; nit: should be with the other LocationBarDecoration overrides - after DrawWithBackgroundInFrame - but GetWidthForSpace really needs to be moved to before DrawInFrame, since it's already ordered that way in the .cc (and in LocationBarDecoration). https://codereview.chromium.org/1909453004/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/location_bar/bubble_decoration.mm (right): https://codereview.chromium.org/1909453004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/bubble_decoration.mm:160: NSFont* BubbleDecoration::GetFont() const { nit: order after DrawWithBackgroundInFrame https://codereview.chromium.org/1909453004/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h (right): https://codereview.chromium.org/1909453004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h:34: static SkColor SkBaseTextColor(bool in_dark_mode); I read `Sk` prefixes as belonging to stuff declared in third_party/skia - maybe BaseTextColorSkia ? https://codereview.chromium.org/1909453004/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right): https://codereview.chromium.org/1909453004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:167: return in_dark_mode ? kFiftyPercentWhite what you have is fine, an alternative if you like it could be return in_dark_mode ? SkColorSetA(SK_ColorWHITE, 127) : SkColorSetA(SK_ColorBLACK, 127); This avoids the assumption that SkColor is ARGB, but that's unlikely to change, and I don't really like those 127s either
https://codereview.chromium.org/1909453004/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/location_bar/bubble_decoration.h (right): https://codereview.chromium.org/1909453004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/bubble_decoration.h:30: NSFont* GetFont() const override; On 2016/04/27 02:37:39, tapted wrote: > nit: should be with the other LocationBarDecoration overrides - after > DrawWithBackgroundInFrame - but GetWidthForSpace really needs to be moved to > before DrawInFrame, since it's already ordered that way in the .cc (and in > LocationBarDecoration). Done. https://codereview.chromium.org/1909453004/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/location_bar/bubble_decoration.mm (right): https://codereview.chromium.org/1909453004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/bubble_decoration.mm:160: NSFont* BubbleDecoration::GetFont() const { On 2016/04/27 02:37:39, tapted wrote: > nit: order after DrawWithBackgroundInFrame Done. https://codereview.chromium.org/1909453004/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h (right): https://codereview.chromium.org/1909453004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h:34: static SkColor SkBaseTextColor(bool in_dark_mode); On 2016/04/27 02:37:39, tapted wrote: > I read `Sk` prefixes as belonging to stuff declared in third_party/skia - maybe > BaseTextColorSkia ? Done. https://codereview.chromium.org/1909453004/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right): https://codereview.chromium.org/1909453004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:167: return in_dark_mode ? kFiftyPercentWhite On 2016/04/27 02:37:39, tapted wrote: > what you have is fine, an alternative if you like it could be > > return in_dark_mode ? SkColorSetA(SK_ColorWHITE, 127) > : SkColorSetA(SK_ColorBLACK, 127); > > This avoids the assumption that SkColor is ARGB, but that's unlikely to change, > and I don't really like those 127s either I like your suggestion better (with 0x7F instead of 127).
The CQ bit was checked by shrike@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org Link to the patchset: https://codereview.chromium.org/1909453004/#ps40001 (title: "Fix nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1909453004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1909453004/40001
Message was sent while issue was closed.
Description was changed from ========== [Mac][Material Design] Adjust EV chip to match Material Design spec. Tweak the EV chip to match the spec, per sgabriel@. R=avi@chromium.org BUG=596594 ========== to ========== [Mac][Material Design] Adjust EV chip to match Material Design spec. Tweak the EV chip to match the spec, per sgabriel@. R=avi@chromium.org BUG=596594 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/41af3ca181bd1abeda8ef5264a646da9d58e6821 Cr-Commit-Position: refs/heads/master@{#390116}
Message was sent while issue was closed.
Description was changed from ========== [Mac][Material Design] Adjust EV chip to match Material Design spec. Tweak the EV chip to match the spec, per sgabriel@. R=avi@chromium.org BUG=596594 ========== to ========== [Mac][Material Design] Adjust EV chip to match Material Design spec. Tweak the EV chip to match the spec, per sgabriel@. R=avi@chromium.org BUG=596594 Committed: https://crrev.com/41af3ca181bd1abeda8ef5264a646da9d58e6821 Cr-Commit-Position: refs/heads/master@{#390116} ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
