Enable external popup menu in Blimp, part 2/2.

Enables Blimp engine to communicate with content/ for handling external popup menu.

Exposes Show/HidePopupMenu to outside content/ via WebContentsViewDelegate, and DidSelectPopupMenuItems& DidCancelPopupMenu via render_frame_host.h.

Drawback: Android and Mac do not need these APIs. So exposing them under USE_EXTERNAL_POPUP_MENU is confusing. However, since we do not want to introduce IS_BLIMP flag to content/, it is unclear how to expose external popup menu API in a cleaner way.



BUG=598764
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

[haibinlu, 2016-05-31 22:56:24]

Description was changed from

==========
[Do not submit]  Enable external popup menu in Blimp, part 2/2.

BUG=598764
==========

to

==========
[Do not submit]  Enable external popup menu in Blimp, part 2/2.

BUG=598764
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[haibinlu, 2016-06-03 17:30:00]

Patchset #1 (id:1) has been deleted

[haibinlu, 2016-06-03 17:33:06]

Description was changed from

==========
[Do not submit]  Enable external popup menu in Blimp, part 2/2.

BUG=598764
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
BUG=598764
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[haibinlu, 2016-06-03 17:36:46]

Patchset #1 (id:20001) has been deleted

[haibinlu, 2016-06-03 20:33:10]

Patchset #1 (id:40001) has been deleted

[haibinlu, 2016-06-03 20:44:07]

Description was changed from

==========
BUG=598764
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Enables Blimp engine to communicate with content/ for handling external popup
menu.

Exposes Show/HidePopupMenu to outside content/ via WebContentsViewDelegate, and
DidSelectPopupMenuItems& DidCancelPopupMenu via render_frame_host.h.

Drawbacks: Android and Mac do not need these APIs. So exposing them under
USE_EXTERNAL_POPUP_MENU is confusing. However, since we do not want to introduce
IS_BLIMP flag to content/, it is unclear how to expose external popup menu API
in a cleaner way.



BUG=598764
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[haibinlu, 2016-06-03 20:44:20]

Description was changed from

==========
Enables Blimp engine to communicate with content/ for handling external popup
menu.

Exposes Show/HidePopupMenu to outside content/ via WebContentsViewDelegate, and
DidSelectPopupMenuItems& DidCancelPopupMenu via render_frame_host.h.

Drawbacks: Android and Mac do not need these APIs. So exposing them under
USE_EXTERNAL_POPUP_MENU is confusing. However, since we do not want to introduce
IS_BLIMP flag to content/, it is unclear how to expose external popup menu API
in a cleaner way.



BUG=598764
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Enables Blimp engine to communicate with content/ for handling external popup
menu.

Exposes Show/HidePopupMenu to outside content/ via WebContentsViewDelegate, and
DidSelectPopupMenuItems& DidCancelPopupMenu via render_frame_host.h.

Drawback: Android and Mac do not need these APIs. So exposing them under
USE_EXTERNAL_POPUP_MENU is confusing. However, since we do not want to introduce
IS_BLIMP flag to content/, it is unclear how to expose external popup menu API
in a cleaner way.



BUG=598764
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[haibinlu, 2016-06-03 20:46:18]

haibinlu@chromium.org changed reviewers:
+ avi@chromium.org, nick@chromium.org, wez@chromium.org

[haibinlu, 2016-06-03 20:49:37]

There will be another CL to add the actual communication between blimp engine
and its Android client, and the client-side external poppup menu handling. That
will be only in blimp/.

[Avi (use Gerrit), 2016-06-03 20:57:48]

This is a really weird proposal. Popup menus on the Mac and Android run
themselves, but for Aura they call out through this WCVDelegate.

