Added menu items for sync to Chrome/wrench menu

Added menu item for sync to wrench menu.

 Wrench menu item is right above "Preferences...".  Chrome menu item will be added in a future CL right above "Clear Browsing Data...".  Sync menu items are hidden if bookmark sync is disabled (the current default for OS X).  UI decisions were made after consulting with Cole.

Added code in browser_window_controller.mm to update sync menu item dynamically.

Added unit tests.

BUG=23073
TEST=manual testing, trybots, unittests

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

[akalin, 2009-11-05 00:50:28]

+nick to review sync stuff
+avi to review cocoa stuff

Also, sync is still disabled for OS X (at compile time) so the sync menu item
will always show up as greyed out.  (Although I did test with sync enabled.)

[akalin, 2009-11-05 01:13:48]

thakis pointed out that the wrench menu is hidden on OS X by default.  So we
probably want to move this menu item to the main menu bar.  Although that seems
like something that requires a UI discussion, so I'd rather do that in a
separate CL...

On 2009/11/05 00:50:28, akalin wrote:
> +nick to review sync stuff
> +avi to review cocoa stuff
> 
> Also, sync is still disabled for OS X (at compile time) so the sync menu item
> will always show up as greyed out.  (Although I did test with sync enabled.)

[ncarter (slow), 2009-11-05 01:36:53]

It looks good to me.

One question I have is whether or not it's okay to show the greyed-out menu item
before sync is user-ready; e.g., in some upcoming beta version of the mac
browser that wouldn't offer sync as a feature.

http://codereview.chromium.org/363017/diff/1005/1008
File chrome/browser/cocoa/toolbar_controller.mm (right):

http://codereview.chromium.org/363017/diff/1005/1008#newcode221
Line 221: [[wrenchButton_ attachedMenu] indexOfItemWithTag:IDC_SYNC_BOOKMARKS];
Does this need 2 more spaces of indent?  I'm not familiar enough with the objc
style to know what's consistent.

[Avi (use Gerrit), 2009-11-05 01:58:35]

LG

http://codereview.chromium.org/363017/diff/1005/1008
File chrome/browser/cocoa/toolbar_controller.mm (right):

http://codereview.chromium.org/363017/diff/1005/1008#newcode221
Line 221: [[wrenchButton_ attachedMenu] indexOfItemWithTag:IDC_SYNC_BOOKMARKS];
On 2009/11/05 01:36:53, nick wrote:
> Does this need 2 more spaces of indent?  I'm not familiar enough with the objc
> style to know what's consistent.

Yes it does. ObjC doesn't change the indenting rules; it only adds new ones for
new situations, which this is not.

[TVL, 2009-11-05 03:54:09]

drive by -

Should you talk to Cole about putting it in the Menubar some place?

Don't we already query menu status before showing the menu, why can't this
use that methods instead of having to find/update the menu manually?

TVL


On Wed, Nov 4, 2009 at 7:50 PM, <akalin@chromium.org> wrote:

>
> Reviewers: nick, Avi,
>
> Message:
> +nick to review sync stuff
> +avi to review cocoa stuff
>
> Also, sync is still disabled for OS X (at compile time) so the sync menu
> item
> will always show up as greyed out.  (Although I did test with sync
> enabled.)
>
> Description:
> Added menu item to wrench menu for sync.
>
> Made toolbar controller a delegate of the wrench menu so that it can update
> the
> sync menu item before the wrench menu is displayed.
>
> Added unit tests.
>
> BUG=23073
> TEST=manual testing, trybots, unittests
>
> Please review this at http://codereview.chromium.org/363017
>
> Affected files:
>  M chrome/app/nibs/Toolbar.xib
>  M chrome/browser/cocoa/toolbar_controller.h
>  M chrome/browser/cocoa/toolbar_controller.mm
>  M chrome/browser/cocoa/toolbar_controller_unittest.mm
>
>
>

[akalin, 2009-11-05 08:09:29]

On 2009/11/05 03:54:09, TVL wrote:
> drive by -
> 
> Should you talk to Cole about putting it in the Menubar some place?
> 
> Don't we already query menu status before showing the menu, why can't this
> use that methods instead of having to find/update the menu manually?

I dug a little further and it seems we do; I will rework this CL.  I may as well
talk to Cole, too.  I'm thinking putting it above 'Preferences...' analogous to
its position in the wrench menu.

