[Mac] Move star button into page-actions area of omnibox.

[Mac] Move star button into page-actions area of omnibox.

Removes the star button from the toolbar entirely.  Adds a
LocationBarImageView subclass for the star icon and wires it to the
RHS of the field.  Adjust the bookmark bubble to move the arrow to the
RHS and position appropriately.

BookmarkBubble.xib: outlet to view so that controller can change the arrow from left to right.

Toolbar.xib: Move reload icon to where star was, remove star icon, adjust spacing.  All spacing was adjusted to specific positions in the relevant inspector, not by mouse drag, so hopefully there's nothing awry there.

BUG=37865
TEST=No star icon on toolbar.
TEST=Optional home button adjustments should work right.
TEST=Star action in omnibox when showing an URL.
TEST=Star action can be clicked to bookmark current page.
TEST=Bookmark bubble arrow points at star like before.
TEST=Command-d brings up bookmark bubble.
TEST=Star action changes from blank to yellow depending on state.
TEST=Star action tooltip changes depending on state.

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

[Scott Hess - ex-Googler, 2010-04-01 01:21:12]

If you could contain yourself to only minor quibbles, like "Please alphabetize
your include files here." that would really make it easier to get this checked
in :-).

[Scott Hess - ex-Googler, 2010-04-01 01:22:46]

On 2010/04/01 01:21:12, shess wrote:
> If you could contain yourself to only minor quibbles, like "Please alphabetize
> your include files here." that would really make it easier to get this checked
> in :-).

Also, I'm on 10.5, I understand 10.6/10.5 differences cause some of these .xib
changes.  My change was to see the bubble view (for setting the arrow position)
in BookmarkBubble.xib, and moving the reload button where the star was (and
resizing appropriately) in Toolbar.xib.  The moves and resizes were done with
addition and subtraction rather than dragging, so hopefully it's spot-on.

[viettrungluu, 2010-04-01 05:22:45]

Drive-by nag: Include a description of the .xib changes in the CL
description/commit message, for when mysterious problems occur in the .xib and
someone has to re-do them.

Other silly drive-by comments below (i.e., this is not a review).

http://codereview.chromium.org/1540009/diff/1/17
File chrome/browser/cocoa/toolbar_controller.mm (right):

http://codereview.chromium.org/1540009/diff/1/17#newcode397
chrome/browser/cocoa/toolbar_controller.mm:397: [backButton_
setEnabled:commands->IsCommandEnabled(IDC_BACK) ? YES : NO];
Argh. See below.

http://codereview.chromium.org/1540009/diff/1/17#newcode406
chrome/browser/cocoa/toolbar_controller.mm:406: locationBarView_->Update(tab,
shouldRestore ? true : false);
Hey, here too (see below)!

http://codereview.chromium.org/1540009/diff/1/17#newcode416
chrome/browser/cocoa/toolbar_controller.mm:416:
locationBarView_->SetStarred(isStarred ? true : false);
Drive-by nit: no need to do any conversion of BOOL to bool.

http://codereview.chromium.org/1540009/diff/1/17#newcode513
chrome/browser/cocoa/toolbar_controller.mm:513: BOOL hide =
showHomeButton_.GetValue() ? NO : YES;
I wonder what was wrong with |!showHomeButton_.GetValue()|.

[rohitrao (ping after 24h), 2010-04-01 14:12:36]

LGTM

I'm starting to dislike how we have so many individual icon views to keep track
of in the cell, although I don't know if there's a good way to consolidate them.
 layedOutIcons: does a decent job of that, I suppose.

There's an existing bug for us not updating the cell when an extension changes
its page action icon.  I haven't dug into how to fix that yet, but it might be
good to add a way for the LocationBarImageViews to force a redraw directly.

http://codereview.chromium.org/1540009/diff/1/9
File chrome/browser/cocoa/bookmark_bubble_controller.mm (right):

