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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Sep 27 12:38:11 PDT 2017


Devin Rousso <webkit at devinrousso.com> has granted 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 321971: Patch

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




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

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

r=me, with a few NITs.

> Source/WebInspectorUI/UserInterface/Views/CollectionContentView.js:28
> +    constructor(collection, contentViewConstructor, contentPlaceholerText)

NIT: typo "contentPlaceholerText" => "contentPlaceholderText" (missing a 'd')

> Source/WebInspectorUI/UserInterface/Views/CollectionContentView.js:167
> +		   if (this._selectedItem === item) {

Style: remove brackets.

> Source/WebInspectorUI/UserInterface/Views/CollectionContentView.js:212
> +    _defaultTitle()

NIT: this could probably be static.

    WI.CollectionContentView.titleForCollection()

...or something like that

> Source/WebInspectorUI/UserInterface/Views/CollectionContentView.js:243
> +	       contentView.element.classList.remove("selected");

Should we also have an if checking the existence of `contentView`, or is the
assertion enough?  Are we doing this mainly for developing purposes or are we
trying to avoid crashes?

> Source/WebInspectorUI/UserInterface/Views/CollectionContentView.js:250
> +	       console.assert(selectedContentView, "Missing ContentView for
selected item.", this._selectedItem);

Ditto (243).

> Source/WebInspectorUI/UserInterface/Views/CollectionContentView.js:254
> +	  
this.dispatchEventToListeners(WI.ContentView.Event.SupplementalRepresentedObjec
tsDidChange);

Nice!  Didn't even know this existed :P

> Source/WebInspectorUI/UserInterface/Views/ResourceCollectionContentView.js:36
> +	   let title = WI.Resource.displayNameForType(collection.resourceType,
true);

NIT: use const variable for inline values.

    const plural = true;
    let title = WI.Resource.displayNameForType(collection.resourceType,
plural);

> Source/WebInspectorUI/UserInterface/Views/ResourceCollectionContentView.js:38
> +	   super(collection, contentViewConstructor, title);

NIT: CollectionContentView uses `contentPlaceholderText` instead of `title`. 
I'd prefer to keep the names consistent.


More information about the webkit-reviews mailing list