[Mac] Show the page info window after clicking the security icon in the URL bar

[Mac] Show the page info window after clicking the security icon in the URL bar

BUG=22922
TEST=Go to https://twitter.com, click the lock icon, see page info window.

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

[rohitrao (ping after 24h), 2009-09-24 17:27:36]

http://codereview.chromium.org/222020/diff/1/2
File chrome/browser/cocoa/autocomplete_text_field.mm (right):

http://codereview.chromium.org/222020/diff/1/2#newcode130
Line 130: TabContents* tab =
BrowserList::GetLastActive()->GetSelectedTabContents();
I'm a bit skittish about the AutocompleteTextField-->BrowserList dependency. 
Does Windows do this, or can we possibly push this functionality into a
delegate?

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

http://codereview.chromium.org/222020/diff/1/3#newcode71
Line 71: - (NSRect)frameForHintImageForFrame:(NSRect)cellFrame;
I dislike this method name, since I had to read it three times to parse it
correctly :)  hintImageFrameForFrame would make it more like the others in this
class.

[Robert Sesek, 2009-09-24 17:41:26]

http://codereview.chromium.org/222020/diff/1/2
File chrome/browser/cocoa/autocomplete_text_field.mm (right):

http://codereview.chromium.org/222020/diff/1/2#newcode130
Line 130: TabContents* tab =
BrowserList::GetLastActive()->GetSelectedTabContents();
On 2009/09/24 17:27:37, rohitrao wrote:
> I'm a bit skittish about the AutocompleteTextField-->BrowserList dependency. 
> Does Windows do this, or can we possibly push this functionality into a
> delegate?

This is copied straight from Windows location_bar_view.cc:1170. I'm not sure
making a delegate for this functionality is worth it; but I'm not passionate
about it one way or the other.

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

http://codereview.chromium.org/222020/diff/1/3#newcode71
Line 71: - (NSRect)frameForHintImageForFrame:(NSRect)cellFrame;
On 2009/09/24 17:27:37, rohitrao wrote:
> I dislike this method name, since I had to read it three times to parse it
> correctly :)  hintImageFrameForFrame would make it more like the others in
this
> class.

Agreed. This is a much saner name.

[Scott Hess - ex-Googler, 2009-09-24 20:03:53]

Beware of http://codereview.chromium.org/216031

http://codereview.chromium.org/222020/diff/1/2
File chrome/browser/cocoa/autocomplete_text_field.mm (right):

http://codereview.chromium.org/222020/diff/1/2#newcode130
Line 130: TabContents* tab =
BrowserList::GetLastActive()->GetSelectedTabContents();
On 2009/09/24 17:41:26, rsesek wrote:
> On 2009/09/24 17:27:37, rohitrao wrote:
> > I'm a bit skittish about the AutocompleteTextField-->BrowserList dependency.

> > Does Windows do this, or can we possibly push this functionality into a
> > delegate?
> 
> This is copied straight from Windows location_bar_view.cc:1170. I'm not sure
> making a delegate for this functionality is worth it; but I'm not passionate
> about it one way or the other.

Why doesn't location_bar_view.cc use delegate_->GetTabContents()?

I'm with Rohit, try to keep stuff that's in location_bar_view* on other
platforms in location_bar_view_mac.mm.  "Worth it" should be measured in terms
of how hard it is for someone to find the code, and if it's in
location_bar_view* elsewhere ... also, putting this in here inhibits
refactoring.  The existing code evolved organically, but at this point I'm kinda
thinking at some point replacing the field/cell/editor combo with a custom text
view subclass or something, so that everything can live in one place.

Besides, we'll probably have similar wiring for page actions, and it definitely
won't be the same.  In fact, that probably argues for this code to mostly be in
the cell (in this case, it's not too horrible, but long-term having the field
reach into the cell to spelunk through a half-dozen rects is probably poor).

As far as how to wire things together ... the field's delegate is already taken,
and the field doesn't really have anything it can get to directly other than
observer_ (and that route would be abusive, I think).

I can't think of a straight-forward way.  I guess for now, go with this, and put
in a TODO(shess): to figure out a cleaner way to wire stuff together.

[Scott Hess - ex-Googler, 2009-09-24 20:31:11]

Further note: Putting this inside the field implementation means it's
essentially impossible to unit-test that the event is hitting the right place. 
Interjecting something to forward the messages would make it possible to
separate the "test that it's hitting the right thing" from "test that it can
bring up the page info".  Not saying you must do this, just that it surely is
worth it to do.

