RWHI should implement BrowserAccessibilityDelegate

RWHI should implement BrowserAccessibilityDelegate

This is just refactoring, there should be no behavior changes.
I realized that most of the functionality of
BrowserAccessibilityDelegate was implemented by
RenderWidgetHostImpl, but each RenderWidgetHostView*
was actually implementing the interface, resulting in
extra code. It's simpler to have RWHI implement it and
just call into RWHV for the few cases where the view is
involved.

This also removes the parallel mac-only delegate, since
it was duplicating most of the same functionality.
Calling OriginInScreen is a couple of extra lines of code
because it needs to convert from NSPoint & NSSize to
gfx::Rect, but in every other case it's the same or simpler
this way.

BUG=none
NOTRY=true

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

[jam, 2014-04-25 21:09:46]

https://codereview.chromium.org/246773008/diff/60001/content/port/browser/ren...
File content/port/browser/render_widget_host_view_port.h (right):

https://codereview.chromium.org/246773008/diff/60001/content/port/browser/ren...
content/port/browser/render_widget_host_view_port.h:331:
BrowserAccessibilityManager* manager) = 0;
this is an internal content class, yet it's being passed as an argument to a
port interface which can't happen. i.e. if someone was to implement
RenderWidgetHostViewPort in another repo, they wouldn't be able to access it.

[dmazzoni, 2014-04-28 21:52:05]

On Fri, Apr 25, 2014 at 2:09 PM, <jam@chromium.org> wrote:

>
> https://codereview.chromium.org/246773008/diff/60001/
> content/port/browser/render_widget_host_view_port.h
> File content/port/browser/render_widget_host_view_port.h (right):
>
> https://codereview.chromium.org/246773008/diff/60001/
> content/port/browser/render_widget_host_view_port.h#newcode331
> content/port/browser/render_widget_host_view_port.h:331:
> BrowserAccessibilityManager* manager) = 0;
> this is an internal content class, yet it's being passed as an argument
> to a port interface which can't happen. i.e. if someone was to implement
> RenderWidgetHostViewPort in another repo, they wouldn't be able to
> access it.
>

