Re: Let Chrome app handle Ash accelerators first if the app is launched as a window (issue 10134036)

[yus...@chromium.org]

+jam, +content-team



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
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/

[Jochen Eisinger]

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

[j...@chromium.org]

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:yus...@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/10134036/>
> >



http://codereview.chromium.org/10134036/

[Jochen Eisinger]

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.

[yus...@chromium.org]

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:

> >> > 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/>>

[b...@chromium.org]

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

https://chromiumcodereview.appspot.com/10134036/

[yus...@chromium.org]

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

http://codereview.chromium.org/10134036/

[commi...@chromium.org]

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


https://chromiumcodereview.appspot.com/10134036/

[commi...@chromium.org]

Change committed as 137629

https://chromiumcodereview.appspot.com/10134036/