-scott

On 2009/09/24 20:03:53, shess wrote:
> Beware of http://codereview.chromium.org/216031
> 
> http://codereview.chromium.org/222020/diff/1/2
> File chrome/browser/cocoa/autocomplete_text_field.mm (right):
> 
> http://codereview.chromium.org/222020/diff/1/2#newcode130
> Line 130: TabContents* tab =
> BrowserList::GetLastActive()->GetSelectedTabContents();
> On 2009/09/24 17:41:26, rsesek wrote:
> > On 2009/09/24 17:27:37, rohitrao wrote:
> > > I'm a bit skittish about the AutocompleteTextField-->BrowserList
dependency.
> 
> > > Does Windows do this, or can we possibly push this functionality into a
> > > delegate?
> > 
> > This is copied straight from Windows location_bar_view.cc:1170. I'm not sure
> > making a delegate for this functionality is worth it; but I'm not passionate
> > about it one way or the other.
> 
> Why doesn't location_bar_view.cc use delegate_->GetTabContents()?
> 
> I'm with Rohit, try to keep stuff that's in location_bar_view* on other
> platforms in location_bar_view_mac.mm.  "Worth it" should be measured in terms
> of how hard it is for someone to find the code, and if it's in
> location_bar_view* elsewhere ... also, putting this in here inhibits
> refactoring.  The existing code evolved organically, but at this point I'm
kinda
> thinking at some point replacing the field/cell/editor combo with a custom
text
> view subclass or something, so that everything can live in one place.
> 
> Besides, we'll probably have similar wiring for page actions, and it
definitely
> won't be the same.  In fact, that probably argues for this code to mostly be
in
> the cell (in this case, it's not too horrible, but long-term having the field
> reach into the cell to spelunk through a half-dozen rects is probably poor).
> 
> As far as how to wire things together ... the field's delegate is already
taken,
> and the field doesn't really have anything it can get to directly other than
> observer_ (and that route would be abusive, I think).
> 
> I can't think of a straight-forward way.  I guess for now, go with this, and
put
> in a TODO(shess): to figure out a cleaner way to wire stuff together.

[Robert Sesek, 2009-09-29 03:57:05]

On 2009/09/24 20:03:53, shess wrote:
> On 2009/09/24 17:41:26, rsesek wrote:
> > On 2009/09/24 17:27:37, rohitrao wrote:
> > > I'm a bit skittish about the AutocompleteTextField-->BrowserList
dependency.
> 
> > > Does Windows do this, or can we possibly push this functionality into a
> > > delegate?
> > 
> > This is copied straight from Windows location_bar_view.cc:1170. I'm not sure
> > making a delegate for this functionality is worth it; but I'm not passionate
> > about it one way or the other.
> 
> Why doesn't location_bar_view.cc use delegate_->GetTabContents()?

That snippet is from an inner image view class that doesn't have access to
LocationBarView's delegate_, though that looks pretty easily changeable.

> I'm with Rohit, try to keep stuff that's in location_bar_view* on other
> platforms in location_bar_view_mac.mm.  "Worth it" should be measured in terms
> of how hard it is for someone to find the code, and if it's in
> location_bar_view* elsewhere ... also, putting this in here inhibits
> refactoring.  The existing code evolved organically, but at this point I'm
kinda
> thinking at some point replacing the field/cell/editor combo with a custom
text
> view subclass or something, so that everything can live in one place.

I think this living in the observer is the way to go, as that seems to be how
field cell's events are handled. In Windows world with Views, they have it nice
that they can embed subviews (e.g. image views) in text views without things
going wonky (they probably would in Cocoa land). So, I think a custom field
would be interesting for this case.

> Besides, we'll probably have similar wiring for page actions, and it
definitely
> won't be the same.  In fact, that probably argues for this code to mostly be
in
> the cell (in this case, it's not too horrible, but long-term having the field
> reach into the cell to spelunk through a half-dozen rects is probably poor).
>
> As far as how to wire things together ... the field's delegate is already
taken,
> and the field doesn't really have anything it can get to directly other than
> observer_ (and that route would be abusive, I think).
> 
> I can't think of a straight-forward way.  I guess for now, go with this, and
put
> in a TODO(shess): to figure out a cleaner way to wire stuff together.

