bookmarks: Add more keyboard accelerators to bookmark bar context menu.

bookmarks: Add more keyboard accelerators to bookmark bar context menu.

BUG=76250
TEST=see bug

R=sky@chromium.org

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

[sky, 2011-08-08 14:19:48]

If we're going for completeness, this still leaves a handful of entries without
mnemonics.

  -Scott

[tfarina, 2011-08-08 14:47:30]

On 2011/08/08 14:19:48, sky wrote:
> If we're going for completeness, this still leaves a handful of entries
without
> mnemonics.
> 
Done.

[sky, 2011-08-08 16:41:20]

http://codereview.chromium.org/7575004/diff/4002/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7575004/diff/4002/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:6258: Open all bookmarks
What about this and the next two?

[tfarina, 2011-08-08 17:02:59]

http://codereview.chromium.org/7575004/diff/4002/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7575004/diff/4002/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:6258: Open all bookmarks
On 2011/08/08 16:41:20, sky wrote:
> What about this and the next two?

Done.

[tfarina, 2011-08-09 21:57:37]

Hi Peter, could you review this to me?

[Peter Kasting, 2011-08-09 22:10:46]

http://codereview.chromium.org/7575004/diff/8001/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7575004/diff/8001/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:6239: <message
name="IDS_BOOMARK_BAR_ALWAYS_SHOW" desc="Menu item for having bookmark bar
always visible">
Can we eliminate this and make code that uses it use IDS_SHOW_BOOKMARK_BAR
instead?  That will give this an accelerator as well ('A').

http://codereview.chromium.org/7575004/diff/8001/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:6262: Add p&amp;age...
This conflicts with the 'A' for 'Always show bookmarks bar'.  Maybe use 'g'
instead?

[tfarina, 2011-08-09 23:58:48]

http://codereview.chromium.org/7575004/diff/8001/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7575004/diff/8001/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:6239: <message
name="IDS_BOOMARK_BAR_ALWAYS_SHOW" desc="Menu item for having bookmark bar
always visible">
On 2011/08/09 22:10:46, Peter Kasting wrote:
> Can we eliminate this and make code that uses it use IDS_SHOW_BOOKMARK_BAR
> instead?  That will give this an accelerator as well ('A').

When I replace IDS_BOOKMARK_BAR_ALWAYS_SHOW by IDS_SHOW_BOOKMARK_BAR I get a
weird context menu entries. I don't know why though.

http://codereview.chromium.org/7575004/diff/8001/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:6262: Add p&amp;age...
On 2011/08/09 22:10:46, Peter Kasting wrote:
> This conflicts with the 'A' for 'Always show bookmarks bar'.
The 'A' doesn't work for me here on Linux.

>  Maybe use 'g' instead?
When I change to 'g', strangely enough the underscore doesn't appear in the menu
item, but the keyboard accelerator works. So I didn't changed.

[Peter Kasting, 2011-08-10 00:05:47]

On 2011/08/09 23:58:48, tfarina wrote:
>
http://codereview.chromium.org/7575004/diff/8001/chrome/app/generated_resourc...
> File chrome/app/generated_resources.grd (right):
> 
>
http://codereview.chromium.org/7575004/diff/8001/chrome/app/generated_resourc...
> chrome/app/generated_resources.grd:6239: <message
> name="IDS_BOOMARK_BAR_ALWAYS_SHOW" desc="Menu item for having bookmark bar
> always visible">
> On 2011/08/09 22:10:46, Peter Kasting wrote:
> > Can we eliminate this and make code that uses it use IDS_SHOW_BOOKMARK_BAR
> > instead?  That will give this an accelerator as well ('A').
> 
> When I replace IDS_BOOKMARK_BAR_ALWAYS_SHOW by IDS_SHOW_BOOKMARK_BAR I get a
> weird context menu entries. I don't know why though.

That is too vague for me to diagnose, and it makes no sense given that these two
are the same except for "&amp;".  Without seeing your change and/or screenshots
I don't know what to tell you.

>
http://codereview.chromium.org/7575004/diff/8001/chrome/app/generated_resourc...
> chrome/app/generated_resources.grd:6262: Add p&amp;age...
> On 2011/08/09 22:10:46, Peter Kasting wrote:
> > This conflicts with the 'A' for 'Always show bookmarks bar'.
> The 'A' doesn't work for me here on Linux.

You have to switch to IDS_SHOW_BOOKMARK_BAR to get it, did you?

> >  Maybe use 'g' instead?
> When I change to 'g', strangely enough the underscore doesn't appear in the
menu
> item, but the keyboard accelerator works. So I didn't changed.

I don't have a clue why that would happen.  Maybe there are bugs in the Linux
menus or something.  I don't see why it should stop you from adding the
accelerator.

[tfarina, 2011-08-10 00:37:19]

On Tue, Aug 9, 2011 at 9:05 PM,  <pkasting@chromium.org> wrote:
>> When I replace IDS_BOOKMARK_BAR_ALWAYS_SHOW by IDS_SHOW_BOOKMARK_BAR I get
>> a
>> weird context menu entries. I don't know why though.
>
> That is too vague for me to diagnose, and it makes no sense given that these
> two
> are the same except for "&amp;".  Without seeing your change and/or
> screenshots
> I don't know what to tell you.
>
Screenshots here -> http://i.imgur.com/YHaWB.png

