Fix accelerators in avatar gaia webview on mac

Fix accelerators in avatar gaia webview on mac

BUG=401023
TEST=launch chrome with flag --enable-new-avatar-menu, open the new avatar menu, click signin to chrome, in the embedded gaia webview, try cmd+a and cmd+v, both should work.

Committed: https://crrev.com/feb45f0af1d064866f0c519306bf66725c5e6d48
Cr-Commit-Position: refs/heads/master@{#301986}

[guohui, 2014-10-28 17:15:58]

guohui@chromium.org changed reviewers:
+ asvitkine@chromium.org, rogerta@chromium.org, sky@chromium.org

[guohui, 2014-10-28 17:15:59]

Hey,

could you please review the CL?

sky@ for ui/views,
alexei@ for ui/cocoa,
rogerta@ for signin and overall cl,

Thanks,

Hui

[sky, 2014-10-28 17:46:54]

https://codereview.chromium.org/682113002/diff/1/chrome/browser/ui/views/prof...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/682113002/diff/1/chrome/browser/ui/views/prof...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:715: #ifndef NDEBUG
#if !defined(NDEBUG_)

Also, spacing is off, and document why the return value differs.

[guohui, 2014-10-28 18:02:53]

https://codereview.chromium.org/682113002/diff/1/chrome/browser/ui/views/prof...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/682113002/diff/1/chrome/browser/ui/views/prof...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:715: #ifndef NDEBUG
On 2014/10/28 17:46:54, sky wrote:
> #if !defined(NDEBUG_)
> 
> Also, spacing is off, and document why the return value differs.

Done.

[Alexei Svitkine (slow), 2014-10-28 18:27:23]

https://codereview.chromium.org/682113002/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.h (right):

https://codereview.chromium.org/682113002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.h:60:
scoped_ptr<GaiaWebContentsDelegate> webContentsDelegate_;
Does this need to be a scoped ptr, or can it just be a member?

https://codereview.chromium.org/682113002/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right):

https://codereview.chromium.org/682113002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:258: 
Nit: No need for empty line here.

https://codereview.chromium.org/682113002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:259: virtual
~GaiaWebContentsDelegate() {}
Nit: I think the preferred syntax is now:

~GaiaWebContentsDelegate() override {}

https://codereview.chromium.org/682113002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:267: };
DISALLOW_COPY_AND_ASSIGN()

https://codereview.chromium.org/682113002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:285: int
chromeCommandId = [BrowserWindowUtils getCommandId:event];
Use hacker_style in C++ classes.

https://codereview.chromium.org/682113002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/profiles/profile_chooser_view.h (right):

https://codereview.chromium.org/682113002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/profile_chooser_view.h:95: // Overridden from
content::WebContentsDelegate.
Nit: Remove "Overridden from " prefix from comment.

[guohui, 2014-10-28 18:53:40]

https://codereview.chromium.org/682113002/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.h (right):

https://codereview.chromium.org/682113002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.h:60:
scoped_ptr<GaiaWebContentsDelegate> webContentsDelegate_;
On 2014/10/28 18:27:23, Alexei Svitkine wrote:
> Does this need to be a scoped ptr, or can it just be a member?

By using the scoped pointer, we only instantiate webContentsDelegate_ when
needed, that is, when a user navigates to the gaia signin view.

https://codereview.chromium.org/682113002/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right):

https://codereview.chromium.org/682113002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:258: 
On 2014/10/28 18:27:23, Alexei Svitkine wrote:
> Nit: No need for empty line here.

Done.

https://codereview.chromium.org/682113002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:259: virtual
~GaiaWebContentsDelegate() {}
On 2014/10/28 18:27:23, Alexei Svitkine wrote:
> Nit: I think the preferred syntax is now:
> 
> ~GaiaWebContentsDelegate() override {}

Done.

https://codereview.chromium.org/682113002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:267: };
On 2014/10/28 18:27:23, Alexei Svitkine wrote:
> DISALLOW_COPY_AND_ASSIGN()

Done.

https://codereview.chromium.org/682113002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:285: int
chromeCommandId = [BrowserWindowUtils getCommandId:event];
On 2014/10/28 18:27:23, Alexei Svitkine wrote:
> Use hacker_style in C++ classes.

Done.

https://codereview.chromium.org/682113002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/profiles/profile_chooser_view.h (right):

https://codereview.chromium.org/682113002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/profile_chooser_view.h:95: // Overridden from
content::WebContentsDelegate.
On 2014/10/28 18:27:23, Alexei Svitkine wrote:
> Nit: Remove "Overridden from " prefix from comment.

Done.

[Roger Tawa OOO till Jul 10th, 2014-10-28 18:58:13]

https://codereview.chromium.org/682113002/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right):

https://codereview.chromium.org/682113002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:290:
event.windowsKeyCode == ui::VKEY_V);
How about VKEY_C ?

[Alexei Svitkine (slow), 2014-10-28 18:58:42]

lgtm % nits

Please add a TEST= line to the CL since this is changing user-visible behavior.

https://codereview.chromium.org/682113002/diff/40001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right):

