Introduce FlexibleArrayBufferView and TypedFlexibleArrayBufferView

Introduce FlexibleArrayBufferView and TypedFlexibleArrayBufferView

Rebase and extension from https://codereview.chromium.org/1091543002/ by jl@.

The [FlexibleArrayBufferView] extended attribute, when used on a method
argument of type ArrayBufferView, causes the value to be processed and
passed to the implementation as the new C++ type FlexibleArrayBufferView
instead of as DOMArrayBufferView*.

A FlexibleArrayBufferView object will either contain a pointer to a
DOMArrayBufferView, or, if the array was small (without a materialized
buffer), a stack allocated buffer containing the contents of the array.

When used on a method argument of a typed array type (e.g. Float32Array)
causes the value to be processed and passed to the implementation as 
TypedFlexibleArrayBufferView instead of the corresponding DOM type.

A TypedFlexibleArrayBufferView extends FlexibleArrayBufferView with
the corresponding type methods for data and lengths.

OORT performance example (single run):
  "marksweep": {
    "count": 10, 
    "pause_min": 7.2, 
    "pause_max": 356.8, 
    "pause_avg": 94.5, 
    "pause_gt_10ms": 9
  }
   --- vs ---
  "marksweep": {
    "count": 6, 
    "pause_min": 7.5, 
    "pause_max": 148.0, 
    "pause_avg": 46.833333333333336, 
    "pause_gt_10ms": 5
  }



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

[Michael Lippautz, 2015-07-30 14:15:20]

mlippautz@chromium.org changed reviewers:
+ haraken@chromium.org, jl@opera.com, jochen@chromium.org, kbr@chromium.org

[Michael Lippautz, 2015-07-30 14:15:21]

PTAL

Rebased and extended https://codereview.chromium.org/1091543002/

The main idea is to reduce the number of global handles in V8. E.g., OORT
creates ~1.8mio global handles to Float32Arrays from uniformXXX() methods which
need to processed in the atomic pause of the GC.

[jochen (gone - plz use gerrit), 2015-07-30 14:30:06]

please include in the CL description that this is based on the patch from jl@

otherwise lgtm

https://codereview.chromium.org/1269443002/diff/100001/Source/bindings/core/v...
File Source/bindings/core/v8/V8BindingMacros.h (right):

https://codereview.chromium.org/1269443002/diff/100001/Source/bindings/core/v...
Source/bindings/core/v8/V8BindingMacros.h:50: (value->IsArrayBufferView() &&
(value.As<v8::ArrayBufferView>()->ByteLength() <= 128) ?
alloca(value.As<v8::ArrayBufferView>()->ByteLength()) : nullptr)
should we change the default for on-heap typed arrays in v8 to <= 128 as well?

https://codereview.chromium.org/1269443002/diff/100001/Source/bindings/core/v...
Source/bindings/core/v8/V8BindingMacros.h:52: 
nit. only one empty line

https://codereview.chromium.org/1269443002/diff/100001/Source/core/dom/Flexib...
File Source/core/dom/FlexibleArrayBufferView.h (right):

https://codereview.chromium.org/1269443002/diff/100001/Source/core/dom/Flexib...
Source/core/dom/FlexibleArrayBufferView.h:23: : m_smallData(nullptr),
m_smallLength(0)
nit. each initializer on its own line:

: m_smallData()
, m_smallLength()

[haraken, 2015-07-30 14:54:57]

Mostly looks good.

https://codereview.chromium.org/1269443002/diff/100001/Source/bindings/IDLExt...
File Source/bindings/IDLExtendedAttributes.txt (right):

https://codereview.chromium.org/1269443002/diff/100001/Source/bindings/IDLExt...
Source/bindings/IDLExtendedAttributes.txt:57: FlexibleArrayBufferView

Please update the chromium wiki:

https://www.chromium.org/blink/webidl/blink-idl-extended-attributes

