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

Blimp Engine is based on Linux with USE_AURA, and its client is an Android app. It is desirable for end users to see the same Chrome Android external popup menu handling.

Thus, add USE_EXTERNAL_POPUP_MENU flag so that Blimp Engine can follow the same code path of the external Android popup menus before it reaches WebContentsView.

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

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

[haibinlu, 2016-05-19 23:56:30]

Description was changed from

==========
[Do not submit] external popup menu code.

BUG=
==========

to

==========
[Do not submit] external popup menu code.

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

[haibinlu, 2016-05-20 00:37:25]

Patchset #1 (id:1) has been deleted

[haibinlu, 2016-05-20 00:37:37]

Patchset #1 (id:20001) has been deleted

[haibinlu, 2016-05-26 20:20:20]

Patchset #1 (id:40001) has been deleted

[haibinlu, 2016-05-26 20:20:28]

Patchset #1 (id:60001) has been deleted

[haibinlu, 2016-05-26 20:29:41]

Patchset #1 (id:80001) has been deleted

[haibinlu, 2016-05-26 20:41:56]

Description was changed from

==========
[Do not submit] external popup menu code.

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

to

==========
Blimp Engine is based on Linux with USE_AURA, and its client is an Android app.
It is desirable for end users to see the same Chrome Android external popup menu
handling.

Thus, add USE_EXTERNAL_POPUP_MENU flag so that Blimp Engine can follow the same
code path of the external Android popup menus before it reaches WebContentsView.
 


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

[haibinlu, 2016-05-26 21:32:34]

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

[haibinlu, 2016-05-26 21:32:34]

avi@ and nasko@,  

Could I have your opinions on the approach I am about to adopt? Thanks!

This is part 1. The next part is to wire up external popup menu handling to
Blimp WebContents manager, and then communicate with Blimp client from there.

[haibinlu, 2016-05-26 21:35:22]

haibinlu@chromium.org changed required reviewers:
+ wez@chromium.org

[Avi (use Gerrit), 2016-05-27 02:00:00]

I think this is reasonable.

https://codereview.chromium.org/1992393002/diff/100001/build/config/BUILD.gn
File build/config/BUILD.gn (right):

https://codereview.chromium.org/1992393002/diff/100001/build/config/BUILD.gn#...
build/config/BUILD.gn:172: defines += [ "USE_EXTERNAL_POPUP_MENU=1" ]
The Mac hasn't fully switched to GN so you'll need to change the .gyp files too.

https://codereview.chromium.org/1992393002/diff/100001/content/browser/web_co...
File content/browser/web_contents/web_contents_view_aura.cc (right):

https://codereview.chromium.org/1992393002/diff/100001/content/browser/web_co...
content/browser/web_contents/web_contents_view_aura.cc:1146: LOG(ERROR) <<
"ShowPopupMenu " << items.size() << " items";
NOTIMPLEMENTED() << ...

[Wez, 2016-05-27 02:24:51]

Looks pretty reasonable to me; I'd tweak some of the #ifs w/ OS_MACOSX checks to
be more like:

#if EXTERNAL_POPUP
#if OS_MAC
#else
#endif
#endif

for clarity.

https://codereview.chromium.org/1992393002/diff/100001/content/renderer/rende...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/1992393002/diff/100001/content/renderer/rende...
content/renderer/render_frame_impl.h:824: #if defined(USE_EXTERNAL_POPUP_MENU)
&& !defined(OS_MACOSX)
nit: This might read more clearly as a nested #if, since USE_EXTERNAL_POPUP_MENU
is used on Mac, right?

[haibinlu, 2016-05-28 01:37:03]

Description was changed from

==========
Blimp Engine is based on Linux with USE_AURA, and its client is an Android app.
It is desirable for end users to see the same Chrome Android external popup menu
handling.

Thus, add USE_EXTERNAL_POPUP_MENU flag so that Blimp Engine can follow the same
code path of the external Android popup menus before it reaches WebContentsView.
 


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

to

==========
Blimp Engine is based on Linux with USE_AURA, and its client is an Android app.
It is desirable for end users to see the same Chrome Android external popup menu
handling.

