[Mac] First part of Omnibox decoration refactor. Enable ev bubble.

[Mac] First part of Omnibox decoration refactor.  Enable ev bubble.

The views implementation of the little things floating around in the
Omnibox uses nested views which are manually laid out by the
location-bar code.  The Mac code has a variety of different
implementations for these items, with the layout distributed across
many files.  This change creates something called a "decoration" which
is similar (different name because Mac has a strong sense of what a
"view" is).  Decorations are kind of like a C++ NSCell, which the
AutocompleteTextFieldCell handles generically and the
LocationBarViewMac::Layout() code will lay out.  The overall goal is
to loosely parallel the views code for decorations and Layout(), so
that future coders can do the right thing more easily.

This CL converts the left-hand items, namely:
 - the location icon
 - the search-keyword bubble
 - the ev security bubble (new)
The Layout() function does not yet deal with trimming things to fit
when space is tight.

location_bar_view_mac_unittest.mm was always a sham, so I'm going to
stop pretending.

BUG=41998
TEST=EV bubble for secure sites.
TEST=location icon can be clicked (page info panel) and dragged.
TEST=EV bubble can be clicked (page info panel) and dragged.
TEST=keyword-search mode shows appropriately.

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

[rohitrao (ping after 24h), 2010-07-08 13:38:06]

So much code!  I made it through half of it, will look at the autocomplete_*
classes in a bit.

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

http://codereview.chromium.org/2805070/diff/1/4#newcode92
chrome/browser/cocoa/location_bar/autocomplete_text_field_cell.h:92: -
(void)addLeftDecoration:(LocationBarDecoration*)decoration;
Will newly-added decorations appear to the left or the right of the existing
decorations?

http://codereview.chromium.org/2805070/diff/1/8
File chrome/browser/cocoa/location_bar/bubble_decoration.h (right):

http://codereview.chromium.org/2805070/diff/1/8#newcode19
chrome/browser/cocoa/location_bar/bubble_decoration.h:19: virtual bool
IsVisible() const {
It might make sense to move these two Visible functions into the base class,
since both subclasses implement them the same way.

http://codereview.chromium.org/2805070/diff/1/8#newcode28
chrome/browser/cocoa/location_bar/bubble_decoration.h:28: CGFloat
GetWidthForImageAndLabel(NSImage* image, NSString* label);
Why is this public instead of protected?

http://codereview.chromium.org/2805070/diff/1/10
File chrome/browser/cocoa/location_bar/ev_bubble_decoration.h (right):

http://codereview.chromium.org/2805070/diff/1/10#newcode14
chrome/browser/cocoa/location_bar/ev_bubble_decoration.h:14: // TODO(shess):
Better name, moron.
I really have no idea what an EV bubble is.

http://codereview.chromium.org/2805070/diff/1/10#newcode15
chrome/browser/cocoa/location_bar/ev_bubble_decoration.h:15: class
EVBubbleDecoration : public BubbleDecoration {
Can you add a class-level comment?  I don't really understand how this class is
supposed to be used, or why |location_icon| even exists.  Is |location_icon|
supposed to be added to the set of decorations separately?  Otherwise, what's
the point of having an icon that never draws itself?

http://codereview.chromium.org/2805070/diff/1/12
File chrome/browser/cocoa/location_bar/location_bar_decoration.h (right):

http://codereview.chromium.org/2805070/diff/1/12#newcode41
chrome/browser/cocoa/location_bar/location_bar_decoration.h:41: // The
pasteboard to drag.
We're not actually dragging the pasteboard, right?  Dragging to the pasteboard,
maybe?

What is a drag pasteboard, anyways? =)

http://codereview.chromium.org/2805070/diff/1/13
File chrome/browser/cocoa/location_bar/location_bar_decoration.mm (right):

http://codereview.chromium.org/2805070/diff/1/13#newcode18
chrome/browser/cocoa/location_bar/location_bar_decoration.mm:18: NOTREACHED();
Why are these two functions implemented instead of being pure virtual?  Do we
ever create an instance of LocationBarDecoration (as opposed to a subclass)?

http://codereview.chromium.org/2805070/diff/1/18
File chrome/browser/cocoa/location_bar/location_icon_decoration.mm (right):

http://codereview.chromium.org/2805070/diff/1/18#newcode63
chrome/browser/cocoa/location_bar/location_icon_decoration.mm:63: DCHECK(tab);
Do you also want to "if (!tab) return nil;"?

http://codereview.chromium.org/2805070/diff/1/19
File chrome/browser/cocoa/location_bar/selected_keyword_decoration.h (right):

http://codereview.chromium.org/2805070/diff/1/19#newcode20
chrome/browser/cocoa/location_bar/selected_keyword_decoration.h:20: void
SetKeyword(const std::wstring& keyword, bool is_extension_keyword);
This can't be string16-ified?

http://codereview.chromium.org/2805070/diff/1/20
File chrome/browser/cocoa/location_bar/selected_keyword_decoration.mm (right):

http://codereview.chromium.org/2805070/diff/1/20#newcode56
chrome/browser/cocoa/location_bar/selected_keyword_decoration.mm:56: // The text
will be like "Search Engine:".  "Engine" is a parameter
s/Engine/myEngine/, or maybe theEngine.

[rohitrao (ping after 24h), 2010-07-09 01:32:01]

Nothing too exciting.

http://codereview.chromium.org/2805070/diff/1/3
File chrome/browser/cocoa/location_bar/autocomplete_text_field.mm (right):

http://codereview.chromium.org/2805070/diff/1/3#newcode210
chrome/browser/cocoa/location_bar/autocomplete_text_field.mm:210: // stuff? 
Sigh.
Right now we're just showing an I-beam over the LHS decorations?

http://codereview.chromium.org/2805070/diff/1/5
File chrome/browser/cocoa/location_bar/autocomplete_text_field_cell.mm (right):

http://codereview.chromium.org/2805070/diff/1/5#newcode106
chrome/browser/cocoa/location_bar/autocomplete_text_field_cell.mm:106: // is
included in their origins).
I don't understand the statement in parens.

