Bug 32638 - [V8] V8CustomBinding.h should vanish.
Summary: [V8] V8CustomBinding.h should vanish.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nate Chapin
URL:
Keywords:
Depends on: 32844 33156 33182 33223 33329 33547 33680 34026
Blocks: 32630
  Show dependency treegraph
 
Reported: 2009-12-16 15:49 PST by Dimitri Glazkov (Google)
Modified: 2010-02-04 11:36 PST (History)
3 users (show)

See Also:


Attachments
Generate header declarations for non-constructor callbacks (119.25 KB, patch)
2009-12-17 15:58 PST, Nate Chapin
abarth: review-
Details | Formatted Diff | Diff
Example output (1.66 KB, text/plain)
2009-12-18 13:47 PST, Nate Chapin
no flags Details
patch2 (121.17 KB, patch)
2009-12-18 15:29 PST, Nate Chapin
no flags Details | Formatted Diff | Diff
patch2 again (121.03 KB, patch)
2009-12-18 15:32 PST, Nate Chapin
japhet: commit-queue-
Details | Formatted Diff | Diff
Death to V8Custom! (38.03 KB, patch)
2010-02-03 16:23 PST, Nate Chapin
dglazkov: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dimitri Glazkov (Google) 2009-12-16 15:49:23 PST
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.
Comment 1 Dimitri Glazkov (Google) 2009-12-16 15:53:28 PST
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.
Comment 2 Nate Chapin 2009-12-17 15:58:48 PST
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.
Comment 3 Adam Barth 2009-12-17 22:18:58 PST
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.
Comment 4 Adam Barth 2009-12-17 22:29:36 PST
+ 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!
Comment 5 Nate Chapin 2009-12-18 13:47:26 PST
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.
Comment 6 Adam Barth 2009-12-18 15:10:02 PST
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.
Comment 7 Nate Chapin 2009-12-18 15:29:58 PST
Created attachment 45193 [details]
patch2
Comment 8 Nate Chapin 2009-12-18 15:32:20 PST
Created attachment 45194 [details]
patch2 again

Doh, I didn't upload an up to date version of the patch.  Trying again.
Comment 9 WebKit Review Bot 2009-12-18 15:35:20 PST
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
Comment 10 Adam Barth 2009-12-18 15:38:27 PST
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
Comment 11 Nate Chapin 2009-12-21 09:29:21 PST
(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.
Comment 12 Adam Barth 2009-12-21 11:33:52 PST
> #if ENABLED(SVG)
> #include "PrimaryHeader.h"

Yeah, that sounds like a false alarm.  We should fix the tool not to complain in that case.
Comment 13 Eric Seidel (no email) 2009-12-28 21:40:29 PST
Comment on attachment 45194 [details]
patch2 again

Clearing Adam Barth's review on this obsolete patch.
Comment 14 Nate Chapin 2010-02-03 16:23:57 PST
Created attachment 48076 [details]
Death to V8Custom!
Comment 15 Dimitri Glazkov (Google) 2010-02-03 18:58:32 PST
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.
Comment 16 Adam Barth 2010-02-03 19:07:36 PST
No GYPI change?
Comment 17 Nate Chapin 2010-02-04 11:36:52 PST
http://trac.webkit.org/changeset/54349