>
>
http://codereview.chromium.org/7575004/diff/8001/chrome/app/generated_resourc...
>>
>> chrome/app/generated_resources.grd:6262: Add p&amp;age...
>> On 2011/08/09 22:10:46, Peter Kasting wrote:
>> > This conflicts with the 'A' for 'Always show bookmarks bar'.
>> The 'A' doesn't work for me here on Linux.
>
> You have to switch to IDS_SHOW_BOOKMARK_BAR to get it, did you?
>
Yeah, switching to IDS_SHOW_BOOKMARK_BAR made it works.

-- 
Thiago

[Peter Kasting, 2011-08-10 00:40:25]

On Tue, Aug 9, 2011 at 5:37 PM, Thiago Farina <tfarina@chromium.org> wrote:

> On Tue, Aug 9, 2011 at 9:05 PM,  <pkasting@chromium.org> wrote:
> >> When I replace IDS_BOOKMARK_BAR_ALWAYS_SHOW by IDS_SHOW_BOOKMARK_BAR I
> get
> >> a
> >> weird context menu entries. I don't know why though.
> >
> > That is too vague for me to diagnose, and it makes no sense given that
> these
> > two
> > are the same except for "&amp;".  Without seeing your change and/or
> > screenshots
> > I don't know what to tell you.
> >
> Screenshots here -> http://i.imgur.com/YHaWB.png


This looks as if you removed IDS_BOOKMARK_BAR_ALWAYS_SHOW from the grd file
but didn't sufficiently recompile enough of Chrome, so some pieces are
getting confused about which strings go with which IDs.

PK

[tfarina, 2011-08-10 01:33:14]

On 2011/08/10 00:40:25, Peter Kasting wrote:
> On Tue, Aug 9, 2011 at 5:37 PM, Thiago Farina <mailto:tfarina@chromium.org>
wrote:
> 
> > On Tue, Aug 9, 2011 at 9:05 PM,  <mailto:pkasting@chromium.org> wrote:
> > >> When I replace IDS_BOOKMARK_BAR_ALWAYS_SHOW by IDS_SHOW_BOOKMARK_BAR I
> > get
> > >> a
> > >> weird context menu entries. I don't know why though.
> > >
> > > That is too vague for me to diagnose, and it makes no sense given that
> > these
> > > two
> > > are the same except for "&amp;".  Without seeing your change and/or
> > > screenshots
> > > I don't know what to tell you.
> > >
> > Screenshots here -> http://i.imgur.com/YHaWB.png
> 
> 
> This looks as if you removed IDS_BOOKMARK_BAR_ALWAYS_SHOW from the grd file
> but didn't sufficiently recompile enough of Chrome, so some pieces are
> getting confused about which strings go with which IDs.
> 
You are right. Exactly. There must be an issue with ninja. I switched back to
make and worked.

The issue with 'g' still persist, but I've added to this change as the
accelerator works.

Please, could you take another look?

[Peter Kasting, 2011-08-10 01:35:22]

LGTM except for one issue

http://codereview.chromium.org/7575004/diff/16001/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (left):

http://codereview.chromium.org/7575004/diff/16001/chrome/app/generated_resour...
chrome/app/generated_resources.grd:10034: <message
name="IDS_BOOMARK_BAR_ALWAYS_SHOW_MAC" desc="The Mac menu item for having
bookmark bar always visible in the view menu.">
This should not be removed.  (Note that it doesn't have &amp; in it.)

[tfarina, 2011-08-10 01:39:09]

http://codereview.chromium.org/7575004/diff/16001/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (left):

http://codereview.chromium.org/7575004/diff/16001/chrome/app/generated_resour...
chrome/app/generated_resources.grd:10034: <message
name="IDS_BOOMARK_BAR_ALWAYS_SHOW_MAC" desc="The Mac menu item for having
bookmark bar always visible in the view menu.">
On 2011/08/10 01:35:22, Peter Kasting wrote:
> This should not be removed.  (Note that it doesn't have &amp; in it.)

Ahh, good catch! I missed it because 'git gs' doesn't search in .xib files. I'll
restore it.

[tfarina, 2011-08-10 01:45:01]

http://codereview.chromium.org/7575004/diff/16001/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (left):

http://codereview.chromium.org/7575004/diff/16001/chrome/app/generated_resour...
chrome/app/generated_resources.grd:10034: <message
name="IDS_BOOMARK_BAR_ALWAYS_SHOW_MAC" desc="The Mac menu item for having
bookmark bar always visible in the view menu.">
On 2011/08/10 01:35:22, Peter Kasting wrote:
> This should not be removed.  (Note that it doesn't have &amp; in it.)

Done. Restored and fixed the typo, it should be BOOKMARK instead of BOOMARK.

[Peter Kasting, 2011-08-10 01:48:20]

LGTM