Bug 32638 – [V8] V8CustomBinding.h should vanish.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Dimitri Glazkov (Google)]

It's just a long list of all custom callback declarations. These should just live in the corresponding generated headers.

Net gain of not having to touch one extra place when custom methods are added to IDLs is an epic win.

[Dimitri Glazkov (Google)]

This may be not-so-trivial, because we'll have to break the V8Custom class up, but I think the reason behind this class' existence is bogus, so I'd say it's doable.

[Nate Chapin]

Created attachment 45106 [details]
Generate header declarations for non-constructor callbacks

I didn't immediately see a good way to generate declarations for constructors, so I left them in V8CustomBinding.h.  All the other callbacks are now declared in their respective headers.

This patch is huge, but it's mostly renaming.

[Adam Barth]

Comment on attachment 45106 [details]
Generate header declarations for non-constructor callbacks

No ChangeLog.  Also, this patch does not download and apply to TOT:

http://webkit-commit-queue.appspot.com/patch-status/chromium-ews/45106

Detailed code comments to follow.

[Adam Barth]

+ static v8::Handle<v8::Value> ${name}Callback(const v8::Arguments&);

Is this adding static declarations to a header file?  Maybe this is part of a class?  But the indent is only two spaces...  Can you attach a sample generated file?

+ class V8HTMLPlugInElement {
+   public:

The public should be indented zero spaces.  You can catch this kind of stuff by running check-webkit-style.  (The stylebot would have caught it if the patch applied to TOT).

+ ACCESSOR_GETTER(ClipboardTypes)

I don't understand the diff for this function.

- for (HashSet<String>::const_iterator it = types.begin(); it != end; ++it, ++index)
+ for (HashSet<String>::const_iterator it = types.begin(); it != end; ++it, index)

Notice that you lost the "++" before index.  Intensional?

Other than that, looks great!

[Nate Chapin]

Created attachment 45184 [details]
Example output

Yeah, sorry for my clumsiness with this patch.

Re: the spacing, I just followed the existing pattern in CodeGeneratorV8.pm, which is hopefully more obvious from the example output.

The removing of the "++" was unintentional and will be fixed in the next patch.

[Adam Barth]

I understand now.  The generated code is out of style, but I think we have bigger things to worry about than that.  If you post a new version with the above comments addressed, I think we're good to go.

[Nate Chapin]

Created attachment 45193 [details]
patch2

[Nate Chapin]

Created attachment 45194 [details]
patch2 again

Doh, I didn't upload an up to date version of the patch.  Trying again.

[WebKit Review Bot]

Attachment 45194 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/bindings/v8/custom/V8SVGElementInstanceCustom.cpp:34:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebCore/bindings/v8/custom/V8SVGLengthCustom.cpp:34:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 2

[Adam Barth]

Comment on attachment 45194 [details]
patch2 again

Ok.  Can you fix the style nits in a followup patch?  The Chromium EWS is probably processing your patch now.  You might want to give it a few minutes to process your patch.  Here's the URL that should have the results (yeah, we need a better status dashboard):

http://webkit-commit-queue.appspot.com/patch-status/chromium-ews/45194

[Nate Chapin]

(In reply to comment #10)
> (From update of attachment 45194 [details])
> Ok.  Can you fix the style nits in a followup patch?  The Chromium EWS is
> probably processing your patch now.  You might want to give it a few minutes to
> process your patch.  Here's the URL that should have the results (yeah, we need
> a better status dashboard):
> 
> http://webkit-commit-queue.appspot.com/patch-status/chromium-ews/45194

Is it alright if I ignore the last couple style nits?  The complaint is because of include headers that go like this:

#include "config.h"

#if ENABLED(SVG)
#include "PrimaryHeader.h"

That seems like a reasonable exception to me, but I'm not certain of it.

[Adam Barth]

> #if ENABLED(SVG)
> #include "PrimaryHeader.h"

Yeah, that sounds like a false alarm.  We should fix the tool not to complain in that case.

[Eric Seidel (no email)]

Comment on attachment 45194 [details]
patch2 again

Clearing Adam Barth's review on this obsolete patch.

[Nate Chapin]

Created attachment 48076 [details]
Death to V8Custom!

[Dimitri Glazkov (Google)]

Comment on attachment 48076 [details]
Death to V8Custom!

Duuude. Are you eventually going to reduce the V8 bindings code to 0? 'cuz I think if you said "yes", I'd believe ya.

[Adam Barth]

No GYPI change?

[Nate Chapin]

http://trac.webkit.org/changeset/54349