Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2122353002:
[DevTools] Make resource tree model optional (Closed)
|
Created:
4 years, 5 months ago by eostroukhov-old Modified:
4 years, 4 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, caseq+blink_chromium.org, shans, rjwright, blink-reviews-style_chromium.org, blink-reviews-animation_chromium.org, darktears, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, chromium-apps-reviews_chromium.org, sergeyv+blink_chromium.org, Eric Willigers, pfeldman, kozyatinskiy+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[DevTools] Make resource tree model optional
Some targets may not support resource tree, in this case relevant object should not be created.
BUG=624494
Committed: https://crrev.com/27d42dc2ec4da4f99302753c0d15b453adf06a64
Cr-Commit-Position: refs/heads/master@{#413644}
Patch Set 1 #Patch Set 2 : Rebased #
Total comments: 18
Patch Set 3 : Comments #Patch Set 4 : Rebased #
Total comments: 60
Patch Set 5 : Fixed a typo in the build file #Patch Set 6 : Comments addressed, rebased #Patch Set 7 : [DevTools] No RTM for non-browser targets #
Total comments: 34
Patch Set 8 : Comments addressed #
Total comments: 8
Patch Set 9 : Comments were addressed #Patch Set 10 : Rebased #Patch Set 11 : Fixed a bug in console view init #
Total comments: 2
Patch Set 12 : Comments #Messages
Total messages: 63 (36 generated)
eostroukhov@google.com changed reviewers: + dgozman@chromium.org
Description was changed from ========== [DevTools] Make resource tree model optional BUG=624494 No NetworkManager and NetworkLog for v8only mode ========== to ========== [DevTools] Make resource tree model optional BUG=624494 ==========
Description was changed from ========== [DevTools] Make resource tree model optional BUG=624494 ========== to ========== [DevTools] Make resource tree model optional Some targets may not support resource tree, in this case relevant object should not be created. BUG=624494 ==========
pfeldman@chromium.org changed reviewers: + pfeldman@chromium.org
The rule of thumb is that if this code should work in Node, it should not mention resourceTreeModel at all. And if it mentions it, it is safe to use it without the null check. https://codereview.chromium.org/2122353002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/animation/AnimationModel.js (right): https://codereview.chromium.org/2122353002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/animation/AnimationModel.js:24: if (resourceTreeModel) No need in this check - we should either have both resourcetreemodel and animation, or none. https://codereview.chromium.org/2122353002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/animation/AnimationModel.js:889: if (resourceTreeModel) ditto https://codereview.chromium.org/2122353002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/bindings/CompilerScriptMapping.js (right): https://codereview.chromium.org/2122353002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/bindings/CompilerScriptMapping.js:324: // script.sourceURL can be a random string, but is generally an absolute path -> complete it to inspected page url for This arguably belongs here, we should have resolved sourceURL elsewhere if we needed to. We need to optionally provide debugger model with the inspected page url that would be used to complete relative sourceURLs. https://codereview.chromium.org/2122353002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/bindings/NetworkProject.js (right): https://codereview.chromium.org/2122353002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/bindings/NetworkProject.js:204: return (frameId && resourceTreeModel) ? resourceTreeModel.frameForId(frameId) : null; If we have frameId, we should have resourceTreeModel, no need to check. https://codereview.chromium.org/2122353002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/bindings/NetworkProject.js:433: resourceTreeModel.removeEventListener(WebInspector.ResourceTreeModel.EventTypes.ResourceAdded, this._resourceAdded, this); We added 3 listeners, but removed 2 - something is wrong! https://codereview.chromium.org/2122353002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js (right): https://codereview.chromium.org/2122353002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js:48: if (resourceTreeModel) We always have resourceTreeModel when we have CSS. https://codereview.chromium.org/2122353002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/components/ExecutionContextSelector.js (right): https://codereview.chromium.org/2122353002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/ExecutionContextSelector.js:133: _isMainFrameContext: function(executionContext) The name is misleading - we want to know whether this is a default context, should be _isDefault() and should check for belonging to the main target. https://codereview.chromium.org/2122353002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js (right): https://codereview.chromium.org/2122353002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js:37: var resourceTreeModel = WebInspector.ResourceTreeModel.fromTarget(executionContext.target()); What we want here is a friendly executionContext name that should have been set on the executionContext itself (executionContext.name). So it seems like we need to fix it elsewhere. The rule of thumb is that if this code should work in Node, it should not mention resourceTreeModel. And if it mentions it, it is not nullable. https://codereview.chromium.org/2122353002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/2122353002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:196: var cachedResources = false; Same, we need to fix it.
The CQ bit was checked by eostroukhov@google.com to run a CQ dry run
Thank you for the review. Please take another look. https://codereview.chromium.org/2122353002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/animation/AnimationModel.js (right): https://codereview.chromium.org/2122353002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/animation/AnimationModel.js:24: if (resourceTreeModel) On 2016/07/07 17:44:13, pfeldman wrote: > No need in this check - we should either have both resourcetreemodel and > animation, or none. To signify the dependency, I turned resourceTreeModel into ctor parameter. https://codereview.chromium.org/2122353002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/animation/AnimationModel.js:889: if (resourceTreeModel) On 2016/07/07 17:44:13, pfeldman wrote: > ditto Done. https://codereview.chromium.org/2122353002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/bindings/CompilerScriptMapping.js (right): https://codereview.chromium.org/2122353002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/bindings/CompilerScriptMapping.js:324: // script.sourceURL can be a random string, but is generally an absolute path -> complete it to inspected page url for On 2016/07/07 17:44:13, pfeldman wrote: > This arguably belongs here, we should have resolved sourceURL elsewhere if we > needed to. We need to optionally provide debugger model with the inspected page > url that would be used to complete relative sourceURLs. Done. https://codereview.chromium.org/2122353002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/bindings/NetworkProject.js (right): https://codereview.chromium.org/2122353002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/bindings/NetworkProject.js:204: return (frameId && resourceTreeModel) ? resourceTreeModel.frameForId(frameId) : null; On 2016/07/07 17:44:13, pfeldman wrote: > If we have frameId, we should have resourceTreeModel, no need to check. I deleted the whole method, it was not used. https://codereview.chromium.org/2122353002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/bindings/NetworkProject.js:433: resourceTreeModel.removeEventListener(WebInspector.ResourceTreeModel.EventTypes.ResourceAdded, this._resourceAdded, this); On 2016/07/07 17:44:14, pfeldman wrote: > We added 3 listeners, but removed 2 - something is wrong! Done. https://codereview.chromium.org/2122353002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js (right): https://codereview.chromium.org/2122353002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js:48: if (resourceTreeModel) On 2016/07/07 17:44:14, pfeldman wrote: > We always have resourceTreeModel when we have CSS. I added an accessor to get it through the cssModel. https://codereview.chromium.org/2122353002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/components/ExecutionContextSelector.js (right): https://codereview.chromium.org/2122353002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/ExecutionContextSelector.js:133: _isMainFrameContext: function(executionContext) On 2016/07/07 17:44:14, pfeldman wrote: > The name is misleading - we want to know whether this is a default context, > should be > > _isDefault() and should check for belonging to the main target. Done. Note that the main target check is by checking if there's a parent target. It is to make sure tests work - discussed it with +dgozman, https://codereview.chromium.org/2122353002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js (right): https://codereview.chromium.org/2122353002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js:37: var resourceTreeModel = WebInspector.ResourceTreeModel.fromTarget(executionContext.target()); On 2016/07/07 17:44:14, pfeldman wrote: > What we want here is a friendly executionContext name that should have been set > on the executionContext itself (executionContext.name). So it seems like we need > to fix it elsewhere. > > The rule of thumb is that if this code should work in Node, it should not > mention resourceTreeModel. And if it mentions it, it is not nullable. Not sure if such a fix is in scope for this refactoring... https://codereview.chromium.org/2122353002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/2122353002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:196: var cachedResources = false; On 2016/07/07 17:44:14, pfeldman wrote: > Same, we need to fix it. Out of scope for this refactoring?
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
This is no longer a mechanical change, you should split it into smaller patches and land them one by one. https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/animation/AnimationModel.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/animation/AnimationModel.js:213: var resourceTreeModel = /** @type {!WebInspector.ResourceTreeModel} */ (WebInspector.ResourceTreeModel.fromTarget(target)); You could as well do this in the WebInspector.AnimationModel. https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/audits/AuditController.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/audits/AuditController.js:65: var mainResourceURL = resourceTreeModel ? resourceTreeModel.inspectedPageURL() : "main"; s/"main"/""/ https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/bindings/NetworkProject.js (left): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/bindings/NetworkProject.js:173: WebInspector.NetworkProject.uiSourceCodeFrame = function(uiSourceCode) Thanks. https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js:47: cssModel.resourceTreeModel().addEventListener(WebInspector.ResourceTreeModel.EventTypes.MainFrameNavigated, this._unbindAllUISourceCodes, this); This isn't nice - we should keep models of different domains independent as much as we can. You could introduce this navigation event on Target itself to mitigate it here. https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/components/DOMPresentationUtils.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/DOMPresentationUtils.js:160: WebInspector.DOMPresentationUtils.buildImagePreviewContents = function(resourceTreeModel, originalImageURL, showDimensions, userCallback, precomputedFeatures) ditto, DOM can easily function without the resource tree model. We need a way of completing URLs that would be abstract from resource tree model. https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/components/ExecutionContextSelector.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/ExecutionContextSelector.js:135: var resourceTreeModel = WebInspector.ResourceTreeModel.fromTarget(executionContext.target()); This will not compile once we split sdk into js_sdk and browser_sdk. https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js:37: var resourceTreeModel = WebInspector.ResourceTreeModel.fromTarget(executionContext.target()); ditto - this can't use frames. https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:198: var resourceTreeModel = WebInspector.ResourceTreeModel.fromTarget(mainTarget); same https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js:566: var resourceTreeModel = WebInspector.ResourceTreeModel.fromTarget(node.target()); ditto https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeOutline.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeOutline.js:657: var resourceTreeModel = WebInspector.ResourceTreeModel.fromTarget(node.target()); ditto
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/bindings/ResourceUtils.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/bindings/ResourceUtils.js:37: var targets = WebInspector.targetManager.targets(); Pass browser capability? This way you can be sure ResourceTreeModel is present. https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/bindings/ResourceUtils.js:52: var targets = WebInspector.targetManager.targets(); ditto https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js:47: cssModel.resourceTreeModel().addEventListener(WebInspector.ResourceTreeModel.EventTypes.MainFrameNavigated, this._unbindAllUISourceCodes, this); On 2016/07/13 23:55:57, pfeldman wrote: > This isn't nice - we should keep models of different domains independent as much > as we can. You could introduce this navigation event on Target itself to > mitigate it here. I'm not that sure. DOM only makes sense together with frames, and our dom nodes operate with frame ids. Should we say that these domains come together as a group? https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/elements/ElementsSidebarPane.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsSidebarPane.js:94: if (this._cssModel && domModel && resourceTreeModel) { We should split "&& resourceTreeModel" out. This pane can function without resourceTreeModel. https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/extensions/ExtensionServer.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/extensions/ExtensionServer.js:487: for (var target of WebInspector.targetManager.targets()) { browser capability https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/AppManifestView.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/AppManifestView.js:73: var resourceTreeModel = WebInspector.ResourceTreeModel.fromTarget(this._target); I'd rather set this._resourceTreeModel in targetAdded instead of this._target. This view doesn't work without ResourceTreeModel. https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/DOMStorageModel.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/DOMStorageModel.js:140: var securityOrigins = resourceTreeModel.securityOrigins(); Looks like this model doesn't function without security origins. Let's make it explicitly depend on ResourceTreeModel instead. https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/IndexedDBModel.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/IndexedDBModel.js:161: if (resourceTreeModel) { ditto https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js:134: var resourceTreeModel = WebInspector.ResourceTreeModel.fromTarget(target); I think ResourcesPanel should not work without ResourceTreeModel, as the name implies. https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js:56: var resourceTreeModel = WebInspector.ResourceTreeModel.fromTarget(this._target); ditto https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/screencast/ScreencastView.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/screencast/ScreencastView.js:100: var resourceTreeModel = WebInspector.ResourceTreeModel.fromTarget(this._target); ditto https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/ApplicationCacheModel.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/ApplicationCacheModel.js:44: { { on previous line https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/DebuggerModel.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/DebuggerModel.js:339: var resourceTreeModel = WebInspector.ResourceTreeModel.fromTarget(this.target()); This is a wrong dependency. Debugger should not depend on frames. https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/NetworkLog.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/NetworkLog.js:45: if (resourceTreeModel) { This dependency looks strange. Can we eliminate it? https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/NetworkRequest.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/NetworkRequest.js:565: if (resourceTreeModel) This is unfortunate. I'm not sure how to make this better though. https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:35: if (resourceTreeModel) { Require it. https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js:220: resourceTreeModel = new WebInspector.ResourceTreeModel(target, networkManager); This is browser capability, not network. https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js:266: resourceTreeModel.addEventListener(WebInspector.ResourceTreeModel.EventTypes.InspectedURLChanged, this._redispatchEvent, this); nit: while you are here, save the results of addEventListener calls into an array, and do WebInspector.EventListener.removeEventListener(array) in removeTarget. https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/WorkerManager.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/WorkerManager.js:74: resourceTreeModel.removeEventListener(WebInspector.TargetManager.Events.MainFrameNavigated, this._mainFrameNavigated, this); This is a very strange dependency. I think it should no be necessary, but let's dig up why it was added.
https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/ResourceTreeModel.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/ResourceTreeModel.js:133: if (error) { This check can be removed with your change. https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/ResourceTreeModel.js:143: if (this.target().hasBrowserCapability()) This check can be removed with your change.
The CQ bit was checked by eostroukhov@google.com
The CQ bit was unchecked by eostroukhov@google.com
Comments addressed, rebased
Thank you for the review. Did my best to address the comments - please take another look. https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/animation/AnimationModel.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/animation/AnimationModel.js:213: var resourceTreeModel = /** @type {!WebInspector.ResourceTreeModel} */ (WebInspector.ResourceTreeModel.fromTarget(target)); On 2016/07/13 23:55:56, pfeldman wrote: > You could as well do this in the WebInspector.AnimationModel. Done. https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/audits/AuditController.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/audits/AuditController.js:65: var mainResourceURL = resourceTreeModel ? resourceTreeModel.inspectedPageURL() : "main"; On 2016/07/13 23:55:57, pfeldman wrote: > s/"main"/""/ Done. https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/bindings/ResourceUtils.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/bindings/ResourceUtils.js:37: var targets = WebInspector.targetManager.targets(); On 2016/07/14 16:29:28, dgozman wrote: > Pass browser capability? This way you can be sure ResourceTreeModel is present. Done. https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/bindings/ResourceUtils.js:52: var targets = WebInspector.targetManager.targets(); On 2016/07/14 16:29:28, dgozman wrote: > ditto Done. https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js:47: cssModel.resourceTreeModel().addEventListener(WebInspector.ResourceTreeModel.EventTypes.MainFrameNavigated, this._unbindAllUISourceCodes, this); On 2016/07/13 23:55:57, pfeldman wrote: > This isn't nice - we should keep models of different domains independent as much > as we can. You could introduce this navigation event on Target itself to > mitigate it here. Switched to TargetManager event https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/components/DOMPresentationUtils.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/DOMPresentationUtils.js:160: WebInspector.DOMPresentationUtils.buildImagePreviewContents = function(resourceTreeModel, originalImageURL, showDimensions, userCallback, precomputedFeatures) On 2016/07/13 23:55:57, pfeldman wrote: > ditto, DOM can easily function without the resource tree model. We need a way of > completing URLs that would be abstract from resource tree model. I'd like to ignore DOM dependency on a Network for now - breaking it seems to be a separate and fairly large task. https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/components/ExecutionContextSelector.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/ExecutionContextSelector.js:135: var resourceTreeModel = WebInspector.ResourceTreeModel.fromTarget(executionContext.target()); On 2016/07/13 23:55:57, pfeldman wrote: > This will not compile once we split sdk into js_sdk and browser_sdk. I believe Joel is looking into it. I am not sure what solution (injection?) he will implement. https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js:37: var resourceTreeModel = WebInspector.ResourceTreeModel.fromTarget(executionContext.target()); On 2016/07/13 23:55:57, pfeldman wrote: > ditto - this can't use frames. Same as my previous comment... https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:198: var resourceTreeModel = WebInspector.ResourceTreeModel.fromTarget(mainTarget); On 2016/07/13 23:55:57, pfeldman wrote: > same Acknowledged. https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js:566: var resourceTreeModel = WebInspector.ResourceTreeModel.fromTarget(node.target()); On 2016/07/13 23:55:57, pfeldman wrote: > ditto Reverted https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/elements/ElementsSidebarPane.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsSidebarPane.js:94: if (this._cssModel && domModel && resourceTreeModel) { On 2016/07/14 16:29:28, dgozman wrote: > We should split "&& resourceTreeModel" out. This pane can function without > resourceTreeModel. This code have been removed. https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeOutline.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeOutline.js:657: var resourceTreeModel = WebInspector.ResourceTreeModel.fromTarget(node.target()); On 2016/07/13 23:55:57, pfeldman wrote: > ditto Reverted. https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/extensions/ExtensionServer.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/extensions/ExtensionServer.js:487: for (var target of WebInspector.targetManager.targets()) { On 2016/07/14 16:29:28, dgozman wrote: > browser capability Done. https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/AppManifestView.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/AppManifestView.js:73: var resourceTreeModel = WebInspector.ResourceTreeModel.fromTarget(this._target); On 2016/07/14 16:29:28, dgozman wrote: > I'd rather set this._resourceTreeModel in targetAdded instead of this._target. > This view doesn't work without ResourceTreeModel. Done. https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/DOMStorageModel.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/DOMStorageModel.js:140: var securityOrigins = resourceTreeModel.securityOrigins(); On 2016/07/14 16:29:28, dgozman wrote: > Looks like this model doesn't function without security origins. Let's make it > explicitly depend on ResourceTreeModel instead. Done. https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/IndexedDBModel.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/IndexedDBModel.js:161: if (resourceTreeModel) { On 2016/07/14 16:29:28, dgozman wrote: > ditto Done. https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js:134: var resourceTreeModel = WebInspector.ResourceTreeModel.fromTarget(target); On 2016/07/14 16:29:28, dgozman wrote: > I think ResourcesPanel should not work without ResourceTreeModel, as the name > implies. Done. https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkersView.js:56: var resourceTreeModel = WebInspector.ResourceTreeModel.fromTarget(this._target); On 2016/07/14 16:29:28, dgozman wrote: > ditto Done. https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/screencast/ScreencastView.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/screencast/ScreencastView.js:100: var resourceTreeModel = WebInspector.ResourceTreeModel.fromTarget(this._target); On 2016/07/14 16:29:29, dgozman wrote: > ditto Done. https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/ApplicationCacheModel.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/ApplicationCacheModel.js:44: { On 2016/07/14 16:29:29, dgozman wrote: > { on previous line Done. https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/DebuggerModel.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/DebuggerModel.js:339: var resourceTreeModel = WebInspector.ResourceTreeModel.fromTarget(this.target()); On 2016/07/14 16:29:29, dgozman wrote: > This is a wrong dependency. Debugger should not depend on frames. Moved the code back to CompilerScriptMapping... Joel is working on splitting resourceTreeModel from other domains. https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/NetworkLog.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/NetworkLog.js:45: if (resourceTreeModel) { On 2016/07/14 16:29:29, dgozman wrote: > This dependency looks strange. Can we eliminate it? NetworkLog is currently considered a browser-only model as it extensively refers to page loads and such. https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/NetworkRequest.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/NetworkRequest.js:565: if (resourceTreeModel) On 2016/07/14 16:29:29, dgozman wrote: > This is unfortunate. I'm not sure how to make this better though. Separating the resource tree model from network is not the goal of this CL... https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/ResourceTreeModel.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/ResourceTreeModel.js:133: if (error) { On 2016/07/14 16:30:28, dgozman wrote: > This check can be removed with your change. (Looks like this comment is a duplicate of the comment below) https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/ResourceTreeModel.js:143: if (this.target().hasBrowserCapability()) On 2016/07/14 16:30:28, dgozman wrote: > This check can be removed with your change. Done. https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:35: if (resourceTreeModel) { On 2016/07/14 16:29:29, dgozman wrote: > Require it. Done. https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js:220: resourceTreeModel = new WebInspector.ResourceTreeModel(target, networkManager); On 2016/07/14 16:29:29, dgozman wrote: > This is browser capability, not network. Apparently, it requires both... I updated it. https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js:266: resourceTreeModel.addEventListener(WebInspector.ResourceTreeModel.EventTypes.InspectedURLChanged, this._redispatchEvent, this); On 2016/07/14 16:29:29, dgozman wrote: > nit: while you are here, save the results of addEventListener calls into an > array, and do WebInspector.EventListener.removeEventListener(array) in > removeTarget. Done. https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/WorkerManager.js (right): https://codereview.chromium.org/2122353002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/WorkerManager.js:74: resourceTreeModel.removeEventListener(WebInspector.TargetManager.Events.MainFrameNavigated, this._mainFrameNavigated, this); On 2016/07/14 16:29:29, dgozman wrote: > This is a very strange dependency. I think it should no be necessary, but let's > dig up why it was added. Switched to listening to TargetManager instead.
eostroukhov@google.com changed reviewers: - dgozman@chromium.org, pfeldman@chromium.org
The CQ bit was checked by eostroukhov@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
eostroukhov@google.com changed reviewers: + dgozman@chromium.org, pfeldman@chromium.org
This is a vastly different patch, big portion of the previous revision went in as a separate CL. Please feel free to consider this a new CL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
dgozman@chromium.org changed reviewers: + dgozman@chromium.org
https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/animation/AnimationModel.js (right): https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/animation/AnimationModel.js:212: if (!target[WebInspector.AnimationModel._symbol]) { style: unnecessary {} https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/bindings/NetworkProject.js (right): https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/bindings/NetworkProject.js:83: if (resourceTreeModel) { Let's make sure we have service worker script in the navigator. https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/bindings/NetworkProject.js:397: this._resourceTreeModel.removeEventListener(WebInspector.ResourceTreeModel.EventTypes.ResourceAdded, this._resourceAdded, this); nit: while you are here, let's migrate to WebInspector.EventTarget.removeEventListeners. https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/bindings/ResourceUtils.js (right): https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/bindings/ResourceUtils.js:52: for (var i = 0; i < targets.length; ++i) { style: unnecessary {} https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js (right): https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js:53: WebInspector.targetManager.addEventListener(WebInspector.TargetManager.Events.MainFrameNavigated, this._unbindAllUISourceCodes, this); This class wants to listen to specific resourceTreeModel, let's make it explicit. https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:193: cachedResources = resourceTreeModel && resourceTreeModel.cachedResourcesLoaded(); cachedResources should be true when there is no main target or there is no resource tree model. https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/elements/ElementsSidebarPane.js (right): https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/elements/ElementsSidebarPane.js:21: stray blank line https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js (right): https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:477: var fileName = mainFrame ? mainFrame.url.trimURL().removeURLFragment() : ""; All this code looks like WI.targetManager.inspectedPageURL().removeURLFragment() https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/extensions/ExtensionServer.js (right): https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/extensions/ExtensionServer.js:946: if (options.frameURL) style: both branches should have {} https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/extensions/ExtensionServer.js:947: frame = resolveURLToFrame(options.frameURL) style: missing semicolon https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js (right): https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js:34: WebInspector.TargetManager._ListenersSymbol = Symbol("WebInspector.TargetManager.Listeners"); style: _listenersSymbol https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js:201: var securityOriginManager = WebInspector.SecurityOriginManager.fromTarget(target); Inline this one? https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js:203: new WebInspector.NetworkLog(target, resourceTreeModel, networkManager); nit: let's avoid nested if's here. Instead I propose to list all dependencies in each if statement, e.g.: if (resourceTreeModel && networkManager) new WebInspector.NetworkLog(target, resourceTreeModel, networkManager); https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js:210: if (resourceTreeModel) { Why this change? If DOMModel or CSSModel cannot work without ResourceTreeModel, why don't we pass it in constructor? https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js:250: listeners.push(resourceTreeModel.addEventListener(WebInspector.ResourceTreeModel.EventTypes.MainFrameNavigated, this._redispatchEvent, this)); nit: instead of push, we can inline: resourceTreeModel[symbol] = [ ..., ..., ]; https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/timeline/TimelineController.js (right): https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/timeline/TimelineController.js:177: resourceTreeModel.suspendReload(); Didn't we agree to move suspend/resume to target manager?
Comments addressed
The CQ bit was checked by eostroukhov@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
eostroukhov@chromium.org changed reviewers: + eostroukhov@chromium.org
Thank you for the review. I addressed the comments, please take another look. https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/animation/AnimationModel.js (right): https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/animation/AnimationModel.js:212: if (!target[WebInspector.AnimationModel._symbol]) { On 2016/08/19 20:23:37, dgozman wrote: > style: unnecessary {} Done. https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/bindings/NetworkProject.js (right): https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/bindings/NetworkProject.js:83: if (resourceTreeModel) { On 2016/08/19 20:23:37, dgozman wrote: > Let's make sure we have service worker script in the navigator. I see the service worker in the view. It is added as a result of the "script parsed" notification. https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/bindings/NetworkProject.js:397: this._resourceTreeModel.removeEventListener(WebInspector.ResourceTreeModel.EventTypes.ResourceAdded, this._resourceAdded, this); On 2016/08/19 20:23:37, dgozman wrote: > nit: while you are here, let's migrate to > WebInspector.EventTarget.removeEventListeners. Done. https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/bindings/ResourceUtils.js (right): https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/bindings/ResourceUtils.js:52: for (var i = 0; i < targets.length; ++i) { On 2016/08/19 20:23:37, dgozman wrote: > style: unnecessary {} Done. https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js (right): https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js:53: WebInspector.targetManager.addEventListener(WebInspector.TargetManager.Events.MainFrameNavigated, this._unbindAllUISourceCodes, this); On 2016/08/19 20:23:37, dgozman wrote: > This class wants to listen to specific resourceTreeModel, let's make it > explicit. Done. https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:193: cachedResources = resourceTreeModel && resourceTreeModel.cachedResourcesLoaded(); On 2016/08/19 20:23:37, dgozman wrote: > cachedResources should be true when there is no main target or there is no > resource tree model. Done. https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/elements/ElementsSidebarPane.js (right): https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/elements/ElementsSidebarPane.js:21: On 2016/08/19 20:23:37, dgozman wrote: > stray blank line Done. https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js (right): https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:477: var fileName = mainFrame ? mainFrame.url.trimURL().removeURLFragment() : ""; On 2016/08/19 20:23:37, dgozman wrote: > All this code looks like WI.targetManager.inspectedPageURL().removeURLFragment() Done. https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/extensions/ExtensionServer.js (right): https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/extensions/ExtensionServer.js:946: if (options.frameURL) On 2016/08/19 20:23:37, dgozman wrote: > style: both branches should have {} Done. https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/extensions/ExtensionServer.js:947: frame = resolveURLToFrame(options.frameURL) On 2016/08/19 20:23:37, dgozman wrote: > style: missing semicolon Done. https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js (right): https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js:34: WebInspector.TargetManager._ListenersSymbol = Symbol("WebInspector.TargetManager.Listeners"); On 2016/08/19 20:23:37, dgozman wrote: > style: _listenersSymbol Done. https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js:201: var securityOriginManager = WebInspector.SecurityOriginManager.fromTarget(target); On 2016/08/19 20:23:37, dgozman wrote: > Inline this one? Done. https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js:203: new WebInspector.NetworkLog(target, resourceTreeModel, networkManager); On 2016/08/19 20:23:37, dgozman wrote: > nit: let's avoid nested if's here. Instead I propose to list all dependencies in > each if statement, e.g.: > > if (resourceTreeModel && networkManager) > new WebInspector.NetworkLog(target, resourceTreeModel, networkManager); Done. https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js:210: if (resourceTreeModel) { On 2016/08/19 20:23:37, dgozman wrote: > Why this change? If DOMModel or CSSModel cannot work without ResourceTreeModel, > why don't we pass it in constructor? I was trying to do this in an earlier CL - ultimately we decided against it as it looks like the CSSModel should not depend on RTM but this can't be fixed right now. https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js:250: listeners.push(resourceTreeModel.addEventListener(WebInspector.ResourceTreeModel.EventTypes.MainFrameNavigated, this._redispatchEvent, this)); On 2016/08/19 20:23:37, dgozman wrote: > nit: instead of push, we can inline: > resourceTreeModel[symbol] = [ > ..., > ..., > ]; Done. https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/timeline/TimelineController.js (right): https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/timeline/TimelineController.js:177: resourceTreeModel.suspendReload(); On 2016/08/19 20:23:37, dgozman wrote: > Didn't we agree to move suspend/resume to target manager? Done.
lgtm https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js (right): https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js:210: if (resourceTreeModel) { On 2016/08/20 01:22:31, eostroukhov wrote: > On 2016/08/19 20:23:37, dgozman wrote: > > Why this change? If DOMModel or CSSModel cannot work without > ResourceTreeModel, > > why don't we pass it in constructor? > > I was trying to do this in an earlier CL - ultimately we decided against it as > it looks like the CSSModel should not depend on RTM but this can't be fixed > right now. Let's add a TODO here then. https://codereview.chromium.org/2122353002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/bindings/NetworkProject.js (right): https://codereview.chromium.org/2122353002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/bindings/NetworkProject.js:402: WebInspector.EventTarget.removeEventListeners(this._eventListeners); Let's also do this._eventListeners = []; https://codereview.chromium.org/2122353002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js (right): https://codereview.chromium.org/2122353002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js:189: if (event.data.target() !== this.target()) Drop this check. https://codereview.chromium.org/2122353002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js (right): https://codereview.chromium.org/2122353002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:475: var url = mainTarget && mainTarget.inspectedURL() style: missing semicolon https://codereview.chromium.org/2122353002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js (right): https://codereview.chromium.org/2122353002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js:198: if (target.hasNetworkCapability()) { style: unnecessary {}
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Comments were addressed
Thanks for the review - I addressed the comments and will now submit the CL. https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js (right): https://codereview.chromium.org/2122353002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js:210: if (resourceTreeModel) { On 2016/08/20 01:32:00, dgozman wrote: > On 2016/08/20 01:22:31, eostroukhov wrote: > > On 2016/08/19 20:23:37, dgozman wrote: > > > Why this change? If DOMModel or CSSModel cannot work without > > ResourceTreeModel, > > > why don't we pass it in constructor? > > > > I was trying to do this in an earlier CL - ultimately we decided against it as > > it looks like the CSSModel should not depend on RTM but this can't be fixed > > right now. > > Let's add a TODO here then. Done. https://codereview.chromium.org/2122353002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/bindings/NetworkProject.js (right): https://codereview.chromium.org/2122353002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/bindings/NetworkProject.js:402: WebInspector.EventTarget.removeEventListeners(this._eventListeners); On 2016/08/20 01:32:00, dgozman wrote: > Let's also do this._eventListeners = []; Done. https://codereview.chromium.org/2122353002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js (right): https://codereview.chromium.org/2122353002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/bindings/StylesSourceMapping.js:189: if (event.data.target() !== this.target()) On 2016/08/20 01:32:00, dgozman wrote: > Drop this check. Done. https://codereview.chromium.org/2122353002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js (right): https://codereview.chromium.org/2122353002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:475: var url = mainTarget && mainTarget.inspectedURL() On 2016/08/20 01:32:00, dgozman wrote: > style: missing semicolon Done. https://codereview.chromium.org/2122353002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js (right): https://codereview.chromium.org/2122353002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js:198: if (target.hasNetworkCapability()) { On 2016/08/20 01:32:00, dgozman wrote: > style: unnecessary {} Done.
The CQ bit was checked by eostroukhov@google.com
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/2122353002/#ps160001 (title: "Comments were addressed")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Rebased
The CQ bit was checked by eostroukhov@google.com
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/2122353002/#ps180001 (title: "Rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by eostroukhov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Fixed a bug in console view init
The CQ bit was checked by eostroukhov@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Please take another look at ConsoleView.js - there was a bug there. Thank you for the review!
lgtm https://codereview.chromium.org/2122353002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/2122353002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:191: var resourcesLoaded = !resourceTreeModel || resourceTreeModel.cachedResourcesLoaded; - style: double space before = - cachedResourcesLoaded() - missing () https://codereview.chromium.org/2122353002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js (right): https://codereview.chromium.org/2122353002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js:211: // TODO [eostroukhov]: CSSModel should not depend on RTM TODO(eostroukhov)
Comments
The CQ bit was checked by eostroukhov@google.com
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/2122353002/#ps220001 (title: "Comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [DevTools] Make resource tree model optional Some targets may not support resource tree, in this case relevant object should not be created. BUG=624494 ========== to ========== [DevTools] Make resource tree model optional Some targets may not support resource tree, in this case relevant object should not be created. BUG=624494 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== [DevTools] Make resource tree model optional Some targets may not support resource tree, in this case relevant object should not be created. BUG=624494 ========== to ========== [DevTools] Make resource tree model optional Some targets may not support resource tree, in this case relevant object should not be created. BUG=624494 Committed: https://crrev.com/27d42dc2ec4da4f99302753c0d15b453adf06a64 Cr-Commit-Position: refs/heads/master@{#413644} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/27d42dc2ec4da4f99302753c0d15b453adf06a64 Cr-Commit-Position: refs/heads/master@{#413644} |
