[webkit-reviews] review denied: [Bug 92993] [V8] Call AdjustAmountOfExternalAllocatedMemory when V8ArrayBuffer constructed and destructed : [Attachment 157193] Add more tests

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has denied Ulan Degenbaev
<ulan at chromium.org>'s request for review:
Bug 92993: [V8] Call AdjustAmountOfExternalAllocatedMemory when V8ArrayBuffer
constructed and destructed
https://bugs.webkit.org/show_bug.cgi?id=92993

Attachment 157193: Add more tests
https://bugs.webkit.org/attachment.cgi?id=157193&action=review

------- Additional Comments from Kenneth Russell <kbr at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=157193&action=review


This looks much better than the last patch. The tests look good and thorough.
I'm only r-'ing this because of the license issue on the new header. A couple
of other questions.

> Source/WTF/wtf/ArrayBuffer.h:40
> +class ArrayBufferDeallocationObserver {

You might want to add a comment that in the current implementation, the
instance of this must be a singleton living for the entire process's lifetime
(implicit because you're using a raw pointer to it in ArrayBuffer -- I'm not
suggesting to change that because of performance considerations).

> Source/WTF/wtf/ArrayBuffer.h:88
> +	   // Notify the current V8 isolate that the buffer is gone.

Are you sure this is the behavior you want to implement? Conceptually, the
other isolate should increase the amount of externally allocated memory, and
the deallocation observer should be copied to the newly allocated ArrayBuffer.
I realize this would involve more code, but do you at least want to add a FIXME
here?

> Source/WebCore/bindings/v8/custom/V8ArrayBufferCustom.h:2
> + * Copyright (C) 2012 Google Inc. All rights reserved.

Wrong license. Use the two-clause license in Source/WebKit/LICENSE.