Summary: | Support for HTML Media Capture API | ||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | jvn <jvnforums10> | ||||||||||||||||||||||||||||||||||||
Component: | Platform | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||
Severity: | Enhancement | CC: | 709922234, agold, chrisjshull, commit-queue, eric.carlson, fred.wang, glenn, gruan, jay, jonlee, js45.yang, jvnforums10, mcatanzaro, mdaiter, mitz, rcombs, sam, skyul, syoichi, thorton, tonikitoo, tonyg, webkit-bug-importer, wonsuk11.lee | ||||||||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||||||||||||||||||
URL: | http://www.w3.org/TR/2011/WD-html-media-capture-20110414/ | ||||||||||||||||||||||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=158799 | ||||||||||||||||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||||||||||||||||
Bug Blocks: | 78947 | ||||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
jvn
2010-07-30 04:23:09 PDT
*** Bug 16474 has been marked as a duplicate of this bug. *** *** Bug 58184 has been marked as a duplicate of this bug. *** (In reply to comment #0) > Add The Capture API interface to WebKit API. > This API provides access to the audio, image and video capture capabilities of the device. > http://www.w3.org/TR/2010/WD-capture-api-20100401/ The most recent working draft is located at [1], dated December 2012. [1] http://www.w3.org/TR/html-media-capture/ *** Bug 158800 has been marked as a duplicate of this bug. *** Created attachment 293962 [details]
Proposed patch.
Comment on attachment 293962 [details] Proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=293962&action=review > Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegatePrivate.h:67 > +- (void)_webView:(WKWebView *)webVIew checkUserMediaPermissionForURL:(NSURL *)url mainFrameURL:(NSURL *)mainFrameURL frameIdentifier:(NSUInteger)frameIdentifier decisionHandler:(void (^)(NSString *salt, BOOL authorized))decisionHandler WK_API_AVAILABLE(ios(10.3)); s/VI/Vi > Source/WebKit2/UIProcess/WKUserMediaProviderIOS.h:45 > +@interface WKUserMediaProviderIOS : NSObject Do you think it would be reasonable to have the same interface on Mac, and call this WKUserMediaProvider, and have it implemented perhaps in platform specific ways (or perhaps shared!). > Source/WebKit2/UIProcess/WKUserMediaProviderIOS.mm:56 > +-(void)decidePolicyForUserMediaRequestFromOrigin:(SecurityOrigin&)userMediaOrigin topLevelOrigin:(SecurityOrigin&)topLevelOrigin frame:(WebFrameProxy&)frame request:(UserMediaPermissionRequestProxy&)permissionRequest view:(WKWebView*)view { Space after the -, and before the star in WKWebView* > Source/WebKit2/UIProcess/WebPageProxy.h:368 > + PageClient& pageClient() { return m_pageClient; } This is surprising. The fact that it wasn't here before suggests that maybe it shouldn't be? (In reply to comment #7) > Comment on attachment 293962 [details] > Proposed patch. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=293962&action=review > > > Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegatePrivate.h:67 > > +- (void)_webView:(WKWebView *)webVIew checkUserMediaPermissionForURL:(NSURL *)url mainFrameURL:(NSURL *)mainFrameURL frameIdentifier:(NSUInteger)frameIdentifier decisionHandler:(void (^)(NSString *salt, BOOL authorized))decisionHandler WK_API_AVAILABLE(ios(10.3)); > > s/VI/Vi Done. > > > Source/WebKit2/UIProcess/WKUserMediaProviderIOS.h:45 > > +@interface WKUserMediaProviderIOS : NSObject > > Do you think it would be reasonable to have the same interface on Mac, and > call this WKUserMediaProvider, and have it implemented perhaps in platform > specific ways (or perhaps shared!). Yes! It will have to be an interface and implemented differently on Mac and iOS because the AVFoundation APIs used in the class are platform specific, but I will make this change and implement it for iOS. > > > Source/WebKit2/UIProcess/WKUserMediaProviderIOS.mm:56 > > +-(void)decidePolicyForUserMediaRequestFromOrigin:(SecurityOrigin&)userMediaOrigin topLevelOrigin:(SecurityOrigin&)topLevelOrigin frame:(WebFrameProxy&)frame request:(UserMediaPermissionRequestProxy&)permissionRequest view:(WKWebView*)view { > > Space after the -, and before the star in WKWebView* Done. > > > Source/WebKit2/UIProcess/WebPageProxy.h:368 > > + PageClient& pageClient() { return m_pageClient; } > > This is surprising. The fact that it wasn't here before suggests that maybe > it shouldn't be? Hm maybe not. I exposed it because the UserMediaPermissionRequestManagerProxy is calling decidePolicyForUserMediaPermissionRequest on the page proxy's uiClient for the C API so I wanted to do something similar for the WebKit2 API, but perhaps it would make more sense to have UserMediaPermissionRequestManagerProxy call decidePolicyForUserMediaPermissionRequest on the page proxy directly and the page proxy can call the appropriate methods on the respective clients. Thanks for the review! V2 coming shortly. Created attachment 294315 [details]
Proposed patch V2
Attachment 294315 [details] did not pass style-queue:
ERROR: Source/WebKit2/UIProcess/ios/WKUserMediaCaptureDeviceAuthorizerIOS.mm:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5]
ERROR: Source/WebKit2/UIProcess/ios/WKUserMediaCaptureDeviceAuthorizerIOS.mm:11: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4]
Total errors found: 2 in 15 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 294315 [details] Proposed patch V2 View in context: https://bugs.webkit.org/attachment.cgi?id=294315&action=review > Source/WebKit2/UIProcess/PageClient.h:142 > + virtual WKUserMediaProvider *userMediaProvider() = 0; You should define WKUserMediaProvider as an abstract C++ API, in a non-ObjC header, so other ports can implement it. Created attachment 294531 [details]
Proposed patc v3
Created attachment 294532 [details]
Proposed patch v3
Comment on attachment 294532 [details] Proposed patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=294532&action=review > Source/WebKit2/UIProcess/Cocoa/WKUserMediaProviderCocoa.mm:67 > + void (^uiDelegateAuthorizationBlock)(void) = ^ { Much of this should be in UIDelegate > Source/WebKit2/UIProcess/Cocoa/WKUserMediaProviderCocoa.mm:73 > + if ((requiresAudio!= authorizedMicrophone) || (requiresVideo != authorizedCamera)) { space before the != > Source/WebKit2/UIProcess/WKUserMediaProvider.h:40 > +class WKUserMediaProvider { Now that this is a C++ class in the WebKit namespace you can (should!) drop the prefix. > Source/WebKit2/UIProcess/WKUserMediaProvider.h:45 > + virtual void mediaStateDidChange(WebCore::MediaProducer::MediaStateFlags, WKWebView*) = 0; Very surprised to see WKWebView here. > Source/WebKit2/UIProcess/ios/PageClientImplIOS.mm:231 > +WKUserMediaProvider& PageClientImpl::userMediaProvider() This all seems wrong. It should be owned by WebPageProxy, know nothing about WKWebView, and communicate with WKWebView and friends when needed via the PageClient/UIClient/etc. You may want to have Sam or Anders sit down and chat about this before going crazy making those changes, they will have Opinions. Comment on attachment 294532 [details] Proposed patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=294532&action=review > Source/WebKit2/UIProcess/Cocoa/WKUserMediaProviderCocoa.h:42 > +class WKUserMediaProviderCocoa final : public WKUserMediaProvider { No WK prefix here either. > Source/WebKit2/UIProcess/Cocoa/WKUserMediaProviderCocoa.mm:86 > + void (^cameraAuthorizationBlock)(void) = ^ { > + if (requiresVideo) { > + [captureDeviceAuthorizer() requestCameraAuthorization:^(BOOL authorized) { I think instead of this mechanism, this should be the default implementation of the UIDelegate, perhaps. > Source/WebKit2/WebKit2.xcodeproj/project.pbxproj:3265 > - 7CB16FE41724B9B5007A0A95 /* com.apple.WebKit.plugin-common.sb.in */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text; name = "com.apple.WebKit.plugin-common.sb.in"; sourceTree = "<group>"; }; > + 7CB16FE41724B9B5007A0A95 /* com.apple.WebKit.plugin-common.sb.in */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text; path = "com.apple.WebKit.plugin-common.sb.in"; sourceTree = "<group>"; }; This shouldn't be in this patch. Created attachment 294713 [details]
Proposed Patch V4
Created attachment 294716 [details]
Proposed Patch V4
Hm my patch still seems to be failing when building for Mac. I'm going to make a few changes and try again. Created attachment 294726 [details]
Proposed Patch V4
Created attachment 294743 [details]
Proposed Patch v5
Thanks to a little help from Eric, I think this one is going to be all green!
Created attachment 294754 [details]
Proposed Patch V6
Okay, let's try that again.
Created attachment 294760 [details]
Proposed Patch V7
Aaaaand again.
Created attachment 294777 [details]
Proposed Patch V8
Added the proper API annotations.
Comment on attachment 294777 [details] Proposed Patch V8 View in context: https://bugs.webkit.org/attachment.cgi?id=294777&action=review > Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:325 > + if (!delegate || !m_uiDelegate.m_delegateMethods.webViewRequestUserMediaAuthorizationForMicrophoneCameraUrlMainFrameURLDecisionHandler) { URL or Url? Created attachment 294874 [details]
Proposed Patch V9
Comment on attachment 294874 [details] Proposed Patch V9 View in context: https://bugs.webkit.org/attachment.cgi?id=294874&action=review > Source/WebKit2/UIProcess/Cocoa/UIDelegate.h:118 > +#if PLATFORM(IOS) > + WKUserMediaCaptureDeviceAuthorizerIOS *captureDeviceAuthorizer(); > + RetainPtr<WKUserMediaCaptureDeviceAuthorizerIOS> m_captureDeviceAuthorizer; > +#endif This class really shouldn't have any data members. Can this be merged into WebPageProxy. > Source/WebKit2/UIProcess/Cocoa/UIDelegate.h:119 > + WebCore::MediaProducer::MediaStateFlags m_mediaState { WebCore::MediaProducer::IsNotPlaying }; Ditto. > Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:375 > + void (^cameraAuthorizationBlock)(void) = ^ { > + if (requiresVideo) { > + [captureDeviceAuthorizer() requestCameraAuthorization:^(BOOL authorized) { > + if (!authorized) > + request.deny(UserMediaPermissionRequestProxy::UserMediaAccessDenialReason::PermissionDenied); > + else > + uiDelegateAuthorizationBlock(); > + }]; > + } else > + uiDelegateAuthorizationBlock(); > + }; > + > + if (requiresAudio) { > + [captureDeviceAuthorizer() requestMicrophoneAuthorization:^(BOOL authorized) { > + if (!authorized) > + request.deny(UserMediaPermissionRequestProxy::UserMediaAccessDenialReason::PermissionDenied); > + else > + cameraAuthorizationBlock(); > + }]; > + } else > + cameraAuthorizationBlock(); Why does this need to be iOS specific? > Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:395 > + [(id <WKUIDelegatePrivate>)delegate _webView:webView checkUserMediaPermissionForURL:requestFrameURL mainFrameURL:mainFrameURL frameIdentifier:frame.frameID() decisionHandler:^(NSString *salt, BOOL authorized) { It's a bit odd that you pass SecurityOrigin's to this level, but then don't use them, but rather get URLs. > Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:402 > +void UIDelegate::UIClient::isPlayingAudioDidChange(WebKit::WebPageProxy& page) It is really weird that isPlayingAudioDidChange is what triggers _webViewDidBeginCaptureSession/_webViewDidEndCaptureSession. This should be re-worked to be more direct. > Source/WebKit2/UIProcess/ios/WKUserMediaCaptureDeviceAuthorizerIOS.h:28 > +@interface WKUserMediaCaptureDeviceAuthorizerIOS : NSObject As this class is not AP/SPII, it should not have the WK prefix. I am generally confused about why this class is necessary, as it seems to store no state. Comment on attachment 294874 [details] Proposed Patch V9 View in context: https://bugs.webkit.org/attachment.cgi?id=294874&action=review Thanks for the review! >> Source/WebKit2/UIProcess/Cocoa/UIDelegate.h:118 >> +#endif > > This class really shouldn't have any data members. Can this be merged into WebPageProxy. Will move these. >> Source/WebKit2/UIProcess/Cocoa/UIDelegate.h:119 >> + WebCore::MediaProducer::MediaStateFlags m_mediaState { WebCore::MediaProducer::IsNotPlaying }; > > Ditto. Ditto. >> Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:375 >> + cameraAuthorizationBlock(); > > Why does this need to be iOS specific? This is iOS specific because Mac does not require apps to request permission to use media capture devices. >> Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:395 >> + [(id <WKUIDelegatePrivate>)delegate _webView:webView checkUserMediaPermissionForURL:requestFrameURL mainFrameURL:mainFrameURL frameIdentifier:frame.frameID() decisionHandler:^(NSString *salt, BOOL authorized) { > > It's a bit odd that you pass SecurityOrigin's to this level, but then don't use them, but rather get URLs. Will refactor. >> Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:402 >> +void UIDelegate::UIClient::isPlayingAudioDidChange(WebKit::WebPageProxy& page) > > It is really weird that isPlayingAudioDidChange is what triggers _webViewDidBeginCaptureSession/_webViewDidEndCaptureSession. This should be re-worked to be more direct. This is what is currently being used on Mac to notify the client of a capture session beginning/ending. Should I write a new method and only call that one and let the Mac team know to change their implementation, or should I continue to call this method to let clients using the C API know of a change in state, and also call a new method to notify WK2 clients? >> Source/WebKit2/UIProcess/ios/WKUserMediaCaptureDeviceAuthorizerIOS.h:28 >> +@interface WKUserMediaCaptureDeviceAuthorizerIOS : NSObject > > As this class is not AP/SPII, it should not have the WK prefix. I am generally confused about why this class is necessary, as it seems to store no state. I will remove this class and folds its logic into UIDelegate. Created attachment 295111 [details]
Proposed Patch v10
Forgot to rebase like an amateur. Fixed patch coming soon. Created attachment 295147 [details]
Rebased patch.
Comment on attachment 295147 [details] Rebased patch. View in context: https://bugs.webkit.org/attachment.cgi?id=295147&action=review So much progress! > Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegatePrivate.h:62 > +- (void)_webView:(WKWebView *)webView checkUserMediaPermissionForURL:(NSURL *)url mainFrameURL:(NSURL *)mainFrameURL frameIdentifier:(NSUInteger)frameIdentifier decisionHandler:(void (^)(NSString *salt, BOOL authorized))decisionHandler WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA)); What's up with the salt parameter? > Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:351 > +#if PLATFORM(IOS) Likely we should have a function that does this work and implement it separately in e.g. new UIDelegateIOS and UIDelegateMac files, doing the different things, to satisfy Sam's comment above. There are also other things around that should be (separately) moved into something like that. Also, tests? Created attachment 295504 [details]
Fixed Patch.
Comment on attachment 295504 [details] Fixed Patch. Rejecting attachment 295504 [details] from commit-queue. agold@apple.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights. Attachment 295504 [details] did not pass style-queue:
ERROR: Source/WebKit2/ChangeLog:45: Line contains tab character. [whitespace/tab] [5]
ERROR: Source/WebKit2/UIProcess/WebPageProxy.cpp:6413: Line contains only semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5]
Total errors found: 2 in 8 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 295508 [details]
Fixed Patch.
Comment on attachment 295508 [details] Fixed Patch. Rejecting attachment 295508 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 295508, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in Source/WebKit2/ChangeLog contains OOPS!. Full output: http://webkit-queues.webkit.org/results/2584191 Created attachment 295510 [details]
Fixed Patch.
(In reply to comment #32) > Comment on attachment 295147 [details] > Rebased patch. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=295147&action=review > > So much progress! > > > Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegatePrivate.h:62 > > +- (void)_webView:(WKWebView *)webView checkUserMediaPermissionForURL:(NSURL *)url mainFrameURL:(NSURL *)mainFrameURL frameIdentifier:(NSUInteger)frameIdentifier decisionHandler:(void (^)(NSString *salt, BOOL authorized))decisionHandler WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA)); > > What's up with the salt parameter? It is the string we use to make sure capture device IDs are unique per domain. > > > Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:351 > > +#if PLATFORM(IOS) > > Likely we should have a function that does this work and implement it > separately in e.g. new UIDelegateIOS and UIDelegateMac files, doing the > different things, to satisfy Sam's comment above. There are also other > things around that should be (separately) moved into something like that. Filed: https://bugs.webkit.org/show_bug.cgi?id=165101 Comment on attachment 295510 [details] Fixed Patch. Clearing flags on attachment: 295510 Committed r209010: <http://trac.webkit.org/changeset/209010> |