Refactor security-icon code to a more general form

Refactor security-icon code to a more general form, also more consistent with
the Windows implementation, in preparation for implementing page actions.

BUG=14899, 22922, 12281
TEST=unit tests included


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

[Pam (message me for reviews), 2009-10-16 21:31:29]

I pulled out some of the icon-handling code into the location bar. This matches
the Windows and GTK implementations better, but more importantly, it will
support page actions.

[Pam (message me for reviews), 2009-10-19 23:26:16]

Thanks for the review. New patch uploaded. (Patch 8 fixes a comment. See patch 7
for trybot results.)

On Mon, Oct 19, 2009 at 3:55 PM, <shess@chromium.org> wrote:
>
> http://codereview.chromium.org/264037/diff/12001/12004
> File chrome/browser/cocoa/autocomplete_text_field_cell.mm (right):
>
> http://codereview.chromium.org/264037/diff/12001/12004#newcode333
> Line 333: labelWidth = ceil([label size].width) + kIconHorizontalPad;
> Is kHintXOffset -> kIconHorizontalPad intentional?  Code ~30 lines up
> uses kHintXOffset with label.width.

Good catch, thanks. It's back to kHintXOffset.

> http://codereview.chromium.org/264037/diff/12001/12004#newcode345
> Line 345: floor((cellFrame.size.height - imageRect.height) / 2);
> Bring the indent back.

Done.

> http://codereview.chromium.org/264037/diff/12001/12004#newcode410
> Line 410: inView:(NSView*)controlView {
> Why are you passing in |image_view|?  I was thinking maybe it was to
> start generalizing, but since we have hard-coded offsets and stuff, I'm
> not sure it was successful.  Suggest just assuming that this method is
> only called if security_image_view_->IsVisible(), and don't pass the
> image_view down at this time.
>
> Though I suppose -securityImageFrameForFrame: is doing the same thing
> (calling a general function which isn't general yet).

They're both in preparation for page actions, which will stick icons in the same
place and so I expect will be able to use the same padding constants. Might need
a little more reworking when page actions are added, but I wanted to checkpoint
the code with no functional changes.

> http://codereview.chromium.org/264037/diff/12001/12006
> File chrome/browser/cocoa/location_bar_view_mac.mm (right):
>
> http://codereview.chromium.org/264037/diff/12001/12006#newcode357
> Line 357: }
> Sigh.  This is gross.  I'm starting to wish I'd just assumed we should
> redraw every time, and save the optimizations for later.
>
> I think this would read clearer if you had one place which collected the
> goal settings, and another place which took the [visible,image,label]
> tuple and stuffed them in place.  The former code wouldn't have to
> concern itself with tracking dirty, the latter code would be concerned
> only with tracking dirty.
>
> Actually, 90% of the goal of the dirty-tracking has been achieved by a
> recent change to how -resetFieldEditorFrameIfNeeded.  It occurs to me
> that that code could optimize itself to make sure it does nothing visual
> in the no-change case, at which point much of this code can just go
> away.  *poof*.  Suggest you just make ANY change to LocationBarImageView
> a dirty change and get rid of all the cruft.

Done. SetSecurityIcon() now always calls through to -onIconUpdated, relying on
that and -resetFieldEditorFrameIfNeeded for any desired optimization.

- Pam

> http://codereview.chromium.org/264037

[Scott Hess - ex-Googler, 2009-10-21 00:17:15]

I'm getting the hang of this change, now.  I initially felt like it was scary
C++ magic, but now it seems overall reasonable :-).  Mostly down to nit-picking.

http://codereview.chromium.org/295036 is my change to "fix" the needs-reset
tracking.  Should be a straightforward merge (mentioning it just-in-case).

http://codereview.chromium.org/264037/diff/12001/12004
File chrome/browser/cocoa/autocomplete_text_field_cell.mm (right):

