DevTools: enable front-end to use external services for additional capabilities.

DevTools: enable front-end to use external services for additional capabilities.

BUG=649808
Committed: https://crrev.com/5ede094c7cd21744a44cc6880cf0c39d46c42f0e
Cr-Commit-Position: refs/heads/master@{#421415}

[pfeldman, 2016-09-26 19:49:47]

Description was changed from

==========
DevTools: [wipterm]

BUG=
==========

to

==========
DevTools: enable front-end to use external services for additional capabilities.

BUG=649808
==========

[pfeldman, 2016-09-26 19:49:47]

pfeldman@chromium.org changed reviewers:
+ dgozman@chromium.org

[pfeldman, 2016-09-27 19:09:03]

PTAL

[dgozman, 2016-09-27 21:01:35]

https://codereview.chromium.org/2370573002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/backend/devtools.js (right):

https://codereview.chromium.org/2370573002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/backend/devtools.js:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
Let's not call this "backend". Can we move it to services?

https://codereview.chromium.org/2370573002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/backend/dispatcher.js (right):

https://codereview.chromium.org/2370573002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/backend/dispatcher.js:16: start:
function(port)
Should we adopt JSDoc here for future?

https://codereview.chromium.org/2370573002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/backend/dispatcher.js:36:
_dispatchMessageWrapped: function(data)
Unused.

https://codereview.chromium.org/2370573002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/backend/dispatcher.js:59:
object.setNotify(this._notify.bind(this, id, objectName));
Maybe just pass notify in constructor?

https://codereview.chromium.org/2370573002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/backend/dispatcher.js:84:
object[method](message.params).then(result => this._sendResponse(message.id,
result));
Let's catch calls into object and send error response.

https://codereview.chromium.org/2370573002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/services/ServiceManager.js
(right):

https://codereview.chromium.org/2370573002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/services/ServiceManager.js:22:
lookupService: function(serviceName)
I'd say lookupService would return existing instance if any. This is more of
createService.

https://codereview.chromium.org/2370573002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/services/ServiceManager.js:26:
return null;
This is not compiled, because jscompiler requires null to be cast to ?T when it
does compile it.

https://codereview.chromium.org/2370573002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/services/ServiceManager.js:82: var
object = JSON.parse(data);
try catch this

https://codereview.chromium.org/2370573002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/services/ServiceManager.js:186:
WebInspector.serviceManager = new WebInspector.ServiceManager();
Let's make it lazy instance().

[pfeldman, 2016-09-27 21:48:59]

https://codereview.chromium.org/2370573002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/backend/devtools.js (right):

https://codereview.chromium.org/2370573002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/backend/devtools.js:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
On 2016/09/27 21:01:34, dgozman wrote:
> Let's not call this "backend". Can we move it to services?

Done.

https://codereview.chromium.org/2370573002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/backend/dispatcher.js (right):

https://codereview.chromium.org/2370573002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/backend/dispatcher.js:16: start:
function(port)
On 2016/09/27 21:01:34, dgozman wrote:
> Should we adopt JSDoc here for future?

We could, but it would not compiler requires above anyways...

https://codereview.chromium.org/2370573002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/backend/dispatcher.js:36:
_dispatchMessageWrapped: function(data)
On 2016/09/27 21:01:34, dgozman wrote:
> Unused.

Done.

https://codereview.chromium.org/2370573002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/backend/dispatcher.js:59:
object.setNotify(this._notify.bind(this, id, objectName));
On 2016/09/27 21:01:34, dgozman wrote:
> Maybe just pass notify in constructor?

Done.

https://codereview.chromium.org/2370573002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/backend/dispatcher.js:84:
object[method](message.params).then(result => this._sendResponse(message.id,
result));
On 2016/09/27 21:01:34, dgozman wrote:
> Let's catch calls into object and send error response.

Done.

https://codereview.chromium.org/2370573002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/services/ServiceManager.js
(right):

https://codereview.chromium.org/2370573002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/services/ServiceManager.js:22:
lookupService: function(serviceName)
On 2016/09/27 21:01:34, dgozman wrote:
> I'd say lookupService would return existing instance if any. This is more of
> createService.

Done.

https://codereview.chromium.org/2370573002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/services/ServiceManager.js:26:
return null;
On 2016/09/27 21:01:34, dgozman wrote:
> This is not compiled, because jscompiler requires null to be cast to ?T when
it
> does compile it.

It did compile though :)

https://codereview.chromium.org/2370573002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/services/ServiceManager.js:82: var
object = JSON.parse(data);
On 2016/09/27 21:01:34, dgozman wrote:
> try catch this

Done.

https://codereview.chromium.org/2370573002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/services/ServiceManager.js:186:
WebInspector.serviceManager = new WebInspector.ServiceManager();
On 2016/09/27 21:01:34, dgozman wrote:
> Let's make it lazy instance().

It is lazy along with its module.

[dgozman, 2016-09-27 22:25:41]

lgtm

https://codereview.chromium.org/2370573002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/services/ServiceManager.js
(right):

https://codereview.chromium.org/2370573002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/services/ServiceManager.js:86:
console.error(e);
return;

https://codereview.chromium.org/2370573002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/services/dispatcher.js (right):

https://codereview.chromium.org/2370573002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/services/dispatcher.js:57: var object = new
constructor(this._notify.bind(this, id, objectName));
Catch this one as well.

https://codereview.chromium.org/2370573002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/services/dispatcher.js:67:
object.dispose().then(() => this._sendResponse(message.id));
Catch this one as well.

https://codereview.chromium.org/2370573002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/services/dispatcher.js:85: } catch (e) {
Should also have promise.catch.

[pfeldman, 2016-09-27 23:00:31]

The CQ bit was checked by pfeldman@chromium.org

[pfeldman, 2016-09-27 23:00:31]

The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/2370573002/#ps140001
(title: "review addressed")

[commit-bot: I haz the power, 2016-09-27 23:00:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 01:24:24]

Description was changed from

==========
DevTools: enable front-end to use external services for additional capabilities.

BUG=649808
==========

to

==========
DevTools: enable front-end to use external services for additional capabilities.

BUG=649808
==========

[commit-bot: I haz the power, 2016-09-28 01:24:25]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-09-28 01:25:56]

Description was changed from

==========
DevTools: enable front-end to use external services for additional capabilities.

BUG=649808
==========

to

==========
DevTools: enable front-end to use external services for additional capabilities.

BUG=649808

Committed: https://crrev.com/5ede094c7cd21744a44cc6880cf0c39d46c42f0e
Cr-Commit-Position: refs/heads/master@{#421415}
==========

[commit-bot: I haz the power, 2016-09-28 01:25:57]

Patchset 8 (id:??) landed as
https://crrev.com/5ede094c7cd21744a44cc6880cf0c39d46c42f0e
Cr-Commit-Position: refs/heads/master@{#421415}