Thus, add USE_EXTERNAL_POPUP_MENU flag so that Blimp Engine can follow the same
code path of the external Android popup menus before it reaches WebContentsView.
 


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

[haibinlu, 2016-05-28 01:37:03]

haibinlu@chromium.org changed reviewers:
+ dpranke@chromium.org

[haibinlu, 2016-05-28 01:37:59]

Ready for review. Thanks!

https://codereview.chromium.org/1992393002/diff/100001/build/config/BUILD.gn
File build/config/BUILD.gn (right):

https://codereview.chromium.org/1992393002/diff/100001/build/config/BUILD.gn#...
build/config/BUILD.gn:172: defines += [ "USE_EXTERNAL_POPUP_MENU=1" ]
On 2016/05/27 02:00:00, Avi wrote:
> The Mac hasn't fully switched to GN so you'll need to change the .gyp files
too.

Done.

https://codereview.chromium.org/1992393002/diff/100001/content/browser/web_co...
File content/browser/web_contents/web_contents_view_aura.cc (right):

https://codereview.chromium.org/1992393002/diff/100001/content/browser/web_co...
content/browser/web_contents/web_contents_view_aura.cc:1146: LOG(ERROR) <<
"ShowPopupMenu " << items.size() << " items";
On 2016/05/27 02:00:00, Avi wrote:
> NOTIMPLEMENTED() << ...

Done.

https://codereview.chromium.org/1992393002/diff/100001/content/renderer/rende...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/1992393002/diff/100001/content/renderer/rende...
content/renderer/render_frame_impl.h:824: #if defined(USE_EXTERNAL_POPUP_MENU)
&& !defined(OS_MACOSX)
On 2016/05/27 02:24:51, Wez wrote:
> nit: This might read more clearly as a nested #if, since
USE_EXTERNAL_POPUP_MENU
> is used on Mac, right?

Done.

[Avi (use Gerrit), 2016-05-28 02:33:15]

If we're gonna do this, yeah, this is what we want.

LGTM

[haibinlu, 2016-06-02 18:50:21]

Friendly ping.

[Avi (use Gerrit), 2016-06-02 18:56:36]

On 2016/06/02 18:50:21, haibinlu wrote:
> Friendly ping.

Who are you pinging? You probably should name who you need approval from.

[haibinlu, 2016-06-02 19:22:25]

nasko@, could you review content/ change?  
dpranke@, could you review build/ change?
Thanks!

[nasko, 2016-06-02 20:32:24]

On 2016/06/02 19:22:25, haibinlu wrote:
> nasko@, could you review content/ change?  
> dpranke@, could you review build/ change?
> Thanks!

Avi already has covered content/ changes, so no need for review on my side.
Still rubberstamp LGTM based on Avi's review, as you are changing
frame_messages.h.

[Dirk Pranke, 2016-06-02 21:01:32]

lgtm.

Do you need to add the flag to //build/args/blimp_engine.gn as well?

[haibinlu, 2016-06-02 21:10:56]

On 2016/06/02 21:01:32, Dirk Pranke wrote:
> lgtm.
> 
> Do you need to add the flag to //build/args/blimp_engine.gn as well?

Done.

[haibinlu, 2016-06-02 21:28:09]

The CQ bit was checked by haibinlu@chromium.org

