Bug 93072 – V8Proxy::retrieve(*) leads to really obfuscated code and should be removed

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

V8Proxy::retrieve(*) leads to really obfuscated code and should be removed

[Adam Barth]

Created attachment 156273 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 156273 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156273&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3324
> +        frame->script()->windowShell()->initContextIfNeeded();

I guess if we're running script in the first place, then script() is never null...

> Source/WebCore/bindings/scripts/test/V8/V8TestNode.cpp:115
> +    V8Proxy* proxy = impl->document()->frame() ? impl->document()->frame()->script()->proxy() : 0;

This seems like a really long idiom... Maybe impl should have an accessor which can return script() for you?

[Adam Barth]

(In reply to comment #2)
> (From update of attachment 156273 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156273&action=review
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3324
> > +        frame->script()->windowShell()->initContextIfNeeded();
> 
> I guess if we're running script in the first place, then script() is never null...

script() is no longer ever null.  It's the object you ask if script is disabled.

> > Source/WebCore/bindings/scripts/test/V8/V8TestNode.cpp:115
> > +    V8Proxy* proxy = impl->document()->frame() ? impl->document()->frame()->script()->proxy() : 0;
> 
> This seems like a really long idiom... Maybe impl should have an accessor which can return script() for you?

Yeah, this is generated code, so I'm not that worried about it.  I'd like to get rid of V8Proxy entirely, we will make it shorter.

[Adam Barth]

Created attachment 156275 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 156275 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156275&action=review

OK.

> Source/WebCore/bindings/v8/V8LazyEventListener.cpp:91
> +        return v8::Local<v8::Value>();

Super ugly.

[Adam Barth]

> > Source/WebCore/bindings/v8/V8LazyEventListener.cpp:91
> > +        return v8::Local<v8::Value>();
> 
> Super ugly.

We can make it v8::Undefined() if you like that better.

[Kentaro Hara]

(In reply to comment #6)
> > > Source/WebCore/bindings/v8/V8LazyEventListener.cpp:91
> > > +        return v8::Local<v8::Value>();
> > 
> > Super ugly.
> 
> We can make it v8::Undefined() if you like that better.

Now the V8 team made v8::Handle<v8::Value>() as fast as v8::Undefined(Isolate). So we can use v8::Handle<v8::Value>() everywhere regardless of Isolate. I'm planning to implement

    v8::Handle<v8::Value> v8Undefined() { return v8::Handle<v8::Value>(); }

in V8Binding.cpp and use v8Undefined() everywhere in V8 bindings.

[Adam Barth]

Comment on attachment 156275 [details]
Patch

Ok.  Sounds like we should land this with the ugly syntax and clean it up with the rest of them.

[WebKit Review Bot]

Comment on attachment 156275 [details]
Patch

Clearing flags on attachment: 156275

Committed r124583: <http://trac.webkit.org/changeset/124583>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.