Don't show the button for hidden page/browser actions or script badges.

Don't show the button for hidden page/browser actions or script badges. 

Browser Action, Page Actions and Script Badges can be hidden, which causes us to get invalid |button| and |frame| (respectively). We'll need to figure out what to do in these cases (on all platforms) but the minimum for now is to ignore the keystroke to avoid a crash. 

BUG=140396
TEST=None
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=152321

[Finnur, 2012-08-17 12:18:16]

http://codereview.chromium.org/10828364/diff/2001/chrome/browser/ui/cocoa/loc...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (left):

http://codereview.chromium.org/10828364/diff/2001/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:413:
DCHECK(!NSIsEmptyRect(frame));
When I factored GetPageActionFrame out of GetPageActionBubblePoint, I moved this
DCHECK along with it. However, GetPageActionFrame can be called for PageActions
that are not visible, so I've moved the DCHECK back into
GetPageActionBubblePoint (and converted it to NOTREACHED).

[Scott Hess - ex-Googler, 2012-08-17 17:13:17]

lgtm.  My comments are just morning grumpiness.

http://codereview.chromium.org/10828364/diff/5001/chrome/browser/ui/cocoa/ext...
File chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm (right):

http://codereview.chromium.org/10828364/diff/5001/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm:216: //
|button| can be NULL when the browser action has its button hidden.
s/NULL/nil/ for Objective-C objects.  Unless BrowserActionButton is some C++
object.

http://codereview.chromium.org/10828364/diff/5001/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm:218: [owner_
browserActionClicked:button];
This combination almost makes me wish you could write:
  [owner_ browserActionClickedForExtension:extension];

http://codereview.chromium.org/10828364/diff/5001/chrome/browser/ui/cocoa/loc...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):

http://codereview.chromium.org/10828364/diff/5001/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:424: NOTREACHED();
I was thinking "Well, then, why is this here?"  Maybe there should be a comment
like "The bubble point positioning assumes that the page action is visible.  If
not, something else needs to be done."

http://codereview.chromium.org/10828364/diff/5001/chrome/browser/ui/cocoa/loc...
File chrome/browser/ui/cocoa/location_bar/page_action_decoration.mm (right):

http://codereview.chromium.org/10828364/diff/5001/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/page_action_decoration.mm:291:
ActivatePageAction(frame);
Could you just check IsVisible() directly rather than bouncing through the
owner_?

Also, is not executing the page action the right choice in this case?  Seems
like the notification shouldn't even have been sent if it's not visible.

[Finnur, 2012-08-20 11:41:45]

Made the changes. Will check in on green.

http://codereview.chromium.org/10828364/diff/5001/chrome/browser/ui/cocoa/ext...
File chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm (right):

http://codereview.chromium.org/10828364/diff/5001/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm:218: [owner_
browserActionClicked:button];
Yeah, it is a bit silly because the function actually doesn't want a
BrowserActionButton* since it converts |button| straight away to a const
Extension* and uses that. Changed that.

http://codereview.chromium.org/10828364/diff/5001/chrome/browser/ui/cocoa/loc...
File chrome/browser/ui/cocoa/location_bar/page_action_decoration.mm (right):

http://codereview.chromium.org/10828364/diff/5001/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/page_action_decoration.mm:291:
ActivatePageAction(frame);
> Also, is not executing the page action the right choice 
> in this case?  Seems like the notification shouldn't
> even have been sent if it's not visible.

So, this is work in progress. Ideally, we'd want the shortcut to not be active
if the page action isn't visible. However, there is more work that is required
for that to take place. But for now I've added the IsVisible check to prevent
the bubble from showing up unanchored when the page action is not visible.

[Finnur, 2012-08-20 14:03:34]

http://codereview.chromium.org/10828364/diff/5001/chrome/browser/ui/cocoa/ext...
File chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm (right):

http://codereview.chromium.org/10828364/diff/5001/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm:218: [owner_
browserActionClicked:button];
Actually, I take that back (and reverted this change).

Basically, changing it to browserActionClickedForExtension:extension causes a
crash in other tests and it wasn't obvious why. We get back a NULL
browser_action in ExtensionToolbarModel::ExecuteBrowserAction and it would (of
course) crash trying to verify that it was visible. Anyway, this change was
perhaps a bit orthogonal to the change at hand, so I'm taking this attempt out
of the CL. If it passes green on the try servers (after reverting back) then
I'll check in.

On 2012/08/20 11:41:45, Finnur wrote:
> Yeah, it is a bit silly because the function actually doesn't want a
> BrowserActionButton* since it converts |button| straight away to a const
> Extension* and uses that. Changed that.

[Scott Hess - ex-Googler, 2012-08-20 22:59:34]

http://codereview.chromium.org/10828364/diff/5001/chrome/browser/ui/cocoa/ext...
File chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm (right):

http://codereview.chromium.org/10828364/diff/5001/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm:218: [owner_
browserActionClicked:button];
Makes sense (I think).

On 2012/08/20 14:03:34, Finnur wrote:
> Actually, I take that back (and reverted this change).
> 
> Basically, changing it to browserActionClickedForExtension:extension causes a
> crash in other tests and it wasn't obvious why. We get back a NULL
> browser_action in ExtensionToolbarModel::ExecuteBrowserAction and it would (of
> course) crash trying to verify that it was visible. Anyway, this change was
> perhaps a bit orthogonal to the change at hand, so I'm taking this attempt out
> of the CL. If it passes green on the try servers (after reverting back) then
> I'll check in.
> 
> On 2012/08/20 11:41:45, Finnur wrote:
> > Yeah, it is a bit silly because the function actually doesn't want a
> > BrowserActionButton* since it converts |button| straight away to a const
> > Extension* and uses that. Changed that.
>