Add EV certificate text to the Mac location bar...

Add EV certificate text to the Mac location bar
BUG=10910
TEST=EV sites (e.g., http://www.paypal.com and most bank sites) get a green text description next to the lock icon in the location bar, non-EV (and non-SSL) sites do not

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=28627

[Scott Hess - ex-Googler, 2009-09-22 18:26:59]

Is this implementing something new?  My Windows Chrome doesn't show a label for
the paypal example.

http://codereview.chromium.org/216031/diff/1/5
File chrome/browser/cocoa/autocomplete_text_field_cell.h (right):

http://codereview.chromium.org/216031/diff/1/5#newcode68
Line 68: - (void)setHintIconLabel:(NSAttributedString*)label;
Use of NSAttributedString is a decision about how to implement drawing, so
please make this an NSString and pull the styling down from LocationBar.  On Mac
this class is all about UI, LocationBar is all about glue.

Actually, your comment on hintIconLabel_ implies that you could really just have
-setHintIcon:label:, and likewise merge the two methods up in LocationBar.

Could you also throw in a TODO: comment around figuring out if we actually need
distinct -setHint* and -setSearchHint* methods?  They are pretty similar, it may
be that either the methods can be merged, or the instance variables can be
merged, or internal implementation details can be factored out.  Given that
you'll probably soon be landing tooltips on these things, probably not
reasonable to do the actual work, yet.

[Aside: ToolbarModel::GetInfoText() checks for entry==NULL after dereferencing
it.  Awesome!]

[hawk, 2009-09-23 19:51:06]

On 2009/09/22 18:26:59, shess wrote:
> Is this implementing something new?  My Windows Chrome doesn't show a label
for
> the paypal example.

It's not new UI, we're matching Windows and Linux. As for why you're not seeing
it, I can think of two reasons: you need to have the right system updates
installed (I didn't see the bar on my XP machine, but it worked fine on my Vista
machine, I didn't spend any time trying to track down why); also, there seems to
be a bug somewhere in the stack that causes resumed SSL sessions to not be
marked as EV (i.e., the first visit in a session to an EV site results in the EV
UI, a refresh makes it go away)

[wtc, 2009-09-23 20:20:30]

hawk: On Windows XP, the "check for server certificate revocation"
box is not checked by default.  EV requires revocation checking.
This is why you didn't see the EV status on your XP machine.

The other issue with resumed SSL sessions not being marked as EV
sounds like a bug.  Off the top of my head, I don't know what causes
it.  Please file a bug report if you see it again.

[Scott Hess - ex-Googler, 2009-09-23 20:30:29]

OK, I can see it with that setting.  Probably worth noting that in the TEST=
=3D.

On Wed, Sep 23, 2009 at 1:20 PM,  <wtc@chromium.org> wrote:
> hawk: On Windows XP, the "check for server certificate revocation"
> box is not checked by default. =A0EV requires revocation checking.
> This is why you didn't see the EV status on your XP machine.
>
> The other issue with resumed SSL sessions not being marked as EV
> sounds like a bug. =A0Off the top of my head, I don't know what causes
> it. =A0Please file a bug report if you see it again.
>
> http://codereview.chromium.org/216031
>

[hawk, 2009-09-24 18:22:23]

http://codereview.chromium.org/216031/diff/1/5
File chrome/browser/cocoa/autocomplete_text_field_cell.h (right):

http://codereview.chromium.org/216031/diff/1/5#newcode68
Line 68: - (void)setHintIconLabel:(NSAttributedString*)label;
On 2009/09/22 18:27:00, shess wrote:
> Use of NSAttributedString is a decision about how to implement drawing, so
> please make this an NSString and pull the styling down from LocationBar.  On
Mac
> this class is all about UI, LocationBar is all about glue.
> 
> Actually, your comment on hintIconLabel_ implies that you could really just
have
> -setHintIcon:label:, and likewise merge the two methods up in LocationBar.

At a minimum, the label text and color need to be pushed down from LocationBar
down to the cell. The font and size are up to the cell. How about:

- (void)setHintIcon:(NSImage*)icon label:(NSString*)label color:(NSColor*)color?

[Scott Hess - ex-Googler, 2009-09-24 20:32:54]

http://codereview.chromium.org/216031/diff/1/5
File chrome/browser/cocoa/autocomplete_text_field_cell.h (right):

http://codereview.chromium.org/216031/diff/1/5#newcode68
Line 68: - (void)setHintIconLabel:(NSAttributedString*)label;
On 2009/09/24 18:22:23, hawk wrote:
> On 2009/09/22 18:27:00, shess wrote:
> > Use of NSAttributedString is a decision about how to implement drawing, so
> > please make this an NSString and pull the styling down from LocationBar.  On
> Mac
> > this class is all about UI, LocationBar is all about glue.
> > 
> > Actually, your comment on hintIconLabel_ implies that you could really just
> have
> > -setHintIcon:label:, and likewise merge the two methods up in LocationBar.
> 
> At a minimum, the label text and color need to be pushed down from LocationBar
> down to the cell. The font and size are up to the cell. How about:
> 
> - (void)setHintIcon:(NSImage*)icon label:(NSString*)label
color:(NSColor*)color?

That sounds good to me, since one could imagine the cell changing the drawing
implementation without changing the visual results.

[hawk, 2009-09-25 18:36:47]

On 2009/09/24 20:32:54, shess wrote:
> http://codereview.chromium.org/216031/diff/1/5
> File chrome/browser/cocoa/autocomplete_text_field_cell.h (right):
> 
> http://codereview.chromium.org/216031/diff/1/5#newcode68
> Line 68: - (void)setHintIconLabel:(NSAttributedString*)label;
> On 2009/09/24 18:22:23, hawk wrote:
> > On 2009/09/22 18:27:00, shess wrote:
> > > Use of NSAttributedString is a decision about how to implement drawing, so
> > > please make this an NSString and pull the styling down from LocationBar. 
On
> > Mac
> > > this class is all about UI, LocationBar is all about glue.
> > > 
> > > Actually, your comment on hintIconLabel_ implies that you could really
just
> > have
> > > -setHintIcon:label:, and likewise merge the two methods up in LocationBar.
> > 
> > At a minimum, the label text and color need to be pushed down from
LocationBar
> > down to the cell. The font and size are up to the cell. How about:
> > 
> > - (void)setHintIcon:(NSImage*)icon label:(NSString*)label
> color:(NSColor*)color?
> 
> That sounds good to me, since one could imagine the cell changing the drawing
> implementation without changing the visual results.

Done. As before, it's possible to have an icon with no label (plain ol' HTTPS
site), but not a label without an icon.

[Scott Hess - ex-Googler, 2009-09-30 00:42:11]

Sorry for the delay, I appear to have let this fall through the cracks.

http://codereview.chromium.org/216031/diff/4003/4004
File chrome/browser/cocoa/autocomplete_text_field_cell.mm (right):

http://codereview.chromium.org/216031/diff/4003/4004#newcode183
Line 183: }
Push clause under if(label).