https://codereview.chromium.org/1269443002/diff/100001/Source/bindings/core/v...
File Source/bindings/core/v8/V8Binding.h (right):

https://codereview.chromium.org/1269443002/diff/100001/Source/bindings/core/v...
Source/bindings/core/v8/V8Binding.h:66: class FlexibleArrayBufferView;

Alphabetical order.

https://codereview.chromium.org/1269443002/diff/100001/Source/bindings/core/v...
File Source/bindings/core/v8/V8BindingMacros.h (right):

https://codereview.chromium.org/1269443002/diff/100001/Source/bindings/core/v...
Source/bindings/core/v8/V8BindingMacros.h:49: #define
SMALL_ARRAY_BUFFER_VIEW_STORAGE(value) \

Add a comment about what this is doing.

https://codereview.chromium.org/1269443002/diff/100001/Source/bindings/core/v...
Source/bindings/core/v8/V8BindingMacros.h:49: #define
SMALL_ARRAY_BUFFER_VIEW_STORAGE(value) \

I'd rename this to allocateFlexibleArrayBufferViewStorage().

https://codereview.chromium.org/1269443002/diff/100001/Source/bindings/script...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/1269443002/diff/100001/Source/bindings/script...
Source/bindings/scripts/v8_types.py:381: if base_idl_type == 'ArrayBufferView'
and 'FlexibleArrayBufferView' in extended_attributes:

Can we add this to INCLUDES_FOR_TYPE instead of hardcoding include files here?

I think we can just unconditionally include 8ArrayBufferView.h and
FlexibleArrayBufferView.h if the type is ArrayBufferView.

https://codereview.chromium.org/1269443002/diff/100001/Source/bindings/script...
Source/bindings/scripts/v8_types.py:384: if idl_type.base_type in
TYPED_ARRAY_TYPES and 'FlexibleArrayBufferView' in extended_attributes:

Ditto.

https://codereview.chromium.org/1269443002/diff/100001/Source/bindings/script...
Source/bindings/scripts/v8_types.py:569: base_idl_type =
'FlexibleArrayBufferView'

Shouldn't this be:

  'Flexible' + idl_type.base_type + 'View'

?

https://codereview.chromium.org/1269443002/diff/100001/Source/bindings/script...
Source/bindings/scripts/v8_types.py:669: elif idl_type.base_type in
TYPED_ARRAY_TYPES and 'FlexibleArrayBufferView' in extended_attributes:

Slightly better:

elif 'FlexibleArrayBufferView' in extended_attributes:
  raise "..." if idl_type.base_type != 'ArrayBufferView' or idl_type.base_type
in TYPED_ARRAY_TYPES
  set_expression = cpp_calue

https://codereview.chromium.org/1269443002/diff/100001/Source/core/dom/Flexib...
File Source/core/dom/FlexibleArrayBufferView.h (right):

https://codereview.chromium.org/1269443002/diff/100001/Source/core/dom/Flexib...
Source/core/dom/FlexibleArrayBufferView.h:16: // that is only valid during the
life-time of the FlexibleArrayBufferView object.

Just to confirm: Does the "life-time of the FlexibleArrayBufferView object" mean
"while we're on stack"?

I'd rename baseAddress() to something like baseAddressMaybeOnStack() to make the
danger clearer.

https://codereview.chromium.org/1269443002/diff/100001/Source/core/dom/Flexib...
Source/core/dom/FlexibleArrayBufferView.h:20:
WTF_MAKE_NONCOPYABLE(FlexibleArrayBufferView);

Add WTF_MAKE_FAST_ALLOCATED().

https://codereview.chromium.org/1269443002/diff/100001/Source/core/dom/Flexib...
Source/core/dom/FlexibleArrayBufferView.h:36: unsigned byteOffset() const {
ASSERT(!isEmpty()); return isFull() ? m_full->byteOffset() : 0; }

Is it ok to return 0 for byteOffset()?

