Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(3)

Issue 2688193002: Mojo framework for AndroidOverlay. (Closed)

Created:
9 months, 1 week ago by liberato
Modified:
8 months ago
CC:
Aaron Boodman, abarth-chromium, agrieve+watch_chromium.org, alokp+watch_chromium.org, avayvod+watch_chromium.org, chromium-reviews, creis+watch_chromium.org, darin (slow to review), darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, mlamouri+watch-content_chromium.org, mlamouri+watch-media_chromium.org, nasko+codewatch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Mojo framework for AndroidOverlay. Basic implementation of the AndroidOverlay framework. Requests to create an overlay will create a do-nothing overlay impl. Adds the first instance of a Java-only mojo interface to the GPU interface registry. BUG=691283 TEST=mojo_android_overlay_unittest Review-Url: https://codereview.chromium.org/2688193002 Cr-Commit-Position: refs/heads/master@{#457964} Committed: https://chromium.googlesource.com/chromium/src/+/3df040e473be7b838c1648fc2599287d1e3e3549

Patch Set 1 #

Patch Set 2 : added to gpu process #

Patch Set 3 : compiles #

Patch Set 4 : cleanup #

Patch Set 5 : rebased #

Patch Set 6 : #ifdef android #

Patch Set 7 : client unit test #

Patch Set 8 : rebased #

Patch Set 9 : added #ifdef android #

Patch Set 10 : added some tests, minor fixes #

Patch Set 11 : BUILD.gn fixes #

Total comments: 6

Patch Set 12 : mojo fixes per rockot@, simplified AndroidOverlay api #

Patch Set 13 : fixed unittest #

Total comments: 4

Patch Set 14 : moved UI thread mojo init to Gpu...UIShim #

Total comments: 9

Patch Set 15 : cl comments, refactored MojoAndroidOverlay to inherit from mojom:... #

Total comments: 12

Patch Set 16 : removed OnInitialized, added const. #

Patch Set 17 : removed Setup #

Patch Set 18 : rebased. AndroidOverlay API changed at ToT. #

Patch Set 19 : replaced frame id, pid with token #

Total comments: 5

Patch Set 20 : better comments for mojom file #

Total comments: 4

Patch Set 21 : cl feedback #

Patch Set 22 : rebased #

Patch Set 23 : fixed mojo manifest bug introduced by rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+657 lines, -0 lines) Patch
M content/browser/gpu/gpu_process_host.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +5 lines, -0 lines 0 comments Download
M content/browser/gpu/gpu_process_host_ui_shim.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 2 chunks +10 lines, -0 lines 0 comments Download
M content/browser/gpu/gpu_process_host_ui_shim.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 3 chunks +32 lines, -0 lines 0 comments Download
M content/public/android/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 2 chunks +3 lines, -0 lines 0 comments Download
M content/public/android/java/src/org/chromium/content/browser/InterfaceRegistrarImpl.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 3 chunks +4 lines, -0 lines 0 comments Download
A content/public/android/java/src/org/chromium/content/browser/androidoverlay/AndroidOverlayImpl.java View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +35 lines, -0 lines 0 comments Download
A content/public/android/java/src/org/chromium/content/browser/androidoverlay/AndroidOverlayProviderImpl.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +49 lines, -0 lines 0 comments Download
M content/public/app/mojo/content_browser_manifest.json View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +1 line, -0 lines 0 comments Download
M media/base/android/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +1 line, -0 lines 0 comments Download
M media/base/android/android_overlay.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 2 chunks +4 lines, -0 lines 0 comments Download
A media/base/android/android_overlay_factory.h View 1 2 3 4 5 6 1 chunk +28 lines, -0 lines 0 comments Download
M media/mojo/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +4 lines, -0 lines 0 comments Download
M media/mojo/clients/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 2 chunks +13 lines, -0 lines 0 comments Download
A media/mojo/clients/mojo_android_overlay.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +55 lines, -0 lines 0 comments Download
A media/mojo/clients/mojo_android_overlay.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +74 lines, -0 lines 0 comments Download
A media/mojo/clients/mojo_android_overlay_factory.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +38 lines, -0 lines 0 comments Download
A media/mojo/clients/mojo_android_overlay_factory.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +22 lines, -0 lines 0 comments Download
A media/mojo/clients/mojo_android_overlay_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +215 lines, -0 lines 0 comments Download
M media/mojo/interfaces/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +4 lines, -0 lines 0 comments Download
A media/mojo/interfaces/android_overlay.mojom View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +60 lines, -0 lines 0 comments Download

Messages

