bindings: Adds DOMArrayBuffer, etc. as thin wrappers for ArrayBuffer, etc.

bindings: Adds DOMArrayBuffer, etc. as thin wrappers for ArrayBuffer, etc.
and make them inherit from ScriptWrappable.

This CL adds:
- DOMArrayBuffer
- DOMArrayBufferView
- DOM<TYPE>Array
  where TYPE is {Int,Uint}{8,16,32}, Float{32,64},
  and Uint8Clamped.
- DOMDataView
- DOMArrayPiece

Each class corresponds to WTF::CLASSNAME where CLASSNAME is
the class name without the prefix DOM (say, ArrayBuffer).

DOMxxx classes have a similar class hierarchy as the original
Xxx classes.  i.e. DOM<TYPE>Array and DOMDataView inherit from
DOMArrayBufferView.

BUG=235436
NOPRESUBMIT=true
R=haraken@chromium.org, tkent@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183972

[Yuki, 2014-10-09 14:41:35]

yukishiino@chromium.org changed reviewers:
+ haraken@chromium.org

[Yuki, 2014-10-09 14:41:38]

I think this CL is now ready for review.
Could you review this CL?

[haraken, 2014-10-09 14:49:58]

haraken@chromium.org changed reviewers:
+ dslomov@chromium.org

[haraken, 2014-10-09 14:50:00]

+dslomov

(I'll take a close look tomorrow.)

[haraken, 2014-10-09 15:00:57]

Would you confirm that this CL doesn't cause regression in benchmarks of typed
arrays?

[haraken, 2014-10-09 15:02:04]

On 2014/10/09 15:00:57, haraken wrote:
> Would you confirm that this CL doesn't cause regression in benchmarks of typed
> arrays?

I guess Dmitry knows benchmarks we should run :)

[Yuki, 2014-10-10 13:29:26]

On 2014/10/09 15:02:04, haraken wrote:
> On 2014/10/09 15:00:57, haraken wrote:
> > Would you confirm that this CL doesn't cause regression in benchmarks of
typed
> > arrays?
> 
> I guess Dmitry knows benchmarks we should run :)

dslomov@, could you let me know what benchmarks we care about?
I'll run those benchmarks and let you guys know the results.

[Dmitry Lomov (no reviews), 2014-10-13 11:32:13]

On 2014/10/10 13:29:26, Yuki wrote:
> On 2014/10/09 15:02:04, haraken wrote:
> > On 2014/10/09 15:00:57, haraken wrote:
> > > Would you confirm that this CL doesn't cause regression in benchmarks of
> typed
> > > arrays?
> > 
> > I guess Dmitry knows benchmarks we should run :)
> 
> dslomov@, could you let me know what benchmarks we care about?
> I'll run those benchmarks and let you guys know the results.

I do not know of the right benchmark to really exercise DOM-V8 conversions for
typed arrays.
I would at the very least run WebKit/PerformanceTests/Bindings/.

[haraken, 2014-10-13 15:00:54]

On 2014/10/13 11:32:13, Dmitry Lomov (chromium) wrote:
> On 2014/10/10 13:29:26, Yuki wrote:
> > On 2014/10/09 15:02:04, haraken wrote:
> > > On 2014/10/09 15:00:57, haraken wrote:
> > > > Would you confirm that this CL doesn't cause regression in benchmarks of
> > typed
> > > > arrays?
> > > 
> > > I guess Dmitry knows benchmarks we should run :)
> > 
> > dslomov@, could you let me know what benchmarks we care about?
> > I'll run those benchmarks and let you guys know the results.
> 
> I do not know of the right benchmark to really exercise DOM-V8 conversions for
> typed arrays.
> I would at the very least run WebKit/PerformanceTests/Bindings/.

WebKit/PerformanceTests/Bindings/typed-array-* would be helpful. Would it be
worth trying Sunspider and Octane?

[Dmitry Lomov (no reviews), 2014-10-13 17:39:11]

Unlikely. The do not transition between Blink/V8.

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Yuki, 2014-10-14 07:31:37]

I ran PerformanceTests/Bindings/typed-array-* and I didn't see any regression. 
For all the data, the unit is runs-per-second.

typed-array-construct-from-array.html
	trunk	cl/606653006
avg	323.66	372.77
median	366.37	481.03
stdev	178.50	162.02

typed-array-construct-from-same-type.html
	trunk		cl/606653006
avg	2,115.91	2,109.47
median	2,111.52	2,116.41
stdev	   21.61	37.54

typed-array-construct-from-typed.html
	trunk	cl/606653006
avg	237.92	235.14
median	239.82	234.87
stdev	9.11	1.91

typed-array-set-from-typed.html
	trunk	cl/606653006
avg	540.32	616.59
median	571.99	617.89
stdev	66.86	3.30

[haraken, 2014-10-14 07:38:57]

On 2014/10/14 07:31:37, Yuki wrote:
> I ran PerformanceTests/Bindings/typed-array-* and I didn't see any regression.

> For all the data, the unit is runs-per-second.
> 
> typed-array-construct-from-array.html
> 	trunk	cl/606653006
> avg	323.66	372.77
> median	366.37	481.03
> stdev	178.50	162.02
> 
> typed-array-construct-from-same-type.html
> 	trunk		cl/606653006
> avg	2,115.91	2,109.47
> median	2,111.52	2,116.41
> stdev	   21.61	37.54
> 
> typed-array-construct-from-typed.html
> 	trunk	cl/606653006
> avg	237.92	235.14
> median	239.82	234.87
> stdev	9.11	1.91
> 
> typed-array-set-from-typed.html
> 	trunk	cl/606653006
> avg	540.32	616.59
> median	571.99	617.89
> stdev	66.86	3.30

Sounds great! I'll take a closer look tonight.

[haraken, 2014-10-14 15:11:20]

This is a fantastic CL! The overall approach looks good.

The main question is a programming rule about where we should use DOMArrayXXX
and where we should use ArrayXXX. See the following comments.

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/core/v8...
File Source/bindings/core/v8/SerializedScriptValue.cpp (right):

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/core/v8...
Source/bindings/core/v8/SerializedScriptValue.cpp:2077: *value =
toV8(DOMInt8Array::create(arrayBuffer, byteOffset, byteLength), creationContext,
isolate());

I'd keep .release(), because we're conceptually releasing a RefPtr here.

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/core/v8...
File Source/bindings/core/v8/custom/V8FileReaderCustom.cpp (right):

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/core/v8...
Source/bindings/core/v8/custom/V8FileReaderCustom.cpp:45: RefPtr<DOMArrayBuffer>
arrayBuffer = DOMArrayBuffer::create(impl->arrayBufferResult());

Help me understand: Where do you want to use DOMArrayBuffer and where do you
want to use ArrayBuffer? For example, you return an ArrayBuffer from
FileReader::arrayBufferResult and create a DOMArrayBuffer here. The programming
rule you have in mind is "use ArrayBuffer in core/ and use DOMArrayBuffer in
bindings/", right?

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/core/v8...
Source/bindings/core/v8/custom/V8FileReaderCustom.cpp:46:
v8SetReturnValueFast(info, arrayBuffer.get(), impl);

.get() => .release() (There is no behavioral difference but conceptually this
should be .release().)

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/scripts...
File Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/scripts...
Source/bindings/scripts/v8_interface.py:85: is_array_type =
IdlType(interface.name).is_array_element_type

Can we use interface.idl_type.is_array_element_type ?

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/scripts...
Source/bindings/scripts/v8_interface.py:86: is_typed_array_type =
IdlType(interface.name).is_typed_array_element_type

Ditto.

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/scripts...
Source/bindings/scripts/v8_interface.py:185: 'is_v8object': not is_array_type,