https://codereview.chromium.org/1269443002/diff/100001/Source/core/dom/Flexib...
Source/core/dom/FlexibleArrayBufferView.h:40: void operator= (std::nullptr_t) {
m_full = nullptr; m_smallData = nullptr; }

We normally don't implement operator=(nullptr). It would be better to rename
this to clear().

https://codereview.chromium.org/1269443002/diff/100001/Source/core/dom/TypedF...
File Source/core/dom/TypedFlexibleArrayBufferView.h (right):

https://codereview.chromium.org/1269443002/diff/100001/Source/core/dom/TypedF...
Source/core/dom/TypedFlexibleArrayBufferView.h:17:
WTF_MAKE_NONCOPYABLE(TypedFlexibleArrayBufferView);

Add WTF_MAKE_FAST_ALLOCATED.

https://codereview.chromium.org/1269443002/diff/100001/Source/core/dom/TypedF...
Source/core/dom/TypedFlexibleArrayBufferView.h:19: typedef typename
WTFTypedArray::ValueType ValueType;

Use 'using'.

https://codereview.chromium.org/1269443002/diff/100001/Source/core/dom/TypedF...
Source/core/dom/TypedFlexibleArrayBufferView.h:23: ValueType* data() const {
return static_cast<ValueType*>(baseAddress()); }

I'd rename this to dataMaybeOnStack() or something.

https://codereview.chromium.org/1269443002/diff/100001/Source/core/dom/TypedF...
Source/core/dom/TypedFlexibleArrayBufferView.h:28: typedef
TypedFlexibleArrayBufferView<WTF::Int32Array> FlexibleInt32ArrayView;

Blink prefers 'using' in the new coding style.

[Michael Lippautz, 2015-07-30 17:06:15]

Thanks for all the comments! PTAL

https://codereview.chromium.org/1269443002/diff/100001/Source/bindings/IDLExt...
File Source/bindings/IDLExtendedAttributes.txt (right):

https://codereview.chromium.org/1269443002/diff/100001/Source/bindings/IDLExt...
Source/bindings/IDLExtendedAttributes.txt:57: FlexibleArrayBufferView
On 2015/07/30 14:54:57, haraken wrote:
> 
> Please update the chromium wiki:
> 
> https://www.chromium.org/blink/webidl/blink-idl-extended-attributes

Will update the docs as soon as this is in.

https://codereview.chromium.org/1269443002/diff/100001/Source/bindings/core/v...
File Source/bindings/core/v8/V8Binding.h (right):

https://codereview.chromium.org/1269443002/diff/100001/Source/bindings/core/v...
Source/bindings/core/v8/V8Binding.h:66: class FlexibleArrayBufferView;
On 2015/07/30 14:54:57, haraken wrote:
> 
> Alphabetical order.

Done.

https://codereview.chromium.org/1269443002/diff/100001/Source/bindings/core/v...
File Source/bindings/core/v8/V8BindingMacros.h (right):

https://codereview.chromium.org/1269443002/diff/100001/Source/bindings/core/v...
Source/bindings/core/v8/V8BindingMacros.h:49: #define
SMALL_ARRAY_BUFFER_VIEW_STORAGE(value) \
On 2015/07/30 14:54:57, haraken wrote:
> 
> I'd rename this to allocateFlexibleArrayBufferViewStorage().
> 

Done.

https://codereview.chromium.org/1269443002/diff/100001/Source/bindings/core/v...
Source/bindings/core/v8/V8BindingMacros.h:49: #define
SMALL_ARRAY_BUFFER_VIEW_STORAGE(value) \
On 2015/07/30 14:54:57, haraken wrote:
> 
> Add a comment about what this is doing.

Done.

https://codereview.chromium.org/1269443002/diff/100001/Source/bindings/core/v...
Source/bindings/core/v8/V8BindingMacros.h:50: (value->IsArrayBufferView() &&
(value.As<v8::ArrayBufferView>()->ByteLength() <= 128) ?
alloca(value.As<v8::ArrayBufferView>()->ByteLength()) : nullptr)
On 2015/07/30 14:30:06, jochen wrote:
> should we change the default for on-heap typed arrays in v8 to <= 128 as well?