http://codereview.chromium.org/2805070/diff/1/5#newcode369
chrome/browser/cocoa/location_bar/autocomplete_text_field_cell.mm:369: // Find
our decoration and return the corrosponding frame.
Typo: corrosponding

http://codereview.chromium.org/2805070/diff/1/5#newcode389
chrome/browser/cocoa/location_bar/autocomplete_text_field_cell.mm:389: //
NOTE(shess): This will change once everything is a decoration.
I sort of dislike using NULL to mean "text frame."  When you say everything will
be a decoration, do you really mean everything?  The text frame too?

http://codereview.chromium.org/2805070/diff/1/5#newcode639
chrome/browser/cocoa/location_bar/autocomplete_text_field_cell.mm:639: 
Some part of me wants a DCHECK(decoration xor icon) =)

http://codereview.chromium.org/2805070/diff/1/5#newcode650
chrome/browser/cocoa/location_bar/autocomplete_text_field_cell.mm:650:
(decoration && decoration->IsDraggable())) {
This is all ugly, but the future will be better.

http://codereview.chromium.org/2805070/diff/1/6
File chrome/browser/cocoa/location_bar/autocomplete_text_field_cell_unittest.mm
(right):

http://codereview.chromium.org/2805070/diff/1/6#newcode126
chrome/browser/cocoa/location_bar/autocomplete_text_field_cell_unittest.mm:126:
// TODO(shess): This really wants a |LocationBarViewMac|, but only a
Can we invent a LocationBarDecorationDelegate?

[Scott Hess - ex-Googler, 2010-07-09 07:40:12]

Yeah, this was quite a chunk.  I wished mightily to get a more approachable
core, but the RHS items involved too much open-heart-surgery, while doing only
one of the LHS items involved too much hacking on the magic visibility stuff in
the cell.  Thanks for grinding through it!

http://codereview.chromium.org/2805070/diff/1/3
File chrome/browser/cocoa/location_bar/autocomplete_text_field.mm (right):

http://codereview.chromium.org/2805070/diff/1/3#newcode210
chrome/browser/cocoa/location_bar/autocomplete_text_field.mm:210: // stuff? 
Sigh.
On 2010/07/09 01:32:01, rohitrao wrote:
> Right now we're just showing an I-beam over the LHS decorations?

Yeah.  I'm still noodling on how to reconcile this without needing this code to
specifically understand which decoration is which.  I'm considering adding a
HandlesMouseDown() decoration function and use that to indicate arrow cursor.

I'm also tempted to fix the cursor-twitch stuff by just having an ibeam over the
central area where clicking will cause a text selection, and not tweaking the
cursor at all outside that.

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

http://codereview.chromium.org/2805070/diff/1/4#newcode92
chrome/browser/cocoa/location_bar/autocomplete_text_field_cell.h:92: -
(void)addLeftDecoration:(LocationBarDecoration*)decoration;
On 2010/07/08 13:38:06, rohitrao wrote:
> Will newly-added decorations appear to the left or the right of the existing
> decorations?

Added comment.

http://codereview.chromium.org/2805070/diff/1/5
File chrome/browser/cocoa/location_bar/autocomplete_text_field_cell.mm (right):

http://codereview.chromium.org/2805070/diff/1/5#newcode106
chrome/browser/cocoa/location_bar/autocomplete_text_field_cell.mm:106: // is
included in their origins).
On 2010/07/09 01:32:01, rohitrao wrote:
> I don't understand the statement in parens.

