Skip to content

Commit

Permalink
bindings: Add enum support for callback functions
Browse files Browse the repository at this point in the history
- Update IdlTypeBase.callback_cpp_type to handle enums.
  (This should be removed though)
- Generated call() function won't accept invalid enum values. When
  an invalid enum is passed, it will crash.

BUG=569301

Change-Id: Ie2b7aa1f314ce70e9f7c2cfec0ca995cc165bdec
Reviewed-on: https://chromium-review.googlesource.com/517587
Commit-Queue: Kenichi Ishibashi <bashi@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Hitoshi Yoshida <peria@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Cr-Commit-Position: refs/heads/master@{#475532}
  • Loading branch information
bashi authored and Commit Bot committed May 30, 2017
1 parent 43eaff9 commit cb67bd0
Show file tree
Hide file tree
Showing 20 changed files with 258 additions and 51 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,13 @@
assert_equals('9', results[2]);
}, 'Callback function which takes a number sequence');

test(function() {
var callback = function(enum_value) {
assert_equals(enum_value, 'foo');
};
callbackFunctionTest.testEnumCallback(callback, 'foo');
}, 'Callback function which takes an enum value');

test(function() {
assert_throws(new TypeError(), function() {
callbackFunctionTest.testCallback(null, 'hello', 'world');
Expand Down
2 changes: 2 additions & 0 deletions third_party/WebKit/Source/bindings/core/v8/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,8 @@ generated_core_testing_dictionary_files = [
generated_core_testing_callback_function_files = [
"$bindings_core_v8_output_dir/TestCallback.cpp",
"$bindings_core_v8_output_dir/TestCallback.h",
"$bindings_core_v8_output_dir/TestEnumCallback.cpp",
"$bindings_core_v8_output_dir/TestEnumCallback.h",
"$bindings_core_v8_output_dir/TestInterfaceCallback.cpp",
"$bindings_core_v8_output_dir/TestInterfaceCallback.h",
"$bindings_core_v8_output_dir/TestReceiverObjectCallback.cpp",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,11 +83,15 @@ def find_forward_declaration(idl_type):

def arguments_context(arguments, return_cpp_type):
def argument_context(argument):
idl_type = argument.idl_type
return {
'argument_name': '%sArgument' % argument.name,
'cpp_value_to_v8_value': argument.idl_type.cpp_value_to_v8_value(
'cpp_value_to_v8_value': idl_type.cpp_value_to_v8_value(
argument.name, isolate='script_state_->GetIsolate()',
creation_context='script_state_->GetContext()->Global()'),
'enum_type': idl_type.enum_type,
'enum_values': idl_type.enum_values,
'name': argument.name,
'v8_name': 'v8_%s' % argument.name,
}

argument_declarations = [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ def cpp_type(idl_type):
# FIXME: remove this function by making callback types consistent
# (always use usual v8_types.cpp_type)
idl_type_name = idl_type.name
if idl_type_name == 'String':
if idl_type_name == 'String' or idl_type.is_enum:
return 'const String&'
if idl_type_name == 'void':
return 'void'
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
{% from 'utilities.cpp.tmpl' import v8_value_to_local_cpp_value %}
{% from 'utilities.cpp.tmpl' import declare_enum_validation_variable, v8_value_to_local_cpp_value %}
{% filter format_blink_cpp_source_code %}

{% include 'copyright_block.txt' %}
Expand Down Expand Up @@ -35,14 +35,24 @@ bool {{cpp_class}}::call({{argument_declarations | join(', ')}}) {
if (!script_state_->ContextIsValid())
return false;

// TODO(bashi): Make sure that using DummyExceptionStateForTesting is OK.
// crbug.com/653769
DummyExceptionStateForTesting exceptionState;

{% for argument in arguments if argument.enum_values %}
{% set valid_enum_variables = 'valid_' + argument.name + '_values' %}
{{declare_enum_validation_variable(argument.enum_values, valid_enum_variables) | indent(2)}}
if (!IsValidEnum({{argument.name}}, {{valid_enum_variables}}, WTF_ARRAY_LENGTH({{valid_enum_variables}}), "{{argument.enum_type}}", exceptionState)) {
NOTREACHED();
return false;
}
{% endfor %}

ExecutionContext* context = ExecutionContext::From(script_state_.Get());
DCHECK(context);
if (context->IsContextSuspended() || context->IsContextDestroyed())
return false;

// TODO(bashi): Make sure that using DummyExceptionStateForTesting is OK.
// crbug.com/653769
DummyExceptionStateForTesting exceptionState;
ScriptState::Scope scope(script_state_.Get());
v8::Isolate* isolate = script_state_->GetIsolate();

Expand All @@ -52,10 +62,10 @@ bool {{cpp_class}}::call({{argument_declarations | join(', ')}}) {
isolate);

{% for argument in arguments %}
v8::Local<v8::Value> {{argument.argument_name}} = {{argument.cpp_value_to_v8_value}};
v8::Local<v8::Value> {{argument.v8_name}} = {{argument.cpp_value_to_v8_value}};
{% endfor %}
{% if arguments %}
v8::Local<v8::Value> argv[] = { {{arguments | join(', ', 'argument_name')}} };
v8::Local<v8::Value> argv[] = { {{arguments | join(', ', 'v8_name')}} };
{% else %}
{# Empty array initializers are illegal, and don\'t compile in MSVC. #}
v8::Local<v8::Value> *argv = nullptr;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ callback VoidCallbackFunctionInterfaceArg = void (HTMLDivElement divElement);
callback VoidCallbackFunctionDictionaryArg = void (TestDictionary arg);
callback VoidCallbackFunctionTestInterfaceSequenceArg = void (sequence<TestInterface> arg);
callback StringSequenceCallbackFunctionLongSequenceArg = sequence<DOMString> (sequence<long> arg);
callback VoidCallbackFunctionEnumArg = void (TestEnum arg);
callback VoidCallbackFunctionTypedef = void (String arg);

interface TestCallbackFunctions {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,14 +47,15 @@ bool AnyCallbackFunctionOptionalAnyArg::call(ScriptWrappable* scriptWrappable, S
if (!script_state_->ContextIsValid())
return false;

// TODO(bashi): Make sure that using DummyExceptionStateForTesting is OK.
// crbug.com/653769
DummyExceptionStateForTesting exceptionState;

ExecutionContext* context = ExecutionContext::From(script_state_.Get());
DCHECK(context);
if (context->IsContextSuspended() || context->IsContextDestroyed())
return false;

// TODO(bashi): Make sure that using DummyExceptionStateForTesting is OK.
// crbug.com/653769
DummyExceptionStateForTesting exceptionState;
ScriptState::Scope scope(script_state_.Get());
v8::Isolate* isolate = script_state_->GetIsolate();

Expand All @@ -63,8 +64,8 @@ bool AnyCallbackFunctionOptionalAnyArg::call(ScriptWrappable* scriptWrappable, S
script_state_->GetContext()->Global(),
isolate);

v8::Local<v8::Value> optionalAnyArgArgument = optionalAnyArg.V8Value();
v8::Local<v8::Value> argv[] = { optionalAnyArgArgument };
v8::Local<v8::Value> v8_optionalAnyArg = optionalAnyArg.V8Value();
v8::Local<v8::Value> argv[] = { v8_optionalAnyArg };
v8::TryCatch exceptionCatcher(isolate);
exceptionCatcher.SetVerbose(true);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,14 +47,15 @@ bool LongCallbackFunction::call(ScriptWrappable* scriptWrappable, int32_t num1,
if (!script_state_->ContextIsValid())
return false;

// TODO(bashi): Make sure that using DummyExceptionStateForTesting is OK.
// crbug.com/653769
DummyExceptionStateForTesting exceptionState;

ExecutionContext* context = ExecutionContext::From(script_state_.Get());
DCHECK(context);
if (context->IsContextSuspended() || context->IsContextDestroyed())
return false;

// TODO(bashi): Make sure that using DummyExceptionStateForTesting is OK.
// crbug.com/653769
DummyExceptionStateForTesting exceptionState;
ScriptState::Scope scope(script_state_.Get());
v8::Isolate* isolate = script_state_->GetIsolate();

Expand All @@ -63,9 +64,9 @@ bool LongCallbackFunction::call(ScriptWrappable* scriptWrappable, int32_t num1,
script_state_->GetContext()->Global(),
isolate);

v8::Local<v8::Value> num1Argument = v8::Integer::New(script_state_->GetIsolate(), num1);
v8::Local<v8::Value> num2Argument = v8::Integer::New(script_state_->GetIsolate(), num2);
v8::Local<v8::Value> argv[] = { num1Argument, num2Argument };
v8::Local<v8::Value> v8_num1 = v8::Integer::New(script_state_->GetIsolate(), num1);
v8::Local<v8::Value> v8_num2 = v8::Integer::New(script_state_->GetIsolate(), num2);
v8::Local<v8::Value> argv[] = { v8_num1, v8_num2 };
v8::TryCatch exceptionCatcher(isolate);
exceptionCatcher.SetVerbose(true);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,14 +47,15 @@ bool StringSequenceCallbackFunctionLongSequenceArg::call(ScriptWrappable* script
if (!script_state_->ContextIsValid())
return false;

// TODO(bashi): Make sure that using DummyExceptionStateForTesting is OK.
// crbug.com/653769
DummyExceptionStateForTesting exceptionState;

ExecutionContext* context = ExecutionContext::From(script_state_.Get());
DCHECK(context);
if (context->IsContextSuspended() || context->IsContextDestroyed())
return false;

// TODO(bashi): Make sure that using DummyExceptionStateForTesting is OK.
// crbug.com/653769
DummyExceptionStateForTesting exceptionState;
ScriptState::Scope scope(script_state_.Get());
v8::Isolate* isolate = script_state_->GetIsolate();

Expand All @@ -63,8 +64,8 @@ bool StringSequenceCallbackFunctionLongSequenceArg::call(ScriptWrappable* script
script_state_->GetContext()->Global(),
isolate);

v8::Local<v8::Value> argArgument = ToV8(arg, script_state_->GetContext()->Global(), script_state_->GetIsolate());
v8::Local<v8::Value> argv[] = { argArgument };
v8::Local<v8::Value> v8_arg = ToV8(arg, script_state_->GetContext()->Global(), script_state_->GetIsolate());
v8::Local<v8::Value> argv[] = { v8_arg };
v8::TryCatch exceptionCatcher(isolate);
exceptionCatcher.SetVerbose(true);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,14 +46,15 @@ bool VoidCallbackFunction::call(ScriptWrappable* scriptWrappable) {
if (!script_state_->ContextIsValid())
return false;

// TODO(bashi): Make sure that using DummyExceptionStateForTesting is OK.
// crbug.com/653769
DummyExceptionStateForTesting exceptionState;

ExecutionContext* context = ExecutionContext::From(script_state_.Get());
DCHECK(context);
if (context->IsContextSuspended() || context->IsContextDestroyed())
return false;

// TODO(bashi): Make sure that using DummyExceptionStateForTesting is OK.
// crbug.com/653769
DummyExceptionStateForTesting exceptionState;
ScriptState::Scope scope(script_state_.Get());
v8::Isolate* isolate = script_state_->GetIsolate();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,14 +47,15 @@ bool VoidCallbackFunctionDictionaryArg::call(ScriptWrappable* scriptWrappable, c
if (!script_state_->ContextIsValid())
return false;

// TODO(bashi): Make sure that using DummyExceptionStateForTesting is OK.
// crbug.com/653769
DummyExceptionStateForTesting exceptionState;

ExecutionContext* context = ExecutionContext::From(script_state_.Get());
DCHECK(context);
if (context->IsContextSuspended() || context->IsContextDestroyed())
return false;

// TODO(bashi): Make sure that using DummyExceptionStateForTesting is OK.
// crbug.com/653769
DummyExceptionStateForTesting exceptionState;
ScriptState::Scope scope(script_state_.Get());
v8::Isolate* isolate = script_state_->GetIsolate();

Expand All @@ -63,8 +64,8 @@ bool VoidCallbackFunctionDictionaryArg::call(ScriptWrappable* scriptWrappable, c
script_state_->GetContext()->Global(),
isolate);

v8::Local<v8::Value> argArgument = ToV8(arg, script_state_->GetContext()->Global(), script_state_->GetIsolate());
v8::Local<v8::Value> argv[] = { argArgument };
v8::Local<v8::Value> v8_arg = ToV8(arg, script_state_->GetContext()->Global(), script_state_->GetIsolate());
v8::Local<v8::Value> argv[] = { v8_arg };
v8::TryCatch exceptionCatcher(isolate);
exceptionCatcher.SetVerbose(true);

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
// Copyright 2014 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

// This file has been auto-generated by code_generator_v8.py.
// DO NOT MODIFY!

// This file has been generated from the Jinja2 template in
// third_party/WebKit/Source/bindings/templates/callback_function.cpp.tmpl

// clang-format off

#include "VoidCallbackFunctionEnumArg.h"

#include "bindings/core/v8/ExceptionState.h"
#include "bindings/core/v8/IDLTypes.h"
#include "bindings/core/v8/NativeValueTraitsImpl.h"
#include "bindings/core/v8/ToV8ForCore.h"
#include "bindings/core/v8/V8BindingForCore.h"
#include "core/dom/ExecutionContext.h"
#include "platform/bindings/ScriptState.h"
#include "platform/wtf/Assertions.h"

namespace blink {

// static
VoidCallbackFunctionEnumArg* VoidCallbackFunctionEnumArg::Create(ScriptState* scriptState, v8::Local<v8::Value> callback) {
if (IsUndefinedOrNull(callback))
return nullptr;
return new VoidCallbackFunctionEnumArg(scriptState, v8::Local<v8::Function>::Cast(callback));
}

VoidCallbackFunctionEnumArg::VoidCallbackFunctionEnumArg(ScriptState* scriptState, v8::Local<v8::Function> callback)
: script_state_(scriptState),
callback_(scriptState->GetIsolate(), this, callback) {
DCHECK(!callback_.IsEmpty());
}

DEFINE_TRACE_WRAPPERS(VoidCallbackFunctionEnumArg) {
visitor->TraceWrappers(callback_.Cast<v8::Value>());
}

bool VoidCallbackFunctionEnumArg::call(ScriptWrappable* scriptWrappable, const String& arg) {
if (callback_.IsEmpty())
return false;

if (!script_state_->ContextIsValid())
return false;

// TODO(bashi): Make sure that using DummyExceptionStateForTesting is OK.
// crbug.com/653769
DummyExceptionStateForTesting exceptionState;

const char* valid_arg_values[] = {
"",
"EnumValue1",
"EnumValue2",
"EnumValue3",
};
if (!IsValidEnum(arg, valid_arg_values, WTF_ARRAY_LENGTH(valid_arg_values), "TestEnum", exceptionState)) {
NOTREACHED();
return false;
}

ExecutionContext* context = ExecutionContext::From(script_state_.Get());
DCHECK(context);
if (context->IsContextSuspended() || context->IsContextDestroyed())
return false;

ScriptState::Scope scope(script_state_.Get());
v8::Isolate* isolate = script_state_->GetIsolate();

v8::Local<v8::Value> thisValue = ToV8(
scriptWrappable,
script_state_->GetContext()->Global(),
isolate);

v8::Local<v8::Value> v8_arg = V8String(script_state_->GetIsolate(), arg);
v8::Local<v8::Value> argv[] = { v8_arg };
v8::TryCatch exceptionCatcher(isolate);
exceptionCatcher.SetVerbose(true);

v8::Local<v8::Value> v8ReturnValue;
if (!V8ScriptRunner::CallFunction(callback_.NewLocal(isolate),
context,
thisValue,
1,
argv,
isolate).ToLocal(&v8ReturnValue)) {
return false;
}

return true;
}

VoidCallbackFunctionEnumArg* NativeValueTraits<VoidCallbackFunctionEnumArg>::NativeValue(v8::Isolate* isolate, v8::Local<v8::Value> value, ExceptionState& exceptionState) {
VoidCallbackFunctionEnumArg* nativeValue = VoidCallbackFunctionEnumArg::Create(ScriptState::Current(isolate), value);
if (!nativeValue) {
exceptionState.ThrowTypeError(ExceptionMessages::FailedToConvertJSValue(
"VoidCallbackFunctionEnumArg"));
}
return nativeValue;
}

} // namespace blink
Loading

0 comments on commit cb67bd0

Please sign in to comment.