I don't like the asymmetry. If you want to allow the embedder to override the
handling, sure, that's done all the time. But to say, hey, for this one platform
do it completely different than the others ... :(

https://codereview.chromium.org/2026813003/diff/60001/content/public/browser/...
File content/public/browser/web_contents_view_delegate.h (right):

https://codereview.chromium.org/2026813003/diff/60001/content/public/browser/...
content/public/browser/web_contents_view_delegate.h:65: #if
defined(USE_EXTERNAL_POPUP_MENU)
&& USE(AURA)

or whatever the ifdef there is.

[haibinlu, 2016-06-03 21:35:50]

On 2016/06/03 20:57:48, Avi wrote:
> This is a really weird proposal. Popup menus on the Mac and Android run
> themselves, but for Aura they call out through this WCVDelegate.
> 
> I don't like the asymmetry. If you want to allow the embedder to override the
> handling, sure, that's done all the time. But to say, hey, for this one
platform
> do it completely different than the others ... :(
> 
>
https://codereview.chromium.org/2026813003/diff/60001/content/public/browser/...
> File content/public/browser/web_contents_view_delegate.h (right):
> 
>
https://codereview.chromium.org/2026813003/diff/60001/content/public/browser/...
> content/public/browser/web_contents_view_delegate.h:65: #if
> defined(USE_EXTERNAL_POPUP_MENU)
> && USE(AURA)
> 
> or whatever the ifdef there is.

Agree, this is not a clean design. I am open to suggestions. 

It seems it is inevitable to have some aura specific API (when external popup
menu is enabled) surface via content/public, and thus result in asymmetry
handling.

[haibinlu, 2016-06-03 21:36:09]

https://codereview.chromium.org/2026813003/diff/60001/content/public/browser/...
File content/public/browser/web_contents_view_delegate.h (right):

https://codereview.chromium.org/2026813003/diff/60001/content/public/browser/...
content/public/browser/web_contents_view_delegate.h:65: #if
defined(USE_EXTERNAL_POPUP_MENU)
On 2016/06/03 20:57:48, Avi wrote:
> && USE(AURA)
> 
> or whatever the ifdef there is.

Done.

[Avi (use Gerrit), 2016-06-06 16:46:47]

On 2016/06/03 21:35:50, haibinlu wrote:
> Agree, this is not a clean design. I am open to suggestions. 
> 
> It seems it is inevitable to have some aura specific API (when external popup
> menu is enabled) surface via content/public, and thus result in asymmetry
> handling.

If the asymmetry is "Android and Mac handle this internally and Aura doesn't"
then I can see that. But wiring the flow completely differently is something
that I don't want.

Can we allow the embedder to override the handling?

[haibinlu, 2016-06-06 17:54:28]

jam@, wondering if you could provide suggestion how to expose
show/hide-external-popup-menu APIs to embedders. I agree with avi@'s assessment,
but I need help from content owners. Thanks!

[Khushal, 2016-06-06 20:39:38]

On 2016/06/06 17:54:28, haibinlu wrote:
> jam@, wondering if you could provide suggestion how to expose
> show/hide-external-popup-menu APIs to embedders. I agree with avi@'s
assessment,
> but I need help from content owners. Thanks!

At a high level we are remoting out the RWHV, and since our client is android,
the RWHVAndroid. We have been doing this by using the RWHVAura and being an aura
embedder to RPC the relevant calls for input events and text input using aura
abstractions that content/ exposes to the embedder. The popup menu handling is
one of the things that is done differently for aura and android right now.
Perhaps we just need a structured API from aura to allow an embedder to
similarly override the behaviour?

+sievers, dtrainor for suggestions.

[haibinlu, 2016-06-07 00:01:54]

On 2016/06/06 20:39:38, Khushal wrote:
> On 2016/06/06 17:54:28, haibinlu wrote:
> > jam@, wondering if you could provide suggestion how to expose
> > show/hide-external-popup-menu APIs to embedders. I agree with avi@'s
> assessment,
> > but I need help from content owners. Thanks!
> 
> At a high level we are remoting out the RWHV, and since our client is android,
> the RWHVAndroid. We have been doing this by using the RWHVAura and being an
aura
> embedder to RPC the relevant calls for input events and text input using aura
> abstractions that content/ exposes to the embedder. The popup menu handling is
> one of the things that is done differently for aura and android right now.
> Perhaps we just need a structured API from aura to allow an embedder to
> similarly override the behaviour?
> 
> +sievers, dtrainor for suggestions.

Hi, avi, 

What do you think about the approach below?  

WebContentsViewAura --> RenderWidgetHostViewAura --> aura's new
ExternalPopupMenuClient.

This latter part is similar to how aura handles tooltip. 

void RenderWidgetHostViewAura::SetTooltipText(
    const base::string16& tooltip_text) {
  tooltip_ = tooltip_text;
  aura::Window* root_window = window_->GetRootWindow();
  aura::client::TooltipClient* tooltip_client =
      aura::client::GetTooltipClient(root_window);
  if (tooltip_client) {
    tooltip_client->UpdateTooltip(window_);
    // Content tooltips should be visible indefinitely.
    tooltip_client->SetTooltipShownTimeout(window_, 0);
  }
}

ui/wm/public/tooltip_client.cc

aura::client.

  TooltipClient* GetTooltipClient(Window* root_window)
  void SetTooltipText(Window* window, base::string16* tooltip_text)

[Avi (use Gerrit), 2016-06-07 01:44:36]

On 2016/06/07 00:01:54, haibinlu wrote:
> WebContentsViewAura --> RenderWidgetHostViewAura --> aura's new
> ExternalPopupMenuClient.
> 
> This latter part is similar to how aura handles tooltip. 

That's much nicer, as we wouldn't be exposing a callback for platforms in which
it didn't apply.

I'd approve that.

[Avi (use Gerrit), 2016-06-07 01:45:12]

On 2016/06/07 01:44:36, Avi (OOO 12 June - 25 June) wrote:
> On 2016/06/07 00:01:54, haibinlu wrote:
> > WebContentsViewAura --> RenderWidgetHostViewAura --> aura's new
> > ExternalPopupMenuClient.
> > 
> > This latter part is similar to how aura handles tooltip. 
> 
> That's much nicer, as we wouldn't be exposing a callback for platforms in
which
> it didn't apply.
> 
> I'd approve that.

Double-check with an Aura expert too, BTW. Let's make sure they're happy with
that also.

[ncarter (slow), 2016-06-08 16:36:31]

nick@chromium.org changed reviewers:
- nick@chromium.org

[ncarter (slow), 2016-06-08 16:36:32]

[reviewers -= me, since it seems like avi's got this]