Mac: Content blocked icons.

Mac: Content blocked icons.

Also rewrite how omnibox icons are handled, to make it less repetitive to add the content blocked icons. Remove a O(n^2) while I'm at it.

BUG=35594, 34894
TEST=Go to a page with popups. "popups blocked" icon should appear in omnibox. It should have a tooltip, and a normal arrow cursor on mouse over.  Switching tabs should make it go away, coming back to the tab with the blocked popup should make it go back. Page actions should still work (tooltips, clicking, context menu, display). Security icon should still work. Install rss extension; the preview bubble should point to the right icon.

http://imgur.com/Yo0Ss

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

[Nico, 2010-02-16 20:42:26]

Ignore the changes to Preferences.xib and preferences_window_controller.mm, I
will revert those before submitting (git artifact).

I'm still fighting with a few unit tests, but it's mostly done.

[Nico, 2010-02-16 22:03:11]

http://codereview.chromium.org/600133/diff/16/1021
File chrome/browser/cocoa/autocomplete_text_field_cell.mm (right):

http://codereview.chromium.org/600133/diff/16/1021#newcode254
chrome/browser/cocoa/autocomplete_text_field_cell.mm:254: // Account for the
lock icon, if any, and any visible Page Action icons.
I think this whole branch can be replaced by

    // Leave room for images on the right (lock icon etc).
    NSArray* iconFrames = [self layedOutIcons:cellFrame];
    CGFloat width = 0;
    if ([iconFrames count] > 0)
      width = NSMaxX(cellFrame) - NSMinX([[iconFrames lastItem] rect]);
    if (width > 0)
      width += kIconHorizontalPad;
    if (width < NSWidth(cellFrame))
      textFrame.size.width -= width;

but I needed to sync for the tryservers, and now I have to wait for
recompilation until I can try it.

[Miranda Callahan, 2010-02-16 22:14:02]

http://codereview.chromium.org/600133/diff/5001/5004
File chrome/browser/cocoa/autocomplete_text_field.mm (right):

http://codereview.chromium.org/600133/diff/5001/5004#newcode118
chrome/browser/cocoa/autocomplete_text_field.mm:118: // If the user clicked on
of the icons (security icon, page actions, etc), let
s/on/one

http://codereview.chromium.org/600133/diff/5001/5005
File chrome/browser/cocoa/autocomplete_text_field_cell.h (right):

http://codereview.chromium.org/600133/diff/5001/5005#newcode16
chrome/browser/cocoa/autocomplete_text_field_cell.h:16:
LocationBarViewMac::LocationBarImageView* view_;
Who owns view_?

http://codereview.chromium.org/600133/diff/5001/5005#newcode86
chrome/browser/cocoa/autocomplete_text_field_cell.h:86: // to get the position
of al page actions. Use |-layedOutIcons:| instead in that
s/al/all

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

http://codereview.chromium.org/600133/diff/5001/5006#newcode68
chrome/browser/cocoa/autocomplete_text_field_cell.mm:68:
AutocompleteTextFieldIcon* result = [[AutocompleteTextFieldIcon alloc] init];
Should this be autoreleased?

http://codereview.chromium.org/600133/diff/5001/5006#newcode268
chrome/browser/cocoa/autocomplete_text_field_cell.mm:268:
content_blocked_views_->begin());
shouldn't this indent be some multiple of 4?

http://codereview.chromium.org/600133/diff/5001/5010
File chrome/browser/cocoa/extension_installed_bubble_controller.mm (right):

http://codereview.chromium.org/600133/diff/5001/5010#newcode203
chrome/browser/cocoa/extension_installed_bubble_controller.mm:203: arrowPoint =
NSMakePoint(NSMidX(boundsrect) + 1, NSMinY(boundsrect));
you are awesome, thanks.

http://codereview.chromium.org/600133/diff/5001/5011
File chrome/browser/cocoa/location_bar_view_mac.h (right):

http://codereview.chromium.org/600133/diff/5001/5011#newcode314
chrome/browser/cocoa/location_bar_view_mac.h:314: LocationBarViewMac* owner,
indent--  for this line and below.

http://codereview.chromium.org/600133/diff/5001/5013
File chrome/browser/cocoa/preferences_window_controller.mm (right):

