Let Chrome app handle Ash accelerators first if the app is launched as a window

Let Chrome app handle Ash accelerators first if the app is launched as a window.

Currently, Ash accelerators are handled at a very early stage, right after a native key event is received by aura::RootWindowHost. This CL change the way of handling Ash accelerators as follows to make it more App friendly:

1. If no window is focused, handle an Ash accelerator immediately in ash/accelerators/accelerator_filter.cc in the same way as before.
2. Otherwise, do not handle it in ash/accelerators/accelerator_filter.cc but let a custom views::FocusManager handle it (see ash/shell.cc). There are 3 types of scenarios here depending on the type of the focused window:
2-a. If the focused window is a browser, and the browser is not for an app, let the custom focus manager pre-handle Ash accelerators before passing it to the browser (see PreHandleKeyboardEvent() in chrome/browser/ui/views/frame/browser_view.cc).
2-b. If the focused window is a browser, and the browser is for an app, let the app handle Ash accelerators first (see chrome/browser/ui/views/frame/browser_view.cc). If the accelerator is not consumed by the app, let the custom focus manager handle it.
2-c. If the focused window is not a browser, let the window handle Ash accelerators first. If the accelerator is not consumed by the window, then let the custom focus manager handle it. This means a WebView without the chrome/browser/ui/ layer can handle Ash accelerators first whenever needed.

Other changes:

chrome/browser/ui/views/frame/browser_view.cc:
Support ET_KEY_RELEASED accelerators in BrowserView::PreHandleKeyboardEvent().

ui/views/focus/focus_manager.cc:
Support ET_KEY_RELEASED accelerators. Also fix code for handing VKEY_MENU so that the Shift+Alt+ET_KEY_RELEASED accelerator for Ash could be handled correctly.

This CL depends on http://codereview.chromium.org/10377158/ (by jochen), https://chromiumcodereview.appspot.com/10388023, http://codereview.chromium.org/10389035/, and https://chromiumcodereview.appspot.com/10332051/, and should not be submitted until the 4 CLs are landed.

BUG=123856
TEST=ran aura_shell_unittests
TEST=manual; launch Chromoting app as a window, connect to a Chromoting server, focus Chrome on the remote machine, press Ctrl-n, confirm a new window is opened on the remote machine.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=135791

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

[Ben Goodger (Google), 2012-05-01 16:00:37]

https://chromiumcodereview.appspot.com/10134036/diff/7013/ash/accelerators/ac...
File ash/accelerators/accelerator_controller.h (right):

https://chromiumcodereview.appspot.com/10134036/diff/7013/ash/accelerators/ac...
ash/accelerators/accelerator_controller.h:60: bool IsAccelerator(const
ui::Accelerator& accelerator);
IsRegistered()?

https://chromiumcodereview.appspot.com/10134036/diff/7013/ash/shell_delegate.h
File ash/shell_delegate.h (right):

https://chromiumcodereview.appspot.com/10134036/diff/7013/ash/shell_delegate....
ash/shell_delegate.h:63: virtual bool IsBrowserWindow(const aura::Window*
window) = 0;
IsWebContentsView()

