Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2278513003:
Material Page Info (Views, 2/3): Update security section. (Closed)
|
Created:
4 years, 3 months ago by lgarron Modified:
4 years, 3 months ago Reviewers:
msw CC:
chromium-reviews, markusheintz_, msramek+watch_chromium.org, raymes+watch_chromium.org, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMaterial Page Info (Views, 2/3): Update security section.
- Add a security summary.
- Update the security details paragraph.
- Change the certificate decision button to an explanation and a link.
The first two bullets depend on string changes in https://codereview.chromium.org/2262223002
BUG=512442
Committed: https://crrev.com/6d3f5c139eefd9656e8bcc736d88a747086eac0f
Cr-Commit-Position: refs/heads/master@{#417822}
Patch Set 1 #Patch Set 2 : Remove accidental extra import. #
Total comments: 45
Patch Set 3 : Update tests based on dependent CL. #Patch Set 4 : Address comments. #Patch Set 5 : Add comment. #Patch Set 6 : Material Page Info (Views, 2/3): Update security section #
Total comments: 8
Patch Set 7 : Address nits. #Patch Set 8 : Rebase. #Messages
Total messages: 31 (14 generated)
lgarron@chromium.org changed reviewers: + msw@chromium.org
msw@, could you review? See https://crbug.com/512442#c59 for a screenshot.
For the record: the tests for this CL depend on https://codereview.chromium.org/2262223002
https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:125: // Sets security summary. nit: // Sets the security summary for the current page. (to be consistent with the comment below) https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:130: void SetDetails(const base::string16& security_summary_text, nit: It's a little confusing that we support SetSummary(text) and SetDetails(security_summary_text, bool), even though these are two separate 'summary' text blobs. Would it make sense to call the first 'Summary' a 'Title' or similar? Or rename |security_summary_text| to |security_details_text| or similar? https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:138: // The listener for the styled lables in this view. nit: 'labels' https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:141: // The label that displays the name of the site's identity. nit: update comment. https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:148: // A container for the button for resetting cert decisions. The button is only nit: update mentions of button, now it's a "styled label with a link" https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:191: reset_decisions_label_container_(nullptr) { nit: also init |reset_decisions_label_| to nullptr https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:221: const gfx::FontList& font_list = rb.GetFontListWithDelta(1); nit: do we really need this to be 1px bigger than the default? https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:227: views::ImageButton* close_button = new views::ImageButton(button_listener); aside: It'd be nice if this bubble simply used the standard close button (see BubbleDialogDelegateView::ShouldShowCloseButton), but that might not be possible, and certainly isn't a blocker for this CL. https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:234: layout->AddView(close_button, 1, 2, views::GridLayout::TRAILING, Is this now spanning two rows to vertically center the close button over the summary and the padding row above or below the summary? I'm not sure why we would need either, especially if we are adding a row above with padding specifically designed for the close button (kHeaderPaddingRightForCloseButton); can you explain? https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:260: new views::BoxLayout(views::BoxLayout::kHorizontal, 0, 0, 0)); aside: perhaps this could be a FillLayout (but it's not really important here). https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:316: l10n_util::GetStringUTF16(IDS_PAGEINFO_INVALID_CERTIFICATE_DESCRIPTION)); Please note CL dependencies in the CL description. I was confused by this until I saw your newer comment about https://codereview.chromium.org/2262223002 https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:323: base::ASCIIToUTF16("$1 $2"), subst, &offsets); This is not the right way to combine localized strings; you should structure the two IDS strings such that one has a placeholder for the other, and then use GetStringFUTF16, so translations can differ in how they merge the two. https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:334: reset_decisions_label_->SizeToFit(0); q: Is this needed? https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:337: // Now that it contains a button, the container needs padding at the top. nit: update mentions of button. https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:506: 0)); // Each section handles its own padding. nit: put the comment before the code and avoid wrapping the statement. https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:674: layout->set_minimum_size(gfx::Size(300, 0)); Why is this needed here? If it's needed, use kSiteSettingsViewContentMinWidth. https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:741: if (security_description->summary_style & WebsiteSettingsUI::STYLE_COLOR) { nit: curlies not needed. https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:742: header_->SetSummaryColor(security_description->summary_color); It would be nice if we passed around security levels instead of colors, then translate levels to colors, similar to the omnibox: https://cs.chromium.org/chromium/src/chrome/browser/ui/views/omnibox/omnibox_...
Description was changed from ========== Page Info: Add colored summary message on Views. BUG=512442 ========== to ========== Page Info: Add colored summary message on Views. This depend on the string changes in https://codereview.chromium.org/2262223002 BUG=512442 ==========
Description was changed from ========== Page Info: Add colored summary message on Views. This depend on the string changes in https://codereview.chromium.org/2262223002 BUG=512442 ========== to ========== Page Info: Add colored summary message on Views. This depends on the string changes in https://codereview.chromium.org/2262223002 BUG=512442 ==========
Thanks for the thorough reviews! I think I've fixed everything (or provided reasons for keeping the change, particularly for the "$1 $2" string concatenation). Could you take another look? https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:125: // Sets security summary. On 2016/08/25 at 01:30:21, msw wrote: > nit: // Sets the security summary for the current page. > (to be consistent with the comment below) Done. https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:130: void SetDetails(const base::string16& security_summary_text, On 2016/08/25 at 01:30:22, msw wrote: > nit: It's a little confusing that we support SetSummary(text) and SetDetails(security_summary_text, bool), even though these are two separate 'summary' text blobs. Would it make sense to call the first 'Summary' a 'Title' or similar? Or rename |security_summary_text| to |security_details_text| or similar? I've changed the parameter to security_details_text, which is the right name for it. https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:138: // The listener for the styled lables in this view. On 2016/08/25 at 01:30:22, msw wrote: > nit: 'labels' Done. https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:141: // The label that displays the name of the site's identity. On 2016/08/25 at 01:30:22, msw wrote: > nit: update comment. Done. https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:148: // A container for the button for resetting cert decisions. The button is only On 2016/08/25 at 01:30:21, msw wrote: > nit: update mentions of button, now it's a "styled label with a link" Done. https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:191: reset_decisions_label_container_(nullptr) { On 2016/08/25 at 01:30:21, msw wrote: > nit: also init |reset_decisions_label_| to nullptr Done. https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:221: const gfx::FontList& font_list = rb.GetFontListWithDelta(1); On 2016/08/25 at 01:30:22, msw wrote: > nit: do we really need this to be 1px bigger than the default? Yep, this is an explicit design design decision in the specs. https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:227: views::ImageButton* close_button = new views::ImageButton(button_listener); On 2016/08/25 at 01:30:22, msw wrote: > aside: It'd be nice if this bubble simply used the standard close button (see BubbleDialogDelegateView::ShouldShowCloseButton), but that might not be possible, and certainly isn't a blocker for this CL. Bug filed: https://crbug.com/640851 https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:234: layout->AddView(close_button, 1, 2, views::GridLayout::TRAILING, On 2016/08/25 at 01:30:22, msw wrote: > Is this now spanning two rows to vertically center the close button over the summary and the padding row above or below the summary? I'm not sure why we would need either, especially if we are adding a row above with padding specifically designed for the close button (kHeaderPaddingRightForCloseButton); can you explain? Good catch, this is a vestigial fix. I was originally planning to have the close button span a spacing row and a row with the summary, but apparently you can't add a padding row without `clear: both`ing the existing rows. I ended up using a border on the summary to fix this, so I've gone ahead and changes this back to 1, 1. https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:260: new views::BoxLayout(views::BoxLayout::kHorizontal, 0, 0, 0)); On 2016/08/25 at 01:30:22, msw wrote: > aside: perhaps this could be a FillLayout (but it's not really important here). Just tried, but that resizes the label to be the full width of the bubble, which centers the label. :-( Gonna stick with BoxLayout for now; that will also easily support moving the button to the right, which we will want for Material design soon. https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:316: l10n_util::GetStringUTF16(IDS_PAGEINFO_INVALID_CERTIFICATE_DESCRIPTION)); On 2016/08/25 at 01:30:21, msw wrote: > Please note CL dependencies in the CL description. I was confused by this until I saw your newer comment about https://codereview.chromium.org/2262223002 Hmm, I thought the convention was only to reference git commits rather than transient info, but I guess it doesn't hurt. Added. https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:323: base::ASCIIToUTF16("$1 $2"), subst, &offsets); On 2016/08/25 at 01:30:22, msw wrote: > This is not the right way to combine localized strings; you should structure the two IDS strings such that one has a placeholder for the other, and then use GetStringFUTF16, so translations can differ in how they merge the two. Unfortunately, OSX needs to use separate strings for the label text and its link. This pattern avoids having two translations (one with a placeholder for Views, and one without for Mac). This was also one of the reasons that led to the first implementation of this pattern (in the preceding function). After an email thread and some discussion, we settled on this. https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:334: reset_decisions_label_->SizeToFit(0); On 2016/08/25 at 01:30:22, msw wrote: > q: Is this needed? Yes. I think we discussed it on a previous CL. The view is not properly sized if we don't call this. https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:506: 0)); // Each section handles its own padding. On 2016/08/25 at 01:30:22, msw wrote: > nit: put the comment before the code and avoid wrapping the statement. Done. https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:674: layout->set_minimum_size(gfx::Size(300, 0)); On 2016/08/25 at 01:30:22, msw wrote: > Why is this needed here? If it's needed, use kSiteSettingsViewContentMinWidth. Removed. (I think this is a merge artifact.) https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:741: if (security_description->summary_style & WebsiteSettingsUI::STYLE_COLOR) { On 2016/08/25 at 01:30:22, msw wrote: > nit: curlies not needed. *sheds tear and removes curlies* https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:742: header_->SetSummaryColor(security_description->summary_color); On 2016/08/25 at 01:30:22, msw wrote: > It would be nice if we passed around security levels instead of colors, then translate levels to colors, similar to the omnibox: https://cs.chromium.org/chromium/src/chrome/browser/ui/views/omnibox/omnibox_... I strongly wanted to avoid hard-coding the colors in each platform separately. There doesn't seem to be a central place to get Material colors, so this pattern is the best I could find.
lg modulo colors https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:221: const gfx::FontList& font_list = rb.GetFontListWithDelta(1); On 2016/08/25 03:26:58, lgarron wrote: > On 2016/08/25 at 01:30:22, msw wrote: > > nit: do we really need this to be 1px bigger than the default? > > Yep, this is an explicit design design decision in the specs. Acknowledged. https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:260: new views::BoxLayout(views::BoxLayout::kHorizontal, 0, 0, 0)); On 2016/08/25 03:26:58, lgarron wrote: > On 2016/08/25 at 01:30:22, msw wrote: > > aside: perhaps this could be a FillLayout (but it's not really important > here). > > Just tried, but that resizes the label to be the full width of the bubble, which > centers the label. :-( > Gonna stick with BoxLayout for now; that will also easily support moving the > button to the right, which we will want for Material design soon. Acknowledged. https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:316: l10n_util::GetStringUTF16(IDS_PAGEINFO_INVALID_CERTIFICATE_DESCRIPTION)); On 2016/08/25 03:26:57, lgarron wrote: > On 2016/08/25 at 01:30:21, msw wrote: > > Please note CL dependencies in the CL description. I was confused by this > until I saw your newer comment about https://codereview.chromium.org/2262223002 > > Hmm, I thought the convention was only to reference git commits rather than > transient info, but I guess it doesn't hurt. Added. This is just to make reviewing easier. I was reviewing this patch set and couldn't find either IDS_PAGEINFO_INVALID_CERTIFICATE_DESCRIPTION nor WebsiteSettingsUI::SecurityDescription in codesearch, so I wasn't sure if you forgot to make those changes in this CL, and I'd rather not dig through your open CLs, git history, etc. just to find prerequisite changes. Your comment referencing the dependency came just before I was about to send comments asking about these missing pieces. There's some 'git cl' command for marking dependent CLs, but i'm not familiar with it, I just typically call them out in comments/description/message. https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:323: base::ASCIIToUTF16("$1 $2"), subst, &offsets); On 2016/08/25 03:26:58, lgarron wrote: > On 2016/08/25 at 01:30:22, msw wrote: > > This is not the right way to combine localized strings; you should structure > the two IDS strings such that one has a placeholder for the other, and then use > GetStringFUTF16, so translations can differ in how they merge the two. > > Unfortunately, OSX needs to use separate strings for the label text and its > link. > This pattern avoids having two translations (one with a placeholder for Views, > and one without for Mac). > > This was also one of the reasons that led to the first implementation of this > pattern (in the preceding function). After an email thread and some discussion, > we settled on this. Acknowledged. https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:334: reset_decisions_label_->SizeToFit(0); On 2016/08/25 03:26:57, lgarron wrote: > On 2016/08/25 at 01:30:22, msw wrote: > > q: Is this needed? > > Yes. I think we discussed it on a previous CL. The view is not properly sized if > we don't call this. Right... Can you add a similar comment: // Fit the styled label to occupy available width. https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:742: header_->SetSummaryColor(security_description->summary_color); On 2016/08/25 03:26:58, lgarron wrote: > On 2016/08/25 at 01:30:22, msw wrote: > > It would be nice if we passed around security levels instead of colors, then > translate levels to colors, similar to the omnibox: > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/omnibox/omnibox_... > > I strongly wanted to avoid hard-coding the colors in each platform separately. > There doesn't seem to be a central place to get Material colors, so this pattern > is the best I could find. The right way to do is usually defining native theme colors. I highly suggest looking at which colors are used and how they are picked and modified for themes and readability in LocationBarView::GetSecureTextColor. Otherwise, I presume these colors won't look very good in high-contrast inverted color themes, etc.
Description was changed from ========== Page Info: Add colored summary message on Views. This depends on the string changes in https://codereview.chromium.org/2262223002 BUG=512442 ========== to ========== Page Info: Add colored summary message on Views. This depends on the string and styling changes in https://codereview.chromium.org/2262223002 BUG=512442 ==========
https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:316: l10n_util::GetStringUTF16(IDS_PAGEINFO_INVALID_CERTIFICATE_DESCRIPTION)); On 2016/08/25 at 03:56:46, msw wrote: > On 2016/08/25 03:26:57, lgarron wrote: > > On 2016/08/25 at 01:30:21, msw wrote: > > > Please note CL dependencies in the CL description. I was confused by this > > until I saw your newer comment about https://codereview.chromium.org/2262223002 > > > > Hmm, I thought the convention was only to reference git commits rather than > > transient info, but I guess it doesn't hurt. Added. > > This is just to make reviewing easier. I was reviewing this patch set and couldn't find either IDS_PAGEINFO_INVALID_CERTIFICATE_DESCRIPTION nor WebsiteSettingsUI::SecurityDescription in codesearch, so I wasn't sure if you forgot to make those changes in this CL, and I'd rather not dig through your open CLs, git history, etc. just to find prerequisite changes. Your comment referencing the dependency came just before I was about to send comments asking about these missing pieces. There's some 'git cl' command for marking dependent CLs, but i'm not familiar with it, I just typically call them out in comments/description/message. Good point. Acknowledged. :-) https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:334: reset_decisions_label_->SizeToFit(0); On 2016/08/25 at 03:56:46, msw wrote: > On 2016/08/25 03:26:57, lgarron wrote: > > On 2016/08/25 at 01:30:22, msw wrote: > > > q: Is this needed? > > > > Yes. I think we discussed it on a previous CL. The view is not properly sized if > > we don't call this. > > Right... Can you add a similar comment: > // Fit the styled label to occupy available width. Done. https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:742: header_->SetSummaryColor(security_description->summary_color); On 2016/08/25 at 03:56:46, msw wrote: > On 2016/08/25 03:26:58, lgarron wrote: > > On 2016/08/25 at 01:30:22, msw wrote: > > > It would be nice if we passed around security levels instead of colors, then > > translate levels to colors, similar to the omnibox: > > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/omnibox/omnibox_... > > > > I strongly wanted to avoid hard-coding the colors in each platform separately. > > There doesn't seem to be a central place to get Material colors, so this pattern > > is the best I could find. > > The right way to do is usually defining native theme colors. I highly suggest looking at which colors are used and how they are picked and modified for themes and readability in LocationBarView::GetSecureTextColor. Otherwise, I presume these colors won't look very good in high-contrast inverted color themes, etc. In this case, the UI surface is not colored by the theme (Page Info always white/a light background), and the colors for the summary are always the same fixed green/red Material colors. Is the current code acceptable for this case?
https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:742: header_->SetSummaryColor(security_description->summary_color); On 2016/08/25 04:10:14, lgarron wrote: > On 2016/08/25 at 03:56:46, msw wrote: > > On 2016/08/25 03:26:58, lgarron wrote: > > > On 2016/08/25 at 01:30:22, msw wrote: > > > > It would be nice if we passed around security levels instead of colors, > then > > > translate levels to colors, similar to the omnibox: > > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/omnibox/omnibox_... > > > > > > I strongly wanted to avoid hard-coding the colors in each platform > separately. > > > There doesn't seem to be a central place to get Material colors, so this > pattern > > > is the best I could find. > > > > The right way to do is usually defining native theme colors. I highly suggest > looking at which colors are used and how they are picked and modified for themes > and readability in LocationBarView::GetSecureTextColor. Otherwise, I presume > these colors won't look very good in high-contrast inverted color themes, etc. > > In this case, the UI surface is not colored by the theme (Page Info always > white/a light background), and the colors for the summary are always the same > fixed green/red Material colors. > Is the current code acceptable for this case? Just tested; the bubble has a black background in Win7 "High Contrast Black".
On 2016/08/25 at 04:44:57, msw wrote: > https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... > File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): > > https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... > chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:742: header_->SetSummaryColor(security_description->summary_color); > On 2016/08/25 04:10:14, lgarron wrote: > > On 2016/08/25 at 03:56:46, msw wrote: > > > On 2016/08/25 03:26:58, lgarron wrote: > > > > On 2016/08/25 at 01:30:22, msw wrote: > > > > > It would be nice if we passed around security levels instead of colors, > > then > > > > translate levels to colors, similar to the omnibox: > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/omnibox/omnibox_... > > > > > > > > I strongly wanted to avoid hard-coding the colors in each platform > > separately. > > > > There doesn't seem to be a central place to get Material colors, so this > > pattern > > > > is the best I could find. > > > > > > The right way to do is usually defining native theme colors. I highly suggest > > looking at which colors are used and how they are picked and modified for themes > > and readability in LocationBarView::GetSecureTextColor. Otherwise, I presume > > these colors won't look very good in high-contrast inverted color themes, etc. > > > > In this case, the UI surface is not colored by the theme (Page Info always > > white/a light background), and the colors for the summary are always the same > > fixed green/red Material colors. > > Is the current code acceptable for this case? > > Just tested; the bubble has a black background in Win7 "High Contrast Black". 😳😡 Appears you're right. Why does no one tell me these things? (I mean, you did. Thanks for that. This project has just had a lot of undocumented caveats.) I'm thinking of changing WebsiteSettingsUI::IdentityInfo::GetSecurityDescription() to GetSecurityDescriptionForBackground(SkColor background) so that the only way to get the text color is by taking the background into account. Does that sound good, or is it overkill? (I could just add a comment that the color should only be used if the color is not dark according to color_utils::IsDark() and rely on the Views code to call color_utils::IsDark() itself.)
On 2016/08/25 05:46:35, lgarron wrote: > On 2016/08/25 at 04:44:57, msw wrote: > > > https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... > > File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc > (right): > > > > > https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... > > chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:742: > header_->SetSummaryColor(security_description->summary_color); > > On 2016/08/25 04:10:14, lgarron wrote: > > > On 2016/08/25 at 03:56:46, msw wrote: > > > > On 2016/08/25 03:26:58, lgarron wrote: > > > > > On 2016/08/25 at 01:30:22, msw wrote: > > > > > > It would be nice if we passed around security levels instead of > colors, > > > then > > > > > translate levels to colors, similar to the omnibox: > > > > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/omnibox/omnibox_... > > > > > > > > > > I strongly wanted to avoid hard-coding the colors in each platform > > > separately. > > > > > There doesn't seem to be a central place to get Material colors, so this > > > pattern > > > > > is the best I could find. > > > > > > > > The right way to do is usually defining native theme colors. I highly > suggest > > > looking at which colors are used and how they are picked and modified for > themes > > > and readability in LocationBarView::GetSecureTextColor. Otherwise, I presume > > > these colors won't look very good in high-contrast inverted color themes, > etc. > > > > > > In this case, the UI surface is not colored by the theme (Page Info always > > > white/a light background), and the colors for the summary are always the > same > > > fixed green/red Material colors. > > > Is the current code acceptable for this case? > > > > Just tested; the bubble has a black background in Win7 "High Contrast Black". > > 😳😡 > Appears you're right. Why does no one tell me these things? > (I mean, you did. Thanks for that. This project has just had a lot of > undocumented caveats.) > > I'm thinking of changing > WebsiteSettingsUI::IdentityInfo::GetSecurityDescription() to > GetSecurityDescriptionForBackground(SkColor background) so that the only way to > get the text color is by taking the background into account. Does that sound > good, or is it overkill? (I could just add a comment that the color should only > be used if the color is not dark according to color_utils::IsDark() and rely on > the Views code to call color_utils::IsDark() itself.) I suggest passing a security level via IdentityInfo, and letting the bubble view decide how to present that. Please look at LocationBarView::GetSecureTextColor and LocationBarView::GetColor; it'd be nice if both ui surfaces shared that code, but a good step in that direction is using a similar pattern. It'd be good to structure your level->color code such that it could support getting a SELECTED_TEXT color; I've heard that we want this bubble to eventually support selectable text. If you want other platforms to share this code, you can put helpers on IdentityInfo or WebsiteSettingsUI.
Description was changed from ========== Page Info: Add colored summary message on Views. This depends on the string and styling changes in https://codereview.chromium.org/2262223002 BUG=512442 ========== to ========== Material Page Info: Add colored summary message on Views. This depends on the string and styling changes in https://codereview.chromium.org/2262223002 BUG=512442 ==========
Description was changed from ========== Material Page Info: Add colored summary message on Views. This depends on the string and styling changes in https://codereview.chromium.org/2262223002 BUG=512442 ========== to ========== Material Page Info (Views): Add colored summary message. This depends on the string and styling changes in https://codereview.chromium.org/2262223002 BUG=512442 ==========
Description was changed from ========== Material Page Info (Views): Add colored summary message. This depends on the string and styling changes in https://codereview.chromium.org/2262223002 BUG=512442 ========== to ========== Material Page Info (Views, 2/3): Add colored summary message. This depends on the string and styling changes in https://codereview.chromium.org/2262223002 BUG=512442 ==========
Description was changed from ========== Material Page Info (Views, 2/3): Add colored summary message. This depends on the string and styling changes in https://codereview.chromium.org/2262223002 BUG=512442 ========== to ========== Material Page Info (Views, 2/3): Update security section - Add a colored security summary. - This depends on the string and styling changes in [1]. - Update security details. - Change the certificate decision button to an explanation and a link. [1] https://codereview.chromium.org/2262223002 BUG=512442 ==========
Description was changed from ========== Material Page Info (Views, 2/3): Update security section - Add a colored security summary. - This depends on the string and styling changes in [1]. - Update security details. - Change the certificate decision button to an explanation and a link. [1] https://codereview.chromium.org/2262223002 BUG=512442 ========== to ========== Material Page Info (Views, 2/3): Update security section - Add a colored security summary. - This depends on the string and styling changes in [1]. - Update security details. - Change the certificate decision button to an explanation and a link. [1] https://codereview.chromium.org/2262223002 BUG=512442 ==========
Description was changed from ========== Material Page Info (Views, 2/3): Update security section - Add a colored security summary. - This depends on the string and styling changes in [1]. - Update security details. - Change the certificate decision button to an explanation and a link. [1] https://codereview.chromium.org/2262223002 BUG=512442 ========== to ========== Material Page Info (Views, 2/3): Update security section. - Add a colored security summary. - This depends on the string and styling changes in [1]. - Update security details. - Change the certificate decision button to an explanation and a link. [1] https://codereview.chromium.org/2262223002 BUG=512442 ==========
On 2016/08/25 at 15:04:38, msw wrote: > On 2016/08/25 05:46:35, lgarron wrote: > > On 2016/08/25 at 04:44:57, msw wrote: > > > > > https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... > > > File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc > > (right): > > > > > > > > https://codereview.chromium.org/2278513003/diff/20001/chrome/browser/ui/views... > > > chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:742: > > header_->SetSummaryColor(security_description->summary_color); > > > On 2016/08/25 04:10:14, lgarron wrote: > > > > On 2016/08/25 at 03:56:46, msw wrote: > > > > > On 2016/08/25 03:26:58, lgarron wrote: > > > > > > On 2016/08/25 at 01:30:22, msw wrote: > > > > > > > It would be nice if we passed around security levels instead of > > colors, > > > > then > > > > > > translate levels to colors, similar to the omnibox: > > > > > > > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/omnibox/omnibox_... > > > > > > > > > > > > I strongly wanted to avoid hard-coding the colors in each platform > > > > separately. > > > > > > There doesn't seem to be a central place to get Material colors, so this > > > > pattern > > > > > > is the best I could find. > > > > > > > > > > The right way to do is usually defining native theme colors. I highly > > suggest > > > > looking at which colors are used and how they are picked and modified for > > themes > > > > and readability in LocationBarView::GetSecureTextColor. Otherwise, I presume > > > > these colors won't look very good in high-contrast inverted color themes, > > etc. > > > > > > > > In this case, the UI surface is not colored by the theme (Page Info always > > > > white/a light background), and the colors for the summary are always the > > same > > > > fixed green/red Material colors. > > > > Is the current code acceptable for this case? > > > > > > Just tested; the bubble has a black background in Win7 "High Contrast Black". > > > > 😳😡 > > Appears you're right. Why does no one tell me these things? > > (I mean, you did. Thanks for that. This project has just had a lot of > > undocumented caveats.) > > > > I'm thinking of changing > > WebsiteSettingsUI::IdentityInfo::GetSecurityDescription() to > > GetSecurityDescriptionForBackground(SkColor background) so that the only way to > > get the text color is by taking the background into account. Does that sound > > good, or is it overkill? (I could just add a comment that the color should only > > be used if the color is not dark according to color_utils::IsDark() and rely on > > the Views code to call color_utils::IsDark() itself.) > > I suggest passing a security level via IdentityInfo, and letting the bubble view decide how to present that. Please look at LocationBarView::GetSecureTextColor and LocationBarView::GetColor; it'd be nice if both ui surfaces shared that code, but a good step in that direction is using a similar pattern. It'd be good to structure your level->color code such that it could support getting a SELECTED_TEXT color; I've heard that we want this bubble to eventually support selectable text. If you want other platforms to share this code, you can put helpers on IdentityInfo or WebsiteSettingsUI. Unfortunately, there are already a few enums that express "security level", and I don't want to introduce a new one. To do this properly should probably involve removing WebsiteSettings::SiteIdentityStatus and either replacing it with something that actually represents what we need, or adapting values form SecurityStateModel. What do you think of landing this without string coloring, and properly addressing security levels and colors in followup CLs?
> Unfortunately, there are already a few enums that express "security level",
and
> I don't want to introduce a new one.
>
> To do this properly should probably involve removing
> WebsiteSettings::SiteIdentityStatus and either replacing it with something
that
> actually represents what we need, or adapting values form SecurityStateModel.
Ah, IdentityInfo::identity_status is a security level enum
(WebsiteSettings::SiteIdentityStatus) so:
// TODO: Consolidate this code with LocationBarView::GetSecureTextColor...
SkColor WebsiteSettings::GetSecureTextColor(WebsiteSettings::SiteIdentityStatus
status) {
// TODO(lgarron): Support inverted color themes, etc., like
LocationBarView::GetSecureTextColor.
switch (identity_status) {
case WebsiteSettings::SITE_IDENTITY_STATUS_CERT:
return ui::GetNativeTheme()->GetSystemColor(...);
...
> What do you think of landing this without string coloring, and properly
> addressing security levels and colors in followup CLs?
Could you add something like above, leaving TODOs as needed?
Otherwise, I guess you could remove the coloring changes from this CL.
On 2016/09/10 at 00:28:25, msw wrote:
> > Unfortunately, there are already a few enums that express "security level",
and
> > I don't want to introduce a new one.
> >
> > To do this properly should probably involve removing
> > WebsiteSettings::SiteIdentityStatus and either replacing it with something
that
> > actually represents what we need, or adapting values form
SecurityStateModel.
>
> Ah, IdentityInfo::identity_status is a security level enum
(WebsiteSettings::SiteIdentityStatus) so:
> // TODO: Consolidate this code with LocationBarView::GetSecureTextColor...
> SkColor
WebsiteSettings::GetSecureTextColor(WebsiteSettings::SiteIdentityStatus status)
{
> // TODO(lgarron): Support inverted color themes, etc., like
LocationBarView::GetSecureTextColor.
> switch (identity_status) {
> case WebsiteSettings::SITE_IDENTITY_STATUS_CERT:
> return ui::GetNativeTheme()->GetSystemColor(...);
> ...
>
> > What do you think of landing this without string coloring, and properly
> > addressing security levels and colors in followup CLs?
>
> Could you add something like above, leaving TODOs as needed?
> Otherwise, I guess you could remove the coloring changes from this CL.
I'd rather force myself to pay down the technical debt. ;-)
The latest patch removes the coloring code, and I've also removed the coloring
parts of the dependent CL (https://codereview.chromium.org/2262223002).
lgtm with nits; please update the cl description. https://codereview.chromium.org/2278513003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/2278513003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:145: // A container for the styled label with link for resetting cert decisions. nit: "a link" https://codereview.chromium.org/2278513003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:213: layout->AddPaddingRow(0, kHeaderPaddingRightForCloseButton); nit: If the value is a height, using kHeaderPaddingRight* doesn't seem correct. https://codereview.chromium.org/2278513003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:218: const gfx::FontList& font_list = rb.GetFontListWithDelta(1); nit: I still think it's odd to increase this font size by just 1px. I wonder if the spec simply cited a random size that happened to be 1px off from the base font size, instead of explicitly declaring that this label should be 1px taller than the base size that's used nearly everywhere else? Can you point me to that spec/decision? https://codereview.chromium.org/2278513003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:221: kHeaderPaddingTop - kHeaderPaddingRightForCloseButton, 0, 0, 0)); ditto nit: it seems weird to subtract 'Top' and 'Right' padding values.
Description was changed from ========== Material Page Info (Views, 2/3): Update security section. - Add a colored security summary. - This depends on the string and styling changes in [1]. - Update security details. - Change the certificate decision button to an explanation and a link. [1] https://codereview.chromium.org/2262223002 BUG=512442 ========== to ========== Material Page Info (Views, 2/3): Update security section. - Add a security summary. - Update the security details paragraph. - Change the certificate decision button to an explanation and a link. The first two bullets depend on string changes in https://codereview.chromium.org/2262223002 BUG=512442 ==========
https://codereview.chromium.org/2278513003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/2278513003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:145: // A container for the styled label with link for resetting cert decisions. On 2016/09/10 at 01:51:26, msw wrote: > nit: "a link" Done. https://codereview.chromium.org/2278513003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:213: layout->AddPaddingRow(0, kHeaderPaddingRightForCloseButton); On 2016/09/10 at 01:51:26, msw wrote: > nit: If the value is a height, using kHeaderPaddingRight* doesn't seem correct. Changed to kHeaderPaddingForCloseButton. https://codereview.chromium.org/2278513003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:218: const gfx::FontList& font_list = rb.GetFontListWithDelta(1); On 2016/09/10 at 01:51:26, msw wrote: > nit: I still think it's odd to increase this font size by just 1px. I wonder if the spec simply cited a random size that happened to be 1px off from the base font size, instead of explicitly declaring that this label should be 1px taller than the base size that's used nearly everywhere else? Can you point me to that spec/decision? This is based off these mocks [1], this comment [2], and an okay from Max Walker (the current designer for the project) by saying "Thanks" and picking other nits at [3]. [1] https://drive.google.com/corp/drive/folders/0B1kg1oK-WfnuU1NJV2xrRFYzRFE [2] https://bugs.chromium.org/p/chromium/issues/detail?id=512442#c48 [3] https://bugs.chromium.org/p/chromium/issues/detail?id=512442#c55 https://codereview.chromium.org/2278513003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:221: kHeaderPaddingTop - kHeaderPaddingRightForCloseButton, 0, 0, 0)); On 2016/09/10 at 01:51:26, msw wrote: > ditto nit: it seems weird to subtract 'Top' and 'Right' padding values. [see above]
The CQ bit was checked by lgarron@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org Link to the patchset: https://codereview.chromium.org/2278513003/#ps140001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Material Page Info (Views, 2/3): Update security section. - Add a security summary. - Update the security details paragraph. - Change the certificate decision button to an explanation and a link. The first two bullets depend on string changes in https://codereview.chromium.org/2262223002 BUG=512442 ========== to ========== Material Page Info (Views, 2/3): Update security section. - Add a security summary. - Update the security details paragraph. - Change the certificate decision button to an explanation and a link. The first two bullets depend on string changes in https://codereview.chromium.org/2262223002 BUG=512442 Committed: https://crrev.com/6d3f5c139eefd9656e8bcc736d88a747086eac0f Cr-Commit-Position: refs/heads/master@{#417822} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/6d3f5c139eefd9656e8bcc736d88a747086eac0f Cr-Commit-Position: refs/heads/master@{#417822} |
