New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 867875 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 10
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Dictionary bindings don't correctly include callback functions' headers.

Project Member Reported by mkwst@chromium.org, Jul 26

Issue description

Given a callback function like:

    callback Whatever = void (DOMString argument);

And a dictionary like:

    dictionary WhateverHolder {
        Whatever aCallbackInTheDictionary;
    };

The resulting `v8_whatever_holder.h` does not include either headers or a forward declaration for `V8WhateverCallback`. As you might expect, this explodes. :(
 
Description: Show this description
Owner: peria@chromium.org
Status: Assigned (was: Untriaged)
peria@: Could you take a look at this?
Labels: Hotlist-Bindings-IDLCompiler
Cc: yukishiino@chromium.org
Labels: -Pri-3 Pri-2
Just for my working note;
We need to..
1) include a header file, as the title describes
2) have callback_function as a Persistent handle in dict_impl
3) make ToV8 for V8CallbackFunctionBase

We can make V8CllbackFuncitonBase::CallbackFunction() public, instead of (3).
In this case, we need to update generated code to call it.
Project Member

Comment 5 by bugdroid1@chromium.org, Aug 8

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/154e570c696872ae0a56f41eb7e92dfe092f0ac0

commit 154e570c696872ae0a56f41eb7e92dfe092f0ac0
Author: Hitoshi Yoshida <peria@chromium.org>
Date: Wed Aug 08 07:41:38 2018

bindings: Implement ToV8 for IDL callback function

To support more types in ToV8().
This will be needed in defining IDL callback functions
in IDL dictoinaries.


Bug:  867875 
Change-Id: Id905ed1143899a3b455b5aca24e376e5ac50739c
Reviewed-on: https://chromium-review.googlesource.com/1166618
Commit-Queue: Hitoshi Yoshida <peria@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581496}
[modify] https://crrev.com/154e570c696872ae0a56f41eb7e92dfe092f0ac0/third_party/blink/renderer/platform/bindings/callback_function_base.h
[modify] https://crrev.com/154e570c696872ae0a56f41eb7e92dfe092f0ac0/third_party/blink/renderer/platform/bindings/to_v8.h

Project Member

Comment 6 by bugdroid1@chromium.org, Aug 10

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/287a5501cb8891da464d873f19e559484379fa9a

commit 287a5501cb8891da464d873f19e559484379fa9a
Author: Hitoshi Yoshida <peria@chromium.org>
Date: Fri Aug 10 00:51:46 2018

IDL compiler: Support IDL callback functions in IDL dictionary

We could not have IDL callback functions in IDL dictionary.
This CL makes it possible.


Bug:  867875 
Change-Id: I156407fcf3e18b6e3a7ca507771fd3f168809c8f
Reviewed-on: https://chromium-review.googlesource.com/1152336
Commit-Queue: Hitoshi Yoshida <peria@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581988}
[modify] https://crrev.com/287a5501cb8891da464d873f19e559484379fa9a/third_party/WebKit/LayoutTests/bindings/idl-dictionary-unittest-expected.txt
[modify] https://crrev.com/287a5501cb8891da464d873f19e559484379fa9a/third_party/WebKit/LayoutTests/bindings/idl-dictionary-unittest.html
[modify] https://crrev.com/287a5501cb8891da464d873f19e559484379fa9a/third_party/blink/renderer/bindings/scripts/v8_types.py
[modify] https://crrev.com/287a5501cb8891da464d873f19e559484379fa9a/third_party/blink/renderer/bindings/tests/idls/core/test_dictionary.idl
[modify] https://crrev.com/287a5501cb8891da464d873f19e559484379fa9a/third_party/blink/renderer/bindings/tests/results/core/test_dictionary.cc
[modify] https://crrev.com/287a5501cb8891da464d873f19e559484379fa9a/third_party/blink/renderer/bindings/tests/results/core/test_dictionary.h
[modify] https://crrev.com/287a5501cb8891da464d873f19e559484379fa9a/third_party/blink/renderer/bindings/tests/results/core/v8_test_dictionary.cc
[modify] https://crrev.com/287a5501cb8891da464d873f19e559484379fa9a/third_party/blink/renderer/core/testing/dictionary_test.cc
[modify] https://crrev.com/287a5501cb8891da464d873f19e559484379fa9a/third_party/blink/renderer/core/testing/dictionary_test.h
[modify] https://crrev.com/287a5501cb8891da464d873f19e559484379fa9a/third_party/blink/renderer/core/testing/internal_dictionary.idl

Cc: kabusm@google.com
Status: Fixed (was: Assigned)
This issue should be fixed.

kabusm@; if you get any trouble on this, please let me know. :)
Project Member

Comment 8 by bugdroid1@chromium.org, Aug 10

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/00e27ae10343c8db954d9ada3315e2dbbd6b32ba

commit 00e27ae10343c8db954d9ada3315e2dbbd6b32ba
Author: Maja Kabus <kabusm@google.com>
Date: Fri Aug 10 16:33:40 2018

IDL Compiler: Add include for trace_wrapper_member.h to IDL dictionaries.

Change 1152336 may generate TraceWrapperMember<..> instances but does not include the header.
(In test dictionary it is implicitly included e.g. via element.h include)

Bug:  867875 
Change-Id: I2e627826624c391d3b993b81ca6ed04f477139f6
Reviewed-on: https://chromium-review.googlesource.com/1170684
Reviewed-by: Daniel Vogelheim <vogelheim@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: Hitoshi Yoshida <peria@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Maja Kabus <kabusm@google.com>
Cr-Commit-Position: refs/heads/master@{#582199}
[modify] https://crrev.com/00e27ae10343c8db954d9ada3315e2dbbd6b32ba/third_party/blink/renderer/bindings/scripts/v8_types.py
[modify] https://crrev.com/00e27ae10343c8db954d9ada3315e2dbbd6b32ba/third_party/blink/renderer/bindings/tests/results/core/test_dictionary.h

Sign in to add a comment