Total messages: 91 (58 generated)
liberato
boliu: i've not included any code that might do something useful, but that will be ...
9 months ago (2017-02-14 21:45:51 UTC) #23
boliu
looks much simpler than binder, although I guess all the implementation is not actually here ...
9 months ago (2017-02-16 22:24:19 UTC) #37
Ken Rockot(use gerrit already)
This all looks very reasonable to me. See inline comments about fixing the GpuProcessHost code. ...
9 months ago (2017-02-17 06:29:57 UTC) #38
liberato
rockot: i really appreciate your detailed help. i've been testing by adding debug code in ...
9 months ago (2017-02-17 17:47:13 UTC) #43
Ken Rockot(use gerrit already)
On Fri, Feb 17, 2017 at 9:47 AM, <liberato@chromium.org> wrote: > rockot: i really appreciate ...
9 months ago (2017-02-17 17:55:55 UTC) #44
boliu
https://codereview.chromium.org/2688193002/diff/240001/content/browser/gpu/gpu_process_host.cc File content/browser/gpu/gpu_process_host.cc (right): https://codereview.chromium.org/2688193002/diff/240001/content/browser/gpu/gpu_process_host.cc#newcode326 content/browser/gpu/gpu_process_host.cc:326: void BindJavaInterface(mojo::InterfaceRequest<Interface> request) { if we end up creating ...
9 months ago (2017-02-17 21:20:51 UTC) #47
boliu
And getting off topic.. > ultimately we'd love to move the low-level mojo transport on ...
9 months ago (2017-02-17 21:24:10 UTC) #48
liberato
sorry for the delay. thanks for the comments! -fl https://codereview.chromium.org/2688193002/diff/240001/content/browser/gpu/gpu_process_host.cc File content/browser/gpu/gpu_process_host.cc (right): https://codereview.chromium.org/2688193002/diff/240001/content/browser/gpu/gpu_process_host.cc#newcode326 content/browser/gpu/gpu_process_host.cc:326: ...
8 months, 4 weeks ago (2017-02-22 16:25:01 UTC) #49
Ken Rockot(use gerrit already)
On 2017/02/22 at 16:25:01, liberato wrote: > sorry for the delay. thanks for the comments! ...
8 months, 4 weeks ago (2017-02-22 16:30:10 UTC) #50
boliu
only minor things since this CL is not really complete https://codereview.chromium.org/2688193002/diff/260001/content/browser/gpu/gpu_process_host_ui_shim.cc File content/browser/gpu/gpu_process_host_ui_shim.cc (right): https://codereview.chromium.org/2688193002/diff/260001/content/browser/gpu/gpu_process_host_ui_shim.cc#newcode231 ...
8 months, 4 weeks ago (2017-02-22 18:03:50 UTC) #51
liberato
incomplete: intentionally. my new goal is to make incremental progress after abandoning https://codereview.chromium.org/2178973004/ . it ...
8 months, 4 weeks ago (2017-02-22 19:10:50 UTC) #52
boliu
content lgtm https://codereview.chromium.org/2688193002/diff/260001/content/public/android/java/src/org/chromium/content/browser/androidoverlay/AndroidOverlayImpl.java File content/public/android/java/src/org/chromium/content/browser/androidoverlay/AndroidOverlayImpl.java (right): https://codereview.chromium.org/2688193002/diff/260001/content/public/android/java/src/org/chromium/content/browser/androidoverlay/AndroidOverlayImpl.java#newcode14 content/public/android/java/src/org/chromium/content/browser/androidoverlay/AndroidOverlayImpl.java:14: * Default AndroidOverlay impl. Will use a ...
8 months, 4 weeks ago (2017-02-22 19:16:00 UTC) #53
liberato
On 2017/02/22 19:16:00, boliu wrote: > content lgtm > > https://codereview.chromium.org/2688193002/diff/260001/content/public/android/java/src/org/chromium/content/browser/androidoverlay/AndroidOverlayImpl.java > File > content/public/android/java/src/org/chromium/content/browser/androidoverlay/AndroidOverlayImpl.java ...
8 months, 4 weeks ago (2017-02-22 19:23:38 UTC) #54
dcheng
Sorry for the delayed review =( https://codereview.chromium.org/2688193002/diff/280001/content/public/android/java/src/org/chromium/content/browser/androidoverlay/AndroidOverlayProviderImpl.java File content/public/android/java/src/org/chromium/content/browser/androidoverlay/AndroidOverlayProviderImpl.java (right): https://codereview.chromium.org/2688193002/diff/280001/content/public/android/java/src/org/chromium/content/browser/androidoverlay/AndroidOverlayProviderImpl.java#newcode25 content/public/android/java/src/org/chromium/content/browser/androidoverlay/AndroidOverlayProviderImpl.java:25: public void createOverlay(AndroidOverlayClient ...
8 months, 3 weeks ago (2017-02-28 05:22:04 UTC) #55
liberato
thanks for the comments, everybody. -fl https://codereview.chromium.org/2688193002/diff/280001/content/public/android/java/src/org/chromium/content/browser/androidoverlay/AndroidOverlayProviderImpl.java File content/public/android/java/src/org/chromium/content/browser/androidoverlay/AndroidOverlayProviderImpl.java (right): https://codereview.chromium.org/2688193002/diff/280001/content/public/android/java/src/org/chromium/content/browser/androidoverlay/AndroidOverlayProviderImpl.java#newcode25 content/public/android/java/src/org/chromium/content/browser/androidoverlay/AndroidOverlayProviderImpl.java:25: public void createOverlay(AndroidOverlayClient ...
8 months, 2 weeks ago (2017-03-06 07:51:05 UTC) #56
liberato
sorry about the delay. i've finally gotten around to trying out removing (frame id, pid) ...
8 months, 1 week ago (2017-03-11 21:39:36 UTC) #57
dcheng
sorry for the review delay. mojo lgtm, but if you don't mind, can you include ...
8 months ago (2017-03-16 08:51:40 UTC) #58
dcheng
https://codereview.chromium.org/2688193002/diff/360001/media/mojo/interfaces/android_overlay.mojom File media/mojo/interfaces/android_overlay.mojom (right): https://codereview.chromium.org/2688193002/diff/360001/media/mojo/interfaces/android_overlay.mojom#newcode11 media/mojo/interfaces/android_overlay.mojom:11: interface AndroidOverlayProvider { Also, some interface level comments that ...
8 months ago (2017-03-16 08:54:25 UTC) #59
dcheng
8 months ago (2017-03-16 08:54:57 UTC) #60
dcheng
8 months ago (2017-03-16 08:55:23 UTC) #61
liberato
dcheng: i'll be sure to add you to the rest of the impl CLs. thanks ...
8 months ago (2017-03-16 16:53:53 UTC) #64
xhwang
LGTM with tiny nits https://codereview.chromium.org/2688193002/diff/380001/media/mojo/interfaces/android_overlay.mojom File media/mojo/interfaces/android_overlay.mojom (right): https://codereview.chromium.org/2688193002/diff/380001/media/mojo/interfaces/android_overlay.mojom#newcode19 media/mojo/interfaces/android_overlay.mojom:19: CreateOverlay(AndroidOverlay& overlay, AndroidOverlayClient client, AndroidOverlayConfig ...
8 months ago (2017-03-16 19:12:50 UTC) #66
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2688193002/400001
8 months ago (2017-03-16 19:54:04 UTC) #69
liberato
thanks for the feedback! -fl https://codereview.chromium.org/2688193002/diff/380001/media/mojo/interfaces/android_overlay.mojom File media/mojo/interfaces/android_overlay.mojom (right): https://codereview.chromium.org/2688193002/diff/380001/media/mojo/interfaces/android_overlay.mojom#newcode19 media/mojo/interfaces/android_overlay.mojom:19: CreateOverlay(AndroidOverlay& overlay, AndroidOverlayClient client, ...
8 months ago (2017-03-16 19:54:14 UTC) #70
commit-bot: I haz the power
Failed to apply patch for content/public/android/java/src/org/chromium/content/browser/InterfaceRegistrarImpl.java: While running git apply --index -p1; error: patch failed: ...
8 months ago (2017-03-16 23:46:21 UTC) #72
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2688193002/420001
8 months ago (2017-03-17 05:55:37 UTC) #75
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_tsan_rel_ng/builds/34373)
8 months ago (2017-03-17 08:31:44 UTC) #77
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2688193002/420001
8 months ago (2017-03-17 16:48:44 UTC) #79
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_tsan_rel_ng/builds/34591)
8 months ago (2017-03-17 18:07:11 UTC) #81
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2688193002/440001
8 months ago (2017-03-17 23:06:37 UTC) #84
commit-bot: I haz the power
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_ng/builds/403356)
8 months ago (2017-03-18 01:01:06 UTC) #86
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2688193002/440001
8 months ago (2017-03-18 06:14:45 UTC) #88
commit-bot: I haz the power
8 months ago (2017-03-18 08:07:59 UTC) #91
Message was sent while issue was closed.
Committed patchset #23 (id:440001) as
https://chromium.googlesource.com/chromium/src/+/3df040e473be7b838c1648fc2599...

Powered by Google App Engine
This is Rietveld efc10ee0f