Changed to 64. Let's keep it in sync with V8's typed_array_max_size_heap.

https://codereview.chromium.org/1269443002/diff/100001/Source/bindings/core/v...
Source/bindings/core/v8/V8BindingMacros.h:52: 
On 2015/07/30 14:30:06, jochen wrote:
> nit. only one empty line

Done.

https://codereview.chromium.org/1269443002/diff/100001/Source/bindings/script...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/1269443002/diff/100001/Source/bindings/script...
Source/bindings/scripts/v8_types.py:381: if base_idl_type == 'ArrayBufferView'
and 'FlexibleArrayBufferView' in extended_attributes:
On 2015/07/30 14:54:57, haraken wrote:
> 
> Can we add this to INCLUDES_FOR_TYPE instead of hardcoding include files here?
> 
> I think we can just unconditionally include 8ArrayBufferView.h and
> FlexibleArrayBufferView.h if the type is ArrayBufferView.

Done.

https://codereview.chromium.org/1269443002/diff/100001/Source/bindings/script...
Source/bindings/scripts/v8_types.py:384: if idl_type.base_type in
TYPED_ARRAY_TYPES and 'FlexibleArrayBufferView' in extended_attributes:
On 2015/07/30 14:54:57, haraken wrote:
> 
> Ditto.

For TYPED_ARRAY_TYPES we additionally need the
  bindings/<array_type>/v8/V8<array_type>.h

Returning a union here. If I miss something to write this better just tell me.

https://codereview.chromium.org/1269443002/diff/100001/Source/bindings/script...
Source/bindings/scripts/v8_types.py:569: base_idl_type =
'FlexibleArrayBufferView'
On 2015/07/30 14:54:57, haraken wrote:
> 
> Shouldn't this be:
> 
>   'Flexible' + idl_type.base_type + 'View'
> 
> ?

Nope, both, FlexibleArrayBufferView and
TypedFlexibleArrayBufferView<WTFTypedArray>, filled using the same method: to
FlexibleArrayBufferView (line 512 this file).

https://codereview.chromium.org/1269443002/diff/100001/Source/bindings/script...
Source/bindings/scripts/v8_types.py:669: elif idl_type.base_type in
TYPED_ARRAY_TYPES and 'FlexibleArrayBufferView' in extended_attributes:
On 2015/07/30 14:54:57, haraken wrote:
> 
> Slightly better:
> 
> elif 'FlexibleArrayBufferView' in extended_attributes:
>   raise "..." if idl_type.base_type != 'ArrayBufferView' or idl_type.base_type
> in TYPED_ARRAY_TYPES
>   set_expression = cpp_calue

Done. Rewrote using regular if though as your suggested solutions is apparently
not valid syntax :)

https://codereview.chromium.org/1269443002/diff/100001/Source/core/dom/Flexib...
File Source/core/dom/FlexibleArrayBufferView.h (right):

https://codereview.chromium.org/1269443002/diff/100001/Source/core/dom/Flexib...
Source/core/dom/FlexibleArrayBufferView.h:16: // that is only valid during the
life-time of the FlexibleArrayBufferView object.
On 2015/07/30 14:54:57, haraken wrote:
> 
> Just to confirm: Does the "life-time of the FlexibleArrayBufferView object"
mean
> "while we're on stack"?
> 

Yes.

> I'd rename baseAddress() to something like baseAddressMaybeOnStack() to make
the
> danger clearer.

Done.

https://codereview.chromium.org/1269443002/diff/100001/Source/core/dom/Flexib...
Source/core/dom/FlexibleArrayBufferView.h:20:
WTF_MAKE_NONCOPYABLE(FlexibleArrayBufferView);
On 2015/07/30 14:54:57, haraken wrote:
> 
> Add WTF_MAKE_FAST_ALLOCATED().

