Bug 219231 - Web Inspector: implement Multimap.prototype.take()
Summary: Web Inspector: implement Multimap.prototype.take()
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: Brian Burg
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-11-20 17:34 PST by Brian Burg
Modified: 2020-11-21 11:10 PST (History)
6 users (show)

See Also:


Attachments
Patch (8.94 KB, patch)
2020-11-20 17:36 PST, Brian Burg
no flags Details | Formatted Diff | Diff
Revised fix (8.72 KB, patch)
2020-11-21 09:59 PST, Brian Burg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Burg 2020-11-20 17:34:13 PST
Splitting off from some other work.
Comment 1 Brian Burg 2020-11-20 17:36:33 PST
Created attachment 414746 [details]
Patch
Comment 2 Devin Rousso 2020-11-20 17:44:55 PST
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 3 Joseph Pecoraro 2020-11-20 17:50:08 PST
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?
Comment 4 Brian Burg 2020-11-21 09:59:39 PST
Created attachment 414763 [details]
Revised fix
Comment 5 EWS 2020-11-21 11:09:14 PST
Committed r270149: <https://trac.webkit.org/changeset/270149>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 414763 [details].
Comment 6 Radar WebKit Bug Importer 2020-11-21 11:10:21 PST
<rdar://problem/71656735>