[DevTools] No NetworkManager and NetworkLog for v8only mode

[DevTools] No NetworkManager and NetworkLog for v8only mode

R=alph@chromium.org, dgozman@chromium.org
BUG=624494
Committed: https://crrev.com/33d4a941b4801e7cb2828600675751c35750b330
Cr-Commit-Position: refs/heads/master@{#404032}

[dgozman, 2016-06-29 18:14:36]

Please add a bug number reference (and create one if there is none).
Also, we usually prefix the title with DevTools, so that our changes are easy to
find.

[eostroukhov-old, 2016-06-29 18:36:53]

Description was changed from

==========
No NetworkManager and NetworkLog for v8only mode

R=alph@chromium.org, dgozman@chromium.org
BUG=
==========

to

==========
No NetworkManager and NetworkLog for v8only mode

R=alph@chromium.org, dgozman@chromium.org
BUG=624494
==========

[dgozman, 2016-06-29 18:37:58]

https://codereview.chromium.org/2109813003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/main/Main.js (right):

https://codereview.chromium.org/2109813003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/main/Main.js:314: else if
(Runtime.queryParam("v8only")) {
style: extra {}

https://codereview.chromium.org/2109813003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js
(right):

https://codereview.chromium.org/2109813003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:214:
WebInspector.NetworkLog.fromTarget(target).requests().forEach(this._appendRequest.bind(this));
What if there is no network log for this target?

https://codereview.chromium.org/2109813003/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js
(right):

https://codereview.chromium.org/2109813003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js:40:
var networkManager = this.target &&
WebInspector.NetworkManager.fromTarget(this.target);
this._target, not this.target
Do we have tests for this?

https://codereview.chromium.org/2109813003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/HAREntry.js (right):

https://codereview.chromium.org/2109813003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/HAREntry.js:70: var page =
networkLog && networkLog.pageLoadForRequest(this._request);
I think we should have |this.networkLog()| in NetworkRequest. It cannot exist
without one, and retrieving it like this is cumbersome. We can even store
networkLog instead of a target inside.

https://codereview.chromium.org/2109813003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/NetworkLog.js (right):

https://codereview.chromium.org/2109813003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkLog.js:57:
WebInspector.NetworkLog.fromTarget = function(target) {
style: { on next line

https://codereview.chromium.org/2109813003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js (right):

https://codereview.chromium.org/2109813003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js:66:
console.assert(this.dispose);
Why this check? We have dispose declared in this file below, right?

https://codereview.chromium.org/2109813003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js:95:
WebInspector.NetworkManager.fromTarget = function(target) {
style: { on next line
Doesn't eslint in presubmit catch this?

https://codereview.chromium.org/2109813003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/Target.js (right):

https://codereview.chromium.org/2109813003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/Target.js:29: /** @type
{!Array<!{dispose: !Function}>} */
If we go for this, Disposable should be an interface.

https://codereview.chromium.org/2109813003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/Target.js:40: V8Inspector: 8
I think it's better to use JS term instead of V8. But we already have "v8Only".
Let's add a TODO to rename them.

https://codereview.chromium.org/2109813003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/Target.js:168: for (var
disposable of this.disposables)
I think we can just listen for TargetDisposed event, and remove the listener
there to avoid a reference for GC?

https://codereview.chromium.org/2109813003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js (right):

https://codereview.chromium.org/2109813003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js:201: if
(!target.isV8Inspector()) {
style: extra {}

https://codereview.chromium.org/2109813003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js:224:
target.isDedicatedWorker() || target.isV8Inspector() ? null : new
WebInspector.WorkerManager(target);
Let's introduce Target.supportsWorkers().

https://codereview.chromium.org/2109813003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js:258:
console.assert(target);
Remove?

https://codereview.chromium.org/2109813003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js
(right):

https://codereview.chromium.org/2109813003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:254: if
(networkManager)
Page target must have a network manager. I wonder how we can make this better...

https://codereview.chromium.org/2109813003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:255: {
style: { on the same line

[eostroukhov-old, 2016-06-29 22:54:21]

Thanks for the review!

https://codereview.chromium.org/2109813003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/main/Main.js (right):

https://codereview.chromium.org/2109813003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/main/Main.js:314: else if
(Runtime.queryParam("v8only")) {
On 2016/06/29 18:37:57, dgozman wrote:
> style: extra {}

Done.

https://codereview.chromium.org/2109813003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js
(right):

https://codereview.chromium.org/2109813003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:214:
WebInspector.NetworkLog.fromTarget(target).requests().forEach(this._appendRequest.bind(this));
On 2016/06/29 18:37:57, dgozman wrote:
> What if there is no network log for this target?

My thinking was to let exception be thrown... I changed it to check for null
(both here and below)

https://codereview.chromium.org/2109813003/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js
(right):

https://codereview.chromium.org/2109813003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js:40:
var networkManager = this.target &&
WebInspector.NetworkManager.fromTarget(this.target);
On 2016/06/29 18:37:57, dgozman wrote:
> this._target, not this.target
> Do we have tests for this?

Looks like no tests detect missing toolbar button... Great catch!

https://codereview.chromium.org/2109813003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/HAREntry.js (right):

https://codereview.chromium.org/2109813003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/HAREntry.js:70: var page =
networkLog && networkLog.pageLoadForRequest(this._request);
On 2016/06/29 18:37:57, dgozman wrote:
> I think we should have |this.networkLog()| in NetworkRequest. It cannot exist
> without one, and retrieving it like this is cumbersome. We can even store
> networkLog instead of a target inside.

target() is inherited by NetworkRequest from the WebInspector.SDKObject

I introduced networkLog as an r/o NetworkRequest property.

https://codereview.chromium.org/2109813003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/NetworkLog.js (right):

https://codereview.chromium.org/2109813003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkLog.js:57:
WebInspector.NetworkLog.fromTarget = function(target) {
On 2016/06/29 18:37:57, dgozman wrote:
> style: { on next line

Done.

https://codereview.chromium.org/2109813003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js (right):

https://codereview.chromium.org/2109813003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js:66:
console.assert(this.dispose);
On 2016/06/29 18:37:58, dgozman wrote:
> Why this check? We have dispose declared in this file below, right?

Oops :) Debugging artifact.

https://codereview.chromium.org/2109813003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js:95:
WebInspector.NetworkManager.fromTarget = function(target) {
On 2016/06/29 18:37:58, dgozman wrote:
> style: { on next line
> Doesn't eslint in presubmit catch this?

Done.

I have not seen any linter warnings...

https://codereview.chromium.org/2109813003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/Target.js (right):

https://codereview.chromium.org/2109813003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/Target.js:29: /** @type
{!Array<!{dispose: !Function}>} */
On 2016/06/29 18:37:58, dgozman wrote:
> If we go for this, Disposable should be an interface.

Removed the array, listening to an event instead.

https://codereview.chromium.org/2109813003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/Target.js:40: V8Inspector: 8
On 2016/06/29 18:37:58, dgozman wrote:
> I think it's better to use JS term instead of V8. But we already have
"v8Only".
> Let's add a TODO to rename them.

Done.

https://codereview.chromium.org/2109813003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/Target.js:168: for (var
disposable of this.disposables)
On 2016/06/29 18:37:58, dgozman wrote:
> I think we can just listen for TargetDisposed event, and remove the listener
> there to avoid a reference for GC?

Done.

https://codereview.chromium.org/2109813003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js (right):

https://codereview.chromium.org/2109813003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js:201: if
(!target.isV8Inspector()) {
On 2016/06/29 18:37:58, dgozman wrote:
> style: extra {}

Done.

https://codereview.chromium.org/2109813003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js:224:
target.isDedicatedWorker() || target.isV8Inspector() ? null : new
WebInspector.WorkerManager(target);
On 2016/06/29 18:37:58, dgozman wrote:
> Let's introduce Target.supportsWorkers().

Done.

https://codereview.chromium.org/2109813003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js:258:
console.assert(target);
On 2016/06/29 18:37:58, dgozman wrote:
> Remove?

Done.

https://codereview.chromium.org/2109813003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js
(right):

https://codereview.chromium.org/2109813003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:254: if
(networkManager)
On 2016/06/29 18:37:58, dgozman wrote:
> Page target must have a network manager. I wonder how we can make this
better...

For me it is not obvious that Elements tab always means you should have a
Network tab... I think it is ok to allow page targets without network.

https://codereview.chromium.org/2109813003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:255: {
On 2016/06/29 18:37:58, dgozman wrote:
> style: { on the same line

Done.

[dgozman, 2016-06-30 17:24:33]

I think the more isolation we do, the less we need SDKObject. I'm inclined to
remove it entirely. Let's discuss that.

https://codereview.chromium.org/2109813003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js (right):

https://codereview.chromium.org/2109813003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js:66:
WebInspector.targetManager.addEventListener(WebInspector.TargetManager.Events.TargetDisposed,
this._targetDisposed, this);
Hmm... It's strange this is a TargetManager's event, not Target's. Should we
change that?

https://codereview.chromium.org/2109813003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js:128:
_targetDisposed: function(event)
I wonder whether we should put this functionality into SDKModel. Wouldn't hurt.

https://codereview.chromium.org/2109813003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js:132:
WebInspector.moduleSetting("cacheDisabled").removeChangeListener(this._cacheDisabledSettingChanged,
this);
Please also remove the listener for TargetDisposed, or else network manager will
never be collected due to the reference from TargetManager.

https://codereview.chromium.org/2109813003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js:132:
WebInspector.moduleSetting("cacheDisabled").removeChangeListener(this._cacheDisabledSettingChanged,
this);
Please also do the same for DebuggerModel. Thanks!

https://codereview.chromium.org/2109813003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/NetworkRequest.js (right):

https://codereview.chromium.org/2109813003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkRequest.js:1127:
this._initiatorRequest =
WebInspector.NetworkLog.fromTarget(this.target()).requestForURL(this.initiatorInfo().url);
this.networkLog()

https://codereview.chromium.org/2109813003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkRequest.js:1213: get
networkLog()
Make it a function. Getters are banned, the remaining ones are legacy.

https://codereview.chromium.org/2109813003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkRequest.js:1215: var
networkLog = WebInspector.NetworkLog.fromTarget(this.target());
Let's instead save it into a field in constructor.

https://codereview.chromium.org/2109813003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/Target.js (right):

https://codereview.chromium.org/2109813003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/Target.js:150: return
!this.isDedicatedWorker() && !this.isJSInspector();
Let's whitelist instead of blacklisting.

https://codereview.chromium.org/2109813003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js (right):

https://codereview.chromium.org/2109813003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js:201: if
(!target.isJSInspector()) {
style: extra {}

https://codereview.chromium.org/2109813003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js:208: if
(networkManager) {
ditto

https://codereview.chromium.org/2109813003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js
(right):

https://codereview.chromium.org/2109813003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:176:
originState.certificateDetailsPromise =
networkManager.certificateDetailsPromise(securityDetails.certificateId);
request.networkManager()

[eostroukhov-old, 2016-06-30 21:38:28]

Updated... Please take another look.

https://codereview.chromium.org/2109813003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js (right):

https://codereview.chromium.org/2109813003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js:66:
WebInspector.targetManager.addEventListener(WebInspector.TargetManager.Events.TargetDisposed,
this._targetDisposed, this);
On 2016/06/30 17:24:33, dgozman wrote:
> Hmm... It's strange this is a TargetManager's event, not Target's. Should we
> change that?

Discussed offline, nvm for now.

https://codereview.chromium.org/2109813003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js:128:
_targetDisposed: function(event)
On 2016/06/30 17:24:33, dgozman wrote:
> I wonder whether we should put this functionality into SDKModel. Wouldn't
hurt.

Done.

https://codereview.chromium.org/2109813003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js:132:
WebInspector.moduleSetting("cacheDisabled").removeChangeListener(this._cacheDisabledSettingChanged,
this);
On 2016/06/30 17:24:32, dgozman wrote:
> Please also do the same for DebuggerModel. Thanks!

Done.

https://codereview.chromium.org/2109813003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js:132:
WebInspector.moduleSetting("cacheDisabled").removeChangeListener(this._cacheDisabledSettingChanged,
this);
On 2016/06/30 17:24:33, dgozman wrote:
> Please also remove the listener for TargetDisposed, or else network manager
will
> never be collected due to the reference from TargetManager.

Done.

https://codereview.chromium.org/2109813003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/NetworkRequest.js (right):

https://codereview.chromium.org/2109813003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkRequest.js:1127:
this._initiatorRequest =
WebInspector.NetworkLog.fromTarget(this.target()).requestForURL(this.initiatorInfo().url);
On 2016/06/30 17:24:33, dgozman wrote:
> this.networkLog()

Done.

https://codereview.chromium.org/2109813003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkRequest.js:1213: get
networkLog()
On 2016/06/30 17:24:33, dgozman wrote:
> Make it a function. Getters are banned, the remaining ones are legacy.

Done.

https://codereview.chromium.org/2109813003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkRequest.js:1215: var
networkLog = WebInspector.NetworkLog.fromTarget(this.target());
On 2016/06/30 17:24:33, dgozman wrote:
> Let's instead save it into a field in constructor.

Done.

https://codereview.chromium.org/2109813003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/Target.js (right):

https://codereview.chromium.org/2109813003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/Target.js:150: return
!this.isDedicatedWorker() && !this.isJSInspector();
On 2016/06/30 17:24:33, dgozman wrote:
> Let's whitelist instead of blacklisting.

Done.

https://codereview.chromium.org/2109813003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js (right):

https://codereview.chromium.org/2109813003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js:201: if
(!target.isJSInspector()) {
On 2016/06/30 17:24:33, dgozman wrote:
> style: extra {}

Done.

https://codereview.chromium.org/2109813003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js:208: if
(networkManager) {
On 2016/06/30 17:24:33, dgozman wrote:
> ditto

Done.

https://codereview.chromium.org/2109813003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js
(right):

https://codereview.chromium.org/2109813003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:176:
originState.certificateDetailsPromise =
networkManager.certificateDetailsPromise(securityDetails.certificateId);
On 2016/06/30 17:24:33, dgozman wrote:
> request.networkManager()

Done.

[dgozman, 2016-07-05 17:46:34]

Description was changed from

==========
No NetworkManager and NetworkLog for v8only mode

R=alph@chromium.org, dgozman@chromium.org
BUG=624494
==========

to

==========
[DevTools] No NetworkManager and NetworkLog for v8only mode

R=alph@chromium.org, dgozman@chromium.org
BUG=624494
==========

[dgozman, 2016-07-05 17:54:31]

Looks pretty good!

https://codereview.chromium.org/2109813003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/NetworkLog.js (right):

https://codereview.chromium.org/2109813003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkLog.js:59: return
target[WebInspector.NetworkLog._symbol] || null;
Let's use target.model(WebInspector.NetworkLog). See DebuggerModel for an
example.

https://codereview.chromium.org/2109813003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js (right):

https://codereview.chromium.org/2109813003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js:95: return
target[WebInspector.NetworkManager._symbol] || null;
target.model(WebInspector.NetworkManager)

https://codereview.chromium.org/2109813003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js:151: dispose:
function(event)
Remove the parameter.

https://codereview.chromium.org/2109813003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/Target.js (right):

https://codereview.chromium.org/2109813003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/Target.js:170:
this.cpuProfilerModel.dispose();
Are all these classes SDKModel subclasses? We should move them to overriding
dispose, and remove direct calls from here.

https://codereview.chromium.org/2109813003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js
(right):

https://codereview.chromium.org/2109813003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:174: if
(securityDetails && networkManager) {
networkManager should not be ever null

https://codereview.chromium.org/2109813003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:176:
originState.certificateDetailsPromise =
networkManager.certificateDetailsPromise(securityDetails.certificateId);
I'd just inline request.networkManager() here.

[pfeldman, 2016-07-05 21:01:59]

>> I think the more isolation we do, the less we need SDKObject. I'm inclined to
>> remove it entirely. Let's discuss that.

Why?

>> target.model(WebInspector.NetworkManager)

Why not WebInspector.NetworkManager.fromTarget ?

[eostroukhov-old, 2016-07-06 20:37:53]

Thank you for the review. Please take another look.

https://codereview.chromium.org/2109813003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/NetworkLog.js (right):

https://codereview.chromium.org/2109813003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkLog.js:59: return
target[WebInspector.NetworkLog._symbol] || null;
On 2016/07/05 17:54:31, dgozman wrote:
> Let's use target.model(WebInspector.NetworkLog). See DebuggerModel for an
> example.

Done. Note that this required changing the superclass to SDKModel.

https://codereview.chromium.org/2109813003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js (right):

https://codereview.chromium.org/2109813003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js:95: return
target[WebInspector.NetworkManager._symbol] || null;
On 2016/07/05 17:54:31, dgozman wrote:
> target.model(WebInspector.NetworkManager)

Done.

https://codereview.chromium.org/2109813003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js:151: dispose:
function(event)
On 2016/07/05 17:54:31, dgozman wrote:
> Remove the parameter.

Done.

https://codereview.chromium.org/2109813003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/Target.js (right):

https://codereview.chromium.org/2109813003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/Target.js:170:
this.cpuProfilerModel.dispose();
On 2016/07/05 17:54:31, dgozman wrote:
> Are all these classes SDKModel subclasses? We should move them to overriding
> dispose, and remove direct calls from here.

Worker manager is not a subclass... I will not be changing it for this code
review.

Also note that WebInspector.ServiceWorkerCacheModel.fromTarget(this).dispose()
instantiated the SWCM in one test - I removed this line.

https://codereview.chromium.org/2109813003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js
(right):

https://codereview.chromium.org/2109813003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:174: if
(securityDetails && networkManager) {
On 2016/07/05 17:54:31, dgozman wrote:
> networkManager should not be ever null

Acknowledged.

https://codereview.chromium.org/2109813003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:176:
originState.certificateDetailsPromise =
networkManager.certificateDetailsPromise(securityDetails.certificateId);
On 2016/07/05 17:54:31, dgozman wrote:
> I'd just inline request.networkManager() here.

Done.

[dgozman, 2016-07-06 21:04:12]

lgtm

[eostroukhov-old, 2016-07-06 21:08:36]

The CQ bit was checked by eostroukhov@google.com

[commit-bot: I haz the power, 2016-07-06 21:09:16]

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-07-06 23:11:24]

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

[commit-bot: I haz the power, 2016-07-06 23:11:25]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[eostroukhov-old, 2016-07-06 23:16:28]

The CQ bit was checked by eostroukhov@google.com

[commit-bot: I haz the power, 2016-07-06 23:17:24]

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-07-07 02:36:15]

Description was changed from

==========
[DevTools] No NetworkManager and NetworkLog for v8only mode

R=alph@chromium.org, dgozman@chromium.org
BUG=624494
==========

to

==========
[DevTools] No NetworkManager and NetworkLog for v8only mode

R=alph@chromium.org, dgozman@chromium.org
BUG=624494
==========

[commit-bot: I haz the power, 2016-07-07 02:36:17]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-07-07 02:36:22]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-07 02:39:28]

Description was changed from

==========
[DevTools] No NetworkManager and NetworkLog for v8only mode

R=alph@chromium.org, dgozman@chromium.org
BUG=624494
==========

to

==========
[DevTools] No NetworkManager and NetworkLog for v8only mode

R=alph@chromium.org, dgozman@chromium.org
BUG=624494

Committed: https://crrev.com/33d4a941b4801e7cb2828600675751c35750b330
Cr-Commit-Position: refs/heads/master@{#404032}
==========

[commit-bot: I haz the power, 2016-07-07 02:39:29]

Patchset 6 (id:??) landed as
https://crrev.com/33d4a941b4801e7cb2828600675751c35750b330
Cr-Commit-Position: refs/heads/master@{#404032}