New issue
Advanced search Search tips

Issue 806167 link

Starred by 1 user
Status: Fixed
Owner:
peria@chromium.org
Closed: Apr 2018
Cc:
yukishiino@chromium.org
haraken@chromium.org
bashi@chromium.org
lfg@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Feature



Sign in to add a comment

bindings: Make wrapper functions for installers

Project Member Reported by peria@chromium.org, Jan 26 2018 
Currently we have three styles to install conditionally enabled functions.

For runtime enabled features, for example;
If an interface Foo has no partial interfaces in the other component, we have V8Foo::InstallREF() and we call it directly.
If an interface Bar has partial interfaces in the other component, we have V8BarPartial::InstallREF() which calls V8Bar::InstallREF(), and V8Bar::install_runtime_enabled_feature_function points V8BarPartial::InstallREF().

As you see in above example, it is confusing and difficult to know which one (V8Foo:InstallREF or V8Bar::install_runtime_enabled_feature_function) to call.

Why don't we make a wrapper function which calles appropriate function internally?

Comment 1 by peria@chromium.org, Jan 26 2018 

We may work for member function pointers of V8Foo updated in V8Foo::UpdateWrpperTypeInfo.
 - installV8FooTemplateFunction  (this is not a method)
 - install_runtime_enabled_features_function_
 - install_runtime_enabled_features_on_template_function_
Project Member

Comment 2 by bugdroid1@chromium.org, Jan 26 2018 

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

commit 32ce3342a390e2e9e3268ea082896e76589f5ded
Author: Hitoshi Yoshida <peria@chromium.org>
Date: Fri Jan 26 22:45:22 2018

bindigns: Make InstallRuntimeEnabledFeatures as API

Before this CL,
  we used V8Foo::InstallRuntimeEnabledFeatures() if we don't have V8FooPartial, and
  we used V8Foo::install_runtime_enabled_features_function() if we have.
It was confusing and difficult to call the correct one.

This CL does
 - rename InstallRuntimeEnabledFeatures() as InstallRuntimeEnabledFeaturesImpl()
 - create a new API 'InstallRuntimeEnabledFeautres()', which has same name as before
then callsites do not need to care whether the target interface has partial interface or not.



Bug:  806167 
Change-Id: I6f277f2bb254df7cbe0fb193dd2f91c7ac1b1778
Reviewed-on: https://chromium-review.googlesource.com/888380
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Commit-Queue: Hitoshi Yoshida <peria@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532080}
[modify] https://crrev.com/32ce3342a390e2e9e3268ea082896e76589f5ded/third_party/WebKit/Source/bindings/core/v8/V8ContextSnapshot.cpp
[modify] https://crrev.com/32ce3342a390e2e9e3268ea082896e76589f5ded/third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl
[modify] https://crrev.com/32ce3342a390e2e9e3268ea082896e76589f5ded/third_party/WebKit/Source/bindings/templates/interface.h.tmpl
[modify] https://crrev.com/32ce3342a390e2e9e3268ea082896e76589f5ded/third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl
[modify] https://crrev.com/32ce3342a390e2e9e3268ea082896e76589f5ded/third_party/WebKit/Source/bindings/templates/partial_interface.cpp.tmpl
[modify] https://crrev.com/32ce3342a390e2e9e3268ea082896e76589f5ded/third_party/WebKit/Source/bindings/templates/partial_interface.h.tmpl

Comment 3 by peria@chromium.org, Apr 6 2018

Status: Fixed (was: Started)
►

Sign in to add a comment