http://codereview.chromium.org/1540009/diff/1/9#newcode103
chrome/browser/cocoa/bookmark_bubble_controller.mm:103: [bubble_
setArrowLocation:kTopRight];
Hm, we should probably shift the bubble around when the default placement is
partially/completely offscreen.  I'll file a bug for that.

http://codereview.chromium.org/1540009/diff/1/15
File chrome/browser/cocoa/location_bar_view_mac.mm (right):

http://codereview.chromium.org/1540009/diff/1/15#newcode123
chrome/browser/cocoa/location_bar_view_mac.mm:123:
star_icon_view_.SetVisible(true);
This is redundant with code in the star_icon_view_ constructor.  Do we need/want
it in both places?

http://codereview.chromium.org/1540009/diff/1/17
File chrome/browser/cocoa/toolbar_controller.mm (right):

http://codereview.chromium.org/1540009/diff/1/17#newcode824
chrome/browser/cocoa/toolbar_controller.mm:824: - (gfx::Rect)locationStackBounds
{
This whole method is rewritten in your other change, right?  Watch for merge
conflicts :)

[Scott Hess - ex-Googler, 2010-04-01 17:52:04]

On 2010/04/01 14:12:36, rohitrao wrote:
> LGTM
> 
> I'm starting to dislike how we have so many individual icon views to keep
track
> of in the cell, although I don't know if there's a good way to consolidate
them.
>  layedOutIcons: does a decent job of that, I suppose.

AFAICT there's no reason for the cell to know anything at all about these guys. 
There should be RHS items and LHS items, all of a generic base class.

In fact, I'm not entirely convinced about the distinct subclasses to override
mouse-down and stuff, especially for the single-instance classes.  An
alternative might be to have a mouse-down task so that LocationBarViewMac can
just wire itself into the loop.  I'll have to see how it looks.

> There's an existing bug for us not updating the cell when an extension changes
> its page action icon.  I haven't dug into how to fix that yet, but it might be
> good to add a way for the LocationBarImageViews to force a redraw directly.

Really?  I seem to recall this used to work.  Feel free to loop me in on the
bug.

> http://codereview.chromium.org/1540009/diff/1/9
> File chrome/browser/cocoa/bookmark_bubble_controller.mm (right):
> 
> http://codereview.chromium.org/1540009/diff/1/9#newcode103
> chrome/browser/cocoa/bookmark_bubble_controller.mm:103: [bubble_
> setArrowLocation:kTopRight];
> Hm, we should probably shift the bubble around when the default placement is
> partially/completely offscreen.  I'll file a bug for that.

I had to work hard to not dive in and start refactoring the code to have an
"arrow point", rather than requiring clients to use arbitrary adjustments of the
bubble window position.  Maybe an arrow point plus a left/right preference, or
maybe it could even determine left/right preference based on bubble placement
versus the parent window?

> http://codereview.chromium.org/1540009/diff/1/15
> File chrome/browser/cocoa/location_bar_view_mac.mm (right):
> 
> http://codereview.chromium.org/1540009/diff/1/15#newcode123
> chrome/browser/cocoa/location_bar_view_mac.mm:123:
> star_icon_view_.SetVisible(true);
> This is redundant with code in the star_icon_view_ constructor.  Do we
need/want
> it in both places?

Right you are, left StarIconView() to deal with that.

> http://codereview.chromium.org/1540009/diff/1/17
> File chrome/browser/cocoa/toolbar_controller.mm (right):
> 
> http://codereview.chromium.org/1540009/diff/1/17#newcode824
> chrome/browser/cocoa/toolbar_controller.mm:824: -
(gfx::Rect)locationStackBounds
> {
> This whole method is rewritten in your other change, right?  Watch for merge
> conflicts :)

Merge was easy - that entire section got deleted :-).  I just wanted to get it
in the pipeline, because I knew this change might need more fixing.

[Scott Hess - ex-Googler, 2010-04-01 17:59:11]

On 2010/04/01 14:12:36, rohitrao wrote:
> LGTM

