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
+haraken: is my understanding/explanation as to why Notification.data deserializes its value on every access (and ...
4 years, 10 months ago
(2015-06-24 17:03:27 UTC)
#10
+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
LGTM https://codereview.chromium.org/1191393003/diff/100001/LayoutTests/http/tests/navigatorconnect/resources/test-helpers.js File LayoutTests/http/tests/navigatorconnect/resources/test-helpers.js (right): https://codereview.chromium.org/1191393003/diff/100001/LayoutTests/http/tests/navigatorconnect/resources/test-helpers.js#newcode98 LayoutTests/http/tests/navigatorconnect/resources/test-helpers.js:98: function sequential_promise_test(func, name) { On 2015/06/24 17:03:27, Marijn ...
4 years, 10 months ago
(2015-06-24 23:00:49 UTC)
#11
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
https://codereview.chromium.org/1191393003/diff/100001/LayoutTests/http/tests/navigatorconnect/resources/test-helpers.js File LayoutTests/http/tests/navigatorconnect/resources/test-helpers.js (right): https://codereview.chromium.org/1191393003/diff/100001/LayoutTests/http/tests/navigatorconnect/resources/test-helpers.js#newcode98 LayoutTests/http/tests/navigatorconnect/resources/test-helpers.js:98: function sequential_promise_test(func, name) { On 2015/06/24 at 23:00:48, scheib ...
4 years, 10 months ago
(2015-06-24 23:42:48 UTC)
#12
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.
4 years, 10 months ago
(2015-06-24 23:50:32 UTC)
#15
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.
https://codereview.chromium.org/1191393003/diff/160001/Source/modules/navigatorconnect/ServicePort.cpp File Source/modules/navigatorconnect/ServicePort.cpp (right): https://codereview.chromium.org/1191393003/diff/160001/Source/modules/navigatorconnect/ServicePort.cpp#newcode45 Source/modules/navigatorconnect/ServicePort.cpp:45: // investigate caching this similar to how MessageEvent caches ...
4 years, 10 months ago
(2015-06-24 23:59:59 UTC)
#17
https://codereview.chromium.org/1191393003/diff/160001/Source/modules/navigatorconnect/ServicePort.cpp File Source/modules/navigatorconnect/ServicePort.cpp (right): https://codereview.chromium.org/1191393003/diff/160001/Source/modules/navigatorconnect/ServicePort.cpp#newcode45 Source/modules/navigatorconnect/ServicePort.cpp:45: // investigate caching this similar to how MessageEvent caches ...
4 years, 10 months ago
(2015-06-25 01:07:26 UTC)
#19
Issue 1191393003: Update client side navigator.connect API to use ServicePortCollection [1/3]
(Closed)
Created 4 years, 10 months ago by Marijn Kruisselbrink
Modified 4 years, 10 months ago
Reviewers: haraken, oilpan-reviews, scheib, tkent
Base URL: https://chromium.googlesource.com/chromium/blink.git@serviceport
Comments: 43
Chromium Code Reviews
