Sharing of SharedArrayBuffer via PostMessage transfer

Sharing of SharedArrayBuffer via PostMessage transfer.

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

[binji, 2015-06-05 22:02:25]

binji@chromium.org changed reviewers:
+ jochen@chromium.org

[binji, 2015-06-08 17:23:37]

PTAL.

I'm not sure about adding a new base class for DOMArrayBuffer, but it seemed to
simplify some of the changes. Is there a better way?

[haraken, 2015-06-08 23:40:24]

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

[haraken, 2015-06-08 23:40:25]

+yukishiino for DOMArrayBuffer stuff

[Yuki, 2015-06-09 06:18:08]

https://codereview.chromium.org/1097773004/diff/60001/Source/core/dom/DOMShar...
File Source/core/dom/DOMSharedArrayBuffer.cpp (right):

https://codereview.chromium.org/1097773004/diff/60001/Source/core/dom/DOMShar...
Source/core/dom/DOMSharedArrayBuffer.cpp:14: v8::Handle<v8::Object>
DOMSharedArrayBuffer::wrap(v8::Isolate* isolate, v8::Handle<v8::Object>
creationContext)
v8::Handle is obsolete.  Please use v8::Local.

https://codereview.chromium.org/1097773004/diff/60001/Source/core/dom/DOMShar...
File Source/core/dom/DOMSharedArrayBuffer.h (right):

https://codereview.chromium.org/1097773004/diff/60001/Source/core/dom/DOMShar...
Source/core/dom/DOMSharedArrayBuffer.h:38: virtual v8::Handle<v8::Object>
wrap(v8::Isolate*, v8::Handle<v8::Object> creationContext) override;
nit: 'virtual' is no longer needed when you have 'override'.  The style guide
changed.

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

