Skip to content

Commit

Permalink
Remove [Custom] bindings support!
Browse files Browse the repository at this point in the history 
Change-Id: Ib0ee74c773da0c55df1fb8b70a68975bc03891b5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4807061
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Auto-Submit: Nate Chapin <japhet@chromium.org>
Commit-Queue: Nate Chapin <japhet@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1188499}
  • Loading branch information
@natechapin
natechapin authored and Chromium LUCI CQ committed Aug 25, 2023
1 parent dc25fa4 commit 0b08112
Show file tree
Hide file tree
Showing 5 changed files with 5 additions and 287 deletions.
116 changes: 2 additions & 114 deletions third_party/blink/renderer/bindings/IDLExtendedAttributes.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ Extended attributes are named in UpperCamelCase, and are conventionally written
There are a few rules in naming extended attributes:

Names should be aligned with the Web IDL spec as much as possible.
Extended attributes for custom bindings are prefixed by "Custom".
Lastly, please do not confuse "_extended_ attributes", which go inside `[...]` and modify various IDL elements, and "attributes", which are of the form `attribute foo` and are interface members.

## Special cases
Expand Down Expand Up @@ -142,9 +141,6 @@ are triggered for this method or attribute.

Usage: `[CEReactions]` takes no arguments.

`[CEReacionts]` doesn't work with `[Custom]`. Custom binding code should use
`blink::CEReactionsScope` if the method or the attribute has `[CEReactions]`.

Note that `blink::CEReactionsScope` must be constructed after `blink::ExceptionState`.

