IDL: Use Nullable for union type return value

IDL: Use Nullable for union type return value

Before this CL, we use bool flags to indicate which value should be
returned. We can use Nullable<T> instead.

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

[bashi, 2014-08-13 10:03:02]

Haraken-san,

What do you think about this change? As Shiino-san suggested, we can also use an
index output argument. It looks ok to me, but Nullable<RefPtr<T> > might look a
bit strange.

Also, we might want to do refactoring before moving forward.

[Jens Widell, 2014-08-13 10:34:32]

FYI, I have an experimental implementation of union return types (and also,
intended, union arguments) based on Union2<T1, T2> / Union3<T1, T2, T3> / ...
templates. That approach allowed for more of the complexity and special handling
on the code generator side to be eliminated, but instead we get more template
magic.

I wasn't totally happy about how it turned out, so didn't pursue at the time.

Uploaded here if anyone want to see: https://codereview.chromium.org/465303002/

(It's a bit old so might need some work to rebase cleanly onto ToT.)

[haraken, 2014-08-13 10:41:44]

On 2014/08/13 10:34:32, Jens Lindström wrote:
> FYI, I have an experimental implementation of union return types (and also,
> intended, union arguments) based on Union2<T1, T2> / Union3<T1, T2, T3> / ...
> templates. That approach allowed for more of the complexity and special
handling
> on the code generator side to be eliminated, but instead we get more template
> magic.
> 
> I wasn't totally happy about how it turned out, so didn't pursue at the time.
> 
> Uploaded here if anyone want to see:
https://codereview.chromium.org/465303002/
> 
> (It's a bit old so might need some work to rebase cleanly onto ToT.)

Thanks for the info Jens! We (bashi@, tasak@ and me) also discussed the Union<>
approach and are not happy with the approach either :)

[bashi, 2014-08-13 10:57:16]

On 2014/08/13 10:41:44, haraken wrote:
> On 2014/08/13 10:34:32, Jens Lindström wrote:
> > FYI, I have an experimental implementation of union return types (and also,
> > intended, union arguments) based on Union2<T1, T2> / Union3<T1, T2, T3> /
...
> > templates. That approach allowed for more of the complexity and special
> handling
> > on the code generator side to be eliminated, but instead we get more
template
> > magic.
> > 
> > I wasn't totally happy about how it turned out, so didn't pursue at the
time.
> > 
> > Uploaded here if anyone want to see:
> https://codereview.chromium.org/465303002/
> > 
> > (It's a bit old so might need some work to rebase cleanly onto ToT.)
> 
> Thanks for the info Jens! We (bashi@, tasak@ and me) also discussed the
Union<>
> approach and are not happy with the approach either :)

Thanks Jens!

Yes, we discussed approaches and our current plan is to use Nullable<>. I'm
going to write an WIP CL, and may revisit the design if it doesn't work well.

[haraken, 2014-08-13 13:07:10]

LGTM

https://codereview.chromium.org/466323002/diff/1/Source/bindings/core/v8/Null...
File Source/bindings/core/v8/Nullable.h (right):

https://codereview.chromium.org/466323002/diff/1/Source/bindings/core/v8/Null...
Source/bindings/core/v8/Nullable.h:21: Nullable(const T& value)

Probably add 'explicit' ?

https://codereview.chromium.org/466323002/diff/1/Source/bindings/core/v8/Null...
Source/bindings/core/v8/Nullable.h:45: operator bool() const { return !m_isNull
&& m_value; }

In order to allow implicit conversion to bool but prevent implicit conversion to
other integer types, can we use the "UnspecifiedBoolType trick" in RefPtr.h?

[Jens Widell, 2014-08-13 13:11:08]

https://codereview.chromium.org/466323002/diff/1/Source/bindings/core/v8/Null...
File Source/bindings/core/v8/Nullable.h (right):

https://codereview.chromium.org/466323002/diff/1/Source/bindings/core/v8/Null...
Source/bindings/core/v8/Nullable.h:21: Nullable(const T& value)
On 2014/08/13 13:07:10, haraken wrote:
> 
> Probably add 'explicit' ?

That would require changes to for instance
WebGLRenderingContextBase::getSupportedExtensions(), which now returns a
Vector<String> value directly in the non-null case but whose return type is
Nullable<Vector<String> >.

[haraken, 2014-08-13 13:12:54]

https://codereview.chromium.org/466323002/diff/1/Source/bindings/core/v8/Null...
File Source/bindings/core/v8/Nullable.h (right):

https://codereview.chromium.org/466323002/diff/1/Source/bindings/core/v8/Null...
Source/bindings/core/v8/Nullable.h:21: Nullable(const T& value)
On 2014/08/13 13:11:08, Jens Lindström wrote:
> On 2014/08/13 13:07:10, haraken wrote:
> > 
> > Probably add 'explicit' ?
> 
> That would require changes to for instance
> WebGLRenderingContextBase::getSupportedExtensions(), which now returns a
> Vector<String> value directly in the non-null case but whose return type is
> Nullable<Vector<String> >.

Makes sense.

[bashi, 2014-08-13 13:45:56]

Thank you for review, Haraken-san!

We should avoid using Nullable<String> so I'll revise the CL not to use
Nullable<T> if T is String (maybe Friday).

https://codereview.chromium.org/466323002/diff/1/Source/bindings/core/v8/Null...
File Source/bindings/core/v8/Nullable.h (right):

https://codereview.chromium.org/466323002/diff/1/Source/bindings/core/v8/Null...
Source/bindings/core/v8/Nullable.h:45: operator bool() const { return !m_isNull
&& m_value; }
On 2014/08/13 13:07:10, haraken wrote:
> 
> In order to allow implicit conversion to bool but prevent implicit conversion
to
> other integer types, can we use the "UnspecifiedBoolType trick" in RefPtr.h?

Will do.

[bashi, 2014-08-15 02:43:53]

Haraken-san, (and Jens),

Could you take another look? Added special handling for String, but we might
also want to avoid wrapping *RefPtr in Nullable. I personally feel ok to wrap
them, but I don't have strong opinion here. WDYT?

https://codereview.chromium.org/466323002/diff/1/Source/bindings/core/v8/Null...
File Source/bindings/core/v8/Nullable.h (right):

https://codereview.chromium.org/466323002/diff/1/Source/bindings/core/v8/Null...
Source/bindings/core/v8/Nullable.h:45: operator bool() const { return !m_isNull
&& m_value; }
On 2014/08/13 13:45:56, bashi1 wrote:
> On 2014/08/13 13:07:10, haraken wrote:
> > 
> > In order to allow implicit conversion to bool but prevent implicit
conversion
> to
> > other integer types, can we use the "UnspecifiedBoolType trick" in RefPtr.h?
> 
> Will do.

It turns out we need a meta-function or need to rely on SFINAE to make this
workable for *Ptr family. Since there is no call site at this point, I'd rather
remove this. We can re-add it when we need it.

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

https://codereview.chromium.org/466323002/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_types.py:214: cpp_types = []
Use list (not set) to keep the order of members.

https://codereview.chromium.org/466323002/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_types.py:735: if member_type.is_string_type:
or cpp_type_has_null_value?

[haraken, 2014-08-15 02:52:47]

> Could you take another look? Added special handling for String, but we might
> also want to avoid wrapping *RefPtr in Nullable. I personally feel ok to wrap
> them, but I don't have strong opinion here. WDYT?

I'd prefer not wrapping RefPtr<> with Nullable, because RefPtr<> can use nullptr
to represent Nullable. I guess whether we should use Nullable for RefPtr<> or
not wouldn't be a small issue since RefPtr<> is used for all DOM objects.

[bashi, 2014-08-15 02:59:55]

On 2014/08/15 02:52:47, haraken wrote:
> > Could you take another look? Added special handling for String, but we might
> > also want to avoid wrapping *RefPtr in Nullable. I personally feel ok to
wrap
> > them, but I don't have strong opinion here. WDYT?
> 
> I'd prefer not wrapping RefPtr<> with Nullable, because RefPtr<> can use
nullptr
> to represent Nullable. I guess whether we should use Nullable for RefPtr<> or
> not wouldn't be a small issue since RefPtr<> is used for all DOM objects.

Thanks for the feedback. As chatted offline, I'll revise the CL not to wrap
*Ptr.

[bashi, 2014-08-15 07:13:39]

Haraken-san, PTAL?

I introduced a new context for union member arguments to handle following cases:
- if a type of union member is String
-- don't wrap it in Nullable, nullcheck should be '!resultX.isNull()', return
value should be 'resultX'
- if a type of union member is *Ptr family
-- don't wrap it in Nullable, nullcheck should be '!resultX', return value
should be 'resultX.release()'
- otherwise
-- wrap it in Nullable, nullcheck should be '!resultX', return value should be
'resultX.get()'

It's very difficult to handle all of above with the current approach, i.e.,
'cpp_type' and 'v8_set_return_value' are lists when method's idl type is an
union type. Ideally, we need to re-design how to support union types, but I'd
like to start from this CL.

https://codereview.chromium.org/466323002/diff/40001/Source/core/html/HTMLAll...
File Source/core/html/HTMLAllCollection.cpp (left):

https://codereview.chromium.org/466323002/diff/40001/Source/core/html/HTMLAll...
Source/core/html/HTMLAllCollection.cpp:79: returnValue1 = namedItems.at(0);
Will re-add accidentally removed space.

[haraken, 2014-08-15 07:34:16]

LGTM

> It's very difficult to handle all of above with the current approach, i.e.,
> 'cpp_type' and 'v8_set_return_value' are lists when method's idl type is an
> union type. Ideally, we need to re-design how to support union types, but I'd
> like to start from this CL.

Yeah. We'll need to add .isNull, .release(), operator bool etc to String,
Nullable, *Ptr etc to make the difference among types as small as possible.

https://codereview.chromium.org/466323002/diff/40001/Source/bindings/tests/id...
File Source/bindings/tests/idls/TestTypedefs.idl (right):

https://codereview.chromium.org/466323002/diff/40001/Source/bindings/tests/id...
Source/bindings/tests/idls/TestTypedefs.idl:44: typedef (DOMString or double)
DOMStringOrDouble;

Consider adding more test cases about union types in a follow-up CL. The current
test coverage for union types is low.

https://codereview.chromium.org/466323002/diff/40001/Source/bindings/tests/re...
File Source/bindings/tests/results/V8TestSpecialOperations.cpp (right):

https://codereview.chromium.org/466323002/diff/40001/Source/bindings/tests/re...
Source/bindings/tests/results/V8TestSpecialOperations.cpp:100:
v8SetReturnValueNull(info);

Is this line reachable? If it's always unreachable, we want to add
ASSERT_NOT_REACHED().

[bashi, 2014-08-15 10:14:07]

https://codereview.chromium.org/466323002/diff/40001/Source/bindings/tests/id...
File Source/bindings/tests/idls/TestTypedefs.idl (right):

https://codereview.chromium.org/466323002/diff/40001/Source/bindings/tests/id...
Source/bindings/tests/idls/TestTypedefs.idl:44: typedef (DOMString or double)
DOMStringOrDouble;
On 2014/08/15 07:34:15, haraken wrote:
> 
> Consider adding more test cases about union types in a follow-up CL. The
current
> test coverage for union types is low.

Yes. We should add more test cases. I'll add more tests in follow-up CLs.

https://codereview.chromium.org/466323002/diff/40001/Source/bindings/tests/re...
File Source/bindings/tests/results/V8TestSpecialOperations.cpp (right):

https://codereview.chromium.org/466323002/diff/40001/Source/bindings/tests/re...
Source/bindings/tests/results/V8TestSpecialOperations.cpp:100:
v8SetReturnValueNull(info);
On 2014/08/15 07:34:15, haraken wrote:
> 
> Is this line reachable? If it's always unreachable, we want to add
> ASSERT_NOT_REACHED().
> 

Done.

[bashi, 2014-08-15 12:52:52]

The CQ bit was checked by bashi@chromium.org

[commit-bot: I haz the power, 2014-08-15 12:53:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/bashi@chromium.org/466323002/60001

[commit-bot: I haz the power, 2014-08-15 12:57:17]

Committed patchset #4 (60001) as 180360

[sof, 2014-08-15 19:16:21]

This appears to break two unit tests with Oilpan enabled,

 
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan%...

Crashes gone with it reverted.

[Jens Widell, 2014-08-15 19:47:21]

On 2014/08/15 19:16:21, sof wrote:
> This appears to break two unit tests with Oilpan enabled,
> 
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan%...
> 
> Crashes gone with it reverted.

sof@: can you check if https://codereview.chromium.org/474323002 fixes the
crashes?

[sof, 2014-08-15 19:49:15]

On 2014/08/15 19:47:21, Jens Widell wrote:
> On 2014/08/15 19:16:21, sof wrote:
> > This appears to break two unit tests with Oilpan enabled,
> > 
> >
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan%...
> > 
> > Crashes gone with it reverted.
> 
> sof@: can you check if https://codereview.chromium.org/474323002 fixes the
> crashes?

As long as it initializes the RawPtrs will nullptr, it will.

[Jens Widell, 2014-08-15 19:51:45]

On 2014/08/15 19:49:15, sof wrote:
> On 2014/08/15 19:47:21, Jens Widell wrote:
> > On 2014/08/15 19:16:21, sof wrote:
> > > This appears to break two unit tests with Oilpan enabled,
> > > 
> > >
> >
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan%...
> > > 
> > > Crashes gone with it reverted.
> > 
> > sof@: can you check if https://codereview.chromium.org/474323002 fixes the
> > crashes?
> 
> As long as it initializes the RawPtrs will nullptr, it will.

That was my theory too. :-)