What I meant was that the returned rects are not relative to |cell_frame|, they
are at the same level as it.  Does the parenthetical comment just confuse the
issue more?

http://codereview.chromium.org/2805070/diff/1/5#newcode369
chrome/browser/cocoa/location_bar/autocomplete_text_field_cell.mm:369: // Find
our decoration and return the corrosponding frame.
On 2010/07/09 01:32:01, rohitrao wrote:
> Typo: corrosponding

Done.

http://codereview.chromium.org/2805070/diff/1/5#newcode389
chrome/browser/cocoa/location_bar/autocomplete_text_field_cell.mm:389: //
NOTE(shess): This will change once everything is a decoration.
On 2010/07/09 01:32:01, rohitrao wrote:
> I sort of dislike using NULL to mean "text frame."  When you say everything
will
> be a decoration, do you really mean everything?  The text frame too?

It's just a convenience for CalculatePositionsInFrame().  I'll modify it to
return the remainder by reference.

http://codereview.chromium.org/2805070/diff/1/6
File chrome/browser/cocoa/location_bar/autocomplete_text_field_cell_unittest.mm
(right):

http://codereview.chromium.org/2805070/diff/1/6#newcode126
chrome/browser/cocoa/location_bar/autocomplete_text_field_cell_unittest.mm:126:
// TODO(shess): This really wants a |LocationBarViewMac|, but only a
On 2010/07/09 01:32:01, rohitrao wrote:
> Can we invent a LocationBarDecorationDelegate?

I would kind of rather not :-).

One thing I toyed with was passing callbacks to provide the "meat" of things. 
That would make the code very generic, but would also push all of the
implementation back into LocationBarViewMac.

Per my earlier musing, views has a ClickHandler class to handle probing the
location bar, which is then shared between location icon and ev bubble.  I guess
that might make sense, here, though I don't think it really resolves the central
issue...

http://codereview.chromium.org/2805070/diff/1/8
File chrome/browser/cocoa/location_bar/bubble_decoration.h (right):

