Bug 26882 – Speed up creation of V8 wrappers for DOM nodes

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

[anton muhin]

Speed up creation of V8 wrappers for DOM nodes.

[anton muhin]

Created attachment 32122 [details]
patch to speed up creation of V8 wrappers

[Dimitri Glazkov (Google)]

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.

[anton muhin]

(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.

[Dimitri Glazkov (Google)]

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 ;)

[anton muhin]

Created attachment 32177 [details]
Reworked patch (with fixed indentation)

[anton muhin]

(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.

[anton muhin]

Created attachment 32180 [details]
Next iteration

Thanks to Mads who spotted missing indentation of array.

[Darin Fisher (:fishd, Google)]

Comment on attachment 32180 [details]
Next iteration

> Index: ChangeLog
...
> [phone]  anton muhin  <[e-mail]>

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

[anton muhin]

Created attachment 32184 [details]
Addressing Dmitry's comments

Dmitry!

Thanks a lot for review.  All done (hopefully)

[anton muhin]

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
> ...
> > [phone]  anton muhin  <[e-mail]>
> 
> 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

[anton muhin]

Created attachment 32373 [details]
Uploaded updated version

[Darin Fisher (:fishd, Google)]

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

[Darin Fisher (:fishd, Google)]

Landed as:  http://trac.webkit.org/changeset/45596

[anton muhin]

(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!