Summary: | [V8] V8CustomBinding.h should vanish. | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dimitri Glazkov (Google) <dglazkov> | ||||||||||||
Component: | WebCore JavaScript | Assignee: | Nate Chapin <japhet> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | abarth, japhet, webkit.review.bot | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Bug Depends on: | 32844, 33156, 33182, 33223, 33329, 33547, 33680, 34026 | ||||||||||||||
Bug Blocks: | 32630 | ||||||||||||||
Attachments: |
|
Description
Dimitri Glazkov (Google)
2009-12-16 15:49:23 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. 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? |