When using action box on Mac, hide star when page is not bookmarked.

When using action box on Mac, hide star when page is not bookmarked.

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

[beaudoin, 2012-08-24 21:40:41]

Updating location bar behavior to match Window's, when action box is enabled.

[beaudoin, 2012-08-27 19:15:20]

@shess Friendly ping.

[Scott Hess - ex-Googler, 2012-08-27 19:47:31]

I'm not going to closely review every case, but did want to call out that since
this changes visibility, you need to make sure that every potential change is
followed by the stanza which re-calculates cursor rects and tooltips.  I think
it does.

http://codereview.chromium.org/10874068/diff/1/chrome/browser/ui/cocoa/locati...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):

http://codereview.chromium.org/10874068/diff/1/chrome/browser/ui/cocoa/locati...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:776:
(star_decoration_->starred() || !action_box_enabled));
I think this condition is needlessly complicated.  I think better would be
something like:
  bool visible = IsStarEnabled();
  if (action_box_enabled && !starred())
    visible = false;
though I might not understand the entire truth table for the existing condition
:-).

[beaudoin, 2012-08-27 21:25:47]

I verified that OnDecorationsChanged is called whenever
UpdateStarDecorationVisibility is, which I think re-calculates cursor rects and
tooltips.

http://codereview.chromium.org/10874068/diff/1/chrome/browser/ui/cocoa/locati...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):

http://codereview.chromium.org/10874068/diff/1/chrome/browser/ui/cocoa/locati...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:776:
(star_decoration_->starred() || !action_box_enabled));
On 2012/08/27 19:47:31, shess wrote:
> I think this condition is needlessly complicated.  I think better would be
> something like:
>   bool visible = IsStarEnabled();
>   if (action_box_enabled && !starred())
>     visible = false;
> though I might not understand the entire truth table for the existing
condition
> :-).

Good job on that truth table! This indeed simplifies it! :)

Done.

[Scott Hess - ex-Googler, 2012-08-27 21:27:42]

LGTM.

BTW, if you do too many more of these, I'm going to let you own
location_bar_view_mac.mm ...

[commit-bot: I haz the power, 2012-08-27 21:30:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/beaudoin@chromium.org/10874068/6001

[commit-bot: I haz the power, 2012-08-27 23:52:06]

Change committed as 153587