BTW, I looped in Rohit because he's been seeing the other related reviews on the
page-actions junk, but Pink because making changes to the overall Toolbar seems
scary.  So if Pink doesn't want to look, mention it!

[Scott Hess - ex-Googler, 2010-04-01 18:04:39]

On 2010/04/01 05:22:45, viettrungluu wrote:
> Drive-by nag: Include a description of the .xib changes in the CL
> description/commit message, for when mysterious problems occur in the .xib and
> someone has to re-do them.

Done.

> Other silly drive-by comments below (i.e., this is not a review).
> 
> http://codereview.chromium.org/1540009/diff/1/17
> File chrome/browser/cocoa/toolbar_controller.mm (right):
> 
> http://codereview.chromium.org/1540009/diff/1/17#newcode397
> chrome/browser/cocoa/toolbar_controller.mm:397: [backButton_
> setEnabled:commands->IsCommandEnabled(IDC_BACK) ? YES : NO];
> Argh. See below.
> 
> http://codereview.chromium.org/1540009/diff/1/17#newcode406
> chrome/browser/cocoa/toolbar_controller.mm:406: locationBarView_->Update(tab,
> shouldRestore ? true : false);
> Hey, here too (see below)!
> 
> http://codereview.chromium.org/1540009/diff/1/17#newcode416
> chrome/browser/cocoa/toolbar_controller.mm:416:
> locationBarView_->SetStarred(isStarred ? true : false);
> Drive-by nit: no need to do any conversion of BOOL to bool.
> 
> http://codereview.chromium.org/1540009/diff/1/17#newcode513
> chrome/browser/cocoa/toolbar_controller.mm:513: BOOL hide =
> showHomeButton_.GetValue() ? NO : YES;
> I wonder what was wrong with |!showHomeButton_.GetValue()|.

I don't recall the giant argument about this from early on in the project.  I do
recall that the consensus was that you should do the conversion when you're
passing things around.  I think it came down to whether you could reliably test
YES==true, though it might have been something other than that.  If you really
want to know, I think mentovai knew.

And I might remember things wrong.  But this CL wants to follow the style of the
surrounding code :-).

[pink (ping after 24hrs), 2010-04-01 18:10:18]

pink is out the rest of this week, which is why i'm not reviewing.

On Thu, Apr 1, 2010 at 10:59 AM,  <shess@chromium.org> wrote:
> On 2010/04/01 14:12:36, rohitrao wrote:
>>
>> LGTM
>
> BTW, I looped in Rohit because he's been seeing the other related reviews on
> the
> page-actions junk, but Pink because making changes to the overall Toolbar
> seems
> scary.  So if Pink doesn't want to look, mention it!
>
> http://codereview.chromium.org/1540009
>



-- 
Mike Pinkerton
Mac Weenie
pinkerton@google.com

[Scott Hess - ex-Googler, 2010-04-01 18:17:38]

Aha.  I'm glad you're online, otherwise I'd not have realized until Friday :-).

Rohit, WDYT?

-scott


On Thu, Apr 1, 2010 at 11:10 AM, Mike Pinkerton <pinkerton@chromium.org> wrote:
> pink is out the rest of this week, which is why i'm not reviewing.
>
> On Thu, Apr 1, 2010 at 10:59 AM,  <shess@chromium.org> wrote:
>> On 2010/04/01 14:12:36, rohitrao wrote:
>>>
>>> LGTM
>>
>> BTW, I looped in Rohit because he's been seeing the other related reviews on
>> the
>> page-actions junk, but Pink because making changes to the overall Toolbar
>> seems
>> scary.  So if Pink doesn't want to look, mention it!
>>
>> http://codereview.chromium.org/1540009
>>
>
>
>
> --
> Mike Pinkerton
> Mac Weenie
> pinkerton@google.com
>

[rohitrao (ping after 24h), 2010-04-01 18:45:14]

> Rohit, WDYT?

The browser/toolbar changes don't look particularly scary to me.  Double-check
that things behave when hiding/showing the home button, then submit away.