https://codereview.chromium.org/682113002/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:261: //
Overridden from content::WebContentsDelegate.
Nit: Change comment to:

// content::WebContentsDelegate:

https://codereview.chromium.org/682113002/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/profiles/profile_chooser_view.h (right):

https://codereview.chromium.org/682113002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/profile_chooser_view.h:95: //
content::WebContentsDelegate.
Nit: . -> :

[guohui, 2014-10-28 19:02:39]

thanks alexsei!

https://codereview.chromium.org/682113002/diff/40001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right):

https://codereview.chromium.org/682113002/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:261: //
Overridden from content::WebContentsDelegate.
On 2014/10/28 18:58:42, Alexei Svitkine wrote:
> Nit: Change comment to:
> 
> // content::WebContentsDelegate:

Done.

https://codereview.chromium.org/682113002/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/profiles/profile_chooser_view.h (right):

https://codereview.chromium.org/682113002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/profile_chooser_view.h:95: //
content::WebContentsDelegate.
On 2014/10/28 18:58:42, Alexei Svitkine wrote:
> Nit: . -> :

Done.

[sky, 2014-10-28 22:14:04]

https://codereview.chromium.org/682113002/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/682113002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:716: #ifndef NDEBUG
#if !defined(NDEBUG)

[guohui, 2014-10-29 18:02:08]

https://codereview.chromium.org/682113002/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/682113002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:716: #ifndef NDEBUG
On 2014/10/28 22:14:04, sky wrote:
> #if !defined(NDEBUG)

Done.

[sky, 2014-10-29 19:20:28]

https://codereview.chromium.org/682113002/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/682113002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:715: // Suppresses the
context menu in non-debug mode.
This just documents what the code does and isn't helpful. *Why* do we want to
suppress in release only? Don't we want debug/release to behave the same?

[guohui, 2014-10-29 19:25:34]

https://codereview.chromium.org/682113002/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/682113002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:715: // Suppresses the
context menu in non-debug mode.
On 2014/10/29 19:20:28, sky wrote:
> This just documents what the code does and isn't helpful. *Why* do we want to
> suppress in release only? Don't we want debug/release to behave the same?

because some functions in context menu, such as inspecting elements, could be
useful for chrome developers, but not so for regular users. But i guess a
developer can easily modify the source code to achieve this, and i dont have a
clear use case for it now, so i ll just remove the logic.

[sky, 2014-10-29 19:33:51]

Excellent. Please document why true is a good return value though.

On Wed, Oct 29, 2014 at 12:25 PM,  <guohui@chromium.org> wrote:
>
>
https://codereview.chromium.org/682113002/diff/80001/chrome/browser/ui/views/...
> File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):
>
>
https://codereview.chromium.org/682113002/diff/80001/chrome/browser/ui/views/...
> chrome/browser/ui/views/profiles/profile_chooser_view.cc:715: //
> Suppresses the context menu in non-debug mode.
> On 2014/10/29 19:20:28, sky wrote:
>>
>> This just documents what the code does and isn't helpful. *Why* do we
>
> want to
>>
>> suppress in release only? Don't we want debug/release to behave the
>
> same?
>
> because some functions in context menu, such as inspecting elements,
> could be useful for chrome developers, but not so for regular users. But
> i guess a developer can easily modify the source code to achieve this,
> and i dont have a clear use case for it now, so i ll just remove the
> logic.
>
> https://codereview.chromium.org/682113002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[guohui, 2014-10-29 22:01:30]

On 2014/10/29 19:33:51, sky wrote:
> Excellent. Please document why true is a good return value though.
> 
> On Wed, Oct 29, 2014 at 12:25 PM,  <mailto:guohui@chromium.org> wrote:
> >
> >
>
https://codereview.chromium.org/682113002/diff/80001/chrome/browser/ui/views/...
> > File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):
> >
> >
>
https://codereview.chromium.org/682113002/diff/80001/chrome/browser/ui/views/...
> > chrome/browser/ui/views/profiles/profile_chooser_view.cc:715: //
> > Suppresses the context menu in non-debug mode.
> > On 2014/10/29 19:20:28, sky wrote:
> >>
> >> This just documents what the code does and isn't helpful. *Why* do we
> >
> > want to
> >>
> >> suppress in release only? Don't we want debug/release to behave the
> >
> > same?
> >
> > because some functions in context menu, such as inspecting elements,
> > could be useful for chrome developers, but not so for regular users. But
> > i guess a developer can easily modify the source code to achieve this,
> > and i dont have a clear use case for it now, so i ll just remove the
> > logic.
> >
> > https://codereview.chromium.org/682113002/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

comment added thanks!

[sky, 2014-10-29 22:02:49]

LGTM

[guohui, 2014-10-29 22:02:57]

The CQ bit was checked by guohui@chromium.org

[commit-bot: I haz the power, 2014-10-29 22:04:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/682113002/120001

[commit-bot: I haz the power, 2014-10-30 00:10:42]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2014-10-30 00:11:25]

Patchset 7 (id:??) landed as
https://crrev.com/feb45f0af1d064866f0c519306bf66725c5e6d48
Cr-Commit-Position: refs/heads/master@{#301986}