### [Clamp]
Expand Down Expand Up @@ -430,8 +426,6 @@ interface DOMWindow {
};
```

`[Replaceable]` is _incompatible_ with `[Custom]` and `[Custom=Setter]` (because replaceable attributes have a single interface-level setter callback, rather than individual attribute-level callbacks); if you need custom binding for a replaceable attribute, remove `[Replaceable]` and `readonly`.

Intuitively, "replaceable" means that you can set a new value to the attribute without overwriting the original value. If you delete the new value, then the original value still remains.

Specifically, a writable attribute, without `[Replaceable]`, behaves as follows:
Expand Down Expand Up @@ -1027,7 +1021,7 @@ When specified, caches the resulting object and returns it in later calls so tha

## Rare Blink-specific IDL Extended Attributes

These extended attributes are rarely used, generally only in one or two places. These are often replacements for `[Custom]` bindings, and may be candidates for deprecation and removal.
These extended attributes are rarely used, generally only in one or two places, and may be candidates for deprecation and removal.

### [CachedAccessor]

Expand Down Expand Up @@ -1289,112 +1283,6 @@ Summary: The byte length of buffer source types is currently restricted to be un

Consult with the bindings team before you use this extended attribute.

### [Custom]

Summary: They allow you to write bindings code manually as you like: full bindings for methods and attributes, certain functions for interfaces.

Custom bindings are _strongly discouraged_. They are likely to be buggy, a source of security holes, and represent a significant maintenance burden. Before using `[Custom]`, you should doubly consider if you really need custom bindings. You are recommended to modify code generators and add specialized extended attributes or special cases if necessary to avoid using `[Custom]`.

Usage: `[Custom]` can be specified on methods or attributes. `[Custom=Getter]` and `[Custom=Setter]` can be specified on attributes. `[Custom=A|B]` can be specified on interfaces, with various values (see below).

On read only attributes (that are not `[Replaceable]`), `[Custom]` is equivalent to `[Custom=Getter]` (since there is no setter) and `[Custom=Getter]` is preferred.

The bindings generator largely _ignores_ the specified type information of `[Custom]` members (signature of methods and type of attributes), but they must be valid types. It is best if the signature exactly matches the spec, but if one of the types is an interface that is not implemented, you can use object as the type instead, to indicate "unspecified object type".

`[Replaceable]` is _incompatible_ with `[Custom]` and `[Custom=Setter]` (because replaceable attributes have a single interface-level setter callback, rather than individual attribute-level callbacks); if you need custom binding for a replaceable attribute, remove `[Replaceable]` and readonly.

```webidl
[Custom] void func();
[Custom] attribute DOMString str1;
[Custom=Getter] readonly attribute DOMString str2;
[Custom=Setter] attribute DOMString str3;
```

Before explaining the details, let us clarify the relationship of these IDL attributes.

* `[Custom]` on a method indicates that you can write V8 custom bindings for the method.
* `[Custom=Getter]` or `[Custom=Setter]` on an attribute means custom bindings for the attribute getter or setter.
* `[Custom]` on an attribute means custom bindings for both the getter and the setter

Methods:

```webidl
interface XXX {
[Custom] void func();
};
```

You can write custom bindings in third_party/blink/renderer/bindings/{core,modules}/v8/custom/v8_xxx_custom.cc:

```c++
void V8XXX::FuncMethodCustom(const v8::FunctionCallbackInfo<v8::Value>& info) {
...;
}
```
Attribute getter:
```webidl
interface XXX {
[Custom=Getter] attribute DOMString str;
};
```

You can write custom bindings in Source/bindings/v8/custom/V8XXXCustom.cpp:

```c++
void V8XXX::StrAttributeGetterCustom(const v8::PropertyCallbackInfo<v8::Value>& info) {
...;
}
```
Attribute setter:
```webidl
interface XXX {
[Custom=Setter] attribute DOMString str;
};
```

You can write custom bindings in Source/bindings/v8/custom/V8XXXCustom.cpp:

```c++
void V8XXX::StrAttributeSetterCustom(v8::Local<v8::Value> value, const v8::PropertyCallbackInfo<v8::Value>& info) {
...;
}
```
`[Custom]` may also be specified on special operations:
```webidl
interface XXX { // anonymous special operations
[Custom] getter Node (unsigned long index);
[Custom] setter Node (unsigned long index, Node value);
[Custom] deleter boolean (unsigned long index);
[Custom] getter Node (DOMString name);
[Custom] setter Node (DOMString name, Node value);
[Custom] deleter boolean (DOMString name);
}
interface YYY { // special operations with identifiers
[Custom] getter Node item(unsigned long index);
[Custom] getter Node namedItem(DOMString name);
}
```

`[Custom]` may also be specified on callback functions:

```webidl
[Custom] callback SomeCallback = void ();
interface XXX {
void func(SomeCallback callback);
};
```

When`[Custom]` is specified on a callback function, the code generator doesn't
generate bindings for the callback function. The binding layer uses a
`ScriptValue` instead.

### [TargetOfExposed]

Summary: Interfaces specified with `[Global]` expose top-level IDL constructs specified with `[Exposed]` as JS data properties, however `[Global]` means a lot more (e.g. global object, named properties object, etc.). Interfaces specified with `[TargetOfExposed]` only expose top-level IDL constructs specified with `[Exposed]` and means nothing else.
Expand All @@ -1409,7 +1297,7 @@ These extended attributes are _deprecated_, or are under discussion for deprecat

Summary: `[PermissiveDictionaryConversion]` relaxes the rules about what types of values may be passed for an argument of dictionary type.

Ordinarily when passing in a value for a dictionary argument, the value must be either undefined, null, or an object. In other words, passing a boolean value like true or false must raise TypeError. The PermissiveDictionaryConversion extended attribute ignores non-object types, treating them the same as undefined and null. In order to effect this change, this extended attribute must be specified both on the dictionary type as well as the arguments of methods where it is passed. It exists only to eliminate certain custom bindings.
Ordinarily when passing in a value for a dictionary argument, the value must be either undefined, null, or an object. In other words, passing a boolean value like true or false must raise TypeError. The PermissiveDictionaryConversion extended attribute ignores non-object types, treating them the same as undefined and null. In order to effect this change, this extended attribute must be specified both on the dictionary type as well as the arguments of methods where it is passed.

Usage: applies to dictionaries and arguments of methods. Takes no arguments itself.

Expand Down
7 changes: 0 additions & 7 deletions third_party/blink/renderer/bindings/README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,6 @@ consider moving the class to `Source/bindings/core/` and `Source/bindings/module
The point is that we should put all the complexity around V8 APIs into
`Source/bindings/` so that the code ie kept under control of the binding team.

* `Source/bindings/core/v8/custom/` and `Source/bindings/modules/v8/custom/`
implement custom V8 bindings. Ideally all C++ bindings for Web IDL files
should be auto-generated by the IDL compiler but there are a number of bindings
that need some special care. They are written by hand as custom bindings.
The custom bindings are discouraged. The IDL compiler should be improved so that
it can auto-generate as much code by the IDL compiler as possible.