> 
> TVL
> 
> 
> On Wed, Nov 4, 2009 at 7:50 PM, <mailto:akalin@chromium.org> wrote:
> 
> >
> > Reviewers: nick, Avi,
> >
> > Message:
> > +nick to review sync stuff
> > +avi to review cocoa stuff
> >
> > Also, sync is still disabled for OS X (at compile time) so the sync menu
> > item
> > will always show up as greyed out.  (Although I did test with sync
> > enabled.)
> >
> > Description:
> > Added menu item to wrench menu for sync.
> >
> > Made toolbar controller a delegate of the wrench menu so that it can update
> > the
> > sync menu item before the wrench menu is displayed.
> >
> > Added unit tests.
> >
> > BUG=23073
> > TEST=manual testing, trybots, unittests
> >
> > Please review this at http://codereview.chromium.org/363017
> >
> > Affected files:
> >  M chrome/app/nibs/Toolbar.xib
> >  M chrome/browser/cocoa/toolbar_controller.h
> >  M chrome/browser/cocoa/toolbar_controller.mm
> >  M chrome/browser/cocoa/toolbar_controller_unittest.mm
> >
> >
> >
>

[akalin, 2009-11-05 08:13:39]

On 2009/11/05 03:54:09, TVL wrote:
> drive by -
> 
> Should you talk to Cole about putting it in the Menubar some place?

Also, Cole is J. Nicholas Jitkoff right?

> 
> Don't we already query menu status before showing the menu, why can't this
> use that methods instead of having to find/update the menu manually?
> 
> TVL
> 
> 
> On Wed, Nov 4, 2009 at 7:50 PM, <mailto:akalin@chromium.org> wrote:
> 
> >
> > Reviewers: nick, Avi,
> >
> > Message:
> > +nick to review sync stuff
> > +avi to review cocoa stuff
> >
> > Also, sync is still disabled for OS X (at compile time) so the sync menu
> > item
> > will always show up as greyed out.  (Although I did test with sync
> > enabled.)
> >
> > Description:
> > Added menu item to wrench menu for sync.
> >
> > Made toolbar controller a delegate of the wrench menu so that it can update
> > the
> > sync menu item before the wrench menu is displayed.
> >
> > Added unit tests.
> >
> > BUG=23073
> > TEST=manual testing, trybots, unittests
> >
> > Please review this at http://codereview.chromium.org/363017
> >
> > Affected files:
> >  M chrome/app/nibs/Toolbar.xib
> >  M chrome/browser/cocoa/toolbar_controller.h
> >  M chrome/browser/cocoa/toolbar_controller.mm
> >  M chrome/browser/cocoa/toolbar_controller_unittest.mm
> >
> >
> >
>

[akalin, 2009-11-05 22:06:08]

+tvl

Thomas, since you did a drive-by, can you look at this again?

[TVL, 2009-11-05 22:16:55]

http://codereview.chromium.org/363017/diff/8001/9001
File chrome/app/nibs/MainMenu.xib (right):

