Add SSL icons on Mac OS X

Add SSL icons on Mac OS X

BUG=14899
TEST=https sites have a lock icon in the address field, plain http sites do not

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

[hawk, 2009-09-10 01:28:28]

I coded this up as part of some other SSL related changes I'm working on, then
discovered there's a specific bug for it, so I broke it off into a smaller CL.
The icons work as they do on Windows; lock icon for secure sites, exclamation
icon for secure sites with some issue (expired cert, untrusted cert, etc.), and
no icon for plain http sites, empty tabs, or while typing in the omnibox.

[rohitrao (ping after 24h), 2009-09-10 14:11:40]

+shess

[Pam (message me for reviews), 2009-09-10 18:04:23]

Before long, we'll also need to draw one or more page-action icons in the
location bar. It would be nice if the code to draw a set of icons there were
factored out so both could use it.

[Scott Hess - ex-Googler, 2009-09-10 18:56:06]

http://codereview.chromium.org/199072/diff/1/2
File chrome/browser/autocomplete/autocomplete_edit_view_mac.mm (right):

http://codereview.chromium.org/199072/diff/1/2#newcode213
Line 213: 
This is in LocationBarView::Update() for Windows and
LocationBarViewGtk::Update() for Linux.  Could we wire it in there for Mac, too?

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

http://codereview.chromium.org/199072/diff/1/4#newcode47
Line 47: scoped_nsobject<NSImage> warningImage_;
This knowledge probably also belongs in LocationBarViewMac, this code doesn't
currently have any knowledge about Chrome internals like ToolbarModel::Icon. 
Also, as Pam noted, as some point we'll want to say "List of images at RHS".  I
think this code would be better to have -setHintImage: or something like that,
comparable to -setHintString:.

http://codereview.chromium.org/199072/diff/1/3
File chrome/browser/cocoa/autocomplete_text_field_cell.mm (right):

http://codereview.chromium.org/199072/diff/1/3#newcode57
Line 57: warningImage_.reset([rb.GetNSImageNamed(IDR_WARNING) retain]);
This puts initialization of these items in the path of opening a new window,
regardless of whether that window every uses the images.  Seems like they should
be lazy.

http://codereview.chromium.org/199072/diff/1/3#newcode173
Line 173: securityIcon_ = icon;
Is the SSL lock exclusive WRT the search hint?