http://codereview.chromium.org/600133/diff/5001/5013#newcode1431
chrome/browser/cocoa/preferences_window_controller.mm:1431: -
(void)setBlockImages:(BOOL)value {
remove commented-out code from this method.

[Miranda Callahan, 2010-02-16 22:16:24]

Nice patch, btw -- many things are much clearer.

[Bons, 2010-02-16 22:42:04]

http://codereview.chromium.org/600133/diff/5001/5002
File base/scoped_vector.h (right):

http://codereview.chromium.org/600133/diff/5001/5002#newcode1
base/scoped_vector.h:1: // Copyright (c) 2006-2008 The Chromium Authors. All
rights reserved.
I believe we're updating the copyright lines to reflect the current year for all
files we alter from now on.

http://codereview.chromium.org/600133/diff/5001/5004
File chrome/browser/cocoa/autocomplete_text_field.mm (right):

http://codereview.chromium.org/600133/diff/5001/5004#newcode118
chrome/browser/cocoa/autocomplete_text_field.mm:118: // If the user clicked on
of the icons (security icon, page actions, etc), let
for consistency. "Page Actions" (capitalization).

http://codereview.chromium.org/600133/diff/5001/5004#newcode121
chrome/browser/cocoa/autocomplete_text_field.mm:121: for
(AutocompleteTextFieldIcon* icon in [cell layedOutIcons:bounds]) {
Cache the layedOutIcons so that the method isn't being called for each one? Your
call since there are probably so little that it doesn't really matter.

http://codereview.chromium.org/600133/diff/5001/5004#newcode220
chrome/browser/cocoa/autocomplete_text_field.mm:220: // -[NSView
addToolTipRect:owner:userData] does _not_ retain the tooltip!
"does _not_ retain its owner (in this case, the tooltip)."

nit: I don't think we need the exclamation point. I know programming is exciting
and all, but it seems superfluous.

http://codereview.chromium.org/600133/diff/5001/5005
File chrome/browser/cocoa/autocomplete_text_field_cell.h (right):

http://codereview.chromium.org/600133/diff/5001/5005#newcode15
chrome/browser/cocoa/autocomplete_text_field_cell.h:15: NSRect rect_;
Yes please document each of these and mention whether they are weak references
or not. Also the interface itself.

http://codereview.chromium.org/600133/diff/5001/5005#newcode18
chrome/browser/cocoa/autocomplete_text_field_cell.h:18: +
(AutocompleteTextFieldIcon*)
document.

http://codereview.chromium.org/600133/diff/5001/5005#newcode78
chrome/browser/cocoa/autocomplete_text_field_cell.h:78: // Returns an array of
AutocompleteTextFieldIcon objects. Returns only visible
Returns an array of the visible AutocompleteTextFieldIcon objects within the
given cell frame.

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

http://codereview.chromium.org/600133/diff/5001/5006#newcode69
chrome/browser/cocoa/autocomplete_text_field_cell.mm:69: result->rect_ = rect;
to avoid confusion, this isn't a C++ object, so I would just use [result
setRect:rect]

http://codereview.chromium.org/600133/diff/5001/5006#newcode76
chrome/browser/cocoa/autocomplete_text_field_cell.mm:76: @interface
AutocompleteTextFieldCell(Private)
I'm assuming you're going to put testing methods here? If not then remove.

http://codereview.chromium.org/600133/diff/5001/5006#newcode439
chrome/browser/cocoa/autocomplete_text_field_cell.mm:439: NSMutableArray* result
= [NSMutableArray arrayWithCapacity:0];
just use [[NSMutableArray alloc] init] with an autorelease. no need to init with
a capacity plus it's more clear what the memory situation is.

