Summary: | Web Inspector: implement Multimap.prototype.take() | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Blaze Burg <bburg> | ||||||
Component: | Web Inspector | Assignee: | Blaze Burg <bburg> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | bburg, ews-watchlist, hi, inspector-bugzilla-changes, joepeck, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Blaze Burg
2020-11-20 17:34:13 PST
Created attachment 414746 [details]
Patch
Comment on attachment 414746 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=414746&action=review r=me, with some behavior changes > Source/WebInspectorUI/UserInterface/Base/Multimap.js:94 > + return new Set; this should be `undefined` to match what `get` does when the `key` is not in `_map` > Source/WebInspectorUI/UserInterface/Base/Multimap.js:101 > + let taken = valueSet.take(value); > + > + if (!valueSet.size) > + this._map.delete(key); > + > + return new Set([taken]); this should match `Set.prototype.take` ``` let result = valueSet.take(value); if (!valueSet.size) this._map.delete(key); return result; ``` Comment on attachment 414746 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=414746&action=review > Source/WebInspectorUI/UserInterface/Base/Utilities.js:187 > - let exists = this.has(key); > - if (exists) > + if (this.has(key)) { > this.delete(key); > - return exists; > + return key; > + } > + > + return undefined; > } We should remove `Set.prototype.take`, it serves no purpose. The way this was originally written seems to be identical to `Set.prototype.delete`. The way this is no written is weird for falsey values (E.g. `if (set.take(0)) { ... }`). Lets just remove it and use `Set.prototype.delete` as appropriate? Created attachment 414763 [details]
Revised fix
Committed r270149: <https://trac.webkit.org/changeset/270149> All reviewed patches have been landed. Closing bug and clearing flags on attachment 414763 [details]. |