http://codereview.chromium.org/363017/diff/8001/9001#newcode102
Line 102: <string key="NSTitle">^IDS_SYNC_MY_BOOKMARKS_LABEL</string>
with all the other menubar ones we went with different constants just incase
some language needed different phrasing, can you put a new constant in the right
area of the grd file for this (usually just existing name with _MAC on the end,
you'll see them all)

http://codereview.chromium.org/363017/diff/8001/9004
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/363017/diff/8001/9004#newcode629
Line 629: if (![sync_item isKindOfClass:[NSMenuItem class]]) {
maybe just DCHECK this so you catch bugs in debug, but release doesn't always
have to do the check every time it updates the menu?

[akalin (wrong akalin), 2009-11-05 22:38:17]

Ignore this, for some reason this message arrived half an hour after I
sent it. ??:(

On Thu, Nov 5, 2009 at 2:06 PM,  <akalin@chromium.org> wrote:
> +tvl
>
> Thomas, since you did a drive-by, can you look at this again?
>
> http://codereview.chromium.org/363017
>



-- 
Frederick Akalin
Software Engineer

[akalin, 2009-11-06 01:46:54]

On 2009/11/05 22:16:55, TVL wrote:
> http://codereview.chromium.org/363017/diff/8001/9001
> File chrome/app/nibs/MainMenu.xib (right):
> 
> http://codereview.chromium.org/363017/diff/8001/9001#newcode102
> Line 102: <string key="NSTitle">^IDS_SYNC_MY_BOOKMARKS_LABEL</string>
> with all the other menubar ones we went with different constants just incase
> some language needed different phrasing, can you put a new constant in the
right
> area of the grd file for this (usually just existing name with _MAC on the
end,
> you'll see them all)

I decided to do the MainMenu change in another CL, but I'll do this when I do
the MainMenu change.

> 
> http://codereview.chromium.org/363017/diff/8001/9004
> File chrome/browser/cocoa/browser_window_controller.mm (right):
> 
> http://codereview.chromium.org/363017/diff/8001/9004#newcode629
> Line 629: if (![sync_item isKindOfClass:[NSMenuItem class]]) {
> maybe just DCHECK this so you catch bugs in debug, but release doesn't always
> have to do the check every time it updates the menu?

Fixed.

I reworked this CL a bit, too.  Please take another look.

[TVL, 2009-11-06 13:51:42]

variable names within objc classes/methods shouldn't use underscores, they use
the capital letters instead.

http://codereview.chromium.org/363017/diff/12001/12003
File chrome/browser/cocoa/browser_window_controller.h (right):

http://codereview.chromium.org/363017/diff/12001/12003#newcode190
Line 190: - (void)updateSyncItem:(id)sync_item
use objc style names "syncItem" and "syncEnabled"

http://codereview.chromium.org/363017/diff/12001/12004
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/363017/diff/12001/12004#newcode588
Line 588: ProfileSyncService* sync_service = profile->GetProfileSyncService();
objc names

http://codereview.chromium.org/363017/diff/12001/12004#newcode623
Line 623: - (void)updateSyncItem:(id)sync_item
objc style names

http://codereview.chromium.org/363017/diff/12001/12004#newcode633
Line 633: DCHECK_NE(sync_item_index, -1);
s/-1/NSNotFount/

http://codereview.chromium.org/363017/diff/12001/12004#newcode654
Line 654: title_id = IDS_SYNC_START_SYNC_BUTTON_LABEL;
i believe the normal pattern is to NOTREACHED() in default or leave out the
default and list all possible cases.  That way the you better catch a new state
that wasn't expected.

http://codereview.chromium.org/363017/diff/12001/12004#newcode665
Line 665: [following_separator setEnabled:sync_enabled];
shouldn't need to control the enabled status on the sep (they should never be
selectable, etc.)

[pink (ping after 24hrs), 2009-11-06 17:52:28]

http://codereview.chromium.org/363017/diff/12001/12004
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/363017/diff/12001/12004#newcode633
Line 633: DCHECK_NE(sync_item_index, -1);
On 2009/11/06 13:51:43, TVL wrote:
> s/-1/NSNotFount/

He means NSNotFound :-)

[pink (ping after 24hrs), 2009-11-06 17:54:20]

Please familiarize yourself with

 http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml

Might make TVLs comments more clear.

[akalin (wrong akalin), 2009-11-06 19:27:54]

On Fri, Nov 6, 2009 at 5:51 AM,  <thomasvl@chromium.org> wrote:
> variable names within objc classes/methods shouldn't use underscores, the=
y
> use
> the capital letters instead.

Oops.  Fixed this everywhere.

> http://codereview.chromium.org/363017/diff/12001/12004#newcode633
> Line 633: DCHECK_NE(sync_item_index, -1);
> s/-1/NSNotFount/

Fixed.

> http://codereview.chromium.org/363017/diff/12001/12004#newcode654
> Line 654: title_id =3D IDS_SYNC_START_SYNC_BUTTON_LABEL;
> i believe the normal pattern is to NOTREACHED() in default or leave out
> the default and list all possible cases. =C2=A0That way the you better ca=
tch
> a new state that wasn't expected.

Changed to use NOTREACHED().

> http://codereview.chromium.org/363017/diff/12001/12004#newcode665
> Line 665: [following_separator setEnabled:sync_enabled];
> shouldn't need to control the enabled status on the sep (they should
> never be selectable, etc.)

Oops.  Fixed.

Please take another look!

--=20
Frederick Akalin
Software Engineer

[TVL, 2009-11-06 19:33:29]

lg

[akalin, 2009-11-06 19:54:07]

On 2009/11/06 17:52:28, pink wrote:
> http://codereview.chromium.org/363017/diff/12001/12004#newcode633
> Line 633: DCHECK_NE(sync_item_index, -1);
> On 2009/11/06 13:51:43, TVL wrote:
> > s/-1/NSNotFount/
> 
> He means NSNotFound :-)

NSNotFount: Constant denoting that an object does not, in fact, expel water.

Actually I checked the docs and that method returns -1, which is different from
NSNotFound (!).  Consulted tvl and am changing it back.

[akalin, 2009-11-06 20:29:05]

Committed as r31284.