Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1421813005:
Fullscreen bubble: Draw a white box around "ESC" in "Press ESC to exit". (Closed)
|
Created:
5 years, 1 month ago by Matt Giuca Modified:
5 years, 1 month ago CC:
chromium-reviews, tfarina, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@border-roundedrect Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFullscreen bubble: Draw a white box around "ESC" in "Press ESC to exit".
Also slightly changes the text from "Press Esc to exit." to "Press |ESC|
to exit". This only has an effect when the
--enable-simplified-fullscreen-ui flag is enabled.
This renames the old string IDS_FULLSCREEN_PRESS_ESC_TO_EXIT to
IDS_FULLSCREEN_PRESS_ESC_TO_EXIT_SENTENCE (still used when the flag is
not enabled, and to be deleted some day), while
IDS_FULLSCREEN_PRESS_ESC_TO_EXIT now has the new name and special syntax
to denote the key name.
BUG=352425
Committed: https://crrev.com/f838da5a288ded2f138f6d4bf3c61c8c3855ec7e
Cr-Commit-Position: refs/heads/master@{#359270}
Patch Set 1 #
Total comments: 34
Patch Set 2 : Rebase (no longer using set_collapsed_when_hidden). #Patch Set 3 : Respond to comments. #Patch Set 4 : Respond to comments. #
Total comments: 4
Patch Set 5 : Respond to comments. Also early-return. #
Depends on Patchset: Messages
Total messages: 26 (4 generated)
mgiuca@chromium.org changed reviewers: + msw@chromium.org
Screenshots on bug: https://code.google.com/p/chromium/issues/detail?id=352425#c95 Note that this is dependent on 2 CLs: - https://codereview.chromium.org/1422703004/ - https://codereview.chromium.org/1428623005/
estade@chromium.org changed reviewers: + estade@chromium.org
https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/views/exc... File chrome/browser/ui/views/exclusive_access_bubble_views.cc (right): https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/views/exc... chrome/browser/ui/views/exclusive_access_bubble_views.cc:106: SkColor background_color); I don't think you need to pass background_color, just turn off auto color readability on all the labels. It would be an error to have a situation where fg and bg (whether based on the theme or hardcoded) were not readable relative to each other. https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/views/exc... chrome/browser/ui/views/exclusive_access_bubble_views.cc:109: views::Label* before_key_; are these members necessary? you only use them in the ctor and never reference them again https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/views/exc... chrome/browser/ui/views/exclusive_access_bubble_views.cc:111: views::Label* after_key_; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/views/exc... chrome/browser/ui/views/exclusive_access_bubble_views.cc:139: base::string16 key = base::i18n::ToUpper(segments[1]); why is this ToUpper instead of making the source string all upper case? It's probably also worth pinging sebastien as to why his mock for the tab key is "Tab" and the mock for the escape key is "ESC". Seems inconsistent. https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/views/exc... chrome/browser/ui/views/exclusive_access_bubble_views.cc:151: views::Border::CreateRoundedRectBorder(2, 2, foreground_color)); Interesting how different this is to a very similar UI I just implemented in https://codereview.chromium.org/1416683003/ Perhaps the difference is warranted, because there is less of a vertical space constraint here. In the omnibox, we really need the key to stay a particular height, but here there may be more room to grow based on the string.
https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/views/exc... File chrome/browser/ui/views/exclusive_access_bubble_views.cc (right): https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/views/exc... chrome/browser/ui/views/exclusive_access_bubble_views.cc:139: base::string16 key = base::i18n::ToUpper(segments[1]); On 2015/10/29 00:49:53, Evan Stade wrote: > why is this ToUpper instead of making the source string all upper case? It's > probably also worth pinging sebastien as to why his mock for the tab key is > "Tab" and the mock for the escape key is "ESC". Seems inconsistent. Talked to Sebastien, he says to use "Esc" (he doesn't think it matters much that on OSX "esc" might be more natural).
https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/views/exc... File chrome/browser/ui/views/exclusive_access_bubble_views.cc (right): https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/views/exc... chrome/browser/ui/views/exclusive_access_bubble_views.cc:151: views::Border::CreateRoundedRectBorder(2, 2, foreground_color)); Do you think you could use CreateRoundedRectBorder? Or should I not be creating this? (Note: CreateRoundedRectBorder is something I am creating specifically for this, but putting into Border because it seems generic. Hasn't landed yet: https://codereview.chromium.org/1428623005/ -- comment there if you have an opinion.)
https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/exclusive... File chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc (right): https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/exclusive... chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc:172: } else { no else after return https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/exclusive... File chrome/browser/ui/exclusive_access/exclusive_access_bubble.h (right): https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/exclusive... chrome/browser/ui/exclusive_access/exclusive_access_bubble.h:93: // This string *may* contain the name of the key surrounded in pipe characters Instead of using pipe characters, consider using the variant of GetStringFUTF16 that returns the offset of the replaced parameters: https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/l10n/l10n_... and then this function can return the range of the 'esc' substring, and we won't need the separate '*sentence' resource. https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/views/exc... File chrome/browser/ui/views/exclusive_access_bubble_views.cc (right): https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/views/exc... chrome/browser/ui/views/exclusive_access_bubble_views.cc:127: int margin_vert = has_key ? kKeyNameMarginVertPx : 0; kKeyNameMarginVertPx is also zero, perhaps |margin_vert| isn't needed? https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/views/exc... chrome/browser/ui/views/exclusive_access_bubble_views.cc:147: key_name_->SetLayoutManager(key_name_layout); Why do you need a separate layout manager for the single |key_name_label| view? It seems like you should be able to adjust |layout|'s ctor spacing args to achieve what you want. (or tweak |key_name_label|'s insets/min-size/border?)
https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/views/exc... File chrome/browser/ui/views/exclusive_access_bubble_views.cc (right): https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/views/exc... chrome/browser/ui/views/exclusive_access_bubble_views.cc:151: views::Border::CreateRoundedRectBorder(2, 2, foreground_color)); On 2015/10/29 06:41:01, Matt Giuca wrote: > Do you think you could use CreateRoundedRectBorder? Or should I not be creating > this? > > (Note: CreateRoundedRectBorder is something I am creating specifically for this, > but putting into Border because it seems generic. Hasn't landed yet: > https://codereview.chromium.org/1428623005/ -- comment there if you have an > opinion.) I think I could use it, but it doesn't buy much (CreateRoundedRectBorder is hardlier easier than canvas->DrawRoundRect). I would just as soon put it at the top of this file for now until we were sure someone else wanted it (it's not particularly likely that if this code changed someone would realize there are no other uses of CreateRoundedRectBorder and delete it). Jon Ross is in the midst of creating something similar (but different) which he's putting in c/b/u/v/location_bar, not u/v/border.cc
https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/views/exc... File chrome/browser/ui/views/exclusive_access_bubble_views.cc (right): https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/views/exc... chrome/browser/ui/views/exclusive_access_bubble_views.cc:151: views::Border::CreateRoundedRectBorder(2, 2, foreground_color)); On 2015/10/29 18:36:41, Evan Stade wrote: > On 2015/10/29 06:41:01, Matt Giuca wrote: > > Do you think you could use CreateRoundedRectBorder? Or should I not be > creating > > this? > > > > (Note: CreateRoundedRectBorder is something I am creating specifically for > this, > > but putting into Border because it seems generic. Hasn't landed yet: > > https://codereview.chromium.org/1428623005/ -- comment there if you have an > > opinion.) > > I think I could use it, but it doesn't buy much (CreateRoundedRectBorder is > hardlier easier than canvas->DrawRoundRect). I would just as soon put it at the > top of this file for now until we were sure someone else wanted it (it's not > particularly likely that if this code changed someone would realize there are no > other uses of CreateRoundedRectBorder and delete it). Jon Ross is in the midst > of creating something similar (but different) which he's putting in > c/b/u/v/location_bar, not u/v/border.cc Actually I take it back, I couldn't use this border class because the exact details of the border will be different for these two keys (I'll be reusing jonross's border).
https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/views/exc... File chrome/browser/ui/views/exclusive_access_bubble_views.cc (right): https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/views/exc... chrome/browser/ui/views/exclusive_access_bubble_views.cc:139: base::string16 key = base::i18n::ToUpper(segments[1]); On 2015/10/29 00:58:36, Evan Stade wrote: > On 2015/10/29 00:49:53, Evan Stade wrote: > > why is this ToUpper instead of making the source string all upper case? Separation of presentation from content (as you would do in CSS). If we decide on a different policy decision, we don't want to have to update and re-translate all the strings. (I still haven't made up my mind about whether to re-use the existing key names or hard-code "Esc" into the string, but as the code currently does the former, that would require having separate uppercase versions of key name strings.) > Talked to Sebastien, he says to use "Esc" (he doesn't think it matters much that > on OSX "esc" might be more natural). And confirmed on private thread that we'll go with "Esc".
https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/views/exc... File chrome/browser/ui/views/exclusive_access_bubble_views.cc (right): https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/views/exc... chrome/browser/ui/views/exclusive_access_bubble_views.cc:120: std::vector<base::string16> segments = technically the style guide says not to call virtual fns in a constructor ("Avoid virtual method calls in constructors"). We break this rule a lot in existing code but I think it's not too difficult to follow in this case (just move everything here to an Init function). https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/views/exc... chrome/browser/ui/views/exclusive_access_bubble_views.cc:151: views::Border::CreateRoundedRectBorder(2, 2, foreground_color)); are you sure this thickness matches the spec? I think it's supposed to be 1dp, i.e. 1px and 1x and 2px at 2x. Mock was probably provided in 2x.
https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/exclusive... File chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc (right): https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/exclusive... chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc:172: } else { On 2015/10/29 17:50:02, msw wrote: > no else after return Done. https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/exclusive... File chrome/browser/ui/exclusive_access/exclusive_access_bubble.h (right): https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/exclusive... chrome/browser/ui/exclusive_access/exclusive_access_bubble.h:93: // This string *may* contain the name of the key surrounded in pipe characters On 2015/10/29 17:50:02, msw wrote: > Instead of using pipe characters, consider using the variant of GetStringFUTF16 > that returns the offset of the replaced parameters: > https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/l10n/l10n_... > and then this function can return the range of the 'esc' substring, The bars came about because Evan raised a concern (in a separate email thread) that we might want to avoid using substitutions here for localization reasons. However since for the time being we are using substitutions, I guess we can change this to use GetStringFUTF16 here. > and we won't need the separate '*sentence' resource. We'll still need a separate "sentence" resource here regardless, and in fact that is something I explicitly want. The old resources will be deleted once the old behaviour flag is deleted. There are other differences between the two strings -- one has a period while the other does not, and we want to be free to experiment changing the "new" string without affecting the "old" string. https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/views/exc... File chrome/browser/ui/views/exclusive_access_bubble_views.cc (right): https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/views/exc... chrome/browser/ui/views/exclusive_access_bubble_views.cc:106: SkColor background_color); On 2015/10/29 00:49:53, Evan Stade wrote: > I don't think you need to pass background_color, just turn off auto color > readability on all the labels. Not sure what the problem is here or what that change will solve (are you just trying to simplify the code?). Won't your suggestion *cause* potential contrast issues? Passing background color has two desirable effects: 1. Getting the subpixel rendering to look right for the particular background color, or disabling subpixel if the background is partially or fully transparent. 2. Inverting the text color if the contrast ratio is too low. For both of those, we should be setting auto color readability (which is on by default), AND setting the correct background color. Your suggestion would a) mean that the label is subpixel-rendered for the default theme background (that will "just happen" to be correct for when the flag is off, but not when the background is transparent), and b) will not correct for bad contrast in the system theme. So I'm not sure why you want this. > It would be an error to have a situation where fg > and bg (whether based on the theme or hardcoded) were not readable > relative to each other. It would not be an error; that is valid input to this function (if the user's system theme has a bad contrast ratio). (Note: this behaviour isn't new --- it's just been refactored from the code in ExclusiveAccessView's ctor --- so any change to the way this is done also needs to be reflected there.) https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/views/exc... chrome/browser/ui/views/exclusive_access_bubble_views.cc:109: views::Label* before_key_; On 2015/10/29 00:49:53, Evan Stade wrote: > are these members necessary? you only use them in the ctor and never reference > them again Done. https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/views/exc... chrome/browser/ui/views/exclusive_access_bubble_views.cc:111: views::Label* after_key_; On 2015/10/29 00:49:53, Evan Stade wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/views/exc... chrome/browser/ui/views/exclusive_access_bubble_views.cc:120: std::vector<base::string16> segments = On 2015/11/04 00:37:20, Evan Stade wrote: > technically the style guide says not to call virtual fns in a constructor > ("Avoid virtual method calls in constructors"). We break this rule a lot in > existing code but I think it's not too difficult to follow in this case (just > move everything here to an Init function). Which virtual method am I calling? I can't find any calls to virtual methods of views::View anywhere in this function. Instead of an Init method, I'd rather just remove the InstructionView class and make a function that returns a new View (since it no longer has any fields). But I'd only do that if there were virtual method calls here... https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/views/exc... chrome/browser/ui/views/exclusive_access_bubble_views.cc:127: int margin_vert = has_key ? kKeyNameMarginVertPx : 0; On 2015/10/29 17:50:02, msw wrote: > kKeyNameMarginVertPx is also zero, perhaps |margin_vert| isn't needed? I think it makes sense to keep this (otherwise it is equivalent to hard-coding a constant of 0; if anyone wants to change it to nonzero in the future it is extra work). I just set it to 0 for the time being. This UX is in flux and I may need to change the margins. I don't like the idea of having a configurable horizontal margin but not vertical. https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/views/exc... chrome/browser/ui/views/exclusive_access_bubble_views.cc:147: key_name_->SetLayoutManager(key_name_layout); On 2015/10/29 17:50:02, msw wrote: > Why do you need a separate layout manager for the single |key_name_label| view? > It seems like you should be able to adjust |layout|'s ctor spacing args to > achieve what you want. Essentially what I want (in HTML/CSS terms) is for key_name_layout to have a 2px border and 7px padding. Since Views doesn't have a concept of padding, this is the only way I could think of (without creating a custom view class) of getting the 7px padding *inside* the border. If I change the parent layout spacing, it will change the *margins* of this label, not the *padding*. > (or tweak |key_name_label|'s insets/min-size/border?) Another alternative (which I considered) is that we change the Border class (in this case, RoundedRectBorder) to have a concept of padding (so it produces bigger insets than what it actually draws). But it doesn't seem right to add that to RoundedRectBorder without adding it to all the other Border classes. And in general, it seems like a concept that should be added to View, not to Border. Given that Views doesn't currently have a concept of padding, this seemed like the cleanest way to do it. https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/views/exc... chrome/browser/ui/views/exclusive_access_bubble_views.cc:151: views::Border::CreateRoundedRectBorder(2, 2, foreground_color)); On 2015/11/04 00:37:20, Evan Stade wrote: > are you sure this thickness matches the spec? I think it's supposed to be 1dp, > i.e. 1px and 1x and 2px at 2x. Mock was probably provided in 2x. Maybe... I tried with 1px and 2px and 1px just looks weird (it draws 2px wide but at half opacity; I think it's some anti-aliasing thing). I'll look into that but for now, I'm not trying to match the spec, but just bang this into a basic shape resembling what people are asking for. (The text string and font size is also wrong.) Also I'm still confused about which spec I should be following --- Sebastien's spec includes the word "fullscreen" whereas the previously agreed upon text does not. Let's just get this in and then decide on specifics. I did move this to a constant.
https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/views/exc... File chrome/browser/ui/views/exclusive_access_bubble_views.cc (right): https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/views/exc... chrome/browser/ui/views/exclusive_access_bubble_views.cc:127: int margin_vert = has_key ? kKeyNameMarginVertPx : 0; On 2015/11/09 06:35:12, Matt Giuca wrote: > On 2015/10/29 17:50:02, msw wrote: > > kKeyNameMarginVertPx is also zero, perhaps |margin_vert| isn't needed? > > I think it makes sense to keep this (otherwise it is equivalent to hard-coding a > constant of 0; if anyone wants to change it to nonzero in the future it is extra > work). I just set it to 0 for the time being. This UX is in flux and I may need > to change the margins. I don't like the idea of having a configurable horizontal > margin but not vertical. The 'configurable horizontal margin' you mention is the |between_child_spacing|, where it makes sense to use a constant for the non-zero value; although it's not aptly named as |kKeyNameMarginHorizPx| and I'd actually scope all those constants to this function, since they're only used here. You pass a constant of 0 for the |inside_border_horizontal_spacing|, so why not the same for |inside_border_vertical_spacing|? Instead, you have a constant of zero and then a ternary statement switching between constant 0 and literal 0 on a |has_key| conditional, which doesn't have any tangible value, please simplify. https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/views/exc... chrome/browser/ui/views/exclusive_access_bubble_views.cc:147: key_name_->SetLayoutManager(key_name_layout); On 2015/11/09 06:35:12, Matt Giuca wrote: > On 2015/10/29 17:50:02, msw wrote: > > Why do you need a separate layout manager for the single |key_name_label| > view? > > It seems like you should be able to adjust |layout|'s ctor spacing args to > > achieve what you want. > > Essentially what I want (in HTML/CSS terms) is for key_name_layout to have a 2px > border and 7px padding. > > Since Views doesn't have a concept of padding, this is the only way I could > think of (without creating a custom view class) of getting the 7px padding > *inside* the border. > > If I change the parent layout spacing, it will change the *margins* of this > label, not the *padding*. > > > (or tweak |key_name_label|'s insets/min-size/border?) > > Another alternative (which I considered) is that we change the Border class (in > this case, RoundedRectBorder) to have a concept of padding (so it produces > bigger insets than what it actually draws). But it doesn't seem right to add > that to RoundedRectBorder without adding it to all the other Border classes. And > in general, it seems like a concept that should be added to View, not to Border. > > Given that Views doesn't currently have a concept of padding, this seemed like > the cleanest way to do it. Views have insets, which amount to padding, but they're not easily configured for instances. There may be a better way, but I guess this is fine for now.
https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/views/exc... File chrome/browser/ui/views/exclusive_access_bubble_views.cc (right): https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/views/exc... chrome/browser/ui/views/exclusive_access_bubble_views.cc:120: std::vector<base::string16> segments = On 2015/11/09 06:35:12, Matt Giuca wrote: > On 2015/11/04 00:37:20, Evan Stade wrote: > > technically the style guide says not to call virtual fns in a constructor > > ("Avoid virtual method calls in constructors"). We break this rule a lot in > > existing code but I think it's not too difficult to follow in this case (just > > move everything here to an Init function). > > Which virtual method am I calling? I can't find any calls to virtual methods of > views::View anywhere in this function. > > Instead of an Init method, I'd rather just remove the InstructionView class and > make a function that returns a new View (since it no longer has any fields). But > I'd only do that if there were virtual method calls here... ah, very true, I guess I was thinking AddChildView and SetLayoutManager were virtual. https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/views/exc... chrome/browser/ui/views/exclusive_access_bubble_views.cc:151: views::Border::CreateRoundedRectBorder(2, 2, foreground_color)); On 2015/11/09 06:35:12, Matt Giuca wrote: > On 2015/11/04 00:37:20, Evan Stade wrote: > > are you sure this thickness matches the spec? I think it's supposed to be 1dp, > > i.e. 1px and 1x and 2px at 2x. Mock was probably provided in 2x. > > Maybe... I tried with 1px and 2px and 1px just looks weird (it draws 2px wide > but at half opacity; I think it's some anti-aliasing thing). see my comment in the other CL (which imo should be folded into this CL since it's so closely related) > > I'll look into that but for now, I'm not trying to match the spec, but just bang > this into a basic shape resembling what people are asking for. (The text string > and font size is also wrong.) > > Also I'm still confused about which spec I should be following --- Sebastien's > spec includes the word "fullscreen" whereas the previously agreed upon text does > not. Let's just get this in and then decide on specifics. > > I did move this to a constant.
https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/views/exc... File chrome/browser/ui/views/exclusive_access_bubble_views.cc (right): https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/views/exc... chrome/browser/ui/views/exclusive_access_bubble_views.cc:120: std::vector<base::string16> segments = On 2015/11/09 18:51:24, Evan Stade wrote: > On 2015/11/09 06:35:12, Matt Giuca wrote: > > On 2015/11/04 00:37:20, Evan Stade wrote: > > > technically the style guide says not to call virtual fns in a constructor > > > ("Avoid virtual method calls in constructors"). We break this rule a lot in > > > existing code but I think it's not too difficult to follow in this case > (just > > > move everything here to an Init function). > > > > Which virtual method am I calling? I can't find any calls to virtual methods > of > > views::View anywhere in this function. > > > > Instead of an Init method, I'd rather just remove the InstructionView class > and > > make a function that returns a new View (since it no longer has any fields). > But > > I'd only do that if there were virtual method calls here... > > ah, very true, I guess I was thinking AddChildView and SetLayoutManager were > virtual. Acknowledged. https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/views/exc... chrome/browser/ui/views/exclusive_access_bubble_views.cc:127: int margin_vert = has_key ? kKeyNameMarginVertPx : 0; On 2015/11/09 18:28:47, msw wrote: > The 'configurable horizontal margin' you mention is the |between_child_spacing|, > where it makes sense to use a constant for the non-zero value; although it's not > aptly named as |kKeyNameMarginHorizPx| The naming is correct, from thinking about it as CSS. I am defining the horizontal and vertical margins around the KeyName (which is the middle of 3 elements in the BoxLayout). Again, because Views doesn't have a concept of padding or margin, I define the constants in a semantically correct way (kKeyNameMarginHorizPx and kKeyNameMarginVertPx) and then implement as best I can. In this case, I implement horizontal margin around the middle element of BoxLayout by setting between_child_spacing to the key name's horizontal margin. > and I'd actually scope all those > constants to this function, since they're only used here. Done. > You pass a constant of 0 for the |inside_border_horizontal_spacing|, so why not > the same for |inside_border_vertical_spacing|? Instead, you have a constant of > zero and then a ternary statement switching between constant 0 and literal 0 on > a |has_key| conditional, which doesn't have any tangible value, please simplify. I implemented the vertical margin by setting the |inside_border_vertical_spacing| of the BoxLayout to the key name's vertical margin. Except that, because there is also a padding around the entire bubble, the key name's vertical margin is 0. So I think the way I had it made sense (and allows you to customize both the horiz and vert margins), but as you say, it isn't strictly necessary, so I'm taking out the vertical margin. https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/views/exc... chrome/browser/ui/views/exclusive_access_bubble_views.cc:147: key_name_->SetLayoutManager(key_name_layout); On 2015/11/09 18:28:47, msw wrote: > On 2015/11/09 06:35:12, Matt Giuca wrote: > > On 2015/10/29 17:50:02, msw wrote: > > > Why do you need a separate layout manager for the single |key_name_label| > > view? > > > It seems like you should be able to adjust |layout|'s ctor spacing args to > > > achieve what you want. > > > > Essentially what I want (in HTML/CSS terms) is for key_name_layout to have a > 2px > > border and 7px padding. > > > > Since Views doesn't have a concept of padding, this is the only way I could > > think of (without creating a custom view class) of getting the 7px padding > > *inside* the border. > > > > If I change the parent layout spacing, it will change the *margins* of this > > label, not the *padding*. > > > > > (or tweak |key_name_label|'s insets/min-size/border?) > > > > Another alternative (which I considered) is that we change the Border class > (in > > this case, RoundedRectBorder) to have a concept of padding (so it produces > > bigger insets than what it actually draws). But it doesn't seem right to add > > that to RoundedRectBorder without adding it to all the other Border classes. > And > > in general, it seems like a concept that should be added to View, not to > Border. > > > > Given that Views doesn't currently have a concept of padding, this seemed like > > the cleanest way to do it. > > Views have insets, which amount to padding, but they're not easily configured > for instances. There may be a better way, but I guess this is fine for now. Acknowledged. https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/views/exc... chrome/browser/ui/views/exclusive_access_bubble_views.cc:151: views::Border::CreateRoundedRectBorder(2, 2, foreground_color)); On 2015/11/09 18:51:25, Evan Stade wrote: > On 2015/11/09 06:35:12, Matt Giuca wrote: > > On 2015/11/04 00:37:20, Evan Stade wrote: > > > are you sure this thickness matches the spec? I think it's supposed to be > 1dp, > > > i.e. 1px and 1x and 2px at 2x. Mock was probably provided in 2x. > > > > Maybe... I tried with 1px and 2px and 1px just looks weird (it draws 2px wide > > but at half opacity; I think it's some anti-aliasing thing). > > see my comment in the other CL (which imo should be folded into this CL since > it's so closely related) > > > > > I'll look into that but for now, I'm not trying to match the spec, but just > bang > > this into a basic shape resembling what people are asking for. (The text > string > > and font size is also wrong.) > > > > Also I'm still confused about which spec I should be following --- Sebastien's > > spec includes the word "fullscreen" whereas the previously agreed upon text > does > > not. Let's just get this in and then decide on specifics. > > > > I did move this to a constant. > OK I've fixed the underlying issue in the other CL (with anti-aliasing). But I tried again with 1px and now it just looks too thin compared to the text size. So I'll stick with 2px here; it most closely matches the mocks visually (despite not being the same numbers expressed in the mocks). Again, we can iterate on the specifics later.
lgtm with nit https://codereview.chromium.org/1421813005/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/exclusive_access_bubble_views.cc (right): https://codereview.chromium.org/1421813005/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/exclusive_access_bubble_views.cc:120: bool has_key = segments.size() == 3; nit: inline in the if statement below, or move down there.
https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/views/exc... File chrome/browser/ui/views/exclusive_access_bubble_views.cc (right): https://codereview.chromium.org/1421813005/diff/1/chrome/browser/ui/views/exc... chrome/browser/ui/views/exclusive_access_bubble_views.cc:151: views::Border::CreateRoundedRectBorder(2, 2, foreground_color)); On 2015/11/12 01:03:46, Matt Giuca wrote: > On 2015/11/09 18:51:25, Evan Stade wrote: > > On 2015/11/09 06:35:12, Matt Giuca wrote: > > > On 2015/11/04 00:37:20, Evan Stade wrote: > > > > are you sure this thickness matches the spec? I think it's supposed to be > > 1dp, > > > > i.e. 1px and 1x and 2px at 2x. Mock was probably provided in 2x. > > > > > > Maybe... I tried with 1px and 2px and 1px just looks weird (it draws 2px > wide > > > but at half opacity; I think it's some anti-aliasing thing). > > > > see my comment in the other CL (which imo should be folded into this CL since > > it's so closely related) > > > > > > > > I'll look into that but for now, I'm not trying to match the spec, but just > > bang > > > this into a basic shape resembling what people are asking for. (The text > > string > > > and font size is also wrong.) > > > > > > Also I'm still confused about which spec I should be following --- > Sebastien's > > > spec includes the word "fullscreen" whereas the previously agreed upon text > > does > > > not. Let's just get this in and then decide on specifics. > > > > > > I did move this to a constant. > > > > OK I've fixed the underlying issue in the other CL (with anti-aliasing). But I > tried again with 1px and now it just looks too thin compared to the text size. > So I'll stick with 2px here; it most closely matches the mocks visually (despite > not being the same numbers expressed in the mocks). > > Again, we can iterate on the specifics later. I don't think it does match the mocks visually. The mock you're looking at is probably the 2x mock and you're looking at it in 1x. I vote for matching the mocks and then getting design feedback from there. Either way, lgtm https://codereview.chromium.org/1421813005/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/exclusive_access_bubble_views.cc (right): https://codereview.chromium.org/1421813005/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/exclusive_access_bubble_views.cc:111: const int kKeyNamePaddingPx = 7; imo these are c-style variable declarations (everything at the top of the function) and should be avoided in favor of declaring closer to where they're actually used.
https://codereview.chromium.org/1421813005/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/exclusive_access_bubble_views.cc (right): https://codereview.chromium.org/1421813005/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/exclusive_access_bubble_views.cc:111: const int kKeyNamePaddingPx = 7; On 2015/11/12 01:27:37, Evan Stade wrote: > imo these are c-style variable declarations (everything at the top of the > function) and should be avoided in favor of declaring closer to where they're > actually used. Done. (Personally, I prefer having it at the top of the function, or preferably at the top of the file -- not for C style reasons but because then it gives future editors a nice convenient location to tweak the layout values without having to read or understand code. But whatever, let's land this.) https://codereview.chromium.org/1421813005/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/exclusive_access_bubble_views.cc:120: bool has_key = segments.size() == 3; On 2015/11/12 01:19:49, msw wrote: > nit: inline in the if statement below, or move down there. Done.
On 2015/11/12 01:27:37, Evan Stade wrote: > I don't think it does match the mocks visually. The mock you're looking at is > probably the 2x mock and you're looking at it in 1x. > > I vote for matching the mocks and then getting design feedback from there. > Either way, lgtm https://code.google.com/p/chromium/issues/detail?id=352425#c99 <- shows what it looks like with 1px border. It just looks weird, the line is too thin and the corners are very square (despite asking for 2px radius). Maybe something is screwy with the way gfx::Canvas is drawing a rounded rect, but either way, let's just leave it at 2px which looks good to me.
The CQ bit was checked by mgiuca@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org, estade@chromium.org Link to the patchset: https://codereview.chromium.org/1421813005/#ps80001 (title: "Respond to comments. Also early-return.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1421813005/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1421813005/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
On 2015/11/12 03:33:01, Matt Giuca wrote: > On 2015/11/12 01:27:37, Evan Stade wrote: > > I don't think it does match the mocks visually. The mock you're looking at is > > probably the 2x mock and you're looking at it in 1x. > > > > I vote for matching the mocks and then getting design feedback from there. > > Either way, lgtm > > https://code.google.com/p/chromium/issues/detail?id=352425#c99 <- shows what it > looks like with 1px border. It just looks weird, the line is too thin and the > corners are very square (despite asking for 2px radius). Maybe something is > screwy with the way gfx::Canvas is drawing a rounded rect, but either way, let's > just leave it at 2px which looks good to me. I agree that looks bad, but only because the corners are not round enough.
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/f838da5a288ded2f138f6d4bf3c61c8c3855ec7e Cr-Commit-Position: refs/heads/master@{#359270} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