The cell has to know how to draw each of these icons, etc, so it knowing how to
hit-test them and forward the results to its observer should also be OK. I can
throw a TODO in there, though, so you can revisit it.

[Scott Hess - ex-Googler, 2009-09-30 18:45:47]

This is looking much cleaner.

http://codereview.chromium.org/222020/diff/7001/8004
File chrome/browser/cocoa/autocomplete_text_field.mm (right):

http://codereview.chromium.org/222020/diff/7001/8004#newcode122
Line 122: observer_->OnSecurityIconClicked();
Could this acquire a unit test?  I know it's hard, some idiot neglected to write
a test for -mouseDown: itself.  But, well, idiots are plentiful around here...

http://codereview.chromium.org/222020/diff/7001/8006
File chrome/browser/cocoa/autocomplete_text_field_cell.mm (right):

http://codereview.chromium.org/222020/diff/7001/8006#newcode257
Line 257: - (NSRect)hintImageFrameForFrame:(NSRect)cellFrame {
Now that this is pulled out of the drawing code, it's probably worth a unit
test.  Things like "rect is within the bounds" and "rect is right of
-drawingRectForBounds: for the same input" would be good, I think.

http://codereview.chromium.org/222020/diff/7001/8006#newcode260
Line 260: imageRect.size = [hintIcon_ size];
Since you pulled this out, now you can just have:
  const NSSize imageSize([hintIcon_ size]);
and use imageSize instead of imageRect.size.

http://codereview.chromium.org/222020/diff/7001/8006#newcode323
Line 323: cellFrame = [self hintImageFrameForFrame:cellFrame];
Now that this is so clean, would kinda prefer this more like:
   const NSRect hintFrame([...]);

[Scott Hess - ex-Googler, 2009-10-02 14:32:44]

http://codereview.chromium.org/222020/diff/7001/8004
File chrome/browser/cocoa/autocomplete_text_field.mm (right):

http://codereview.chromium.org/222020/diff/7001/8004#newcode122
Line 122: observer_->OnSecurityIconClicked();
On 2009/09/30 18:45:47, shess wrote:
> Could this acquire a unit test?  I know it's hard, some idiot neglected to
write
> a test for -mouseDown: itself.  But, well, idiots are plentiful around here...

BTW, there is now a primitive set of unit tests for this function, so adding a
new one should be easier.

[Robert Sesek, 2009-10-03 17:42:41]

http://codereview.chromium.org/222020/diff/7001/8004
File chrome/browser/cocoa/autocomplete_text_field.mm (right):

http://codereview.chromium.org/222020/diff/7001/8004#newcode122
Line 122: observer_->OnSecurityIconClicked();
On 2009/09/30 18:45:47, shess wrote:
> Could this acquire a unit test?  I know it's hard, some idiot neglected to
write
> a test for -mouseDown: itself.  But, well, idiots are plentiful around here...

Done.

http://codereview.chromium.org/222020/diff/7001/8006
File chrome/browser/cocoa/autocomplete_text_field_cell.mm (right):

http://codereview.chromium.org/222020/diff/7001/8006#newcode257
Line 257: - (NSRect)hintImageFrameForFrame:(NSRect)cellFrame {
On 2009/09/30 18:45:47, shess wrote:
> Now that this is pulled out of the drawing code, it's probably worth a unit
> test.  Things like "rect is within the bounds" and "rect is right of
> -drawingRectForBounds: for the same input" would be good, I think.

Done.

http://codereview.chromium.org/222020/diff/7001/8006#newcode260
Line 260: imageRect.size = [hintIcon_ size];
On 2009/09/30 18:45:47, shess wrote:
> Since you pulled this out, now you can just have:
>   const NSSize imageSize([hintIcon_ size]);
> and use imageSize instead of imageRect.size.

Done.

http://codereview.chromium.org/222020/diff/7001/8006#newcode323
Line 323: cellFrame = [self hintImageFrameForFrame:cellFrame];
On 2009/09/30 18:45:47, shess wrote:
> Now that this is so clean, would kinda prefer this more like:
>    const NSRect hintFrame([...]);

Done.

[Scott Hess - ex-Googler, 2009-10-06 20:30:53]

lgtm!

http://codereview.chromium.org/222020/diff/8014/7010
File chrome/browser/cocoa/autocomplete_text_field_unittest.mm (right):

http://codereview.chromium.org/222020/diff/8014/7010#newcode529
Line 529: [field_ mouseDown:event];
So Awesome.