Bug 163995 – Web Inspector: Create general model object Collection class

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

Duplicate the logic for WI.ResourceCollection for a more general "object" type (e.g. Script, Frame, ContentFlow, etc.).

[Radar WebKit Bug Importer]

<rdar://problem/28948306>

[Devin Rousso]

Created attachment 292847 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Devin Rousso]

Created attachment 292864 [details]
Patch

[Joseph Pecoraro]

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.

[Devin Rousso]

Created attachment 292996 [details]
Patch

[Joseph Pecoraro]

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.

[Devin Rousso]

Created attachment 293060 [details]
Patch

[WebKit Commit Bot]

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: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 293060 [details]
Patch

Clearing flags on attachment: 293060

Committed r208012: <http://trac.webkit.org/changeset/208012>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.