IDL compiler: [RaisesException] interface type attribute

IDL compiler: [RaisesException] interface type attribute

Interface attributes that [RaiseException] (on getter)
(also nullable attributes) have a slight wrinkle:
we may need to call .release() on jsValue when calling v8SetReturn*.
This changes the *r*value of |cpp_value|, but not the lvalue
(we don't call |release| on assignment!), so fix by introducing an
local auxiliary |cpp_rvalue| variable in the Python code for the getter.

BUG=239771
R=haraken
NOTRY=true

[haraken, 2013-12-17 08:35:41]

I wonder why we need to call .release() only for constructors that have
[RaisesException]. Can't we always call .release()?

[Nils Barth (inactive), 2013-12-18 06:27:53]

On 2013/12/17 08:35:41, haraken wrote:
> I wonder why we need to call .release() only for constructors that have
> [RaisesException]. Can't we always call .release()?

The only other case of |cpp_value = 'jsValue'| for an interface type is
EventHandler,
in which case jsValue is of type EventListener*, where we can't use .release().

I've rewritten the patch to make the logic clearer and flatter (3 cases, all
together);
original patch was short but a bit cryptic.

[Nils Barth (inactive), 2013-12-18 06:30:40]

PTAL
(revised logic to clarify)

[haraken, 2013-12-18 06:57:45]

On 2013/12/18 06:27:53, Nils Barth wrote:
> On 2013/12/17 08:35:41, haraken wrote:
> > I wonder why we need to call .release() only for constructors that have
> > [RaisesException]. Can't we always call .release()?
> 
> The only other case of |cpp_value = 'jsValue'| for an interface type is
> EventHandler,
> in which case jsValue is of type EventListener*, where we can't use
.release().
> 
> I've rewritten the patch to make the logic clearer and flatter (3 cases, all
> together);
> original patch was short but a bit cryptic.

It looks strange that EventHandler and CachedAttribute have to be treated
specially. Can't you rewrite code so that they use RefPtrs?

Also these values are RefPtrs, so it shouldn't be called "jsValue".

[Nils Barth (inactive), 2013-12-19 02:59:48]

On 2013/12/18 06:57:45, haraken wrote:
> It looks strange that EventHandler and CachedAttribute have to be treated
> specially.

Turns out only EventHandler needs to be treated specially, as usual.
(Uploaded patch with only EventHandler special-cased.)

> Can't you rewrite code so that they use RefPtrs?
> Also these values are RefPtrs, so it shouldn't be called "jsValue".

Maybe for EventHandler we should just rename this to "eventListener", as that's
what it is?
(It used to call it "listener")

The current output code is (line breaks added):
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

EventListener* jsValue =
imp->eventHandlerAttribute(isolatedWorldForIsolate(info.GetIsolate()));
v8SetReturnValue(info, jsValue ?
   
v8::Handle<v8::Value>(V8AbstractEventListener::cast(jsValue)->getListenerObject(imp->executionContext()))
  : v8::Handle<v8::Value>(v8::Null(info.GetIsolate())));

Should this code be changed?
Otherwise perhaps just renaming makes it correct?

[haraken, 2013-12-19 03:11:02]

> EventListener* jsValue =
> imp->eventHandlerAttribute(isolatedWorldForIsolate(info.GetIsolate()));
> v8SetReturnValue(info, jsValue ?
>    
>
v8::Handle<v8::Value>(V8AbstractEventListener::cast(jsValue)->getListenerObject(imp->executionContext()))
>   : v8::Handle<v8::Value>(v8::Null(info.GetIsolate())));
> 
> Should this code be changed?
> Otherwise perhaps just renaming makes it correct?

I think we should change the C++ code so that it returns RefPtr<EventListener>,
since other DOM objects are doing that.

[Nils Barth (inactive), 2013-12-19 08:16:49]

On 2013/12/19 03:11:02, haraken wrote:
> > EventListener* jsValue =
> > imp->eventHandlerAttribute(isolatedWorldForIsolate(info.GetIsolate()));
> > v8SetReturnValue(info, jsValue ?
> >    
> >
>
v8::Handle<v8::Value>(V8AbstractEventListener::cast(jsValue)->getListenerObject(imp->executionContext()))
> >   : v8::Handle<v8::Value>(v8::Null(info.GetIsolate())));
> > 
> > Should this code be changed?
> > Otherwise perhaps just renaming makes it correct?
> 
> I think we should change the C++ code so that it returns
RefPtr<EventListener>,
> since other DOM objects are doing that.

Opened separate CL for this:
EventHandler getters return RefPtr<EventListener> (not v8::Handle<v8::Value>) 
https://codereview.chromium.org/115873003/

[Nils Barth (inactive), 2014-03-20 03:42:16]

Ended up being implemented more simply in a later CL:
IDL compiler: [RaisesException] on interface type attributes
https://codereview.chromium.org/131453010/

Simpler implementation was b/c we do need .release() various places, and I'd
added a flag for this, so it was just a matter of setting the flag.