Current use case is only stack allocated, so I guess it should be
STACK_ALLOCATED()

https://codereview.chromium.org/1269443002/diff/100001/Source/core/dom/Flexib...
Source/core/dom/FlexibleArrayBufferView.h:23: : m_smallData(nullptr),
m_smallLength(0)
On 2015/07/30 14:30:06, jochen wrote:
> nit. each initializer on its own line:
> 
> : m_smallData()
> , m_smallLength()

Done.

https://codereview.chromium.org/1269443002/diff/100001/Source/core/dom/Flexib...
Source/core/dom/FlexibleArrayBufferView.h:36: unsigned byteOffset() const {
ASSERT(!isEmpty()); return isFull() ? m_full->byteOffset() : 0; }
On 2015/07/30 14:54:57, haraken wrote:
> 
> Is it ok to return 0 for byteOffset()?

yes

https://codereview.chromium.org/1269443002/diff/100001/Source/core/dom/Flexib...
Source/core/dom/FlexibleArrayBufferView.h:40: void operator= (std::nullptr_t) {
m_full = nullptr; m_smallData = nullptr; }
On 2015/07/30 14:54:57, haraken wrote:
> 
> We normally don't implement operator=(nullptr). It would be better to rename
> this to clear().

Done.

https://codereview.chromium.org/1269443002/diff/100001/Source/core/dom/TypedF...
File Source/core/dom/TypedFlexibleArrayBufferView.h (right):

https://codereview.chromium.org/1269443002/diff/100001/Source/core/dom/TypedF...
Source/core/dom/TypedFlexibleArrayBufferView.h:17:
WTF_MAKE_NONCOPYABLE(TypedFlexibleArrayBufferView);
On 2015/07/30 14:54:57, haraken wrote:
> 
> Add WTF_MAKE_FAST_ALLOCATED.

STACK_ALLOCATED() (?)

https://codereview.chromium.org/1269443002/diff/100001/Source/core/dom/TypedF...
Source/core/dom/TypedFlexibleArrayBufferView.h:19: typedef typename
WTFTypedArray::ValueType ValueType;
On 2015/07/30 14:54:57, haraken wrote:
> 
> Use 'using'.

Done.

https://codereview.chromium.org/1269443002/diff/100001/Source/core/dom/TypedF...
Source/core/dom/TypedFlexibleArrayBufferView.h:23: ValueType* data() const {
return static_cast<ValueType*>(baseAddress()); }
On 2015/07/30 14:54:57, haraken wrote:
> 
> I'd rename this to dataMaybeOnStack() or something.

Done.

https://codereview.chromium.org/1269443002/diff/100001/Source/core/dom/TypedF...
Source/core/dom/TypedFlexibleArrayBufferView.h:28: typedef
TypedFlexibleArrayBufferView<WTF::Int32Array> FlexibleInt32ArrayView;
On 2015/07/30 14:54:57, haraken wrote:
> 
> Blink prefers 'using' in the new coding style.
> 

Done.

[haraken, 2015-07-30 20:34:44]

LGTM

https://codereview.chromium.org/1269443002/diff/110001/Source/bindings/core/v...
File Source/bindings/core/v8/V8Binding.cpp (right):

https://codereview.chromium.org/1269443002/diff/110001/Source/bindings/core/v...
Source/bindings/core/v8/V8Binding.cpp:763: if (!value->IsArrayBufferView())

Is this a valid situation? If not, we can change this to
ASSERT(value->IsArrayBufferView()).

https://codereview.chromium.org/1269443002/diff/110001/Source/bindings/core/v...
File Source/bindings/core/v8/V8Binding.h (right):

https://codereview.chromium.org/1269443002/diff/110001/Source/bindings/core/v...
Source/bindings/core/v8/V8Binding.h:979: // Use
SMALL_ARRAY_BUFFER_VIEW_STORAGE(v8Value) to allocate it using alloca() in

