AndroidOverlay implementation using Dialog.

AndroidOverlay implementation using Dialog.

This CL adds DialogOverlayImpl, which uses AndroidDialogs to provide an
overlay.  The intended use looks something like this:

RenderFrame(Impl) requests a routing token from RenderFrameHostImpl
RFHI returns token to RF
RF sends this token to WebMediaPlayerImpl
WMPI sends this (via chrome IPC) to AVDA (gpu process)
AVDA requests an overlay from AndroidOverlayProviderImpl using this
  token (via mojo to the browser).
AndroidOverlayProviderImpl creates a DialogOverlayImpl in the browser
AOPI returns the DialogOverlayImpl to AVDA.

BUG=618368
TEST=DialogOverlayImplTest, DialogOverlayCoreTest
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2765443004
Cr-Commit-Position: refs/heads/master@{#468760}
Committed: https://chromium.googlesource.com/chromium/src/+/a8da3b572593e1a166bc2e3a22821a17feb0af27

[liberato (no reviews please), 2017-03-20 17:03:08]

Description was changed from

==========
AndroidOverlay implementation using Dialog.

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

to

==========
AndroidOverlay implementation using Dialog.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[liberato (no reviews please), 2017-03-20 17:03:09]

Description was changed from

==========
AndroidOverlay implementation using Dialog.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
AndroidOverlay implementation using Dialog.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[liberato (no reviews please), 2017-03-22 22:28:39]

The CQ bit was checked by liberato@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 22:29:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-22 22:33:42]

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

[commit-bot: I haz the power, 2017-03-22 22:33:43]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[liberato (no reviews please), 2017-03-29 22:00:31]

The CQ bit was checked by liberato@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 22:01:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[liberato (no reviews please), 2017-03-29 22:05:26]

The CQ bit was checked by liberato@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 22:06:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[liberato (no reviews please), 2017-03-29 22:10:43]

Description was changed from

==========
AndroidOverlay implementation using Dialog.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
AndroidOverlay implementation using Dialog.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[liberato (no reviews please), 2017-03-29 22:10:43]

liberato@chromium.org changed reviewers:
+ aelias@chromium.org, boliu@chromium.org, dcheng@chromium.org

[commit-bot: I haz the power, 2017-03-29 22:11:20]

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

[commit-bot: I haz the power, 2017-03-29 22:11:21]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[liberato (no reviews please), 2017-03-29 22:15:44]

Description was changed from

==========
AndroidOverlay implementation using Dialog.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
AndroidOverlay implementation using Dialog.

This CL adds DialogOverlayImpl, which uses AndroidDialogs to provide an
overlay.  The intended use looks something like this:

RenderFrame(Impl) requests a routing token from RenderFrameHostImpl
RFHI returns token to RF
RF sends this token to WebMediaPlayerImpl
WMPI sends this (via chrome IPC) to AVDA (gpu process)
AVDA requests an overlay from AndroidOverlayProviderImpl using this
  token (via mojo to the browser).
AndroidOverlayProviderImpl creates a DialogOverlayImpl in the browser
AOPI returns the DialogOverlayImpl to AVDA.

BUG=618368
TEST=DialogOverlayImplTest, DialogOverlayCoreTest
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[liberato (no reviews please), 2017-03-29 22:28:45]

hi all

Here's the implementation for android overlays via Dialog that goes with the
mojo framework that i landed a few weeks.  it doesn't try hook it up to video
playback yet.

i added test coverage to get all the major bits.  i'd like to add screenshot
based tests, but that's a fairly big separate chunk of work.  i've also tested
manually, and i've seen actual green rectangles showing up on separate layers
when i change the z-order of the dialog to overlay the compositor view.  :)

the intended path for use by video playback will be this: RenderFrameImpl will
provide WebMediaPlayerImpl with an overlay routing token.  that will be sent
over IPC to AndroidVideoDecodeAccelerator in the gpu process.  AVDA will then
ask the browser to create the overlay, via mojo.  none of that happens in this
CL, except for sending the token from RFHI => RFI.

all feedback is appreciated.

the CL is covered by these owners:

boliu: content/ (except content/renderer)
aelias: content/
dcheng: gpu/ and media/mojo/interfaces
(tbd): media/
(tbd): render_frame_host.h

thanks
-fl

[liberato (no reviews please), 2017-03-30 05:10:40]

The CQ bit was checked by liberato@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 05:10:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 05:14:59]

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

[commit-bot: I haz the power, 2017-03-30 05:15:00]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[liberato (no reviews please), 2017-03-30 06:52:11]

The CQ bit was checked by liberato@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 06:52:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 07:41:10]

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

[commit-bot: I haz the power, 2017-03-30 07:41:11]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[liberato (no reviews please), 2017-03-30 18:48:17]

The CQ bit was checked by liberato@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 18:48:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 20:57:56]

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

[commit-bot: I haz the power, 2017-03-30 20:57:57]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[liberato (no reviews please), 2017-03-30 21:14:31]

The CQ bit was checked by liberato@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 21:15:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[boliu, 2017-03-30 22:42:03]

mostly ok, skipped tests and renderer so I can send this out today..

https://codereview.chromium.org/2765443004/diff/160001/content/browser/androi...
File content/browser/android/dialog_overlay_operations_impl.cc (right):

https://codereview.chromium.org/2765443004/diff/160001/content/browser/androi...
content/browser/android/dialog_overlay_operations_impl.cc:43: cvc_ =
GetContentViewCore();
DCHECK everything here is on UI thread

https://codereview.chromium.org/2765443004/diff/160001/content/browser/androi...
content/browser/android/dialog_overlay_operations_impl.cc:76: // |this| might be
destroyed.
what if you use DeleteSoon in UnregisterForTokens to avoid re-entrancy issues
like this?