https://chromiumcodereview.appspot.com/10134036/diff/7013/chrome/browser/ui/v...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://chromiumcodereview.appspot.com/10134036/diff/7013/chrome/browser/ui/v...
chrome/browser/ui/views/frame/browser_view.cc:195: bool
ProcessAshAcceleratorInternal(const NativeWebKeyboardEvent& event,
Can we move this back to src/ash?

I don't like having code required for ash hard coded here.

Think of it this way. What if I were to use views::WebView to wrap a
WebContents... and then wanted to have the app I hosted in that setting to be
able to handle these events. I think this belongs at a different layer.

[Yusuke Sato, 2012-05-02 16:40:11]

It seems that some of interactive_ui_tests for linux_chromeos are red and I have
to fix my code or the tests, but for now, could you let me know if the direction
of Patch Set #4 is good?

https://chromiumcodereview.appspot.com/10134036/diff/7013/ash/accelerators/ac...
File ash/accelerators/accelerator_controller.h (right):

https://chromiumcodereview.appspot.com/10134036/diff/7013/ash/accelerators/ac...
ash/accelerators/accelerator_controller.h:60: bool IsAccelerator(const
ui::Accelerator& accelerator);
On 2012/05/01 16:00:37, Ben Goodger (Google) wrote:
> IsRegistered()?

Removed the function.

https://chromiumcodereview.appspot.com/10134036/diff/7013/ash/shell_delegate.h
File ash/shell_delegate.h (right):

https://chromiumcodereview.appspot.com/10134036/diff/7013/ash/shell_delegate....
ash/shell_delegate.h:63: virtual bool IsBrowserWindow(const aura::Window*
window) = 0;
On 2012/05/01 16:00:37, Ben Goodger (Google) wrote:
> IsWebContentsView()

I've added code directly to accelerator_filter.cc which checks if an
aura::Window is for a Web contents view. Please let me keep the interface as-is
since I also need a way to know whether the |window| is a non-App browser window
or not.

https://chromiumcodereview.appspot.com/10134036/diff/7013/chrome/browser/ui/v...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://chromiumcodereview.appspot.com/10134036/diff/7013/chrome/browser/ui/v...
chrome/browser/ui/views/frame/browser_view.cc:195: bool
ProcessAshAcceleratorInternal(const NativeWebKeyboardEvent& event,
Thanks for the suggestion. Migrated to Ash-only solution which should support
views::WebView.

On 2012/05/01 16:00:37, Ben Goodger (Google) wrote:
> Can we move this back to src/ash?
> 
> I don't like having code required for ash hard coded here.
> 
> Think of it this way. What if I were to use views::WebView to wrap a
> WebContents... and then wanted to have the app I hosted in that setting to be
> able to handle these events. I think this belongs at a different layer.

[Yusuke Sato, 2012-05-03 15:56:24]

On 2012/05/02 16:40:11, Yusuke Sato wrote:
> It seems that some of interactive_ui_tests for linux_chromeos are red and I
have
> to fix my code or the tests, but for now, could you let me know if the
direction
> of Patch Set #4 is good?

Fixed interactive_ui_tests. Please take another look.

-Yusuke

[Ben Goodger (Google), 2012-05-03 17:29:38]

http://codereview.chromium.org/10134036/diff/7013/ash/shell_delegate.h
File ash/shell_delegate.h (right):

http://codereview.chromium.org/10134036/diff/7013/ash/shell_delegate.h#newcode63
ash/shell_delegate.h:63: virtual bool IsBrowserWindow(const aura::Window*
window) = 0;
On 2012/05/02 16:40:11, Yusuke Sato wrote:
> aura::Window is for a Web contents view. Please let me keep the interface
as-is
> since I also need a way to know whether the |window| is a non-App browser
window
> or not.

Why do you need to know if it is a browser window?

[Yusuke Sato, 2012-05-03 17:41:37]

On 2012/05/03 17:29:38, Ben Goodger (Google) wrote:
> http://codereview.chromium.org/10134036/diff/7013/ash/shell_delegate.h
> File ash/shell_delegate.h (right):
> 
>
http://codereview.chromium.org/10134036/diff/7013/ash/shell_delegate.h#newcode63
> ash/shell_delegate.h:63: virtual bool IsBrowserWindow(const aura::Window*
> window) = 0;
> On 2012/05/02 16:40:11, Yusuke Sato wrote:
> > aura::Window is for a Web contents view. Please let me keep the interface
> as-is
> > since I also need a way to know whether the |window| is a non-App browser
> window
> > or not.
> 
> Why do you need to know if it is a browser window?

This is because when it is a (non-app) browser window, an Ash accelerator such
as Ctrl+N and F6-F10 has to be processed in Ash before the accelerator is passed
to the browser.

[Yusuke Sato, 2012-05-03 17:45:58]

http://codereview.chromium.org/10134036/diff/7013/ash/shell_delegate.h
File ash/shell_delegate.h (right):

http://codereview.chromium.org/10134036/diff/7013/ash/shell_delegate.h#newcode63
ash/shell_delegate.h:63: virtual bool IsBrowserWindow(const aura::Window*
window) = 0;
On 2012/05/03 17:29:38, Ben Goodger (Google) wrote:
> On 2012/05/02 16:40:11, Yusuke Sato wrote:
> > aura::Window is for a Web contents view. Please let me keep the interface
> as-is
> > since I also need a way to know whether the |window| is a non-App browser
> window
> > or not.
> 
> Why do you need to know if it is a browser window?

This is because when it is a (non-app) browser window, an Ash accelerator such
as Ctrl+N and F6-F10 has to be processed in Ash before the accelerator is passed
to the browser.

[Ben Goodger (Google), 2012-05-03 22:22:30]

On 2012/05/03 17:45:58, Yusuke Sato wrote:
> > Why do you need to know if it is a browser window?
> 
> This is because when it is a (non-app) browser window, an Ash accelerator such
> as Ctrl+N and F6-F10 has to be processed in Ash before the accelerator is
passed
> to the browser.

I can see F6-F10 (those are the special keys, right?) but not Ctrl+N??

Anyway I'd like this not to be cast in terms of whether or not it's a browser
window. I'd rather see it stated as something specific to key handling.

[Yusuke Sato, 2012-05-04 12:51:39]

On 2012/05/03 22:22:30, Ben Goodger (Google) wrote:
> On 2012/05/03 17:45:58, Yusuke Sato wrote:
> > > Why do you need to know if it is a browser window?
> > 
> > This is because when it is a (non-app) browser window, an Ash accelerator
such
> > as Ctrl+N and F6-F10 has to be processed in Ash before the accelerator is
> passed
> > to the browser.
> 
> I can see F6-F10 (those are the special keys, right?) but not Ctrl+N??
> 
> Anyway I'd like this not to be cast in terms of whether or not it's a browser
> window. I'd rather see it stated as something specific to key handling.

Sure, I've removed the new function from ash::ShellDelegate and stopped checking
type of the target window in accelerator_filter.cc (instead, browser_view.cc has
slightly been modified). For details, please check the CL description I've just
rewritten.

-Yusuke

[Ben Goodger (Google), 2012-05-04 16:25:33]

http://codereview.chromium.org/10134036/diff/44013/ash/shell.cc
File ash/shell.cc (right):

http://codereview.chromium.org/10134036/diff/44013/ash/shell.cc#newcode512
ash/shell.cc:512: #if !defined(OS_MACOSX)
why the OS_MACOSX?

http://codereview.chromium.org/10134036/diff/44013/ash/shell.cc#newcode525
ash/shell.cc:525: target);
This injection makes me a little scared. Is there a way to have the focus
manager delegate to a higher power if there is no handler?

i.e. in PreHandleKeyboardEvent, you would just pass to the focus manager to
handle... and if the focus manager can't handle then there is some aura client
interface that code in ash implements to receive unhandled accelerators.

http://codereview.chromium.org/10134036/diff/44013/chrome/browser/ui/views/fr...
File chrome/browser/ui/views/frame/browser_view.cc (right):

http://codereview.chromium.org/10134036/diff/44013/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/browser_view.cc:1247: if (target == this)
not sure if I understand the target checking. was BrowserView previously always
the accelerator target for browser window keyboard shortcuts??

[Yusuke Sato, 2012-05-07 08:27:28]

Please take another look.

http://codereview.chromium.org/10134036/diff/44013/ash/shell.cc
File ash/shell.cc (right):

http://codereview.chromium.org/10134036/diff/44013/ash/shell.cc#newcode512
ash/shell.cc:512: #if !defined(OS_MACOSX)
On 2012/05/04 16:25:33, Ben Goodger (Google) wrote:
> why the OS_MACOSX?

Because accelerator_controller() is currently not available on Mac OS X.
According to dhollowa@, Mac Aura does not work with the accelerator controller
yet.

http://codereview.chromium.org/10134036/diff/44013/ash/shell.cc#newcode525
ash/shell.cc:525: target);
Done.

