[webkit-reviews] review denied: [Bug 177419] Web Inspector: Create ResourceCollectionContentView and make CollectionContentView easier to extend : [Attachment 321772] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <webkit at devinrousso.com> has denied Matt Baker
<mattbaker at apple.com>'s request for review:
Bug 177419: Web Inspector: Create ResourceCollectionContentView and make
CollectionContentView easier to extend
https://bugs.webkit.org/show_bug.cgi?id=177419

Attachment 321772: Patch

https://bugs.webkit.org/attachment.cgi?id=321772&action=review




--- Comment #5 from Devin Rousso <webkit at devinrousso.com> ---
Comment on attachment 321772
  --> https://bugs.webkit.org/attachment.cgi?id=321772
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=321772&action=review

r-, mainly because I think there is some logic inside
ResourceCollectionContentView that shouldn't be there.	If your goal is to
truly split these apart, I think you tweak it slightly.

> Source/WebInspectorUI/ChangeLog:36
> +	   Made public.

NIT: it's up to you, but I don't really find these types of comments that
useful.

> Source/WebInspectorUI/UserInterface/Views/CollectionContentView.js:36
> +	   this._contentPlaceholder = new WI.TitleView(contentPlaceholerText ||
WI.UIString("No Items"));

Instead of "No Text", perhaps you can create a function for all of the
non-resource types we can see in a Collection that provides some text:

    static titleForCollectionType(collection)
    {
	if (!collection || !collection.typeVerifier)
	    return WI.UIString("Collection"); // or something else...

	switch (collection.typeVerifier) {
	case Any:
	    return WI.UIString("Collection");

	case Frame:
	    return WI.UIString("Frames");

	case Resource:
	    if (collection instanceof WI.ResourceCollection)
		return
WI.ResourceCollection.titleForCollectionType(collection); // you'd have to
create this function
	    return WI.UIString("Resources");

	case Script:
	    return WI.UIString("Scripts");

	case CSSStyleSheet:
	    return WI.UIString("Stylesheets");

	case Canvas:
	    return WI.UIString("Canvases");

	case ShaderProgram:
	    return WI.UIString("Shader Programs");
	}

	console.error("Unknown type verifier for collection",
collection.typeVerifier);
	return WI.UIString("Collection");
    }

> Source/WebInspectorUI/UserInterface/Views/CollectionContentView.js:45
> +    addContentViewForItem(item)

NIT: why does this need to be public?  The only user of this (and
`removeContentViewForItem`) is a subclass, so could it be protected instead?

> Source/WebInspectorUI/UserInterface/Views/CollectionContentView.js:63
> +	       this.selectedItemChanged(previousSelection, item);

I would imagine that most callers wouldn't care too much about the previous
selection, so I'd reverse the order of the arguments:

    this.selectedItemChanged(item, previousSelection);

> Source/WebInspectorUI/UserInterface/Views/CollectionContentView.js:74
> +    removeContentViewForItem(item)

Ditto (45)

> Source/WebInspectorUI/UserInterface/Views/ContentView.js:165
> +	       return new WI.ResourceCollectionContentView(representedObject,
extraArguments);

I don't think this is what we want to do, as this won't work for Canvases.  We
will only want to create a ResourceCollectionContentView if we have an instance
of ResourceCollection.

As a side note, it looks like there's an existing bug in that selecting the
Canvases folder doesn't show any text.	We should probably fix that :|

> Source/WebInspectorUI/UserInterface/Views/ResourceCollectionContentView.js:36
> +	       contentViewConstructor = collection.resourceType ===
WI.Resource.Type.Image ? WI.ImageResourceContentView : null;

Instead of having this ternary, I think it'd be cleaner to just use an if:

    if (collection.resourceType === WI.Resource.Type.Image)
	contentViewConstructor = WI.ImageResourceContentView;

> Source/WebInspectorUI/UserInterface/Views/ResourceCollectionContentView.js:52
> +	   } else {
> +	       switch (collection.typeVerifier) {
> +	       case WI.Collection.TypeVerifier.Frame:
> +		   title = WI.UIString("Frames");
> +		   break;
> +
> +	       case WI.Collection.TypeVerifier.Script:
> +		   title = WI.UIString("Extra Scripts");
> +		   break;
> +
> +	       case WI.Collection.TypeVerifier.Resource:
> +		   title = WI.UIString("Resource");
> +		   break;
> +	       }
> +	   }

It seems odd to have this inside ResourceCollectionContentView.  I would expect
the provided collection to be a ResourceCollection, and would consider it an
error if just a Collection was provided.