[webkit-reviews] review denied: [Bug 74084] Web Inspector: Introduce a Map class allowing to store values indexed by arbitrary objects. : [Attachment 118381] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Vsevolod Vlasov
<vsevik at chromium.org>'s request for review:
Bug 74084: Web Inspector: Introduce a Map class allowing to store values
indexed by arbitrary objects.
https://bugs.webkit.org/show_bug.cgi?id=74084

Attachment 118381: Patch
https://bugs.webkit.org/attachment.cgi?id=118381&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=118381&action=review


> Source/WebCore/inspector/front-end/Map.js:49
> +	       key[this._mapFieldName] = this._lastObjectIdentifier++;

Why would you need such complexity? Why lastObjectIdentifier can't be static
and keys have the same identifiers in all the maps? Also, the pattern for this
kind of code it

var objectIdentifier = key[name];
if (!objectIdentifier) {
    objectIdentifier = ++this.lastObjectIdentifier;
    key[name] = objectIdentifier
}

but as I mentioned, you should not need it.

> Source/WebCore/inspector/front-end/inspector.html:42
> +    <script type="text/javascript" src="treeoutline.js"></script>

treeoutline and utilities.s were considered reusable utilities with no
WebInspector* dependency. I don't think we should change that.

> Source/WebCore/inspector/front-end/treeoutline.js:-41
> -    this._knownTreeElements = [];

Just leave this as it or define Map in the utilities.js with no WebInspector
prefix.