http://codereview.chromium.org/600133/diff/5001/5006#newcode456
chrome/browser/cocoa/autocomplete_text_field_cell.mm:456: [self
rightJustifyImage:iconSize
this is indented three spaces.

http://codereview.chromium.org/600133/diff/5001/5006#newcode467
chrome/browser/cocoa/autocomplete_text_field_cell.mm:467: // right to left but
in the vector they're ordered left to right.
nice.

http://codereview.chromium.org/600133/diff/5001/5006#newcode468
chrome/browser/cocoa/autocomplete_text_field_cell.mm:468: for
(LocationBarViewMac::ContentBlockedViews::const_reverse_iterator
ugh. this nested namespace stuff is killing readability. these classes really
need to be in separate files.

http://codereview.chromium.org/600133/diff/5001/5009
File chrome/browser/cocoa/autocomplete_text_field_unittest.mm (right):

http://codereview.chromium.org/600133/diff/5001/5009#newcode31
chrome/browser/cocoa/autocomplete_text_field_unittest.mm:31: // NSRect argument
to OnMousePressed.
Why did this work before? What do you mean by "it doesn't like" it? It can't
read the NSRect in the test methods?

http://codereview.chromium.org/600133/diff/5001/5011
File chrome/browser/cocoa/location_bar_view_mac.h (right):

http://codereview.chromium.org/600133/diff/5001/5011#newcode162
chrome/browser/cocoa/location_bar_view_mac.h:162: virtual NSMenu* GetMenu() {
return nil; }
nice.

http://codereview.chromium.org/600133/diff/5001/5012
File chrome/browser/cocoa/location_bar_view_mac.mm (right):

http://codereview.chromium.org/600133/diff/5001/5012#newcode861
chrome/browser/cocoa/location_bar_view_mac.mm:861: // TODO(thakis): Implement.
add a NOTIMPLEMENTED()?

http://codereview.chromium.org/600133/diff/5001/5013
File chrome/browser/cocoa/preferences_window_controller.mm (right):

http://codereview.chromium.org/600133/diff/5001/5013#newcode1428
chrome/browser/cocoa/preferences_window_controller.mm:1428:
CONTENT_SETTINGS_TYPE_IMAGES) !=  CONTENT_SETTING_ALLOW;
two spaces between != and CONTENT...

http://codereview.chromium.org/600133/diff/5001/5013#newcode1448
chrome/browser/cocoa/preferences_window_controller.mm:1448:
CONTENT_SETTINGS_TYPE_JAVASCRIPT) !=  CONTENT_SETTING_ALLOW;
same here.

[Nico, 2010-02-16 22:45:37]

on to andy's comments…

http://codereview.chromium.org/600133/diff/5001/5004
File chrome/browser/cocoa/autocomplete_text_field.mm (right):

http://codereview.chromium.org/600133/diff/5001/5004#newcode118
chrome/browser/cocoa/autocomplete_text_field.mm:118: // If the user clicked on
of the icons (security icon, page actions, etc), let
On 2010/02/16 22:14:02, Miranda Callahan wrote:
> s/on/one

Done.

http://codereview.chromium.org/600133/diff/5001/5005
File chrome/browser/cocoa/autocomplete_text_field_cell.h (right):

http://codereview.chromium.org/600133/diff/5001/5005#newcode16
chrome/browser/cocoa/autocomplete_text_field_cell.h:16:
LocationBarViewMac::LocationBarImageView* view_;
On 2010/02/16 22:14:02, Miranda Callahan wrote:
> Who owns view_?

The LocationBarViewMac object. I added a few comments.

http://codereview.chromium.org/600133/diff/5001/5005#newcode86
chrome/browser/cocoa/autocomplete_text_field_cell.h:86: // to get the position
of al page actions. Use |-layedOutIcons:| instead in that
On 2010/02/16 22:14:02, Miranda Callahan wrote:
> s/al/all

Done.

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

http://codereview.chromium.org/600133/diff/5001/5006#newcode68
chrome/browser/cocoa/autocomplete_text_field_cell.mm:68:
AutocompleteTextFieldIcon* result = [[AutocompleteTextFieldIcon alloc] init];
On 2010/02/16 22:14:02, Miranda Callahan wrote:
> Should this be autoreleased?

Yes! Thanks.

http://codereview.chromium.org/600133/diff/5001/5006#newcode268
chrome/browser/cocoa/autocomplete_text_field_cell.mm:268:
content_blocked_views_->begin());
On 2010/02/16 22:14:02, Miranda Callahan wrote:
> shouldn't this indent be some multiple of 4?

Done.

http://codereview.chromium.org/600133/diff/5001/5011
File chrome/browser/cocoa/location_bar_view_mac.h (right):

http://codereview.chromium.org/600133/diff/5001/5011#newcode314
chrome/browser/cocoa/location_bar_view_mac.h:314: LocationBarViewMac* owner,
On 2010/02/16 22:14:02, Miranda Callahan wrote:
> indent--  for this line and below.

Done.

http://codereview.chromium.org/600133/diff/5001/5013
File chrome/browser/cocoa/preferences_window_controller.mm (right):