## Resources

* [IDLCompiler.md](IDLCompiler.md)
Expand Down
158 changes: 0 additions & 158 deletions third_party/blink/renderer/bindings/scripts/bind_gen/interface.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -219,28 +219,6 @@ def constant_name(cg_context):
return name_style.constant(property_name)


def custom_function_name(cg_context):
assert isinstance(cg_context, CodeGenContext)

if cg_context.named_property_getter:
return "NamedPropertyGetterCustom"
if cg_context.named_property_setter:
return "NamedPropertySetterCustom"
if cg_context.named_property_deleter:
return "NamedPropertyDeleterCustom"

if cg_context.attribute_get:
suffix = "AttributeGetterCustom"
elif cg_context.attribute_set:
suffix = "AttributeSetterCustom"
elif cg_context.operation_group:
suffix = "MethodCustom"
else:
assert False

return name_style.func(cg_context.property_.identifier, suffix)


# ----------------------------------------------------------------------------
# Callback functions
# ----------------------------------------------------------------------------
Expand Down Expand Up @@ -1891,16 +1869,6 @@ def make_attribute_get_callback_def(cg_context, function_name):
EmptyNode(),
make_check_coop_restrict_properties_access(cg_context),
EmptyNode(),
])

if "Getter" in cg_context.property_.extended_attributes.values_of(
"Custom"):
text = _format("${class_name}::{}(${info});",
custom_function_name(cg_context))
body.append(TextNode(text))
return func_def

body.extend([
make_return_value_cache_return_early(cg_context),
EmptyNode(),
make_check_security_of_return_value(cg_context),
Expand Down Expand Up @@ -1942,13 +1910,6 @@ def make_attribute_set_callback_def(cg_context, function_name):
EmptyNode(),
])

if "Setter" in cg_context.property_.extended_attributes.values_of(
"Custom"):
text = _format("${class_name}::{}(${v8_property_value}, ${info});",
custom_function_name(cg_context))
body.append(TextNode(text))
return func_def

# Binary size reduction hack
# 1. Drop the check of argument length although this is a violation of
# Web IDL.
Expand Down Expand Up @@ -2704,15 +2665,6 @@ def make_operation_function_def(cg_context, function_name):
EmptyNode(),
make_check_coop_restrict_properties_access(cg_context),
EmptyNode(),
])

if "Custom" in cg_context.property_.extended_attributes:
text = _format("${class_name}::{}(${info});",
custom_function_name(cg_context))
body.append(TextNode(text))
return func_def

body.extend([
make_no_alloc_direct_call_flush_deferred_actions(cg_context),
EmptyNode(),
make_check_argument_length(cg_context),
Expand All @@ -2735,9 +2687,6 @@ def make_operation_callback_def(cg_context, function_name):

operation_group = cg_context.operation_group

assert (not ("Custom" in operation_group.extended_attributes)
or len(operation_group) == 1)

nodes = SequenceNode()
if "NoAllocDirectCall" in operation_group.extended_attributes:
for entry in list_no_alloc_direct_call_callbacks(cg_context):
Expand Down Expand Up @@ -3234,12 +3183,6 @@ def make_named_property_getter_callback(cg_context, function_name):
bind_return_value(
body, cg_context, overriding_args=["${blink_property_name}"])

if "Custom" in cg_context.named_property_getter.extended_attributes:
text = _format("${class_name}::{}(${blink_property_name}, ${info});",
custom_function_name(cg_context))
body.append(TextNode(text))
return func_decl, func_def

# The named property getter's implementation of Blink is not designed to
# represent the property existence, and we have to determine the property
# existence by heuristics.
Expand Down Expand Up @@ -3343,14 +3286,6 @@ def make_named_property_setter_callback(cg_context, function_name):
cg_context.named_property_setter.arguments[1].idl_type,
argument=cg_context.named_property_setter.arguments[1]))

if "Custom" in cg_context.named_property_setter.extended_attributes:
text = _format(
"${class_name}::{}"
"(${blink_property_name}, ${v8_property_value}, ${info});",
custom_function_name(cg_context))
body.append(TextNode(text))
return func_decl, func_def