http://codereview.chromium.org/2805070/diff/1/8#newcode19
chrome/browser/cocoa/location_bar/bubble_decoration.h:19: virtual bool
IsVisible() const {
On 2010/07/08 13:38:06, rohitrao wrote:
> It might make sense to move these two Visible functions into the base class,
> since both subclasses implement them the same way.

I was mixed as to whether it was appropriate to have state in an abstract base
class.  But so far I can't think of any decoration which doesn't want the
ability to be set to not visible, so I'll push it up.

http://codereview.chromium.org/2805070/diff/1/8#newcode28
chrome/browser/cocoa/location_bar/bubble_decoration.h:28: CGFloat
GetWidthForImageAndLabel(NSImage* image, NSString* label);
On 2010/07/08 13:38:06, rohitrao wrote:
> Why is this public instead of protected?

Because it didn't occur to me.  Done.

http://codereview.chromium.org/2805070/diff/1/10
File chrome/browser/cocoa/location_bar/ev_bubble_decoration.h (right):

http://codereview.chromium.org/2805070/diff/1/10#newcode14
chrome/browser/cocoa/location_bar/ev_bubble_decoration.h:14: // TODO(shess):
Better name, moron.
On 2010/07/08 13:38:06, rohitrao wrote:
> I really have no idea what an EV bubble is.

Oops.  Still haven't thought of a better name which isn't horrible.  And views
uses something similar, so people making changes there will be "comfortable".

http://codereview.chromium.org/2805070/diff/1/10#newcode15
chrome/browser/cocoa/location_bar/ev_bubble_decoration.h:15: class
EVBubbleDecoration : public BubbleDecoration {
On 2010/07/08 13:38:06, rohitrao wrote:
> Can you add a class-level comment?  I don't really understand how this class
is
> supposed to be used, or why |location_icon| even exists.  Is |location_icon|
> supposed to be added to the set of decorations separately?  Otherwise, what's
> the point of having an icon that never draws itself?

The drawing is inherited from the superclass and this class sets up the
customizations in the constructor.  The |location_icon| reference is to share
functionality (this decoration replaces the location icon when appropriate). 
I've added a class-level comment, but you're making me think I should refactor
the part of |location_icon| I want to share and put it somewhere else, like some
LocationBarViewMac methods.

After reading the style guidelines to inform the earlier comment (pushing
visible_ into base decoration class), I'm thinking I've got a
non-style-compliant breakdown, here.  The ev-bubble and keyword-bubble should
perhaps contain a bubble-drawing member instead.  Against that, the views
implementation has this same hierarchy.  WDYT?

[I don't think it would be hard to refactor the bubble-handling code out, it's
already almost written that way anyhow.]

http://codereview.chromium.org/2805070/diff/1/12
File chrome/browser/cocoa/location_bar/location_bar_decoration.h (right):

http://codereview.chromium.org/2805070/diff/1/12#newcode41
chrome/browser/cocoa/location_bar/location_bar_decoration.h:41: // The
pasteboard to drag.
On 2010/07/08 13:38:06, rohitrao wrote:
> We're not actually dragging the pasteboard, right?  Dragging to the
pasteboard,
> maybe?
> 
> What is a drag pasteboard, anyways? =)

I guess I didn't parse that.  You're visually dragging an image with an
associated NSPasteboard named NSDragPboard which contains the logical stuff that
you're dragging.

http://codereview.chromium.org/2805070/diff/1/13
File chrome/browser/cocoa/location_bar/location_bar_decoration.mm (right):

http://codereview.chromium.org/2805070/diff/1/13#newcode18
chrome/browser/cocoa/location_bar/location_bar_decoration.mm:18: NOTREACHED();
On 2010/07/08 13:38:06, rohitrao wrote:
> Why are these two functions implemented instead of being pure virtual?  Do we
> ever create an instance of LocationBarDecoration (as opposed to a subclass)?

Honestly?  Because when I forgot to implement them in a subclass, the error in
xcode was completely useless.  I think it was a link error which told me
absolutely nothing about what I had forgotten, and maybe referenced a
mis-leading class or something.  Once I needed a .mm to put kOmittedWidth, I
decided using NOTREACHED() would work better to signal that (and it also works
better for mocks in tests which don't hit those functions in the first place). 
I'll go re-construct what I was seeing and drop a comment to that end here.

http://codereview.chromium.org/2805070/diff/1/18
File chrome/browser/cocoa/location_bar/location_icon_decoration.mm (right):

http://codereview.chromium.org/2805070/diff/1/18#newcode63
chrome/browser/cocoa/location_bar/location_icon_decoration.mm:63: DCHECK(tab);
On 2010/07/08 13:38:06, rohitrao wrote:
> Do you also want to "if (!tab) return nil;"?

I think we've had this discussion before.  I _want_ to make it a CHECK().  The
various implementations seem to sometimes check NULL and take action, and other
times dereference without checking, and there doesn't seem to be any clear
rationale for one versus the other.  I'm pretty sure all the checks are
extraneous, that you can't get here in the first place when !tab.

Or maybe I had this discussion with Peter.

I'm going to push a test up to IsDraggable().  That way the caller won't even
get to here if there is no tab.

http://codereview.chromium.org/2805070/diff/1/19
File chrome/browser/cocoa/location_bar/selected_keyword_decoration.h (right):

http://codereview.chromium.org/2805070/diff/1/19#newcode20
chrome/browser/cocoa/location_bar/selected_keyword_decoration.h:20: void
SetKeyword(const std::wstring& keyword, bool is_extension_keyword);
On 2010/07/08 13:38:06, rohitrao wrote:
> This can't be string16-ified?

It is implemented in terms of location_bar_util.h, which calls into the template
url model, which wants wstring.  Likewise the caller is calling things which
return wstring.  So ... I guess someone has to draw the line.  I could make this
string16 and make conversions.  But while that feels good, I'm not entirely sure
it improves the world.

["Improves the world" might look more like a CL which starts at
GetKeywordShortName() and unwinds up the callers for all implementations, so
that a future developer doesn't get confused by egregious implementation
differences.]

http://codereview.chromium.org/2805070/diff/1/20
File chrome/browser/cocoa/location_bar/selected_keyword_decoration.mm (right):

http://codereview.chromium.org/2805070/diff/1/20#newcode56
chrome/browser/cocoa/location_bar/selected_keyword_decoration.mm:56: // The text
will be like "Search Engine:".  "Engine" is a parameter
On 2010/07/08 13:38:06, rohitrao wrote:
> s/Engine/myEngine/, or maybe theEngine.

Revised to <name> and whacked the comment a bit.

[Scott Hess - ex-Googler, 2010-07-12 19:36:10]

ping?  I can't deliver the other scary CLs without getting this one into the
can!

[rohitrao (ping after 24h), 2010-07-12 19:50:38]

LGTM!

http://codereview.chromium.org/2805070/diff/1/5
File chrome/browser/cocoa/location_bar/autocomplete_text_field_cell.mm (right):

http://codereview.chromium.org/2805070/diff/1/5#newcode106
chrome/browser/cocoa/location_bar/autocomplete_text_field_cell.mm:106: // is
included in their origins).
Does the parenthetical comment just confuse the
> issue more?

I think it confused me.  You already say that they are in the same coordinate
system as cell_frame.  Btw, s/|frame|/|cell_frame|/?

http://codereview.chromium.org/2805070/diff/18001/19004#newcode108
chrome/browser/cocoa/location_bar/autocomplete_text_field_cell.mm:108: // An
additional |NULL| decoration is pushed on to provide a spot for
The NULL comment no longer applies.

[Scott Hess - ex-Googler, 2010-07-12 20:31:16]

Wow.  Didn't expect that, I figured on a few more rounds of debate :-).

I'm going to log bugs (and CC you) on a couple design points in this.
If you think that I'm being overly sensitive, suggest WontFix and I'll
move on.

-scott


On Mon, Jul 12, 2010 at 12:50 PM,  <rohitrao@chromium.org> wrote:
> LGTM!
>
>
> http://codereview.chromium.org/2805070/diff/1/5
> File chrome/browser/cocoa/location_bar/autocomplete_text_field_cell.mm
> (right):
>
> http://codereview.chromium.org/2805070/diff/1/5#newcode106
> chrome/browser/cocoa/location_bar/autocomplete_text_field_cell.mm:106:
> // is included in their origins).
> Does the parenthetical comment just confuse the
>>
>> issue more?
>
> I think it confused me.  You already say that they are in the same
> coordinate system as cell_frame.  Btw, s/|frame|/|cell_frame|/?
>
> http://codereview.chromium.org/2805070/diff/18001/19004#newcode108
> chrome/browser/cocoa/location_bar/autocomplete_text_field_cell.mm:108:
> // An additional |NULL| decoration is pushed on to provide a spot for
> The NULL comment no longer applies.
>
> http://codereview.chromium.org/2805070/show
>

[rohitrao (ping after 24h), 2010-07-12 21:57:11]

Heh, but you made all the changes I asked you to!  I'll be more
critical once the RHS decoration code comes in.  Until then, it's hard
to separate the more-cls-are-coming ugly from the real ugly.

On Mon, Jul 12, 2010 at 1:30 PM, Scott Hess <shess@chromium.org> wrote:
> Wow.  Didn't expect that, I figured on a few more rounds of debate :-).
>
> I'm going to log bugs (and CC you) on a couple design points in this.
> If you think that I'm being overly sensitive, suggest WontFix and I'll
> move on.
>
> -scott
>
>
> On Mon, Jul 12, 2010 at 12:50 PM,  <rohitrao@chromium.org> wrote:
>> LGTM!
>>
>>
>> http://codereview.chromium.org/2805070/diff/1/5
>> File chrome/browser/cocoa/location_bar/autocomplete_text_field_cell.mm
>> (right):
>>
>> http://codereview.chromium.org/2805070/diff/1/5#newcode106
>> chrome/browser/cocoa/location_bar/autocomplete_text_field_cell.mm:106:
>> // is included in their origins).
>> Does the parenthetical comment just confuse the
>>>
>>> issue more?
>>
>> I think it confused me.  You already say that they are in the same
>> coordinate system as cell_frame.  Btw, s/|frame|/|cell_frame|/?
>>
>> http://codereview.chromium.org/2805070/diff/18001/19004#newcode108
>> chrome/browser/cocoa/location_bar/autocomplete_text_field_cell.mm:108:
>> // An additional |NULL| decoration is pushed on to provide a spot for
>> The NULL comment no longer applies.
>>
>> http://codereview.chromium.org/2805070/show
>>
>