http://codereview.chromium.org/600133/diff/5001/5013#newcode1431
chrome/browser/cocoa/preferences_window_controller.mm:1431: -
(void)setBlockImages:(BOOL)value {
On 2010/02/16 22:14:02, Miranda Callahan wrote:
> remove commented-out code from this method.

I won't check in this file, see my first comment on the mail thread.

[Nico, 2010-02-16 23:04:11]

Thanks!

http://codereview.chromium.org/600133/diff/5001/5002
File base/scoped_vector.h (right):

http://codereview.chromium.org/600133/diff/5001/5002#newcode1
base/scoped_vector.h:1: // Copyright (c) 2006-2008 The Chromium Authors. All
rights reserved.
On 2010/02/16 22:42:04, Andrew Bonventre (Bons) wrote:
> I believe we're updating the copyright lines to reflect the current year for
all
> files we alter from now on.

Done.

http://codereview.chromium.org/600133/diff/5001/5004
File chrome/browser/cocoa/autocomplete_text_field.mm (right):

http://codereview.chromium.org/600133/diff/5001/5004#newcode118
chrome/browser/cocoa/autocomplete_text_field.mm:118: // If the user clicked on
of the icons (security icon, page actions, etc), let
On 2010/02/16 22:42:04, Andrew Bonventre (Bons) wrote:
> for consistency. "Page Actions" (capitalization).

Done.

http://codereview.chromium.org/600133/diff/5001/5004#newcode121
chrome/browser/cocoa/autocomplete_text_field.mm:121: for
(AutocompleteTextFieldIcon* icon in [cell layedOutIcons:bounds]) {
On 2010/02/16 22:42:04, Andrew Bonventre (Bons) wrote:
> Cache the layedOutIcons so that the method isn't being called for each one?
Your
> call since there are probably so little that it doesn't really matter.

Huh? I'm pretty sure that fast enumeration evaluates the "in" expression only
once. Is that wrong? Am I missing something?

http://codereview.chromium.org/600133/diff/5001/5004#newcode220
chrome/browser/cocoa/autocomplete_text_field.mm:220: // -[NSView
addToolTipRect:owner:userData] does _not_ retain the tooltip!
On 2010/02/16 22:42:04, Andrew Bonventre (Bons) wrote:
> "does _not_ retain its owner (in this case, the tooltip)."
> 
> nit: I don't think we need the exclamation point. I know programming is
exciting
> and all, but it seems superfluous.

Done.

http://codereview.chromium.org/600133/diff/5001/5005
File chrome/browser/cocoa/autocomplete_text_field_cell.h (right):

http://codereview.chromium.org/600133/diff/5001/5005#newcode15
chrome/browser/cocoa/autocomplete_text_field_cell.h:15: NSRect rect_;
On 2010/02/16 22:42:04, Andrew Bonventre (Bons) wrote:
> Yes please document each of these and mention whether they are weak references
> or not. Also the interface itself.

Done.

http://codereview.chromium.org/600133/diff/5001/5005#newcode18
chrome/browser/cocoa/autocomplete_text_field_cell.h:18: +
(AutocompleteTextFieldIcon*)
On 2010/02/16 22:42:04, Andrew Bonventre (Bons) wrote:
> document.

Done.

http://codereview.chromium.org/600133/diff/5001/5005#newcode78
chrome/browser/cocoa/autocomplete_text_field_cell.h:78: // Returns an array of
AutocompleteTextFieldIcon objects. Returns only visible
On 2010/02/16 22:42:04, Andrew Bonventre (Bons) wrote:
> Returns an array of the visible AutocompleteTextFieldIcon objects within the
> given cell frame.

No, it returns all visible objects, "cellFrame" is only used for layout.

Done.

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

http://codereview.chromium.org/600133/diff/5001/5006#newcode69
chrome/browser/cocoa/autocomplete_text_field_cell.mm:69: result->rect_ = rect;
On 2010/02/16 22:42:04, Andrew Bonventre (Bons) wrote:
> to avoid confusion, this isn't a C++ object, so I would just use [result
> setRect:rect]

Done.

http://codereview.chromium.org/600133/diff/5001/5006#newcode76
chrome/browser/cocoa/autocomplete_text_field_cell.mm:76: @interface
AutocompleteTextFieldCell(Private)
On 2010/02/16 22:42:04, Andrew Bonventre (Bons) wrote:
> I'm assuming you're going to put testing methods here? If not then remove.

Oops. Done. At one point, there was something in there.

http://codereview.chromium.org/600133/diff/5001/5006#newcode439
chrome/browser/cocoa/autocomplete_text_field_cell.mm:439: NSMutableArray* result
= [NSMutableArray arrayWithCapacity:0];
On 2010/02/16 22:42:04, Andrew Bonventre (Bons) wrote:
> just use [[NSMutableArray alloc] init] with an autorelease. no need to init
with
> a capacity plus it's more clear what the memory situation is.

But the I also need an autorelease, and then it's longer than what's currently
there. I think this is a pretty common idiom.