[haibinlu, 2016-06-02 21:28:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, avi@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1992393002/#ps140001
(title: "rebase and update blimp_engine.gn")

[commit-bot: I haz the power, 2016-06-02 21:29:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1992393002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1992393002/140001

[commit-bot: I haz the power, 2016-06-02 21:29:34]

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

[commit-bot: I haz the power, 2016-06-02 21:29:35]

All required reviewers (with asterisk prefixes) have not yet approved this CL.

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Avi (use Gerrit), 2016-06-02 21:33:35]

On 2016/06/02 21:29:35, commit-bot: I haz the power wrote:
> All required reviewers (with asterisk prefixes) have not yet approved this CL.
> 
> No L-G-T-M from a valid reviewer yet. 
> CQ run can only be started by full committers or once the patch has
> received an L-G-T-M from a full committer.
> Even if an L-G-T-M may have been provided, it was from a non-committer,
> _not_ a full super star committer.
> See http://www.chromium.org/getting-involved/become-a-committer
> Note that this has nothing to do with OWNERS files.

You starred Wez, making him a required reviewer, but he hasn't given his OK.

[haibinlu, 2016-06-02 21:33:35]

haibinlu@chromium.org changed required reviewers:
- wez@chromium.org

[haibinlu, 2016-06-02 21:36:47]

On 2016/06/02 21:33:35, Avi wrote:
> On 2016/06/02 21:29:35, commit-bot: I haz the power wrote:
> > All required reviewers (with asterisk prefixes) have not yet approved this
CL.
> > 
> > No L-G-T-M from a valid reviewer yet. 
> > CQ run can only be started by full committers or once the patch has
> > received an L-G-T-M from a full committer.
> > Even if an L-G-T-M may have been provided, it was from a non-committer,
> > _not_ a full super star committer.
> > See http://www.chromium.org/getting-involved/become-a-committer
> > Note that this has nothing to do with OWNERS files.
> 
> You starred Wez, making him a required reviewer, but he hasn't given his OK.

Talked with wez@. He is fine without the star :).

[haibinlu, 2016-06-02 21:36:57]

The CQ bit was checked by haibinlu@chromium.org

[commit-bot: I haz the power, 2016-06-02 21:38:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1992393002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1992393002/140001

[commit-bot: I haz the power, 2016-06-03 02:22:59]

Description was changed from

==========
Blimp Engine is based on Linux with USE_AURA, and its client is an Android app.
It is desirable for end users to see the same Chrome Android external popup menu
handling.

Thus, add USE_EXTERNAL_POPUP_MENU flag so that Blimp Engine can follow the same
code path of the external Android popup menus before it reaches WebContentsView.
 


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

to

==========
Blimp Engine is based on Linux with USE_AURA, and its client is an Android app.
It is desirable for end users to see the same Chrome Android external popup menu
handling.

Thus, add USE_EXTERNAL_POPUP_MENU flag so that Blimp Engine can follow the same
code path of the external Android popup menus before it reaches WebContentsView.
 


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

[commit-bot: I haz the power, 2016-06-03 02:23:01]

Committed patchset #3 (id:140001)

[commit-bot: I haz the power, 2016-06-03 02:24:07]

Description was changed from

==========
Blimp Engine is based on Linux with USE_AURA, and its client is an Android app.
It is desirable for end users to see the same Chrome Android external popup menu
handling.

Thus, add USE_EXTERNAL_POPUP_MENU flag so that Blimp Engine can follow the same
code path of the external Android popup menus before it reaches WebContentsView.
 


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

to

==========
Blimp Engine is based on Linux with USE_AURA, and its client is an Android app.
It is desirable for end users to see the same Chrome Android external popup menu
handling.

Thus, add USE_EXTERNAL_POPUP_MENU flag so that Blimp Engine can follow the same
code path of the external Android popup menus before it reaches WebContentsView.

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

Committed: https://crrev.com/c643d33cf668665bac4562e69a72f9ef29b5bbed
Cr-Commit-Position: refs/heads/master@{#397575}
==========

[commit-bot: I haz the power, 2016-06-03 02:24:11]

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

[Wez, 2016-06-03 17:03:07]

nit: Consider removing "do not submit" from the CL description, otherwise it's a
little alarming to see this submitted!

[Avi (use Gerrit), 2016-06-03 17:15:02]

On 2016/06/03 17:03:07, Wez wrote:
> nit: Consider removing "do not submit" from the CL description, otherwise it's
a
> little alarming to see this submitted!

Wez, he did!

But altering the description doesn't change the title of the email that is sent
for review.

[Wez, 2016-06-03 17:35:34]

On 2016/06/03 17:15:02, Avi wrote:
> On 2016/06/03 17:03:07, Wez wrote:
> > nit: Consider removing "do not submit" from the CL description, otherwise
it's
> a
> > little alarming to see this submitted!
> 
> Wez, he did!
> 
> But altering the description doesn't change the title of the email that is
sent
> for review.

*facepalm* Of course! :P