Honestly, (color ? color : [NSColor blackColor]) would be reasonable, too, I
think.  Your call.

Stupid version: put color, NSForegroundColorAttributeName just before the NULL
line, then it will early terminate if color is nil.  But that's too tricksy and
cute, don't check that in :-).

http://codereview.chromium.org/216031/diff/4003/4004#newcode187
Line 187: ResourceBundle::BaseFont).DeriveFont(-2).nativeFont();
Should this instead be using the control's font adjusted downward 2 points?  I
know that the above phrasing probably matches something from another platform, I
just want to make sure that things are internally consistent.  I logged a bug
around font size for this control, and was told not to adjust it, so it's
already a bit different from other platforms.

http://codereview.chromium.org/216031/diff/4003/4004#newcode211
Line 211: }
Suggest pushing the new-label building code to the top of the method, then
having all of the did-things-change testing down here in one place.  Right now
it's in three places, and I suspect it's more complicated than it needs to be. 
With that change it could be something like:

  if (hintString_ || keywordString_ || icon != hintIcon_ || ![hintLabel_
isEqualToAttributedString:as]) {
    hintString_.reset();
    keywordString_.reset();
    hintIcon_.reset([icon retain]);
    hintLabel_.reset([as retain]);
  }

http://codereview.chromium.org/216031/diff/4003/4005
File chrome/browser/cocoa/location_bar_view_mac.mm (right):

http://codereview.chromium.org/216031/diff/4003/4005#newcode280
Line 280: NSString *icon_label = nil;
No space before *, space after *.

[hawk, 2009-10-03 00:59:21]

http://codereview.chromium.org/216031/diff/4003/4004
File chrome/browser/cocoa/autocomplete_text_field_cell.mm (right):

http://codereview.chromium.org/216031/diff/4003/4004#newcode183
Line 183: }
On 2009/09/30 00:42:11, shess wrote:
> Push clause under if(label).
> 
> Honestly, (color ? color : [NSColor blackColor]) would be reasonable, too, I
> think.  Your call.
> 
> Stupid version: put color, NSForegroundColorAttributeName just before the NULL
> line, then it will early terminate if color is nil.  But that's too tricksy
and
> cute, don't check that in :-).

Done ((color ? color : [NSColor blackColor]), that is).

http://codereview.chromium.org/216031/diff/4003/4004#newcode187
Line 187: ResourceBundle::BaseFont).DeriveFont(-2).nativeFont();
On 2009/09/30 00:42:11, shess wrote:
> Should this instead be using the control's font adjusted downward 2 points?  I
> know that the above phrasing probably matches something from another platform,
I
> just want to make sure that things are internally consistent.  I logged a bug
> around font size for this control, and was told not to adjust it, so it's
> already a bit different from other platforms.

Done.