On 2012/05/04 16:25:33, Ben Goodger (Google) wrote:
> This injection makes me a little scared. Is there a way to have the focus
> manager delegate to a higher power if there is no handler?

Added ui/views/focus/focus_manager_delegate.h and simplified
AshFocusManagerFactory.

> i.e. in PreHandleKeyboardEvent, you would just pass to the focus manager to
> handle... and if the focus manager can't handle then there is some aura client
> interface that code in ash implements to receive unhandled accelerators.

Removed the |target| checking code from PreHandleKeyboardEvent. Not the function
simply passes an accelerator to the focus manager.

http://codereview.chromium.org/10134036/diff/44013/chrome/browser/ui/views/fr...
File chrome/browser/ui/views/frame/browser_view.cc (right):

http://codereview.chromium.org/10134036/diff/44013/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/browser_view.cc:1247: if (target == this)
Done.

On 2012/05/04 16:25:33, Ben Goodger (Google) wrote:
> not sure if I understand the target checking. was BrowserView previously
always
> the accelerator target for browser window keyboard shortcuts??

Yes, it was (but I've removed the target checking to make the code simpler.)

[Ben Goodger (Google), 2012-05-07 15:42:51]

lgtm

http://codereview.chromium.org/10134036/diff/46029/ash/shell.cc
File ash/shell.cc (right):

http://codereview.chromium.org/10134036/diff/46029/ash/shell.cc#newcode505
ash/shell.cc:505: class AshFocusManagerFactory : public
views::FocusManagerFactory {
Can we move this closer to the accelerators stuff rather than hosting it here?
This file is getting a bit cramped.

http://codereview.chromium.org/10134036/diff/46029/ash/shell.cc#newcode521
ash/shell.cc:521: #if !defined(OS_MACOSX)
dhollowa has become distracted from implementing MacOS/ash stuff for the time
being. I imagine he'll be distracted for at least six months. We should not
bother supporting OS_MACOSX like this at this level right now.

[Yusuke Sato, 2012-05-08 02:15:57]

http://codereview.chromium.org/10134036/diff/46029/ash/shell.cc
File ash/shell.cc (right):

http://codereview.chromium.org/10134036/diff/46029/ash/shell.cc#newcode505
ash/shell.cc:505: class AshFocusManagerFactory : public
views::FocusManagerFactory {
On 2012/05/07 15:42:51, Ben Goodger (Google) wrote:
> Can we move this closer to the accelerators stuff rather than hosting it here?
> This file is getting a bit cramped.

Done.

http://codereview.chromium.org/10134036/diff/46029/ash/shell.cc#newcode521
ash/shell.cc:521: #if !defined(OS_MACOSX)
On 2012/05/07 15:42:51, Ben Goodger (Google) wrote:
> dhollowa has become distracted from implementing MacOS/ash stuff for the time
> being. I imagine he'll be distracted for at least six months. We should not
> bother supporting OS_MACOSX like this at this level right now.

Done.

[commit-bot: I haz the power, 2012-05-08 03:40:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yusukes@chromium.org/10134036/38016

[commit-bot: I haz the power, 2012-05-08 04:03:05]

Try job failure for 10134036-38016 on win_rel for step "update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[Yusuke Sato, 2012-05-10 15:44:17]

Ben, could you take another look? I've fixed the following issues of Patch Set
#10:

30692: Audio and brightness buttons (F6, F7, F8, F9, F10) don't work when Flash
video is playing in full screen mode. (crosbug)

Unlike normal web content, Pepper seems to use a raw RenderWidgetHostImpl (i.e.
not RenderViewHostImpl) when full screen. Since RenderWidgetHostImpl's
PreHandleKeyEvent() is empty, Ash shortcuts including F6-10 cannot be processed.
Fixed the issue by adding
content/browser/renderer_host/render_widget_host_factory.* interface and
ash/accelerators/render_widget_host_factory.* implementation, and use the
interface in content/browser/web_contents/web_contents_view_helper.cc.

127333: ash::AcceleratorController::AcceleratorPressed() is called twice for a
single keypress.

Removed F5-F10, that had been added just in case, from
Browser::IsReservedCommandOrKey() in browser.cc.

127275:	Backspace key is not working in crosh terminal and Google
docs/spreadsheet
127240:	backspace not working in many websites on windows

Found that a RawKeyDown event for VKEY_BACK was consumed in PreHandleKeyEvent()
in browser_view.cc by mistake. Fixed the function.

Other files are unchanged.

Thanks,
Yusuke

[Yusuke Sato, 2012-05-14 08:07:02]

ping?

On 2012/05/10 15:44:17, Yusuke Sato wrote:
> Ben, could you take another look? I've fixed the following issues of Patch Set
> #10:
> 
> 30692: Audio and brightness buttons (F6, F7, F8, F9, F10) don't work when
Flash
> video is playing in full screen mode. (crosbug)
> 
> Unlike normal web content, Pepper seems to use a raw RenderWidgetHostImpl
(i.e.
> not RenderViewHostImpl) when full screen. Since RenderWidgetHostImpl's
> PreHandleKeyEvent() is empty, Ash shortcuts including F6-10 cannot be
processed.
> Fixed the issue by adding
> content/browser/renderer_host/render_widget_host_factory.* interface and
> ash/accelerators/render_widget_host_factory.* implementation, and use the
> interface in content/browser/web_contents/web_contents_view_helper.cc.
> 
> 127333: ash::AcceleratorController::AcceleratorPressed() is called twice for a
> single keypress.
> 
> Removed F5-F10, that had been added just in case, from
> Browser::IsReservedCommandOrKey() in browser.cc.
> 
> 127275:	Backspace key is not working in crosh terminal and Google
> docs/spreadsheet
> 127240:	backspace not working in many websites on windows
> 
> Found that a RawKeyDown event for VKEY_BACK was consumed in
PreHandleKeyEvent()
> in browser_view.cc by mistake. Fixed the function.
> 
> Other files are unchanged.
> 
> Thanks,
> Yusuke

[Ben Goodger (Google), 2012-05-14 17:10:55]

http://codereview.chromium.org/10134036/diff/49016/ash/accelerators/render_wi...
File ash/accelerators/render_widget_host_factory.h (right):

http://codereview.chromium.org/10134036/diff/49016/ash/accelerators/render_wi...
ash/accelerators/render_widget_host_factory.h:10: #include
"content/browser/renderer_host/render_widget_host_factory.h"
You're not allowed to include from anything other than content/public outside of
content. Can you talk to content-team@ about how best to do this?

[Yusuke Sato, 2012-05-15 02:15:19]

+jam, +content-team

http://codereview.chromium.org/10134036/diff/49016/ash/accelerators/render_wi...
File ash/accelerators/render_widget_host_factory.h (right):

http://codereview.chromium.org/10134036/diff/49016/ash/accelerators/render_wi...
ash/accelerators/render_widget_host_factory.h:10: #include
"content/browser/renderer_host/render_widget_host_factory.h"
On 2012/05/14 17:10:56, Ben Goodger (Google) wrote:
> You're not allowed to include from anything other than content/public outside
of
> content. Can you talk to content-team@ about how best to do this?

Hi John, content-team@,
Could you give me some advice about content/ part of my CL?

I'm trying to move Ash shortcuts (like F7-10 for volume control) from ash/ to
the browser side to make the shortcut key handing process more Apps friendly
(crbug.com/123856). With the CL, the BrowserView::PreHandleKeyEvent() function
called by RenderViewHostImpl::PreHandleKeyboardEvent() handles an Ash shortcut
in most cases, which is straight forward.

The problem is full-screen Pepper. When a Pepper window becomes full-screen, a
RenderWidgetHostImpl (without RenderViewHostImpl) object seems to be created,
and since RWHI's PreHandleKeyboardEvent() is empty, an Ash shortcut like F7 is
simply ignored in this case. To fix the problem, I added a factory class called
content/browser/renderer_host/render_widget_host_factory.h, which is very
similar to render_view_host_factory.h, and used the new factory class inside
ash/ to override RWHI's PreHandleKeyboardEvent(), but according to Ben's
comment, the way seems to violate Content API's design.

So my question is: Could you let me know the best way to customize RWHI's
PreHandleKeyboardEvent() behavior for Ash?

Thanks!
-Yusuke

[jochen (gone - plz use gerrit), 2012-05-15 09:33:59]

I would propose to add a RenderWidgetHostDelegate (there's already an
implicit definition for it in render_widget_host_impl.h) and move the
respective methods like PreHandleKeyboardEvent from RenderViewHostDelegate
to RenderWidgetHostDelegate.

Instead of making the methods like RWHI::PreHandleKeyboardEvent virtual and
overriding them in RVHI, RWHI would immediately call out to its delegate,
and a RVHD that is interested in these signals should also implement RWHD.

WebContentsImpl would then implement RWHD as well, and the
WebContentsViewHelper could pass the WebContentsImpl it gets in the
CreateNewWidget call to RWHI.

does that make sense?
-jochen

On Tue, May 15, 2012 at 4:15 AM, <yusukes@chromium.org> wrote:

> +jam, +content-team
>
>
>
> http://codereview.chromium.**org/10134036/diff/49016/ash/**
>
accelerators/render_widget_**host_factory.h<http://codereview.chromium.org/10134036/diff/49016/ash/accelerators/render_widget_host_factory.h>
> File ash/accelerators/render_**widget_host_factory.h (right):
>
> http://codereview.chromium.**org/10134036/diff/49016/ash/**
>
accelerators/render_widget_**host_factory.h#newcode10<http://codereview.chromium.org/10134036/diff/49016/ash/accelerators/render_widget_host_factory.h#newcode10>
> ash/accelerators/render_**widget_host_factory.h:10: #include
> "content/browser/renderer_**host/render_widget_host_**factory.h"
> On 2012/05/14 17:10:56, Ben Goodger (Google) wrote:
>
>> You're not allowed to include from anything other than content/public
>>
> outside of
>
>> content. Can you talk to content-team@ about how best to do this?
>>
>
> Hi John, content-team@,
> Could you give me some advice about content/ part of my CL?
>
> I'm trying to move Ash shortcuts (like F7-10 for volume control) from
> ash/ to the browser side to make the shortcut key handing process more
> Apps friendly (crbug.com/123856). With the CL, the
> BrowserView::**PreHandleKeyEvent() function called by
> RenderViewHostImpl::**PreHandleKeyboardEvent() handles an Ash shortcut in
> most cases, which is straight forward.
>
> The problem is full-screen Pepper. When a Pepper window becomes
> full-screen, a RenderWidgetHostImpl (without RenderViewHostImpl) object
> seems to be created, and since RWHI's PreHandleKeyboardEvent() is empty,
> an Ash shortcut like F7 is simply ignored in this case. To fix the
> problem, I added a factory class called
> content/browser/renderer_host/**render_widget_host_factory.h, which is
> very similar to render_view_host_factory.h, and used the new factory
> class inside ash/ to override RWHI's PreHandleKeyboardEvent(), but
> according to Ben's comment, the way seems to violate Content API's
> design.
>
> So my question is: Could you let me know the best way to customize
> RWHI's PreHandleKeyboardEvent() behavior for Ash?
>
> Thanks!
> -Yusuke
>
>
http://codereview.chromium.**org/10134036/<http://codereview.chromium.org/101...
>

[jam, 2012-05-15 17:07:24]

On 2012/05/15 09:33:59, jochen wrote:
> I would propose to add a RenderWidgetHostDelegate (there's already an
> implicit definition for it in render_widget_host_impl.h) and move the
> respective methods like PreHandleKeyboardEvent from RenderViewHostDelegate
> to RenderWidgetHostDelegate.

We're trying to move RenderViewHostDelegate out of the content public API, so we
wouldn't want to add RenderWidgetHostDelegate (that and we've avoided adding
RWHD for a long time).

> 
> Instead of making the methods like RWHI::PreHandleKeyboardEvent virtual and
> overriding them in RVHI, RWHI would immediately call out to its delegate,
> and a RVHD that is interested in these signals should also implement RWHD.
> 
> WebContentsImpl would then implement RWHD as well, and the
> WebContentsViewHelper could pass the WebContentsImpl it gets in the
> CreateNewWidget call to RWHI.
> 
> does that make sense?
> -jochen
> 
> On Tue, May 15, 2012 at 4:15 AM, <mailto:yusukes@chromium.org> wrote:
> 
> > +jam, +content-team
> >
> >
> >
> > http://codereview.chromium.**org/10134036/diff/49016/ash/**
> >
>
accelerators/render_widget_**host_factory.h<http://codereview.chromium.org/10134036/diff/49016/ash/accelerators/render_widget_host_factory.h>
> > File ash/accelerators/render_**widget_host_factory.h (right):
> >
> > http://codereview.chromium.**org/10134036/diff/49016/ash/**
> >
>
accelerators/render_widget_**host_factory.h#newcode10<http://codereview.chromium.org/10134036/diff/49016/ash/accelerators/render_widget_host_factory.h#newcode10>
> > ash/accelerators/render_**widget_host_factory.h:10: #include
> > "content/browser/renderer_**host/render_widget_host_**factory.h"
> > On 2012/05/14 17:10:56, Ben Goodger (Google) wrote:
> >
> >> You're not allowed to include from anything other than content/public
> >>
> > outside of
> >
> >> content. Can you talk to content-team@ about how best to do this?
> >>
> >
> > Hi John, content-team@,
> > Could you give me some advice about content/ part of my CL?
> >
> > I'm trying to move Ash shortcuts (like F7-10 for volume control) from
> > ash/ to the browser side to make the shortcut key handing process more
> > Apps friendly (crbug.com/123856). With the CL, the
> > BrowserView::**PreHandleKeyEvent() function called by
> > RenderViewHostImpl::**PreHandleKeyboardEvent() handles an Ash shortcut in
> > most cases, which is straight forward.
> >
> > The problem is full-screen Pepper. When a Pepper window becomes
> > full-screen, a RenderWidgetHostImpl (without RenderViewHostImpl) object
> > seems to be created, 

I'm curious why Pepper creates a RenderWidgetHostImpl directly instead of a
RenderViewHostImpl, or even WebContents? We 

> > and since RWHI's PreHandleKeyboardEvent() is empty,
> > an Ash shortcut like F7 is simply ignored in this case. To fix the
> > problem, I added a factory class called
> > content/browser/renderer_host/**render_widget_host_factory.h, which is
> > very similar to render_view_host_factory.h, and used the new factory
> > class inside ash/ to override RWHI's PreHandleKeyboardEvent(), but
> > according to Ben's comment, the way seems to violate Content API's
> > design.
> >
> > So my question is: Could you let me know the best way to customize
> > RWHI's PreHandleKeyboardEvent() behavior for Ash?
> >
> > Thanks!
> > -Yusuke
> >
> >
>
http://codereview.chromium.**org/10134036/%3Chttp://codereview.chromium.org/1...>
> >

[jochen (gone - plz use gerrit), 2012-05-15 17:56:05]

On Tue, May 15, 2012 at 7:07 PM, <jam@chromium.org> wrote:

> On 2012/05/15 09:33:59, jochen wrote:
>
>> I would propose to add a RenderWidgetHostDelegate (there's already an
>> implicit definition for it in render_widget_host_impl.h) and move the
>> respective methods like PreHandleKeyboardEvent from RenderViewHostDelegate
>> to RenderWidgetHostDelegate.
>>
>
> We're trying to move RenderViewHostDelegate out of the content public API,
> so we
> wouldn't want to add RenderWidgetHostDelegate (that and we've avoided
> adding
> RWHD for a long time).
>
>
>
>  Instead of making the methods like RWHI::PreHandleKeyboardEvent virtual
>> and
>> overriding them in RVHI, RWHI would immediately call out to its delegate,
>> and a RVHD that is interested in these signals should also implement RWHD.
>>
>
>  WebContentsImpl would then implement RWHD as well, and the
>> WebContentsViewHelper could pass the WebContentsImpl it gets in the
>> CreateNewWidget call to RWHI.
>>
>
>  does that make sense?
>> -jochen
>>
>
>  On Tue, May 15, 2012 at 4:15 AM, <mailto:yusukes@chromium.org> wrote:
>>
>
>  > +jam, +content-team
>> >
>> >
>> >
>> > http://codereview.chromium.****org/10134036/diff/49016/ash/**
>> >
>>
>
> accelerators/render_widget_****host_factory.h<http://**
> codereview.chromium.org/**10134036/diff/49016/ash/**
>
accelerators/render_widget_**host_factory.h<http://codereview.chromium.org/10134036/diff/49016/ash/accelerators/render_widget_host_factory.h>
> >
>
>> > File ash/accelerators/render_****widget_host_factory.h (right):
>> >
>> > http://codereview.chromium.****org/10134036/diff/49016/ash/**
>> >
>>
>
> accelerators/render_widget_****host_factory.h#newcode10<http:**
> //codereview.chromium.org/**10134036/diff/49016/ash/**
>
accelerators/render_widget_**host_factory.h#newcode10<http://codereview.chromium.org/10134036/diff/49016/ash/accelerators/render_widget_host_factory.h#newcode10>
> >
>
>> > ash/accelerators/render_****widget_host_factory.h:10: #include
>> > "content/browser/renderer_****host/render_widget_host_****factory.h"
>>
>> > On 2012/05/14 17:10:56, Ben Goodger (Google) wrote:
>> >
>> >> You're not allowed to include from anything other than content/public
>> >>
>> > outside of
>> >
>> >> content. Can you talk to content-team@ about how best to do this?
>> >>
>> >
>> > Hi John, content-team@,
>> > Could you give me some advice about content/ part of my CL?
>> >
>> > I'm trying to move Ash shortcuts (like F7-10 for volume control) from
>> > ash/ to the browser side to make the shortcut key handing process more
>> > Apps friendly (crbug.com/123856). With the CL, the
>> > BrowserView::****PreHandleKeyEvent() function called by
>> > RenderViewHostImpl::****PreHandleKeyboardEvent() handles an Ash
>> shortcut in
>>
>> > most cases, which is straight forward.
>> >
>> > The problem is full-screen Pepper. When a Pepper window becomes
>> > full-screen, a RenderWidgetHostImpl (without RenderViewHostImpl) object
>> > seems to be created,
>>
>
> I'm curious why Pepper creates a RenderWidgetHostImpl directly instead of a
> RenderViewHostImpl, or even WebContents? We
>

There is lots of code in WebContents that assumes that it is displaying a
tab, e.g. only the RenderViewHostManager can shut down hosts.
RenderWidgetHostView* on the other hand has some host management code
itself, e.g. if you hit escape it shuts down the host as well. These code
paths are (hopefully) never executed when the view is attached to a web
contents.

Of course it is arguable whether that code should actually live there.


>
>  > and since RWHI's PreHandleKeyboardEvent() is empty,
>> > an Ash shortcut like F7 is simply ignored in this case. To fix the
>> > problem, I added a factory class called
>> > content/browser/renderer_host/****render_widget_host_factory.**h,
>> which is
>>
>> > very similar to render_view_host_factory.h, and used the new factory
>> > class inside ash/ to override RWHI's PreHandleKeyboardEvent(), but
>> > according to Ben's comment, the way seems to violate Content API's
>> > design.
>> >
>> > So my question is: Could you let me know the best way to customize
>> > RWHI's PreHandleKeyboardEvent() behavior for Ash?
>> >
>> > Thanks!
>> > -Yusuke
>> >
>> >
>>
>
> http://codereview.chromium.****org/10134036/%3Chttp://coderev**
> iew.chromium.org/10134036/ <http://codereview.chromium.org/10134036/>>
>
>> >
>>
>
>
>
>
http://codereview.chromium.**org/10134036/<http://codereview.chromium.org/101...
>

[Yusuke Sato, 2012-05-16 11:12:17]

Ben, could you take another look? Since the full-screen Pepper issue will be
fixed by jochen's CL http://codereview.chromium.org/10377158/ soon (thanks!),
I've reverted all of my changes against content/.

2 files (chrome/browser/ui/browser.cc and
hrome/browser/ui/views/frame/browser_view.cc) have been modified since the last
commit (#10), and others are unchanged.

127333: ash::AcceleratorController::AcceleratorPressed() is called twice for a
single keypress.

Removed F5-F10, that had been added just in case, from
Browser::IsReservedCommandOrKey() in browser.cc.

127275:	Backspace key is not working in crosh terminal and Google
docs/spreadsheet
127240:	backspace not working in many websites on windows

Found that a RawKeyDown event for VKEY_BACK was consumed in PreHandleKeyEvent()
in browser_view.cc by mistake. Fixed the function.

-Yusuke

On 2012/05/15 17:56:05, jochen wrote:
> On Tue, May 15, 2012 at 7:07 PM, <mailto:jam@chromium.org> wrote:
> 
> > On 2012/05/15 09:33:59, jochen wrote:
> >
> >> I would propose to add a RenderWidgetHostDelegate (there's already an
> >> implicit definition for it in render_widget_host_impl.h) and move the
> >> respective methods like PreHandleKeyboardEvent from RenderViewHostDelegate
> >> to RenderWidgetHostDelegate.
> >>
> >
> > We're trying to move RenderViewHostDelegate out of the content public API,
> > so we
> > wouldn't want to add RenderWidgetHostDelegate (that and we've avoided
> > adding
> > RWHD for a long time).
> >
> >
> >
> >  Instead of making the methods like RWHI::PreHandleKeyboardEvent virtual
> >> and
> >> overriding them in RVHI, RWHI would immediately call out to its delegate,
> >> and a RVHD that is interested in these signals should also implement RWHD.
> >>
> >
> >  WebContentsImpl would then implement RWHD as well, and the
> >> WebContentsViewHelper could pass the WebContentsImpl it gets in the
> >> CreateNewWidget call to RWHI.
> >>
> >
> >  does that make sense?
> >> -jochen
> >>
> >
> >  On Tue, May 15, 2012 at 4:15 AM, <mailto:yusukes@chromium.org> wrote:
> >>
> >
> >  > +jam, +content-team
> >> >
> >> >
> >> >
> >> > http://codereview.chromium.****org/10134036/diff/49016/ash/**
> >> >
> >>
> >
> > accelerators/render_widget_****host_factory.h<http://**
> > codereview.chromium.org/**10134036/diff/49016/ash/**
> >
>
accelerators/render_widget_**host_factory.h<http://codereview.chromium.org/10134036/diff/49016/ash/accelerators/render_widget_host_factory.h>
> > >
> >
> >> > File ash/accelerators/render_****widget_host_factory.h (right):
> >> >
> >> > http://codereview.chromium.****org/10134036/diff/49016/ash/**
> >> >
> >>
> >
> > accelerators/render_widget_****host_factory.h#newcode10<http:**
> > //codereview.chromium.org/**10134036/diff/49016/ash/**
> >
>
accelerators/render_widget_**host_factory.h#newcode10<http://codereview.chromium.org/10134036/diff/49016/ash/accelerators/render_widget_host_factory.h#newcode10>
> > >
> >
> >> > ash/accelerators/render_****widget_host_factory.h:10: #include
> >> > "content/browser/renderer_****host/render_widget_host_****factory.h"
> >>
> >> > On 2012/05/14 17:10:56, Ben Goodger (Google) wrote:
> >> >
> >> >> You're not allowed to include from anything other than content/public
> >> >>
> >> > outside of
> >> >
> >> >> content. Can you talk to content-team@ about how best to do this?
> >> >>
> >> >
> >> > Hi John, content-team@,
> >> > Could you give me some advice about content/ part of my CL?
> >> >
> >> > I'm trying to move Ash shortcuts (like F7-10 for volume control) from
> >> > ash/ to the browser side to make the shortcut key handing process more
> >> > Apps friendly (crbug.com/123856). With the CL, the
> >> > BrowserView::****PreHandleKeyEvent() function called by
> >> > RenderViewHostImpl::****PreHandleKeyboardEvent() handles an Ash
> >> shortcut in
> >>
> >> > most cases, which is straight forward.
> >> >
> >> > The problem is full-screen Pepper. When a Pepper window becomes
> >> > full-screen, a RenderWidgetHostImpl (without RenderViewHostImpl) object
> >> > seems to be created,
> >>
> >
> > I'm curious why Pepper creates a RenderWidgetHostImpl directly instead of a
> > RenderViewHostImpl, or even WebContents? We
> >
> 
> There is lots of code in WebContents that assumes that it is displaying a
> tab, e.g. only the RenderViewHostManager can shut down hosts.
> RenderWidgetHostView* on the other hand has some host management code
> itself, e.g. if you hit escape it shuts down the host as well. These code
> paths are (hopefully) never executed when the view is attached to a web
> contents.
> 
> Of course it is arguable whether that code should actually live there.
> 
> 
> >
> >  > and since RWHI's PreHandleKeyboardEvent() is empty,
> >> > an Ash shortcut like F7 is simply ignored in this case. To fix the
> >> > problem, I added a factory class called
> >> > content/browser/renderer_host/****render_widget_host_factory.**h,
> >> which is
> >>
> >> > very similar to render_view_host_factory.h, and used the new factory
> >> > class inside ash/ to override RWHI's PreHandleKeyboardEvent(), but
> >> > according to Ben's comment, the way seems to violate Content API's
> >> > design.
> >> >
> >> > So my question is: Could you let me know the best way to customize
> >> > RWHI's PreHandleKeyboardEvent() behavior for Ash?
> >> >
> >> > Thanks!
> >> > -Yusuke
> >> >
> >> >
> >>
> >
> > http://codereview.chromium.****org/10134036/%253Chttp://coderev**
> > iew.chromium.org/10134036/ <http://codereview.chromium.org/10134036/>>
> >
> >> >
> >>
> >
> >
> >
> >
>
http://codereview.chromium.**org/10134036/%3Chttp://codereview.chromium.org/1...>
> >

[Ben Goodger (Google), 2012-05-16 16:41:40]

LGTM, but can you please make a set of diagrams showing the flow of accelerators
through the system for various cases.

[Yusuke Sato, 2012-05-17 02:22:04]

On 2012/05/16 16:41:40, Ben Goodger (Google) wrote:
> LGTM, but can you please make a set of diagrams showing the flow of
accelerators
> through the system for various cases.

Thanks. Filed crbug.com/128476

[commit-bot: I haz the power, 2012-05-17 02:28:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yusukes@chromium.org/10134036/51026

[commit-bot: I haz the power, 2012-05-17 04:42:38]

Change committed as 137629