SMALL_ARRAY_BUFFER_VIEW_STORAGE => allocateFlexibleArrayBufferViewStorage

https://codereview.chromium.org/1269443002/diff/110001/Source/bindings/core/v...
Source/bindings/core/v8/V8Binding.h:981: CORE_EXPORT void
toFlexibleArrayBufferView(v8::Isolate*, v8::Handle<v8::Value>,
FlexibleArrayBufferView&, void* storage = nullptr);

v8::Handle => v8::Local

Handle is deprecated. The same comment for other Handles in this CL.

https://codereview.chromium.org/1269443002/diff/110001/Source/bindings/script...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/1269443002/diff/110001/Source/bindings/script...
Source/bindings/scripts/v8_types.py:385: set(['bindings/%s/v8/V8%s.h' %
(component_dir[base_idl_type], base_idl_type)])

I'm wondering why we didn't need to include V8<array_type>.h before this CL but
need to include it after this CL.

https://codereview.chromium.org/1269443002/diff/110001/Source/bindings/script...
Source/bindings/scripts/v8_types.py:567: base_idl_type =
'FlexibleArrayBufferView'

if 'FlexibleArrayBufferView' in extended_attributes:
  if ...:
    raise ...
  base_idl_type = 'FlexibleArrayBufferView'

https://codereview.chromium.org/1269443002/diff/110001/Source/core/dom/Flexib...
File Source/core/dom/FlexibleArrayBufferView.h (right):

https://codereview.chromium.org/1269443002/diff/110001/Source/core/dom/Flexib...
Source/core/dom/FlexibleArrayBufferView.h:15: // WARNING: The pointer returned
by baseAddress() may point to temporary storage

baseAddress() => baseAddressMaybeOnStack()

Move this comment to just the above of baseAddressMaybeOnStack().

https://codereview.chromium.org/1269443002/diff/110001/Source/core/dom/Flexib...
Source/core/dom/FlexibleArrayBufferView.h:29: void setSmall(void* data, size_t
length) { m_smallData = data; m_smallLength = length; }

Write sentences in separate lines.

https://codereview.chromium.org/1269443002/diff/110001/Source/core/dom/Flexib...
Source/core/dom/FlexibleArrayBufferView.h:33: m_smallData = nullptr;

m_smallLength = 0;

just in case.

https://codereview.chromium.org/1269443002/diff/110001/Source/core/dom/Flexib...
Source/core/dom/FlexibleArrayBufferView.h:39: DOMArrayBufferView* full() const {
ASSERT(isFull()); return m_full.get(); }

Write sentences in separate lines.

https://codereview.chromium.org/1269443002/diff/110001/Source/core/dom/Flexib...
Source/core/dom/FlexibleArrayBufferView.h:43: unsigned byteLength() const {
ASSERT(!isEmpty()); return isFull() ? m_full->byteLength() : m_smallLength; }

Ditto.

https://codereview.chromium.org/1269443002/diff/110001/Source/core/dom/TypedF...
File Source/core/dom/TypedFlexibleArrayBufferView.h (right):

https://codereview.chromium.org/1269443002/diff/110001/Source/core/dom/TypedF...
Source/core/dom/TypedFlexibleArrayBufferView.h:24: unsigned length() const {
return byteLength() / sizeof(ValueType); }

Shall we add:

  ASSERT(byteLength() % sizeof(ValueType) == 0);

?

[Hannes Payer (out of office), 2015-07-31 07:01:00]

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

[Hannes Payer (out of office), 2015-07-31 07:01:01]

https://codereview.chromium.org/1269443002/diff/110001/Source/bindings/core/v...
File Source/bindings/core/v8/V8BindingMacros.h (right):

https://codereview.chromium.org/1269443002/diff/110001/Source/bindings/core/v...
Source/bindings/core/v8/V8BindingMacros.h:50: // below a certain size limit. If
below the limit allocates memory on the stack
If below the limit memory is allocated on...

