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.
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.
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 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.
+ 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!
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.
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.
Created attachment 45193 [details] patch2
Created attachment 45194 [details] patch2 again Doh, I didn't upload an up to date version of the patch. Trying again.
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 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
(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.
> #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 on attachment 45194 [details] patch2 again Clearing Adam Barth's review on this obsolete patch.
Created attachment 48076 [details] Death to V8Custom!
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.
No GYPI change?
http://trac.webkit.org/changeset/54349