Speed up creation of V8 wrappers for DOM nodes.
Created attachment 32122 [details] patch to speed up creation of V8 wrappers
Anton, can you re-upload as patch (check the checkbox)? As-is, this is not formally reviewable -- bugzilla sees it as an attachment, not a patch. Also, I downloaded the patch and it looks like you still have plenty of style issues. Please look at the spacing (4 spaces, not 2), positioning of brackets, naming convention, etc.
(In reply to comment #2) > Anton, can you re-upload as patch (check the checkbox)? As-is, this is not > formally reviewable -- bugzilla sees it as an attachment, not a patch. > > Also, I downloaded the patch and it looks like you still have plenty of style > issues. Please look at the spacing (4 spaces, not 2), positioning of brackets, > naming convention, etc. > Dmitry, dreadful sorry, but I cannot find out a checkbox which would allow me to upload a patch proper, what's the text close to it? I searched for 'patch', but to no avail. May I need some bits for that? Regarding style. I rechecked the diff and didn't spot misplaced {} and wrong names. I fixed many lines with invalid indentation. If you can point out me some place which violates code style, it'd make it easier to spot more of them to me.
No problem -- glad to help. When you click to create new attachment, There is this option: if the attachment is a patch, check the box below. [ ] patch Let's look at the style when you re-upload :) BTW, you're sitting right next to a WebKit committer -- I highly recommend bugging him when in doubt ;)
Created attachment 32177 [details] Reworked patch (with fixed indentation)
(In reply to comment #4) > No problem -- glad to help. When you click to create new attachment, There is > this option: > > if the attachment is a patch, check the box below. > [ ] patch > > Let's look at the style when you re-upload :) > > BTW, you're sitting right next to a WebKit committer -- I highly recommend > bugging him when in doubt ;) > :) Thanks a lot, Dmitry. He just left by that time. yours, anton.
Created attachment 32180 [details] Next iteration Thanks to Mads who spotted missing indentation of array.
Comment on attachment 32180 [details] Next iteration > Index: ChangeLog ... > +2009-07-01 anton muhin <antonm@chromium.org> nit: your name should be Mixed Case > + Reviewed by NOBODY (OOPS!). > + > + Speed up creation of V8 wrappers for DOM nodes. > + > + https://bugs.webkit.org/show_bug.cgi?id=26882 nit: please do not put tabs in the ChangeLog > + > + This patch doesn't require new tests as it a set of refactorings > + to speed up wrapper creation. > + > + * ../../../bindings/v8/V8Proxy.cpp: > + * ../../../bindings/v8/V8Proxy.h: > + nit: these paths should be relative to the WebCore directory. > Index: bindings/v8/V8Proxy.h ... > + static v8::Local<v8::Object> instantiateV8Object(V8ClassIndex::V8WrapperType descType, V8ClassIndex::V8WrapperType cptrType, void* impl) > + { > + return instantiateV8Object(NULL, descType, cptrType, impl); nit: indent by 4 spaces
Created attachment 32184 [details] Addressing Dmitry's comments Dmitry! Thanks a lot for review. All done (hopefully)
Darin, sorry, I already answered your comments, but thought it was a review by Dmitry, dreadful sorry. May you have another pass? tia and yours, anton. (In reply to comment #8) > (From update of attachment 32180 [details]) > > Index: ChangeLog > ... > > +2009-07-01 anton muhin <antonm@chromium.org> > > nit: your name should be Mixed Case > > > > + Reviewed by NOBODY (OOPS!). > > + > > + Speed up creation of V8 wrappers for DOM nodes. > > + > > + https://bugs.webkit.org/show_bug.cgi?id=26882 > > nit: please do not put tabs in the ChangeLog > > > > + > > + This patch doesn't require new tests as it a set of refactorings > > + to speed up wrapper creation. > > + > > + * ../../../bindings/v8/V8Proxy.cpp: > > + * ../../../bindings/v8/V8Proxy.h: > > + > > nit: these paths should be relative to the WebCore directory. > > > > Index: bindings/v8/V8Proxy.h > ... > > + static v8::Local<v8::Object> instantiateV8Object(V8ClassIndex::V8WrapperType descType, V8ClassIndex::V8WrapperType cptrType, void* impl) > > + { > > + return instantiateV8Object(NULL, descType, cptrType, impl); > > nit: indent by 4 spaces
Created attachment 32373 [details] Uploaded updated version
Comment on attachment 32373 [details] Uploaded updated version >Index: bindings/v8/V8Proxy.cpp >+static const V8ClassIndex::V8WrapperType mapping[] = { >+ V8ClassIndex::INVALID_CLASS_INDEX, // NONE >+ V8ClassIndex::INVALID_CLASS_INDEX, // ELEMENT_NODE needs special treatment >+ V8ClassIndex::ATTR, // ATTRIBUTE_NODE >+ V8ClassIndex::TEXT, // TEXT_NODE >+ V8ClassIndex::CDATASECTION, // CDATA_SECTION_NODE >+ V8ClassIndex::ENTITYREFERENCE, // ENTITY_REFERENCE_NODE >+ V8ClassIndex::ENTITY, // ENTITY_NODE >+ V8ClassIndex::PROCESSINGINSTRUCTION, // PROCESSING_INSTRUCTION_NODE >+ V8ClassIndex::COMMENT, // COMMENT_NODE >+ V8ClassIndex::INVALID_CLASS_INDEX, // DOCUMENT_NODE needs special treatment >+ V8ClassIndex::DOCUMENTTYPE, // DOCUMENT_TYPE_NODE >+ V8ClassIndex::DOCUMENTFRAGMENT, // DOCUMENT_FRAGMENT_NODE >+ V8ClassIndex::NOTATION, // NOTATION_NODE >+ V8ClassIndex::NODE, // XPATH_NAMESPACE_NODE >+}; This is a little bit fragile. If someone changes the NodeType enum, then we'll be in trouble. I don't have a good suggestion to avoid this though :( R=me
Landed as: http://trac.webkit.org/changeset/45596
(In reply to comment #13) > Landed as: http://trac.webkit.org/changeset/45596 (In reply to comment #12) > (From update of attachment 32373 [details]) > >Index: bindings/v8/V8Proxy.cpp > > >+static const V8ClassIndex::V8WrapperType mapping[] = { > >+ V8ClassIndex::INVALID_CLASS_INDEX, // NONE > >+ V8ClassIndex::INVALID_CLASS_INDEX, // ELEMENT_NODE needs special treatment > >+ V8ClassIndex::ATTR, // ATTRIBUTE_NODE > >+ V8ClassIndex::TEXT, // TEXT_NODE > >+ V8ClassIndex::CDATASECTION, // CDATA_SECTION_NODE > >+ V8ClassIndex::ENTITYREFERENCE, // ENTITY_REFERENCE_NODE > >+ V8ClassIndex::ENTITY, // ENTITY_NODE > >+ V8ClassIndex::PROCESSINGINSTRUCTION, // PROCESSING_INSTRUCTION_NODE > >+ V8ClassIndex::COMMENT, // COMMENT_NODE > >+ V8ClassIndex::INVALID_CLASS_INDEX, // DOCUMENT_NODE needs special treatment > >+ V8ClassIndex::DOCUMENTTYPE, // DOCUMENT_TYPE_NODE > >+ V8ClassIndex::DOCUMENTFRAGMENT, // DOCUMENT_FRAGMENT_NODE > >+ V8ClassIndex::NOTATION, // NOTATION_NODE > >+ V8ClassIndex::NODE, // XPATH_NAMESPACE_NODE > >+}; > > This is a little bit fragile. If someone changes the NodeType > enum, then we'll be in trouble. I don't have a good suggestion > to avoid this though :( > > R=me There is minimal safety net below (when bounds of array are checked in debug mode)---hopefully that might catch at least some troubles. Plus I believe in LayoutTests :) Thanks a lot for review!