[Michael Lippautz, 2015-07-31 07:53:39]

Thanks a lot!

https://codereview.chromium.org/1269443002/diff/110001/Source/bindings/core/v...
File Source/bindings/core/v8/V8Binding.cpp (right):

https://codereview.chromium.org/1269443002/diff/110001/Source/bindings/core/v...
Source/bindings/core/v8/V8Binding.cpp:763: if (!value->IsArrayBufferView())
On 2015/07/30 20:34:43, haraken wrote:
> 
> Is this a valid situation? If not, we can change this to
> ASSERT(value->IsArrayBufferView()).

As far as I see, it should always be an ArrayBufferView. Turned it into an
assertion.

https://codereview.chromium.org/1269443002/diff/110001/Source/bindings/core/v...
File Source/bindings/core/v8/V8Binding.h (right):

https://codereview.chromium.org/1269443002/diff/110001/Source/bindings/core/v...
Source/bindings/core/v8/V8Binding.h:979: // Use
SMALL_ARRAY_BUFFER_VIEW_STORAGE(v8Value) to allocate it using alloca() in
On 2015/07/30 20:34:44, haraken wrote:
> 
> SMALL_ARRAY_BUFFER_VIEW_STORAGE => allocateFlexibleArrayBufferViewStorage

Done.

https://codereview.chromium.org/1269443002/diff/110001/Source/bindings/core/v...
Source/bindings/core/v8/V8Binding.h:981: CORE_EXPORT void
toFlexibleArrayBufferView(v8::Isolate*, v8::Handle<v8::Value>,
FlexibleArrayBufferView&, void* storage = nullptr);
On 2015/07/30 20:34:44, haraken wrote:
> 
> v8::Handle => v8::Local
> 
> Handle is deprecated. The same comment for other Handles in this CL.

Done.

https://codereview.chromium.org/1269443002/diff/110001/Source/bindings/core/v...
File Source/bindings/core/v8/V8BindingMacros.h (right):

https://codereview.chromium.org/1269443002/diff/110001/Source/bindings/core/v...
Source/bindings/core/v8/V8BindingMacros.h:50: // below a certain size limit. If
below the limit allocates memory on the stack
On 2015/07/31 07:01:00, Hannes Payer wrote:
> If below the limit memory is allocated on...

Done.

https://codereview.chromium.org/1269443002/diff/110001/Source/bindings/script...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/1269443002/diff/110001/Source/bindings/script...
Source/bindings/scripts/v8_types.py:385: set(['bindings/%s/v8/V8%s.h' %
(component_dir[base_idl_type], base_idl_type)])
On 2015/07/30 20:34:44, haraken wrote:
> 
> I'm wondering why we didn't need to include V8<array_type>.h before this CL
but
> need to include it after this CL.

As TYPED_ARRAY_TYPES didn't get specialized handling so far, the fall-through
case (line 401) included the corresponding headers till now.

https://codereview.chromium.org/1269443002/diff/110001/Source/bindings/script...
Source/bindings/scripts/v8_types.py:567: base_idl_type =
'FlexibleArrayBufferView'
On 2015/07/30 20:34:44, haraken wrote:
> 
> if 'FlexibleArrayBufferView' in extended_attributes:
>   if ...:
>     raise ...
>   base_idl_type = 'FlexibleArrayBufferView'

Done.

https://codereview.chromium.org/1269443002/diff/110001/Source/core/dom/Flexib...
File Source/core/dom/FlexibleArrayBufferView.h (right):

https://codereview.chromium.org/1269443002/diff/110001/Source/core/dom/Flexib...
Source/core/dom/FlexibleArrayBufferView.h:15: // WARNING: The pointer returned
by baseAddress() may point to temporary storage
On 2015/07/30 20:34:44, haraken wrote:
> 
> baseAddress() => baseAddressMaybeOnStack()
> 
> Move this comment to just the above of baseAddressMaybeOnStack().

