Bug 158945 – Implement functionality of media capture on iOS

• Component: New Bugs
• Status: RESOLVED FIXED

[George Ruan]

Implement functionality of media capture on iOS WK2

[George Ruan]

Created attachment 281655 [details]
Patch

[Sam Weinig]

Comment on attachment 281655 [details]
Patch

Any way we can test this?

[George Ruan]

(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.

[Tim Horton]

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 :

[Joseph Pecoraro]

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.

[George Ruan]

Created attachment 281687 [details]
Patch

[Darin Adler]

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?

[George Ruan]

Created attachment 285588 [details]
Patch

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

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.

[George Ruan]

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.

[George Ruan]

Created attachment 285592 [details]
Patch

[Tim Horton]

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.

[George Ruan]

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.

[WebKit Commit Bot]

Comment on attachment 285592 [details]
Patch

Clearing flags on attachment: 285592

Committed r204312: <http://trac.webkit.org/changeset/204312>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.