Bug 163995

<rdar://problem/28948306>

Created attachment 292847 [details] Patch

Comment on attachment 292847 [details] Patch Attachment 292847 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2371162 New failing tests: inspector/debugger/breakpoints/resolved-dump-all-pause-locations.html

Created attachment 292858 [details] Archive of layout-test-results from ews105 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5

Comment on attachment 292847 [details] Patch Attachment 292847 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2371362 New failing tests: inspector/debugger/breakpoints/resolved-dump-all-pause-locations.html

Created attachment 292862 [details] Archive of layout-test-results from ews103 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5

Created attachment 292864 [details] Patch

Comment on attachment 292864 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=292864&action=review r- because this needs a unit-test. That unit-test should cover at least the tricky case above. Where a ResourceCollection adds a resource, and that resource changes type. > Source/WebInspectorUI/UserInterface/Models/Collection.js:33 > + this._typeVerifier = typeof typeVerifier === "function" ? typeVerifier : WebInspector.Collection.TypeVerifier.Any; I'd put an assert(!typeVerifier || typeVerifier === "function"), and then here simplify to just: this._typeVerifier = typeVerifier || WebInspector.Collection.TypeVerifier.Any; > Source/WebInspectorUI/UserInterface/Models/Collection.js:40 > + if (items) { > + if (!Array.isArray(items)) > + items = [items]; > + > + this.add(...items); > + } Lets just have Collection take an "array" instead of dealing with this. Just have: for (let item of items) this.add(item); > Source/WebInspectorUI/UserInterface/Models/Frame.js:355 > + get resourceCollection() { return this._resourceCollection; } Move this up at the top of the Public methods. > Source/WebInspectorUI/UserInterface/Models/ResourceCollection.js:33 > + let verifier = null; > + switch (resourceType) { > + case WebInspector.Resource.Type.Document: I would put this switch into a static function so the constructor does not have so much code before the super(): super(items, WebInspector.ResourceCollection.typeVerifierForResourceType(resourceType)); I worry whenever there is non-trivial code before super(). > Source/WebInspectorUI/UserInterface/Models/ResourceCollection.js:86 > + resourcesCollectionForType = new ResourceCollection(null, type); This `null` would need to be `[]` if you take my earlier suggestion. > Source/WebInspectorUI/UserInterface/Models/ResourceCollection.js:181 > + if (this._resourceType) { > + console.assert(resource.type !== this._resourceType); > + this.remove(resource); > + return; > + } Wow this is subtle and sketchy. When you are an "inner resource collection" we just remove. This needs tests. > Source/WebInspectorUI/UserInterface/Views/CookieStorageContentView.js:163 > - allResources = allResources.concat(frame.resources); > + allResources = allResources.concat(Array.from(frame.resourceCollection.items)); This reads rather obtusely to me. Maybe we should leave in a convenience frame.resources which does this.

Created attachment 292996 [details] Patch

Comment on attachment 292996 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=292996&action=review r=me > Source/WebInspectorUI/UserInterface/Models/Collection.js:34 > + console.assert(!typeVerifier || typeVerifier === "function"); The second half of this assert should have typeof somewhere. Style: Also we normally clump parameter asserts after the super call. > Source/WebInspectorUI/UserInterface/Models/Collection.js:115 > + ContentFlow: (object) => object instanceof WebInspector.ContentFlow, This one doesn't seem particularly useful to be a top level. If/when it is used it seems it could be done at the callsite since it seems unlikely to be shared. > Source/WebInspectorUI/UserInterface/Models/ResourceCollection.js:31 > + super(WebInspector.ResourceCollection.verifierForType(resourceType)); Much nicer! > Source/WebInspectorUI/UserInterface/Models/ResourceCollection.js:185 > + let resourcesWithNewType = this.resourceCollectionForType(resource.type); > + resourcesWithNewType.add(resource); > > - if (this._resourcesTypeMap.has(oldType)) > - this._resourcesTypeMap.get(oldType).remove(resource); > + let resourcesWithOldType = this.resourceCollectionForType(oldType); > + resourcesWithOldType.remove(resource); This code is untested. It would be good to have a type change that actually goes through at the top level. I was expecting a test like this: let collection = new WebInspector.ResourceCollection; let imageResource = createResource("image.php", Type.Other); collection.add(imageResource); InspectorTest.expectEqual(collection.items.size, 1, "Should have 1 resource total."); InspectorTest.expectEqual(collection.resourceCollectionForType(Type.Other).items.size, 1, "Should have 1 Other resource."); InspectorTest.expectEqual(collection.resourceCollectionForType(Type.Image).items.size, 0, "Should have 0 Image resources."); // Dispatch a type change. imageResource._type = Type.Image; imageResource.dispatchEventToListeners(WebInspector.Resource.Event.TypeDidChange, {oldType: WebInspector.Resource.Type.Image}); InspectorTest.expectEqual(collection.items.size, 1, "Should still have 1 resource total."); InspectorTest.expectEqual(collection.resourceCollectionForType(Type.Other).items.size, 0, "Should have 0 Other resources."); InspectorTest.expectEqual(collection.resourceCollectionForType(Type.Image).items.size, 1, "Should have 1 Image resource."); Which covers the outer ResourceCollection and the inner typed ResourceCollections.

Created attachment 293060 [details] Patch

The commit-queue encountered the following flaky tests while processing attachment 293060 [details]: http/tests/security/svg-image-with-css-cross-domain.html bug 163922 (author: sabouhallawa@apple.com) The commit-queue is continuing to process your patch.

Comment on attachment 293060 [details] Patch Clearing flags on attachment: 293060 Committed r208012: <http://trac.webkit.org/changeset/208012>

All reviewed patches have been landed. Closing bug.