IDL: Drop redundant null check for string and wrapper type attributes

IDL: Drop redundant null check for string and wrapper type attributes

For nullable string and wrapper type attributes, we use a null string or
null pointer return value from the C++ getter to represent a null
Javascript value.

Rather than generating a separate null check and v8SetReturnValueNull()
call for these types, we can use v8SetReturnValueStringOrNull() for
strings, and for wrapper types, we can always use the per-type
v8SetReturnValue() overload, since they convert null pointers to null
Javascript values already.

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

[Jens Widell, 2014-06-30 19:54:31]

PTAL

[haraken, 2014-07-01 00:57:33]

LGTM with comments.

https://codereview.chromium.org/357413003/diff/1/Source/bindings/scripts/v8_t...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/357413003/diff/1/Source/bindings/scripts/v8_t...
Source/bindings/scripts/v8_types.py:682: 'StringOrNull': '!{cpp_value}.isNull()
? v8String({isolate}, {cpp_value}) :
v8::Handle<v8::Value>(v8::Null({isolate}))',

Flip the order and avoid a negation.

{cpp_value}.isNull() ? v8::Handle<v8::Value>(v8::Null({isolate})) :
v8String({isolate}, {cpp_value})

Do we have a run-bindings-test for this? I couldn't find a diff caused by this
change.

https://codereview.chromium.org/357413003/diff/1/Source/bindings/scripts/v8_t...
Source/bindings/scripts/v8_types.py:683: 'StringOrUndefined':
'!{cpp_value}.isNull() ? v8String({isolate}, {cpp_value}) : v8Undefined()',

Ditto.

BTW, who needs StringOrUndefined? The behavior sounds a bit strange in terms of
the spec.

[Jens Widell, 2014-07-01 06:43:23]

https://codereview.chromium.org/357413003/diff/1/Source/bindings/scripts/v8_a...
File Source/bindings/scripts/v8_attributes.py (right):

https://codereview.chromium.org/357413003/diff/1/Source/bindings/scripts/v8_a...
Source/bindings/scripts/v8_attributes.py:97: 'cpp_value_to_v8_value':
idl_type.cpp_value_to_v8_value(cpp_value='original',
creation_context='info.Holder()'),
Dropped this because it's overridden by getter_context() anyway, with the right
cpp_value ('cppValue' instead of 'original').

It wouldn't be overridden for an attribute with a custom getter, but I think
it's a safe to assume that we won't need this in a setter for an attribute with
a custom getter. (This expression is essentially a low-level getter call most of
the time, and it seems wrong to generate that code for an attribute with a
custom getter anyway.)

https://codereview.chromium.org/357413003/diff/1/Source/bindings/scripts/v8_t...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/357413003/diff/1/Source/bindings/scripts/v8_t...
Source/bindings/scripts/v8_types.py:682: 'StringOrNull': '!{cpp_value}.isNull()
? v8String({isolate}, {cpp_value}) :
v8::Handle<v8::Value>(v8::Null({isolate}))',
On 2014/07/01 00:57:33, haraken wrote:
> 
> Flip the order and avoid a negation.
> 
> {cpp_value}.isNull() ? v8::Handle<v8::Value>(v8::Null({isolate})) :
> v8String({isolate}, {cpp_value})
> 
> Do we have a run-bindings-test for this? I couldn't find a diff caused by this
> change.

Done.

https://codereview.chromium.org/357413003/diff/1/Source/bindings/scripts/v8_t...
Source/bindings/scripts/v8_types.py:683: 'StringOrUndefined':
'!{cpp_value}.isNull() ? v8String({isolate}, {cpp_value}) : v8Undefined()',
On 2014/07/01 00:57:33, haraken wrote:
> 
> Ditto.
> 
> BTW, who needs StringOrUndefined? The behavior sounds a bit strange in terms
of
> the spec.

A bit strange, yes. Kicks in with [TreatReturnedNullStringAs=Undefined], which
is used in a few places in real IDL files. Somewhat unclear if any of them
actually ever return a null string, so this may be dead code.

These added code snippets don't end up anywhere in generated code with current
IDL files, but the dictionary items are always accessed to calculate (unused)
attribute context values, so not having them raises KeyError.

I've added tests for the cases where they would be used; DOMString? or
[TreatReturnedNullStringAs] combined with [CachedAttribute] or
[LogPreviousValue].

https://codereview.chromium.org/357413003/diff/20001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_attributes.py (right):

https://codereview.chromium.org/357413003/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_attributes.py:175: 'LogPreviousValue' in
extended_attributes or
Added this to avoid the [LogPreviousValue] code calling the C++ getter multiple
times. It'll lead to redundant use of a local variable in the corresponding
getter instead, but that's fine I think.

Attributes with [LogPreviousValue] are rare anyway.

https://codereview.chromium.org/357413003/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_attributes.py:194:
extended_attributes=extended_attributes),
Passing 'extended_attributes' here since it affects
v8_types.v8_conversion_type().

[haraken, 2014-07-01 06:47:17]

Still LGTM

https://codereview.chromium.org/357413003/diff/20001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_attributes.py (right):

https://codereview.chromium.org/357413003/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_attributes.py:175: 'LogPreviousValue' in
extended_attributes or
On 2014/07/01 06:43:22, Jens Lindström wrote:
> Added this to avoid the [LogPreviousValue] code calling the C++ getter
multiple
> times. It'll lead to redundant use of a local variable in the corresponding
> getter instead, but that's fine I think.
> 
> Attributes with [LogPreviousValue] are rare anyway.

Yes, we're deprecating it with pmarch@.

[Jens Widell, 2014-07-01 06:50:39]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2014-07-01 06:52:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jl@opera.com/357413003/20001

[commit-bot: I haz the power, 2014-07-01 07:58:00]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)

[commit-bot: I haz the power, 2014-07-01 09:05:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-01 09:05:04]

Try jobs failed on following builders:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)

[Jens Widell, 2014-07-01 09:39:11]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2014-07-01 09:39:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jl@opera.com/357413003/20001

[commit-bot: I haz the power, 2014-07-01 10:43:21]

Change committed as 177292