https://codereview.chromium.org/2765443004/diff/160001/content/browser/androi...
content/browser/android/dialog_overlay_operations_impl.cc:106: if
(!web_contents) {
there's a chromium-dev thread by dcheng that this can't be null :p

https://codereview.chromium.org/2765443004/diff/160001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayOperationsImpl.java
(right):

https://codereview.chromium.org/2765443004/diff/160001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayOperationsImpl.java:18:
* Basic impl of DialogOverlayOperations
nit: java doc require a period at the end

https://codereview.chromium.org/2765443004/diff/160001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayOperationsImpl.java:43:
public Dialog createDialog(Context context) {
static, but I guess no longer overridable..

was this supposed to have a test implementation or something?

https://codereview.chromium.org/2765443004/diff/160001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayOperationsImpl.java:62:
mNativeHandle = nativeRegisterForTokens(token.high, token.low);
assert mNativeHandle == 0?

https://codereview.chromium.org/2765443004/diff/160001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayOperationsImpl.java:68:
mNativeHandle = 0;
probably should check or assert for non-0 before use, which.. is only in this
method

https://codereview.chromium.org/2765443004/diff/160001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayOperationsImpl.java:83:
public void onWindowToken(final IBinder token) {
@CalledByNative should be private

https://codereview.chromium.org/2765443004/diff/160001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayOperationsImpl.java:95:
native long nativeRegisterForTokens(long high, long low);
all native methods private, and looks like should all be static as well

https://codereview.chromium.org/2765443004/diff/160001/content/public/android...
File
content/public/android/java/src/org/chromium/content_public/browser/RenderFrameHost.java
(right):

https://codereview.chromium.org/2765443004/diff/160001/content/public/android...
content/public/android/java/src/org/chromium/content_public/browser/RenderFrameHost.java:24:
UnguessableToken getAndroidOverlayRoutingToken();
doesn't need to be in content_public if nothing outside of content needs this

https://codereview.chromium.org/2765443004/diff/160001/content/public/browser...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/2765443004/diff/160001/content/public/browser...
content/public/browser/render_frame_host.h:84: virtual const
base::UnguessableToken& GetOverlayRoutingToken() = 0;
ditto about public

https://codereview.chromium.org/2765443004/diff/160001/gpu/ipc/common/gpu_sur...
File gpu/ipc/common/gpu_surface_tracker.h (right):

https://codereview.chromium.org/2765443004/diff/160001/gpu/ipc/common/gpu_sur...
gpu/ipc/common/gpu_surface_tracker.h:46: int RegisterViewSurface(jobject
j_surface);
err, I asked this to be refactored when this code moved here, but I guess that
never happened.

So before this CL, surface_map_ and surface_view_map_ are parallel arrays, so we
should code it as a single map

and then I think this CL is allowing a surface view to be registered without a
AcceleratedWidget. imo that's easier to understand?

https://codereview.chromium.org/2765443004/diff/160001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right):

https://codereview.chromium.org/2765443004/diff/160001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:735: if (context ==
null) return null;
this check not needed, activityFromContext should return null if you pass it
null

https://codereview.chromium.org/2765443004/diff/160001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:738: View decorView
= activity.getWindow().peekDecorView();
actually need null check for getWindow? docs says
"Window The current window, or null if the activity is not visual."

https://codereview.chromium.org/2765443004/diff/180001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2765443004/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:169:
base::LazyInstance<TokenFrameMap>::DestructorAtExit g_token_frame_map =
Leaky is fine for android, android doesn't have clean shutdowns

https://codereview.chromium.org/2765443004/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:372: return
RenderFrameHostImpl::FromOverlayRoutingToken(token);
DCHECK UI thread as well

https://codereview.chromium.org/2765443004/diff/180001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/androidoverlay/AndroidOverlayProviderImpl.java
(right):

https://codereview.chromium.org/2765443004/diff/180001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/AndroidOverlayProviderImpl.java:29:
// surfaceDestroyed.  Instead, we run all AndroidOverlays on one shared
overlay-ui thread.
maybe just call this the "overlay" thread everywhere, overlay-ui is kinda
confusing with ui thread

https://codereview.chromium.org/2765443004/diff/180001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/AndroidOverlayProviderImpl.java:45:
private static final int MAX_OVERLAYS = 1;
nit: constants at the top

https://codereview.chromium.org/2765443004/diff/180001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/AndroidOverlayProviderImpl.java:51:
public void createOverlay(InterfaceRequest<AndroidOverlay> request,
AndroidOverlayClient client,
is this @Override?

https://codereview.chromium.org/2765443004/diff/180001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/AndroidOverlayProviderImpl.java:83:
public void notifyReleased() {
only called by runnable, right? so can be private

https://codereview.chromium.org/2765443004/diff/180001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayCore.java
(right):

https://codereview.chromium.org/2765443004/diff/180001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayCore.java:41:
Host mHost;
private?

https://codereview.chromium.org/2765443004/diff/180001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayImpl.java
(right):

https://codereview.chromium.org/2765443004/diff/180001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayImpl.java:28:
private AndroidOverlayClient mClient;
any/all of this can be final?

https://codereview.chromium.org/2765443004/diff/180001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayImpl.java:48:
mClient = client;
probably worth asserting what thread each method runs on, more for documentation
than anything else

https://codereview.chromium.org/2765443004/diff/180001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayImpl.java:202:
if (mDialogCore != null) mDialogCore = null;
redundant check

https://codereview.chromium.org/2765443004/diff/200001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayCore.java
(right):

https://codereview.chromium.org/2765443004/diff/200001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayCore.java:204:
(Integer) layoutParams.getClass().getField("privateFlags").get(layoutParams);
arrr, same comment as last time about reflection..

[commit-bot: I haz the power, 2017-03-30 23:19:06]

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

[commit-bot: I haz the power, 2017-03-30 23:19:07]

Dry run: This issue passed the CQ dry run.

[liberato (no reviews please), 2017-03-31 21:28:31]

The CQ bit was checked by liberato@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 21:29:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-31 23:47:48]

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

[commit-bot: I haz the power, 2017-03-31 23:47:51]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-04-03 19:12:39]

dcheng@chromium.org changed reviewers:
+ tguilbert@chromium.org

[dcheng, 2017-04-03 19:12:40]

https://codereview.chromium.org/2765443004/diff/220001/content/common/frame_m...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/2765443004/diff/220001/content/common/frame_m...
content/common/frame_messages.h:895: uint64_t /* high */,
Why not just send the base::UnguessableToken directly?

https://codereview.chromium.org/2765443004/diff/220001/content/public/android...
File
content/public/android/javatests/src/org/chromium/content/browser/androidoverlay/DialogOverlayImplTest.java
(right):

https://codereview.chromium.org/2765443004/diff/220001/content/public/android...
content/public/android/javatests/src/org/chromium/content/browser/androidoverlay/DialogOverlayImplTest.java:145:
mojoToken.high = routingToken.getHighForSerialization();
+tguilbert... I don't know much about the Java glue, but maybe there's a way to
avoid having to make this public.

https://codereview.chromium.org/2765443004/diff/220001/media/mojo/interfaces/...
File media/mojo/interfaces/android_overlay.mojom (right):

https://codereview.chromium.org/2765443004/diff/220001/media/mojo/interfaces/...
media/mojo/interfaces/android_overlay.mojom:60: bool secure;
I'm not sure what |secure| means here.

[liberato (no reviews please), 2017-04-04 17:49:29]

thanks for the feedback.

still outstanding:

- gpu_surface_tracker refactor is in a separate CL
(https://codereview.chromium.org/2791723003/)

- any changes to UnguessableToken based on tguilbert@'s feedback.

- separtely, the routing token is moving out of AndroidOverlay::Config (stays in
mojo config), since it turns out to be more convenient in application code
(https://codereview.chromium.org/2796943002/) to bake it into the mojo factory
instance.  doesn't really impact this CL much, but included for completeness.

also, tguilbert@, would you mind doing an owners review of media/ ?

thanks
-fl

https://codereview.chromium.org/2765443004/diff/160001/content/browser/androi...
File content/browser/android/dialog_overlay_operations_impl.cc (right):

https://codereview.chromium.org/2765443004/diff/160001/content/browser/androi...
content/browser/android/dialog_overlay_operations_impl.cc:43: cvc_ =
GetContentViewCore();
On 2017/03/30 22:42:02, boliu wrote:
> DCHECK everything here is on UI thread

Done.

https://codereview.chromium.org/2765443004/diff/160001/content/browser/androi...
content/browser/android/dialog_overlay_operations_impl.cc:76: // |this| might be
destroyed.
On 2017/03/30 22:42:02, boliu wrote:
> what if you use DeleteSoon in UnregisterForTokens to avoid re-entrancy issues
> like this?

good point.  i'll explicitly remove the CVC observer to prevent additional token
callbacks while the delete is pending.  won't make a difference if this is in
response to onDismissed, but the java side can unregister for other reasons.

https://codereview.chromium.org/2765443004/diff/160001/content/browser/androi...
content/browser/android/dialog_overlay_operations_impl.cc:106: if
(!web_contents) {
On 2017/03/30 22:42:02, boliu wrote:
> there's a chromium-dev thread by dcheng that this can't be null :p

DCHECK it is then!

https://codereview.chromium.org/2765443004/diff/160001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayOperationsImpl.java
(right):

https://codereview.chromium.org/2765443004/diff/160001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayOperationsImpl.java:18:
* Basic impl of DialogOverlayOperations
On 2017/03/30 22:42:02, boliu wrote:
> nit: java doc require a period at the end

Done.

https://codereview.chromium.org/2765443004/diff/160001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayOperationsImpl.java:43:
public Dialog createDialog(Context context) {
On 2017/03/30 22:42:02, boliu wrote:
> static, but I guess no longer overridable..
> 
> was this supposed to have a test implementation or something?

yeah, initially it was for DialogOverlayCoreTest.  we now send in the dialog in
DialogOverlayCore::initialize, i think.

i will move this out of Ops, into DialogOverlayImpl.

https://codereview.chromium.org/2765443004/diff/160001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayOperationsImpl.java:62:
mNativeHandle = nativeRegisterForTokens(token.high, token.low);
On 2017/03/30 22:42:02, boliu wrote:
> assert mNativeHandle == 0?

Done.

https://codereview.chromium.org/2765443004/diff/160001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayOperationsImpl.java:68:
mNativeHandle = 0;
On 2017/03/30 22:42:02, boliu wrote:
> probably should check or assert for non-0 before use, which.. is only in this
> method

switched to do-nothing if |!mNativeHandle|.  simplifies DialogOverlayImpl, which
kept a boolean to remember if it needed to do so.

https://codereview.chromium.org/2765443004/diff/160001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayOperationsImpl.java:83:
public void onWindowToken(final IBinder token) {
On 2017/03/30 22:42:02, boliu wrote:
> @CalledByNative should be private

Done, and elsewhere.

https://codereview.chromium.org/2765443004/diff/160001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayOperationsImpl.java:95:
native long nativeRegisterForTokens(long high, long low);
On 2017/03/30 22:42:02, boliu wrote:
> all native methods private, and looks like should all be static as well

Done, except for nativeRegisterForTokens which requires |this| to send
callbacks.

it reminds me that declaring something "native static" doesn't work, but "static
native" does.  i keep forgetting to look through the jni generator to make it
either fail outright or work.  right now, it just ignores the methods.

https://codereview.chromium.org/2765443004/diff/160001/content/public/android...
File
content/public/android/java/src/org/chromium/content_public/browser/RenderFrameHost.java
(right):

https://codereview.chromium.org/2765443004/diff/160001/content/public/android...
content/public/android/java/src/org/chromium/content_public/browser/RenderFrameHost.java:24:
UnguessableToken getAndroidOverlayRoutingToken();
On 2017/03/30 22:42:02, boliu wrote:
> doesn't need to be in content_public if nothing outside of content needs this

good point, done.

https://codereview.chromium.org/2765443004/diff/160001/content/public/browser...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/2765443004/diff/160001/content/public/browser...
content/public/browser/render_frame_host.h:84: virtual const
base::UnguessableToken& GetOverlayRoutingToken() = 0;
On 2017/03/30 22:42:02, boliu wrote:
> ditto about public

Done.

https://codereview.chromium.org/2765443004/diff/160001/gpu/ipc/common/gpu_sur...
File gpu/ipc/common/gpu_surface_tracker.h (right):

https://codereview.chromium.org/2765443004/diff/160001/gpu/ipc/common/gpu_sur...
gpu/ipc/common/gpu_surface_tracker.h:46: int RegisterViewSurface(jobject
j_surface);
On 2017/03/30 22:42:02, boliu wrote:
> err, I asked this to be refactored when this code moved here, but I guess that
> never happened.
> 
> So before this CL, surface_map_ and surface_view_map_ are parallel arrays, so
we
> should code it as a single map
> 
> and then I think this CL is allowing a surface view to be registered without a
> AcceleratedWidget. imo that's easier to understand?

sorry about that.  will refactor per our offline discussion.  the CL is
https://codereview.chromium.org/2791723003/

https://codereview.chromium.org/2765443004/diff/160001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right):

https://codereview.chromium.org/2765443004/diff/160001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:735: if (context ==
null) return null;
On 2017/03/30 22:42:02, boliu wrote:
> this check not needed, activityFromContext should return null if you pass it
> null

Done.

https://codereview.chromium.org/2765443004/diff/160001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:738: View decorView
= activity.getWindow().peekDecorView();
On 2017/03/30 22:42:02, boliu wrote:
> actually need null check for getWindow? docs says
> "Window The current window, or null if the activity is not visual."

unclear.  i'll add it.

https://codereview.chromium.org/2765443004/diff/180001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2765443004/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:169:
base::LazyInstance<TokenFrameMap>::DestructorAtExit g_token_frame_map =
On 2017/03/30 22:42:02, boliu wrote:
> Leaky is fine for android, android doesn't have clean shutdowns

Done.

https://codereview.chromium.org/2765443004/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:372: return
RenderFrameHostImpl::FromOverlayRoutingToken(token);
On 2017/03/30 22:42:02, boliu wrote:
> DCHECK UI thread as well

Done.

https://codereview.chromium.org/2765443004/diff/180001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/androidoverlay/AndroidOverlayProviderImpl.java
(right):

https://codereview.chromium.org/2765443004/diff/180001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/AndroidOverlayProviderImpl.java:29:
// surfaceDestroyed.  Instead, we run all AndroidOverlays on one shared
overlay-ui thread.
On 2017/03/30 22:42:02, boliu wrote:
> maybe just call this the "overlay" thread everywhere, overlay-ui is kinda
> confusing with ui thread

Done.

https://codereview.chromium.org/2765443004/diff/180001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/AndroidOverlayProviderImpl.java:45:
private static final int MAX_OVERLAYS = 1;
On 2017/03/30 22:42:02, boliu wrote:
> nit: constants at the top

Done.

https://codereview.chromium.org/2765443004/diff/180001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/AndroidOverlayProviderImpl.java:51:
public void createOverlay(InterfaceRequest<AndroidOverlay> request,
AndroidOverlayClient client,
On 2017/03/30 22:42:02, boliu wrote:
> is this @Override?

Done.

https://codereview.chromium.org/2765443004/diff/180001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/AndroidOverlayProviderImpl.java:83:
public void notifyReleased() {
On 2017/03/30 22:42:02, boliu wrote:
> only called by runnable, right? so can be private

Done.

https://codereview.chromium.org/2765443004/diff/180001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayCore.java
(right):

https://codereview.chromium.org/2765443004/diff/180001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayCore.java:41:
Host mHost;
On 2017/03/30 22:42:02, boliu wrote:
> private?

Done.

https://codereview.chromium.org/2765443004/diff/180001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayImpl.java
(right):

https://codereview.chromium.org/2765443004/diff/180001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayImpl.java:28:
private AndroidOverlayClient mClient;
On 2017/03/30 22:42:02, boliu wrote:
> any/all of this can be final?

client, dialogcore: no, cleared when they're cleaned up.
ops, host: done.

https://codereview.chromium.org/2765443004/diff/180001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayImpl.java:48:
mClient = client;
On 2017/03/30 22:42:03, boliu wrote:
> probably worth asserting what thread each method runs on, more for
documentation
> than anything else

Done.

https://codereview.chromium.org/2765443004/diff/180001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayImpl.java:202:
if (mDialogCore != null) mDialogCore = null;
On 2017/03/30 22:42:03, boliu wrote:
> redundant check

Done.

https://codereview.chromium.org/2765443004/diff/200001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayCore.java
(right):

https://codereview.chromium.org/2765443004/diff/200001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayCore.java:204:
(Integer) layoutParams.getClass().getField("privateFlags").get(layoutParams);
On 2017/03/30 22:42:03, boliu wrote:
> arrr, same comment as last time about reflection..

my apologies, i meant to have an offline conversation with you about this but i
forgot.

https://codereview.chromium.org/2765443004/diff/220001/content/common/frame_m...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/2765443004/diff/220001/content/common/frame_m...
content/common/frame_messages.h:895: uint64_t /* high */,
On 2017/04/03 19:12:40, dcheng wrote:
> Why not just send the base::UnguessableToken directly?

because i forgot that it had paramtraits already.  thanks!

https://codereview.chromium.org/2765443004/diff/220001/content/public/android...
File
content/public/android/javatests/src/org/chromium/content/browser/androidoverlay/DialogOverlayImplTest.java
(right):

https://codereview.chromium.org/2765443004/diff/220001/content/public/android...
content/public/android/javatests/src/org/chromium/content/browser/androidoverlay/DialogOverlayImplTest.java:145:
mojoToken.high = routingToken.getHighForSerialization();
On 2017/04/03 19:12:40, dcheng wrote:
> +tguilbert... I don't know much about the Java glue, but maybe there's a way
to
> avoid having to make this public.

Acknowledged.

https://codereview.chromium.org/2765443004/diff/220001/media/mojo/interfaces/...
File media/mojo/interfaces/android_overlay.mojom (right):

https://codereview.chromium.org/2765443004/diff/220001/media/mojo/interfaces/...
media/mojo/interfaces/android_overlay.mojom:60: bool secure;
On 2017/04/03 19:12:40, dcheng wrote:
> I'm not sure what |secure| means here.

secure surface, as in protected content.

i've added a comment.

[liberato (no reviews please), 2017-04-04 22:56:21]

The CQ bit was checked by liberato@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 22:56:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-04 23:34:26]

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

[commit-bot: I haz the power, 2017-04-04 23:34:27]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[boliu, 2017-04-04 23:53:21]

looked at diff, so only minor things

https://codereview.chromium.org/2765443004/diff/160001/gpu/ipc/common/gpu_sur...
File gpu/ipc/common/gpu_surface_tracker.h (right):

https://codereview.chromium.org/2765443004/diff/160001/gpu/ipc/common/gpu_sur...
gpu/ipc/common/gpu_surface_tracker.h:46: int RegisterViewSurface(jobject
j_surface);
On 2017/04/04 17:49:28, liberato wrote:
> On 2017/03/30 22:42:02, boliu wrote:
> > err, I asked this to be refactored when this code moved here, but I guess
that
> > never happened.
> > 
> > So before this CL, surface_map_ and surface_view_map_ are parallel arrays,
so
> we
> > should code it as a single map
> > 
> > and then I think this CL is allowing a surface view to be registered without
a
> > AcceleratedWidget. imo that's easier to understand?
> 
> sorry about that.  will refactor per our offline discussion.  the CL is
> https://codereview.chromium.org/2791723003/

Oh I originally asked someone else, not you

Thanks for refactoring this :)

https://codereview.chromium.org/2765443004/diff/260001/content/browser/androi...
File content/browser/android/browser_jni_registrar.cc (right):

https://codereview.chromium.org/2765443004/diff/260001/content/browser/androi...
content/browser/android/browser_jni_registrar.cc:61: 
nit: remove empty line

https://codereview.chromium.org/2765443004/diff/260001/content/browser/androi...
File content/browser/android/dialog_overlay_operations_impl.cc (right):

https://codereview.chromium.org/2765443004/diff/260001/content/browser/androi...
content/browser/android/dialog_overlay_operations_impl.cc:51:
reinterpret_cast<DialogOverlayOperationsImpl*>(handle));
impl?

https://codereview.chromium.org/2765443004/diff/260001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayOperationsImpl.java
(right):

https://codereview.chromium.org/2765443004/diff/260001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayOperationsImpl.java:39:
public Handler getOverlayUiHandler() {
this is named OverlayUi :p

https://codereview.chromium.org/2765443004/diff/260001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayOperationsImpl.java:84:
native long nativeRegisterForTokens(long high, long low);
all native methods should be private

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

https://codereview.chromium.org/2765443004/diff/260001/content/renderer/rende...
content/renderer/render_frame_impl.h:54: #include
"media/base/android/routing_token_callback.h"
nit: platform-specific includes after the cross-platform ones, ie put this next
to renderer_media_player_manager.h

[tguilbert, 2017-04-05 01:03:56]

LGTM -- media/*

https://codereview.chromium.org/2765443004/diff/220001/content/public/android...
File
content/public/android/javatests/src/org/chromium/content/browser/androidoverlay/DialogOverlayImplTest.java
(right):

https://codereview.chromium.org/2765443004/diff/220001/content/public/android...
content/public/android/javatests/src/org/chromium/content/browser/androidoverlay/DialogOverlayImplTest.java:145:
mojoToken.high = routingToken.getHighForSerialization();
On 2017/04/04 17:49:29, liberato wrote:
> On 2017/04/03 19:12:40, dcheng wrote:
> > +tguilbert... I don't know much about the Java glue, but maybe there's a way
> to
> > avoid having to make this public.
> 
> Acknowledged.

There doesn't seem to be an obvious way to do this. From looking at the
generated mojo srcjar, there seems to be "public static UnguessableToken
deserialize(java.nio.ByteBuffer data)", but it might be simpler to leave it as
is.

I do not have another proposal otherwise.

[liberato (no reviews please), 2017-04-05 15:12:15]

The CQ bit was checked by liberato@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-05 15:12:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[liberato (no reviews please), 2017-04-05 15:13:31]

thanks for the feedback.

-fl

https://codereview.chromium.org/2765443004/diff/260001/content/browser/androi...
File content/browser/android/browser_jni_registrar.cc (right):

https://codereview.chromium.org/2765443004/diff/260001/content/browser/androi...
content/browser/android/browser_jni_registrar.cc:61: 
On 2017/04/04 23:53:21, boliu wrote:
> nit: remove empty line

Done.

https://codereview.chromium.org/2765443004/diff/260001/content/browser/androi...
File content/browser/android/dialog_overlay_operations_impl.cc (right):

https://codereview.chromium.org/2765443004/diff/260001/content/browser/androi...
content/browser/android/dialog_overlay_operations_impl.cc:51:
reinterpret_cast<DialogOverlayOperationsImpl*>(handle));
On 2017/04/04 23:53:21, boliu wrote:
> impl?

Done.

https://codereview.chromium.org/2765443004/diff/260001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayOperationsImpl.java
(right):

https://codereview.chromium.org/2765443004/diff/260001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayOperationsImpl.java:39:
public Handler getOverlayUiHandler() {
On 2017/04/04 23:53:21, boliu wrote:
> this is named OverlayUi :p

Done.

https://codereview.chromium.org/2765443004/diff/260001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayOperationsImpl.java:84:
native long nativeRegisterForTokens(long high, long low);
On 2017/04/04 23:53:21, boliu wrote:
> all native methods should be private

Done.

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

https://codereview.chromium.org/2765443004/diff/260001/content/renderer/rende...
content/renderer/render_frame_impl.h:54: #include
"media/base/android/routing_token_callback.h"
On 2017/04/04 23:53:21, boliu wrote:
> nit: platform-specific includes after the cross-platform ones, ie put this
next
> to renderer_media_player_manager.h

Done.

[commit-bot: I haz the power, 2017-04-05 15:16:09]

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

[commit-bot: I haz the power, 2017-04-05 15:16:10]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dcheng, 2017-04-06 07:14:51]

ipc lgtm

it's unfortunate, but feels unavoidable. hopefully we can get rid of this soon
without growing too many dependencies on it.

https://codereview.chromium.org/2765443004/diff/280001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2765443004/diff/280001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:542: if
(!overlay_routing_token_.has_value()) {
Slightly shorter to omit has_value()

https://codereview.chromium.org/2765443004/diff/280001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:544:
g_token_frame_map.Get().insert(
Out of curiosity, does emplace work now?

https://codereview.chromium.org/2765443004/diff/280001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:548: return
overlay_routing_token_.value();
Shorter alternative: return *overlay_routing_token_.

(Similar comments for uses of Optional elsewhere)

[liberato (no reviews please), 2017-04-10 18:31:03]

The CQ bit was checked by liberato@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-10 18:32:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[liberato (no reviews please), 2017-04-10 21:24:54]

thanks for the feedback.

-fl

https://codereview.chromium.org/2765443004/diff/280001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2765443004/diff/280001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:542: if
(!overlay_routing_token_.has_value()) {
On 2017/04/06 07:14:51, dcheng wrote:
> Slightly shorter to omit has_value()

Done.

https://codereview.chromium.org/2765443004/diff/280001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:544:
g_token_frame_map.Get().insert(
On 2017/04/06 07:14:51, dcheng wrote:
> Out of curiosity, does emplace work now?

yes, thanks.  done.

https://codereview.chromium.org/2765443004/diff/280001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:548: return
overlay_routing_token_.value();
On 2017/04/06 07:14:51, dcheng wrote:
> Shorter alternative: return *overlay_routing_token_.
> 
> (Similar comments for uses of Optional elsewhere)

Done, and elsewhere.

[commit-bot: I haz the power, 2017-04-10 23:08:46]

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

[commit-bot: I haz the power, 2017-04-10 23:08:48]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[liberato (no reviews please), 2017-04-21 00:39:46]

i rebased this onto the GpuSurfaceTracker CL for clarity, though it doesn't show
it as a dependent PS for some reason.

thanks
-fl

[boliu, 2017-04-22 00:22:43]

https://codereview.chromium.org/2765443004/diff/340001/base/android/java/src/...
File base/android/java/src/org/chromium/base/UnguessableToken.java (right):

https://codereview.chromium.org/2765443004/diff/340001/base/android/java/src/...
base/android/java/src/org/chromium/base/UnguessableToken.java:36: public long
getHighForSerialization() {
I think you should be using use native FromJavaUnguessableToken rather than
exposing these

https://codereview.chromium.org/2765443004/diff/340001/content/browser/androi...
File content/browser/android/dialog_overlay_operations_impl.cc (right):

https://codereview.chromium.org/2765443004/diff/340001/content/browser/androi...
content/browser/android/dialog_overlay_operations_impl.cc:41: if (!impl)
check redundant with java side one

https://codereview.chromium.org/2765443004/diff/340001/content/browser/androi...
content/browser/android/dialog_overlay_operations_impl.cc:81:
UnregisterForTokensIfNeeded();
DCHECK(!cvc_) instead?

https://codereview.chromium.org/2765443004/diff/340001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2765443004/diff/340001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:166: typedef
base::hash_map<base::UnguessableToken,
nit: c++11 "using" syntax

https://codereview.chromium.org/2765443004/diff/340001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/androidoverlay/AndroidOverlayProviderImpl.java
(right):

https://codereview.chromium.org/2765443004/diff/340001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/AndroidOverlayProviderImpl.java:61:
mNumOverlays++;
add thread asserts to this file as well? eg it's not easy to see access to this
is thread safe.

https://codereview.chromium.org/2765443004/diff/340001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayImpl.java
(right):

https://codereview.chromium.org/2765443004/diff/340001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayImpl.java:62:
Dialog dialog = new Dialog(context, android.R.style.Theme_NoDisplay);
do this stuff in dialogcore? seems odd this is here

https://codereview.chromium.org/2765443004/diff/340001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayImpl.java:156:
public void onWindowToken(final IBinder token) {
this class is implementing three interfaces, and it's really hard to keep track
of which method is from which interface. maybe make Host/Listener inner classes?
although at some point you have to wonder if all of these classes are actually
worth it..

at minimum add comments and group methods from same interface together

https://codereview.chromium.org/2765443004/diff/340001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayOperations.java
(right):

https://codereview.chromium.org/2765443004/diff/340001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayOperations.java:14:
interface DialogOverlayOperations {
is this interface necessary? I don't see any tests actually mocking this out

similarly, is the WindowTokenListener interface necessary?

Basically I don't quite understand the split between Ops and DialogSurfaceImpl.

https://codereview.chromium.org/2765443004/diff/340001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/androidoverlay/ThreadHoppingHost.java
(right):

https://codereview.chromium.org/2765443004/diff/340001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/ThreadHoppingHost.java:24:
private Semaphore mSemaphore = new Semaphore(0);
final

[liberato (no reviews please), 2017-04-24 21:22:34]

The CQ bit was checked by liberato@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-24 21:22:59]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[liberato (no reviews please), 2017-04-24 22:19:42]

thanks for the feedback.

-fl

https://codereview.chromium.org/2765443004/diff/340001/base/android/java/src/...
File base/android/java/src/org/chromium/base/UnguessableToken.java (right):

https://codereview.chromium.org/2765443004/diff/340001/base/android/java/src/...
base/android/java/src/org/chromium/base/UnguessableToken.java:36: public long
getHighForSerialization() {
On 2017/04/22 00:22:42, boliu wrote:
> I think you should be using use native FromJavaUnguessableToken rather than
> exposing these

i don't see how to convert from java or native UnguessableToken to mojom's java
UnguessableToken.

i'll add a method to UnguessableToken.java that will convert to mojo.  ... (some
typing happens here) ... but it turns out that i can't. base/ doesn't have
examples that make me think that it can depend on mojo/.  do you know if it's
allowed?

dcheng@ and tguilbert@ also looked at this, but so far none of us has figured
out how to remove it.

https://codereview.chromium.org/2765443004/diff/340001/content/browser/androi...
File content/browser/android/dialog_overlay_operations_impl.cc (right):

https://codereview.chromium.org/2765443004/diff/340001/content/browser/androi...
content/browser/android/dialog_overlay_operations_impl.cc:41: if (!impl)
On 2017/04/22 00:22:43, boliu wrote:
> check redundant with java side one

Done.

https://codereview.chromium.org/2765443004/diff/340001/content/browser/androi...
content/browser/android/dialog_overlay_operations_impl.cc:81:
UnregisterForTokensIfNeeded();
On 2017/04/22 00:22:42, boliu wrote:
> DCHECK(!cvc_) instead?

Done.

https://codereview.chromium.org/2765443004/diff/340001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2765443004/diff/340001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:166: typedef
base::hash_map<base::UnguessableToken,
On 2017/04/22 00:22:43, boliu wrote:
> nit: c++11 "using" syntax

Done.

https://codereview.chromium.org/2765443004/diff/340001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/androidoverlay/AndroidOverlayProviderImpl.java
(right):

https://codereview.chromium.org/2765443004/diff/340001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/AndroidOverlayProviderImpl.java:61:
mNumOverlays++;
On 2017/04/22 00:22:43, boliu wrote:
> add thread asserts to this file as well? eg it's not easy to see access to
this
> is thread safe.

Done.

https://codereview.chromium.org/2765443004/diff/340001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayImpl.java
(right):

https://codereview.chromium.org/2765443004/diff/340001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayImpl.java:62:
Dialog dialog = new Dialog(context, android.R.style.Theme_NoDisplay);
On 2017/04/22 00:22:43, boliu wrote:
> do this stuff in dialogcore? seems odd this is here

it was originally to allow DialogOverlayCoreTest to inject its own dialogs. 
however, i think that it's no longer needed.  i'll merge it back.

https://codereview.chromium.org/2765443004/diff/340001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayImpl.java:156:
public void onWindowToken(final IBinder token) {
On 2017/04/22 00:22:43, boliu wrote:
> this class is implementing three interfaces, and it's really hard to keep
track
> of which method is from which interface. maybe make Host/Listener inner
classes?
> although at some point you have to wonder if all of these classes are actually
> worth it..
> 
> at minimum add comments and group methods from same interface together

i've merged Ops, so WindowTokenListener is gone now.

Host: already an inner class of DialogOverlayCore for the junit test.

https://codereview.chromium.org/2765443004/diff/340001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayOperations.java
(right):

https://codereview.chromium.org/2765443004/diff/340001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayOperations.java:14:
interface DialogOverlayOperations {
On 2017/04/22 00:22:43, boliu wrote:
> is this interface necessary? I don't see any tests actually mocking this out
> 
> similarly, is the WindowTokenListener interface necessary?
> 
> Basically I don't quite understand the split between Ops and
DialogSurfaceImpl.

i had planned to add a DialogOverlayImpl junit test, but ended up going with an
integration test since it seemed more useful.

i'll merge all of it back into DialogOverlayImpl .

https://codereview.chromium.org/2765443004/diff/340001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/androidoverlay/ThreadHoppingHost.java
(right):

https://codereview.chromium.org/2765443004/diff/340001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/ThreadHoppingHost.java:24:
private Semaphore mSemaphore = new Semaphore(0);
On 2017/04/22 00:22:43, boliu wrote:
> final

Done.

[boliu, 2017-04-26 00:19:08]

some other suggestions, I think weakref is the only material comment left from
me :)

https://codereview.chromium.org/2765443004/diff/380001/content/browser/androi...
File content/browser/android/dialog_overlay_impl.h (right):

https://codereview.chromium.org/2765443004/diff/380001/content/browser/androi...
content/browser/android/dialog_overlay_impl.h:42: private:
nit: no need for double private

https://codereview.chromium.org/2765443004/diff/380001/content/browser/androi...
content/browser/android/dialog_overlay_impl.h:44:
base::android::ScopedJavaGlobalRef<jobject> receiver_;
the convention is to call this obj_, when native and java classes have the same
name, and roughly share the same lifetime

also.. I *think* this should be a JavaObjectWeakGlobalRef:
Every strong reference in native is a gc root to the garbage collector. So this
reference means that the context, and transitively the entire view tree will be
prevented from being gc-ed. But webview in particular relies on gc to collect
itself, so this would cause a leak, unless something guarantees DialogOverlays
are always dismissed.

The simple fix is use JavaObjectWeakGlobalRef. native is entirely owned by java
side, so shouldn't be a problem. Java side is kept alive by mojo, so I don't
think it's actually relying on this ref. Just need a null check after every
dereference, you can look at ContentViewCore as an example.

Alternative is to the ref be destroyed OnDetachedFromWindow (and created in
OnAttachedToWindow), which doesn't really work unless the observer is in java
code. So I guess JavaObjectWeakGlobalRef it is then..

https://codereview.chromium.org/2765443004/diff/380001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayImpl.java
(right):

https://codereview.chromium.org/2765443004/diff/380001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayImpl.java:256:
private native long nativeRegisterForTokens(long high, long low);
maybe should rename these to make it should like java-native pair, so can be
named nativeInit

https://codereview.chromium.org/2765443004/diff/380001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayImpl.java:261:
private static native void nativeUnregisterForTokens(long handle);
if you name the parameter "long nativeDialogOverlayImpl", then the generated jni
binding will call the an instance method on the native DialogOverlayImpl. then
this can probably just renamed nativeDestroy

https://codereview.chromium.org/2765443004/diff/380001/content/public/android...
File
content/public/android/javatests/src/org/chromium/content/browser/androidoverlay/DialogOverlayImplTest.java
(right):

https://codereview.chromium.org/2765443004/diff/380001/content/public/android...
content/public/android/javatests/src/org/chromium/content/browser/androidoverlay/DialogOverlayImplTest.java:172:
abstract class UIAction<T> {
ThreadUtils.runOnUiThreadBlockingNoException :p

[liberato (no reviews please), 2017-04-26 17:23:26]

The CQ bit was checked by liberato@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 17:24:06]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[liberato (no reviews please), 2017-04-26 17:26:35]

thanks for the feedback.  also added a fix for the initial position in
DialogOverlayCore.  not sure when i broke that.

now i'm worried about the GC-based cleanup path in webview...

thanks
-fl

https://codereview.chromium.org/2765443004/diff/380001/content/browser/androi...
File content/browser/android/dialog_overlay_impl.h (right):

https://codereview.chromium.org/2765443004/diff/380001/content/browser/androi...
content/browser/android/dialog_overlay_impl.h:42: private:
On 2017/04/26 00:19:08, boliu wrote:
> nit: no need for double private

Done.

https://codereview.chromium.org/2765443004/diff/380001/content/browser/androi...
content/browser/android/dialog_overlay_impl.h:44:
base::android::ScopedJavaGlobalRef<jobject> receiver_;
On 2017/04/26 00:19:08, boliu wrote:
> the convention is to call this obj_, when native and java classes have the
same
> name, and roughly share the same lifetime
> 
> also.. I *think* this should be a JavaObjectWeakGlobalRef:
> Every strong reference in native is a gc root to the garbage collector. So
this
> reference means that the context, and transitively the entire view tree will
be
> prevented from being gc-ed. But webview in particular relies on gc to collect
> itself, so this would cause a leak, unless something guarantees DialogOverlays
> are always dismissed.
> 
> The simple fix is use JavaObjectWeakGlobalRef. native is entirely owned by
java
> side, so shouldn't be a problem. Java side is kept alive by mojo, so I don't
> think it's actually relying on this ref. Just need a null check after every
> dereference, you can look at ContentViewCore as an example.
> 
> Alternative is to the ref be destroyed OnDetachedFromWindow (and created in
> OnAttachedToWindow), which doesn't really work unless the observer is in java
> code. So I guess JavaObjectWeakGlobalRef it is then..

done.

however, now i'm worried about the webview GC cleanup path.  if the java object
is finalized without a DialogOverlayImpl.cleanup() call, then it'll leak the
native side.

i suppose that i can |delete this| on a failed weak ref promotion, but that
seems a little hacky.

do you know if holding a native mojo client, which in turn has a mojo binding to
the DialogOverlayImpl java object, is enough to prevent GC?  i'm worried about
what happens if whatever mojo magic exists in java is shut down before AVDA is
destroyed.

https://codereview.chromium.org/2765443004/diff/380001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayImpl.java
(right):

https://codereview.chromium.org/2765443004/diff/380001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayImpl.java:256:
private native long nativeRegisterForTokens(long high, long low);
On 2017/04/26 00:19:08, boliu wrote:
> maybe should rename these to make it should like java-native pair, so can be
> named nativeInit

Done.

https://codereview.chromium.org/2765443004/diff/380001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayImpl.java:261:
private static native void nativeUnregisterForTokens(long handle);
On 2017/04/26 00:19:08, boliu wrote:
> if you name the parameter "long nativeDialogOverlayImpl", then the generated
jni
> binding will call the an instance method on the native DialogOverlayImpl. then
> this can probably just renamed nativeDestroy

Done.

https://codereview.chromium.org/2765443004/diff/380001/content/public/android...
File
content/public/android/javatests/src/org/chromium/content/browser/androidoverlay/DialogOverlayImplTest.java
(right):

https://codereview.chromium.org/2765443004/diff/380001/content/public/android...
content/public/android/javatests/src/org/chromium/content/browser/androidoverlay/DialogOverlayImplTest.java:172:
abstract class UIAction<T> {
On 2017/04/26 00:19:08, boliu wrote:
> ThreadUtils.runOnUiThreadBlockingNoException :p

shiny!

Done.

[boliu, 2017-04-26 17:40:11]

lgtm

https://codereview.chromium.org/2765443004/diff/380001/content/browser/androi...
File content/browser/android/dialog_overlay_impl.h (right):

https://codereview.chromium.org/2765443004/diff/380001/content/browser/androi...
content/browser/android/dialog_overlay_impl.h:44:
base::android::ScopedJavaGlobalRef<jobject> receiver_;
On 2017/04/26 17:26:34, liberato wrote:
> On 2017/04/26 00:19:08, boliu wrote:
> > the convention is to call this obj_, when native and java classes have the
> same
> > name, and roughly share the same lifetime
> > 
> > also.. I *think* this should be a JavaObjectWeakGlobalRef:
> > Every strong reference in native is a gc root to the garbage collector. So
> this
> > reference means that the context, and transitively the entire view tree will
> be
> > prevented from being gc-ed. But webview in particular relies on gc to
collect
> > itself, so this would cause a leak, unless something guarantees
DialogOverlays
> > are always dismissed.
> > 
> > The simple fix is use JavaObjectWeakGlobalRef. native is entirely owned by
> java
> > side, so shouldn't be a problem. Java side is kept alive by mojo, so I don't
> > think it's actually relying on this ref. Just need a null check after every
> > dereference, you can look at ContentViewCore as an example.
> > 
> > Alternative is to the ref be destroyed OnDetachedFromWindow (and created in
> > OnAttachedToWindow), which doesn't really work unless the observer is in
java
> > code. So I guess JavaObjectWeakGlobalRef it is then..
> 
> done.
> 
> however, now i'm worried about the webview GC cleanup path.  if the java
object
> is finalized without a DialogOverlayImpl.cleanup() call, then it'll leak the
> native side.
> 
> i suppose that i can |delete this| on a failed weak ref promotion, but that
> seems a little hacky.
> 
> do you know if holding a native mojo client, which in turn has a mojo binding
to
> the DialogOverlayImpl java object, is enough to prevent GC?  i'm worried about
> what happens if whatever mojo magic exists in java is shut down before AVDA is
> destroyed.

I would certainly hope so, but that's a question for mojo folks..

https://codereview.chromium.org/2765443004/diff/420001/content/public/android...
File
content/public/android/javatests/src/org/chromium/content/browser/androidoverlay/DialogOverlayImplTest.java
(right):

https://codereview.chromium.org/2765443004/diff/420001/content/public/android...
content/public/android/javatests/src/org/chromium/content/browser/androidoverlay/DialogOverlayImplTest.java:204:
ThreadUtils.runOnUiThreadBlockingNoException(new Callable<Void>() {
for void, there's runOnUiThreadBlocking which takes a Runnable

[commit-bot: I haz the power, 2017-04-26 17:48:28]

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

[commit-bot: I haz the power, 2017-04-26 17:48:30]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[liberato (no reviews please), 2017-04-26 18:26:28]

thanks for the feedback!

aelias: PTAL as owner @ content/renderer/render_frame_impl*

thanks
-fl

https://codereview.chromium.org/2765443004/diff/420001/content/public/android...
File
content/public/android/javatests/src/org/chromium/content/browser/androidoverlay/DialogOverlayImplTest.java
(right):

https://codereview.chromium.org/2765443004/diff/420001/content/public/android...
content/public/android/javatests/src/org/chromium/content/browser/androidoverlay/DialogOverlayImplTest.java:204:
ThreadUtils.runOnUiThreadBlockingNoException(new Callable<Void>() {
On 2017/04/26 17:40:11, boliu wrote:
> for void, there's runOnUiThreadBlocking which takes a Runnable

Done.

[aelias_OOO_until_Jul13, 2017-04-26 23:41:22]

https://codereview.chromium.org/2765443004/diff/440001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2765443004/diff/440001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:539: #if
defined(OS_ANDROID)
Likewise I would prefer OS_ANDROID be removed in renderer_host as well (I think
it would be needed to make your browsertests pass on non-Android anyway)

https://codereview.chromium.org/2765443004/diff/440001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2765443004/diff/440001/content/renderer/rende...
content/renderer/render_frame_impl.cc:5887: void
RenderFrameImpl::GetOverlayRoutingToken(
I typically expect methods named "Get" to be more passive than this, please
rename to RequestOverlayRoutingToken.

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

https://codereview.chromium.org/2765443004/diff/440001/content/renderer/rende...
content/renderer/render_frame_impl.h:1129: #if defined(OS_ANDROID)
Can you remove all "#if defined(OS_ANDROID)" from the renderer side?  I prefer
for renderer-side code to be compiled cross-platform unless there is a strong
reason otherwise.  One big advantage is that then you can run its unit tests on
Linux as well.  (Most developers find it hard/slow to locally run unit tests on
Android.)

[aelias_OOO_until_Jul13, 2017-04-26 23:54:07]

lgtm otherwise

[liberato (no reviews please), 2017-05-02 16:15:30]

The CQ bit was checked by liberato@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-02 16:15:50]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-02 16:47:33]

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

[commit-bot: I haz the power, 2017-05-02 16:47:34]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[liberato (no reviews please), 2017-05-02 17:03:34]

The CQ bit was checked by liberato@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-02 17:03:55]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[liberato (no reviews please), 2017-05-02 17:05:12]

thanks for the feedback, everybody!

the (hidden) CL on which this depends landed yesterday, so i'll start landing
this now.

thanks
-fl

https://codereview.chromium.org/2765443004/diff/440001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2765443004/diff/440001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:539: #if
defined(OS_ANDROID)
On 2017/04/26 23:41:22, aelias wrote:
> Likewise I would prefer OS_ANDROID be removed in renderer_host as well (I
think
> it would be needed to make your browsertests pass on non-Android anyway)

Done.

https://codereview.chromium.org/2765443004/diff/440001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2765443004/diff/440001/content/renderer/rende...
content/renderer/render_frame_impl.cc:5887: void
RenderFrameImpl::GetOverlayRoutingToken(
On 2017/04/26 23:41:22, aelias wrote:
> I typically expect methods named "Get" to be more passive than this, please
> rename to RequestOverlayRoutingToken.

Done.

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

https://codereview.chromium.org/2765443004/diff/440001/content/renderer/rende...
content/renderer/render_frame_impl.h:1129: #if defined(OS_ANDROID)
On 2017/04/26 23:41:22, aelias wrote:
> Can you remove all "#if defined(OS_ANDROID)" from the renderer side?  I prefer
> for renderer-side code to be compiled cross-platform unless there is a strong
> reason otherwise.  One big advantage is that then you can run its unit tests
on
> Linux as well.  (Most developers find it hard/slow to locally run unit tests
on
> Android.)

Done.

[commit-bot: I haz the power, 2017-05-02 17:54:51]

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

[commit-bot: I haz the power, 2017-05-02 17:54:52]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[liberato (no reviews please), 2017-05-02 18:17:18]

The CQ bit was checked by liberato@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-02 18:17:52]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-02 20:16:39]

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

[commit-bot: I haz the power, 2017-05-02 20:16:40]

Dry run: This issue passed the CQ dry run.

[liberato (no reviews please), 2017-05-02 20:17:29]

The CQ bit was checked by liberato@chromium.org

[liberato (no reviews please), 2017-05-02 20:17:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
tguilbert@chromium.org, dcheng@chromium.org, boliu@chromium.org,
aelias@chromium.org
Link to the patchset: https://codereview.chromium.org/2765443004/#ps520001
(title: "fixed test")

[liberato (no reviews please), 2017-05-02 20:17:38]

The CQ bit was checked by liberato@chromium.org

[commit-bot: I haz the power, 2017-05-02 20:18:10]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-02 20:23:58]

CQ is committing da patch.

Bot data: {"patchset_id": 520001, "attempt_start_ts": 1493756258338990,
"parent_rev": "8e6eb4e3f0954589213463fb852ebc6f14d77eea", "commit_rev":
"a8da3b572593e1a166bc2e3a22821a17feb0af27"}

[commit-bot: I haz the power, 2017-05-02 20:24:08]

Description was changed from

==========
AndroidOverlay implementation using Dialog.

This CL adds DialogOverlayImpl, which uses AndroidDialogs to provide an
overlay.  The intended use looks something like this:

RenderFrame(Impl) requests a routing token from RenderFrameHostImpl
RFHI returns token to RF
RF sends this token to WebMediaPlayerImpl
WMPI sends this (via chrome IPC) to AVDA (gpu process)
AVDA requests an overlay from AndroidOverlayProviderImpl using this
  token (via mojo to the browser).
AndroidOverlayProviderImpl creates a DialogOverlayImpl in the browser
AOPI returns the DialogOverlayImpl to AVDA.

BUG=618368
TEST=DialogOverlayImplTest, DialogOverlayCoreTest
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
AndroidOverlay implementation using Dialog.

This CL adds DialogOverlayImpl, which uses AndroidDialogs to provide an
overlay.  The intended use looks something like this:

RenderFrame(Impl) requests a routing token from RenderFrameHostImpl
RFHI returns token to RF
RF sends this token to WebMediaPlayerImpl
WMPI sends this (via chrome IPC) to AVDA (gpu process)
AVDA requests an overlay from AndroidOverlayProviderImpl using this
  token (via mojo to the browser).
AndroidOverlayProviderImpl creates a DialogOverlayImpl in the browser
AOPI returns the DialogOverlayImpl to AVDA.

BUG=618368
TEST=DialogOverlayImplTest, DialogOverlayCoreTest
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2765443004
Cr-Commit-Position: refs/heads/master@{#468760}
Committed:
https://chromium.googlesource.com/chromium/src/+/a8da3b572593e1a166bc2e3a2282...
==========

[commit-bot: I haz the power, 2017-05-02 20:24:10]

Committed patchset #27 (id:520001) as
https://chromium.googlesource.com/chromium/src/+/a8da3b572593e1a166bc2e3a2282...