Move video_capture.mojom out of content. [chromium/src : master]
Xiangjun Zhang (Gerrit)
Thanks for the quick reviewing! Addressed comments.
4 comments:
File content/browser/BUILD.gn:
Patch Set #1, Line 103:
//media/midi:mojo",Instead of adding this here, should it just be added to //media/capture's public_deps: https://cs. […]
Done
Accidental paste?
ah, yes. Thanks for catching. :)
File content/public/app/mojo/content_browser_manifest.json:
Patch Set #1, Line 60:
"media::mojom::VideoCaptureHost",This seems correct to have here. Just makes me wonder why things worked without it before. […]
It seems that this is only required after the *.mojom was moved out of content.
File media/capture/mojo/BUILD.gn:
Not sure if it's better, but have you considered merging this in with mojom("capture_types") and cal […]
Done as suggested.
To view, visit change 872101. To unsubscribe, or for help writing mail filters, visit settings.
Xiangjun Zhang (Gerrit)
Xiangjun Zhang would like Nasko Oskov to review this change.
Move video_capture.mojom out of content.
This is a prerequisite for the coming mirroring service CLs to not
depend on content. Moved the interfaces to media/capture/mojo/. No
functional change.
Bug: 734672
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel
Change-Id: Ie7a1d618920860cba08f1708f6cd9486db0b4956
---
M content/browser/BUILD.gn
M content/browser/renderer_host/media/video_capture_host.cc
M content/browser/renderer_host/media/video_capture_host.h
M content/browser/renderer_host/media/video_capture_unittest.cc
M content/common/BUILD.gn
M content/public/app/mojo/content_browser_manifest.json
M content/public/app/mojo/content_renderer_manifest.json
M content/renderer/media/video_capture_impl.cc
M content/renderer/media/video_capture_impl.h
M content/renderer/media/video_capture_impl_manager_unittest.cc
M content/renderer/media/video_capture_impl_unittest.cc
M content/test/BUILD.gn
M media/capture/BUILD.gn
M media/capture/mojo/BUILD.gn
R media/capture/mojo/video_capture.mojom
M services/video_capture/BUILD.gn
M services/video_capture/public/interfaces/BUILD.gn
M services/viz/privileged/interfaces/BUILD.gn
M services/viz/privileged/interfaces/compositing/BUILD.gn
M services/viz/public/cpp/compositing/BUILD.gn
M third_party/WebKit/public/BUILD.gn
21 files changed, 92 insertions(+), 86 deletions(-)
Xiangjun Zhang (Gerrit)
+nasko@: Need RS on content BUILD.gn files, video_capture.mojom, and the manifest files. PTAL. Thanks!
Nasko Oskov (Gerrit)
The move itself looks good, but I don't understand why the manifest files need changes. Can you clarify?
1 comment:
File content/public/app/mojo/content_browser_manifest.json:
Patch Set #4, Line 60:
"media::mojom::VideoCaptureHost",Why is this needed? If code is just being moved around where it lives in the directory structure, it should not affect what interfaces are exposed by each process.
To view, visit change 872101. To unsubscribe, or for help writing mail filters, visit settings.
Xiangjun Zhang (Gerrit)
nasko@: Thanks for reviewing. PTAL at the reply. Thanks!
1 comment:
Patch Set #4, Line 60:
"media::mojom::VideoCaptureHost",
Why is this needed? If code is just being moved around where it lives in the directory structure, it […]
This was not needed before. I think it might because the mojo interface was in content/common. After the move, when running Chrome, it ended with the following error: "Check failed: false. The Service Manager prevented service "content_renderer" from binding interface "media::mojom::VideoCaptureHost" in target service "content_browser". You probably need to update one or more service manifests to ensure that "content_browser" exposes "media::mojom::VideoCaptureHost" through a capability and that "content_renderer" requires that capability from the "content_browser" service." So I modified the manifest files as guided. Please let me know if I misunderstand anything.
To view, visit change 872101. To unsubscribe, or for help writing mail filters, visit settings.
Nasko Oskov (Gerrit)
Adding dcheng@ and tsepez@ so one of them can double check the changes in the manifest. I find it surprising that this was not required before and things worked, as it shouldn't matter where the code is physically located, but what module it lives in.
1 comment:
File content/public/app/mojo/content_browser_manifest.json:
Patch Set #4, Line 60:
"media::mojom::VideoDecodePerfHistory",This was not needed before. I think it might because the mojo interface was in content/common. […]
Thanks for the details. I find this a bit surprising that we didn't get this error before. I'll cc dcheng@ and tsepez@ to double check me here.
To view, visit change 872101. To unsubscribe, or for help writing mail filters, visit settings.
Xiangjun Zhang (Gerrit)
deng@, tsepez@: PTAL at the manifest changes as nasko@ commented. Thanks!
Daniel Cheng (Gerrit)
1 comment:
File content/public/app/mojo/content_browser_manifest.json:
Patch Set #5, Line 59:
"media::mojom::VideoCaptureHost",Let's clean up the listing on line 52 which is no longer needed (thus solving the mystery). Same for the other manifest files.
To view, visit change 872101. To unsubscribe, or for help writing mail filters, visit settings.
Tom Sepez (Gerrit)
Patch set 5:Code-Review +1
Xiangjun Zhang (Gerrit)
Addressed dcheng's comment in PS#6. Thanks for reviewing!
1 comment:
File content/public/app/mojo/content_browser_manifest.json:
Patch Set #5, Line 59:
"media::mojom::VideoDecodePerfHistory",Let's clean up the listing on line 52 which is no longer needed (thus solving the mystery). […]
Ah yes. Done. Thanks! :)
To view, visit change 872101. To unsubscribe, or for help writing mail filters, visit settings.
Xiangjun Zhang (Gerrit)
nasko@: Your concern is addressed in PS# 6. Still needs your RS. PTAL again. Thanks!
Nasko Oskov (Gerrit)
Patch set 6:Code-Review +1
Daniel Cheng (Gerrit)
1 comment:
File content/public/app/mojo/content_renderer_manifest.json:
Patch Set #5, Line 21:
"visitedlink::mojom::VisitedLinkNotificationSink",I think the renderer manifest still needs this change, unless I'm missing something.
To view, visit change 872101. To unsubscribe, or for help writing mail filters, visit settings.
Xiangjun Zhang (Gerrit)
dcheng@: PTAL at the reply to your comment. Thanks!
1 comment:
Patch Set #5, Line 21:
"visitedlink::mojom::VisitedLinkNotificationSink",I think the renderer manifest still needs this change, unless I'm missing something.
I did try. It seems the capture works properly without this line. The mojom was not in this manifest before either. So I removed this line. Should I re-add it?
To view, visit change 872101. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Cheng (Gerrit)
Patch set 6:Code-Review +1
1 comment:
Patch Set #5, Line 21:
"visitedlink::mojom::VisitedLinkNotificationSink",
I did try. It seems the capture works properly without this line. […]
Ah, it's because service manager doesn't broker those connections—it gets directly passed to VideoCaptureHost. So you're right that it's not needed (sorry I misread and thought the old name was in this file)
To view, visit change 872101. To unsubscribe, or for help writing mail filters, visit settings.
Xiangjun Zhang (Gerrit)
Patch set 6:Commit-Queue +2
Commit Bot (Gerrit)
Commit Bot merged this change.
Move video_capture.mojom out of content.
This is a prerequisite for the coming mirroring service CLs to not
depend on content. Moved the interfaces to media/capture/mojo/. No
functional change.
Bug: 734672
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel
Change-Id: Ie7a1d618920860cba08f1708f6cd9486db0b4956
Reviewed-on: https://chromium-review.googlesource.com/872101
Reviewed-by: Nasko Oskov <na...@chromium.org>
Reviewed-by: Daniel Cheng <dch...@chromium.org>
Reviewed-by: Tom Sepez <tse...@chromium.org>
Reviewed-by: Yuri Wiitala <m...@chromium.org>
Reviewed-by: Christian Fremerey <chfr...@chromium.org>
Commit-Queue: Xiangjun Zhang <x...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#531379}
---
M content/browser/BUILD.gn
M content/browser/renderer_host/media/video_capture_controller.cc
M content/browser/renderer_host/media/video_capture_controller_event_handler.h
M content/browser/renderer_host/media/video_capture_host.cc
M content/browser/renderer_host/media/video_capture_host.h
M content/browser/renderer_host/media/video_capture_unittest.cc
M content/common/BUILD.gn
M content/public/app/mojo/content_browser_manifest.json
M content/renderer/media/video_capture_impl.cc
M content/renderer/media/video_capture_impl.h
M content/renderer/media/video_capture_impl_manager_unittest.cc
M content/renderer/media/video_capture_impl_unittest.cc
M content/test/BUILD.gn
M media/capture/BUILD.gn
M media/capture/mojo/BUILD.gn
R media/capture/mojo/video_capture.mojom
M services/video_capture/BUILD.gn
M services/video_capture/public/interfaces/BUILD.gn
M services/viz/privileged/interfaces/BUILD.gn
M services/viz/privileged/interfaces/compositing/BUILD.gn
M services/viz/public/cpp/compositing/BUILD.gn
M third_party/WebKit/public/BUILD.gn
22 files changed, 93 insertions(+), 89 deletions(-)