body.extend([
TextNode("""\
// 3.9.2. [[Set]]
Expand Down Expand Up @@ -3446,12 +3381,6 @@ def make_named_property_deleter_callback(cg_context, function_name):
bind_return_value(
body, cg_context, overriding_args=["${blink_property_name}"])

if "Custom" in cg_context.named_property_deleter.extended_attributes:
text = _format("${class_name}::{}(${blink_property_name}, ${info});",
custom_function_name(cg_context))
body.append(TextNode(text))
return func_decl, func_def

body.extend([
TextNode("""\
// 3.9.4. [[Delete]]
Expand Down Expand Up @@ -7138,86 +7067,6 @@ def generate_class_like(class_like,
constants_def.public_section.append(
make_constant_constant_def(cgc, constant_name(cgc)))

# Custom callback implementations
custom_callback_impl_decls = ListNode()

def add_custom_callback_impl_decl(**params):
arg_decls = params.pop("arg_decls")
name = params.pop("name", None)
if name is None:
name = custom_function_name(cg_context.make_copy(**params))
custom_callback_impl_decls.append(
CxxFuncDeclNode(
name=name,
arg_decls=arg_decls,
return_type="void",
static=True))

for attribute in class_like.attributes:
custom_values = attribute.extended_attributes.values_of("Custom")
is_cross_origin = "CrossOrigin" in attribute.extended_attributes
cross_origin_values = attribute.extended_attributes.values_of(
"CrossOrigin")
if "Getter" in custom_values:
add_custom_callback_impl_decl(
attribute=attribute,
attribute_get=True,
arg_decls=["const v8::FunctionCallbackInfo<v8::Value>&"])
if is_cross_origin and (not cross_origin_values
or "Getter" in cross_origin_values):
add_custom_callback_impl_decl(
attribute=attribute,
attribute_get=True,
arg_decls=["const v8::PropertyCallbackInfo<v8::Value>&"])
if "Setter" in custom_values:
add_custom_callback_impl_decl(
attribute=attribute,
attribute_set=True,
arg_decls=[
"v8::Local<v8::Value>",
"const v8::FunctionCallbackInfo<v8::Value>&",
])
if is_cross_origin and "Setter" in cross_origin_values:
add_custom_callback_impl_decl(
attribute=attribute,
attribute_set=True,
arg_decls=[
"v8::Local<v8::Value>",
"const v8::PropertyCallbackInfo<void>&",
])
for operation_group in class_like.operation_groups:
if "Custom" in operation_group.extended_attributes:
add_custom_callback_impl_decl(
operation_group=operation_group,
arg_decls=["const v8::FunctionCallbackInfo<v8::Value>&"])
if interface and interface.indexed_and_named_properties:
props = interface.indexed_and_named_properties
operation = props.own_named_getter
if operation and "Custom" in operation.extended_attributes:
add_custom_callback_impl_decl(
named_property_getter=operation,
arg_decls=[
"const AtomicString& property_name",
"const v8::PropertyCallbackInfo<v8::Value>&",
])
operation = props.own_named_setter
if operation and "Custom" in operation.extended_attributes:
add_custom_callback_impl_decl(
named_property_setter=operation,
arg_decls=[
"const AtomicString& property_name",
"v8::Local<v8::Value> v8_property_value",
"const v8::PropertyCallbackInfo<v8::Value>&",
])
operation = props.own_named_deleter
if operation and "Custom" in operation.extended_attributes:
add_custom_callback_impl_decl(
named_property_deleter=operation,
arg_decls=[
"const AtomicString& property_name",
"const v8::PropertyCallbackInfo<v8::Value>&",
])

# Cross-component trampolines
if is_cross_components:
# tp_ = trampoline name
Expand Down Expand Up @@ -7555,13 +7404,6 @@ def add_custom_callback_impl_decl(**params):
api_class_def.public_section.append(installer_function_decls)
api_class_def.public_section.append(EmptyNode())

if custom_callback_impl_decls:
api_class_def.public_section.extend([
TextNode("// Custom callback implementations"),
custom_callback_impl_decls,
EmptyNode(),
])

if indexed_and_named_property_decls:
api_class_def.public_section.extend([
TextNode("// Indexed properties and named properties"),
Expand Down

0 comments on commit 0b08112

Please sign in to comment.