http://codereview.chromium.org/264037/diff/12001/12004#newcode333
Line 333: labelWidth = ceil([label size].width) + kIconHorizontalPad;
Is kHintXOffset -> kIconHorizontalPad intentional?  Code ~30 lines up uses
kHintXOffset with label.width.

http://codereview.chromium.org/264037/diff/12001/12004#newcode345
Line 345: floor((cellFrame.size.height - imageRect.height) / 2);
Bring the indent back.

http://codereview.chromium.org/264037/diff/12001/12004#newcode410
Line 410: inView:(NSView*)controlView {
Why are you passing in |image_view|?  I was thinking maybe it was to start
generalizing, but since we have hard-coded offsets and stuff, I'm not sure it
was successful.  Suggest just assuming that this method is only called if
security_image_view_->IsVisible(), and don't pass the image_view down at this
time.

Though I suppose -securityImageFrameForFrame: is doing the same thing (calling a
general function which isn't general yet).

http://codereview.chromium.org/264037/diff/12001/12006
File chrome/browser/cocoa/location_bar_view_mac.mm (right):

http://codereview.chromium.org/264037/diff/12001/12006#newcode357
Line 357: }
Sigh.  This is gross.  I'm starting to wish I'd just assumed we should redraw
every time, and save the optimizations for later.

I think this would read clearer if you had one place which collected the goal
settings, and another place which took the [visible,image,label] tuple and
stuffed them in place.  The former code wouldn't have to concern itself with
tracking dirty, the latter code would be concerned only with tracking dirty.

Actually, 90% of the goal of the dirty-tracking has been achieved by a recent
change to how -resetFieldEditorFrameIfNeeded.  It occurs to me that that code
could optimize itself to make sure it does nothing visual in the no-change case,
at which point much of this code can just go away.  *poof*.  Suggest you just
make ANY change to LocationBarImageView a dirty change and get rid of all the
cruft.

http://codereview.chromium.org/264037/diff/20004/20010
File chrome/browser/cocoa/autocomplete_text_field_cell.h (right):

http://codereview.chromium.org/264037/diff/20004/20010#newcode41
Line 41: LocationBarViewMac::SecurityImageView* security_image_view_;
Add a comment on the order of // weak, owned by locationbar.  Just so that
people don't think it's locally owned or something.

http://codereview.chromium.org/264037/diff/20004/20007
File chrome/browser/cocoa/autocomplete_text_field_cell.mm (right):

http://codereview.chromium.org/264037/diff/20004/20007#newcode6
Line 6: #import "chrome/browser/cocoa/location_bar_view_mac.h"
Redundant.

http://codereview.chromium.org/264037/diff/20004/20007#newcode238
Line 238: security_image_view_->OnMousePressed();
It occurs to me that you should either call it a click or a mouse-press, or make
it OnSomethingElseEntirely().  [I like OnClicked().]

http://codereview.chromium.org/264037/diff/20004/20006
File chrome/browser/cocoa/location_bar_view_mac.h (right):

http://codereview.chromium.org/264037/diff/20004/20006#newcode94
Line 94: virtual void SetImage(NSImage* image);
Is there any reason for SetImage(), SetVisible(), or SetLabel() to be virtual at
this time?  As currently presented, it seems you are separating things into
container-for-common-stuff and extra-functionality-setting-common-stuff.  That
seems reasonable to me.

http://codereview.chromium.org/264037/diff/20004/20006#newcode100
Line 100: // |font| are ignored if |text| is NULL.
Use nil for objects rather than NULL.

http://codereview.chromium.org/264037/diff/20004/20006#newcode105
Line 105: const NSAttributedString* GetLabel() { return label_; }
const on all of these?  I mean after the ()?

http://codereview.chromium.org/264037/diff/20004/20006#newcode113
Line 113: // The label shown next to the icon, or NULL if none.
s/NULL/nil/;

http://codereview.chromium.org/264037/diff/20004/20006#newcode116
Line 116: DISALLOW_COPY_AND_ASSIGN(LocationBarImageView);
Indentation.

http://codereview.chromium.org/264037/diff/20004/20009
File chrome/browser/cocoa/location_bar_view_mac.mm (right):

http://codereview.chromium.org/264037/diff/20004/20009#newcode324
Line 324: }
With your change, you could probably make this read cleaner by pushing |color|
and |label| and SetLabel() into the if(), and having an else clause call
SetLabel(nil, nil, nil).

