Bug 30520 – Enable creation of custom SidebarTreeElements for different ProfileTypes

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Alexander Pavlov (apavlov)]

Currently ProfileSidebarTreeElement is always created as the sidebar tree node for profiles. This is too limiting for some types of profiles which would rather provide their own tree elements (e.g. they require non-empty subtitles).

[Alexander Pavlov (apavlov)]

Created attachment 41433 [details]
patch

[Timothy Hatcher]

Comment on attachment 41433 [details]
patch

> +    sidebarTreeElementForProfile: function(profile)
> +    {
> +        return new WebInspector.ProfileSidebarTreeElement(profile);
> +    },

Why does this return a new tree element each time? Shouldn't this return the same one each time? If not, then this function needs a "create" prefix.


> +    createView: function(profile)
> +    {
> +        return undefined;
> +    },
> +
> +    sidebarTreeElementForProfile: function(profile)
> +    {
> +        return undefined;
> +    },

It is better to use null in cases like this. But why do these exist?


> +    viewForProfile: function(profile)
> +    {
> +        if (!profile._profileView)
> +            profile._profileView = this.createView(profile);
> +        return profile._profileView;
>      }

I am confused why there are multiple versions of these functions. Also this clearly calls "this.createView", which just returns undefined. Can you explain this? I don't think the functions that just return "undefined" are needed or useful.


> +    _parseKey: function(key)
> +    {
> +        var match = key.match(/^([^\/]+)\/([^\/]+)$/);
> +        if (match)
> +            return [unescape(match[1]), unescape(match[2])];
> +        return undefined;
> +    },

Better to return null.


> +    _viewForProfile: function(profile)
> +    {
> +        for (var key in this._profilesIdMap) {
> +            if (this._profilesIdMap[key] === profile) {
> +                var parsedKey = this._parseKey(key);
> +                if (parsedKey)
> +                    return this.getProfileType(parsedKey[1]).viewForProfile(profile);
> +            }
> +        }
> +        return undefined;
> +    },

Better to return null.


r-. This needs clerified and not have functions that do nothing but return undefined. (Or explained with good comments.)

[Pavel Feldman]

> +    _viewForProfile: function(profile)
> +    {
> +        for (var key in this._profilesIdMap) {
> +            if (this._profilesIdMap[key] === profile) {
> +                var parsedKey = this._parseKey(key);
> +                if (parsedKey)
> +                    return this.getProfileType(parsedKey[1]).viewForProfile(profile);

This sounds much more complex than "profile.type.viewForProfile(profile)".
Should we make profile have this property in order to get rid of key parsing?

[Alexander Pavlov (apavlov)]

(In reply to comment #3)
> > +    _viewForProfile: function(profile)
> > +    {
> > +        for (var key in this._profilesIdMap) {
> > +            if (this._profilesIdMap[key] === profile) {
> > +                var parsedKey = this._parseKey(key);
> > +                if (parsedKey)
> > +                    return this.getProfileType(parsedKey[1]).viewForProfile(profile);
> 
> This sounds much more complex than "profile.type.viewForProfile(profile)".
> Should we make profile have this property in order to get rid of key parsing?

Good point. I was trying to decrease coupling as much as possible but this one seems fine.

[Timothy Hatcher]

Comment on attachment 41433 [details]
patch

I understand this design better now. You should add comments to each function that the ProfileType subclasses should implement.

I also think ProfileType.sidebarTreeElementForProfile should be ProfileType.createSidebarTreeElementForProfile to show it returns a new instance each time.

[Alexander Pavlov (apavlov)]

Created attachment 41493 [details]
patch (fixed)

[Pavel Feldman]

Comment on attachment 41493 [details]
patch (fixed)

> +    // Must be implemented by subclasses.
> +    createView: function(profile)

Nit, can be done in subsequent patch: usually abstract methods that are implemented in concrete classes are named as doCreateSomething*.
Otherwise it is easy to get confused on what should be overriden and what not.

> +    {
> +        throw new Error("Needs implemented.");
> +    },

"Not implemented" ?

>  
> +        profile._type = profileType;

This looks like a hack: what if concrete profile object already has this property?
Either declare it a hack and name it "__profilesPanelProfileType" or make profile define "type" getter and put it into docs.

[Timothy Hatcher]

(In reply to comment #7)
> (From update of attachment 41493 [details])
> > +    // Must be implemented by subclasses.
> > +    createView: function(profile)
> 
> Nit, can be done in subsequent patch: usually abstract methods that are
> implemented in concrete classes are named as doCreateSomething*.
> Otherwise it is easy to get confused on what should be overriden and what not.

This is not something we normally do in WebKit. I think the normal names are better.

[Alexander Pavlov (apavlov)]

(In reply to comment #7)
> (From update of attachment 41493 [details])
> > +    // Must be implemented by subclasses.
> > +    createView: function(profile)
> 
> Nit, can be done in subsequent patch: usually abstract methods that are
> implemented in concrete classes are named as doCreateSomething*.
> Otherwise it is easy to get confused on what should be overriden and what not.

Timothy has commented on this one.

> > +    {
> > +        throw new Error("Needs implemented.");
> > +    },
> 
> "Not implemented" ?

This is fine, as in "Has a comment that points out this needs implemented when network headers are added" (WebCore/ChangeLog, 2008-05-13) or at the bottom of Comment #2. And, it was suggested by Timothy.

> >  
> > +        profile._type = profileType;
> 
> This looks like a hack: what if concrete profile object already has this
> property?
> Either declare it a hack and name it "__profilesPanelProfileType" or make
> profile define "type" getter and put it into docs.

Profiles should be definitely ProfileType-agnostic since ProfileTypes deal with visual representation of profiles, and they are bound only at the point of the addProfileHeader() invocation. That's why I was parsing the keys (which was cleaner than "caching" ProfileTypes inside profiles) rather than using an explicit association. I will stick to "__profilesPanelProfileType".

[Alexander Pavlov (apavlov)]

Created attachment 41560 [details]
patch (rename field)

Renamed the "cached" ProfileType field inside profiles.

[WebKit Commit Bot]

Comment on attachment 41560 [details]
patch (rename field)

Clearing flags on attachment: 41560

Committed r49905: <http://trac.webkit.org/changeset/49905>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.