Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2107123002:
Convert OSX Page Info Bubble to Material Design (Closed)
|
Created:
4 years, 5 months ago by lgarron Modified:
4 years, 3 months ago CC:
chromium-reviews, markusheintz_, msramek+watch_chromium.org, oshima+watch_chromium.org, palmer, raymes+watch_chromium.org, toyoshim+midi_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionConvert OSX Page Info Bubble to Material Design
BUG=512442, 525304
TEST=
1. google.com > click on the lock icon > "Secure connection" should show up in green
2. Visit https://permission.site, try granting and revoking permissions, then clicking on the lock icon to make sure changes reflected.
3. expired.badssl.com > see warning > Advanced > Proceed > click on the lock icon > Re-enable warnings > reload page > the warning should show up again.
4. mixed.badssl.com
Patch Set 1 #
Total comments: 12
Patch Set 2 : Address comments. #
Total comments: 2
Patch Set 3 : Convert OSX Page Info Bubble to Material Design #Patch Set 4 : Layout fixes. #
Total comments: 5
Patch Set 5 : Convert OSX Page Info Bubble to Material Design #
Messages
Total messages: 26 (9 generated)
Description was changed from ========== Refactor dimensions. BUG=512442, 525304 was: 6321aaf ========== to ========== Refactor dimensions. BUG=512442, 525304 TEST= 1. google.com > click on the lock icon > "Secure connection" should show up in green 2. Visit https://permission.site, try granting and revoking permissions, then clicking on the lock icon to make sure changes reflected. 3. expired.badssl.com > see warning > Advanced > Proceed > click on the lock icon > Re-enable warnings > reload page > the warning should show up again. 4. mixed.badssl.com ==========
lgarron@chromium.org changed reviewers: + felt@chromium.org, palmer@chromium.org, rsesek@chromium.org
rsesek@, would you mind doing an initial review? See https://bugs.chromium.org/p/chromium/issues/detail?id=512442#c33 for screenshots. Strings are not final; decisions in progress at [1] and [2]. There are still some sketchy parts, but I've tried to leave everything in a better state, although I still haven't addressed the string overlap issue in the screenshot comment. [1] https://docs.google.com/document/d/1aZkYU0x4GxQi9mEpR3mSGZpLXRrnXh_WMRozO79ru... (Googler-only) [2] https://docs.google.com/document/d/1wMKbWsXKHzTT1e_UZkI1jtHrCCcVsNTX18P6db-y6...
Description was changed from ========== Refactor dimensions. BUG=512442, 525304 TEST= 1. google.com > click on the lock icon > "Secure connection" should show up in green 2. Visit https://permission.site, try granting and revoking permissions, then clicking on the lock icon to make sure changes reflected. 3. expired.badssl.com > see warning > Advanced > Proceed > click on the lock icon > Re-enable warnings > reload page > the warning should show up again. 4. mixed.badssl.com ========== to ========== Convert OSX Page Info Bubble to Material Design BUG=512442, 525304 TEST= 1. google.com > click on the lock icon > "Secure connection" should show up in green 2. Visit https://permission.site, try granting and revoking permissions, then clicking on the lock icon to make sure changes reflected. 3. expired.badssl.com > see warning > Advanced > Proceed > click on the lock icon > Re-enable warnings > reload page > the warning should show up again. 4. mixed.badssl.com ==========
lgarron@chromium.org changed reviewers: - felt@chromium.org, palmer@chromium.org, rsesek@chromium.org
rsesek@chromium.org changed reviewers: + rsesek@chromium.org
Overall looks pretty good. https://codereview.chromium.org/2107123002/diff/1/chrome/browser/ui/cocoa/web... File chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.mm (right): https://codereview.chromium.org/2107123002/diff/1/chrome/browser/ui/cocoa/web... chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.mm:685: [securitySummaryField_ setTextColor:ColorWithRGBBytes(0x0B, 0x80, 0x43)]; Are these colors going to be the same on Mac and other platforms? If so, you could add a method SecurityDescription to get the SkColor for the summary_style. https://codereview.chromium.org/2107123002/diff/1/chrome/browser/ui/cocoa/web... chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.mm:959: // position.x += BYEkPermissionPopUpXSpacing; // TODO ?
lgarron@chromium.org changed reviewers: + felt@chromium.org
felt@, I will need to add the SkColor handling rsesek@ suggested, but could you
review
chrome/browser/ui/website_settings/website_settings_ui.{cc, h}?
https://codereview.chromium.org/2107123002/diff/1/chrome/browser/ui/cocoa/web...
File
chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.mm
(right):
https://codereview.chromium.org/2107123002/diff/1/chrome/browser/ui/cocoa/web...
chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.mm:685:
[securitySummaryField_ setTextColor:ColorWithRGBBytes(0x0B, 0x80, 0x43)];
On 2016/06/29 at 18:22:06, Robert Sesek wrote:
> Are these colors going to be the same on Mac and other platforms? If so, you
could add a method SecurityDescription to get the SkColor for the summary_style.
Sharing sounds good. iOS seems to have a utility for converting an SkColor to
UIColor[1]; is there something I can use on desktop Mac?
[1]
https://cs.chromium.org/chromium/src/skia/ext/skia_utils_ios.mm?q=SkColor+fil...
https://codereview.chromium.org/2107123002/diff/1/chrome/browser/ui/cocoa/web...
chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.mm:959:
// position.x += BYEkPermissionPopUpXSpacing; // TODO
On 2016/06/29 at 18:22:06, Robert Sesek wrote:
> ?
I did a lot of simplifications of constants. This one needs to go BYE. :-P
https://codereview.chromium.org/2107123002/diff/1/chrome/browser/ui/cocoa/web... File chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.mm (right): https://codereview.chromium.org/2107123002/diff/1/chrome/browser/ui/cocoa/web... chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.mm:685: [securitySummaryField_ setTextColor:ColorWithRGBBytes(0x0B, 0x80, 0x43)]; On 2016/06/29 20:04:41, lgarron wrote: > On 2016/06/29 at 18:22:06, Robert Sesek wrote: > > Are these colors going to be the same on Mac and other platforms? If so, you > could add a method SecurityDescription to get the SkColor for the summary_style. > > Sharing sounds good. iOS seems to have a utility for converting an SkColor to > UIColor[1]; is there something I can use on desktop Mac? > > [1] > https://cs.chromium.org/chromium/src/skia/ext/skia_utils_ios.mm?q=SkColor+fil... Did you check skia_utils_mac.h? ;-)
Did you already optimize the pngs? I think this is the script for it: https://cs.chromium.org/chromium/src/tools/resources/optimize-png-files.sh
On 2016/06/30 05:27:00, felt wrote: > Did you already optimize the pngs? I think this is the script for it: > https://cs.chromium.org/chromium/src/tools/resources/optimize-png-files.sh Readme here: https://cs.chromium.org/chromium/src/chrome/app/theme/README?q=optimize-png-f...
https://codereview.chromium.org/2107123002/diff/1/chrome/browser/ui/website_s... File chrome/browser/ui/website_settings/website_settings_ui.cc (right): https://codereview.chromium.org/2107123002/diff/1/chrome/browser/ui/website_s... chrome/browser/ui/website_settings/website_settings_ui.cc:128: security_description->summary_style = summary_style; I'm a little confused, where is the summary_style read/used? https://codereview.chromium.org/2107123002/diff/1/chrome/browser/ui/website_s... chrome/browser/ui/website_settings/website_settings_ui.cc:191: return CreateSecurityDescription(IDS_WEBSITE_SETTINGS_NOT_SECURE_SUMMARY, Since some of these now share strings and styles, could those be collapsed into one case, like: Case A: Case B: Case C: return Foo(); It looks to me like SITE_IDENTITY_STATUS_UNKNOWN, SITE_IDENTITY_STATUS_ADMIN_PROVIDED_CERT, and SITE_IDENTITY_STATUS_DEPRECATED_SIGNATURE_ALGORITHM_MAJOR could be combined for example. https://codereview.chromium.org/2107123002/diff/1/chrome/browser/ui/website_s... File chrome/browser/ui/website_settings/website_settings_ui.h (right): https://codereview.chromium.org/2107123002/diff/1/chrome/browser/ui/website_s... chrome/browser/ui/website_settings/website_settings_ui.h:126: // Helper to get the status text style (i.e. color). This comment seems mismatched, it is not above a helper method?
Patchset #2 (id:20001) has been deleted
Latest patch addresses nits, but there will probably be string changes, and
possible be website_settings_ui.{h, cc} changes depending on Views.
felt@: Yep, the .PNGs are optimized.
https://codereview.chromium.org/2107123002/diff/1/chrome/browser/ui/cocoa/web...
File
chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.mm
(right):
https://codereview.chromium.org/2107123002/diff/1/chrome/browser/ui/cocoa/web...
chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.mm:685:
[securitySummaryField_ setTextColor:ColorWithRGBBytes(0x0B, 0x80, 0x43)];
On 2016/06/29 at 20:15:02, Robert Sesek wrote:
> On 2016/06/29 20:04:41, lgarron wrote:
> > On 2016/06/29 at 18:22:06, Robert Sesek wrote:
> > > Are these colors going to be the same on Mac and other platforms? If so,
you
> > could add a method SecurityDescription to get the SkColor for the
summary_style.
> >
> > Sharing sounds good. iOS seems to have a utility for converting an SkColor
to
> > UIColor[1]; is there something I can use on desktop Mac?
> >
> > [1]
> >
https://cs.chromium.org/chromium/src/skia/ext/skia_utils_ios.mm?q=SkColor+fil...
>
> Did you check skia_utils_mac.h? ;-)
I didn't find it. ;-)
Thanks for the pointer; I've used it.
https://codereview.chromium.org/2107123002/diff/1/chrome/browser/ui/website_s...
File chrome/browser/ui/website_settings/website_settings_ui.cc (right):
https://codereview.chromium.org/2107123002/diff/1/chrome/browser/ui/website_s...
chrome/browser/ui/website_settings/website_settings_ui.cc:128:
security_description->summary_style = summary_style;
On 2016/06/30 at 05:39:24, felt wrote:
> I'm a little confused, where is the summary_style read/used?
In the OS-specific code (website_settings_bubble_controller.mm >
setIdentityInfo:).
https://codereview.chromium.org/2107123002/diff/1/chrome/browser/ui/website_s...
chrome/browser/ui/website_settings/website_settings_ui.cc:191: return
CreateSecurityDescription(IDS_WEBSITE_SETTINGS_NOT_SECURE_SUMMARY,
On 2016/06/30 at 05:39:24, felt wrote:
> Since some of these now share strings and styles, could those be collapsed
into one case, like:
>
> Case A:
> Case B:
> Case C:
> return Foo();
>
> It looks to me like SITE_IDENTITY_STATUS_UNKNOWN,
SITE_IDENTITY_STATUS_ADMIN_PROVIDED_CERT, and
SITE_IDENTITY_STATUS_DEPRECATED_SIGNATURE_ALGORITHM_MAJOR could be combined for
example.
I was planning to collapse these once the strings are final. I've condensed them
in the latest patch based on the current draft strings.
https://codereview.chromium.org/2107123002/diff/1/chrome/browser/ui/website_s...
File chrome/browser/ui/website_settings/website_settings_ui.h (right):
https://codereview.chromium.org/2107123002/diff/1/chrome/browser/ui/website_s...
chrome/browser/ui/website_settings/website_settings_ui.h:126: // Helper to get
the status text style (i.e. color).
On 2016/06/30 at 05:39:24, felt wrote:
> This comment seems mismatched, it is not above a helper method?
Woops, I deleted the old comment instead of a temporary new one. Fixed.
the changes to chrome/browser/ui/website_settings/ look good now, but hesitant to give stamp if there are possibly more changes coming - what are you blocked on there? new strings from UI team?
https://codereview.chromium.org/2107123002/diff/40001/chrome/browser/ui/websi... File chrome/browser/ui/website_settings/website_settings_ui.h (right): https://codereview.chromium.org/2107123002/diff/40001/chrome/browser/ui/websi... chrome/browser/ui/website_settings/website_settings_ui.h:122: // Deprecated method go get just the summary from GetSecurityDescription(). nit: is that a typo? "go get" => "to get"? (and if it's deprecated, why not remove it -- is that for the sake of Views?)
On 2016/07/11 18:47:57, felt wrote: > the changes to chrome/browser/ui/website_settings/ look good now, but hesitant > to give stamp if there are possibly more changes coming - what are you blocked > on there? new strings from UI team? ping on this -- are you blocked on something here?
On 2016/07/11 18:47:57, felt wrote: > the changes to chrome/browser/ui/website_settings/ look good now, but hesitant > to give stamp if there are possibly more changes coming - what are you blocked > on there? new strings from UI team? ping on this -- are you blocked on something here?
rsesek@, could you review forrealz? See https://bugs.chromium.org/p/chromium/issues/detail?id=512442#c57 for screenshots and latest tweak reasons. https://codereview.chromium.org/2107123002/diff/40001/chrome/browser/ui/websi... File chrome/browser/ui/website_settings/website_settings_ui.h (right): https://codereview.chromium.org/2107123002/diff/40001/chrome/browser/ui/websi... chrome/browser/ui/website_settings/website_settings_ui.h:122: // Deprecated method go get just the summary from GetSecurityDescription(). On 2016/07/11 at 18:48:55, felt wrote: > nit: is that a typo? "go get" => "to get"? (and if it's deprecated, why not remove it -- is that for the sake of Views?) Yeah, it's needed for Views. Will fix the typo.
lgarron@chromium.org changed reviewers: + avi@chromium.org
avi@, could you take a look? :-D
https://codereview.chromium.org/2107123002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.h (right): https://codereview.chromium.org/2107123002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.h:55: // This field only shows when there is an acrive certificate exception. active https://codereview.chromium.org/2107123002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.h:59: // This link only shows when there is an acrive certificate exception. active https://codereview.chromium.org/2107123002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.mm (right): https://codereview.chromium.org/2107123002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.mm:968: // position.x += BYEkPermissionPopUpXSpacing; // TODO :( Can you not commit commented-out code?
Patchset #5 (id:100001) has been deleted
FYI, I'm going to split this CL in half (security section update vs. site settings section update). Still teasing apart the two halves, though. https://codereview.chromium.org/2107123002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.h (right): https://codereview.chromium.org/2107123002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.h:55: // This field only shows when there is an acrive certificate exception. On 2016/08/24 at 04:02:41, Avi wrote: > active Whoops, it seems I accidentally uploaded the latest patch to a new CL instead of this one. All three comments should be addressed in patch #5; thanks for noticing! https://codereview.chromium.org/2107123002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.mm (right): https://codereview.chromium.org/2107123002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.mm:968: // position.x += BYEkPermissionPopUpXSpacing; // TODO On 2016/08/24 at 04:02:41, Avi wrote: > :( Can you not commit commented-out code? I most certainly can! I mean, uh, can not. Am able not to. (This is probably an accidental outcome of a refactor. I've removed it.)
Message was sent while issue was closed.
On 2016/08/26 at 23:27:04, lgarron wrote: > FYI, I'm going to split this CL in half (security section update vs. site settings section update). Still teasing apart the two halves, though. > > https://codereview.chromium.org/2107123002/diff/80001/chrome/browser/ui/cocoa... > File chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.h (right): > > https://codereview.chromium.org/2107123002/diff/80001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.h:55: // This field only shows when there is an acrive certificate exception. > On 2016/08/24 at 04:02:41, Avi wrote: > > active > > Whoops, it seems I accidentally uploaded the latest patch to a new CL instead of this one. All three comments should be addressed in patch #5; thanks for noticing! > > https://codereview.chromium.org/2107123002/diff/80001/chrome/browser/ui/cocoa... > File chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.mm (right): > > https://codereview.chromium.org/2107123002/diff/80001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.mm:968: // position.x += BYEkPermissionPopUpXSpacing; // TODO > On 2016/08/24 at 04:02:41, Avi wrote: > > :( Can you not commit commented-out code? > > I most certainly can! I mean, uh, can not. Am able not to. > > (This is probably an accidental outcome of a refactor. I've removed it.) Split into three parts: https://crrev.com/2283753003 Material page info (Mac, 1/3): Update section padding variables. https://crrev.com/2285683003 Material Page Info (Mac, 2/3): Update security section. https://crrev.com/2298963002 Material Page Info (Mac, 3/3): Update site settings section. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