"v8object" sounds a bit too general to me. I'd just remove this variable since
'not is_array_type' will just work in template files.

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/scripts...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/scripts...
Source/bindings/scripts/v8_types.py:69: ARRAY_TYPES =
TYPED_ARRAY_TYPES.union(frozenset([

ARRAY_TYPE => ARRAY_BUFFER_TYPE

ARRAY is confusing with a normal array.

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/scripts...
Source/bindings/scripts/v8_types.py:75: IdlType.is_array_element_type =
property(

is_array_element_type => is_array_buffer ?

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/scripts...
Source/bindings/scripts/v8_types.py:78: IdlType.is_typed_array_element_type =
property(

is_typed_array_element_type => is_typed_array ?

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/templat...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/templat...
Source/bindings/templates/interface.cpp:782: {% if interface_name ==
'ArrayBuffer' %}

Can we add a run-binding-test for ArrayBuffer interface so that we can test the
auto-generated code?

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/templat...
Source/bindings/templates/interface.cpp:836: if (internalPointer)

Just help me understand: In what scenario can the internalPointer be 0?

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/templat...
Source/bindings/templates/interface.cpp:841:
typedArray->associateWithWrapper(typedArray->wrapperTypeInfo(), object,
v8::Isolate::GetCurrent());

We should pass |isolate| to toImpl() and avoid calling Isolate::GetCurrent(),
but it's OK in this CL. Passing |isolate| to toImpl() would complicate the CL
more :)

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/templat...
File Source/bindings/templates/interface.h (right):

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/templat...
Source/bindings/templates/interface.h:46: {% if is_v8object %}

I'd prefer {% if not is_array_type %}

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/templat...
Source/bindings/templates/interface.h:50: {% if is_array_type %}

You can merge this if branch with the above if branch.

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/tests/r...
File Source/bindings/tests/results/core/V8SVGTestInterface.cpp (right):

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/tests/r...
Source/bindings/tests/results/core/V8SVGTestInterface.cpp:105: 

Unnecessary empty line.

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/tests/r...
Source/bindings/tests/results/core/V8SVGTestInterface.cpp:108: return
hasInstance(value, isolate) ? toImpl(v8::Handle<v8::Object>::Cast(value)) : 0;

Isn't this a regression? We want to use ScriptWrappable's virtual toImpl() as
much as possible.

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/ArrayBu...
File Source/core/dom/ArrayBufferView.idl (right):

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/ArrayBu...
Source/core/dom/ArrayBufferView.idl:5: [

We want to have a link to the spec in IDL files. Shall we add the link?

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/DOMArra...
File Source/core/dom/DOMArrayBuffer.cpp (right):

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/DOMArra...
Source/core/dom/DOMArrayBuffer.cpp:19: RefPtrWillBeRawPtr<DOMArrayBuffer>
protect(this);

DOMArrayBuffer is not yet moved to Oilpan's heap. So this should be RefPtr, not
RefPtrWillBeRawPtr.

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/DOMArra...
File Source/core/dom/DOMArrayBuffer.h (right):

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/DOMArra...
Source/core/dom/DOMArrayBuffer.h:19: if (!buffer.get())

if (!buffer)

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/DOMArra...
Source/core/dom/DOMArrayBuffer.h:49: virtual v8::Handle<v8::Object>
wrap(v8::Handle<v8::Object> creationContext, v8::Isolate*) OVERRIDE;

OVERRIDE => override

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/DOMArra...
Source/core/dom/DOMArrayBuffer.h:56: ASSERT(m_buffer.get());

ASSERT(m_buffer)

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/DOMArra...
File Source/core/dom/DOMArrayBufferDeallocationObserver.cpp (right):

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/DOMArra...
Source/core/dom/DOMArrayBufferDeallocationObserver.cpp:21:
v8::Isolate::GetCurrent()->AdjustAmountOfExternalAllocatedMemory(-static_cast<int>(sizeInBytes));

If possible, we want to pass |isolate| to this method and avoid calling
v8::Isolate::GetCurrent(). You can address this in a follow-up CL.

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/DOMArra...
Source/core/dom/DOMArrayBufferDeallocationObserver.cpp:26:
v8::Isolate::GetCurrent()->AdjustAmountOfExternalAllocatedMemory(static_cast<int>(sizeInBytes));

Ditto.

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/DOMArra...
File Source/core/dom/DOMArrayBufferDeallocationObserver.h (right):

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/DOMArra...
Source/core/dom/DOMArrayBufferDeallocationObserver.h:12: class
DOMArrayBufferDeallocationObserver FINAL : public
WTF::ArrayBufferDeallocationObserver {

FINAL => final

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/DOMArra...
File Source/core/dom/DOMArrayBufferView.h (right):

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/DOMArra...
Source/core/dom/DOMArrayBufferView.h:20: if (!m_domArrayBuffer.get())

if (m_domArrayBuffer)

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/DOMArra...
Source/core/dom/DOMArrayBufferView.h:38: virtual v8::Handle<v8::Object>
associateWithWrapper(const WrapperTypeInfo*, v8::Handle<v8::Object> wrapper,
v8::Isolate*) OVERRIDE

Remove |wrapper|.

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/DOMArra...
Source/core/dom/DOMArrayBufferView.h:48: ASSERT(m_bufferView.get());

ASSERT(m_bufferView)

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/DOMArra...
Source/core/dom/DOMArrayBufferView.h:53: ASSERT(m_bufferView.get());

ASSERT(m_bufferView)

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/DOMArra...
Source/core/dom/DOMArrayBufferView.h:54: ASSERT(m_domArrayBuffer.get());

ASSERT(m_domArrayBuffer)

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/DOMArra...
File Source/core/dom/DOMArrayPiece.h (right):

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/DOMArra...
Source/core/dom/DOMArrayPiece.h:16: DOMArrayPiece(DOMArrayBuffer* buffer)

Add 'explicit'.

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/DOMArra...
Source/core/dom/DOMArrayPiece.h:18: DOMArrayPiece(DOMArrayBufferView* view)

Add 'explicit'.

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/DOMData...
File Source/core/dom/DOMDataView.cpp (right):

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/DOMData...
Source/core/dom/DOMDataView.cpp:14: PassRefPtr<DOMDataView>
DOMDataView::create(PassRefPtr<DOMArrayBuffer> buffer, unsigned byteOffset,
unsigned byteLength)

buffer => prpBuffer (We normally use prpXXX)

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/DOMData...
Source/core/dom/DOMDataView.cpp:19: RefPtr<DOMArrayBuffer> buf = buffer;

buf => buffer

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/DOMData...
Source/core/dom/DOMDataView.cpp:28: RefPtrWillBeRawPtr<DOMDataView>
protect(this);

DOMDataView is not yet on Oilpan's heap, so this should be RefPtr.

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/DOMData...
Source/core/dom/DOMDataView.cpp:34: v8::Local<v8::Value> v8Buffer =
toV8(buffer.get(), creationContext, isolate);

buffer.get() => buffer.release()

https://codereview.chromium.org/606653006/diff/300001/Source/core/html/ImageD...
File Source/core/html/ImageData.cpp (right):

https://codereview.chromium.org/606653006/diff/300001/Source/core/html/ImageD...
Source/core/html/ImageData.cpp:62: return adoptRefWillBeNoop(new ImageData(size,
DOMUint8ClampedArray::create(byteArray)));

I have the same question as before :)

I want to understand why you pass Uint8ClampedArray to ImageData::create, and
pass DOMUint8ClampedArray to ImageData's constructor. What's a rule about using
Uint8ClampedArray and DOMUint8ClampedArray?

https://codereview.chromium.org/606653006/diff/300001/Source/modules/webmidi/...
File Source/modules/webmidi/MIDIOutput.cpp (right):

https://codereview.chromium.org/606653006/diff/300001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIOutput.cpp:200: void MIDIOutput::send(Uint8Array*
array, double timestamp, ExceptionState& exceptionState)

Do we need both MIDIOutput(DOMUint8Array*, ...) and MIDIOutput(Uint8Array*,
...)?

https://codereview.chromium.org/606653006/diff/300001/Source/modules/websocke...
File Source/modules/websockets/DOMWebSocket.cpp (right):

https://codereview.chromium.org/606653006/diff/300001/Source/modules/websocke...
Source/modules/websockets/DOMWebSocket.cpp:415: ASSERT(arrayBufferView);

This check should be:

  ASSERT(arrayBufferView && arrayBufferView->view());

...this is another reason why we don't want to mix DOMXXX and XXX in the code
base. It's very likely we misuse if conditions.

I don't have a strong opinion about the rule, but I hope the rule is as simple
and consistent as possible (e.g., use DOMXXX as much as possible).

https://codereview.chromium.org/606653006/diff/300001/Source/web/WebArrayBuff...
File Source/web/WebArrayBufferConverter.cpp (right):

https://codereview.chromium.org/606653006/diff/300001/Source/web/WebArrayBuff...
Source/web/WebArrayBufferConverter.cpp:43: return toV8(arrayBuffer,
creationContext, isolate);

arrayBuffer.release()

https://codereview.chromium.org/606653006/diff/300001/Source/wtf/ArrayBuffer.h
File Source/wtf/ArrayBuffer.h (right):

https://codereview.chromium.org/606653006/diff/300001/Source/wtf/ArrayBuffer....
Source/wtf/ArrayBuffer.h:70: inline ArrayBuffer(ArrayBufferContents&);

Add 'explicit'.

[bashi, 2014-10-14 23:36:21]

bashi@chromium.org changed reviewers:
+ bashi@chromium.org

[bashi, 2014-10-14 23:36:24]

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/scripts...
File Source/bindings/scripts/code_generator_v8.py (right):

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/scripts...
Source/bindings/scripts/code_generator_v8.py:171: if
IdlType(interface_name).is_typed_array_element_type:
Can't we move this into interface_context() in v8_interface.py?

[Yuki, 2014-10-15 09:35:24]

Thanks for the reviews.

For the policy about what are to be DOMArrayXxx and what are to be ArrayXxx,
let's make a clear policy.  I'll follow it in a follow-up CL.  However, I'd like
to keep this CL as small as possible.

For run-bindings-tests, I'll add test cases and let you guys know.

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/core/v8...
File Source/bindings/core/v8/SerializedScriptValue.cpp (right):

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/core/v8...
Source/bindings/core/v8/SerializedScriptValue.cpp:2077: *value =
toV8(DOMInt8Array::create(arrayBuffer, byteOffset, byteLength), creationContext,
isolate());
On 2014/10/14 15:11:18, haraken wrote:
> 
> I'd keep .release(), because we're conceptually releasing a RefPtr here.

Done.

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/core/v8...
File Source/bindings/core/v8/custom/V8FileReaderCustom.cpp (right):

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/core/v8...
Source/bindings/core/v8/custom/V8FileReaderCustom.cpp:45: RefPtr<DOMArrayBuffer>
arrayBuffer = DOMArrayBuffer::create(impl->arrayBufferResult());
On 2014/10/14 15:11:18, haraken wrote:
> 
> Help me understand: Where do you want to use DOMArrayBuffer and where do you
> want to use ArrayBuffer? For example, you return an ArrayBuffer from
> FileReader::arrayBufferResult and create a DOMArrayBuffer here. The
programming
> rule you have in mind is "use ArrayBuffer in core/ and use DOMArrayBuffer in
> bindings/", right?

I made this CL with the least effort, and it's not ideal for me.  I just wanted
to keep this CL as small as possible as the first step.  So there is no wonder
that the DOMArrayXxx/ArrayXxx policy is very unclear.  Sorry for confusion.

Ideally I'd like to use DOMArrayXxx instead of the ArrayXxx which may need
wrappers.  It's almost everywhere.  Having said that, we still need to live with
ArrayBuffer created in platform/ though.

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/core/v8...
Source/bindings/core/v8/custom/V8FileReaderCustom.cpp:46:
v8SetReturnValueFast(info, arrayBuffer.get(), impl);
On 2014/10/14 15:11:18, haraken wrote:
> 
> .get() => .release() (There is no behavioral difference but conceptually this
> should be .release().)

Done.

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/scripts...
File Source/bindings/scripts/code_generator_v8.py (right):

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/scripts...
Source/bindings/scripts/code_generator_v8.py:171: if
IdlType(interface_name).is_typed_array_element_type:
On 2014/10/14 23:36:24, bashi1 wrote:
> Can't we move this into interface_context() in v8_interface.py?

It seems that |interface_info| is not available in IdlInterface and we cannot
retrieve 'include_path' there.  If it's possible, let me know.

Or, we can make it possible in a separate CL.

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/scripts...
File Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/scripts...
Source/bindings/scripts/v8_interface.py:85: is_array_type =
IdlType(interface.name).is_array_element_type
On 2014/10/14 15:11:18, haraken wrote:
> 
> Can we use interface.idl_type.is_array_element_type ?

|interface| of type IdlInterface does not have |idl_type|.
Would you like me to add idl_type into IdlInterface?
Or is it okay with the current code?

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/scripts...
Source/bindings/scripts/v8_interface.py:185: 'is_v8object': not is_array_type,
On 2014/10/14 15:11:18, haraken wrote:
> 
> "v8object" sounds a bit too general to me. I'd just remove this variable since
> 'not is_array_type' will just work in template files.

is_v8object (meant for v8::Object) is used at places where it's important that
the object is bound to v8::Object, and where it's not important that the object
is not an array buffer or its view.

I'm worrying that it would be confusing in future when we need to care about
v8::Foo which is neither of v8::Object nor v8::ArrayBuffer, etc.

I've added a comment about what v8object is meant.

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/scripts...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/scripts...
Source/bindings/scripts/v8_types.py:69: ARRAY_TYPES =
TYPED_ARRAY_TYPES.union(frozenset([
On 2014/10/14 15:11:18, haraken wrote:
> 
> ARRAY_TYPE => ARRAY_BUFFER_TYPE
> 
> ARRAY is confusing with a normal array.

Changed to ARRAY_BUFFER_AND_VIEW_TYPES.

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/scripts...
Source/bindings/scripts/v8_types.py:75: IdlType.is_array_element_type =
property(
On 2014/10/14 15:11:18, haraken wrote:
> 
> is_array_element_type => is_array_buffer ?

Changed to is_array_buffer_or_view.

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/scripts...
Source/bindings/scripts/v8_types.py:78: IdlType.is_typed_array_element_type =
property(
On 2014/10/14 15:11:18, haraken wrote:
> 
> is_typed_array_element_type => is_typed_array ?

Done.

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/templat...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/templat...
Source/bindings/templates/interface.cpp:782: {% if interface_name ==
'ArrayBuffer' %}
On 2014/10/14 15:11:18, haraken wrote:
> 
> Can we add a run-binding-test for ArrayBuffer interface so that we can test
the
> auto-generated code?
> 

Will do.

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/templat...
Source/bindings/templates/interface.cpp:836: if (internalPointer)
On 2014/10/14 15:11:18, haraken wrote:
> 
> Just help me understand: In what scenario can the internalPointer be 0?

There are cases that JS code (i.e. V8) creates an array buffer view
(v8::Uint8Array for example) allocating its own memory while Blink knows nothing
about it.  In those cases, internalPointer is zero, and we have to create a
corresponding instance in Blink.

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/templat...
File Source/bindings/templates/interface.h (right):

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/templat...
Source/bindings/templates/interface.h:50: {% if is_array_type %}
On 2014/10/14 15:11:18, haraken wrote:
> 
> You can merge this if branch with the above if branch.

I'd like *not* to put an assumption that is_v8object (v8::Object) == not
is_array_type.  There are types in V8 which types are not v8::Object or
v8::ArrayBuffer family.

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/tests/r...
File Source/bindings/tests/results/core/V8SVGTestInterface.cpp (right):

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/tests/r...
Source/bindings/tests/results/core/V8SVGTestInterface.cpp:105: 
On 2014/10/14 15:11:18, haraken wrote:
> 
> Unnecessary empty line.

Will check when I add a run-binding-test.

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/tests/r...
Source/bindings/tests/results/core/V8SVGTestInterface.cpp:108: return
hasInstance(value, isolate) ? toImpl(v8::Handle<v8::Object>::Cast(value)) : 0;
On 2014/10/14 15:11:18, haraken wrote:
> 
> Isn't this a regression? We want to use ScriptWrappable's virtual toImpl() as
> much as possible.

For v8::ArrayBuffer and its view, it's possible that the internal pointer is
null, and we cannot simply call internalPointer->toImpl<T>().

I'll revisit this point later.  Let me go on with this code for now.

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/ArrayBu...
File Source/core/dom/ArrayBufferView.idl (right):

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/ArrayBu...
Source/core/dom/ArrayBufferView.idl:5: [
On 2014/10/14 15:11:18, haraken wrote:
> 
> We want to have a link to the spec in IDL files. Shall we add the link?

Done.

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/DOMArra...
File Source/core/dom/DOMArrayBuffer.cpp (right):

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/DOMArra...
Source/core/dom/DOMArrayBuffer.cpp:19: RefPtrWillBeRawPtr<DOMArrayBuffer>
protect(this);
On 2014/10/14 15:11:19, haraken wrote:
> 
> DOMArrayBuffer is not yet moved to Oilpan's heap. So this should be RefPtr,
not
> RefPtrWillBeRawPtr.

Done.
Also done for DOMDataView and DOMTypedArray.

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/DOMArra...
File Source/core/dom/DOMArrayBuffer.h (right):

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/DOMArra...
Source/core/dom/DOMArrayBuffer.h:19: if (!buffer.get())
On 2014/10/14 15:11:19, haraken wrote:
> 
> if (!buffer)

Done.

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/DOMArra...
Source/core/dom/DOMArrayBuffer.h:49: virtual v8::Handle<v8::Object>
wrap(v8::Handle<v8::Object> creationContext, v8::Isolate*) OVERRIDE;
On 2014/10/14 15:11:19, haraken wrote:
> 
> OVERRIDE => override

Done.

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/DOMArra...
Source/core/dom/DOMArrayBuffer.h:56: ASSERT(m_buffer.get());
On 2014/10/14 15:11:19, haraken wrote:
> 
> ASSERT(m_buffer)

Done.

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/DOMArra...
File Source/core/dom/DOMArrayBufferDeallocationObserver.cpp (right):

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/DOMArra...
Source/core/dom/DOMArrayBufferDeallocationObserver.cpp:21:
v8::Isolate::GetCurrent()->AdjustAmountOfExternalAllocatedMemory(-static_cast<int>(sizeInBytes));
On 2014/10/14 15:11:19, haraken wrote:
> 
> If possible, we want to pass |isolate| to this method and avoid calling
> v8::Isolate::GetCurrent(). You can address this in a follow-up CL.

It seems very hard.  These are callback functions and wtf::ArrayBufferContents's
dtor and clear() method calls these callback.

One idea is that we create DOMArrayBufferDeallocationObserver for each isolate,
and make the observer remember the isolate.  However, the observer is expected
permanent lifetime by ArrayBufferContents and it makes the things difficult
(isolate doesn't have permanent lifetime).

I have several options here in my mind, but all the options make the code
complicated (e.g. lifetime management of observers, isolate -> observer
mapping).

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/DOMArra...
File Source/core/dom/DOMArrayBufferDeallocationObserver.h (right):

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/DOMArra...
Source/core/dom/DOMArrayBufferDeallocationObserver.h:12: class
DOMArrayBufferDeallocationObserver FINAL : public
WTF::ArrayBufferDeallocationObserver {
On 2014/10/14 15:11:19, haraken wrote:
> 
> FINAL => final

Done.

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/DOMArra...
File Source/core/dom/DOMArrayBufferView.h (right):

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/DOMArra...
Source/core/dom/DOMArrayBufferView.h:20: if (!m_domArrayBuffer.get())
On 2014/10/14 15:11:19, haraken wrote:
> 
> if (m_domArrayBuffer)

Done.

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/DOMArra...
Source/core/dom/DOMArrayBufferView.h:38: virtual v8::Handle<v8::Object>
associateWithWrapper(const WrapperTypeInfo*, v8::Handle<v8::Object> wrapper,
v8::Isolate*) OVERRIDE
On 2014/10/14 15:11:19, haraken wrote:
> 
> Remove |wrapper|.

It's not crystal clear that v8::Handle<v8::Object> is a wrapper, and we wrote
|wrapper| in other part of code, especially the declaration of
associateWithWrapper in ScriptWrappable.h.  It's not always that v8::Handle is a
wrapper.

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/DOMArra...
Source/core/dom/DOMArrayBufferView.h:48: ASSERT(m_bufferView.get());
On 2014/10/14 15:11:19, haraken wrote:
> 
> ASSERT(m_bufferView)

Done.

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/DOMArra...
Source/core/dom/DOMArrayBufferView.h:53: ASSERT(m_bufferView.get());
On 2014/10/14 15:11:19, haraken wrote:
> 
> ASSERT(m_bufferView)

Done.

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/DOMArra...
Source/core/dom/DOMArrayBufferView.h:54: ASSERT(m_domArrayBuffer.get());
On 2014/10/14 15:11:19, haraken wrote:
> 
> ASSERT(m_domArrayBuffer)

Done.

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/DOMArra...
File Source/core/dom/DOMArrayPiece.h (right):

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/DOMArra...
Source/core/dom/DOMArrayPiece.h:16: DOMArrayPiece(DOMArrayBuffer* buffer)
On 2014/10/14 15:11:19, haraken wrote:
> 
> Add 'explicit'.

We want the implicit conversion here, as same as WTF::ArrayPiece and
StringPiece.

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/DOMData...
File Source/core/dom/DOMDataView.cpp (right):

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/DOMData...
Source/core/dom/DOMDataView.cpp:14: PassRefPtr<DOMDataView>
DOMDataView::create(PassRefPtr<DOMArrayBuffer> buffer, unsigned byteOffset,
unsigned byteLength)
On 2014/10/14 15:11:19, haraken wrote:
> 
> buffer => prpBuffer (We normally use prpXXX)

Done.

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/DOMData...
Source/core/dom/DOMDataView.cpp:19: RefPtr<DOMArrayBuffer> buf = buffer;
On 2014/10/14 15:11:19, haraken wrote:
> 
> buf => buffer

Done.

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/DOMData...
Source/core/dom/DOMDataView.cpp:28: RefPtrWillBeRawPtr<DOMDataView>
protect(this);
On 2014/10/14 15:11:19, haraken wrote:
> 
> DOMDataView is not yet on Oilpan's heap, so this should be RefPtr.

Done.

https://codereview.chromium.org/606653006/diff/300001/Source/core/dom/DOMData...
Source/core/dom/DOMDataView.cpp:34: v8::Local<v8::Value> v8Buffer =
toV8(buffer.get(), creationContext, isolate);
On 2014/10/14 15:11:19, haraken wrote:
> 
> buffer.get() => buffer.release()

Done.

https://codereview.chromium.org/606653006/diff/300001/Source/modules/webmidi/...
File Source/modules/webmidi/MIDIOutput.cpp (right):

https://codereview.chromium.org/606653006/diff/300001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIOutput.cpp:200: void MIDIOutput::send(Uint8Array*
array, double timestamp, ExceptionState& exceptionState)
On 2014/10/14 15:11:19, haraken wrote:
> 
> Do we need both MIDIOutput(DOMUint8Array*, ...) and MIDIOutput(Uint8Array*,
> ...)?

There are two types of callers.

Auto-generated V8T passes DOMUint8Array* because the IDL file says that send()
receives Uint8Array which is implementedAs=DOMUint8Array.

On the other hand, the existing Blink-internal calls to send() pass Uint8Array*
as same as before.

https://codereview.chromium.org/606653006/diff/300001/Source/modules/websocke...
File Source/modules/websockets/DOMWebSocket.cpp (right):

https://codereview.chromium.org/606653006/diff/300001/Source/modules/websocke...
Source/modules/websockets/DOMWebSocket.cpp:415: ASSERT(arrayBufferView);
On 2014/10/14 15:11:20, haraken wrote:
> 
> This check should be:
> 
>   ASSERT(arrayBufferView && arrayBufferView->view());
> 
> ...this is another reason why we don't want to mix DOMXXX and XXX in the code
> base. It's very likely we misuse if conditions.
> 
> I don't have a strong opinion about the rule, but I hope the rule is as simple
> and consistent as possible (e.g., use DOMXXX as much as possible).

I agree that we'll need cleaner policy for this point.

For the ASSERT, I think it's okay as is.  I found that it's very problematic
that DOMArrayBufferView points to null of ArrayBufferView.  So I made
DOMxxx::create return nullptr if the underlying Xxx is null.  Also we have
ASSERT in DOMxxx's ctors to check that the underlying Xxx is not null.

https://codereview.chromium.org/606653006/diff/300001/Source/web/WebArrayBuff...
File Source/web/WebArrayBufferConverter.cpp (right):

https://codereview.chromium.org/606653006/diff/300001/Source/web/WebArrayBuff...
Source/web/WebArrayBufferConverter.cpp:43: return toV8(arrayBuffer,
creationContext, isolate);
On 2014/10/14 15:11:20, haraken wrote:
> 
> arrayBuffer.release()

Done.

https://codereview.chromium.org/606653006/diff/300001/Source/wtf/ArrayBuffer.h
File Source/wtf/ArrayBuffer.h (right):

https://codereview.chromium.org/606653006/diff/300001/Source/wtf/ArrayBuffer....
Source/wtf/ArrayBuffer.h:70: inline ArrayBuffer(ArrayBufferContents&);
On 2014/10/14 15:11:20, haraken wrote:
> 
> Add 'explicit'.

Done.

[bashi, 2014-10-15 10:17:34]

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/scripts...
File Source/bindings/scripts/code_generator_v8.py (right):

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/scripts...
Source/bindings/scripts/code_generator_v8.py:171: if
IdlType(interface_name).is_typed_array_element_type:
On 2014/10/15 09:35:23, Yuki wrote:
> On 2014/10/14 23:36:24, bashi1 wrote:
> > Can't we move this into interface_context() in v8_interface.py?
> 
> It seems that |interface_info| is not available in IdlInterface and we cannot
> retrieve 'include_path' there.  If it's possible, let me know.
> 
> Or, we can make it possible in a separate CL.

Ah, I see. I don't think we have a policy that we shouldn't pass interface_info
to interface_context(), but I don't have strong opinion here.

[haraken, 2014-10-16 05:41:42]

LGTM with comments.

It's OK to address some of the comments in follow-up CLs, but in that case
please add FIXMEs so that we won't miss :)

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/scripts...
File Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/scripts...
Source/bindings/scripts/v8_interface.py:85: is_array_type =
IdlType(interface.name).is_array_element_type
On 2014/10/15 09:35:23, Yuki wrote:
> On 2014/10/14 15:11:18, haraken wrote:
> > 
> > Can we use interface.idl_type.is_array_element_type ?
> 
> |interface| of type IdlInterface does not have |idl_type|.
> Would you like me to add idl_type into IdlInterface?
> Or is it okay with the current code?

I think it's better to introduce interface.idl_type.

https://codereview.chromium.org/606653006/diff/380001/Source/bindings/core/v8...
File Source/bindings/core/v8/SerializedScriptValue.cpp (right):

https://codereview.chromium.org/606653006/diff/380001/Source/bindings/core/v8...
Source/bindings/core/v8/SerializedScriptValue.cpp:2039:
arrayBuffer->setDeallocationObserver(DOMArrayBufferDeallocationObserver::instance());

We always pass DOMArrayBufferDeallocationObserver::instance() to
setDeallocationObserver, which seems redundant. Can we make
setDeallocationObserver use DOMArrayBufferDeallocationObserver::instance() by
default?

Actually it would be better to move the DOMArrayBufferDeallocationObserver class
into ArrayBuffer.cpp, since DOMArrayBufferDeallocationObserver is tightly
coupled with ArrayBuffer and is not used by anyone else.

https://codereview.chromium.org/606653006/diff/380001/Source/bindings/core/v8...
File Source/bindings/core/v8/custom/V8FileReaderCustom.cpp (right):

https://codereview.chromium.org/606653006/diff/380001/Source/bindings/core/v8...
Source/bindings/core/v8/custom/V8FileReaderCustom.cpp:46:
v8SetReturnValueFast(info, arrayBuffer.release(), impl);

You can write DOMArrayBuffer::create(impl->arrayBufferResult()) here.

https://codereview.chromium.org/606653006/diff/380001/Source/bindings/modules...
File Source/bindings/modules/v8/custom/V8CryptoKeyCustom.cpp (right):

https://codereview.chromium.org/606653006/diff/380001/Source/bindings/modules...
Source/bindings/modules/v8/custom/V8CryptoKeyCustom.cpp:47:
m_dictionary.set(propertyName, toV8(uint8Array.get(), m_holder, m_isolate));

uint8Array.get() => uint8Array.release()

or you can just directly write DOMUint8Array::create(...) here.

https://codereview.chromium.org/606653006/diff/380001/Source/bindings/scripts...
File Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/606653006/diff/380001/Source/bindings/scripts...
Source/bindings/scripts/v8_interface.py:85: is_array_type =
IdlType(interface.name).is_array_buffer_or_view

is_array_type => is_array_buffer_or_view (or simply is_array_buffer is fine)

I'd like to avoid array_type since it's confusing with a normal array type.
array_buffer is clearer.

https://codereview.chromium.org/606653006/diff/380001/Source/bindings/scripts...
Source/bindings/scripts/v8_interface.py:177: 'is_array_type': is_array_type,

is_array_type => is_array_buffer

https://codereview.chromium.org/606653006/diff/380001/Source/bindings/scripts...
Source/bindings/scripts/v8_interface.py:185: 'is_v8object': not is_array_type, 
# The object is wrapped with v8::Object.

Hmm, I'd prefer not introducing is_v8object, given that the current code base
uses only v8::Object and v8::ArrayBuffer and that we don't want to introduce
unnecessary complexity to the code base.

Basically, everything should be wrapped with v8::Object. Typed array is a
special case for performance and it's unlikely that we'll add more special
casings.

https://codereview.chromium.org/606653006/diff/380001/Source/bindings/templat...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/606653006/diff/380001/Source/bindings/templat...
Source/bindings/templates/interface.cpp:785: if (internalPointer)

BTW, let's rename internalPointer to scriptWrappableBase in a follow-up.

https://codereview.chromium.org/606653006/diff/380001/Source/bindings/templat...
Source/bindings/templates/interface.cpp:817: if (internalPointer)

We have the internalPointer check both in V8ArrayBufferView::toImpl and
{{v8_class}}::toImpl. Do we need both?

https://codereview.chromium.org/606653006/diff/380001/Source/bindings/templat...
Source/bindings/templates/interface.cpp:825: ASSERT(internalPointer);

ASSERT(internalPointer == typedArray) ?

https://codereview.chromium.org/606653006/diff/380001/Source/bindings/templat...
Source/bindings/templates/interface.cpp:826: return
internalPointer->toImpl<{{cpp_class}}>();

Actually it's redundant to get an internalPointer by
blink::toScriptWrappableBase(object). You can just call typedArray->toImpl().

https://codereview.chromium.org/606653006/diff/380001/Source/bindings/templat...
File Source/bindings/templates/interface_base.cpp (right):

https://codereview.chromium.org/606653006/diff/380001/Source/bindings/templat...
Source/bindings/templates/interface_base.cpp:419: {% if is_v8object and
has_conditional_attributes %}

I think you can drop the 'is_v8object and' check because typed array shouldn't
have has_conditional_attributes.

A better idea would be to add a verification to the IDL compiler to check that
typed array interfaces don't have has_conditional_attributes.

https://codereview.chromium.org/606653006/diff/380001/Source/bindings/templat...
Source/bindings/templates/interface_base.cpp:440: {% if is_v8object and
conditionally_enabled_methods %}

Ditto.

https://codereview.chromium.org/606653006/diff/380001/Source/bindings/tests/r...
File Source/bindings/tests/results/core/V8SVGTestInterface.cpp (right):

https://codereview.chromium.org/606653006/diff/380001/Source/bindings/tests/r...
Source/bindings/tests/results/core/V8SVGTestInterface.cpp:107: return
hasInstance(value, isolate) ? toImpl(v8::Handle<v8::Object>::Cast(value)) : 0;

I still think this is a regression.

I agree that we cannot call toScriptWrappableBase()->toImpl for v8::ArrayBuffer
and its view, but only for v8::ArrayBuffer and its view. It shouldn't affect
other toImpl like SVGTestInterface.

https://codereview.chromium.org/606653006/diff/380001/Source/core/dom/DOMArra...
File Source/core/dom/DOMArrayBuffer.cpp (right):

https://codereview.chromium.org/606653006/diff/380001/Source/core/dom/DOMArra...
Source/core/dom/DOMArrayBuffer.cpp:26:
wrapperTypeInfo->installConditionallyEnabledProperties(wrapper, isolate);

Is it possible that the typed array has installConditionallyEnabledProperties? I
think you skip generating installConditionallyEnabledProperties for typed array
interfaces.

I think you can remove this.

https://codereview.chromium.org/606653006/diff/380001/Source/core/dom/DOMArra...
File Source/core/dom/DOMArrayBuffer.h (right):

https://codereview.chromium.org/606653006/diff/380001/Source/core/dom/DOMArra...
Source/core/dom/DOMArrayBuffer.h:14: class DOMArrayBuffer : public
RefCounted<DOMArrayBuffer>, public ScriptWrappable {

Can we add final?

https://codereview.chromium.org/606653006/diff/380001/Source/core/dom/DOMArra...
Source/core/dom/DOMArrayBuffer.h:19: if (!buffer)

Can this happen? It's nasty that a create method can return nullptr. If this
shouldn't happen, you can add ASSERT(!buffer).

https://codereview.chromium.org/606653006/diff/380001/Source/core/dom/DOMArra...
Source/core/dom/DOMArrayBuffer.h:52: protected:

DOMArrayBuffer isn't inherited, so protected won't make sense?

https://codereview.chromium.org/606653006/diff/380001/Source/core/dom/DOMArra...
File Source/core/dom/DOMArrayBufferView.h (right):

https://codereview.chromium.org/606653006/diff/380001/Source/core/dom/DOMArra...
Source/core/dom/DOMArrayBufferView.h:20: if (!m_domArrayBuffer)

Just to confirm: You want to create DOMArrayBuffer lazily, right?

https://codereview.chromium.org/606653006/diff/380001/Source/core/dom/DOMData...
File Source/core/dom/DOMDataView.cpp (right):

https://codereview.chromium.org/606653006/diff/380001/Source/core/dom/DOMData...
Source/core/dom/DOMDataView.cpp:31: v8::Local<v8::Value> v8Buffer =
toV8(buffer.release(), creationContext, isolate);

You can just write toV8(buffer(), creationContext, isolate) here.

https://codereview.chromium.org/606653006/diff/380001/Source/core/dom/DOMData...
Source/core/dom/DOMDataView.cpp:36:
wrapperTypeInfo->installConditionallyEnabledProperties(wrapper, isolate);

I think you can remove this.

https://codereview.chromium.org/606653006/diff/380001/Source/core/dom/DOMData...
File Source/core/dom/DOMDataView.h (right):

https://codereview.chromium.org/606653006/diff/380001/Source/core/dom/DOMData...
Source/core/dom/DOMDataView.h:13: class DOMDataView : public DOMArrayBufferView
{

Can we add final?

https://codereview.chromium.org/606653006/diff/380001/Source/core/dom/DOMType...
File Source/core/dom/DOMTypedArray.cpp (right):

https://codereview.chromium.org/606653006/diff/380001/Source/core/dom/DOMType...
Source/core/dom/DOMTypedArray.cpp:31:
wrapperTypeInfo->installConditionallyEnabledProperties(wrapper, isolate);

Ditto. Remove this.

https://codereview.chromium.org/606653006/diff/380001/Source/core/dom/DOMType...
File Source/core/dom/DOMTypedArray.h (right):

https://codereview.chromium.org/606653006/diff/380001/Source/core/dom/DOMType...
Source/core/dom/DOMTypedArray.h:24: class DOMTypedArray : public
DOMArrayBufferView {

Add final.

https://codereview.chromium.org/606653006/diff/380001/Source/core/dom/DOMType...
Source/core/dom/DOMTypedArray.h:32: if (!bufferView.get())

Can this happen? It's nasty that a create method returns nullptr. Can we add
ASSERT(bufferView) instead of the if branch?

https://codereview.chromium.org/606653006/diff/380001/Source/modules/crypto/C...
File Source/modules/crypto/CryptoResultImpl.cpp (right):

https://codereview.chromium.org/606653006/diff/380001/Source/modules/crypto/C...
Source/modules/crypto/CryptoResultImpl.cpp:120:
m_resolver->resolve(DOMArrayBuffer::create(PassRefPtr<ArrayBuffer>(buffer)));

PassRefPtr<ArrayBuffer>() won't be needed.

[tyoshino (SeeGerritForStatus), 2014-10-16 06:39:12]

tyoshino@chromium.org changed reviewers:
+ tyoshino@chromium.org

[tyoshino (SeeGerritForStatus), 2014-10-16 06:39:15]

https://codereview.chromium.org/606653006/diff/380001/Source/core/dom/DOMArra...
File Source/core/dom/DOMArrayBuffer.h (right):

https://codereview.chromium.org/606653006/diff/380001/Source/core/dom/DOMArra...
Source/core/dom/DOMArrayBuffer.h:19: if (!buffer)
On 2014/10/16 05:41:41, haraken wrote:
> 
> Can this happen? It's nasty that a create method can return nullptr. If this
> shouldn't happen, you can add ASSERT(!buffer).
> 

ArrayBuffer::create() returns null when it fails to allocate memory. In
DOMWebSocket.cpp, this is used to propagate such an error. That said, we could
choose to allocate an ArrayBuffer and then only if it's successful create a
DOMArrayBuffer for DOMWebSocket.cpp's case.

https://codereview.chromium.org/606653006/diff/380001/Source/modules/websocke...
File Source/modules/websockets/DOMWebSocket.cpp (right):

https://codereview.chromium.org/606653006/diff/380001/Source/modules/websocke...
Source/modules/websockets/DOMWebSocket.cpp:415: ASSERT(arrayBufferView);
check arrayBufferView->view() is not NULL too?

https://codereview.chromium.org/606653006/diff/380001/Source/modules/websocke...
File Source/modules/websockets/DOMWebSocket.h (left):

https://codereview.chromium.org/606653006/diff/380001/Source/modules/websocke...
Source/modules/websockets/DOMWebSocket.h:43: #include "wtf/Forward.h"
Not only ArrayBuffer and ArrayBufferView but forward declarations of String and
Vector are used in this file.

[Yuki, 2014-10-16 14:21:52]

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/scripts...
File Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/scripts...
Source/bindings/scripts/v8_interface.py:85: is_array_type =
IdlType(interface.name).is_array_element_type
On 2014/10/16 05:41:40, haraken wrote:
> On 2014/10/15 09:35:23, Yuki wrote:
> > On 2014/10/14 15:11:18, haraken wrote:
> > > 
> > > Can we use interface.idl_type.is_array_element_type ?
> > 
> > |interface| of type IdlInterface does not have |idl_type|.
> > Would you like me to add idl_type into IdlInterface?
> > Or is it okay with the current code?
> 
> I think it's better to introduce interface.idl_type.

Done.

https://codereview.chromium.org/606653006/diff/300001/Source/bindings/scripts...
Source/bindings/scripts/v8_interface.py:86: is_typed_array_type =
IdlType(interface.name).is_typed_array_element_type
On 2014/10/14 15:11:18, haraken wrote:
> 
> Ditto.

Done.

https://codereview.chromium.org/606653006/diff/380001/Source/bindings/core/v8...
File Source/bindings/core/v8/SerializedScriptValue.cpp (right):

https://codereview.chromium.org/606653006/diff/380001/Source/bindings/core/v8...
Source/bindings/core/v8/SerializedScriptValue.cpp:2039:
arrayBuffer->setDeallocationObserver(DOMArrayBufferDeallocationObserver::instance());
On 2014/10/16 05:41:40, haraken wrote:
> 
> We always pass DOMArrayBufferDeallocationObserver::instance() to
> setDeallocationObserver, which seems redundant. Can we make
> setDeallocationObserver use DOMArrayBufferDeallocationObserver::instance() by
> default?
> 
> Actually it would be better to move the DOMArrayBufferDeallocationObserver
class
> into ArrayBuffer.cpp, since DOMArrayBufferDeallocationObserver is tightly
> coupled with ArrayBuffer and is not used by anyone else.

I found that we can remove most of setDeallocationObserver and removed them.  We
only need to set the observer in DOMArrayBuffer::wrap, it means that we tell to
V8 an increase of external memory allocation only when V8 gets owning the
wrapper to that external memory.  It's exactly when DOMArrayBuffer::wrap is
called.

The reason why we cannot move DOMArrayBufferDeallocationObserver into
WTF::ArrayBuffer is that wtf/ cannot depend on v8/.

https://codereview.chromium.org/606653006/diff/380001/Source/bindings/core/v8...
File Source/bindings/core/v8/custom/V8FileReaderCustom.cpp (right):

https://codereview.chromium.org/606653006/diff/380001/Source/bindings/core/v8...
Source/bindings/core/v8/custom/V8FileReaderCustom.cpp:46:
v8SetReturnValueFast(info, arrayBuffer.release(), impl);
On 2014/10/16 05:41:40, haraken wrote:
> 
> You can write DOMArrayBuffer::create(impl->arrayBufferResult()) here.

Done.

https://codereview.chromium.org/606653006/diff/380001/Source/bindings/modules...
File Source/bindings/modules/v8/custom/V8CryptoKeyCustom.cpp (right):

https://codereview.chromium.org/606653006/diff/380001/Source/bindings/modules...
Source/bindings/modules/v8/custom/V8CryptoKeyCustom.cpp:47:
m_dictionary.set(propertyName, toV8(uint8Array.get(), m_holder, m_isolate));
On 2014/10/16 05:41:41, haraken wrote:
> 
> uint8Array.get() => uint8Array.release()
> 
> or you can just directly write DOMUint8Array::create(...) here.

Done.

https://codereview.chromium.org/606653006/diff/380001/Source/bindings/scripts...
File Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/606653006/diff/380001/Source/bindings/scripts...
Source/bindings/scripts/v8_interface.py:85: is_array_type =
IdlType(interface.name).is_array_buffer_or_view
On 2014/10/16 05:41:41, haraken wrote:
> 
> is_array_type => is_array_buffer_or_view (or simply is_array_buffer is fine)
> 
> I'd like to avoid array_type since it's confusing with a normal array type.
> array_buffer is clearer.

Done.

https://codereview.chromium.org/606653006/diff/380001/Source/bindings/scripts...
Source/bindings/scripts/v8_interface.py:177: 'is_array_type': is_array_type,
On 2014/10/16 05:41:41, haraken wrote:
> 
> is_array_type => is_array_buffer

Done.

https://codereview.chromium.org/606653006/diff/380001/Source/bindings/scripts...
Source/bindings/scripts/v8_interface.py:185: 'is_v8object': not is_array_type, 
# The object is wrapped with v8::Object.
On 2014/10/16 05:41:41, haraken wrote:
> 
> Hmm, I'd prefer not introducing is_v8object, given that the current code base
> uses only v8::Object and v8::ArrayBuffer and that we don't want to introduce
> unnecessary complexity to the code base.
> 
> Basically, everything should be wrapped with v8::Object. Typed array is a
> special case for performance and it's unlikely that we'll add more special
> casings.
> 

Done.

https://codereview.chromium.org/606653006/diff/380001/Source/bindings/templat...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/606653006/diff/380001/Source/bindings/templat...
Source/bindings/templates/interface.cpp:785: if (internalPointer)
On 2014/10/16 05:41:41, haraken wrote:
> 
> BTW, let's rename internalPointer to scriptWrappableBase in a follow-up.

Will do.

https://codereview.chromium.org/606653006/diff/380001/Source/bindings/templat...
Source/bindings/templates/interface.cpp:817: if (internalPointer)
On 2014/10/16 05:41:41, haraken wrote:
> 
> We have the internalPointer check both in V8ArrayBufferView::toImpl and
> {{v8_class}}::toImpl. Do we need both?

It's a matter of the speed performance.

Obviously we cannot remove it in {{v8_class}}::toImpl.

We can remove the one in V8ArrayBufferView::toImpl, but in that case, we're
going to check object->IsXxxArray() every time.  It looks a kind of fast-path,
and the original code had this path.

Note that there are 38 lines which call V8ArrayBufferView::toImpl, including
TextDecoder, Blob, Crypt, XMLHttpRequest, WebSocket, FontFace, etc.

I do not have a strong opinion whether we should keep this fast path or not. 
I'm okay to remove it if you want.

https://codereview.chromium.org/606653006/diff/380001/Source/bindings/templat...
Source/bindings/templates/interface.cpp:825: ASSERT(internalPointer);
On 2014/10/16 05:41:41, haraken wrote:
> 
> ASSERT(internalPointer == typedArray) ?

Done.

https://codereview.chromium.org/606653006/diff/380001/Source/bindings/templat...
Source/bindings/templates/interface.cpp:826: return
internalPointer->toImpl<{{cpp_class}}>();
On 2014/10/16 05:41:41, haraken wrote:
> 
> Actually it's redundant to get an internalPointer by
> blink::toScriptWrappableBase(object). You can just call typedArray->toImpl().

Done.

https://codereview.chromium.org/606653006/diff/380001/Source/bindings/templat...
File Source/bindings/templates/interface_base.cpp (right):

https://codereview.chromium.org/606653006/diff/380001/Source/bindings/templat...
Source/bindings/templates/interface_base.cpp:419: {% if is_v8object and
has_conditional_attributes %}
On 2014/10/16 05:41:41, haraken wrote:
> 
> I think you can drop the 'is_v8object and' check because typed array shouldn't
> have has_conditional_attributes.
> 
> A better idea would be to add a verification to the IDL compiler to check that
> typed array interfaces don't have has_conditional_attributes.

Done.

https://codereview.chromium.org/606653006/diff/380001/Source/bindings/templat...
Source/bindings/templates/interface_base.cpp:440: {% if is_v8object and
conditionally_enabled_methods %}
On 2014/10/16 05:41:41, haraken wrote:
> 
> Ditto.

Done.

https://codereview.chromium.org/606653006/diff/380001/Source/bindings/tests/r...
File Source/bindings/tests/results/core/V8SVGTestInterface.cpp (right):

https://codereview.chromium.org/606653006/diff/380001/Source/bindings/tests/r...
Source/bindings/tests/results/core/V8SVGTestInterface.cpp:107: return
hasInstance(value, isolate) ? toImpl(v8::Handle<v8::Object>::Cast(value)) : 0;
On 2014/10/16 05:41:41, haraken wrote:
> 
> I still think this is a regression.
> 
> I agree that we cannot call toScriptWrappableBase()->toImpl for
v8::ArrayBuffer
> and its view, but only for v8::ArrayBuffer and its view. It shouldn't affect
> other toImpl like SVGTestInterface.

Okay, I added a {% if ... %} branch to the template.

https://codereview.chromium.org/606653006/diff/380001/Source/core/dom/DOMArra...
File Source/core/dom/DOMArrayBuffer.cpp (right):

https://codereview.chromium.org/606653006/diff/380001/Source/core/dom/DOMArra...
Source/core/dom/DOMArrayBuffer.cpp:26:
wrapperTypeInfo->installConditionallyEnabledProperties(wrapper, isolate);
On 2014/10/16 05:41:41, haraken wrote:
> 
> Is it possible that the typed array has installConditionallyEnabledProperties?
I
> think you skip generating installConditionallyEnabledProperties for typed
array
> interfaces.
> 
> I think you can remove this.
> 

Done.

https://codereview.chromium.org/606653006/diff/380001/Source/core/dom/DOMArra...
File Source/core/dom/DOMArrayBuffer.h (right):

https://codereview.chromium.org/606653006/diff/380001/Source/core/dom/DOMArra...
Source/core/dom/DOMArrayBuffer.h:14: class DOMArrayBuffer : public
RefCounted<DOMArrayBuffer>, public ScriptWrappable {
On 2014/10/16 05:41:41, haraken wrote:
> 
> Can we add final?

Done.

https://codereview.chromium.org/606653006/diff/380001/Source/core/dom/DOMArra...
Source/core/dom/DOMArrayBuffer.h:19: if (!buffer)
On 2014/10/16 06:39:15, tyoshino wrote:
> On 2014/10/16 05:41:41, haraken wrote:
> > 
> > Can this happen? It's nasty that a create method can return nullptr. If this
> > shouldn't happen, you can add ASSERT(!buffer).
> > 
> 
> ArrayBuffer::create() returns null when it fails to allocate memory. In
> DOMWebSocket.cpp, this is used to propagate such an error. That said, we could
> choose to allocate an ArrayBuffer and then only if it's successful create a
> DOMArrayBuffer for DOMWebSocket.cpp's case.

IIRC, XMLHttpRequest also treats nullptr to ArrayBuffer and I had to add this
check to pass the tests.

https://codereview.chromium.org/606653006/diff/380001/Source/core/dom/DOMArra...
Source/core/dom/DOMArrayBuffer.h:52: protected:
On 2014/10/16 05:41:41, haraken wrote:
> 
> DOMArrayBuffer isn't inherited, so protected won't make sense?

It's because I don't have any reason to make this class final and I prefer
extensible code in general.  Changed to private:.

https://codereview.chromium.org/606653006/diff/380001/Source/core/dom/DOMArra...
File Source/core/dom/DOMArrayBufferView.h (right):

https://codereview.chromium.org/606653006/diff/380001/Source/core/dom/DOMArra...
Source/core/dom/DOMArrayBufferView.h:20: if (!m_domArrayBuffer)
On 2014/10/16 05:41:42, haraken wrote:
> 
> Just to confirm: You want to create DOMArrayBuffer lazily, right?
> 

Yes.  IIUC, it's not often that we really need the wrapper to the underlying
ArrayBuffer.

https://codereview.chromium.org/606653006/diff/380001/Source/core/dom/DOMData...
File Source/core/dom/DOMDataView.cpp (right):

https://codereview.chromium.org/606653006/diff/380001/Source/core/dom/DOMData...
Source/core/dom/DOMDataView.cpp:31: v8::Local<v8::Value> v8Buffer =
toV8(buffer.release(), creationContext, isolate);
On 2014/10/16 05:41:42, haraken wrote:
> 
> You can just write toV8(buffer(), creationContext, isolate) here.

Done.

https://codereview.chromium.org/606653006/diff/380001/Source/core/dom/DOMData...
Source/core/dom/DOMDataView.cpp:36:
wrapperTypeInfo->installConditionallyEnabledProperties(wrapper, isolate);
On 2014/10/16 05:41:42, haraken wrote:
> 
> I think you can remove this.
> 

Done.

https://codereview.chromium.org/606653006/diff/380001/Source/core/dom/DOMData...
File Source/core/dom/DOMDataView.h (right):

https://codereview.chromium.org/606653006/diff/380001/Source/core/dom/DOMData...
Source/core/dom/DOMDataView.h:13: class DOMDataView : public DOMArrayBufferView
{
On 2014/10/16 05:41:42, haraken wrote:
> 
> Can we add final?

Done.

https://codereview.chromium.org/606653006/diff/380001/Source/core/dom/DOMType...
File Source/core/dom/DOMTypedArray.cpp (right):

https://codereview.chromium.org/606653006/diff/380001/Source/core/dom/DOMType...
Source/core/dom/DOMTypedArray.cpp:31:
wrapperTypeInfo->installConditionallyEnabledProperties(wrapper, isolate);
On 2014/10/16 05:41:42, haraken wrote:
> 
> Ditto. Remove this.

Done.

https://codereview.chromium.org/606653006/diff/380001/Source/core/dom/DOMType...
File Source/core/dom/DOMTypedArray.h (right):

https://codereview.chromium.org/606653006/diff/380001/Source/core/dom/DOMType...
Source/core/dom/DOMTypedArray.h:24: class DOMTypedArray : public
DOMArrayBufferView {
On 2014/10/16 05:41:42, haraken wrote:
> 
> Add final.

No, we cannot.  We have derived classes.

https://codereview.chromium.org/606653006/diff/380001/Source/core/dom/DOMType...
Source/core/dom/DOMTypedArray.h:32: if (!bufferView.get())
On 2014/10/16 05:41:42, haraken wrote:
> 
> Can this happen? It's nasty that a create method returns nullptr. Can we add
> ASSERT(bufferView) instead of the if branch?

Ditto.

https://codereview.chromium.org/606653006/diff/380001/Source/modules/crypto/C...
File Source/modules/crypto/CryptoResultImpl.cpp (right):

https://codereview.chromium.org/606653006/diff/380001/Source/modules/crypto/C...
Source/modules/crypto/CryptoResultImpl.cpp:120:
m_resolver->resolve(DOMArrayBuffer::create(PassRefPtr<ArrayBuffer>(buffer)));
On 2014/10/16 05:41:42, haraken wrote:
> 
> PassRefPtr<ArrayBuffer>() won't be needed.

Done.

https://codereview.chromium.org/606653006/diff/380001/Source/modules/websocke...
File Source/modules/websockets/DOMWebSocket.cpp (right):

https://codereview.chromium.org/606653006/diff/380001/Source/modules/websocke...
Source/modules/websockets/DOMWebSocket.cpp:415: ASSERT(arrayBufferView);
On 2014/10/16 06:39:15, tyoshino wrote:
> check arrayBufferView->view() is not NULL too?

As you know, we have a discussion whether it's okay or not that
DOMArrayBufferView::create returns nullptr.

If the underlying buffer view is null, we never create DOMArrayBufferView.  We
return nullptr instead.  So if |arrayBufferView| is not null, there must be the
underlying view as well.

I already added several ASSERTs to check the underlying buffer/view in
DOMArrayXxx's constructors.  Don't worry.

https://codereview.chromium.org/606653006/diff/380001/Source/modules/websocke...
File Source/modules/websockets/DOMWebSocket.h (left):

https://codereview.chromium.org/606653006/diff/380001/Source/modules/websocke...
Source/modules/websockets/DOMWebSocket.h:43: #include "wtf/Forward.h"
On 2014/10/16 06:39:15, tyoshino wrote:
> Not only ArrayBuffer and ArrayBufferView but forward declarations of String
and
> Vector are used in this file.

Done.  Reverted this line.

[Yuki, 2014-10-16 14:24:51]

yukishiino@chromium.org changed reviewers:
+ eseidel@chromium.org

[Yuki, 2014-10-16 14:24:53]

eseidel@, could you review this CL as an owner of wtf/ ?
You don't need to review other directories, but any comments are welcome.

[Yuki, 2014-10-17 06:51:48]

Patchset #21 (id:440001) has been deleted

[Yuki, 2014-10-17 07:02:29]

Added test cases for run-bindings-tests.

[haraken, 2014-10-17 07:34:24]

On 2014/10/17 07:02:29, Yuki wrote:
> Added test cases for run-bindings-tests.

run-bindings-tests look good.

[Yuki, 2014-10-20 04:22:09]

yukishiino@chromium.org changed reviewers:
+ tkent@chromium.org

[Yuki, 2014-10-20 04:22:12]

tkent@, could you review this CL as an owner of wtf/ ?  Thanks.

[tkent, 2014-10-20 04:30:57]

wtf lgtm

[Yuki, 2014-10-20 06:37:57]

The CQ bit was checked by yukishiino@chromium.org

[commit-bot: I haz the power, 2014-10-20 06:38:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/606653006/500001

[commit-bot: I haz the power, 2014-10-20 06:48:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-20 06:48:46]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)

[Yuki, 2014-10-20 07:12:48]

The CQ bit was checked by yukishiino@chromium.org

[commit-bot: I haz the power, 2014-10-20 07:13:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/606653006/500001

[commit-bot: I haz the power, 2014-10-20 07:25:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-20 07:25:08]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)

[Yuki, 2014-10-20 08:29:56]

The CQ bit was checked by yukishiino@chromium.org

[commit-bot: I haz the power, 2014-10-20 08:30:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/606653006/510001

[commit-bot: I haz the power, 2014-10-20 08:40:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-20 08:40:21]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)

[Yuki, 2014-10-20 08:59:58]

The CQ bit was checked by yukishiino@chromium.org

[commit-bot: I haz the power, 2014-10-20 09:00:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/606653006/510001

[commit-bot: I haz the power, 2014-10-20 09:11:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-20 09:11:15]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)

[haraken, 2014-10-20 09:12:43]

I think you need to rebase with ToT and rerun run-bindings-tests
--reset-results.

[Yuki, 2014-10-20 09:27:16]

On 2014/10/20 09:12:43, haraken wrote:
> I think you need to rebase with ToT and rerun run-bindings-tests
> --reset-results.

Already tried, but continue failing.
I think the trybots(presubmit) cannot see my new
tests/idls/core/ArrayBuffer.idl, etc., and generating code based on
core/dom/ArrayBuffer.idl.  Added NOPRESUBMIT=true, but it didn't help.

Will do dcommit.

[haraken, 2014-10-20 09:30:47]

On 2014/10/20 09:27:16, Yuki wrote:
> On 2014/10/20 09:12:43, haraken wrote:
> > I think you need to rebase with ToT and rerun run-bindings-tests
> > --reset-results.
> 
> Already tried, but continue failing.
> I think the trybots(presubmit) cannot see my new
> tests/idls/core/ArrayBuffer.idl, etc., and generating code based on
> core/dom/ArrayBuffer.idl.  Added NOPRESUBMIT=true, but it didn't help.
> 
> Will do dcommit.

Makes sense.

[Yuki, 2014-10-20 11:37:44]

Committed patchset #24 (id:510001) manually as 183972 (presubmit successful).

[yhirano, 2014-10-21 03:15:56]

On 2014/10/20 11:37:44, Yuki wrote:
> Committed patchset #24 (id:510001) manually as 183972 (presubmit successful).

Binding tests fail on my local environment. Does this CL cause the failure?

[yhirano, 2014-10-21 03:16:03]

On 2014/10/20 11:37:44, Yuki wrote:
> Committed patchset #24 (id:510001) manually as 183972 (presubmit successful).

Binding tests fail on my local environment. Does this CL cause the failure?

[Yuki, 2014-10-21 04:02:58]

Will look.

2014-10-21 12:16 GMT+09:00 <yhirano@chromium.org>:

> On 2014/10/20 11:37:44, Yuki wrote:
>
>> Committed patchset #24 (id:510001) manually as 183972 (presubmit
>> successful).
>>
>
> Binding tests fail on my local environment. Does this CL cause the failure?
>
> https://codereview.chromium.org/606653006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.