Update client side navigator.connect API to use ServicePortCollection [1/3]

Update client side navigator.connect API to use ServicePortCollection [1/3]

This is part of a series of patches to update the client side API of what was
formerly known as navigator.connect to follow the new spec based on
navigator.services and a new ServicePort type.

This patch adds the new client side API to blink, and updates tests to work with
either the old or new API. Also deletes the system-services test as it is unclear
in what form that functionality will be kept.

[1/3] This patch
[2/3] https://codereview.chromium.org/1192713004
      Content side changes to implement the new API
[3/3] https://codereview.chromium.org/1198653004/
      Cleanup and removing old code paths on the blink side and in tests.

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

[Marijn Kruisselbrink, 2015-06-22 17:40:54]

Patchset #1 (id:1) has been deleted

[Marijn Kruisselbrink, 2015-06-22 17:41:01]

Patchset #1 (id:20001) has been deleted

[Marijn Kruisselbrink, 2015-06-22 17:41:08]

Patchset #1 (id:40001) has been deleted

[Marijn Kruisselbrink, 2015-06-22 17:41:13]

Patchset #1 (id:60001) has been deleted

[Marijn Kruisselbrink, 2015-06-22 17:41:20]

Patchset #1 (id:80001) has been deleted

[Marijn Kruisselbrink, 2015-06-22 17:47:48]

mek@chromium.org changed reviewers:
+ scheib@chromium.org

[scheib, 2015-06-24 04:16:55]

https://codereview.chromium.org/1191393003/diff/100001/LayoutTests/http/tests...
File LayoutTests/http/tests/navigatorconnect/resources/test-helpers.js (right):