https://codereview.chromium.org/1097773004/diff/60001/Source/core/dom/DOMType...
Source/core/dom/DOMTypedArray.cpp:36: if (isShared()) {
Why don't you use DOMArrayBufferView::bufferBase() that you introduced in this
CL?  You don't need any if-branch, I think.

RefPtr<DOMArrayBufferBase> buffer = this->bufferBase();
v8::Local<v8::Value> v8Buffer = toV8(buffer.get(), creationContext, isolate);
// optional check if you want
if (ENABLE_ASSERT) {
  if (isShared())
    ASSERT(v8Buffer->IsSharedArrayBuffer());
  else
    ASSERT(v8Buffer->IsArrayBuffer());
}
// or
ASSERT(isShared() == v8Buffer->IsSharedArrayBuffer());
ASSERT(v8Buffer->IsArrayBuffer());  // SharedArrayBuffer is an ArrayBuffer,
right?  Otherwise, !isShared() == v8Buffer->IsArrayBuffer().

https://codereview.chromium.org/1097773004/diff/60001/Source/core/dom/DOMType...
Source/core/dom/DOMTypedArray.cpp:48: if (isShared()) {
Declare |wrapper| just before its use.

https://codereview.chromium.org/1097773004/diff/60001/Source/wtf/ArrayBufferC...
File Source/wtf/ArrayBufferContents.cpp (right):

https://codereview.chromium.org/1097773004/diff/60001/Source/wtf/ArrayBufferC...
Source/wtf/ArrayBufferContents.cpp:104: other.m_holder->m_sizeInBytes =
m_holder->m_sizeInBytes;
I'd recommend to use adopt() or create a new instance.

allocateMemory(size, ..., data);
other.m_holder->adopt(data, size, ...);

or

other.m_holder.reset(adoptRef(new DataHolder(data, size, ...)));

Then, you don't need setters for each member and also copyToWithoutBuffer.

https://codereview.chromium.org/1097773004/diff/60001/Source/wtf/ArrayBufferC...
Source/wtf/ArrayBufferContents.cpp:139: m_data = data;
ASSERT(!m_data); ?

It seems you're assuming that the instance does not hold memory.

https://codereview.chromium.org/1097773004/diff/60001/Source/wtf/ArrayBufferC...
File Source/wtf/ArrayBufferContents.h (right):

https://codereview.chromium.org/1097773004/diff/60001/Source/wtf/ArrayBufferC...
Source/wtf/ArrayBufferContents.h:47: ArrayBufferContents(unsigned numElements,
unsigned elementByteSize, bool isShared,
ArrayBufferContents::InitializationPolicy);
Our policy (style guide) prefers enum value to boolean value.
For example,
  enum Sharable {NON_SHARED_BUFFER, SHARED_BUFFER};
  ArrayBufferContents(..., Sharable isShared, ...);

Just as an example.  I expect better names ;)

https://codereview.chromium.org/1097773004/diff/60001/Source/wtf/ArrayBufferC...
Source/wtf/ArrayBufferContents.h:96: void* m_data;
Maybe you think it's overkilling, but I'd recommend to have regular accessors.

public:
  void* data() const { return m_data; }
private:
  void* m_data;

Then, it's crystal clear that only adopt() changes the contents.

[binji, 2015-06-10 06:04:39]

https://codereview.chromium.org/1097773004/diff/60001/Source/core/dom/DOMShar...
File Source/core/dom/DOMSharedArrayBuffer.cpp (right):

https://codereview.chromium.org/1097773004/diff/60001/Source/core/dom/DOMShar...
Source/core/dom/DOMSharedArrayBuffer.cpp:14: v8::Handle<v8::Object>
DOMSharedArrayBuffer::wrap(v8::Isolate* isolate, v8::Handle<v8::Object>
creationContext)
On 2015/06/09 at 06:18:07, Yuki wrote:
> v8::Handle is obsolete.  Please use v8::Local.

Done.

https://codereview.chromium.org/1097773004/diff/60001/Source/core/dom/DOMShar...
File Source/core/dom/DOMSharedArrayBuffer.h (right):

https://codereview.chromium.org/1097773004/diff/60001/Source/core/dom/DOMShar...
Source/core/dom/DOMSharedArrayBuffer.h:38: virtual v8::Handle<v8::Object>
wrap(v8::Isolate*, v8::Handle<v8::Object> creationContext) override;
On 2015/06/09 at 06:18:07, Yuki wrote:
> nit: 'virtual' is no longer needed when you have 'override'.  The style guide
changed.

Done.

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

https://codereview.chromium.org/1097773004/diff/60001/Source/core/dom/DOMType...
Source/core/dom/DOMTypedArray.cpp:36: if (isShared()) {
On 2015/06/09 at 06:18:07, Yuki wrote:
> Why don't you use DOMArrayBufferView::bufferBase() that you introduced in this
CL?  You don't need any if-branch, I think.

I only added it recently, but I've tried to simplify the CL by using it.

> 
> RefPtr<DOMArrayBufferBase> buffer = this->bufferBase();
> v8::Local<v8::Value> v8Buffer = toV8(buffer.get(), creationContext, isolate);
> // optional check if you want
> if (ENABLE_ASSERT) {
>   if (isShared())
>     ASSERT(v8Buffer->IsSharedArrayBuffer());
>   else
>     ASSERT(v8Buffer->IsArrayBuffer());
> }
> // or
> ASSERT(isShared() == v8Buffer->IsSharedArrayBuffer());
> ASSERT(v8Buffer->IsArrayBuffer());  // SharedArrayBuffer is an ArrayBuffer,
right?  Otherwise, !isShared() == v8Buffer->IsArrayBuffer().

Done.

https://codereview.chromium.org/1097773004/diff/60001/Source/core/dom/DOMType...
Source/core/dom/DOMTypedArray.cpp:48: if (isShared()) {
On 2015/06/09 at 06:18:07, Yuki wrote:
> Declare |wrapper| just before its use.

Done.

https://codereview.chromium.org/1097773004/diff/60001/Source/wtf/ArrayBufferC...
File Source/wtf/ArrayBufferContents.cpp (right):

https://codereview.chromium.org/1097773004/diff/60001/Source/wtf/ArrayBufferC...
Source/wtf/ArrayBufferContents.cpp:104: other.m_holder->m_sizeInBytes =
m_holder->m_sizeInBytes;
On 2015/06/09 at 06:18:08, Yuki wrote:
> I'd recommend to use adopt() or create a new instance.
> 
> allocateMemory(size, ..., data);
> other.m_holder->adopt(data, size, ...);
> 
> or
> 
> other.m_holder.reset(adoptRef(new DataHolder(data, size, ...)));
> 
> Then, you don't need setters for each member and also copyToWithoutBuffer.

Done.

https://codereview.chromium.org/1097773004/diff/60001/Source/wtf/ArrayBufferC...
Source/wtf/ArrayBufferContents.cpp:139: m_data = data;
On 2015/06/09 at 06:18:08, Yuki wrote:
> ASSERT(!m_data); ?
> 
> It seems you're assuming that the instance does not hold memory.

Done.

https://codereview.chromium.org/1097773004/diff/60001/Source/wtf/ArrayBufferC...
File Source/wtf/ArrayBufferContents.h (right):

https://codereview.chromium.org/1097773004/diff/60001/Source/wtf/ArrayBufferC...
Source/wtf/ArrayBufferContents.h:47: ArrayBufferContents(unsigned numElements,
unsigned elementByteSize, bool isShared,
ArrayBufferContents::InitializationPolicy);
On 2015/06/09 at 06:18:08, Yuki wrote:
> Our policy (style guide) prefers enum value to boolean value.
> For example,
>   enum Sharable {NON_SHARED_BUFFER, SHARED_BUFFER};
>   ArrayBufferContents(..., Sharable isShared, ...);
> 
> Just as an example.  I expect better names ;)

Done.

https://codereview.chromium.org/1097773004/diff/60001/Source/wtf/ArrayBufferC...
Source/wtf/ArrayBufferContents.h:96: void* m_data;
On 2015/06/09 at 06:18:08, Yuki wrote:
> Maybe you think it's overkilling, but I'd recommend to have regular accessors.
> 
> public:
>   void* data() const { return m_data; }
> private:
>   void* m_data;
> 
> Then, it's crystal clear that only adopt() changes the contents.

Done.

[Yuki, 2015-06-10 07:44:56]

https://codereview.chromium.org/1097773004/diff/100001/Source/core/dom/DOMTyp...
File Source/core/dom/DOMTypedArray.cpp (right):

https://codereview.chromium.org/1097773004/diff/100001/Source/core/dom/DOMTyp...
Source/core/dom/DOMTypedArray.cpp:36: v8Buffer = toV8(buffer.get(),
creationContext, isolate);
nit: You can write as follows.  Initialization at declaration is preferred.

v8::Local<v8::Value> v8Buffer = toV8(...);

https://codereview.chromium.org/1097773004/diff/100001/Source/core/dom/DOMTyp...
Source/core/dom/DOMTypedArray.cpp:37: ASSERT(isShared() ==
v8Buffer->IsSharedArrayBuffer());
This line and below should be swapped, otherwise you'll access to the empty
handle's member.

if (v8Buffer.IsEmpty())
    return ...;
ASSERT(isShared() == v8Buffer->IsSharedArrayBuffer());

https://codereview.chromium.org/1097773004/diff/100001/Source/wtf/ArrayBuffer...
File Source/wtf/ArrayBufferContents.cpp (right):

https://codereview.chromium.org/1097773004/diff/100001/Source/wtf/ArrayBuffer...
Source/wtf/ArrayBufferContents.cpp:152: other.allocateNew(m_sizeInBytes,
m_isShared, DontInitialize, m_deallocationObserver);
I'm concerned about the (implicit) change of the API spec.

The old implementation does:
    other.freeMemory(other.m_data, other.m_sizeInBytes);
before allocating a new memory where the data will be copied.
However, your new implementation actually does:
    ASSERT(!other.m_data);
    allocateMemory(sizeInBytes, policy, other.m_data);

Since ArrayBufferContents's ctor allocates memory even if the size is zero in
order to give it a unique address, I'm afraid that it's not always true that
other.m_data == nullptr.  It's safer to call freeMemory, isn't it?  Or, are you
sure it's always true that other.m_data == nullptr?

[binji, 2015-06-10 20:53:40]

https://codereview.chromium.org/1097773004/diff/100001/Source/core/dom/DOMTyp...
File Source/core/dom/DOMTypedArray.cpp (right):

https://codereview.chromium.org/1097773004/diff/100001/Source/core/dom/DOMTyp...
Source/core/dom/DOMTypedArray.cpp:36: v8Buffer = toV8(buffer.get(),
creationContext, isolate);
On 2015/06/10 at 07:44:55, Yuki wrote:
> nit: You can write as follows.  Initialization at declaration is preferred.
> 
> v8::Local<v8::Value> v8Buffer = toV8(...);

Done.

https://codereview.chromium.org/1097773004/diff/100001/Source/core/dom/DOMTyp...
Source/core/dom/DOMTypedArray.cpp:37: ASSERT(isShared() ==
v8Buffer->IsSharedArrayBuffer());
On 2015/06/10 at 07:44:55, Yuki wrote:
> This line and below should be swapped, otherwise you'll access to the empty
handle's member.
> 
> if (v8Buffer.IsEmpty())
>     return ...;
> ASSERT(isShared() == v8Buffer->IsSharedArrayBuffer());

Done.

https://codereview.chromium.org/1097773004/diff/100001/Source/wtf/ArrayBuffer...
File Source/wtf/ArrayBufferContents.cpp (right):

https://codereview.chromium.org/1097773004/diff/100001/Source/wtf/ArrayBuffer...
Source/wtf/ArrayBufferContents.cpp:152: other.allocateNew(m_sizeInBytes,
m_isShared, DontInitialize, m_deallocationObserver);
On 2015/06/10 at 07:44:55, Yuki wrote:
> I'm concerned about the (implicit) change of the API spec.
> 
> The old implementation does:
>     other.freeMemory(other.m_data, other.m_sizeInBytes);
> before allocating a new memory where the data will be copied.
> However, your new implementation actually does:
>     ASSERT(!other.m_data);
>     allocateMemory(sizeInBytes, policy, other.m_data);
> 
> Since ArrayBufferContents's ctor allocates memory even if the size is zero in
order to give it a unique address, I'm afraid that it's not always true that
other.m_data == nullptr.  It's safer to call freeMemory, isn't it?  Or, are you
sure it's always true that other.m_data == nullptr?

Yes, you're right. Done.

[Yuki, 2015-06-11 05:16:14]

lgtm for DOMArrayBuffer-related files.

https://codereview.chromium.org/1097773004/diff/120001/Source/wtf/ArrayBuffer...
File Source/wtf/ArrayBufferContents.cpp (right):

https://codereview.chromium.org/1097773004/diff/120001/Source/wtf/ArrayBuffer...
Source/wtf/ArrayBufferContents.cpp:130: allocateMemory(sizeInBytes, policy,
data);
nit: Better to be consistent with the same style.
You sometimes write ArrayBufferContents::allocateMemory/freeMemory and sometimes
not.

[haraken, 2015-06-11 05:59:29]

- It seems this CL is doing multiple things in one go. Can we split the CL into
pieces? (e.g., Add shared buffer, add transfer etc)

- I think that the deallocationObserver is making things complicated. Would it
be possible to remove the deallocationObserver? It was introduced a long time
ago because we didn't want to report memory of ArrayBuffers that do not (yet)
have wrappers to V8. The underlying idea was that we don't want to report memory
that is not guaranteed to be freed by V8 GC to V8. However, the assumption has
changed, and now we're reporting Blink's memory to V8 even if the memory is not
guaranteed to be freed by V8 GC. We can do the same thing for ArrayBuffers. In
other words, we can just report the memory in ArrayBuffer::create() etc. Then we
can remove the deallocationObserver. (It's great if we can remove the
deallocationObserver before landing this CL, since it's hard to verify that this
CL is handling the deallocationObserver correctly.)

https://codereview.chromium.org/1097773004/diff/120001/Source/bindings/core/v...
File Source/bindings/core/v8/ScriptValueSerializer.cpp (right):

https://codereview.chromium.org/1097773004/diff/120001/Source/bindings/core/v...
Source/bindings/core/v8/ScriptValueSerializer.cpp:30: #include <stdio.h>

Remove this.

https://codereview.chromium.org/1097773004/diff/120001/Source/bindings/core/v...
Source/bindings/core/v8/ScriptValueSerializer.cpp:1930: result =
toV8(buffer.get(), creationContext, isolate);

You need to add:

  if (result.isEmpty())
    return false;

https://codereview.chromium.org/1097773004/diff/120001/Source/bindings/core/v...
File Source/bindings/core/v8/SerializationTag.h (right):

https://codereview.chromium.org/1097773004/diff/120001/Source/bindings/core/v...
Source/bindings/core/v8/SerializationTag.h:63: // TODO(binji): better tag
characters? Most of the good ones are already taken.

Remove this comment.

https://codereview.chromium.org/1097773004/diff/120001/Source/bindings/core/v...
File Source/bindings/core/v8/SerializedScriptValue.cpp (right):

https://codereview.chromium.org/1097773004/diff/120001/Source/bindings/core/v...
Source/bindings/core/v8/SerializedScriptValue.cpp:123: if
(!arrayBuffers[i]->isShared()) {

Avoid if(!f()) {} else {}. Instead you can use if(f()) {} else {}.

https://codereview.chromium.org/1097773004/diff/120001/Source/bindings/core/v...
File Source/bindings/core/v8/V8Initializer.cpp (right):

https://codereview.chromium.org/1097773004/diff/120001/Source/bindings/core/v...
Source/bindings/core/v8/V8Initializer.cpp:362: // TODO(binji): where should this
go?

I think this is a good place to put this. You can remove the comment.

https://codereview.chromium.org/1097773004/diff/120001/Source/bindings/core/v...
Source/bindings/core/v8/V8Initializer.cpp:364: const char kSharedArrayBuffer[] =
"--harmony-sharedarraybuffer";

Add static.

https://codereview.chromium.org/1097773004/diff/120001/Source/bindings/script...
File Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/1097773004/diff/120001/Source/bindings/script...
Source/bindings/scripts/v8_interface.py:98: if interface.name ==
'SharedArrayBuffer':

I'm not really happy about hard-coding a interface name in the IDL compiler.

Do we really need this branch? If is_array_buffer_or_view is true for
SharedArrayBuffer, we can just use the next branch.

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

https://codereview.chromium.org/1097773004/diff/120001/Source/bindings/script...
Source/bindings/scripts/v8_types.py:72: 'SharedArrayBuffer',

Alphabetical order.

https://codereview.chromium.org/1097773004/diff/120001/Source/bindings/tests/...
File Source/bindings/tests/results/core/V8ArrayBuffer.cpp (right):

https://codereview.chromium.org/1097773004/diff/120001/Source/bindings/tests/...
Source/bindings/tests/results/core/V8ArrayBuffer.cpp:14: #include
"bindings/core/v8/V8SharedArrayBuffer.h"

This won't be needed.

https://codereview.chromium.org/1097773004/diff/120001/Source/bindings/tests/...
File Source/bindings/tests/results/core/V8ArrayBufferView.cpp (right):

https://codereview.chromium.org/1097773004/diff/120001/Source/bindings/tests/...
Source/bindings/tests/results/core/V8ArrayBufferView.cpp:21: #include
"bindings/core/v8/V8SharedArrayBuffer.h"

This won't be needed.

https://codereview.chromium.org/1097773004/diff/120001/Source/bindings/tests/...
File Source/bindings/tests/results/core/V8DataView.cpp (right):

https://codereview.chromium.org/1097773004/diff/120001/Source/bindings/tests/...
Source/bindings/tests/results/core/V8DataView.cpp:14: #include
"bindings/core/v8/V8SharedArrayBuffer.h"

This won't be needed.

https://codereview.chromium.org/1097773004/diff/120001/Source/core/dom/DOMSha...
File Source/core/dom/DOMSharedArrayBuffer.cpp (right):

https://codereview.chromium.org/1097773004/diff/120001/Source/core/dom/DOMSha...
Source/core/dom/DOMSharedArrayBuffer.cpp:30: // TODO(binji): Should this be a
different observer? Perhaps we want to track shared memory separately.

Help me understand: Can the shared buffer be shared between isolates? If yes, we
should have a different deallocation observer for shared buffers. If no, we can
just use the same deallocation observer.

https://codereview.chromium.org/1097773004/diff/120001/Source/core/dom/DOMSha...
File Source/core/dom/DOMSharedArrayBuffer.h (right):

https://codereview.chromium.org/1097773004/diff/120001/Source/core/dom/DOMSha...
Source/core/dom/DOMSharedArrayBuffer.h:19: // TODO(binji): Would be nice to be
able to check this at compile time.

If you plan to fix this, you can leave the comment. But if this is just a memo
that is not serious, you can remove the comment.

https://codereview.chromium.org/1097773004/diff/120001/Source/core/dom/DOMSha...
Source/core/dom/DOMSharedArrayBuffer.h:33: // TODO(binji): Would be nice to be
able to check this at compile time.

Ditto.

https://codereview.chromium.org/1097773004/diff/120001/Source/core/dom/DOMTyp...
File Source/core/dom/DOMTypedArray.cpp (right):

https://codereview.chromium.org/1097773004/diff/120001/Source/core/dom/DOMTyp...
Source/core/dom/DOMTypedArray.cpp:44: wrapper =
V8TypedArray::New(v8Buffer.As<v8::SharedArrayBuffer>(), byteOffset(), length());

Is this correct? You are casting a non-shared buffer to v8::SharedArrayBuffer.

https://codereview.chromium.org/1097773004/diff/120001/Source/core/dom/Shared...
File Source/core/dom/SharedArrayBuffer.idl (right):

https://codereview.chromium.org/1097773004/diff/120001/Source/core/dom/Shared...
Source/core/dom/SharedArrayBuffer.idl:5: // TODO(binji): spec

Elaborate the comment a bit more. Comments are not your memo but for future
contributors :)

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

https://codereview.chromium.org/1097773004/diff/120001/Source/wtf/ArrayBuffer...
Source/wtf/ArrayBuffer.h:170: RELEASE_ASSERT(contents.data());

Shall we add ASSERT(contents.isShared())?

https://codereview.chromium.org/1097773004/diff/120001/Source/wtf/ArrayBuffer...
Source/wtf/ArrayBuffer.h:170: RELEASE_ASSERT(contents.data());

Any reason to make this a RELEASE assert?

https://codereview.chromium.org/1097773004/diff/120001/Source/wtf/ArrayBuffer...
Source/wtf/ArrayBuffer.h:179: RELEASE_ASSERT(contents.data());

Ditto.

https://codereview.chromium.org/1097773004/diff/120001/Source/wtf/ArrayBuffer...
Source/wtf/ArrayBuffer.h:217: // If we do support this, make sure to call
createShared instead of create below.

I'm just curious but do we need to make the sliced buffer shared? I guess we can
just create a non-shared buffer for the sliced buffer.

https://codereview.chromium.org/1097773004/diff/120001/Source/wtf/ArrayBuffer...
File Source/wtf/ArrayBufferContents.cpp (right):

https://codereview.chromium.org/1097773004/diff/120001/Source/wtf/ArrayBuffer...
Source/wtf/ArrayBufferContents.cpp:89: // TODO(binji): where is this function
used? It probably shouldn't be used

If it's unused, we can remove the function.

https://codereview.chromium.org/1097773004/diff/120001/Source/wtf/ArrayBuffer...
Source/wtf/ArrayBufferContents.cpp:117: if (m_data && m_deallocationObserver)

Is it possible that we have m_deallocationObserver but don't have m_data?

https://codereview.chromium.org/1097773004/diff/120001/Source/wtf/ArrayBuffer...
File Source/wtf/ArrayBufferContents.h (right):

https://codereview.chromium.org/1097773004/diff/120001/Source/wtf/ArrayBuffer...
Source/wtf/ArrayBufferContents.h:65: void* data() const { return m_holder ?
m_holder->data() : nullptr; }

Is it a valid thing to access these fields after the m_holder is cleared? I
guess we can change this to ASSERT(m_holder).

[jochen (gone - plz use gerrit), 2015-06-11 12:40:28]

https://codereview.chromium.org/1097773004/diff/120001/Source/bindings/core/v...
File Source/bindings/core/v8/V8Initializer.cpp (right):

https://codereview.chromium.org/1097773004/diff/120001/Source/bindings/core/v...
Source/bindings/core/v8/V8Initializer.cpp:364: const char kSharedArrayBuffer[] =
"--harmony-sharedarraybuffer";
I'd rather first see test coverage on the v8 side before we enable an
experimental feature in blink

[binji, 2015-07-09 18:06:14]

Updated this CL after landing https://codereview.chromium.org/1179373004,
https://codereview.chromium.org/1186863005 and
https://codereview.chromium.org/1183763003.

PTAL

[haraken, 2015-07-10 00:50:30]

Thanks for splitting the patch! Now the logic looks much clearer. LGTM.

[jochen (gone - plz use gerrit), 2015-07-10 09:07:40]

https://codereview.chromium.org/1097773004/diff/220001/Source/bindings/core/v...
File Source/bindings/core/v8/V8Initializer.cpp (right):

https://codereview.chromium.org/1097773004/diff/220001/Source/bindings/core/v...
Source/bindings/core/v8/V8Initializer.cpp:364:
v8::V8::SetFlagsFromString(kSharedArrayBuffer, sizeof(kSharedArrayBuffer));
we should first land this once shared array buffers are enabled in v8

[binji, 2015-07-10 18:34:46]

https://codereview.chromium.org/1097773004/diff/220001/Source/bindings/core/v...
File Source/bindings/core/v8/V8Initializer.cpp (right):

https://codereview.chromium.org/1097773004/diff/220001/Source/bindings/core/v...
Source/bindings/core/v8/V8Initializer.cpp:364:
v8::V8::SetFlagsFromString(kSharedArrayBuffer, sizeof(kSharedArrayBuffer));
On 2015/07/10 at 09:07:40, jochen wrote:
> we should first land this once shared array buffers are enabled in v8

Sorry, I don't understand. Is it dangerous to add a blink runtime flag? Should I
just set the status to "test" for now?

[jochen (gone - plz use gerrit), 2015-07-10 18:43:51]

On 2015/07/10 at 18:34:46, binji wrote:
>
https://codereview.chromium.org/1097773004/diff/220001/Source/bindings/core/v...
> File Source/bindings/core/v8/V8Initializer.cpp (right):
> 
>
https://codereview.chromium.org/1097773004/diff/220001/Source/bindings/core/v...
> Source/bindings/core/v8/V8Initializer.cpp:364:
v8::V8::SetFlagsFromString(kSharedArrayBuffer, sizeof(kSharedArrayBuffer));
> On 2015/07/10 at 09:07:40, jochen wrote:
> > we should first land this once shared array buffers are enabled in v8
> 
> Sorry, I don't understand. Is it dangerous to add a blink runtime flag? Should
I just set the status to "test" for now?

adding a runtime flag is fine. but passing a flag that enables a not yet
launched v8 feature is not ok, whether it's behind another flag or not.

before we expose shared array buffers to blink, we should decide to ship them in
v8.

[binji, 2015-07-10 18:48:53]

> adding a runtime flag is fine. but passing a flag that enables a not yet
launched v8 feature is not ok, whether it's behind another flag or not.
> 
> before we expose shared array buffers to blink, we should decide to ship them
in v8.

Thanks, I understand now.

[binji, 2015-07-15 23:44:38]

Discussed a path forward with jochen@ offline. Sounds like using a virtual test
suite is better than conditionally enabling a V8 flag.

I've also set the Blink runtime flag for SharedArrayBuffer to test, instead of
experimental, so it is only turned on for LayoutTests.

PTAL

[jochen (gone - plz use gerrit), 2015-07-16 10:47:25]

On 2015/07/15 at 23:44:38, binji wrote:
> Discussed a path forward with jochen@ offline. Sounds like using a virtual
test suite is better than conditionally enabling a V8 flag.
> 
> I've also set the Blink runtime flag for SharedArrayBuffer to test, instead of
experimental, so it is only turned on for LayoutTests.
> 
> PTAL

i'd prefer no status at all, and enabling the flag via the command line as well
(--enable-blink-features=...)

[binji, 2015-07-16 17:28:31]

On 2015/07/16 at 10:47:25, jochen wrote:
> On 2015/07/15 at 23:44:38, binji wrote:
> > Discussed a path forward with jochen@ offline. Sounds like using a virtual
test suite is better than conditionally enabling a V8 flag.
> > 
> > I've also set the Blink runtime flag for SharedArrayBuffer to test, instead
of experimental, so it is only turned on for LayoutTests.
> > 
> > PTAL
> 
> i'd prefer no status at all, and enabling the flag via the command line as
well (--enable-blink-features=...)

Done.

[jochen (gone - plz use gerrit), 2015-07-17 09:29:43]

lgtm

[binji, 2015-07-17 16:08:03]

The CQ bit was checked by binji@chromium.org

[binji, 2015-07-17 16:08:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
yukishiino@chromium.org, haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1097773004/#ps300001
(title: "additional flag in virtual test suite")

[commit-bot: I haz the power, 2015-07-17 16:08:25]

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

[commit-bot: I haz the power, 2015-07-17 16:12:37]

Committed patchset #16 (id:300001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=199111