Preliminary work toward page actions on Mac.

First steps toward page actions on Mac. Page action icons shold show and
perform their click actions when clicked. This is very rough, being committed
in an early form so more people can contribute to the effort.

Includes some early work from andybons too, toward making
popups work.

Known issues:
* Icons don't refresh properly when an extension is installed or removed.
Switching windows, then back again, updates the location bar properly.
* Right- and middle-clicks do nothing
* No tooltips, badges, installation previews, or popups yet
* No unit tests yet

BUG=12281
TEST=none yet

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

[Pam (message me for reviews), 2009-11-21 03:34:04]

(Not actually a review yet.)

Here's the work I have so far for page actions on Mac, if you'd like to work on
it next week while I'm away.

Known issues:
* Icons don't show up when a matching page is first loaded; you have to refresh
the location bar (e.g. by switching to another app and then back) to get them to
appear
* Looks like the page-action icons are a few pixels too far to the right in the
location bar
* Some 80-col cleanup needed in code

* Clicking doesn't do anything yet
* Right- and middle-clicking also don't do anything
* No tooltips
* No popups
* No badges
* No previews on installation

Despite all that, it is a fair start on things; displaying the icons is where
Mac diverges most from other platforms. I expect that most of the remaining
implementation (except perhaps for badges) will be pretty straightforward, with
the Windows page-actions and Mac browser-actions as guides.

I'll be happy to dive back into this when I get back on the 30th, or divide the
work if there's a logical seam.

- Pam

[Pam (message me for reviews), 2009-12-11 00:05:57]

OK, this is still an early implementation, but let's get it reviewed and checked
in so we can split up the remaining work (and not have a titanic review later).

[Bons, 2009-12-11 01:17:25]

LG with comments below.

http://codereview.chromium.org/427009/diff/14001/15005
File chrome/browser/cocoa/autocomplete_text_field.mm (right):

http://codereview.chromium.org/427009/diff/14001/15005#newcode119
chrome/browser/cocoa/autocomplete_text_field.mm:119: // Check to see if the user
clicked a page-action icon. If so, we need to
Let's have this be proper. s/page-action/Page Action
Also, if you avoid 'we' then you can write this in just one sentence.

http://codereview.chromium.org/427009/diff/14001/15005#newcode122
chrome/browser/cocoa/autocomplete_text_field.mm:122: size_t pageActionCount =
[cell pageActionCount];
no need for this var since you only use it once and inlining it won't push the
for loop over the edge.

http://codereview.chromium.org/427009/diff/14001/15004
File chrome/browser/cocoa/autocomplete_text_field_cell.h (right):

http://codereview.chromium.org/427009/diff/14001/15004#newcode32
chrome/browser/cocoa/autocomplete_text_field_cell.h:32: // List of views showing
visible page actions. Owned by the location bar.
Page Actions here and everywhere.

http://codereview.chromium.org/427009/diff/14001/15002
File chrome/browser/cocoa/autocomplete_text_field_cell.mm (right):

http://codereview.chromium.org/427009/diff/14001/15002#newcode251
chrome/browser/cocoa/autocomplete_text_field_cell.mm:251: width +=
[security_image_view_->GetImage() size].width +
NSWidth(security_image_view_->GetImage())

http://codereview.chromium.org/427009/diff/14001/15002#newcode287
chrome/browser/cocoa/autocomplete_text_field_cell.mm:287: NSSize imageRect =
[security_image_view_->GetImage() size];
this is called imageRect, but is of type NSSize, which is confusing. imageSize,
maybe?

http://codereview.chromium.org/427009/diff/14001/15002#newcode305
chrome/browser/cocoa/autocomplete_text_field_cell.mm:305:
page_action_views_->ViewAt(index); // pamg
// pamg?

http://codereview.chromium.org/427009/diff/14001/15001
File chrome/browser/cocoa/location_bar_view_mac.h (right):

http://codereview.chromium.org/427009/diff/14001/15001#newcode164
chrome/browser/cocoa/location_bar_view_mac.h:164: // PageActionImageView is used
to display the icon for a given PageAction
Page Action

http://codereview.chromium.org/427009/diff/14001/15003
File chrome/browser/cocoa/location_bar_view_mac.mm (right):

http://codereview.chromium.org/427009/diff/14001/15003#newcode564
chrome/browser/cocoa/location_bar_view_mac.mm:564: NSLog(@"Show page action icon
for tab id: %d? %d", current_tab_id_, visible);
Remove or comment out this NSLog

[Pam (message me for reviews), 2009-12-11 04:41:08]

Patch coming tomorrow, but in the meantime one question.

http://codereview.chromium.org/427009/diff/14001/15005
File chrome/browser/cocoa/autocomplete_text_field.mm (right):

http://codereview.chromium.org/427009/diff/14001/15005#newcode122
chrome/browser/cocoa/autocomplete_text_field.mm:122: size_t pageActionCount =
[cell pageActionCount];
Is the compiler smart enough to only run the count once, or will it percolate
the Page Action count from the location bar every time through the loop? (And if
the latter... do we really care? There aren't likely to be more than 20 Page
Actions installed, and probably more like 3.)

[Bons, 2009-12-11 15:44:27]

http://codereview.chromium.org/427009/diff/14001/15005
File chrome/browser/cocoa/autocomplete_text_field.mm (right):

http://codereview.chromium.org/427009/diff/14001/15005#newcode122
chrome/browser/cocoa/autocomplete_text_field.mm:122: size_t pageActionCount =
[cell pageActionCount];
On 2009/12/11 04:41:08, Pam wrote:
> Is the compiler smart enough to only run the count once, or will it percolate
> the Page Action count from the location bar every time through the loop? (And
if
> the latter... do we really care? There aren't likely to be more than 20 Page
> Actions installed, and probably more like 3.)

I think it's actually OK to leave it as is, but just make it a const.

[Pam (message me for reviews), 2009-12-11 23:16:48]

New patch uploaded, addressing your comments and fixing unit tests.

http://codereview.chromium.org/427009/diff/14001/15002
File chrome/browser/cocoa/autocomplete_text_field_cell.mm (right):

http://codereview.chromium.org/427009/diff/14001/15002#newcode251
chrome/browser/cocoa/autocomplete_text_field_cell.mm:251: width +=
[security_image_view_->GetImage() size].width +
On 2009/12/11 01:17:25, Andrew Bonventre (chromium) wrote:
> NSWidth(security_image_view_->GetImage())

NSWidth only works on NSRects.

[Bons, 2009-12-12 16:00:34]

LGTM. Nice work :)