Uh-oh, it looks like we were already violating this for some time -
RenderWidgetHostViewPort has GetBrowserAccessibilityManager, and we’re
using it in several tests, and also in AccessibilityUI
(chrome://accessibility).

Here are some ideas:

1. An easy out is we could make the tests and AccessibilityUI only support
RenderWidgetHostViewBase - is it reasonable to do a dynamic type check
somehow and not support this in ports?

2. Or, we could put an abstract interface for BrowserAccessibilityManager
in content/port/browser and make it possible for 3rd-party ports to
implement accessibility. I don't think we need to let ports reimplement
BrowserAccessibilityManager, we'd just want them to be able to interface
with the existing implementation at a high level and hook it up to a
different RenderWidgetHostViewPort implementation.

3. Another ideas - the main reason each RWHV* is creating the
BrowserAccessibilityManager rather than RWHI is that it needs to call a
platform-specific constructor, and pass it the platform-specific parent
window. Perhaps we could define platform-specific functions to retrieve the
accessible parent window for each port to implement?

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

[jam, 2014-04-29 04:46:17]

first, apologies for the delay in replying, I missed your message. please IM me
in the future if I don't reply within a few hours.

On 2014/04/28 21:52:05, dmazzoni wrote:
> On Fri, Apr 25, 2014 at 2:09 PM, <mailto:jam@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.org/246773008/diff/60001/
> > content/port/browser/render_widget_host_view_port.h
> > File content/port/browser/render_widget_host_view_port.h (right):
> >
> > https://codereview.chromium.org/246773008/diff/60001/
> > content/port/browser/render_widget_host_view_port.h#newcode331
> > content/port/browser/render_widget_host_view_port.h:331:
> > BrowserAccessibilityManager* manager) = 0;
> > this is an internal content class, yet it's being passed as an argument
> > to a port interface which can't happen. i.e. if someone was to implement
> > RenderWidgetHostViewPort in another repo, they wouldn't be able to
> > access it.
> >
> 
> Uh-oh, it looks like we were already violating this for some time -
> RenderWidgetHostViewPort has GetBrowserAccessibilityManager, and we’re
> using it in several tests, and also in AccessibilityUI
> (chrome://accessibility).

ah, I missed that before. It's unfortunate that we don't have a way to catch
these sort of things, but i can't think of a good way.

> 
> Here are some ideas:
> 
> 1. An easy out is we could make the tests and AccessibilityUI only support
> RenderWidgetHostViewBase - is it reasonable to do a dynamic type check
> somehow and not support this in ports?

We want to avoid this since ports are supposed to be able to be first-class if
they implement interfaces in content/port, as opposed to having reduced
functionality.

> 
> 2. Or, we could put an abstract interface for BrowserAccessibilityManager
> in content/port/browser and make it possible for 3rd-party ports to
> implement accessibility. I don't think we need to let ports reimplement
> BrowserAccessibilityManager, we'd just want them to be able to interface
> with the existing implementation at a high level and hook it up to a
> different RenderWidgetHostViewPort implementation.

In general, it would be good to avoid adding new interfaces in content/port or
content/public unless we absolutely need them.

> 
> 3. Another ideas - the main reason each RWHV* is creating the
> BrowserAccessibilityManager rather than RWHI is that it needs to call a
> platform-specific constructor, and pass it the platform-specific parent
> window. Perhaps we could define platform-specific functions to retrieve the
> accessible parent window for each port to implement?

can we keep this entirely within content and use methods from existing
interfaces? i.e. RenderWidgetHostView::GetNative*?

[dmazzoni, 2014-04-29 15:03:51]

On Mon, Apr 28, 2014 at 9:46 PM, <jam@chromium.org> wrote:

> ah, I missed that before. It's unfortunate that we don't have a way to
> catch
> these sort of things, but i can't think of a good way.


Just brainstorming, but one thing that might help would be to enforce that
render_widget_host_view_port.h compiles without including any additional
header files, plus a presubmit that checks that nothing from
content/browser is included.

Also, some of the functions seem to take IPC message structs defined in
content/common, like ViewHostMsg_TextInputState_Params - is this also a
problem? If not, why not? Perhaps we should forbid forward declarations in
this file.

3. Another ideas - the main reason each RWHV* is creating the
>> BrowserAccessibilityManager rather than RWHI is that it needs to call a
>> platform-specific constructor, and pass it the platform-specific parent
>> window. Perhaps we could define platform-specific functions to retrieve
>> the
>> accessible parent window for each port to implement?
>>
>
> can we keep this entirely within content and use methods from existing
> interfaces? i.e. RenderWidgetHostView::GetNative*?
>

We'd need two more: GetParentNativeView, and GetParentNativeViewAccessible.
That would solve the problem for Win and Mac. I'd need to do even more
refactoring for Android but I think it's possible.

Let me try going this route and I'll let you know how it goes.

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

[jam, 2014-04-29 16:30:17]

On 2014/04/29 15:03:51, dmazzoni wrote:
> On Mon, Apr 28, 2014 at 9:46 PM, <mailto:jam@chromium.org> wrote:
> 
> > ah, I missed that before. It's unfortunate that we don't have a way to
> > catch
> > these sort of things, but i can't think of a good way.
> 
> 
> Just brainstorming, but one thing that might help would be to enforce that
> render_widget_host_view_port.h compiles without including any additional
> header files, plus a presubmit that checks that nothing from
> content/browser is included.
> 
> Also, some of the functions seem to take IPC message structs defined in
> content/common, like ViewHostMsg_TextInputState_Params - is this also a
> problem? If not, why not? Perhaps we should forbid forward declarations in
> this file.

ah, wow I didn't notice that before.

Ok, thanks a lot for bringing this topic up. I looked into it now, and CEF who
we had added these port interfaces for have switched to Aura, so we don't need
this anymore. I will remove this port interface for WebContentsView and
RenderWidgetHostView. That should make your patch simpler since you can keep as
is.

I think it'll be better if you land first, then I can do this; that way you're
not blocked on a cleanup.

lgtm

> 
> 3. Another ideas - the main reason each RWHV* is creating the
> >> BrowserAccessibilityManager rather than RWHI is that it needs to call a
> >> platform-specific constructor, and pass it the platform-specific parent
> >> window. Perhaps we could define platform-specific functions to retrieve
> >> the
> >> accessible parent window for each port to implement?
> >>
> >
> > can we keep this entirely within content and use methods from existing
> > interfaces? i.e. RenderWidgetHostView::GetNative*?
> >
> 
> We'd need two more: GetParentNativeView, and GetParentNativeViewAccessible.
> That would solve the problem for Win and Mac. I'd need to do even more
> refactoring for Android but I think it's possible.
> 
> Let me try going this route and I'll let you know how it goes.
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[dmazzoni, 2014-04-29 16:43:26]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2014-04-29 16:44:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/246773008/80001

[commit-bot: I haz the power, 2014-04-29 16:54:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-29 16:54:09]

Try jobs failed on following builders:
  android_clang_dbg on tryserver.chromium

[dmazzoni, 2014-04-29 17:45:19]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2014-04-29 17:46:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/246773008/100001

[commit-bot: I haz the power, 2014-04-29 23:55:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-29 23:55:51]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium

[dmazzoni, 2014-04-30 05:44:49]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2014-04-30 05:45:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/246773008/100001

[commit-bot: I haz the power, 2014-04-30 05:47:36]

Change committed as 267097