http://codereview.chromium.org/199072/diff/1/3#newcode241
Line 241: } else {
If you has -setHintImage:, then this should become else if (hintImage_), in case
we later wish to add additional types of crap to the autocomplete field.

http://codereview.chromium.org/199072/diff/1/3#newcode352
Line 352: } else {
Hmm.  OK, I'm convinced, even if you make an argument about -setSecurityIcon:,
please either have that method set an image* instance variable, or add a helper
method to fetch the image.  Then the switch() statements can all be consolidated
there, and you can pull the test for whether the image is non-nil out here, in
line with hintString_ and keyword_string_ and their helper methods.

[hawk, 2009-09-10 22:15:20]

http://codereview.chromium.org/199072/diff/1/2
File chrome/browser/autocomplete/autocomplete_edit_view_mac.mm (right):

http://codereview.chromium.org/199072/diff/1/2#newcode213
Line 213: 
On 2009/09/10 18:56:06, shess wrote:
> This is in LocationBarView::Update() for Windows and
> LocationBarViewGtk::Update() for Linux.  Could we wire it in there for Mac,
too?

Done.

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

http://codereview.chromium.org/199072/diff/1/4#newcode47
Line 47: scoped_nsobject<NSImage> warningImage_;
On 2009/09/10 18:56:06, shess wrote:
> This knowledge probably also belongs in LocationBarViewMac, this code doesn't
> currently have any knowledge about Chrome internals like ToolbarModel::Icon. 
> Also, as Pam noted, as some point we'll want to say "List of images at RHS". 
I
> think this code would be better to have -setHintImage: or something like that,
> comparable to -setHintString:.

Done.

http://codereview.chromium.org/199072/diff/1/3
File chrome/browser/cocoa/autocomplete_text_field_cell.mm (right):

http://codereview.chromium.org/199072/diff/1/3#newcode57
Line 57: warningImage_.reset([rb.GetNSImageNamed(IDR_WARNING) retain]);
On 2009/09/10 18:56:06, shess wrote:
> This puts initialization of these items in the path of opening a new window,
> regardless of whether that window every uses the images.  Seems like they
should
> be lazy.

Done.

http://codereview.chromium.org/199072/diff/1/3#newcode173
Line 173: securityIcon_ = icon;
On 2009/09/10 18:56:06, shess wrote:
> Is the SSL lock exclusive WRT the search hint?

Do you mean either the search hint or the icon shows, but never both at the same
time? Yes, and the search hint takes precedence, which mirrors the Windows
behavior.

http://codereview.chromium.org/199072/diff/1/3#newcode241
Line 241: } else {
On 2009/09/10 18:56:06, shess wrote:
> If you has -setHintImage:, then this should become else if (hintImage_), in
case
> we later wish to add additional types of crap to the autocomplete field.

Done.

http://codereview.chromium.org/199072/diff/1/3#newcode352
Line 352: } else {
On 2009/09/10 18:56:06, shess wrote:
> Hmm.  OK, I'm convinced, even if you make an argument about -setSecurityIcon:,
> please either have that method set an image* instance variable, or add a
helper
> method to fetch the image.  Then the switch() statements can all be
consolidated
> there, and you can pull the test for whether the image is non-nil out here, in
> line with hintString_ and keyword_string_ and their helper methods.

Done.

[Scott Hess - ex-Googler, 2009-09-11 20:03:59]

My current understanding is that if there is a keyword string or hint string,
then the icon is ignored.  A difference for this item versus the other
decorations is that you can actually have a keyword string and a hint icon at
the same time.  Right?  If so, that should be commented, somewhere.

Also, SetSecurityIcon() will need to call -resetFieldEditorIfNeeded.  It might
also be worthwhile to have -setHintIcon: only mark for reset if there is no
keyword or hint string, so that we don't reset too frequently.

Also unit-test that when the hint icon is set, a keyword or hint string takes
precedence when generating the text frame.

http://codereview.chromium.org/199072/diff/5001/5002
File chrome/browser/autocomplete/autocomplete_edit_view_mac.mm (right):

http://codereview.chromium.org/199072/diff/5001/5002#newcode18
Line 18: #include "chrome/browser/cocoa/autocomplete_text_field_cell.h"
No longer needed.

http://codereview.chromium.org/199072/diff/5001/5003
File chrome/browser/cocoa/autocomplete_text_field_cell.mm (right):

http://codereview.chromium.org/199072/diff/5001/5003#newcode286
Line 286: if (hintIcon_) {
Use same structure as other -draw helpers.  if() test is in
-drawInteriorWithFrame:, the -draw helper just draws.

http://codereview.chromium.org/199072/diff/5001/5003#newcode299
Line 299: cellFrame.origin.y += (cellFrame.size.height - imageRect.size.height)
/ 2;
Is this fine if the value has a fractional part?  I'm not saying it isn't, just
wanted to make sure it was thought about.

http://codereview.chromium.org/199072/diff/5001/5003#newcode301
Line 301: cellFrame.size.height = imageRect.size.height;
Consolidate with cellFrame.size = imageRect.size.  Or just use -drawAtPoint:* to
draw.

http://codereview.chromium.org/199072/diff/5001/5003#newcode303
Line 303: [hintIcon_ setFlipped:[controlView isFlipped]];
Is this necessary for -drawInRect:*?

[hawk, 2009-09-11 22:26:52]

One new change that's not covered by the comments is that
LocationBarViewMac::OnInputInProgress() is now implemented to update the icon
properly while the user is typing or clicking around the pop-up.

On 2009/09/11 20:03:59, shess wrote:
> My current understanding is that if there is a keyword string or hint string,
> then the icon is ignored.  A difference for this item versus the other
> decorations is that you can actually have a keyword string and a hint icon at
> the same time.  Right?

You can have them set at the same time, but the icon will only draw if there's
no keyword or hint string.

> Also, SetSecurityIcon() will need to call -resetFieldEditorIfNeeded.

Done.

> It might
> also be worthwhile to have -setHintIcon: only mark for reset if there is no
> keyword or hint string, so that we don't reset too frequently.

Done.

> Also unit-test that when the hint icon is set, a keyword or hint string takes
> precedence when generating the text frame.
> 
> http://codereview.chromium.org/199072/diff/5001/5002
> File chrome/browser/autocomplete/autocomplete_edit_view_mac.mm (right):
> 
> http://codereview.chromium.org/199072/diff/5001/5002#newcode18
> Line 18: #include "chrome/browser/cocoa/autocomplete_text_field_cell.h"
> No longer needed.
> 
> http://codereview.chromium.org/199072/diff/5001/5003
> File chrome/browser/cocoa/autocomplete_text_field_cell.mm (right):
> 
> http://codereview.chromium.org/199072/diff/5001/5003#newcode286
> Line 286: if (hintIcon_) {
> Use same structure as other -draw helpers.  if() test is in
> -drawInteriorWithFrame:, the -draw helper just draws.
> 
> http://codereview.chromium.org/199072/diff/5001/5003#newcode299
> Line 299: cellFrame.origin.y += (cellFrame.size.height -
imageRect.size.height)
> / 2;
> Is this fine if the value has a fractional part?  I'm not saying it isn't,
just
> wanted to make sure it was thought about.
> 
> http://codereview.chromium.org/199072/diff/5001/5003#newcode301
> Line 301: cellFrame.size.height = imageRect.size.height;
> Consolidate with cellFrame.size = imageRect.size.  Or just use -drawAtPoint:*
to
> draw.
> 
> http://codereview.chromium.org/199072/diff/5001/5003#newcode303
> Line 303: [hintIcon_ setFlipped:[controlView isFlipped]];
> Is this necessary for -drawInRect:*?

[hawk, 2009-09-11 22:27:01]

http://codereview.chromium.org/199072/diff/5001/5002
File chrome/browser/autocomplete/autocomplete_edit_view_mac.mm (right):

http://codereview.chromium.org/199072/diff/5001/5002#newcode18
Line 18: #include "chrome/browser/cocoa/autocomplete_text_field_cell.h"
On 2009/09/11 20:03:59, shess wrote:
> No longer needed.

Done.

http://codereview.chromium.org/199072/diff/5001/5003
File chrome/browser/cocoa/autocomplete_text_field_cell.mm (right):

http://codereview.chromium.org/199072/diff/5001/5003#newcode286
Line 286: if (hintIcon_) {
On 2009/09/11 20:03:59, shess wrote:
> Use same structure as other -draw helpers.  if() test is in
> -drawInteriorWithFrame:, the -draw helper just draws.

Done.

http://codereview.chromium.org/199072/diff/5001/5003#newcode299
Line 299: cellFrame.origin.y += (cellFrame.size.height - imageRect.size.height)
/ 2;
On 2009/09/11 20:03:59, shess wrote:
> Is this fine if the value has a fractional part?  I'm not saying it isn't,
just
> wanted to make sure it was thought about.

Done.

http://codereview.chromium.org/199072/diff/5001/5003#newcode301
Line 301: cellFrame.size.height = imageRect.size.height;
On 2009/09/11 20:03:59, shess wrote:
> Consolidate with cellFrame.size = imageRect.size.  Or just use -drawAtPoint:*
to
> draw.

Done.

http://codereview.chromium.org/199072/diff/5001/5003#newcode303
Line 303: [hintIcon_ setFlipped:[controlView isFlipped]];
On 2009/09/11 20:03:59, shess wrote:
> Is this necessary for -drawInRect:*?

Yes.

[Scott Hess - ex-Googler, 2009-09-15 18:16:52]

LGTM, with the change indicated below.

http://codereview.chromium.org/199072/diff/7002/1004
File chrome/browser/cocoa/autocomplete_text_field_cell.mm (right):

http://codereview.chromium.org/199072/diff/7002/1004#newcode316
Line 316: } else {
else if (hintIcon_), like the other two cases.

[hawk, 2009-09-15 21:49:17]

http://codereview.chromium.org/199072/diff/7002/1004
File chrome/browser/cocoa/autocomplete_text_field_cell.mm (right):

http://codereview.chromium.org/199072/diff/7002/1004#newcode316
Line 316: } else {
On 2009/09/15 18:16:52, shess wrote:
> else if (hintIcon_), like the other two cases.

Done.