Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
795933009:
[AiS] for desktop, two lines and font sytles (Closed)
Description[AiS] This adds the remaining bits of the Answers in Suggest for Desktop
+ Color fonts for AiS
BUG=455418
Committed: https://crrev.com/4baa58f7573b1be55872bb30ece372c617915544
Cr-Commit-Position: refs/heads/master@{#321664}
Patch Set 1 : narrower suggest window and larger calculator results #Patch Set 2 : went back to wider search results; has icon; some answer style #Patch Set 3 : Added flags for two vs one line AiS and flag for larger fonts in AiS #Patch Set 4 : test build #Patch Set 5 : test #Patch Set 6 : merged in new code #Patch Set 7 : updated histograms #Patch Set 8 : merged from icon_wip, si2, and calculator #Patch Set 9 : fix for ios unit tests #Patch Set 10 : Merge from master, icon_wip, si3, calculator #Patch Set 11 : Merge by hand from master #Patch Set 12 : Restore baseline styles #Patch Set 13 : Merge from related branches #Patch Set 14 : Removing extra options for two line and big fonts #Patch Set 15 : Clean up of old two line and big font flags #Patch Set 16 : Cleaning up #Patch Set 17 : fix for outside vertical padding #Patch Set 18 : Positive/Negative font change #
Total comments: 45
Patch Set 19 : Colors from the UI theme #
Total comments: 1
Patch Set 20 : Increase vertical spacing in suggestions #
Total comments: 14
Patch Set 21 : Added GetAnswerLineHeight #Patch Set 22 : Changed answers colors to results table colors #Patch Set 23 : Renamed some results table vars; cl format #
Total comments: 20
Patch Set 24 : added hovered colors; adding windows positive/negative colors #Patch Set 25 : added RenderText AppendText #
Total comments: 2
Patch Set 26 : Using RGB macros for GTK colors #
Total comments: 28
Patch Set 27 : Merge from master #Patch Set 28 : Review updates #
Total comments: 6
Patch Set 29 : Added state colors struct #Patch Set 30 : Merge from master #
Total comments: 6
Patch Set 31 : Made state colors an array #Patch Set 32 : Using SK color macros #
Total comments: 8
Patch Set 33 : Removed old function declaration; fixed reference to color macros #
Total comments: 48
Patch Set 34 : Cleaned up includes; added DoAppendText #Patch Set 35 : Removed braces #
Total comments: 3
Patch Set 36 : Merged from master #Patch Set 37 : Merge from master #Patch Set 38 : merge from master #Patch Set 39 : Removed unnecessary include #
Messages
Total messages: 64 (15 generated)
dschuyler@chromium.org changed reviewers: + pkasting@chromium.org
Hi Peter, Please review this for commit. (This will also supersede the CL about adding a flag for two line answers. Fyi, this CL only makes answers two line, non-answer results are one line.)
https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc (right): https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:389: // In the current mockup, the cell heights for the result views can Nit: Remove "In the current mockup" https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:394: result_view_at(0)->get_minimum_text_vertical_padding(); So, in practice, what does this end up computing the padding as, compared with the other method? https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:398: bottom_shadow_->height() - kBorderInterior; // Bottom border. This return statement is a duplicate of the one below and should be moved below the conditional. https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:117: // Colors from the Answers in Suggest UI spec. This comment doesn't seem to tell the reader anything useful; I'd remove it and the one below. https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:118: const SkColor kColorBlack = SkColorSetRGB(0x00, 0x00, 0x00); Don't define new constants for anything that's already a Skia color constant (see SkColor.h for all the SK_ColorXXX values). Also, try to use existing Chrome UI colors (e.g. the text color, the background color, the link color, etc. all of which are already defined) where the default definitions match the spec. Where the spec gives something that doesn't already exist, we'll either want to dynamically compute it from other colors, change to a different color that does exist, or add it as (likely) a UI-wide constant, though in those cases for now it's OK to hardcode things (but this will have to be fixed before this is used by actual users). https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:132: } const kTextStyles[]{ Nit: Missing space https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:133: // 0 Zero is not used as a style type. Nit: Remove this comment, all it does is imply there ought to be a (placeholder) zeroth entry here. Also shouldn't this all be 2-space indented and not 4? https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:136: ui::ResourceBundle::LargeFont, Nit: Indent 2 more, not 1 https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:209: kColorPurple, While google.com uses purple for personalized suggestions, we've always intentionally not made them purple in the Chrome omnibox dropdown, and I suggest continuing to display them as the normal suggestion color instead of as something else. https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:213: const int kTextStyleCount = sizeof(kTextStyles) / sizeof(kTextStyles[0]); Don't do this; use arraysize(kTextStyles) below where you would use this. https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:217: return kTextStyles[(type - 1) % kTextStyleCount]; Using % here seems questionable; if the server gives us a style we don't understand, we probably want to pick a sane default style instead of using what will appear to be a random selection, or even refuse to render the suggestion entirely. https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:358: if (match_.answer) { Nit: If you reverse this conditional, you can omit the {} and save a line. https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:362: .GetFontList(ui::ResourceBundle::LargeFont) Hmm, technically rather than hardcoding LargeFont here, shouldn't we be using the font from the first answer style defined above? https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:413: int answer_icon_size = 1 + Why 1? https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:415: .GetFontList(ui::ResourceBundle::LargeFont) Same question https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:423: answer_icon_size, answer_icon_size, true); Do we really want to scale the icon to the text height? This seems likely to produce really ugly artifacts, as on different systems the font height is going to take on every integral value in a range, and it's a small enough number that surely the icon isn't going to scale well to all of those sizes. I would think we'd want to simply display the icon at the provided size and never scale. https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:725: const base::string16 spacer(base::UTF8ToUTF16(" ")); You can't concatenate strings this way; this doesn't work for i18n. You need to define localizable string constants with placeholders for the different substrings and then use methods like GetStringFUTF16() to get the complete string along with the offsets at which various substrings start. (It's also really weird to use two spaces for something, as depending on the font, the width of that could be wildly different. You probably either want one space, or some kind of number of pixels of space based on the font's n-width, or something.) This should also obviate the need for StyleAnswerText() to compute and return offsets. https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:797: NativeTheme::kColorId_ResultsTableSelectedText) This turns all the text the same color when the answer is selected. That doesn't seem right. See e.g. how the existing omnibox dropdown preserves multiple different colors in the selected rows.
jdonnelly@chromium.org changed reviewers: + jdonnelly@chromium.org
https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:209: kColorPurple, On 2015/03/12 09:46:00, Peter Kasting wrote: > While http://google.com uses purple for personalized suggestions, we've always > intentionally not made them purple in the Chrome omnibox dropdown, and I suggest > continuing to display them as the normal suggestion color instead of as > something else. Yes, I used normal suggestion color for these on mobile. https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:423: answer_icon_size, answer_icon_size, true); On 2015/03/12 09:45:59, Peter Kasting wrote: > Do we really want to scale the icon to the text height? This seems likely to > produce really ugly artifacts, as on different systems the font height is going > to take on every integral value in a range, and it's a small enough number that > surely the icon isn't going to scale well to all of those sizes. > > I would think we'd want to simply display the icon at the provided size and > never scale. The provided size is not in any way adjusted for the display in use, it's always 128x128. I've talked to the folks who built the backend code and they're open to changing this based on a client-provided spec but nobody's done this yet. On Android and iOS the scaling has worked well across a large variety of sizes and pixel densities. They're clearly not using a cheap scaling algorithm. Dave, can you try this at varying fonts to see how the scaling performs? Peter, if you have reason to believe that the scaling algorithm in Skia or whatever's responsible for this won't work well, let us know. https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:725: const base::string16 spacer(base::UTF8ToUTF16(" ")); On 2015/03/12 09:46:00, Peter Kasting wrote: > You can't concatenate strings this way; this doesn't work for i18n. > > You need to define localizable string constants with placeholders for the > different substrings and then use methods like GetStringFUTF16() to get the > complete string along with the offsets at which various substrings start. (It's > also really weird to use two spaces for something, as depending on the font, the > width of that could be wildly different. You probably either want one space, or > some kind of number of pixels of space based on the font's n-width, or > something.) > > This should also obviate the need for StyleAnswerText() to compute and return > offsets. I'm not quite sure what you're proposing here, Peter. Are you suggesting that the localizable string would be something like "$1 $2 $3"? And we'd substitute text_fields(), additional_text() and status_text()? This point of this would be to presumably reorder the three elements for RTL languages? On mobile, I handled this by simply reordering the three elements for RTL languages manually. I don't know that there's any more that translators could do that that, the three answers elements are pretty abstract. Regarding the two spaces, that's based on a hack I did on Android and iOS. I'm not proud of it, but it works reasonably well in practice. Pixel spacing is what I had in mind to add at some point. But in the meantime, you suggest one space is acceptable. That's already variable based on font, so is there an issue with two spaces for a bit of extra padding? Is your concern over users who have chosen a fixed-width font?
https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:423: answer_icon_size, answer_icon_size, true); On 2015/03/12 14:34:18, Justin Donnelly wrote: > Dave, can you try this at varying fonts to see how the scaling performs? Peter, > if you have reason to believe that the scaling algorithm in Skia or whatever's > responsible for this won't work well, let us know. I'm just concerned that this can probably take every integral value between about 16 and about 48. I would be surprised if the source icons looked good at all those sizes. I guess if that's what we have to work with, it's what we have. https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:725: const base::string16 spacer(base::UTF8ToUTF16(" ")); On 2015/03/12 14:34:18, Justin Donnelly wrote: > On 2015/03/12 09:46:00, Peter Kasting wrote: > > You can't concatenate strings this way; this doesn't work for i18n. > > > > You need to define localizable string constants with placeholders for the > > different substrings and then use methods like GetStringFUTF16() to get the > > complete string along with the offsets at which various substrings start. > (It's > > also really weird to use two spaces for something, as depending on the font, > the > > width of that could be wildly different. You probably either want one space, > or > > some kind of number of pixels of space based on the font's n-width, or > > something.) > > > > This should also obviate the need for StyleAnswerText() to compute and return > > offsets. > > I'm not quite sure what you're proposing here, Peter. Are you suggesting that > the localizable string would be something like "$1 $2 $3"? And we'd substitute > text_fields(), additional_text() and status_text()? Yes. > This point of this would be > to presumably reorder the three elements for RTL languages? That, as well as to insert or remove whitespace of various types (e.g. Thai may not use whitespace at all). > On mobile, I handled > this by simply reordering the three elements for RTL languages manually. I don't > know that there's any more that translators could do that that, the three > answers elements are pretty abstract. I think you should change what you did :) I've become very leery of assuming what localizers might do. It seems like there are always weird cases where people want to insert dashes or remove spaces or whatever. It's not particularly wonderful if the answers response is so free-form that there's literally no definable meaning to any of the strings and it's impossible to create a meaningful example string for translators. Is that what you're saying the data here looks like? > Regarding the two spaces, that's based on a hack I did on Android and iOS. I'm > not proud of it, but it works reasonably well in practice. Pixel spacing is what > I had in mind to add at some point. But in the meantime, you suggest one space > is acceptable. That's already variable based on font, so is there an issue with > two spaces for a bit of extra padding? Is your concern over users who have > chosen a fixed-width font? I'm concerned that between different fonts, the relative width of a space is not fixed, so depending on the font face chosen, what looks good on one system may look too close or too far on another system. I don't know that this is definitively wrong, it's just (as I said) weird; I've never seen us do this anywhere else.
Hi Peter, For this patch, please let me know if the method of getting the theme colors for the answers is going in the correct direction. https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc (right): https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:389: // In the current mockup, the cell heights for the result views can On 2015/03/12 09:45:59, Peter Kasting wrote: > Nit: Remove "In the current mockup" Done. https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:394: result_view_at(0)->get_minimum_text_vertical_padding(); On 2015/03/12 09:45:59, Peter Kasting wrote: > So, in practice, what does this end up computing the padding as, compared with > the other method? The padding was 3 pixels. In the other case it may vary depending on whether there is an icon present (which is common). I've changed the padding to more closely match the prior method. https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:398: bottom_shadow_->height() - kBorderInterior; // Bottom border. On 2015/03/12 09:45:59, Peter Kasting wrote: > This return statement is a duplicate of the one below and should be moved below > the conditional. Done. https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:117: // Colors from the Answers in Suggest UI spec. On 2015/03/12 09:46:00, Peter Kasting wrote: > This comment doesn't seem to tell the reader anything useful; I'd remove it and > the one below. Done. https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:118: const SkColor kColorBlack = SkColorSetRGB(0x00, 0x00, 0x00); On 2015/03/12 09:46:00, Peter Kasting wrote: > Don't define new constants for anything that's already a Skia color constant > (see SkColor.h for all the SK_ColorXXX values). > > Also, try to use existing Chrome UI colors (e.g. the text color, the background > color, the link color, etc. all of which are already defined) where the default > definitions match the spec. Where the spec gives something that doesn't already > exist, we'll either want to dynamically compute it from other colors, change to > a different color that does exist, or add it as (likely) a UI-wide constant, > though in those cases for now it's OK to hardcode things (but this will have to > be fixed before this is used by actual users). Done. https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:132: } const kTextStyles[]{ On 2015/03/12 09:46:00, Peter Kasting wrote: > Nit: Missing space Done. https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:133: // 0 Zero is not used as a style type. On 2015/03/12 09:46:00, Peter Kasting wrote: > Nit: Remove this comment, all it does is imply there ought to be a (placeholder) > zeroth entry here. > > Also shouldn't this all be 2-space indented and not 4? This is cl format. Should I report a bug in cl format? My guess is that cl format is seeing it as a continuation of the struct TextStyles line. https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:136: ui::ResourceBundle::LargeFont, On 2015/03/12 09:45:59, Peter Kasting wrote: > Nit: Indent 2 more, not 1 I can adjust this by hand. This should be already fixed in cl format, but the patch hasn't gotten out to users (us) yet. https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:209: kColorPurple, On 2015/03/12 09:46:00, Peter Kasting wrote: > While http://google.com uses purple for personalized suggestions, we've always > intentionally not made them purple in the Chrome omnibox dropdown, and I suggest > continuing to display them as the normal suggestion color instead of as > something else. Done. https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:209: kColorPurple, On 2015/03/12 14:34:18, Justin Donnelly wrote: > On 2015/03/12 09:46:00, Peter Kasting wrote: > > While http://google.com uses purple for personalized suggestions, we've always > > intentionally not made them purple in the Chrome omnibox dropdown, and I > suggest > > continuing to display them as the normal suggestion color instead of as > > something else. > > Yes, I used normal suggestion color for these on mobile. Done. https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:213: const int kTextStyleCount = sizeof(kTextStyles) / sizeof(kTextStyles[0]); On 2015/03/12 09:46:00, Peter Kasting wrote: > Don't do this; use arraysize(kTextStyles) below where you would use this. Done. https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:217: return kTextStyles[(type - 1) % kTextStyleCount]; On 2015/03/12 09:46:00, Peter Kasting wrote: > Using % here seems questionable; if the server gives us a style we don't > understand, we probably want to pick a sane default style instead of using what > will appear to be a random selection, or even refuse to render the suggestion > entirely. :) too much time focusing on performance. The modulus avoids the potential branch prediction miss. Done. https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:358: if (match_.answer) { On 2015/03/12 09:45:59, Peter Kasting wrote: > Nit: If you reverse this conditional, you can omit the {} and save a line. Done. https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:362: .GetFontList(ui::ResourceBundle::LargeFont) On 2015/03/12 09:45:59, Peter Kasting wrote: > Hmm, technically rather than hardcoding LargeFont here, shouldn't we be using > the font from the first answer style defined above? Done. https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:413: int answer_icon_size = 1 + On 2015/03/12 09:46:00, Peter Kasting wrote: > Why 1? I've added a comment. While it's hard to tell from size to side on the icon, the top to bottom clearly shows a missing line of pixels when compared to the baseline of the adjacent text. It's fairly common for a scale to rect or paint rect to draw 0 to n-1. In this case it looks better to draw 0 to N inclusive. Done. https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:415: .GetFontList(ui::ResourceBundle::LargeFont) On 2015/03/12 09:46:00, Peter Kasting wrote: > Same question Done. https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:423: answer_icon_size, answer_icon_size, true); On 2015/03/12 15:55:44, Peter Kasting wrote: > On 2015/03/12 14:34:18, Justin Donnelly wrote: > > Dave, can you try this at varying fonts to see how the scaling performs? > Peter, > > if you have reason to believe that the scaling algorithm in Skia or whatever's > > responsible for this won't work well, let us know. > > I'm just concerned that this can probably take every integral value between > about 16 and about 48. I would be surprised if the source icons looked good at > all those sizes. > > I guess if that's what we have to work with, it's what we have. For now at least, I think it's what we have to work with. Done. https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:725: const base::string16 spacer(base::UTF8ToUTF16(" ")); On 2015/03/12 15:55:44, Peter Kasting wrote: > On 2015/03/12 14:34:18, Justin Donnelly wrote: > > On 2015/03/12 09:46:00, Peter Kasting wrote: > > > You can't concatenate strings this way; this doesn't work for i18n. > > > > > > You need to define localizable string constants with placeholders for the > > > different substrings and then use methods like GetStringFUTF16() to get the > > > complete string along with the offsets at which various substrings start. > > (It's > > > also really weird to use two spaces for something, as depending on the font, > > the > > > width of that could be wildly different. You probably either want one > space, > > or > > > some kind of number of pixels of space based on the font's n-width, or > > > something.) > > > > > > This should also obviate the need for StyleAnswerText() to compute and > return > > > offsets. > > > > I'm not quite sure what you're proposing here, Peter. Are you suggesting that > > the localizable string would be something like "$1 $2 $3"? And we'd substitute > > text_fields(), additional_text() and status_text()? > > Yes. > > > This point of this would be > > to presumably reorder the three elements for RTL languages? > > That, as well as to insert or remove whitespace of various types (e.g. Thai may > not use whitespace at all). > > > On mobile, I handled > > this by simply reordering the three elements for RTL languages manually. I > don't > > know that there's any more that translators could do that that, the three > > answers elements are pretty abstract. > > I think you should change what you did :) > > I've become very leery of assuming what localizers might do. It seems like > there are always weird cases where people want to insert dashes or remove spaces > or whatever. > > It's not particularly wonderful if the answers response is so free-form that > there's literally no definable meaning to any of the strings and it's impossible > to create a meaningful example string for translators. Is that what you're > saying the data here looks like? > > > Regarding the two spaces, that's based on a hack I did on Android and iOS. I'm > > not proud of it, but it works reasonably well in practice. Pixel spacing is > what > > I had in mind to add at some point. But in the meantime, you suggest one space > > is acceptable. That's already variable based on font, so is there an issue > with > > two spaces for a bit of extra padding? Is your concern over users who have > > chosen a fixed-width font? > > I'm concerned that between different fonts, the relative width of a space is not > fixed, so depending on the font face chosen, what looks good on one system may > look too close or too far on another system. I don't know that this is > definitively wrong, it's just (as I said) weird; I've never seen us do this > anywhere else. I agree that it's weird. Justin and I have discussed RTL and i18n related to answers. So far there is not a super great direction to go with this, so we're going with 'reasonable in the interim'. While I agree with you Peter, I'd like to put this in as a bug or TODO. Something else that would be helpful is edge-to-edge justification with an expanding spacer/leading element. There is Answer UI that is supposed to be right justified in the UI rectangle the text is drawn in. In RTL I expect that may be left justified - i.e. 'away' from the other text. I'd like to make this a feature of RenderText. Overall, it seems like a topic that needs some discussion in a timely fashion, but I'd like to get this CL landed and revisit this part of it shortly afterwards. https://codereview.chromium.org/795933009/diff/360001/chrome/browser/ui/libgt... File chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc (right): https://codereview.chromium.org/795933009/diff/360001/chrome/browser/ui/libgt... chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc:454: return SkColorToGdkColor(kInvalidColorIdColor); I removed the blank line by mistake. I'll add it back.
https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:725: const base::string16 spacer(base::UTF8ToUTF16(" ")); On 2015/03/12 15:55:44, Peter Kasting wrote: > On 2015/03/12 14:34:18, Justin Donnelly wrote: > > On 2015/03/12 09:46:00, Peter Kasting wrote: > > > You can't concatenate strings this way; this doesn't work for i18n. > > > > > > You need to define localizable string constants with placeholders for the > > > different substrings and then use methods like GetStringFUTF16() to get the > > > complete string along with the offsets at which various substrings start. > > (It's > > > also really weird to use two spaces for something, as depending on the font, > > the > > > width of that could be wildly different. You probably either want one > space, > > or > > > some kind of number of pixels of space based on the font's n-width, or > > > something.) > > > > > > This should also obviate the need for StyleAnswerText() to compute and > return > > > offsets. > > > > I'm not quite sure what you're proposing here, Peter. Are you suggesting that > > the localizable string would be something like "$1 $2 $3"? And we'd substitute > > text_fields(), additional_text() and status_text()? > > Yes. > > > This point of this would be > > to presumably reorder the three elements for RTL languages? > > That, as well as to insert or remove whitespace of various types (e.g. Thai may > not use whitespace at all). > > > On mobile, I handled > > this by simply reordering the three elements for RTL languages manually. I > don't > > know that there's any more that translators could do that that, the three > > answers elements are pretty abstract. > > I think you should change what you did :) > > I've become very leery of assuming what localizers might do. It seems like > there are always weird cases where people want to insert dashes or remove spaces > or whatever. > > It's not particularly wonderful if the answers response is so free-form that > there's literally no definable meaning to any of the strings and it's impossible > to create a meaningful example string for translators. Is that what you're > saying the data here looks like? > > > Regarding the two spaces, that's based on a hack I did on Android and iOS. I'm > > not proud of it, but it works reasonably well in practice. Pixel spacing is > what > > I had in mind to add at some point. But in the meantime, you suggest one space > > is acceptable. That's already variable based on font, so is there an issue > with > > two spaces for a bit of extra padding? Is your concern over users who have > > chosen a fixed-width font? > > I'm concerned that between different fonts, the relative width of a space is not > fixed, so depending on the font face chosen, what looks good on one system may > look too close or too far on another system. I don't know that this is > definitively wrong, it's just (as I said) weird; I've never seen us do this > anywhere else. For sure, you're right to be wary of a simplistic approach to localization. But, yes, there really is very little in the way of semantics attached to these three values. The spec says things like, "Additional text with a bit of padding on the left against the main text field." The web UI at google.com puts these three values in three separate divs and then just re-orders them for RTL, so I took a similar approach in Chrome. The fact that I did it by simply concatenating them into a single string is none too sophisticated, I admit. I don't think that a localized string is the right answer (given that there's no meaningful instructions we could give the translators), but having three separate text labels positioned appropriately would be a better approach. https://codereview.chromium.org/795933009/diff/380001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/795933009/diff/380001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:126: NativeTheme::kColorId_AnswerNormalText, You've pushed some knowledge of the Answers spec into the theme layer here. I think what we should do here is define new, abstract color types where they don't already exist (specifically, Negative and Positive) and then just choose from those and the existing types in this mapping. https://codereview.chromium.org/795933009/diff/380001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:341: .GetFontList(GetTextStyle(1).font) An explanation of why 1 would be helpful. https://codereview.chromium.org/795933009/diff/380001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:394: .GetFontList(GetTextStyle(1).font) This is used in a couple places. How about GetAnswerLineHeight() or some such? https://codereview.chromium.org/795933009/diff/380001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.h (right): https://codereview.chromium.org/795933009/diff/380001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.h:13: #include "components/omnibox/suggestion_answer.h" You can just forward declare this, no? https://codereview.chromium.org/795933009/diff/380001/ui/native_theme/fallbac... File ui/native_theme/fallback_theme.cc (right): https://codereview.chromium.org/795933009/diff/380001/ui/native_theme/fallbac... ui/native_theme/fallback_theme.cc:93: static const SkColor kAnswerNormalText = I don't think it makes sense to define this as synonymous with kResultsNormalText. https://codereview.chromium.org/795933009/diff/380001/ui/native_theme/fallbac... ui/native_theme/fallback_theme.cc:94: color_utils::AlphaBlend(SK_ColorBLACK, kTextfieldDefaultBackground, 0xDD); How did you choose the alpha value? 0xDD is the most common value above but it's not clear what calculus was used (I'm guessing just what looked good). https://codereview.chromium.org/795933009/diff/380001/ui/native_theme/native_... File ui/native_theme/native_theme.h (right): https://codereview.chromium.org/795933009/diff/380001/ui/native_theme/native_... ui/native_theme/native_theme.h:317: // Answers within the chrome omnibox. nit: s/chrome omnibox/Chrome Omnibox/ More importantly, everything else here seems more abstract. From benrg's TODO below, maybe this is just because less abstract things haven't been moved here yet, but I wonder if there's a better place to define these. If this is the best/only place to define these, how about defining them in a more abstract way? (e.g. "ResultsNegativeText" rather than "AnswerNegativeText")
dschuyler@chromium.org changed reviewers: - jdonnelly@chromium.org
https://codereview.chromium.org/795933009/diff/380001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/795933009/diff/380001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:126: NativeTheme::kColorId_AnswerNormalText, On 2015/03/13 21:08:33, Justin Donnelly wrote: > You've pushed some knowledge of the Answers spec into the theme layer here. I > think what we should do here is define new, abstract color types where they > don't already exist (specifically, Negative and Positive) and then just choose > from those and the existing types in this mapping. Seems reasonable. https://codereview.chromium.org/795933009/diff/380001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:341: .GetFontList(GetTextStyle(1).font) On 2015/03/13 21:08:33, Justin Donnelly wrote: > An explanation of why 1 would be helpful. Done. https://codereview.chromium.org/795933009/diff/380001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:394: .GetFontList(GetTextStyle(1).font) On 2015/03/13 21:08:33, Justin Donnelly wrote: > This is used in a couple places. How about GetAnswerLineHeight() or some such? Done. https://codereview.chromium.org/795933009/diff/380001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.h (right): https://codereview.chromium.org/795933009/diff/380001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.h:13: #include "components/omnibox/suggestion_answer.h" On 2015/03/13 21:08:33, Justin Donnelly wrote: > You can just forward declare this, no? It's a nested class. I think that's not doable (unless something changed in the language - it didn't used to be possible). Tips on how to forward declare a nested class welcome :) https://codereview.chromium.org/795933009/diff/380001/ui/native_theme/fallbac... File ui/native_theme/fallback_theme.cc (right): https://codereview.chromium.org/795933009/diff/380001/ui/native_theme/fallbac... ui/native_theme/fallback_theme.cc:93: static const SkColor kAnswerNormalText = On 2015/03/13 21:08:33, Justin Donnelly wrote: > I don't think it makes sense to define this as synonymous with > kResultsNormalText. The reason I went with a separate list is that there are separate entries for table, result table, tree, tool tip, text field, label, menu, and so on. I would have expected many of those to use the same values. It felt like the style of the code previously was to be specific rather than general. I'm happy to change it, I just wanted to mention the thinking behind the choice. https://codereview.chromium.org/795933009/diff/380001/ui/native_theme/fallbac... ui/native_theme/fallback_theme.cc:94: color_utils::AlphaBlend(SK_ColorBLACK, kTextfieldDefaultBackground, 0xDD); On 2015/03/13 21:08:33, Justin Donnelly wrote: > How did you choose the alpha value? 0xDD is the most common value above but > it's not clear what calculus was used (I'm guessing just what looked good). Yes, I used what was used previously. I don't know what previous math was used. I'm also guessing that that it was a prior developer's taste. So another way to say this is that I didn't have a compelling/defensible reason to vary from the prior ratio used. :) https://codereview.chromium.org/795933009/diff/380001/ui/native_theme/native_... File ui/native_theme/native_theme.h (right): https://codereview.chromium.org/795933009/diff/380001/ui/native_theme/native_... ui/native_theme/native_theme.h:317: // Answers within the chrome omnibox. On 2015/03/13 21:08:33, Justin Donnelly wrote: > nit: s/chrome omnibox/Chrome Omnibox/ > > More importantly, everything else here seems more abstract. From benrg's TODO > below, maybe this is just because less abstract things haven't been moved here > yet, but I wonder if there's a better place to define these. > > If this is the best/only place to define these, how about defining them in a > more abstract way? (e.g. "ResultsNegativeText" rather than "AnswerNegativeText") The 'done' part here is the nit. I'm still working on the other bits. Done.
This version has the color names for positive and negative text blended in better with the other colors. It also makes better use of existing results colors. This version may be ready(?). Peter, please review this one for landing.
dschuyler@chromium.org changed reviewers: + estade@chromium.org, sadrul@chromium.org
Reviewers: Adding sadrul for src/ui/native_theme/... files. Adding estade for src/chorme/broaswer/ui/libgtk2ui/... file.
You need to add a native_theme_win.cc section for your new colors that behaves like the code in that file does for the existing colors (i.e. uses the system colors, passes hardcoded source colors to GetReadableColor(), etc.). Check if any of the other native_theme_xxx.cc files handle these existing colors specially. I saw a lot of opportunity to improve the existing code for these colors, but filed that as a separate bug. https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:725: const base::string16 spacer(base::UTF8ToUTF16(" ")); At the least, I think you can make this code cleaner. I don't think we should have a block of "traverse all the answer fields again to apply styles" code when traversing has so many conditionals instead of just being a loop. One way would be to add some sort of RenderText::AppendStyledText() function where you could pass in some new text and a style (and maybe color), which would let you just go through these objects once. That's kind of special-purpose, though. Another would be to keep e.g. a list of pairs of (range end position, text type) as you add pieces to the string. Then write a single loop that goes through and sets the relevant style/baseline/color for each range. This basically omits the need for StyleAnswerText(). https://codereview.chromium.org/795933009/diff/430001/chrome/browser/ui/libgt... File chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc (right): https://codereview.chromium.org/795933009/diff/430001/chrome/browser/ui/libgt... chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc:379: return color; You should use GetReadableColor() anytime you hardcode a source color, to ensure it's readable over the background. See how native_theme_win.cc does this. https://codereview.chromium.org/795933009/diff/430001/chrome/browser/ui/libgt... chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc:391: return GdkAlphaBlend(color, entry_style->base[GTK_STATE_SELECTED], 0x80); I don't think this is what you want. You're making the selected text much lower contrast by blending it with the selected background. We use that trick above to dim text, not to adjust for being selected. I think instead what you want to do is use GetReadableColor() again as suggested above, but with the selected background color instead of the normal one. https://codereview.chromium.org/795933009/diff/430001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc (right): https://codereview.chromium.org/795933009/diff/430001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:395: result_view_at(0)->get_minimum_text_vertical_padding() * 2; Good digging. After reading the code in both files more, I think we should just switch to doing this unconditionally. We can change the comment to something like what's below, but with the second paragraph replaced by a comment about how normally each row pads the top and bottom with the minimum text padding, so between two rows would be 2 * that padding. I also think we can nuke the accessor for the result view's minimum text padding, because the setter (and, indeed, the other padding setters) are never called (says code search). So let's just kill all the setters, kill the member variables, use the constants directly in that file, and make the relevant constant here public. https://codereview.chromium.org/795933009/diff/430001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/795933009/diff/430001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:192: if (type > (int)arraysize(kTextStyles)) No C-style casts. If you're going to take ints here (and in general plumb them through this whole system), do you need to check for negative values, or has the parsing code checked for those? If the latter -- or if it _could_ be the latter -- consider making the SuggestionAnswer::type filed a size_t instead, which would eliminate any need to cast here. https://codereview.chromium.org/795933009/diff/430001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:195: return kTextStyles[(type - 1)]; Nit: No {} https://codereview.chromium.org/795933009/diff/430001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:389: int answer_icon_size = 1 + GetAnswerLineHeight(); This seems wrong. If the answer line height is, for example, 10 pixels, then asking DrawImageInt to use a size of 10x10 is actually going to draw 10 pixels high. After all, we're passing a width and height, not right/bottom coordinates. Either I don't understand what you're doing, or you don't, or both :) https://codereview.chromium.org/795933009/diff/430001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:767: GetState() == SELECTED Don't you want to deal with HOVERED here too? After all, you added color constants for it. https://codereview.chromium.org/795933009/diff/430001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:777: // reference for line height. Isn't it more that (1) is used in basically every answer, and that it's intended to be the size of the whole line, with all other text either using the same font face or fitted into (1)'s height as subscript/superscript/whatever? (Also, do you actually render, say, superscript as possibly extending above the parent font height? If so, isn't this potentially wrong?)
A few other comments https://codereview.chromium.org/795933009/diff/430001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/795933009/diff/430001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:146: NativeTheme::kColorId_ResultsTableNegativeText, Your original CL used a dimmer shade of red/green for this pair. Did you want to implement something like kColorId_ResultsTableNegative[Normal,Dimmed]Text or something so you can get those shades in, or are you happy using just bright red and bright green? I think since you're having to add custom colors here anyway either is acceptable. https://codereview.chromium.org/795933009/diff/430001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:186: NativeTheme::kColorId_ResultsTableNormalDimmedText, I think you want to use the same colors as SUGGESTION_TEXT here. You may want to implement this by just shortening the style array and mapping 13 to 8 in GetTextStyle. This will ensure that if we change the suggestion style later the psuggestion style changes too.
I added append text and did more with the hovered text. https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:725: const base::string16 spacer(base::UTF8ToUTF16(" ")); On 2015/03/14 01:54:13, Peter Kasting wrote: > At the least, I think you can make this code cleaner. I don't think we should > have a block of "traverse all the answer fields again to apply styles" code when > traversing has so many conditionals instead of just being a loop. > > One way would be to add some sort of RenderText::AppendStyledText() function > where you could pass in some new text and a style (and maybe color), which would > let you just go through these objects once. That's kind of special-purpose, > though. > > Another would be to keep e.g. a list of pairs of (range end position, text type) > as you add pieces to the string. Then write a single loop that goes through and > sets the relevant style/baseline/color for each range. This basically omits the > need for StyleAnswerText(). How about AppendText, so as to be a bit more general than AppendStyledText. That still allows for setting the styles more efficiently without looping so much. That's what is now checked in for your review. Done. https://codereview.chromium.org/795933009/diff/340001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:797: NativeTheme::kColorId_ResultsTableSelectedText) On 2015/03/12 09:46:00, Peter Kasting wrote: > This turns all the text the same color when the answer is selected. That > doesn't seem right. See e.g. how the existing omnibox dropdown preserves > multiple different colors in the selected rows. Done. https://codereview.chromium.org/795933009/diff/430001/chrome/browser/ui/libgt... File chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc (right): https://codereview.chromium.org/795933009/diff/430001/chrome/browser/ui/libgt... chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc:379: return color; On 2015/03/14 01:54:13, Peter Kasting wrote: > You should use GetReadableColor() anytime you hardcode a source color, to ensure > it's readable over the background. See how native_theme_win.cc does this. Done. https://codereview.chromium.org/795933009/diff/430001/chrome/browser/ui/libgt... chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc:391: return GdkAlphaBlend(color, entry_style->base[GTK_STATE_SELECTED], 0x80); On 2015/03/14 01:54:14, Peter Kasting wrote: > I don't think this is what you want. You're making the selected text much lower > contrast by blending it with the selected background. We use that trick above > to dim text, not to adjust for being selected. > > I think instead what you want to do is use GetReadableColor() again as suggested > above, but with the selected background color instead of the normal one. Done. https://codereview.chromium.org/795933009/diff/430001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc (right): https://codereview.chromium.org/795933009/diff/430001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:395: result_view_at(0)->get_minimum_text_vertical_padding() * 2; On 2015/03/14 01:54:14, Peter Kasting wrote: > Good digging. > > After reading the code in both files more, I think we should just switch to > doing this unconditionally. We can change the comment to something like what's > below, but with the second paragraph replaced by a comment about how normally > each row pads the top and bottom with the minimum text padding, so between two > rows would be 2 * that padding. > > I also think we can nuke the accessor for the result view's minimum text > padding, because the setter (and, indeed, the other padding setters) are never > called (says code search). So let's just kill all the setters, kill the member > variables, use the constants directly in that file, and make the relevant > constant here public. Done. https://codereview.chromium.org/795933009/diff/430001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/795933009/diff/430001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:146: NativeTheme::kColorId_ResultsTableNegativeText, On 2015/03/14 02:12:25, Peter Kasting wrote: > Your original CL used a dimmer shade of red/green for this pair. Did you want > to implement something like kColorId_ResultsTableNegative[Normal,Dimmed]Text or > something so you can get those shades in, or are you happy using just bright red > and bright green? I think since you're having to add custom colors here anyway > either is acceptable. That may come up again. Like the comment about the purple text below, I'd like to keep it more simple for now. I imagine that UI may have several alterations requested after seeing the basic version of AiS for desktop. Also, I haven't found cases where the different colors are used in practice. I'd like to follow up on this after this CL. https://codereview.chromium.org/795933009/diff/430001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:186: NativeTheme::kColorId_ResultsTableNormalDimmedText, On 2015/03/14 02:12:25, Peter Kasting wrote: > I think you want to use the same colors as SUGGESTION_TEXT here. > > You may want to implement this by just shortening the style array and mapping 13 > to 8 in GetTextStyle. This will ensure that if we change the suggestion style > later the psuggestion style changes too. I changed it to be the same color as suggestion text. I'd like to make proposal that we keep the array entry because + it makes the code simpler + to the spec this color entry should be purple. We've taken the liberty of making it not purple, but design may come back with another color or re-request purple. Let's let UI weigh in before fully shorting out 13 to 8. https://codereview.chromium.org/795933009/diff/430001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:192: if (type > (int)arraysize(kTextStyles)) On 2015/03/14 01:54:14, Peter Kasting wrote: > No C-style casts. > > If you're going to take ints here (and in general plumb them through this whole > system), do you need to check for negative values, or has the parsing code > checked for those? > > If the latter -- or if it _could_ be the latter -- consider making the > SuggestionAnswer::type filed a size_t instead, which would eliminate any need to > cast here. Done. https://codereview.chromium.org/795933009/diff/430001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:195: return kTextStyles[(type - 1)]; On 2015/03/14 01:54:14, Peter Kasting wrote: > Nit: No {} I'm reading that as No () Done. https://codereview.chromium.org/795933009/diff/430001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:389: int answer_icon_size = 1 + GetAnswerLineHeight(); On 2015/03/14 01:54:14, Peter Kasting wrote: > This seems wrong. If the answer line height is, for example, 10 pixels, then > asking DrawImageInt to use a size of 10x10 is actually going to draw 10 pixels > high. After all, we're passing a width and height, not right/bottom > coordinates. > > Either I don't understand what you're doing, or you don't, or both :) After some internal reflection, I think I simply felt that it looked nice with the extra pixel line. It's not necessary, so I've removed it. Done. https://codereview.chromium.org/795933009/diff/430001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:767: GetState() == SELECTED On 2015/03/14 01:54:14, Peter Kasting wrote: > Don't you want to deal with HOVERED here too? After all, you added color > constants for it. I wasn't sure why the existing code sometimes ignored hovered. I've gone ahead and added them. Done. https://codereview.chromium.org/795933009/diff/430001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:777: // reference for line height. On 2015/03/14 01:54:14, Peter Kasting wrote: > Isn't it more that (1) is used in basically every answer, and that it's intended > to be the size of the whole line, with all other text either using the same font > face or fitted into (1)'s height as subscript/superscript/whatever? That sounds correct. I think that's saying something very similar. I'll change the wording a bit. > (Also, do you actually render, say, superscript as possibly extending above the > parent font height? If so, isn't this potentially wrong?) When adding baseline styles to RenderText I felt that would be 'proper' to add the four main baseline styles of superscript, superior, inferior, and subscript. In practice though, the answers only really use superior and inferior script styles.
https://codereview.chromium.org/795933009/diff/460001/chrome/browser/ui/libgt... File chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc (right): https://codereview.chromium.org/795933009/diff/460001/chrome/browser/ui/libgt... chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc:383: GdkColor color = {0}; use GDK_COLOR_RGB here and elsewhere. GdkColor color = {0}; color.green = ~0; is not easy to read
dschuyler@chromium.org changed reviewers: - pkasting@chromium.org, sadrul@chromium.org
Hi Evan, this version is now using the RGB macro for the color. https://codereview.chromium.org/795933009/diff/460001/chrome/browser/ui/libgt... File chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc (right): https://codereview.chromium.org/795933009/diff/460001/chrome/browser/ui/libgt... chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc:383: GdkColor color = {0}; On 2015/03/16 23:39:27, Evan Stade wrote: > use GDK_COLOR_RGB here and elsewhere. > > GdkColor color = {0}; > color.green = ~0; > > is not easy to read Done.
libgtk lgtm for some reason you removed the other reviewers
dschuyler@chromium.org changed reviewers: + pkasting@chromium.org, sadrul@chromium.org
Hmm, in my head I was sending that patch to just Evan for review. I see now that it looks weird to remove the others reviewers. I'm adding pkasting and sadrul back now.
https://codereview.chromium.org/795933009/diff/480001/chrome/browser/ui/libgt... File chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc (right): https://codereview.chromium.org/795933009/diff/480001/chrome/browser/ui/libgt... chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc:26: const GdkColor kPositiveTextColor = GDK_COLOR_RGB(0x00, 0xff, 0x00); Nit: kURLTextColor is used in multiple functions and thus has to be declared up here, but since your new colors are only used in one, declare them in that narrower scope. https://codereview.chromium.org/795933009/diff/480001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc (right): https://codereview.chromium.org/795933009/diff/480001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:396: OmniboxResultView::kMinimumTextVerticalPadding * 2; Man, the more times I look at this code the more I find was wrong :P. It turns out the * 2 is a mistake. If we want the space between e.g. the bottom row and the bottom border to match, we should just be using kMinimumVerticalTextPadding directly. This is because we'll already have one copy of that padding present in the bottom row, so we just need one more. I verified on a Chrome screenshot that this would look more correct. So what I would do is eliminate this member variable entirely, and use OmniboxResultView::kMinimumTextVerticalPadding directly everywhere we currently use |outside_vertical_padding_|. https://codereview.chromium.org/795933009/diff/480001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/795933009/diff/480001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:45: NativeTheme::ColorId color; Here's an idea: Instead of three member ColorIds, declare an array: NativeTheme::ColorId colors[OmniboxResultView::NUM_STATES]; Now you don't need a GetStateColor() method, because AppendAnswerText() can just do: render_text->ApplyColor(text_style.colors[GetState()], range); Then, since GetStateColor() was the only method taking a TextStyle, the header no longer needs to forward-declare TextStyle, and the definition of TextStyle here can move back into the anonymous namespace as you had it previously. https://codereview.chromium.org/795933009/diff/480001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:752: // GetTextStyle(1) is the largest fonts used and so set the boundary that all Nit: "....largest font used and so defines the..." https://codereview.chromium.org/795933009/diff/480001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:756: .GetHeight(); Nit: Can the result value here change at runtime, e.g. due to the user changing their system default font or theme or something? If not, you could cache this in a static local to avoid computing it on every call. https://codereview.chromium.org/795933009/diff/480001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:769: GetNativeTheme()->GetSystemColor(text_style.color_hovered); Missing return here! https://codereview.chromium.org/795933009/diff/480001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:782: render_text->SetFontList( Doesn't this change the font for the whole RenderText object? Can RenderText even support multiple fonts? If not maybe we need to use different RenderText objects for the different pieces here. https://codereview.chromium.org/795933009/diff/480001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.h (right): https://codereview.chromium.org/795933009/diff/480001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.h:55: kMinimumIconVerticalPadding = 2, The icon padding should still be a file-scoped constant in the .cc file because no one else needs to access it. https://codereview.chromium.org/795933009/diff/480001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.h:56: kMinimumTextVerticalPadding = 3, Don't use the "enum hack" to declare constants like this; declare this as a const int instead. (We're compiling in C++11 mode, so that ought to work everywhere.) https://codereview.chromium.org/795933009/diff/480001/ui/native_theme/fallbac... File ui/native_theme/fallback_theme.cc (right): https://codereview.chromium.org/795933009/diff/480001/ui/native_theme/fallbac... ui/native_theme/fallback_theme.cc:93: color_utils::GetReadableColor(SK_ColorGREEN, kTextfieldDefaultBackground); Since all the colors in the fallback theme are hardcoded, there's no need to use GetReadableColor() here. Instead, I would copy the way the "normal text" and the like constants above do things, so: static const SkColor kResultsTablePositiveText = color_utils::AlphaBlend( SK_ColorGREEN, kTextfieldDefaultBackground, 0xDD); ... https://codereview.chromium.org/795933009/diff/480001/ui/native_theme/native_... File ui/native_theme/native_theme.h (right): https://codereview.chromium.org/795933009/diff/480001/ui/native_theme/native_... ui/native_theme/native_theme.h:301: // Results Tables, such as the Chrome Omnibox. Nit: "omnibox" is not normally capitalized. I'd just omit the word "Chrome" here since we assume everything in Chromium is for Chrome :) https://codereview.chromium.org/795933009/diff/480001/ui/native_theme/native_... File ui/native_theme/native_theme_win.cc (right): https://codereview.chromium.org/795933009/diff/480001/ui/native_theme/native_... ui/native_theme/native_theme_win.cc:646: return color_utils::GetReadableColor(SkColorSetRGB(0, 255, 0), Nit: Use SK_ColorGREEN here (and RED below). These are in SkColor.h, which is likely already included. https://codereview.chromium.org/795933009/diff/480001/ui/native_theme/native_... ui/native_theme/native_theme_win.cc:647: system_colors_[COLOR_WINDOW]); The "Hovered" text states should be computed atop GetSystemColor(kColorId_ResultsTableHoveredBackground) rather than system_colors_[COLOR_WINDOW].
Hi Peter, there are a couple places I've made a TODO comment rather than try to make all the changes suggested. If that's cool with you, please review this patch for landing :) https://codereview.chromium.org/795933009/diff/480001/chrome/browser/ui/libgt... File chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc (right): https://codereview.chromium.org/795933009/diff/480001/chrome/browser/ui/libgt... chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc:26: const GdkColor kPositiveTextColor = GDK_COLOR_RGB(0x00, 0xff, 0x00); On 2015/03/17 22:01:44, Peter Kasting wrote: > Nit: kURLTextColor is used in multiple functions and thus has to be declared up > here, but since your new colors are only used in one, declare them in that > narrower scope. Done. https://codereview.chromium.org/795933009/diff/480001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc (right): https://codereview.chromium.org/795933009/diff/480001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:396: OmniboxResultView::kMinimumTextVerticalPadding * 2; On 2015/03/17 22:01:44, Peter Kasting wrote: > Man, the more times I look at this code the more I find was wrong :P. > > It turns out the * 2 is a mistake. If we want the space between e.g. the bottom > row and the bottom border to match, we should just be using > kMinimumVerticalTextPadding directly. This is because we'll already have one > copy of that padding present in the bottom row, so we just need one more. I > verified on a Chrome screenshot that this would look more correct. > > So what I would do is eliminate this member variable entirely, and use > OmniboxResultView::kMinimumTextVerticalPadding directly everywhere we currently > use |outside_vertical_padding_|. It's mostly an aesthetic choice, so all the changes have been variations of right (not wrong) :) Done. https://codereview.chromium.org/795933009/diff/480001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/795933009/diff/480001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:45: NativeTheme::ColorId color; On 2015/03/17 22:01:44, Peter Kasting wrote: > Here's an idea: > > Instead of three member ColorIds, declare an array: > > NativeTheme::ColorId colors[OmniboxResultView::NUM_STATES]; > > Now you don't need a GetStateColor() method, because AppendAnswerText() can just > do: > > render_text->ApplyColor(text_style.colors[GetState()], range); > > Then, since GetStateColor() was the only method taking a TextStyle, the header > no longer needs to forward-declare TextStyle, and the definition of TextStyle > here can move back into the anonymous namespace as you had it previously. I've added a TODO aobut this. https://codereview.chromium.org/795933009/diff/480001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:752: // GetTextStyle(1) is the largest fonts used and so set the boundary that all On 2015/03/17 22:01:44, Peter Kasting wrote: > Nit: "....largest font used and so defines the..." Done. https://codereview.chromium.org/795933009/diff/480001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:756: .GetHeight(); On 2015/03/17 22:01:44, Peter Kasting wrote: > Nit: Can the result value here change at runtime, e.g. due to the user changing > their system default font or theme or something? If not, you could cache this > in a static local to avoid computing it on every call. I don't know if that varies by platform. Can you recommend how I can find out? I also don't know how much the performance may vary for this call as is on different platforms. Is it better optimize now or get the basics working first? (I know it's a common rule of thumb to optimize later, but it can depend on the circumstances). https://codereview.chromium.org/795933009/diff/480001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:769: GetNativeTheme()->GetSystemColor(text_style.color_hovered); On 2015/03/17 22:01:44, Peter Kasting wrote: > Missing return here! ! Done. https://codereview.chromium.org/795933009/diff/480001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:782: render_text->SetFontList( On 2015/03/17 22:01:44, Peter Kasting wrote: > Doesn't this change the font for the whole RenderText object? > > Can RenderText even support multiple fonts? If not maybe we need to use > different RenderText objects for the different pieces here. Thanks for pointing that out. I added a TODO to follow up on this. It's not currently showing issues, but it could in the future. So it will be good to follow up on. https://codereview.chromium.org/795933009/diff/480001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.h (right): https://codereview.chromium.org/795933009/diff/480001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.h:55: kMinimumIconVerticalPadding = 2, On 2015/03/17 22:01:44, Peter Kasting wrote: > The icon padding should still be a file-scoped constant in the .cc file because > no one else needs to access it. Done. https://codereview.chromium.org/795933009/diff/480001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.h:56: kMinimumTextVerticalPadding = 3, On 2015/03/17 22:01:44, Peter Kasting wrote: > Don't use the "enum hack" to declare constants like this; declare this as a > const int instead. (We're compiling in C++11 mode, so that ought to work > everywhere.) Done. https://codereview.chromium.org/795933009/diff/480001/ui/native_theme/fallbac... File ui/native_theme/fallback_theme.cc (right): https://codereview.chromium.org/795933009/diff/480001/ui/native_theme/fallbac... ui/native_theme/fallback_theme.cc:93: color_utils::GetReadableColor(SK_ColorGREEN, kTextfieldDefaultBackground); On 2015/03/17 22:01:44, Peter Kasting wrote: > Since all the colors in the fallback theme are hardcoded, there's no need to use > GetReadableColor() here. Instead, I would copy the way the "normal text" and > the like constants above do things, so: > > static const SkColor kResultsTablePositiveText = color_utils::AlphaBlend( > SK_ColorGREEN, kTextfieldDefaultBackground, 0xDD); > ... Done. https://codereview.chromium.org/795933009/diff/480001/ui/native_theme/native_... File ui/native_theme/native_theme.h (right): https://codereview.chromium.org/795933009/diff/480001/ui/native_theme/native_... ui/native_theme/native_theme.h:301: // Results Tables, such as the Chrome Omnibox. On 2015/03/17 22:01:45, Peter Kasting wrote: > Nit: "omnibox" is not normally capitalized. > > I'd just omit the word "Chrome" here since we assume everything in Chromium is > for Chrome :) Done. https://codereview.chromium.org/795933009/diff/480001/ui/native_theme/native_... File ui/native_theme/native_theme_win.cc (right): https://codereview.chromium.org/795933009/diff/480001/ui/native_theme/native_... ui/native_theme/native_theme_win.cc:646: return color_utils::GetReadableColor(SkColorSetRGB(0, 255, 0), On 2015/03/17 22:01:45, Peter Kasting wrote: > Nit: Use SK_ColorGREEN here (and RED below). These are in SkColor.h, which is > likely already included. Done. https://codereview.chromium.org/795933009/diff/480001/ui/native_theme/native_... ui/native_theme/native_theme_win.cc:647: system_colors_[COLOR_WINDOW]); On 2015/03/17 22:01:45, Peter Kasting wrote: > The "Hovered" text states should be computed atop > GetSystemColor(kColorId_ResultsTableHoveredBackground) rather than > system_colors_[COLOR_WINDOW]. Done.
https://codereview.chromium.org/795933009/diff/480001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/795933009/diff/480001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:756: .GetHeight(); On 2015/03/17 22:57:55, dschuyler wrote: > On 2015/03/17 22:01:44, Peter Kasting wrote: > > Nit: Can the result value here change at runtime, e.g. due to the user > changing > > their system default font or theme or something? If not, you could cache this > > in a static local to avoid computing it on every call. > > I don't know if that varies by platform. Can you recommend how I can find out? The only ways I know of are trying to do code inspection and trying to test Chrome to see what it does with system theme changes. The problem with the latter is that we probably get it wrong today, as we have a variety of bugs on file about not respecting theme changes. I did some code inspection. I think the fonts can potentially change at runtime on ChromeOS at least, so I would leave the code as-is. > I also don't know how much the performance may vary for this call as is on > different platforms. Is it better optimize now or get the basics working first? > (I know it's a common rule of thumb to optimize later, but it can depend on the > circumstances). The reason I suggested an optimization here is that it would be easy and you call this function very frequently, and we've heard a lot of complaints about omnibox rendering doing unnecessary work in the past. Examining the code that implements this, it's not very expensive, so I'm not worried. https://codereview.chromium.org/795933009/diff/520001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc (right): https://codereview.chromium.org/795933009/diff/520001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:72: outside_vertical_padding_(0) { I meant for you to entirely remove this member variable, not just remove its use in one of the functions. As you are no longer setting it, code that uses it is now wrong. https://codereview.chromium.org/795933009/diff/520001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/795933009/diff/520001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:44: // TODO(dschuyler): Look into moving this back into the unnamed namespace. I'm not sure why you TODOd this when I had a very concrete suggestion that should have only taken a couple minutes to implement, and the TODO is vague enough to make it unclear what the way to do this would be. https://codereview.chromium.org/795933009/diff/520001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:790: // for a sub-range within RenderText. You said this is "not currently showing issues". Does that mean that we never actually get an answer that uses more than one font size in its markup? Or does that mean that this somehow manages to work as-is in that scenario?
https://codereview.chromium.org/795933009/diff/480001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/795933009/diff/480001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:756: .GetHeight(); On 2015/03/17 23:18:06, Peter Kasting wrote: > On 2015/03/17 22:57:55, dschuyler wrote: > > On 2015/03/17 22:01:44, Peter Kasting wrote: > > > Nit: Can the result value here change at runtime, e.g. due to the user > > changing > > > their system default font or theme or something? If not, you could cache > this > > > in a static local to avoid computing it on every call. > > > > I don't know if that varies by platform. Can you recommend how I can find > out? > > The only ways I know of are trying to do code inspection and trying to test > Chrome to see what it does with system theme changes. The problem with the > latter is that we probably get it wrong today, as we have a variety of bugs on > file about not respecting theme changes. > > I did some code inspection. I think the fonts can potentially change at runtime > on ChromeOS at least, so I would leave the code as-is. > > > I also don't know how much the performance may vary for this call as is on > > different platforms. Is it better optimize now or get the basics working > first? > > (I know it's a common rule of thumb to optimize later, but it can depend on > the > > circumstances). > > The reason I suggested an optimization here is that it would be easy and you > call this function very frequently, and we've heard a lot of complaints about > omnibox rendering doing unnecessary work in the past. > > Examining the code that implements this, it's not very expensive, so I'm not > worried. Thanks for checking into that. https://codereview.chromium.org/795933009/diff/520001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc (right): https://codereview.chromium.org/795933009/diff/520001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:72: outside_vertical_padding_(0) { On 2015/03/17 23:18:06, Peter Kasting wrote: > I meant for you to entirely remove this member variable, not just remove its use > in one of the functions. As you are no longer setting it, code that uses it is > now wrong. Sorry, I hurried. Done. https://codereview.chromium.org/795933009/diff/520001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/795933009/diff/520001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:44: // TODO(dschuyler): Look into moving this back into the unnamed namespace. On 2015/03/17 23:18:06, Peter Kasting wrote: > I'm not sure why you TODOd this when I had a very concrete suggestion that > should have only taken a couple minutes to implement, and the TODO is vague > enough to make it unclear what the way to do this would be. I didn't mean to TODO it indefinitely. Only that it can be done later today or tomorrow. Done. https://codereview.chromium.org/795933009/diff/520001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:790: // for a sub-range within RenderText. On 2015/03/17 23:18:06, Peter Kasting wrote: > You said this is "not currently showing issues". Does that mean that we never > actually get an answer that uses more than one font size in its markup? Or does > that mean that this somehow manages to work as-is in that scenario? The UI font info seems to cover more text options than I've currently used in practice. The size variations that are there are done as baseline styles. For example, the "degrees Fahrenheit" are superior script of the large font size; and the "stock quote delta" is the inferior script of the large font size. There are some font sizes that wouldn't work as a baseline style of the large font, but the design is still a work-in-progress from what I understand.
https://codereview.chromium.org/795933009/diff/560001/chrome/browser/ui/libgt... File chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc (right): https://codereview.chromium.org/795933009/diff/560001/chrome/browser/ui/libgt... chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc:218: const GdkColor kNegativeTextColor = GDK_COLOR_RGB(0xff, 0x00, 0x00); I didn't notice this before, but: You can actually avoid these constants entirely and use the Skia variants (SK_Color[GREEN,RED]) directly below, and simply remove the first GdkColorToSkColor() call inside your GetReadableColor helper above. This makes the code both slightly simpler and slightly more efficient. https://codereview.chromium.org/795933009/diff/560001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/795933009/diff/560001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:786: // for a sub-range within RenderText. I didn't completely understand your comments about this. Were you saying that, today, we only get style combinations from the server that result in the same font size being used across all styles within a single RenderText? If so, then this TODO is probably OK, but instead of specifically prescribing a "set font size for sub-range" mechanism, I'd simply say that you want to address the problem of different font sizes within one RenderText. This leaves the door open to e.g. use multiple RenderTexts instead of making RenderText more complex. If you were saying something else, then I didn't follow. https://codereview.chromium.org/795933009/diff/560001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.h (right): https://codereview.chromium.org/795933009/diff/560001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.h:52: struct StateColors { Why did you choose to do this with a struct rather than an array? The array made things shorter and simpler, but this struct makes the code more complex, because now you still have GetStateColor(), but you also have nested structs. Basically the code got strictly worse instead of improving :(
https://codereview.chromium.org/795933009/diff/560001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.h (right): https://codereview.chromium.org/795933009/diff/560001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.h:52: struct StateColors { On 2015/03/18 00:36:19, Peter Kasting wrote: > Why did you choose to do this with a struct rather than an array? The array > made things shorter and simpler, but this struct makes the code more complex, > because now you still have GetStateColor(), but you also have nested structs. > Basically the code got strictly worse instead of improving :( Is this patch more like what you were hoping for? Done.
https://codereview.chromium.org/795933009/diff/560001/chrome/browser/ui/libgt... File chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc (right): https://codereview.chromium.org/795933009/diff/560001/chrome/browser/ui/libgt... chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc:218: const GdkColor kNegativeTextColor = GDK_COLOR_RGB(0xff, 0x00, 0x00); On 2015/03/18 00:36:19, Peter Kasting wrote: > I didn't notice this before, but: > > You can actually avoid these constants entirely and use the Skia variants > (SK_Color[GREEN,RED]) directly below, and simply remove the first > GdkColorToSkColor() call inside your GetReadableColor helper above. > > This makes the code both slightly simpler and slightly more efficient. Done. https://codereview.chromium.org/795933009/diff/560001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/795933009/diff/560001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:786: // for a sub-range within RenderText. On 2015/03/18 00:36:19, Peter Kasting wrote: > I didn't completely understand your comments about this. > > Were you saying that, today, we only get style combinations from the server that > result in the same font size being used across all styles within a single > RenderText? > > If so, then this TODO is probably OK, but instead of specifically prescribing a > "set font size for sub-range" mechanism, I'd simply say that you want to address > the problem of different font sizes within one RenderText. This leaves the door > open to e.g. use multiple RenderTexts instead of making RenderText more complex. > > If you were saying something else, then I didn't follow. Yeah. For this specific CL, I believe the TODO is a good choice. It feels like what to do with RenderText could be a separate discussion.
LGTM https://codereview.chromium.org/795933009/diff/600001/chrome/browser/ui/libgt... File chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc (right): https://codereview.chromium.org/795933009/diff/600001/chrome/browser/ui/libgt... chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc:97: GdkColor GetReadableColor(const SkColor& color, const GdkColor& background) { Nit: Pass SkColor by value as it's a uint32_t. https://codereview.chromium.org/795933009/diff/600001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/795933009/diff/600001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:774: // for a sub-range within RenderText. Nit: I still think you should change the text of this TODO as I suggested earlier as it seems too specific. https://codereview.chromium.org/795933009/diff/600001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.h (right): https://codereview.chromium.org/795933009/diff/600001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.h:52: typedef ui::NativeTheme::ColorId StateColors[NUM_STATES]; This typedef doesn't need to be here since no one outside the .cc file uses this. It's also only used once in the .cc itself, so I would just declare that member as being this type directly, and eliminate this. https://codereview.chromium.org/795933009/diff/600001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.h:163: int StyleAnswerText(gfx::RenderText* render_text, Nit: I suggest ordering these so that the two XXXAnswerText() functions are together, and all three GetXXX() functions are together. It's also strange that the existing declarations of this function insert some non-overrides into the middle of the "View:" overrides list, but you don't need to fix that while you're here (would be nice to do separately though).
sadrul@chromium.org changed reviewers: + msw@chromium.org
+msw@ for RenderText changes.
https://codereview.chromium.org/795933009/diff/600001/chrome/browser/ui/libgt... File chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc (right): https://codereview.chromium.org/795933009/diff/600001/chrome/browser/ui/libgt... chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc:97: GdkColor GetReadableColor(const SkColor& color, const GdkColor& background) { On 2015/03/18 02:10:35, Peter Kasting wrote: > Nit: Pass SkColor by value as it's a uint32_t. Done. https://codereview.chromium.org/795933009/diff/600001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/795933009/diff/600001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:774: // for a sub-range within RenderText. On 2015/03/18 02:10:35, Peter Kasting wrote: > Nit: I still think you should change the text of this TODO as I suggested > earlier as it seems too specific. Done. https://codereview.chromium.org/795933009/diff/600001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.h (right): https://codereview.chromium.org/795933009/diff/600001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.h:52: typedef ui::NativeTheme::ColorId StateColors[NUM_STATES]; On 2015/03/18 02:10:35, Peter Kasting wrote: > This typedef doesn't need to be here since no one outside the .cc file uses > this. > > It's also only used once in the .cc itself, so I would just declare that member > as being this type directly, and eliminate this. Done. https://codereview.chromium.org/795933009/diff/600001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.h:163: int StyleAnswerText(gfx::RenderText* render_text, On 2015/03/18 02:10:35, Peter Kasting wrote: > Nit: I suggest ordering these so that the two XXXAnswerText() functions are > together, and all three GetXXX() functions are together. > > It's also strange that the existing declarations of this function insert some > non-overrides into the middle of the "View:" overrides list, but you don't need > to fix that while you're here (would be nice to do separately though). One of the answer text function declarations was residue from a prior patch in this CL. I just needed to be removed. Done.
On 2015/03/18 03:59:52, dschuyler wrote: > https://codereview.chromium.org/795933009/diff/600001/chrome/browser/ui/libgt... > File chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc (right): > > https://codereview.chromium.org/795933009/diff/600001/chrome/browser/ui/libgt... > chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc:97: GdkColor > GetReadableColor(const SkColor& color, const GdkColor& background) { > On 2015/03/18 02:10:35, Peter Kasting wrote: > > Nit: Pass SkColor by value as it's a uint32_t. > > Done. > > https://codereview.chromium.org/795933009/diff/600001/chrome/browser/ui/views... > File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): > > https://codereview.chromium.org/795933009/diff/600001/chrome/browser/ui/views... > chrome/browser/ui/views/omnibox/omnibox_result_view.cc:774: // for a sub-range > within RenderText. > On 2015/03/18 02:10:35, Peter Kasting wrote: > > Nit: I still think you should change the text of this TODO as I suggested > > earlier as it seems too specific. > > Done. > > https://codereview.chromium.org/795933009/diff/600001/chrome/browser/ui/views... > File chrome/browser/ui/views/omnibox/omnibox_result_view.h (right): > > https://codereview.chromium.org/795933009/diff/600001/chrome/browser/ui/views... > chrome/browser/ui/views/omnibox/omnibox_result_view.h:52: typedef > ui::NativeTheme::ColorId StateColors[NUM_STATES]; > On 2015/03/18 02:10:35, Peter Kasting wrote: > > This typedef doesn't need to be here since no one outside the .cc file uses > > this. > > > > It's also only used once in the .cc itself, so I would just declare that > member > > as being this type directly, and eliminate this. > > Done. > > https://codereview.chromium.org/795933009/diff/600001/chrome/browser/ui/views... > chrome/browser/ui/views/omnibox/omnibox_result_view.h:163: int > StyleAnswerText(gfx::RenderText* render_text, > On 2015/03/18 02:10:35, Peter Kasting wrote: > > Nit: I suggest ordering these so that the two XXXAnswerText() functions are > > together, and all three GetXXX() functions are together. > > > > It's also strange that the existing declarations of this function insert some > > non-overrides into the middle of the "View:" overrides list, but you don't > need > > to fix that while you're here (would be nice to do separately though). > > One of the answer text function declarations was residue from a prior patch in > this CL. I just needed to be removed. > Done. Ehem, s/I just needed/It just needed/
msw@chromium.org changed reviewers: + oshima@chromium.org
https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/libgt... File chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc (right): https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/libgt... chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc:382: case kColorId_ResultsTablePositiveText: { Colors have different meanings in other cultures. Is there any precedent for selecting locale-based colors? Do UI/UX folks support using hard-coded green and red as the base for good and bad? https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/libgt... chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc:383: return GetReadableColor(SK_ColorGREEN, Should the positive/negative base colors have their own themed ids? (or perhaps at least a hard-coded const value in their files) https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc (right): https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:14: #include "components/omnibox/omnibox_field_trial.h" Why is this needed? https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:122: // 1 ANSWER_TEXT Should we have an enum somewhere with these types? https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:204: type = 1; Should an invalid type really default to ANSWER_TEXT with its LargeFont instead of something like SUGGESTION_TEXT with the base font, colors and baseline? https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:753: int OmniboxResultView::GetAnswerLineHeight() const { This may be called a lot, should the result be cached? https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:767: gfx::RenderText* render_text, Why pass in the RenderText? The callers always use description_rendertext_.get() https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:774: // one RenderText nit: trailing period. https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.h (right): https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.h:19: #include "ui/native_theme/native_theme.h" Why is this needed here? https://codereview.chromium.org/795933009/diff/620001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/795933009/diff/620001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:455: for (size_t style = 0; style < NUM_TEXT_STYLES; ++style) { nit: inline this for a one-liner and remove loop curly-braces. for (size_t style = 0; style < NUM_TEXT_STYLES; ++style) styles_[style].SetValue(styles_[style].breaks().begin()->second) https://codereview.chromium.org/795933009/diff/620001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:476: BreakList<bool>& break_list = styles_[style]; ditto nit: inline this for a one-liner and remove loop curly-braces. for (size_t style = 0; style < NUM_TEXT_STYLES; ++style) styles_[style].SetMax(text_length) https://codereview.chromium.org/795933009/diff/620001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:481: OnTextAttributeChanged(); +CC oshima... I think that calling this in the middle of SetText, before the styles have been finalized, is insufficient. You'll probably need to call this at the end of SetText, but calling it twice per SetText is likely to cause a performance regression. I suppose you'll need to have a private AppendText[Impl|Internal] (or something) that SetText and AppendText can call that does everything above, except call OnTextAttributeChanged() (and perhaps also modify cached_bounds_and_offset_valid_ and obscured_reveal_index_), then have the two public functions do that separately. https://codereview.chromium.org/795933009/diff/620001/ui/native_theme/fallbac... File ui/native_theme/fallback_theme.cc (right): https://codereview.chromium.org/795933009/diff/620001/ui/native_theme/fallbac... ui/native_theme/fallback_theme.cc:93: color_utils::AlphaBlend(SK_ColorGREEN, kTextfieldDefaultBackground, 0xDD); ditto nits about red/green locale values and file-local constants. https://codereview.chromium.org/795933009/diff/620001/ui/native_theme/native_... File ui/native_theme/native_theme_win.cc (right): https://codereview.chromium.org/795933009/diff/620001/ui/native_theme/native_... ui/native_theme/native_theme_win.cc:613: return color_utils::GetReadableColor( Should the other kColorId_ResultsTable*Text (Normal, Selected, etc.) entries use GetReadableColor with their respective backgrounds? https://codereview.chromium.org/795933009/diff/620001/ui/native_theme/native_... ui/native_theme/native_theme_win.cc:649: return color_utils::GetReadableColor(SK_ColorGREEN, ditto nits about red/green locale values and file-local constants.
Also, can we get some screenshots?
Review response for msw. https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/libgt... File chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc (right): https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/libgt... chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc:382: case kColorId_ResultsTablePositiveText: { On 2015/03/18 18:03:36, msw wrote: > Colors have different meanings in other cultures. Is there any precedent for > selecting locale-based colors? Do UI/UX folks support using hard-coded green and > red as the base for good and bad? Excellent point. So far the UI doc says red and green, but I agree it would be worth bringing up the localization question specifically. Should I add a TODO or enter a bug or email someone in particular about it? https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/libgt... chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc:383: return GetReadableColor(SK_ColorGREEN, On 2015/03/18 18:03:36, msw wrote: > Should the positive/negative base colors have their own themed ids? > (or perhaps at least a hard-coded const value in their files) I'm trying to understand what you mean - like as a data driven resource file entry for the colors? That sounds good. I could use some help/tips on how to follow up on that. Should this get a TODO or bug or similar? https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc (right): https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:14: #include "components/omnibox/omnibox_field_trial.h" On 2015/03/18 18:03:36, msw wrote: > Why is this needed? It is not needed. This CL is old and has gone through many changes. This was a holdover from a prior patch. Removed. Done. https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:122: // 1 ANSWER_TEXT On 2015/03/18 18:03:37, msw wrote: > Should we have an enum somewhere with these types? There may be one in the server code. It wouldn't be used in the client though. I'm open to adding the enum or leaving it out. I can see it being a form of documentation. https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:204: type = 1; On 2015/03/18 18:03:37, msw wrote: > Should an invalid type really default to ANSWER_TEXT with its LargeFont instead > of something like SUGGESTION_TEXT with the base font, colors and baseline? It's an excellent question. I also thought of making it the negative text so it would look more like an error. Type 1 seems like it would look less like an error. I'm open as to what type an invalid type should switch to. I'm pretty sure we should check the type though, since it's input form an outside source at runtime. https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:753: int OmniboxResultView::GetAnswerLineHeight() const { On 2015/03/18 18:03:37, msw wrote: > This may be called a lot, should the result be cached? The concern is whether one or more platforms could change their theme. Please let me know what you think of the prior discussion with pkasting in this same CL about this function. https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:767: gfx::RenderText* render_text, On 2015/03/18 18:03:37, msw wrote: > Why pass in the RenderText? The callers always use description_rendertext_.get() I don't think I follow the question. Would it be better to do something different? I've gotten different suggestions on passing member variables rather than using them directly - please let me know if this version that uses the member directly is what you were looking for. https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:774: // one RenderText On 2015/03/18 18:03:37, msw wrote: > nit: trailing period. Done. https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.h (right): https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.h:19: #include "ui/native_theme/native_theme.h" On 2015/03/18 18:03:37, msw wrote: > Why is this needed here? That would be another holdover from a prior patch in this CL. Thanks for pointing that out. Removed. Done. https://codereview.chromium.org/795933009/diff/620001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/795933009/diff/620001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:455: for (size_t style = 0; style < NUM_TEXT_STYLES; ++style) { On 2015/03/18 18:03:37, msw wrote: > nit: inline this for a one-liner and remove loop curly-braces. > for (size_t style = 0; style < NUM_TEXT_STYLES; ++style) > styles_[style].SetValue(styles_[style].breaks().begin()->second) Done. https://codereview.chromium.org/795933009/diff/620001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:476: BreakList<bool>& break_list = styles_[style]; On 2015/03/18 18:03:37, msw wrote: > ditto nit: inline this for a one-liner and remove loop curly-braces. > for (size_t style = 0; style < NUM_TEXT_STYLES; ++style) > styles_[style].SetMax(text_length) Done. https://codereview.chromium.org/795933009/diff/620001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:481: OnTextAttributeChanged(); On 2015/03/18 18:03:37, msw wrote: > +CC oshima... I think that calling this in the middle of SetText, before the > styles have been finalized, is insufficient. You'll probably need to call this > at the end of SetText, but calling it twice per SetText is likely to cause a > performance regression. I suppose you'll need to have a private > AppendText[Impl|Internal] (or something) that SetText and AppendText can call > that does everything above, except call OnTextAttributeChanged() (and perhaps > also modify cached_bounds_and_offset_valid_ and obscured_reveal_index_), then > have the two public functions do that separately. I went ahead and separated out AppendText into a DoAppendText. Done. https://codereview.chromium.org/795933009/diff/620001/ui/native_theme/fallbac... File ui/native_theme/fallback_theme.cc (right): https://codereview.chromium.org/795933009/diff/620001/ui/native_theme/fallbac... ui/native_theme/fallback_theme.cc:93: color_utils::AlphaBlend(SK_ColorGREEN, kTextfieldDefaultBackground, 0xDD); On 2015/03/18 18:03:37, msw wrote: > ditto nits about red/green locale values and file-local constants. Acknowledged. https://codereview.chromium.org/795933009/diff/620001/ui/native_theme/native_... File ui/native_theme/native_theme_win.cc (right): https://codereview.chromium.org/795933009/diff/620001/ui/native_theme/native_... ui/native_theme/native_theme_win.cc:613: return color_utils::GetReadableColor( On 2015/03/18 18:03:37, msw wrote: > Should the other kColorId_ResultsTable*Text (Normal, Selected, etc.) entries use > GetReadableColor with their respective backgrounds? I'm not sure on that. Either way, I'd prefer to address that in a separate CL. Maybe it can be part of cr bug 467259. https://codereview.chromium.org/795933009/diff/620001/ui/native_theme/native_... ui/native_theme/native_theme_win.cc:649: return color_utils::GetReadableColor(SK_ColorGREEN, On 2015/03/18 18:03:37, msw wrote: > ditto nits about red/green locale values and file-local constants. Acknowledged.
not lgtm with the discrepancy in color behaviors... https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/libgt... File chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc (right): https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/libgt... chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc:382: case kColorId_ResultsTablePositiveText: { On 2015/03/18 19:16:48, dschuyler wrote: > On 2015/03/18 18:03:36, msw wrote: > > Colors have different meanings in other cultures. Is there any precedent for > > selecting locale-based colors? Do UI/UX folks support using hard-coded green > and > > red as the base for good and bad? > > Excellent point. So far the UI doc says red and green, but I agree it would be > worth bringing up the localization question specifically. Should I add a TODO > or enter a bug or email someone in particular about it? Filing a bug would be a good way to loop in the right people and start a discussion. You can CC the AiS PM, ainslie@, and maybe ping chrome-ui-review@google.com. https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/libgt... chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc:383: return GetReadableColor(SK_ColorGREEN, On 2015/03/18 19:16:48, dschuyler wrote: > On 2015/03/18 18:03:36, msw wrote: > > Should the positive/negative base colors have their own themed ids? > > (or perhaps at least a hard-coded const value in their files) > > I'm trying to understand what you mean - like as a data driven resource file > entry for the colors? That sounds good. I could use some help/tips on how to > follow up on that. Should this get a TODO or bug or similar? I meant either (1) Making kColorId_ResultsTable[Positive|Negative]Color or similar, or (2) just defining const SkColor kResultsTable[Positive|Negative]Color = SK_ColorGREEN/SK_ColorRED; in this file and the other theme files for use in these calculations. I'd suggest (1) if that meant we could have a centralized place of getting consistent/localized values across the theme implementations, but otherwise (2) might be okay. https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:204: type = 1; On 2015/03/18 19:16:48, dschuyler wrote: > On 2015/03/18 18:03:37, msw wrote: > > Should an invalid type really default to ANSWER_TEXT with its LargeFont > instead > > of something like SUGGESTION_TEXT with the base font, colors and baseline? > > It's an excellent question. I also thought of making it the negative text so it > would look more like an error. Type 1 seems like it would look less like an > error. I'm open as to what type an invalid type should switch to. I'm pretty > sure we should check the type though, since it's input form an outside source at > runtime. Probably doesn't matter, but I'd go with something that we'd consider "baseline", like SUGGESTION_TEXT, over the larger ANSWER_TEXT (which would make bad server behavior appear more prominent, instead of a no-op). Perhaps explicitly return an inlined TextStyle here with such values: {ui::ResourceBundle::BaseFont, {NativeTheme::kColorId_ResultsTableNormalText, NativeTheme::kColorId_ResultsTableHoveredText, NativeTheme::kColorId_ResultsTableSelectedText}, gfx::NORMAL_BASELINE} https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:753: int OmniboxResultView::GetAnswerLineHeight() const { On 2015/03/18 19:16:48, dschuyler wrote: > On 2015/03/18 18:03:37, msw wrote: > > This may be called a lot, should the result be cached? > > The concern is whether one or more platforms could change their theme. Please > let me know what you think of the prior discussion with pkasting in this same CL > about this function. I don't think we dynamically update the resource bundle fonts with theme changes, and I presume changes wouldn't be reflected within the lifetime of am OmniboxResultView object... I think the performance hit could be severe here, since these result views are updated as the user types in the omnibox. I'd suggest caching this height (at least per OmniboxResultView instance, if not much more widely) without a clear benefit to recalculating the value constantly. https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:767: gfx::RenderText* render_text, On 2015/03/18 19:16:48, dschuyler wrote: > On 2015/03/18 18:03:37, msw wrote: > > Why pass in the RenderText? The callers always use > description_rendertext_.get() > > I don't think I follow the question. Would it be better to do something > different? I've gotten different suggestions on passing member variables rather > than using them directly - please let me know if this version that uses the > member directly is what you were looking for. You got it. https://codereview.chromium.org/795933009/diff/620001/ui/native_theme/native_... File ui/native_theme/native_theme_win.cc (right): https://codereview.chromium.org/795933009/diff/620001/ui/native_theme/native_... ui/native_theme/native_theme_win.cc:613: return color_utils::GetReadableColor( On 2015/03/18 19:16:49, dschuyler wrote: > On 2015/03/18 18:03:37, msw wrote: > > Should the other kColorId_ResultsTable*Text (Normal, Selected, etc.) entries > use > > GetReadableColor with their respective backgrounds? > > I'm not sure on that. Either way, I'd prefer to address that in a separate CL. > Maybe it can be part of cr bug 467259. I'd assume that these colors should be consistent in their behavior... Why would you change one and not the others in this CL? Why would the colors here be pre-calculated against their backgrounds, but the corresponding Ids for other themes not pre-calculated? Wouldn't that yield fundamentally inconsistent/incorrect behavior across themes? Really, I think the omnibox_result_view.cc code should be doing the readable color blending, not the theme files. These theme files should just return what the text color should be [on a white background], and the UI controls that determine the background color should to the readable color blending, since they would know better than this file what the actual background colors would be... Ditto for other themes. https://codereview.chromium.org/795933009/diff/660001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/795933009/diff/660001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:1250: cached_bounds_and_offset_valid_ = false; Sorry, I changed my mind here a bit. I think it'd be more appropriate if this function limited its behavior to just extending the breaklists, and call it something like "UpdateStyleLengths". The callers should either assign or append to |text_|, set |cached_bounds_and_offset_valid_| and |obscured_reveal_index_|, and call OnTextAttributeChanged()... Otherwise, I fear that this function might be misconstrued as a sufficient and self-contained mechanism to extend the text content and leave the RenderText in a good state; it isn't. https://codereview.chromium.org/795933009/diff/660001/ui/native_theme/fallbac... File ui/native_theme/fallback_theme.cc (right): https://codereview.chromium.org/795933009/diff/660001/ui/native_theme/fallbac... ui/native_theme/fallback_theme.cc:93: color_utils::AlphaBlend(SK_ColorGREEN, kTextfieldDefaultBackground, 0xDD); Why are these blended, when others determine readable colors? I'm starting to think that all you need are "case kColorId_ResultsTablePositiveText: return SK_ColorGREEN;" and "case kColorId_ResultsTableNegativeText return SK_ColorRED;", and do any readable color determination at the control level, where the background colors are known.
jdonnelly@chromium.org changed reviewers: + jdonnelly@chromium.org
https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/libgt... File chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc (right): https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/libgt... chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc:382: case kColorId_ResultsTablePositiveText: { On 2015/03/18 20:20:16, msw wrote: > On 2015/03/18 19:16:48, dschuyler wrote: > > On 2015/03/18 18:03:36, msw wrote: > > > Colors have different meanings in other cultures. Is there any precedent for > > > selecting locale-based colors? Do UI/UX folks support using hard-coded green > > and > > > red as the base for good and bad? > > > > Excellent point. So far the UI doc says red and green, but I agree it would > be > > worth bringing up the localization question specifically. Should I add a TODO > > or enter a bug or email someone in particular about it? > > Filing a bug would be a good way to loop in the right people and start a > discussion. You can CC the AiS PM, ainslie@, and maybe ping > mailto:chrome-ui-review@google.com. It's probably also worth checking in first with johanstr@ and suggest-answers@ to see what consideration the GWS folks gave this question. https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:767: gfx::RenderText* render_text, On 2015/03/18 20:20:16, msw wrote: > On 2015/03/18 19:16:48, dschuyler wrote: > > On 2015/03/18 18:03:37, msw wrote: > > > Why pass in the RenderText? The callers always use > > description_rendertext_.get() > > > > I don't think I follow the question. Would it be better to do something > > different? I've gotten different suggestions on passing member variables > rather > > than using them directly - please let me know if this version that uses the > > member directly is what you were looking for. > > You got it. The more we can make methods depend on their local state rather than a more global state, the better, I'd say. Did you feel strongly about this, Mike, or were you just asking out of curiosity?
https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:767: gfx::RenderText* render_text, On 2015/03/18 20:42:57, Justin Donnelly wrote: > On 2015/03/18 20:20:16, msw wrote: > > On 2015/03/18 19:16:48, dschuyler wrote: > > > On 2015/03/18 18:03:37, msw wrote: > > > > Why pass in the RenderText? The callers always use > > > description_rendertext_.get() > > > > > > I don't think I follow the question. Would it be better to do something > > > different? I've gotten different suggestions on passing member variables > > rather > > > than using them directly - please let me know if this version that uses the > > > member directly is what you were looking for. > > > > You got it. > > The more we can make methods depend on their local state rather than a more > global state, the better, I'd say. Did you feel strongly about this, Mike, or > were you just asking out of curiosity? I disagree, member functions should access member state without hesitation, unless they're to be generalized across multiple parts of the member state that can't be determined in the function context. Otherwise, make this file-local and pass in State().
On 2015/03/18 20:51:39, msw wrote: > https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/views... > File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): > > https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/views... > chrome/browser/ui/views/omnibox/omnibox_result_view.cc:767: gfx::RenderText* > render_text, > On 2015/03/18 20:42:57, Justin Donnelly wrote: > > On 2015/03/18 20:20:16, msw wrote: > > > On 2015/03/18 19:16:48, dschuyler wrote: > > > > On 2015/03/18 18:03:37, msw wrote: > > > > > Why pass in the RenderText? The callers always use > > > > description_rendertext_.get() > > > > > > > > I don't think I follow the question. Would it be better to do something > > > > different? I've gotten different suggestions on passing member variables > > > rather > > > > than using them directly - please let me know if this version that uses > the > > > > member directly is what you were looking for. > > > > > > You got it. > > > > The more we can make methods depend on their local state rather than a more > > global state, the better, I'd say. Did you feel strongly about this, Mike, or > > were you just asking out of curiosity? > > I disagree, member functions should access member state without hesitation, > unless they're to be generalized across multiple parts of the member state that > can't be determined in the function context. Otherwise, make this file-local and > pass in State(). This doesn't strike me as a big deal either way, but I'm curious to know what your reasoning is here? Why is important that methods *not* rely only on their own local scope? My reasoning is that by making methods that all operate on the class state you inevitably end up in a situation where all your logic is side-effect based and your methods can only ever be called in a particular orchestration that gets locked down over time. Changes then get increasingly difficult to make and when they are made they often break other things. Methods operating on local state can be more easily modified and recomposed to achieve things like the generalization across multiple states that you mention.
On 2015/03/18 21:43:28, Justin Donnelly wrote: > On 2015/03/18 20:51:39, msw wrote: > > > https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/views... > > File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): > > > > > https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/views... > > chrome/browser/ui/views/omnibox/omnibox_result_view.cc:767: gfx::RenderText* > > render_text, > > On 2015/03/18 20:42:57, Justin Donnelly wrote: > > > On 2015/03/18 20:20:16, msw wrote: > > > > On 2015/03/18 19:16:48, dschuyler wrote: > > > > > On 2015/03/18 18:03:37, msw wrote: > > > > > > Why pass in the RenderText? The callers always use > > > > > description_rendertext_.get() > > > > > > > > > > I don't think I follow the question. Would it be better to do something > > > > > different? I've gotten different suggestions on passing member > variables > > > > rather > > > > > than using them directly - please let me know if this version that uses > > the > > > > > member directly is what you were looking for. > > > > > > > > You got it. > > > > > > The more we can make methods depend on their local state rather than a more > > > global state, the better, I'd say. Did you feel strongly about this, Mike, > or > > > were you just asking out of curiosity? > > > > I disagree, member functions should access member state without hesitation, > > unless they're to be generalized across multiple parts of the member state > that > > can't be determined in the function context. Otherwise, make this file-local > and > > pass in State(). > > This doesn't strike me as a big deal either way, but I'm curious to know what > your reasoning is here? Why is important that methods *not* rely only on their > own local scope? > > My reasoning is that by making methods that all operate on the class state you > inevitably end up in a situation where all your logic is side-effect based and > your methods can only ever be called in a particular orchestration that gets > locked down over time. Changes then get increasingly difficult to make and when > they are made they often break other things. Methods operating on local state > can be more easily modified and recomposed to achieve things like the > generalization across multiple states that you mention. Agreed, it's not a big deal, but a nuanced decision that's influenced by personal preferences. Here, I appreciate the simplification of the function signature and call sites, with no loss of generalization for the existing use cases. Otherwise, I'd suggest the generalized file-local helper that passes all needed instance data to the function, to avoid adding a new member function to the class and simplifying the header. As we agree, it's not a big deal, but I'd opt for one direction or the other, not a mix that seems to combine the worst of both (extra param, but still needing to access some instance data directly via GetState()).
A bunch of responses to Mike's comments below. https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/libgt... File chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc (right): https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/libgt... chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc:383: return GetReadableColor(SK_ColorGREEN, On 2015/03/18 20:20:16, msw wrote: > On 2015/03/18 19:16:48, dschuyler wrote: > > On 2015/03/18 18:03:36, msw wrote: > > > Should the positive/negative base colors have their own themed ids? > > > (or perhaps at least a hard-coded const value in their files) > > > > I'm trying to understand what you mean - like as a data driven resource file > > entry for the colors? That sounds good. I could use some help/tips on how to > > follow up on that. Should this get a TODO or bug or similar? > > I meant either (1) Making kColorId_ResultsTable[Positive|Negative]Color or > similar, or (2) just defining const SkColor > kResultsTable[Positive|Negative]Color = SK_ColorGREEN/SK_ColorRED; in this file > and the other theme files for use in these calculations. I'd suggest (1) if that > meant we could have a centralized place of getting consistent/localized values > across the theme implementations, but otherwise (2) might be okay. (1) would make sense if we need it (but we don't know yet). (2) would only make sense if we do it once in some central location that all different theme files pull from. But honestly, I don't love the fact that we have these global theme constants at all, and I filed a separate bug against Dave already hoping to clean this up in the form of moving all this back to omnibox-specific files. Given that, I wouldn't spend time now on trying to centralize these colors. https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:122: // 1 ANSWER_TEXT On 2015/03/18 19:16:48, dschuyler wrote: > On 2015/03/18 18:03:37, msw wrote: > > Should we have an enum somewhere with these types? > > There may be one in the server code. It wouldn't be used in the client though. > I'm open to adding the enum or leaving it out. I can see it being a form of > documentation. I wouldn't do it right now, since the only use of this array is as raw numbers (not enum values) from the server JSON messages. https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:204: type = 1; On 2015/03/18 20:20:16, msw wrote: > On 2015/03/18 19:16:48, dschuyler wrote: > > On 2015/03/18 18:03:37, msw wrote: > > > Should an invalid type really default to ANSWER_TEXT with its LargeFont > > instead > > > of something like SUGGESTION_TEXT with the base font, colors and baseline? > > > > It's an excellent question. I also thought of making it the negative text so > it > > would look more like an error. Type 1 seems like it would look less like an > > error. I'm open as to what type an invalid type should switch to. I'm pretty > > sure we should check the type though, since it's input form an outside source > at > > runtime. > > Probably doesn't matter, but I'd go with something that we'd consider > "baseline", like SUGGESTION_TEXT, over the larger ANSWER_TEXT (which would make > bad server behavior appear more prominent, instead of a no-op). I disagree, because ANSWER_TEXT is the conceptual "baseline style", despite the fact that it's large. Every answer is built around an ANSWER_TEXT, with some other styles. So that's the style that should be used on error. SUGGESTION_TEXT has a very specific meaning and defaulting to it is not conceptually correct. https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:753: int OmniboxResultView::GetAnswerLineHeight() const { On 2015/03/18 20:20:16, msw wrote: > On 2015/03/18 19:16:48, dschuyler wrote: > > On 2015/03/18 18:03:37, msw wrote: > > > This may be called a lot, should the result be cached? > > > > The concern is whether one or more platforms could change their theme. Please > > let me know what you think of the prior discussion with pkasting in this same > CL > > about this function. > > I don't think we dynamically update the resource bundle fonts with theme > changes, and I presume changes wouldn't be reflected within the lifetime of am > OmniboxResultView object... I think the performance hit could be severe here, > since these result views are updated as the user types in the omnibox. I'd > suggest caching this height (at least per OmniboxResultView instance, if not > much more widely) without a clear benefit to recalculating the value constantly. I already looked into this for quite a while after I suggested the same thing, and I don't think we should cache. The performance his is not actually severe here. The resource bundle instance, font list, and height are all already cached by the functions we're calling. There is ChromeOS code that does, in fact, dynamically update the font styles and rebuild the font tables, so given the chance of problems and the minimal benefit, we shouldn't cache. https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:767: gfx::RenderText* render_text, On 2015/03/18 20:51:39, msw wrote: > On 2015/03/18 20:42:57, Justin Donnelly wrote: > > On 2015/03/18 20:20:16, msw wrote: > > > On 2015/03/18 19:16:48, dschuyler wrote: > > > > On 2015/03/18 18:03:37, msw wrote: > > > > > Why pass in the RenderText? The callers always use > > > > description_rendertext_.get() > > > > > > > > I don't think I follow the question. Would it be better to do something > > > > different? I've gotten different suggestions on passing member variables > > > rather > > > > than using them directly - please let me know if this version that uses > the > > > > member directly is what you were looking for. > > > > > > You got it. > > > > The more we can make methods depend on their local state rather than a more > > global state, the better, I'd say. Did you feel strongly about this, Mike, or > > were you just asking out of curiosity? > > I disagree, member functions should access member state without hesitation, > unless they're to be generalized across multiple parts of the member state that > can't be determined in the function context. Otherwise, make this file-local and > pass in State(). I think Mike is correct. If a function is a member function, it should access member variables directly, not have them passed to it. Member variables are not "global state" for the purpose of member functions. https://codereview.chromium.org/795933009/diff/620001/ui/native_theme/native_... File ui/native_theme/native_theme_win.cc (right): https://codereview.chromium.org/795933009/diff/620001/ui/native_theme/native_... ui/native_theme/native_theme_win.cc:613: return color_utils::GetReadableColor( On 2015/03/18 20:20:16, msw wrote: > On 2015/03/18 19:16:49, dschuyler wrote: > > On 2015/03/18 18:03:37, msw wrote: > > > Should the other kColorId_ResultsTable*Text (Normal, Selected, etc.) entries > > use > > > GetReadableColor with their respective backgrounds? > > > > I'm not sure on that. Either way, I'd prefer to address that in a separate > CL. > > Maybe it can be part of cr bug 467259. > > I'd assume that these colors should be consistent in their behavior... Why would > you change one and not the others in this CL? Why would the colors here be > pre-calculated against their backgrounds, but the corresponding Ids for other > themes not pre-calculated? Wouldn't that yield fundamentally > inconsistent/incorrect behavior across themes? Really, I think the > omnibox_result_view.cc code should be doing the readable color blending, not the > theme files. These theme files should just return what the text color should be > [on a white background], and the UI controls that determine the background color > should to the readable color blending, since they would know better than this > file what the actual background colors would be... Ditto for other themes. I disagree with all of this :) First, it would be incorrect to force the normal and selected text colors through GetReadableColor(), because those are intended to be the user's precise system colors. If we use GetReadableColor(), it will either do nothing, or it will change those colors away from system colors, which isn't what we want. The colors that are not user-set system colors, but are hardcoded or synthesized by us, DO need to be put through GetReadableColor() to minimize the chance of contrast problems. Since the user never set these colors, they won't have any expectation for how they behave. The GetReadableColor() calls belong here, not on the caller side, because it's the job of the theming system to give you the actual colors you need. Making every consumer adjust the colors identically is extra work on the caller side and is more error-prone. Besides this, I intend for all this code to move back into the omnibox-specific files anyway, at which point the question of which end does the adjusting will be moot. https://codereview.chromium.org/795933009/diff/660001/ui/native_theme/fallbac... File ui/native_theme/fallback_theme.cc (right): https://codereview.chromium.org/795933009/diff/660001/ui/native_theme/fallbac... ui/native_theme/fallback_theme.cc:93: color_utils::AlphaBlend(SK_ColorGREEN, kTextfieldDefaultBackground, 0xDD); On 2015/03/18 20:20:17, msw wrote: > Why are these blended, when others determine readable colors? Because all colors in this file are hardcoded and therefore it's unnecessary to call GetReadableColor(). We already know it will be a no-op. The alpha-blending is to be consistent with the other text colors in this file, which all mute the text slightly by alpha-blending with 0xDD to prevent things from looking too contrasty. > I'm starting to think that all you need are "case > kColorId_ResultsTablePositiveText: return SK_ColorGREEN;" and "case > kColorId_ResultsTableNegativeText return SK_ColorRED;", and do any readable > color determination at the control level, where the background colors are known. To repeat what I said elsewhere: No, that's not right. This code is what knows the background colors and should do the right thing.
In an effort to simplify, I've began moving pieces this CL into separate change lists. Please consider giving an LGTM to the separated CLs.
https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:767: gfx::RenderText* render_text, On 2015/03/18 22:19:33, Peter Kasting wrote: > On 2015/03/18 20:51:39, msw wrote: > > On 2015/03/18 20:42:57, Justin Donnelly wrote: > > > On 2015/03/18 20:20:16, msw wrote: > > > > On 2015/03/18 19:16:48, dschuyler wrote: > > > > > On 2015/03/18 18:03:37, msw wrote: > > > > > > Why pass in the RenderText? The callers always use > > > > > description_rendertext_.get() > > > > > > > > > > I don't think I follow the question. Would it be better to do something > > > > > different? I've gotten different suggestions on passing member > variables > > > > rather > > > > > than using them directly - please let me know if this version that uses > > the > > > > > member directly is what you were looking for. > > > > > > > > You got it. > > > > > > The more we can make methods depend on their local state rather than a more > > > global state, the better, I'd say. Did you feel strongly about this, Mike, > or > > > were you just asking out of curiosity? > > > > I disagree, member functions should access member state without hesitation, > > unless they're to be generalized across multiple parts of the member state > that > > can't be determined in the function context. Otherwise, make this file-local > and > > pass in State(). > > I think Mike is correct. If a function is a member function, it should access > member variables directly, not have them passed to it. Member variables are not > "global state" for the purpose of member functions. We could debate whether "global state" is the correct term, but the problems with relying on state outside the narrowest possible scope are not specific to some particular boundary (process-local, thread-local, class-local or whatever). It's simply the fact that relying on communication via side-effects causes brittleness. I take the point that the passed value is both unnecessary here and adds complexity. Using a combination of a passed value and class state in this case, as Mike points out, is kind of confusing. But in the future, I think we'd be better off if we tried harder to avoid side-effect based communication. Where we can keep our classes small and focused, member functions accessing member variables makes sense. But inevitably some of our classes get very large and they start to resemble medium-sized C programs. At that point, saying it's not "global state" becomes a bit academic.
https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/795933009/diff/620001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:767: gfx::RenderText* render_text, On 2015/03/19 15:05:05, Justin Donnelly wrote: > On 2015/03/18 22:19:33, Peter Kasting wrote: > > On 2015/03/18 20:51:39, msw wrote: > > > On 2015/03/18 20:42:57, Justin Donnelly wrote: > > > > On 2015/03/18 20:20:16, msw wrote: > > > > > On 2015/03/18 19:16:48, dschuyler wrote: > > > > > > On 2015/03/18 18:03:37, msw wrote: > > > > > > > Why pass in the RenderText? The callers always use > > > > > > description_rendertext_.get() > > > > > > > > > > > > I don't think I follow the question. Would it be better to do > something > > > > > > different? I've gotten different suggestions on passing member > > variables > > > > > rather > > > > > > than using them directly - please let me know if this version that > uses > > > the > > > > > > member directly is what you were looking for. > > > > > > > > > > You got it. > > > > > > > > The more we can make methods depend on their local state rather than a > more > > > > global state, the better, I'd say. Did you feel strongly about this, Mike, > > or > > > > were you just asking out of curiosity? > > > > > > I disagree, member functions should access member state without hesitation, > > > unless they're to be generalized across multiple parts of the member state > > that > > > can't be determined in the function context. Otherwise, make this file-local > > and > > > pass in State(). > > > > I think Mike is correct. If a function is a member function, it should access > > member variables directly, not have them passed to it. Member variables are > not > > "global state" for the purpose of member functions. > > We could debate whether "global state" is the correct term, but the problems > with relying on state outside the narrowest possible scope are not specific to > some particular boundary (process-local, thread-local, class-local or whatever). > It's simply the fact that relying on communication via side-effects causes > brittleness. It sounds like you're advocating a sort of "functional programming" style of design, where every function takes all the args it needs and only modifies things directly passed to it (and the return value). This can be a powerful tool, but much like constness, it only really works if we go all the way and do it. The equivalent of that here would be a file-scoped local function or a static class member function. A nonstatic class member is inherently operating on the state of the class. I don't think it makes sense to add nonstatic (and, in this case, non-const) member functions that we try to make "more side-effect-free" by passing things to them, because splitting the difference just causes confusion rather than more maintainable code. > But in the future, I think we'd be better off if we tried harder to avoid > side-effect based communication. Where we can keep our classes small and > focused, member functions accessing member variables makes sense. But inevitably > some of our classes get very large and they start to resemble medium-sized C > programs. At that point, saying it's not "global state" becomes a bit academic. Sure, but that argument makes sense if, in fact, the classes in question are large enough to begin to make interaction complex. Are you saying you believe that's the case here? The class certainly seems small enough to be tractable to me, and the use of member variables here not confusing, but maybe you have a different perspective. If your conviction is that in general all classes get too big and unwieldy, then the right answer would be to not use classes, and (as before) write everything as side-effect-free standalone functions. I think that's actually a legitimate style, but it's exceptional in Chromium code. The summary of all this is that I agree with some of your concerns in principle, but in practice I don't think passing member values in to this particular function is a win.
https://codereview.chromium.org/795933009/diff/620001/ui/native_theme/native_... File ui/native_theme/native_theme_win.cc (right): https://codereview.chromium.org/795933009/diff/620001/ui/native_theme/native_... ui/native_theme/native_theme_win.cc:613: return color_utils::GetReadableColor( On 2015/03/18 22:19:33, Peter Kasting wrote: > On 2015/03/18 20:20:16, msw wrote: > > On 2015/03/18 19:16:49, dschuyler wrote: > > > On 2015/03/18 18:03:37, msw wrote: > > > > Should the other kColorId_ResultsTable*Text (Normal, Selected, etc.) > entries > > > use > > > > GetReadableColor with their respective backgrounds? > > > > > > I'm not sure on that. Either way, I'd prefer to address that in a separate > > CL. > > > Maybe it can be part of cr bug 467259. > > > > I'd assume that these colors should be consistent in their behavior... Why > would > > you change one and not the others in this CL? Why would the colors here be > > pre-calculated against their backgrounds, but the corresponding Ids for other > > themes not pre-calculated? Wouldn't that yield fundamentally > > inconsistent/incorrect behavior across themes? Really, I think the > > omnibox_result_view.cc code should be doing the readable color blending, not > the > > theme files. These theme files should just return what the text color should > be > > [on a white background], and the UI controls that determine the background > color > > should to the readable color blending, since they would know better than this > > file what the actual background colors would be... Ditto for other themes. > > I disagree with all of this :) > > First, it would be incorrect to force the normal and selected text colors > through GetReadableColor(), because those are intended to be the user's precise > system colors. If we use GetReadableColor(), it will either do nothing, or it > will change those colors away from system colors, which isn't what we want. > > The colors that are not user-set system colors, but are hardcoded or synthesized > by us, DO need to be put through GetReadableColor() to minimize the chance of > contrast problems. Since the user never set these colors, they won't have any > expectation for how they behave. > > The GetReadableColor() calls belong here, not on the caller side, because it's > the job of the theming system to give you the actual colors you need. Making > every consumer adjust the colors identically is extra work on the caller side > and is more error-prone. > > Besides this, I intend for all this code to move back into the omnibox-specific > files anyway, at which point the question of which end does the adjusting will > be moot. Okay, that's actually reasonable.
dschuyler@chromium.org changed reviewers: - estade@chromium.org, jdonnelly@chromium.org, msw@chromium.org, oshima@chromium.org, sadrul@chromium.org
Hi Peter, This is now down to the color styling for answers. Does it look good to you?
LGTM
The CQ bit was checked by dschuyler@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from estade@chromium.org Link to the patchset: https://codereview.chromium.org/795933009/#ps740001 (title: "Removed unnecessary include")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/795933009/740001
The CQ bit was unchecked by commit-bot@chromium.org
A disapproval has been posted by following reviewers: msw@chromium.org. Please make sure to get positive review before another CQ attempt.
Mike, I think we resolved the stuff you objected to separately, can you sign off on this?
msw@chromium.org changed reviewers: + msw@chromium.org
lgtm
The CQ bit was checked by dschuyler@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/795933009/740001
Message was sent while issue was closed.
Committed patchset #39 (id:740001)
Message was sent while issue was closed.
Patchset 39 (id:??) landed as https://crrev.com/4baa58f7573b1be55872bb30ece372c617915544 Cr-Commit-Position: refs/heads/master@{#321664} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