http://codereview.chromium.org/216031/diff/4003/4004#newcode211
Line 211: }
On 2009/09/30 00:42:11, shess wrote:
> Suggest pushing the new-label building code to the top of the method, then
> having all of the did-things-change testing down here in one place.  Right now
> it's in three places, and I suspect it's more complicated than it needs to be.

> With that change it could be something like:
> 
>   if (hintString_ || keywordString_ || icon != hintIcon_ || ![hintLabel_
> isEqualToAttributedString:as]) {
>     hintString_.reset();
>     keywordString_.reset();
>     hintIcon_.reset([icon retain]);
>     hintLabel_.reset([as retain]);
>   }
> 

Done.

http://codereview.chromium.org/216031/diff/4003/4005
File chrome/browser/cocoa/location_bar_view_mac.mm (right):

http://codereview.chromium.org/216031/diff/4003/4005#newcode280
Line 280: NSString *icon_label = nil;
On 2009/09/30 00:42:11, shess wrote:
> No space before *, space after *.

Done.

[Scott Hess - ex-Googler, 2009-10-06 20:25:05]

http://codereview.chromium.org/216031/diff/11002/17001
File chrome/browser/cocoa/autocomplete_text_field_cell.mm (right):

http://codereview.chromium.org/216031/diff/11002/17001#newcode190
Line 190: (as && ![hintIconLabel_.get() isEqualToAttributedString:as])) {
I think ![hintIconLabel_.get() isEqualToAttributedString:as] is sufficient for
all of the as/hintIconLabel_ tests.  It only mis-fires when both are nil, so if
you want to be pedantic you could additionally test that case.

http://codereview.chromium.org/216031/diff/11002/17004
File chrome/browser/cocoa/autocomplete_text_field_cell_unittest.mm (right):

http://codereview.chromium.org/216031/diff/11002/17004#newcode186
Line 186: EXPECT_FALSE([cell fieldEditorNeedsReset]);
Test transition of label from nil->non-nil and non-nil to nil WRT
-fieldEditorNeedsReset.

I can't think of an easy way to test that nil for color gets you black.  Would
it be reasonable to just DCHECK(color) when label is non-nil, and pass
-blackColor from the caller in the first place?

[hawk, 2009-10-07 21:08:27]

http://codereview.chromium.org/216031/diff/11002/17001
File chrome/browser/cocoa/autocomplete_text_field_cell.mm (right):

http://codereview.chromium.org/216031/diff/11002/17001#newcode190
Line 190: (as && ![hintIconLabel_.get() isEqualToAttributedString:as])) {
On 2009/10/06 20:25:05, shess wrote:
> I think ![hintIconLabel_.get() isEqualToAttributedString:as] is sufficient for
> all of the as/hintIconLabel_ tests.  It only mis-fires when both are nil, so
if
> you want to be pedantic you could additionally test that case.

I added the extra tests because of that case, but mostly because passing nil as
an parameter to isEqualToAttributedString: is undefined (it does happen to work,
though, at least in 10.5.8) so I don't want to call it if !as, and that required
the extra tests.

http://codereview.chromium.org/216031/diff/11002/17004
File chrome/browser/cocoa/autocomplete_text_field_cell_unittest.mm (right):

http://codereview.chromium.org/216031/diff/11002/17004#newcode186
Line 186: EXPECT_FALSE([cell fieldEditorNeedsReset]);
On 2009/10/06 20:25:05, shess wrote:
> Test transition of label from nil->non-nil and non-nil to nil WRT
> -fieldEditorNeedsReset.

Done.

To simplify things WRT to color, I added a DCHECK and noted in the header that
if you give a label, you have to give a color too.

[Scott Hess - ex-Googler, 2009-10-08 18:04:02]

LGTM, change suggested is your call.

http://codereview.chromium.org/216031/diff/11002/17001
File chrome/browser/cocoa/autocomplete_text_field_cell.mm (right):

http://codereview.chromium.org/216031/diff/11002/17001#newcode190
Line 190: (as && ![hintIconLabel_.get() isEqualToAttributedString:as])) {
On 2009/10/07 21:08:28, hawk wrote:
> On 2009/10/06 20:25:05, shess wrote:
> > I think ![hintIconLabel_.get() isEqualToAttributedString:as] is sufficient
for
> > all of the as/hintIconLabel_ tests.  It only mis-fires when both are nil, so
> if
> > you want to be pedantic you could additionally test that case.
> 
> I added the extra tests because of that case, but mostly because passing nil
as
> an parameter to isEqualToAttributedString: is undefined (it does happen to
work,
> though, at least in 10.5.8) so I don't want to call it if !as, and that
required
> the extra tests.

Ick, you're right.  The (as && !hintIconLabel_.get()) check is still redundent. 
If you'd rather keep it, then put it on a separate line so that the (true &&
!false) and (!false && true) cases are more obvious.

[Sorry to harp on this test, it just looks so complicated, it vexes me :-).]