Done.

https://codereview.chromium.org/1269443002/diff/110001/Source/core/dom/Flexib...
Source/core/dom/FlexibleArrayBufferView.h:29: void setSmall(void* data, size_t
length) { m_smallData = data; m_smallLength = length; }
On 2015/07/30 20:34:44, haraken wrote:
> 
> Write sentences in separate lines.

Done.

https://codereview.chromium.org/1269443002/diff/110001/Source/core/dom/Flexib...
Source/core/dom/FlexibleArrayBufferView.h:33: m_smallData = nullptr;
On 2015/07/30 20:34:44, haraken wrote:
> 
> m_smallLength = 0;
> 
> just in case.

Done.

https://codereview.chromium.org/1269443002/diff/110001/Source/core/dom/Flexib...
Source/core/dom/FlexibleArrayBufferView.h:39: DOMArrayBufferView* full() const {
ASSERT(isFull()); return m_full.get(); }
On 2015/07/30 20:34:44, haraken wrote:
> 
> Write sentences in separate lines.

Done.

https://codereview.chromium.org/1269443002/diff/110001/Source/core/dom/Flexib...
Source/core/dom/FlexibleArrayBufferView.h:43: unsigned byteLength() const {
ASSERT(!isEmpty()); return isFull() ? m_full->byteLength() : m_smallLength; }
On 2015/07/30 20:34:44, haraken wrote:
> 
> Ditto.

Done.

https://codereview.chromium.org/1269443002/diff/110001/Source/core/dom/TypedF...
File Source/core/dom/TypedFlexibleArrayBufferView.h (right):

https://codereview.chromium.org/1269443002/diff/110001/Source/core/dom/TypedF...
Source/core/dom/TypedFlexibleArrayBufferView.h:24: unsigned length() const {
return byteLength() / sizeof(ValueType); }
On 2015/07/30 20:34:44, haraken wrote:
> 
> Shall we add:
> 
>   ASSERT(byteLength() % sizeof(ValueType) == 0);
> 
> ?

Good idea, done.

[haraken, 2015-07-31 07:56:21]

Still LGTM

[Michael Lippautz, 2015-07-31 07:56:31]

The CQ bit was checked by mlippautz@chromium.org

[Michael Lippautz, 2015-07-31 07:56:32]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1269443002/#ps130001
(title: "Addressed comments: round 2")

[commit-bot: I haz the power, 2015-07-31 07:56:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1269443002/130001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1269443002/130001

[commit-bot: I haz the power, 2015-07-31 08:10:22]

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

[commit-bot: I haz the power, 2015-07-31 08:10:23]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/64583)

[Michael Lippautz, 2015-07-31 08:18:46]

The CQ bit was unchecked by mlippautz@chromium.org

[Michael Lippautz, 2015-07-31 08:18:47]

The CQ bit was checked by mlippautz@chromium.org

[Michael Lippautz, 2015-07-31 08:18:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1269443002/#ps150001
(title: "Fixed typo")

[Michael Lippautz, 2015-07-31 08:18:48]

The CQ bit was checked by mlippautz@chromium.org

[commit-bot: I haz the power, 2015-07-31 08:19:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1269443002/150001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1269443002/150001

[commit-bot: I haz the power, 2015-07-31 09:43:41]

Committed patchset #9 (id:150001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=199809

[Ken Russell (switch to Gerrit), 2015-07-31 22:17:37]

Sorry for the delay looking at this. Please also include bajones@ and zmo@ on
WebGL reviews. WebGL changes LGTM after the fact.

[bajones, 2015-07-31 23:19:10]

On 2015/07/31 22:17:37, Ken Russell wrote:
> Sorry for the delay looking at this. Please also include bajones@ and zmo@ on
> WebGL reviews. WebGL changes LGTM after the fact.

Post-commit LGTM as well. Happy to see work happening in this area!