Speed up creation of DOM node wrappers

This is an attempt to get several low hanging fruits like not retrieve a proxy when instantiating new wrapper, don't invoke getDOMNodeMap more than once, etc.

LayoutTests (almost) pass.   Almost: only one test fails, but it fails w/o my change as well (it's LayoutTests/http/tests/security/cross-frame-access-history-put.html, does it ring a bell?).

[Mads Ager (chromium), 2009-06-30 17:21:19]

LGTM, but please wait for Dimitri's comments before sending the patch upstream.

The test failure that you have is because you are running with V8 bleeding edge.
 It is a line number issue in an error message.  The test needs to be
rebaselined when we update V8 in Chromium again.

http://codereview.chromium.org/151078/diff/1/3
File bindings/v8/V8Proxy.cpp (right):

http://codereview.chromium.org/151078/diff/1/3#newcode1223
Line 1223: if (!clone.IsEmpty()) {
I believe for WebKit style you should remove the begin and end brace for single
line if-statement bodies.

http://codereview.chromium.org/151078/diff/1/3#newcode2826
Line 2826: DOMWrapperMap<Node>& domNodeMap = /* proxy ? proxy->m_domNodeMap : */
getDOMNodeMap();
Remove the comment.

http://codereview.chromium.org/151078/diff/1/3#newcode2885
Line 2885: domNodeMap.set(node, v8::Persistent<v8::Object>::New(result));
Was this the only use of setJSWrapperForDOMNode?  In that case, we should remove
the method.

http://codereview.chromium.org/151078/diff/1/2
File bindings/v8/V8Proxy.h (right):

http://codereview.chromium.org/151078/diff/1/2#newcode562
Line 562: void* impl) {
Dimitri knows more about WebKit style than I do, but the '{' should be at the
beginning of the next line, and I think all the parameters should be on one long
line.

[Mads Ager (chromium), 2009-06-30 17:31:33]

One more thing.

http://codereview.chromium.org/151078/diff/1/2
File bindings/v8/V8Proxy.h (right):

http://codereview.chromium.org/151078/diff/1/2#newcode188
Line 188: explicit V8Proxy(Frame* frame) : m_frame(frame),
m_domNodeMap(getDOMNodeMap()), m_inlineCode(false), m_timerCallback(false),
m_recursion(0) { }
You don't use m_domNodeMap do you?  Remove.

http://codereview.chromium.org/151078/diff/1/2#newcode606
Line 606: DOMWrapperMap<Node>& m_domNodeMap;
Remove?

[dglazkov, 2009-06-30 17:33:05]

LGTM on the concept. Lots of style issues -- I would highly recommend looking at
http://webkit.org/coding/coding-style.html and cleaning up before sending for
review.

When ready, submit this to bugs.webkit.org:
http://webkit.org/coding/contributing.html

cc me, I'll review there.

[antonm, 2009-07-01 12:56:31]

Dmitri, Mads,

thanks a lot for review!

Notable change: I've uncommented using of m_domNodeMap---sorry that was an
oversight on my side.

I tried to address all the code style issues (pointed out and others), but most
probably missed something.

Going to start WebKit review process, but would address any new comments for
this CL.

yours,
anton.

http://codereview.chromium.org/151078/diff/1/3
File bindings/v8/V8Proxy.cpp (right):

http://codereview.chromium.org/151078/diff/1/3#newcode1223
Line 1223: if (!clone.IsEmpty()) {
On 2009/06/30 17:21:19, Mads Ager wrote:
> I believe for WebKit style you should remove the begin and end brace for
single
> line if-statement bodies.

Done.

http://codereview.chromium.org/151078/diff/1/3#newcode2826
Line 2826: DOMWrapperMap<Node>& domNodeMap = /* proxy ? proxy->m_domNodeMap : */
getDOMNodeMap();
On 2009/06/30 17:21:19, Mads Ager wrote:
> Remove the comment.

Done.

http://codereview.chromium.org/151078/diff/1/3#newcode2885
Line 2885: domNodeMap.set(node, v8::Persistent<v8::Object>::New(result));
On 2009/06/30 17:21:19, Mads Ager wrote:
> Was this the only use of setJSWrapperForDOMNode?  In that case, we should
remove
> the method.

At least for now () there are some other usages (in
V8HTML{Option,Image,Audio}ElementConstructor.cpp).

http://codereview.chromium.org/151078/diff/1/2
File bindings/v8/V8Proxy.h (right):

http://codereview.chromium.org/151078/diff/1/2#newcode188
Line 188: explicit V8Proxy(Frame* frame) : m_frame(frame),
m_domNodeMap(getDOMNodeMap()), m_inlineCode(false), m_timerCallback(false),
m_recursion(0) { }
On 2009/06/30 17:31:34, Mads Ager wrote:
> You don't use m_domNodeMap do you?  Remove.

Dreadful sorry, I commented it out to check if it was a culprit of regression in
LayoutTests and forgot to uncomment.  So I indeed use it.

http://codereview.chromium.org/151078/diff/1/2#newcode562
Line 562: void* impl) {
On 2009/06/30 17:21:19, Mads Ager wrote:
> Dimitri knows more about WebKit style than I do, but the '{' should be at the
> beginning of the next line, and I think all the parameters should be on one
long
> line.

Done.

http://codereview.chromium.org/151078/diff/1/2#newcode606
Line 606: DOMWrapperMap<Node>& m_domNodeMap;
On 2009/06/30 17:31:34, Mads Ager wrote:
> Remove?

No, see above.  Unless it's safe, of course.

[Mads Ager (chromium), 2009-07-01 13:19:26]

LGTM

[antonm, 2009-07-03 18:27:48]

On 2009/07/01 13:19:26, Mads Ager wrote:
> LGTM

Close this one as it has been moved to webkit review process.