https://codereview.chromium.org/1191393003/diff/100001/LayoutTests/http/tests...
LayoutTests/http/tests/navigatorconnect/resources/test-helpers.js:98: function
sequential_promise_test(func, name) {
Instead of duplicating this, move the definition to e.g.
LayoutTests/resources/sequentialPromiseTest.js

https://codereview.chromium.org/1191393003/diff/100001/Source/modules/navigat...
File Source/modules/navigatorconnect/ServicePort.cpp (right):

https://codereview.chromium.org/1191393003/diff/100001/Source/modules/navigat...
Source/modules/navigatorconnect/ServicePort.cpp:81: m_serializedData =
SerializedScriptValueFactory::instance().createFromWire(m_port.data);
Just checking, but there can be different contexts when the data is later
deserialized? Just curious why it's deserialized each time it is accessed from
the attribute.

https://codereview.chromium.org/1191393003/diff/100001/public/platform/module...
File public/platform/modules/navigator_services/WebServicePort.h (right):

https://codereview.chromium.org/1191393003/diff/100001/public/platform/module...
public/platform/modules/navigator_services/WebServicePort.h:16: WebURL
targetUrl;
// Members corresponding to ServicePort.idl attributes.

https://codereview.chromium.org/1191393003/diff/100001/public/platform/module...
public/platform/modules/navigator_services/WebServicePort.h:19: WebServicePortID
id = -1;
Any attributes about this id? Unique across all of chrome? shared with the other
side of the port? Maybe just simple to separate it from the others, // Unique ID
to this port.

https://codereview.chromium.org/1191393003/diff/100001/public/platform/module...
File public/platform/modules/navigator_services/WebServicePortProvider.h
(right):

https://codereview.chromium.org/1191393003/diff/100001/public/platform/module...
public/platform/modules/navigator_services/WebServicePortProvider.h:37: virtual
void postMessage(WebServicePortID, const WebString&,
WebMessagePortChannelArray*) { BLINK_ASSERT_NOT_REACHED(); }
I think: // Takes ownership of WebMessagePortChannelArray

[Marijn Kruisselbrink, 2015-06-24 17:03:26]

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

[Marijn Kruisselbrink, 2015-06-24 17:03:27]

+haraken: is my understanding/explanation as to why Notification.data
deserializes its value on every access (and why I've done the same in
ServicePort) correct or am I completely misunderstanding how the v8 bindings
work

https://codereview.chromium.org/1191393003/diff/100001/LayoutTests/http/tests...
File LayoutTests/http/tests/navigatorconnect/resources/test-helpers.js (right):

https://codereview.chromium.org/1191393003/diff/100001/LayoutTests/http/tests...
LayoutTests/http/tests/navigatorconnect/resources/test-helpers.js:98: function
sequential_promise_test(func, name) {
On 2015/06/24 at 04:16:55, scheib wrote:
> Instead of duplicating this, move the definition to e.g.
LayoutTests/resources/sequentialPromiseTest.js

It would have to be under LayoutTests/http/tests/resources (and
testharness-helpers.js is probably the place to put it then), but that is
probably better for a separate CL. I'm also not sure why bluetooth tests decided
to duplicate this from fetch/service worker tests. Would probably make sense to
deduplicate all of those.

https://codereview.chromium.org/1191393003/diff/100001/Source/modules/navigat...
File Source/modules/navigatorconnect/ServicePort.cpp (right):

https://codereview.chromium.org/1191393003/diff/100001/Source/modules/navigat...
Source/modules/navigatorconnect/ServicePort.cpp:81: m_serializedData =
SerializedScriptValueFactory::instance().createFromWire(m_port.data);
On 2015/06/24 at 04:16:55, scheib wrote:
> Just checking, but there can be different contexts when the data is later
deserialized? Just curious why it's deserialized each time it is accessed from
the attribute.

Great question, I should probably add a comment, although I'm not entirely sure
myself. To start with, this is the same thing Notification.data does
(https://codereview.chromium.org/993893002), although that is a terrible reason
without understanding why that code does it that way of course. My limited
understanding of bindings, v8 etc is that this possibly could indeed be called
from multiple contexts. As I understand it (but my understanding could be
completely wrong) the same ServicePort instance can get accessed from multiple
isolated worlds, and this should return a ScriptValue that was deserialized in
the world it is returned to (partly because SerializedScriptValueFactory uses
ScriptState::current() to deserialize the message, and deserializing can
potentially have side effects/cause arbitrary code to be executed in that
context).

This could probably use caching similar to how MessageEvent caches its
deserialized script values of course, I'll add a TODO for that.

https://codereview.chromium.org/1191393003/diff/100001/public/platform/module...
File public/platform/modules/navigator_services/WebServicePort.h (right):

https://codereview.chromium.org/1191393003/diff/100001/public/platform/module...
public/platform/modules/navigator_services/WebServicePort.h:16: WebURL
targetUrl;
On 2015/06/24 at 04:16:55, scheib wrote:
> // Members corresponding to ServicePort.idl attributes.

Done

https://codereview.chromium.org/1191393003/diff/100001/public/platform/module...
public/platform/modules/navigator_services/WebServicePort.h:19: WebServicePortID
id = -1;
On 2015/06/24 at 04:16:55, scheib wrote:
> Any attributes about this id? Unique across all of chrome? shared with the
other side of the port? Maybe just simple to separate it from the others, //
Unique ID to this port.

Done

[scheib, 2015-06-24 23:00:49]

LGTM

https://codereview.chromium.org/1191393003/diff/100001/LayoutTests/http/tests...
File LayoutTests/http/tests/navigatorconnect/resources/test-helpers.js (right):

https://codereview.chromium.org/1191393003/diff/100001/LayoutTests/http/tests...
LayoutTests/http/tests/navigatorconnect/resources/test-helpers.js:98: function
sequential_promise_test(func, name) {
On 2015/06/24 17:03:27, Marijn Kruisselbrink wrote:
> On 2015/06/24 at 04:16:55, scheib wrote:
> > Instead of duplicating this, move the definition to e.g.
> LayoutTests/resources/sequentialPromiseTest.js
> 
> It would have to be under LayoutTests/http/tests/resources (and
> testharness-helpers.js is probably the place to put it then), but that is
> probably better for a separate CL. I'm also not sure why bluetooth tests
decided
> to duplicate this from fetch/service worker tests. Would probably make sense
to
> deduplicate all of those.

Seems worthwhile to do -- a precursor CL or in this CL seems fine.

[Marijn Kruisselbrink, 2015-06-24 23:42:48]

https://codereview.chromium.org/1191393003/diff/100001/LayoutTests/http/tests...
File LayoutTests/http/tests/navigatorconnect/resources/test-helpers.js (right):

https://codereview.chromium.org/1191393003/diff/100001/LayoutTests/http/tests...
LayoutTests/http/tests/navigatorconnect/resources/test-helpers.js:98: function
sequential_promise_test(func, name) {
On 2015/06/24 at 23:00:48, scheib wrote:
> On 2015/06/24 17:03:27, Marijn Kruisselbrink wrote:
> > On 2015/06/24 at 04:16:55, scheib wrote:
> > > Instead of duplicating this, move the definition to e.g.
> > LayoutTests/resources/sequentialPromiseTest.js
> > 
> > It would have to be under LayoutTests/http/tests/resources (and
> > testharness-helpers.js is probably the place to put it then), but that is
> > probably better for a separate CL. I'm also not sure why bluetooth tests
decided
> > to duplicate this from fetch/service worker tests. Would probably make sense
to
> > deduplicate all of those.
> 
> Seems worthwhile to do -- a precursor CL or in this CL seems fine.

Will do, but in a separate CL, since adjusting all existing tests will make that
a somewhat significantly sized CL, even though the two different implementations
are mostly API compatible.

[Marijn Kruisselbrink, 2015-06-24 23:42:59]

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

[Marijn Kruisselbrink, 2015-06-24 23:43:00]

+tkent for public/ OWNERS

[haraken, 2015-06-24 23:50:32]

https://codereview.chromium.org/1191393003/diff/100001/Source/modules/navigat...
File Source/modules/navigatorconnect/ServicePort.cpp (right):

https://codereview.chromium.org/1191393003/diff/100001/Source/modules/navigat...
Source/modules/navigatorconnect/ServicePort.cpp:81: m_serializedData =
SerializedScriptValueFactory::instance().createFromWire(m_port.data);
On 2015/06/24 17:03:27, Marijn Kruisselbrink wrote:
> On 2015/06/24 at 04:16:55, scheib wrote:
> > Just checking, but there can be different contexts when the data is later
> deserialized? Just curious why it's deserialized each time it is accessed from
> the attribute.
> 
> Great question, I should probably add a comment, although I'm not entirely
sure
> myself. To start with, this is the same thing Notification.data does
> (https://codereview.chromium.org/993893002), although that is a terrible
reason
> without understanding why that code does it that way of course. My limited
> understanding of bindings, v8 etc is that this possibly could indeed be called
> from multiple contexts. As I understand it (but my understanding could be
> completely wrong) the same ServicePort instance can get accessed from multiple
> isolated worlds, and this should return a ScriptValue that was deserialized in
> the world it is returned to (partly because SerializedScriptValueFactory uses
> ScriptState::current() to deserialize the message, and deserializing can
> potentially have side effects/cause arbitrary code to be executed in that
> context).
> 
> This could probably use caching similar to how MessageEvent caches its
> deserialized script values of course, I'll add a TODO for that.

Yes, your understanding is correct. Given that we must not leak ScriptValues
between isolated worlds, we must serialize/deserialize the value. Basically a
C++ DOM object must not hold a ScriptValue (unless you're understanding what
you're doing :-) because the C++ DOM object is shared between multiple isolated
worlds.

Another reason is that ScriptValue holds a strong Persistent reference from
Blink to V8. This has a risk of producing leak (this "leak" means "memory
leak"). SerializedScriptValue doesn't have a risk.

[haraken, 2015-06-24 23:59:58]

haraken@chromium.org changed reviewers:
+ oilpan-reviews@chromium.org

[haraken, 2015-06-24 23:59:59]

https://codereview.chromium.org/1191393003/diff/160001/Source/modules/navigat...
File Source/modules/navigatorconnect/ServicePort.cpp (right):

https://codereview.chromium.org/1191393003/diff/160001/Source/modules/navigat...
Source/modules/navigatorconnect/ServicePort.cpp:45: // investigate caching this
similar to how MessageEvent caches data.

If this really becomes a performance problem, we can consider caching it
somehow. However, it is going to be very complicated. The way MessageEvent
caches it is not already correct -- it causes a memory leak :)

https://codereview.chromium.org/1191393003/diff/160001/Source/modules/navigat...
File Source/modules/navigatorconnect/ServicePort.h (right):

https://codereview.chromium.org/1191393003/diff/160001/Source/modules/navigat...
Source/modules/navigatorconnect/ServicePort.h:22: typedef
WillBeHeapVector<RefPtrWillBeMember<MessagePort>, 1> MessagePortArray;

Nit: It is a bit unfortunate we duplicate this typedef in multiple places in the
code base...

https://codereview.chromium.org/1191393003/diff/160001/Source/modules/navigat...
Source/modules/navigatorconnect/ServicePort.h:44: explicit
ServicePort(WeakPtrWillBeRawPtr<ServicePortCollection>, const WebServicePort&);

Drop explicit.

https://codereview.chromium.org/1191393003/diff/160001/Source/modules/navigat...
Source/modules/navigatorconnect/ServicePort.h:49:
WeakPtrWillBeWeakMember<ServicePortCollection> m_collection;

Would you help me understand why this needs to be a weak pointer? Can
ServicePort outlive ServicePortCollection? It sounds a bit strange to me.

https://codereview.chromium.org/1191393003/diff/160001/Source/modules/navigat...
File Source/modules/navigatorconnect/ServicePortCollection.h (right):

https://codereview.chromium.org/1191393003/diff/160001/Source/modules/navigat...
Source/modules/navigatorconnect/ServicePortCollection.h:23: class MODULES_EXPORT
ServicePortCollection final

Maybe it's better to move this class to Oilpan by default. By doing that you can
remove the complexity of WeakPtr from this CL.

You can make this RefCountedGarbageCollectedEventTargetWithInlineData.

https://codereview.chromium.org/1191393003/diff/160001/Source/modules/navigat...
Source/modules/navigatorconnect/ServicePortCollection.h:47:
WeakPtr<ServicePortCollection> weakPtr() { return m_weakFactory.createWeakPtr();
}

Then you can remove this.

https://codereview.chromium.org/1191393003/diff/160001/Source/modules/navigat...
Source/modules/navigatorconnect/ServicePortCollection.h:71:
PersistentHeapVectorWillBeHeapVector<Member<ServicePort>> m_ports;

This can be HeapVector<Member<>>.

https://codereview.chromium.org/1191393003/diff/160001/Source/modules/navigat...
Source/modules/navigatorconnect/ServicePortCollection.h:73:
WeakPtrFactory<ServicePortCollection> m_weakFactory;

And you can remove this.

[tkent, 2015-06-25 00:07:57]

public lgtm with nits

https://codereview.chromium.org/1191393003/diff/160001/public/platform/module...
File public/platform/modules/navigator_services/WebServicePortProvider.h
(right):

https://codereview.chromium.org/1191393003/diff/160001/public/platform/module...
public/platform/modules/navigator_services/WebServicePortProvider.h:16: typedef
WebVector<class WebMessagePortChannel*> WebMessagePortChannelArray;
typdef -> using

https://codereview.chromium.org/1191393003/diff/160001/public/platform/module...
public/platform/modules/navigator_services/WebServicePortProvider.h:18: typedef
WebCallbacks<WebServicePortID, void> WebServicePortConnectCallbacks;
typedef -> using

https://codereview.chromium.org/1191393003/diff/160001/public/platform/module...
public/platform/modules/navigator_services/WebServicePortProvider.h:44:
protected:
add a blank line before protected:

https://codereview.chromium.org/1191393003/diff/160001/public/platform/module...
public/platform/modules/navigator_services/WebServicePortProvider.h:45: virtual
~WebServicePortProvider() { }
nit: now we allow to omit a space between { and }.

https://codereview.chromium.org/1191393003/diff/160001/public/platform/module...
File public/platform/modules/navigator_services/WebServicePortProviderClient.h
(right):

https://codereview.chromium.org/1191393003/diff/160001/public/platform/module...
public/platform/modules/navigator_services/WebServicePortProviderClient.h:21:
virtual ~WebServicePortProviderClient() { }
nit: we may omit a space between { and }

[Marijn Kruisselbrink, 2015-06-25 01:07:26]

https://codereview.chromium.org/1191393003/diff/160001/Source/modules/navigat...
File Source/modules/navigatorconnect/ServicePort.cpp (right):

https://codereview.chromium.org/1191393003/diff/160001/Source/modules/navigat...
Source/modules/navigatorconnect/ServicePort.cpp:45: // investigate caching this
similar to how MessageEvent caches data.
On 2015/06/24 at 23:59:59, haraken wrote:
> If this really becomes a performance problem, we can consider caching it
somehow. However, it is going to be very complicated. The way MessageEvent
caches it is not already correct -- it causes a memory leak :)

Ah, okay, I'll just remove that comment for now then.

https://codereview.chromium.org/1191393003/diff/160001/Source/modules/navigat...
File Source/modules/navigatorconnect/ServicePort.h (right):

https://codereview.chromium.org/1191393003/diff/160001/Source/modules/navigat...
Source/modules/navigatorconnect/ServicePort.h:22: typedef
WillBeHeapVector<RefPtrWillBeMember<MessagePort>, 1> MessagePortArray;
On 2015/06/24 at 23:59:59, haraken wrote:
> Nit: It is a bit unfortunate we duplicate this typedef in multiple places in
the code base...

Agreed, I changed it to just include one of the headers that already have that
typedef.

https://codereview.chromium.org/1191393003/diff/160001/Source/modules/navigat...
Source/modules/navigatorconnect/ServicePort.h:44: explicit
ServicePort(WeakPtrWillBeRawPtr<ServicePortCollection>, const WebServicePort&);
On 2015/06/24 at 23:59:59, haraken wrote:
> Drop explicit.

Done

https://codereview.chromium.org/1191393003/diff/160001/Source/modules/navigat...
Source/modules/navigatorconnect/ServicePort.h:49:
WeakPtrWillBeWeakMember<ServicePortCollection> m_collection;
On 2015/06/24 at 23:59:59, haraken wrote:
> Would you help me understand why this needs to be a weak pointer? Can
ServicePort outlive ServicePortCollection? It sounds a bit strange to me.

With ServicePortCollection being refcounted I had to somehow break the circular
reference between ServicePort and ServicePortCollection (at least I think I had
assertions fail/memory leaks before changing that to be a WeakPtr). But now SPC
is garbage collected as well I think I can indeed just make this a regular
Member<>.

https://codereview.chromium.org/1191393003/diff/160001/Source/modules/navigat...
File Source/modules/navigatorconnect/ServicePortCollection.h (right):

https://codereview.chromium.org/1191393003/diff/160001/Source/modules/navigat...
Source/modules/navigatorconnect/ServicePortCollection.h:23: class MODULES_EXPORT
ServicePortCollection final
On 2015/06/24 at 23:59:59, haraken wrote:
> Maybe it's better to move this class to Oilpan by default. By doing that you
can remove the complexity of WeakPtr from this CL.
> 
> You can make this RefCountedGarbageCollectedEventTargetWithInlineData.

Yes, that would be much nicer. Done.

https://codereview.chromium.org/1191393003/diff/160001/Source/modules/navigat...
Source/modules/navigatorconnect/ServicePortCollection.h:47:
WeakPtr<ServicePortCollection> weakPtr() { return m_weakFactory.createWeakPtr();
}
On 2015/06/24 at 23:59:59, haraken wrote:
> Then you can remove this.

Done

https://codereview.chromium.org/1191393003/diff/160001/Source/modules/navigat...
Source/modules/navigatorconnect/ServicePortCollection.h:71:
PersistentHeapVectorWillBeHeapVector<Member<ServicePort>> m_ports;
On 2015/06/24 at 23:59:59, haraken wrote:
> This can be HeapVector<Member<>>.

Done

https://codereview.chromium.org/1191393003/diff/160001/Source/modules/navigat...
Source/modules/navigatorconnect/ServicePortCollection.h:73:
WeakPtrFactory<ServicePortCollection> m_weakFactory;
On 2015/06/24 at 23:59:59, haraken wrote:
> And you can remove this.

Done

https://codereview.chromium.org/1191393003/diff/160001/public/platform/module...
File public/platform/modules/navigator_services/WebServicePortProvider.h
(right):

https://codereview.chromium.org/1191393003/diff/160001/public/platform/module...
public/platform/modules/navigator_services/WebServicePortProvider.h:18: typedef
WebCallbacks<WebServicePortID, void> WebServicePortConnectCallbacks;
On 2015/06/25 at 00:07:57, tkent wrote:
> typedef -> using

Done

https://codereview.chromium.org/1191393003/diff/160001/public/platform/module...
public/platform/modules/navigator_services/WebServicePortProvider.h:44:
protected:
On 2015/06/25 at 00:07:57, tkent wrote:
> add a blank line before protected:

Done

https://codereview.chromium.org/1191393003/diff/160001/public/platform/module...
public/platform/modules/navigator_services/WebServicePortProvider.h:45: virtual
~WebServicePortProvider() { }
On 2015/06/25 at 00:07:57, tkent wrote:
> nit: now we allow to omit a space between { and }.

Done

https://codereview.chromium.org/1191393003/diff/160001/public/platform/module...
File public/platform/modules/navigator_services/WebServicePortProviderClient.h
(right):

https://codereview.chromium.org/1191393003/diff/160001/public/platform/module...
public/platform/modules/navigator_services/WebServicePortProviderClient.h:21:
virtual ~WebServicePortProviderClient() { }
On 2015/06/25 at 00:07:57, tkent wrote:
> nit: we may omit a space between { and }

Done

[haraken, 2015-06-25 01:21:40]

LGTM in terms of oilpan and bindings.

https://codereview.chromium.org/1191393003/diff/160001/Source/modules/navigat...
File Source/modules/navigatorconnect/ServicePort.h (right):

https://codereview.chromium.org/1191393003/diff/160001/Source/modules/navigat...
Source/modules/navigatorconnect/ServicePort.h:49:
WeakPtrWillBeWeakMember<ServicePortCollection> m_collection;
On 2015/06/25 01:07:26, Marijn Kruisselbrink wrote:
> On 2015/06/24 at 23:59:59, haraken wrote:
> > Would you help me understand why this needs to be a weak pointer? Can
> ServicePort outlive ServicePortCollection? It sounds a bit strange to me.
> 
> With ServicePortCollection being refcounted I had to somehow break the
circular
> reference between ServicePort and ServicePortCollection (at least I think I
had
> assertions fail/memory leaks before changing that to be a WeakPtr). But now
SPC
> is garbage collected as well I think I can indeed just make this a regular
> Member<>.

Cool!

https://codereview.chromium.org/1191393003/diff/180001/Source/modules/navigat...
File Source/modules/navigatorconnect/ServicePort.cpp (right):

https://codereview.chromium.org/1191393003/diff/180001/Source/modules/navigat...
Source/modules/navigatorconnect/ServicePort.cpp:41: return
ScriptValue(scriptState, m_serializedData->deserialize(scriptState->isolate()));

binding team: desrialize should take a ScriptState parameter instead of getting
ScriptState::current in deserialize. Conceptually deserialize is a
ScriptState-dependent method.

https://codereview.chromium.org/1191393003/diff/180001/Source/modules/navigat...
Source/modules/navigatorconnect/ServicePort.cpp:67: if (m_collection) {

This null check won't be needed.

https://codereview.chromium.org/1191393003/diff/180001/Source/modules/navigat...
File Source/modules/navigatorconnect/ServicePortCollection.h (right):

https://codereview.chromium.org/1191393003/diff/180001/Source/modules/navigat...
Source/modules/navigatorconnect/ServicePortCollection.h:15: #include
"wtf/WeakPtr.h"

Remove this.

[Marijn Kruisselbrink, 2015-06-25 02:18:55]

https://codereview.chromium.org/1191393003/diff/180001/Source/modules/navigat...
File Source/modules/navigatorconnect/ServicePort.cpp (right):

https://codereview.chromium.org/1191393003/diff/180001/Source/modules/navigat...
Source/modules/navigatorconnect/ServicePort.cpp:67: if (m_collection) {
On 2015/06/25 at 01:21:40, haraken wrote:
> This null check won't be needed.

Done

https://codereview.chromium.org/1191393003/diff/180001/Source/modules/navigat...
File Source/modules/navigatorconnect/ServicePortCollection.h (right):

https://codereview.chromium.org/1191393003/diff/180001/Source/modules/navigat...
Source/modules/navigatorconnect/ServicePortCollection.h:15: #include
"wtf/WeakPtr.h"
On 2015/06/25 at 01:21:40, haraken wrote:
> Remove this.
Done

[Marijn Kruisselbrink, 2015-06-25 03:21:26]

The CQ bit was checked by mek@chromium.org

[Marijn Kruisselbrink, 2015-06-25 03:21:26]

The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org,
tkent@chromium.org, haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1191393003/#ps200001
(title: "address more comments")

[commit-bot: I haz the power, 2015-06-25 03:21:34]

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

[commit-bot: I haz the power, 2015-06-25 03:25:04]

Committed patchset #6 (id:200001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197794