Implement functionality of media capture on iOS WK2
Created attachment 281655 [details] Patch
Comment on attachment 281655 [details] Patch Any way we can test this?
(In reply to comment #2) > Comment on attachment 281655 [details] > Patch > > Any way we can test this? I spoke with Jon and Eric, and both think it is difficult as the functionality involves use of the camera and UI interaction, both of which aren't accessible from LayoutTests. Jon thinks that we could possibly use UI-automation tests, but that isn't fully set up for use currently.
Comment on attachment 281655 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=281655&action=review > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:502 > + if (![UIImagePickerController isCameraDeviceAvailable: UIImagePickerControllerCameraDeviceFront]) No spaces after colons. > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:508 > + return YES; What if this enum had more values, and none of the ones you looked for were available? Seems like this should be structured differently. > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:514 > +- (UIImagePickerControllerCameraDevice)_getCameraDevice No "get" unless there are out arguments. Also, maybe this should be a free function that does the enum translation, and you pass in _captureType from the caller. > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:535 > + [_imagePicker setCameraDevice: [self _getCameraDevice]]; No space after the :
Comment on attachment 281655 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=281655&action=review > Source/WebCore/ChangeLog:9 > + No new tests (OOPS!). UI-Automation is necessary for tests. You should just remove this line. > Source/WebCore/html/HTMLInputElement.cpp:1774 > + if (isFileUpload() && fastHasAttribute(captureAttr)) { I'm not sure the fastHasAttribute adds anything here. fastGet and fastHas do the same thing. So here it just does the same thing twice. > Source/WebCore/html/HTMLInputElement.cpp:1775 > + if (fastGetAttribute(captureAttr).convertToASCIIUppercase() == WTF::AtomicString("FRONT")) It is more common to use equalLettersIgnoringASCIICase: if (equalLettersIgnoringASCIICase(fastGetAttribute(captureAttr), "front") > Source/WebCore/html/HTMLInputElement.idl:134 > // See http://www.w3.org/TR/html-media-capture/ > - [Conditional=MEDIA_CAPTURE, Reflect] attribute boolean capture; > + [Conditional=MEDIA_CAPTURE, Reflect] attribute DOMString capture; The latest draft of this spec still says this attribute should be a boolean. Am I missing something? https://w3c.github.io/html-media-capture/ (W3C Editor's Draft 26 April 2016) partial interface HTMLInputElement { [CEReactions] attribute boolean capture; }; If this really is a non-standard web-facing change, there should be some comment about this in the ChangeLog justifying the non-standard behavior. > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:499 > +- (BOOL)_shouldOpenMediaDevice This method name is deceptive since it sounds like it would not change state, but it does possibly change state. Perhaps there should be two methods. - (BOOL)_shouldOpenMediaDevice; - (void)_adjustCaptureType; Since this work is only done in one place, the adjust would only need to happen if we shouldOpenMediaDevice. > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:501 > + if (_captureType != WebCore::CaptureTypeNone && [UIImagePickerController isSourceTypeAvailable:UIImagePickerControllerSourceTypeCamera]) { This would read better as early returns.
Created attachment 281687 [details] Patch
Comment on attachment 281655 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=281655&action=review > Source/WebCore/html/HTMLInputElement.cpp:1779 > + if (isFileUpload() && fastHasAttribute(captureAttr)) { > + if (fastGetAttribute(captureAttr).convertToASCIIUppercase() == WTF::AtomicString("FRONT")) > + return CaptureTypeFront; > + > + return CaptureTypeRear; > + } This is an inefficient way to check the value of the capture attribute for multiple reasons: 1) Calling first fastHasAttribute and then fastGetAttribute does two hash table lookups. Instead we should call just fastGetAttribute, and check isNull to see if we have an attribute or not. 2) Calling convertToASCIIUppercase allocates a copy of the capture attribute string, which is then immediately destroyed. This is inefficient. 3) Calling AtomicString("FRONT") allocates a new atomic string, which likely computes a hash, does a hash table lookup, finds there is no existing string in the atomic string table, allocates a new string, puts it in the table, then destroys it, then removes it from the atomic string table. The right way to write this is: If (!isFileUpload()) return CaptureTypeNone; auto& captureAttribute = fastGetAttribute(captureAttr); If (captureAttribute.isNull()) return CaptureTypeNone; if (equalLettersIgnoringASCIICase(captureAttribute, "front")) return CaptureTypeFront; return CaptureTypeRear; > Source/WebCore/html/HTMLInputElement.h:293 > + CaptureType getCaptureType() const; WebKit coding style prohibits the use of the word "get" in the name of a function like this one. The function should just be named captureType in the WebKit coding style. But also, maybe it needs to be MediaCaptureType and mediaCaptureType?
Created attachment 285588 [details] Patch
Comment on attachment 285588 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=285588&action=review > Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:202 > +ENABLE_MEDIA_CAPTURE[sdk=iphone*] = ENABLE_MEDIA_CAPTURE; It might make the most sense to put this alongside the other ENABLE_MEDIA_* flags which appear to be grouped closely together. > Source/WebKit/ios/WebView/WebUIKitDelegate.h:125 > -#if ENABLE(ORIENTATION_EVENTS) > +#if ENABLE_ORIENTATION_EVENTS This seems unrelated.
Comment on attachment 285588 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=285588&action=review >> Source/WebKit/ios/WebView/WebUIKitDelegate.h:125 >> +#if ENABLE_ORIENTATION_EVENTS > > This seems unrelated. I see this is the first time this is being included outside of WebKit. Interestingly, WebUIDelegatePrivate.h does some ugly hack: #if !defined(ENABLE_DASHBOARD_SUPPORT) #if !TARGET_OS_IPHONE #define ENABLE_DASHBOARD_SUPPORT 1 #else #define ENABLE_DASHBOARD_SUPPORT 0 #endif #endif What you have here is fine.
Comment on attachment 285588 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=285588&action=review >> Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:202 >> +ENABLE_MEDIA_CAPTURE[sdk=iphone*] = ENABLE_MEDIA_CAPTURE; > > It might make the most sense to put this alongside the other ENABLE_MEDIA_* flags which appear to be grouped closely together. Sure! That does make more sense.
Created attachment 285592 [details] Patch
Comment on attachment 285592 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=285592&action=review > Source/WebKit/ios/WebView/WebUIKitDelegate.h:73 > -- (void)webView:(WebView *)webView runOpenPanelForFileButtonWithResultListener:(id<WebOpenPanelResultListener>)resultListener allowMultipleFiles:(BOOL)allowMultipleFiles acceptMIMETypes:(NSArray *)mimeTypes; > +- (void)webView:(WebView *)webView runOpenPanelForFileButtonWithResultListener:(id<WebOpenPanelResultListener>)resultListener configuration:(NSDictionary *)configuration; Is it OK to change this? Isn't UIKit depending on it? > Source/WebKit/ios/WebView/WebUIKitDelegate.h:125 > -#if ENABLE(ORIENTATION_EVENTS) > +#if ENABLE_ORIENTATION_EVENTS What's up with this? Seems wrong.
Comment on attachment 285592 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=285592&action=review >> Source/WebKit/ios/WebView/WebUIKitDelegate.h:73 >> +- (void)webView:(WebView *)webView runOpenPanelForFileButtonWithResultListener:(id<WebOpenPanelResultListener>)resultListener configuration:(NSDictionary *)configuration; > > Is it OK to change this? Isn't UIKit depending on it? I have a follow up UIKit patch that changes the function call accordingly as well. Should I note this in the ChangeLogs? >> Source/WebKit/ios/WebView/WebUIKitDelegate.h:125 >> +#if ENABLE_ORIENTATION_EVENTS > > What's up with this? Seems wrong. See Joe's comment above.
Comment on attachment 285592 [details] Patch Clearing flags on attachment: 285592 Committed r204312: <http://trac.webkit.org/changeset/204312>
All reviewed patches have been landed. Closing bug.