http://codereview.chromium.org/264037/diff/20004/20009#newcode326
Line 326: security_image_view_.SetLabel(icon_label, [cell font], color);
[field_ font] should work the same as [cell font] in this case.

http://codereview.chromium.org/264037/diff/20004/20009#newcode329
Line 329: void LocationBarViewMac::SetSecurityIcon(ToolbarModel::Icon icon) {
I'm not really feeling the need for breaking this out into SetSecurityIcon()
versus SetSecurityIconLabel().  It seems to imply that they are somehow
independent, but they don't really seem to be independent at all.

Likewise for caching the LOCK and WARNING icons and using SetImageShown() to
select them.  Everything is always one-to-one, so it just feels like an extra
layer of indirection to no great end.

http://codereview.chromium.org/264037/diff/20004/20009#newcode378
Line 378: NULL];
Opening [ should indent under c in NSDict.  These long objective-c calls are
just annoying to write!

http://codereview.chromium.org/264037/diff/20004/20009#newcode383
Line 383: label_.reset([attrStr retain]);
Rather than this, how about label_.reset() above the if(), then pass the
attributed string to reset() directly w/o the -autorelease?

[Pam (message me for reviews), 2009-10-21 22:23:39]

Thanks, new patch coming momentarily.

http://codereview.chromium.org/264037/diff/20004/20010
File chrome/browser/cocoa/autocomplete_text_field_cell.h (right):

http://codereview.chromium.org/264037/diff/20004/20010#newcode41
Line 41: LocationBarViewMac::SecurityImageView* security_image_view_;
On 2009/10/21 00:17:15, shess wrote:
> Add a comment on the order of // weak, owned by locationbar.  Just so that
> people don't think it's locally owned or something.

Is the comment on line 38 sufficient?

http://codereview.chromium.org/264037/diff/20004/20007
File chrome/browser/cocoa/autocomplete_text_field_cell.mm (right):

http://codereview.chromium.org/264037/diff/20004/20007#newcode238
Line 238: security_image_view_->OnMousePressed();
On 2009/10/21 00:17:15, shess wrote:
> It occurs to me that you should either call it a click or a mouse-press, or
make
> it OnSomethingElseEntirely().  [I like OnClicked().]

I prefer OnClicked() intrinsically too, but I'll keep OnMousePressed() to match
other platforms.

http://codereview.chromium.org/264037/diff/20004/20006
File chrome/browser/cocoa/location_bar_view_mac.h (right):

http://codereview.chromium.org/264037/diff/20004/20006#newcode100
Line 100: // |font| are ignored if |text| is NULL.
On 2009/10/21 00:17:15, shess wrote:
> Use nil for objects rather than NULL.

Done. Should I also be using BOOL and YES/NO instead of bool and true/false?

http://codereview.chromium.org/264037/diff/20004/20009
File chrome/browser/cocoa/location_bar_view_mac.mm (right):

http://codereview.chromium.org/264037/diff/20004/20009#newcode329
Line 329: void LocationBarViewMac::SetSecurityIcon(ToolbarModel::Icon icon) {
On 2009/10/21 00:17:15, shess wrote:
> I'm not really feeling the need for breaking this out into SetSecurityIcon()
> versus SetSecurityIconLabel().  It seems to imply that they are somehow
> independent, but they don't really seem to be independent at all.

SetSecurityIconLabel() is just a convenience function so the code for finding
the label, font, and color is neither duplicated nor run unnecessarily. I can
make it a file static if that's clearer.

> Likewise for caching the LOCK and WARNING icons and using SetImageShown() to
> select them.  Everything is always one-to-one, so it just feels like an extra
> layer of indirection to no great end.

It doesn't feel right to have the LocationBarViewMac know details of the lock
images themselves. This also keeps consistency with the Windows implementation
-- not that that couldn't be changed to match. What do you propose?

http://codereview.chromium.org/264037/diff/20004/20009#newcode378
Line 378: NULL];
On 2009/10/21 00:17:15, shess wrote:
> Opening [ should indent under c in NSDict.  These long objective-c calls are
> just annoying to write!

Gah, copy-paste in Xcode keeps trampling my indentation. Is there some setting I
can change to make it stop?

http://codereview.chromium.org/264037/diff/20004/20009#newcode383
Line 383: label_.reset([attrStr retain]);
On 2009/10/21 00:17:15, shess wrote:
> Rather than this, how about label_.reset() above the if(), then pass the
> attributed string to reset() directly w/o the -autorelease?

Let me verify my understanding. In the old code, reset() releases the old string
and retains the new one behind the scenes, and we add another
retain/(auto)release pair manually so the object survives past the end of the
block. The new code simply drops the redundant retain/release. Right?

[Scott Hess - ex-Googler, 2009-10-21 23:27:19]

My change which makes -resetFieldEditorIfNeeded work without having to set the
cell's flag is in, so that part should be good-to-go.

http://codereview.chromium.org/264037/diff/20004/20010
File chrome/browser/cocoa/autocomplete_text_field_cell.h (right):

http://codereview.chromium.org/264037/diff/20004/20010#newcode41
Line 41: LocationBarViewMac::SecurityImageView* security_image_view_;
On 2009/10/21 22:23:39, Pam wrote:
> On 2009/10/21 00:17:15, shess wrote:
> > Add a comment on the order of // weak, owned by locationbar.  Just so that
> > people don't think it's locally owned or something.
> 
> Is the comment on line 38 sufficient?

Oooh.  Maybe I wasn't paying attention.

http://codereview.chromium.org/264037/diff/20004/20007
File chrome/browser/cocoa/autocomplete_text_field_cell.mm (right):

http://codereview.chromium.org/264037/diff/20004/20007#newcode238
Line 238: security_image_view_->OnMousePressed();
On 2009/10/21 22:23:39, Pam wrote:
> On 2009/10/21 00:17:15, shess wrote:
> > It occurs to me that you should either call it a click or a mouse-press, or
> make
> > it OnSomethingElseEntirely().  [I like OnClicked().]
> 
> I prefer OnClicked() intrinsically too, but I'll keep OnMousePressed() to
match
> other platforms.

You make a compelling argument.

http://codereview.chromium.org/264037/diff/20004/20006
File chrome/browser/cocoa/location_bar_view_mac.h (right):

http://codereview.chromium.org/264037/diff/20004/20006#newcode100
Line 100: // |font| are ignored if |text| is NULL.
On 2009/10/21 22:23:39, Pam wrote:
> On 2009/10/21 00:17:15, shess wrote:
> > Use nil for objects rather than NULL.
> 
> Done. Should I also be using BOOL and YES/NO instead of bool and true/false?

If you're in Objective-C code, definitely BOOL and YES/NO.  If you're in the
interface between, best judgement.  I think a member variable in a C++ object is
probably best as a bool, unless you mostly plan to pass it to BOOL.

http://codereview.chromium.org/264037/diff/20004/20009
File chrome/browser/cocoa/location_bar_view_mac.mm (right):

http://codereview.chromium.org/264037/diff/20004/20009#newcode329
Line 329: void LocationBarViewMac::SetSecurityIcon(ToolbarModel::Icon icon) {
On 2009/10/21 22:23:39, Pam wrote:
> On 2009/10/21 00:17:15, shess wrote:
> > I'm not really feeling the need for breaking this out into SetSecurityIcon()
> > versus SetSecurityIconLabel().  It seems to imply that they are somehow
> > independent, but they don't really seem to be independent at all.
> 
> SetSecurityIconLabel() is just a convenience function so the code for finding
> the label, font, and color is neither duplicated nor run unnecessarily. I can
> make it a file static if that's clearer.

Hmm.  OK, I guess that seems reasonable.  Note that you only need to find the
info when ToolbarModel::INFO_EV_TEXT, so I don't think this saves much.

> > Likewise for caching the LOCK and WARNING icons and using SetImageShown() to
> > select them.  Everything is always one-to-one, so it just feels like an
extra
> > layer of indirection to no great end.
> 
> It doesn't feel right to have the LocationBarViewMac know details of the lock
> images themselves. This also keeps consistency with the Windows implementation
> -- not that that couldn't be changed to match. What do you propose?

Consistency is good, I guess stick with bridge code between
ToolbarModel::LOCK_ICON and SecurityImageView::LOCK and kin.

Would it be reasonable to have SecurityImageView export a LockImage() static
instead?  Why I ask is because the current code requires us to retain both
images in all cases, even if they are not needed.  Using a static function would
give the same benefits as currently, with lazy creation.

[So I mean like SetImageShown(SecurityImageView::LockImage());]

http://codereview.chromium.org/264037/diff/20004/20009#newcode378
Line 378: NULL];
On 2009/10/21 22:23:39, Pam wrote:
> On 2009/10/21 00:17:15, shess wrote:
> > Opening [ should indent under c in NSDict.  These long objective-c calls are
> > just annoying to write!
> 
> Gah, copy-paste in Xcode keeps trampling my indentation. Is there some setting
I
> can change to make it stop?

I use emacs and have an in-my-brain macro for going to lines and hitting TAB
repeatedly.

http://codereview.chromium.org/264037/diff/20004/20009#newcode383
Line 383: label_.reset([attrStr retain]);
On 2009/10/21 22:23:39, Pam wrote:
> On 2009/10/21 00:17:15, shess wrote:
> > Rather than this, how about label_.reset() above the if(), then pass the
> > attributed string to reset() directly w/o the -autorelease?
> 
> Let me verify my understanding. In the old code, reset() releases the old
string
> and retains the new one behind the scenes, and we add another
> retain/(auto)release pair manually so the object survives past the end of the
> block. The new code simply drops the redundant retain/release. Right?

I do not know.  I'm just looking at initializing with nil, then making it an
-autoreleased value, then stuffing it into label_ with a retain.  Everything is
correct, but since attrStr will always overwrite label_, it just seems easier to
blow it away right up front.

[Pam (message me for reviews), 2009-10-22 19:30:40]

On 2009/10/21 23:27:19, shess wrote:

> Would it be reasonable to have SecurityImageView export a LockImage() static
> instead?  Why I ask is because the current code requires us to retain both
> images in all cases, even if they are not needed.  Using a static function
would
> give the same benefits as currently, with lazy creation.
> 
> [So I mean like SetImageShown(SecurityImageView::LockImage());]

It seems odd to me to call a class method with an argument that's part of that
same class. How about this (see patch)? Still indirect through the enum, but now
loaded lazily.

- Pam

[Scott Hess - ex-Googler, 2009-10-22 20:26:56]

On 2009/10/22 19:30:40, Pam wrote:
> On 2009/10/21 23:27:19, shess wrote:
> 
> > Would it be reasonable to have SecurityImageView export a LockImage() static
> > instead?  Why I ask is because the current code requires us to retain both
> > images in all cases, even if they are not needed.  Using a static function
> would
> > give the same benefits as currently, with lazy creation.
> > 
> > [So I mean like SetImageShown(SecurityImageView::LockImage());]
> 
> It seems odd to me to call a class method with an argument that's part of that
> same class.

security_image_view_.SetImage(SecurityImageView::LockImage());
   vs
security_image_view_.SetImageShown(SecurityImageView::LOCK);

> How about this (see patch)? Still indirect through the enum, but now
> loaded lazily.

LGTM, my main goal was the lazy part.

Thanks for putting up with me,
scott