Bug 163995 - Web Inspector: Create general model object Collection class
Summary: Web Inspector: Create general model object Collection class
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks: 145906
  Show dependency treegraph
 
Reported: 2016-10-25 16:50 PDT by Devin Rousso
Modified: 2016-10-27 15:28 PDT (History)
9 users (show)

See Also:


Attachments
Patch (27.28 KB, patch)
2016-10-25 16:52 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (1.10 MB, application/zip)
2016-10-25 17:54 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews103 for mac-yosemite (1.12 MB, application/zip)
2016-10-25 18:38 PDT, Build Bot
no flags Details
Patch (28.82 KB, patch)
2016-10-25 19:31 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (37.65 KB, patch)
2016-10-26 23:37 PDT, Devin Rousso
joepeck: review+
Details | Formatted Diff | Diff
Patch (36.59 KB, patch)
2016-10-27 14:41 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2016-10-25 16:50:26 PDT
Duplicate the logic for WI.ResourceCollection for a more general "object" type (e.g. Script, Frame, ContentFlow, etc.).
Comment 1 Radar WebKit Bug Importer 2016-10-25 16:50:51 PDT
<rdar://problem/28948306>
Comment 2 Devin Rousso 2016-10-25 16:52:09 PDT
Created attachment 292847 [details]
Patch
Comment 3 Build Bot 2016-10-25 17:54:43 PDT
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
Comment 4 Build Bot 2016-10-25 17:54:46 PDT
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 5 Build Bot 2016-10-25 18:38:54 PDT
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
Comment 6 Build Bot 2016-10-25 18:38:57 PDT
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
Comment 7 Devin Rousso 2016-10-25 19:31:51 PDT
Created attachment 292864 [details]
Patch
Comment 8 Joseph Pecoraro 2016-10-26 16:00:27 PDT
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.
Comment 9 Devin Rousso 2016-10-26 23:37:30 PDT
Created attachment 292996 [details]
Patch
Comment 10 Joseph Pecoraro 2016-10-27 11:47:45 PDT
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.
Comment 11 Devin Rousso 2016-10-27 14:41:00 PDT
Created attachment 293060 [details]
Patch
Comment 12 WebKit Commit Bot 2016-10-27 15:27:38 PDT
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 13 WebKit Commit Bot 2016-10-27 15:28:16 PDT
Comment on attachment 293060 [details]
Patch

Clearing flags on attachment: 293060

Committed r208012: <http://trac.webkit.org/changeset/208012>
Comment 14 WebKit Commit Bot 2016-10-27 15:28:22 PDT
All reviewed patches have been landed.  Closing bug.