| Summary: | Web Inspector: Create ResourceCollectionContentView and make CollectionContentView easier to extend | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Matt Baker <mattbaker> | ||||||||||||||||||
| Component: | Web Inspector | Assignee: | Matt Baker <mattbaker> | ||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||
| Severity: | Normal | CC: | commit-queue, hi, inspector-bugzilla-changes, webkit-bug-importer | ||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||
| OS: | All | ||||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||||
| Bug Blocks: | 175485 | ||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||
Created attachment 321656 [details]
Patch
I suppose `title` could be a ResourceCollectionContentView thing, rather than being in the base class. Will change. Created attachment 321772 [details]
Patch
(In reply to Matt Baker from comment #2) > I suppose `title` could be a ResourceCollectionContentView thing, rather > than being in the base class. Will change. Decided to keep it in the base class, so that subclasses can provide an empty message. Comment on attachment 321772 [details] 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. Comment on attachment 321772 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=321772&action=review > Source/WebInspectorUI/UserInterface/Views/CollectionContentView.js:50 > + if (this._contentViewConstructor.parentView) Oops. Created attachment 321827 [details]
Patch
Comment on attachment 321772 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=321772&action=review >> Source/WebInspectorUI/ChangeLog:36 >> + Made public. > > NIT: it's up to you, but I don't really find these types of comments that useful. Removed. >> 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"); > } Done! >> 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? Moved to the protected section. >> 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); This has been changed to an event. >> Source/WebInspectorUI/UserInterface/Views/ResourceCollectionContentView.js:52 >> + } > > 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. Fixed. Created attachment 321829 [details]
Patch
Comment on attachment 321829 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=321829&action=review > Source/WebInspectorUI/UserInterface/Views/CollectionContentView.js:154 > + this.representedObject.addEventListener(WI.Collection.Event.ItemAdded, this._handleItemAdded, this); > + this.representedObject.addEventListener(WI.Collection.Event.ItemRemoved, this._handleItemRemoved, this); I think this might have an issue with items that were added in the following scenario: 1. CollectionContentView is shown (initialLayout and attached) 2. A different item in the NavigationSidebar is selected (detached) 3. Items are added to the Collection (which would fire WI.Collection.Event.ItemAdded) 4. The CollectionContentView is shown again (attached) => Items are missing from the shown list We probably want to modify addContentViewForItem to early-return in the case that item is already in _contentViewMap. This means that all the logic inside initialLayout should really be inside attached, right before the event listeners are added. > Source/WebInspectorUI/UserInterface/Views/CollectionContentView.js:209 > + console.error("Unknown type verifier for collection.", typeVerifier); It is allowed for Collection to not have a typeVerifier, so we might only want to error if it is not null. Comment on attachment 321829 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=321829&action=review >> Source/WebInspectorUI/UserInterface/Views/CollectionContentView.js:154 >> + this.representedObject.addEventListener(WI.Collection.Event.ItemRemoved, this._handleItemRemoved, this); > > I think this might have an issue with items that were added in the following scenario: > > 1. CollectionContentView is shown (initialLayout and attached) > 2. A different item in the NavigationSidebar is selected (detached) > 3. Items are added to the Collection (which would fire WI.Collection.Event.ItemAdded) > 4. The CollectionContentView is shown again (attached) > => Items are missing from the shown list > > We probably want to modify addContentViewForItem to early-return in the case that item is already in _contentViewMap. This means that all the logic inside initialLayout should really be inside attached, right before the event listeners are added. We should just track pending added and removed items and sync ContentViews during layout. >> Source/WebInspectorUI/UserInterface/Views/CollectionContentView.js:209 >> + console.error("Unknown type verifier for collection.", typeVerifier); > > It is allowed for Collection to not have a typeVerifier, so we might only want to error if it is not null. It could also have a type verifier that isn't a property of WI.Collection.TypeVerifier. We probably shouldn't error, even if it is non-null. Comment on attachment 321829 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=321829&action=review >>> Source/WebInspectorUI/UserInterface/Views/CollectionContentView.js:154 >>> + this.representedObject.addEventListener(WI.Collection.Event.ItemRemoved, this._handleItemRemoved, this); >> >> I think this might have an issue with items that were added in the following scenario: >> >> 1. CollectionContentView is shown (initialLayout and attached) >> 2. A different item in the NavigationSidebar is selected (detached) >> 3. Items are added to the Collection (which would fire WI.Collection.Event.ItemAdded) >> 4. The CollectionContentView is shown again (attached) >> => Items are missing from the shown list >> >> We probably want to modify addContentViewForItem to early-return in the case that item is already in _contentViewMap. This means that all the logic inside initialLayout should really be inside attached, right before the event listeners are added. > > We should just track pending added and removed items and sync ContentViews during layout. This means that an additional set/array is needed to keep track of added items, and more management logic is required to handle this. I'd personally prefer to keep UI logic confined to only things that are visible, and I think that it would be logically simpler to just iterate over the existing items and compare to the ones in the Collection. If the view is never attached() again, then all of this management work is unnecessary. for (let item of this._contentViewMap.keys()) { if (!this.representedObject.items.has(item)) this.removeContentViewForItem(item); } for (let item of this.representedObject.items) { if (!this._contentViewMap.has(item)) this.addContentViewForItem(item); } >>> Source/WebInspectorUI/UserInterface/Views/CollectionContentView.js:209 >>> + console.error("Unknown type verifier for collection.", typeVerifier); >> >> It is allowed for Collection to not have a typeVerifier, so we might only want to error if it is not null. > > It could also have a type verifier that isn't a property of WI.Collection.TypeVerifier. We probably shouldn't error, even if it is non-null. In that case, something custom should be provided in the constructor for `contentPlaceholerText`. I'd want an error to show here so that if we use this in the future we don't miss it (exactly like what happened with Canvas). (In reply to Devin Rousso from comment #12) > Comment on attachment 321829 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=321829&action=review > > >>> Source/WebInspectorUI/UserInterface/Views/CollectionContentView.js:154 > >>> + this.representedObject.addEventListener(WI.Collection.Event.ItemRemoved, this._handleItemRemoved, this); > >> > >> I think this might have an issue with items that were added in the following scenario: > >> > >> 1. CollectionContentView is shown (initialLayout and attached) > >> 2. A different item in the NavigationSidebar is selected (detached) > >> 3. Items are added to the Collection (which would fire WI.Collection.Event.ItemAdded) > >> 4. The CollectionContentView is shown again (attached) > >> => Items are missing from the shown list > >> > >> We probably want to modify addContentViewForItem to early-return in the case that item is already in _contentViewMap. This means that all the logic inside initialLayout should really be inside attached, right before the event listeners are added. > > > > We should just track pending added and removed items and sync ContentViews during layout. > > This means that an additional set/array is needed to keep track of added > items, and more management logic is required to handle this. I'd personally > prefer to keep UI logic confined to only things that are visible, and I > think that it would be logically simpler to just iterate over the existing > items and compare to the ones in the Collection. If the view is never > attached() again, then all of this management work is unnecessary. > > for (let item of this._contentViewMap.keys()) { > if (!this.representedObject.items.has(item)) > this.removeContentViewForItem(item); > } I like this approach, but WeakMap keys cannot be iterated. We'll need to make it a regular Map. Created attachment 321864 [details]
Patch
Comment on attachment 321829 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=321829&action=review >>>> Source/WebInspectorUI/UserInterface/Views/CollectionContentView.js:209 >>>> + console.error("Unknown type verifier for collection.", typeVerifier); >>> >>> It is allowed for Collection to not have a typeVerifier, so we might only want to error if it is not null. >> >> It could also have a type verifier that isn't a property of WI.Collection.TypeVerifier. We probably shouldn't error, even if it is non-null. > > In that case, something custom should be provided in the constructor for `contentPlaceholerText`. I'd want an error to show here so that if we use this in the future we don't miss it (exactly like what happened with Canvas). I just made this a console.warn instead, and updated the output message. Created attachment 321963 [details]
Patch
(In reply to Matt Baker from comment #16) > Created attachment 321963 [details] > Patch Two changes: - Use ContentView SupplementalRepresentedObjectsDidChange instead of custom item selection events/hooks. - Update supplemental represented objects/selection style if the selected item's ContentView is removed. - Get rid of ItemClicked event. Go back to just calling WI.showRepresentedObject. Er, three changes. Created attachment 321971 [details]
Patch
Comment on attachment 321971 [details] 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.SupplementalRepresentedObjectsDidChange); 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. Created attachment 322005 [details]
Patch for landing
Comment on attachment 322005 [details] Patch for landing Clearing flags on attachment: 322005 Committed r222573: <http://trac.webkit.org/changeset/222573> All reviewed patches have been landed. Closing bug. |
Summary: Create ResourceCollectionContentView and make CollectionContentView easier to extend. CollectionContentView should be generic, work with any represented object Collection, and not perform any type checking. It should just map items to ContentViews using the provided ContentView constructor. class CollectionContentView extends ContentView { constructor(collection, contentViewConstructor, title) // Public addContentViewForItem(item) removeContentViewForItem(item) // Protected contentViewAdded(contentView) selectedItemChanged(previousSelection, selectedItem) } Subclasses can override `contentViewAdded` and `selectedItemChanged` to add behavior, such as toggling CSS classes or calling WI.showRepresentedObject.