Bug 74084 - Web Inspector: Introduce a Map class allowing to store values indexed by arbitrary objects.
Summary: Web Inspector: Introduce a Map class allowing to store values indexed by arbi...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Vsevolod Vlasov
URL:
Keywords:
Depends on:
Blocks: 73086
  Show dependency treegraph
 
Reported: 2011-12-08 06:20 PST by Vsevolod Vlasov
Modified: 2011-12-09 01:26 PST (History)
10 users (show)

See Also:


Attachments
Patch (16.89 KB, patch)
2011-12-08 06:31 PST, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
Patch (11.95 KB, patch)
2011-12-08 11:29 PST, Vsevolod Vlasov
pfeldman: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vsevolod Vlasov 2011-12-08 06:20:49 PST
Introduce a Map class allowing to store values indexed by arbitrary objects.
Comment 1 Vsevolod Vlasov 2011-12-08 06:31:00 PST
Created attachment 118381 [details]
Patch
Comment 2 Pavel Feldman 2011-12-08 06:39:43 PST
Comment on attachment 118381 [details]
Patch

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.
Comment 3 Timothy Hatcher 2011-12-08 10:04:20 PST
Comment on attachment 118381 [details]
Patch

What is the motivation for this change?
Comment 4 Vsevolod Vlasov 2011-12-08 11:29:56 PST
Created attachment 118433 [details]
Patch
Comment 5 Vsevolod Vlasov 2011-12-08 11:36:40 PST
(In reply to comment #3)
> (From update of attachment 118381 [details])
> What is the motivation for this change?

The Map class is added because I am going to need it the new scripts panel navigator I am developing to map script to a tree elements that is representing it. Note, that I am going to have several TreeOutlines there (scripts, content scripts, later snippets), so using tree outline's findTreeElement is not convenient.

Similar functionality is currently used in scripts panel when we set file select options as a field on UISourceCode. This is making code hard to understand and can be abstracted out using this Map class.

Moving TreeOutline to use Map once we have it seems natural to me.
Comment 6 Vsevolod Vlasov 2011-12-08 11:37:45 PST
(In reply to comment #2)
> (From update of attachment 118381 [details])
> 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

It was done to make it possible to remove identifiers from an object once object is not a key in any map anymore, but you are right, this seems redundant.
Comment 7 Vsevolod Vlasov 2011-12-09 01:26:21 PST
Committed r102447: <http://trac.webkit.org/changeset/102447>