http://codereview.chromium.org/600133/diff/5001/5006#newcode456
chrome/browser/cocoa/autocomplete_text_field_cell.mm:456: [self
rightJustifyImage:iconSize
On 2010/02/16 22:42:04, Andrew Bonventre (Bons) wrote:
> this is indented three spaces.

Done.

http://codereview.chromium.org/600133/diff/5001/5006#newcode468
chrome/browser/cocoa/autocomplete_text_field_cell.mm:468: for
(LocationBarViewMac::ContentBlockedViews::const_reverse_iterator
On 2010/02/16 22:42:04, Andrew Bonventre (Bons) wrote:
> ugh. this nested namespace stuff is killing readability. these classes really
> need to be in separate files.

It is in GTK. But for now, I think this is fine.

http://codereview.chromium.org/600133/diff/5001/5009
File chrome/browser/cocoa/autocomplete_text_field_unittest.mm (right):

http://codereview.chromium.org/600133/diff/5001/5009#newcode31
chrome/browser/cocoa/autocomplete_text_field_unittest.mm:31: // NSRect argument
to OnMousePressed.
On 2010/02/16 22:42:04, Andrew Bonventre (Bons) wrote:
> Why did this work before? What do you mean by "it doesn't like" it? It can't
> read the NSRect in the test methods?

'cause OnMousePressed() didn't take a NSRect for security views before.

See a couple lines below :-P

http://codereview.chromium.org/600133/diff/5001/5012
File chrome/browser/cocoa/location_bar_view_mac.mm (right):

http://codereview.chromium.org/600133/diff/5001/5012#newcode861
chrome/browser/cocoa/location_bar_view_mac.mm:861: // TODO(thakis): Implement.
On 2010/02/16 22:42:04, Andrew Bonventre (Bons) wrote:
> add a NOTIMPLEMENTED()?

Sure, why not.

http://codereview.chromium.org/600133/diff/5001/5013
File chrome/browser/cocoa/preferences_window_controller.mm (right):

http://codereview.chromium.org/600133/diff/5001/5013#newcode1428
chrome/browser/cocoa/preferences_window_controller.mm:1428:
CONTENT_SETTINGS_TYPE_IMAGES) !=  CONTENT_SETTING_ALLOW;
On 2010/02/16 22:42:04, Andrew Bonventre (Bons) wrote:
> two spaces between != and CONTENT...

I won't check in this file, see my first comment on the mail thread.

http://codereview.chromium.org/600133/diff/5001/5013#newcode1448
chrome/browser/cocoa/preferences_window_controller.mm:1448:
CONTENT_SETTINGS_TYPE_JAVASCRIPT) !=  CONTENT_SETTING_ALLOW;
On 2010/02/16 22:42:04, Andrew Bonventre (Bons) wrote:
> same here.

I won't check in this file, see my first comment on the mail thread.

[Bons, 2010-02-16 23:23:02]

LGTM

http://codereview.chromium.org/600133/diff/5001/5004
File chrome/browser/cocoa/autocomplete_text_field.mm (right):

http://codereview.chromium.org/600133/diff/5001/5004#newcode121
chrome/browser/cocoa/autocomplete_text_field.mm:121: for
(AutocompleteTextFieldIcon* icon in [cell layedOutIcons:bounds]) {
On 2010/02/16 23:04:11, Nico wrote:
> On 2010/02/16 22:42:04, Andrew Bonventre (Bons) wrote:
> > Cache the layedOutIcons so that the method isn't being called for each one?
> Your
> > call since there are probably so little that it doesn't really matter.
> 
> Huh? I'm pretty sure that fast enumeration evaluates the "in" expression only
> once. Is that wrong? Am I missing something?

Ah. I'm not familiar enough with FE to know, so I'll trust your assumption.

[Miranda Callahan, 2010-02-16 23:25:23]

LGTM++

On 2010/02/16 23:23:02, Andrew Bonventre (Bons) wrote:
> LGTM
> 
> http://codereview.chromium.org/600133/diff/5001/5004
> File chrome/browser/cocoa/autocomplete_text_field.mm (right):
> 
> http://codereview.chromium.org/600133/diff/5001/5004#newcode121
> chrome/browser/cocoa/autocomplete_text_field.mm:121: for
> (AutocompleteTextFieldIcon* icon in [cell layedOutIcons:bounds]) {
> On 2010/02/16 23:04:11, Nico wrote:
> > On 2010/02/16 22:42:04, Andrew Bonventre (Bons) wrote:
> > > Cache the layedOutIcons so that the method isn't being called for each
one?
> > Your
> > > call since there are probably so little that it doesn't really matter.
> > 
> > Huh? I'm pretty sure that fast enumeration evaluates the "in" expression
only
> > once. Is that wrong? Am I missing something?
> 
> Ah. I'm not familiar enough with FE to know, so I'll trust your assumption.