Bug 148434

Summary: Distinguish Web IDL callback interfaces from Web IDL callback functions
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: BindingsAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, commit-queue, ggaren, kling, rniwa, sam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 148415, 148449    
Attachments:
Description Flags
Patch
none
Patch none

Description Chris Dumez 2015-08-25 10:02:47 PDT
Distinguish Web IDL callback interfaces [2] from Web IDL callback functions [2].

One Web Exposed difference is that when using a callback interface, the user can pass either a function / callable object or a non-callable object that has a method with the same name as the callback interface operation:
https://heycam.github.io/webidl/#es-user-objects

When using a callback function, the user needs to pass a function / callable object:
https://heycam.github.io/webidl/#es-callback-function

[1] https://heycam.github.io/webidl/#dfn-callback-interface
[2] https://heycam.github.io/webidl/#idl-callback-functions
Comment 1 Chris Dumez 2015-08-25 10:21:47 PDT
Currently, it seems our bindings code enforces that callback arguments are Functions:
EncodedJSValue JSC_HOST_CALL jsDataTransferItemPrototypeFunctionGetAsString(ExecState* exec)
{
    JSValue thisValue = exec->thisValue();
    JSDataTransferItem* castedThis = jsDynamicCast<JSDataTransferItem*>(thisValue);
    if (UNLIKELY(!castedThis))
        return throwThisTypeError(*exec, "DataTransferItem", "getAsString");
    ASSERT_GC_OBJECT_INHERITS(castedThis, JSDataTransferItem::info());
    auto& impl = castedThis->impl();
    RefPtr<StringCallback> callback;
    if (!exec->argument(0).isUndefinedOrNull()) {
        if (!exec->uncheckedArgument(0).isFunction())
            return throwArgumentMustBeFunctionError(*exec, 0, "callback", "DataTransferItem", "getAsString");
        callback = JSStringCallback::create(asObject(exec->uncheckedArgument(0)), castedThis->globalObject());
    }
    impl.getAsString(callback);
    return JSValue::encode(jsUndefined());
}

Corresponding spec: https://html.spec.whatwg.org/multipage/interaction.html#the-datatransferitem-interface

We have no distinction between callback functions and callback interfaces. We basically only support callback functions even though they are specified as callback interfaces in our IDL.
Comment 2 Chris Dumez 2015-08-25 14:40:49 PDT
Created attachment 259881 [details]
Patch
Comment 3 WebKit Commit Bot 2015-08-25 14:44:25 PDT
Attachment 259881 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestCallbackFunction.cpp:25:  You should not add a blank line before implementation file's own header.  [build/include_order] [4]
ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestCallbackFunction.cpp:83:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
ERROR: Source/WebCore/bindings/scripts/test/ObjC/DOMTestCallbackFunction.mm:33:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestCallbackFunction.h:46:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 4 in 44 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Geoffrey Garen 2015-08-25 14:52:57 PDT
Comment on attachment 259881 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=259881&action=review

> Source/WebCore/bindings/js/JSCallbackData.h:69
> +    enum class InvokeMethod { FunctionOnly, NonCallableObjectOnly, FunctionAndNonCallableObject };

enum class CallbackType { Function, Object, FunctionOrObject };
Comment 5 Chris Dumez 2015-08-25 14:57:55 PDT
Created attachment 259883 [details]
Patch
Comment 6 WebKit Commit Bot 2015-08-25 14:59:35 PDT
Attachment 259883 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestCallbackFunction.cpp:25:  You should not add a blank line before implementation file's own header.  [build/include_order] [4]
ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestCallbackFunction.cpp:83:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
ERROR: Source/WebCore/bindings/scripts/test/ObjC/DOMTestCallbackFunction.mm:33:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestCallbackFunction.h:46:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 4 in 44 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Geoffrey Garen 2015-08-26 14:23:25 PDT
Comment on attachment 259883 [details]
Patch

r=me
Comment 8 WebKit Commit Bot 2015-08-26 15:09:46 PDT
Comment on attachment 259883 [details]
Patch

Clearing flags on attachment: 259883

Committed r188994: <http://trac.webkit.org/changeset/188994>
Comment 9 WebKit Commit Bot 2015-08-26 15:09:50 PDT
All reviewed patches have been landed.  Closing bug.