[bindings] Support passing of ScriptState for namedPropertyQuery form of getter.

[bindings] Support passing of ScriptState for namedPropertyQuery form of getter.

Pass the ScriptState for the namedPropertyQuery form of the getter.

R=haraken@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=194699

[vivekg, 2015-04-29 08:46:52]

vivekg@chromium.org changed reviewers:
+ haraken@chromium.org, vivekg@chromium.org

[vivekg, 2015-04-29 08:46:52]

PTAL, thanks!

[haraken, 2015-04-29 15:53:46]

The approach looks good but this CL is doing multiple non-trivial things in one
go. Can we split the CL into pieces?

- Support ScriptState* in named getters.
- Replace the named getter.
- Replace the named setter.
- Replace the named enumelator.
- Replace the named query.

https://codereview.chromium.org/1109323002/diff/1/Source/bindings/core/v8/cus...
File Source/bindings/core/v8/custom/V8CSSStyleDeclarationCustom.cpp (left):

https://codereview.chromium.org/1109323002/diff/1/Source/bindings/core/v8/cus...
Source/bindings/core/v8/custom/V8CSSStyleDeclarationCustom.cpp:138: // Also,
certain prefixes such as 'css-' are stripped.

Keep this comment.

https://codereview.chromium.org/1109323002/diff/1/Source/bindings/templates/i...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/1109323002/diff/1/Source/bindings/templates/i...
Source/bindings/templates/interface.cpp:320: {% endif %}

{% set getter_arguments = ['propertyName', 'exceptionState'] %}
{% if getter.is_call_with_script_state %}
ScriptState* scriptState = ScriptState::current(info.GetIsolate());
{% set getter_arguments = ['scriptState'] + getter_arguments %}
{% endif %}

https://codereview.chromium.org/1109323002/diff/1/Source/bindings/templates/i...
Source/bindings/templates/interface.cpp:369: {% if
deleter.is_call_with_script_state %}

Ditto. (Not related to your CL though.)

https://codereview.chromium.org/1109323002/diff/1/Source/core/css/CSSStyleDec...
File Source/core/css/CSSStyleDeclaration.cpp (right):

https://codereview.chromium.org/1109323002/diff/1/Source/core/css/CSSStyleDec...
Source/core/css/CSSStyleDeclaration.cpp:118:
result.setString(cssValue->cssText());

The previous code was using v8SetReturnValueStringOrNull, not
v8SetReturnValueString. Does the setString in the new code keep the behavior?

https://codereview.chromium.org/1109323002/diff/1/Source/core/css/CSSStyleDec...
Source/core/css/CSSStyleDeclaration.cpp:131:
setPropertyInternal(unresolvedProperty, propertyValue, false, exceptionState);

Don't we need:

  if (exceptionState.throwIfNeeded())
    return false;

?

https://codereview.chromium.org/1109323002/diff/1/Source/core/css/CSSStyleDec...
Source/core/css/CSSStyleDeclaration.cpp:137: if (!name.length())

Did we have this check in the previous code?

https://codereview.chromium.org/1109323002/diff/1/Source/core/css/CSSStyleDec...
Source/core/css/CSSStyleDeclaration.cpp:141: return cssPropertyInfo(name,
scriptState->isolate());

In the previous code, we're doing v8SetReturnValueInt(info, 0). Does the new
code keep the behavior?

https://codereview.chromium.org/1109323002/diff/1/Source/core/css/CSSStyleDec...
Source/core/css/CSSStyleDeclaration.cpp:167: }

// namespace blink

https://codereview.chromium.org/1109323002/diff/1/Source/core/css/CSSStyleDec...
File Source/core/css/CSSStyleDeclaration.idl (left):

https://codereview.chromium.org/1109323002/diff/1/Source/core/css/CSSStyleDec...
Source/core/css/CSSStyleDeclaration.idl:47:
[Custom=(PropertyGetter,PropertyEnumerator,PropertyQuery)] getter (DOMString or
float) (DOMString name);

Can we drop these Custom=X from run-bindings-tests and
IDLExtendedAttributes.txt?

[vivekg, 2015-04-30 03:59:29]

On 2015/04/29 at 15:53:46, haraken wrote:
> The approach looks good but this CL is doing multiple non-trivial things in
one go. Can we split the CL into pieces?
> 

Thank you haraken@, I wanted to get a preliminary review on this before I split
it further.

> - Support ScriptState* in named getters.
> - Replace the named getter.
> - Replace the named setter.
> - Replace the named enumelator.
> - Replace the named query.

Sure, will do the same.

In this CL, we are adding the support of passing ScriptState to the
namedPropertyQuery method. PTAL, thanks.

[haraken, 2015-04-30 04:00:29]

LGTM

[vivekg, 2015-04-30 04:00:54]

The CQ bit was checked by vivekg@chromium.org

[commit-bot: I haz the power, 2015-04-30 04:01:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1109323002/20001

[commit-bot: I haz the power, 2015-04-30 05:07:29]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=194699

[Jens Widell, 2015-04-30 13:14:12]

jl@opera.com changed reviewers:
+ jl@opera.com

[Jens Widell, 2015-04-30 13:14:13]

https://codereview.chromium.org/1109323002/diff/1/Source/core/css/CSSStyleDec...
File Source/core/css/CSSStyleDeclaration.cpp (right):

https://codereview.chromium.org/1109323002/diff/1/Source/core/css/CSSStyleDec...
Source/core/css/CSSStyleDeclaration.cpp:131:
setPropertyInternal(unresolvedProperty, propertyValue, false, exceptionState);
On 2015/04/29 15:53:45, haraken wrote:
> 
> Don't we need:
> 
>   if (exceptionState.throwIfNeeded())
>     return false;
> 
> ?

We do not. It's the outermost layer's job to actually throw; i.e. the generated
code that calls this function.

If necessary, this function could check 'exceptionState.hadException()' to bail
out early, but it's not necessary in this case since the generated code detects
an exception regardless of what this function returns, and since this function
does nothing more after this and thus has no need to bail out early.