Bug 127117 – Web Inspector: add probe manager and model objects to the frontend

• Component: Web Inspector
• Status: RESOLVED FIXED

[Blaze Burg]

Splitting this from the actual views so that those can be reviewed in parallel.

[Radar WebKit Bug Importer]

<rdar://problem/15836243>

[Blaze Burg]

Created attachment 222967 [details]
patch

[Timothy Hatcher]

Comment on attachment 222967 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=222967&action=review

I'll look at this more later today.

> Source/JavaScriptCore/inspector/protocol/Debugger.json:41
> +                { "name": "identifier", "type": "integer", "optional": true, "description": "A frontend-assigned identifier for this breakpoint action." }

Lets us "id" here too.

> Source/WebInspectorUI/UserInterface/BreakpointAction.js:69
> +    get identifier()
> +    {
> +        return this._identifier;
> +    },

"id" is fine. I usually only spell out identifier when the "I" is capitalized cause "Id" looks weird to me.

But we can standardize on "identifier", I'm cool with that.

> Source/WebInspectorUI/UserInterface/DebuggerManager.js:448
> +    getNextBreakpointActionIdentifier: function()
> +    {
> +        return this._nextBreakpointActionIdentifier++;
> +    },

I'd like this better as a normal getter.

> Source/WebInspectorUI/UserInterface/ProbeManager.js:118
> +        var knownProbeIds = this._knownProbeIdsForBreakpoint.get(breakpoint);

This is a case where I would use "knownProbeIdentifiers".

> Source/WebInspectorUI/UserInterface/ProbeManager.js:119
> +        var seenProbeIds = new Set;

Ditto.

> Source/WebInspectorUI/UserInterface/ProbeManager.js:122
> +            var probeId = probeAction.identifier;

Ditto.

[Joseph Pecoraro]

Comment on attachment 222967 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=222967&action=review

> Source/WebInspectorUI/ChangeLog:36
> +        be able to recieved breakpoint added events.

Typo: recieved

> Source/JavaScriptCore/inspector/ScriptDebugListener.h:72
> +    virtual void didSampleProbe(JSC::ExecState*, const ScriptBreakpointAction&, int hitCount, const Deprecated::ScriptValue& result) = 0;

I wonder if we should rename this breakpointActionProbe, to match breakpointActionLog and breakpointActionSound. I'm ambivalent though. Did sample probe is good.

> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:231
> +        object->getNumber("identifier", &identifier);

Nit: ASCIILiteral("identifier")

>> Source/JavaScriptCore/inspector/protocol/Debugger.json:41
>> +                { "name": "identifier", "type": "integer", "optional": true, "description": "A frontend-assigned identifier for this breakpoint action." }
> 
> Lets us "id" here too.

This should also be an $ref to BreakpointActionIdentifier.

> Source/WebInspectorUI/UserInterface/DebuggerManager.js:73
> +        var breakpoints = this._breakpointsSetting.value.map(function(breakpointCookie) {
> +            return new WebInspector.Breakpoint(breakpointCookie);
> +        });
> +        breakpoints.forEach(this.addBreakpoint, this);

I liked this better as a single for loop. Is there an advantage to using map/forEach over a for..of loop here?

> Source/WebInspectorUI/UserInterface/ProbeManager.js:42
> +    // N.B. saved breakpoints should not be restored on the first event loop turn, because

N.B.?

> Source/WebInspectorUI/UserInterface/ProbeManager.js:61
> +        this._probeSetsByBreakpoint.forEach(sets.push.bind(this));

Hmm, I don't think this actually works:

    > var map = new Map;
    < undefined
    
    > map.set(1,1); map.set(2,2);
    < Map
    
    > arr = []; map.forEach(arr.push.bind(this)); arr
    < []
    
    > arr = []; map.forEach(arr.push.bind(arr)); arr
    < [1, 1, 2, 2]

Just do the easy route:

    > arr = []; for (var x of map.values()) arr.push(x); arr
    < [1, 2]

> Source/WebInspectorUI/UserInterface/ProbeManager.js:140
> +        }.bind(this));

Nit: No need for .bind(this) here. Just use the optional thisObject parameter to forEach.

    "}.bind(this));" => "}, this);"

> Source/WebInspectorUI/UserInterface/ProbeManager.js:160
> +        }.bind(this));

Ditto

> Source/WebInspectorUI/UserInterface/ProbeManager.js:163
> +    // This function will create a probe set for the specified breakpoint if it does not exist.

Not sure if this comment adds value. We have this idiom in a lot of places. There was even a discussion on giving them a consistent naming but nothing came out of it. Also, we don't normally prefix methods with "set" or "get".

> Source/WebInspectorUI/UserInterface/ProbeObject.js:35
> +WebInspector.ProbeObject = function(probeId, breakpoint, expression)

How about just WebInspector.Probe?

> Source/WebInspectorUI/UserInterface/ProbeObject.js:37
> +    WebInspector.Object.call(this);

We often assert in constructors to catch possible issues in advance.

    console.assert(probeId);
    console.assert(breakpoint);

> Source/WebInspectorUI/UserInterface/ProbeObject.js:97
> +    // Protected (Called by ProbeManager)
> +
> +    addSample: function(sample)

Sounds like this is public to me.

> Source/WebInspectorUI/UserInterface/ProbeSetDataFrame.js:42
> +Object.defineProperty(WebInspector.ProbeSetDataFrame, "compare",
> +{
> +    value: function(a, b) {
> +        console.assert(a instanceof WebInspector.ProbeSetDataFrame, a);
> +        console.assert(b instanceof WebInspector.ProbeSetDataFrame, b);
> +
> +        return a.index - b.index;
> +    }
> +});

Is there an advantage to this over just WebInspector.ProbeSetDataFrame.compare = function() { ... }? Property descriptor differences?

> Source/WebInspectorUI/UserInterface/ProbeSetDataFrame.js:53
> +        return "%d".format(this._index);

Seems overkill. How about String(this._index) or ("" + this._index).

> Source/WebInspectorUI/UserInterface/ProbeSetDataFrame.js:74
> +    set isSeparator(value)
> +    {
> +        this._separator = !!value;
> +    },

A setter named "isSeparator" is weird. How about not giving this a setter and just making it an argument of the constructor (or a separate constructor).

> Source/WebInspectorUI/UserInterface/ProbeSetDataFrame.js:86
> +        return probeSet.probes.filter(function(probe) {
> +            return !this.hasOwnProperty(probe.identifier);
> +        }.bind(this));

Same thing with unnecessary bind. Array.prototype.filter takes an optional thisObject:
<https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/filter>

> Source/WebInspectorUI/UserInterface/ProbeSetDataFrame.js:97
> +        for (var i = 0; i < keys.length; ++i)

for..of!

> Source/WebInspectorUI/UserInterface/ProbeSetDataTable.js:41
> +    FrameInserted: "probe-set-frame-inserted",
> +    FrameReplaced: "probe-set-frame-replaced",
> +    SeparatorInserted: "probe-set-separator-inserted",
> +    SeparatorReplaced: "probe-set-separator-replaced",
> +    WillRemove: "probe-set-data-table-will-remove"

One is prefixed with "probe-set-data-table", the rest aren't. For consistency they should all have it.

Also, some of these events are unused (each of the replaced ones). Are there plans to add them or are they stale?

> Source/WebInspectorUI/UserInterface/ProbeSetDataTable.js:99
> +        for (var i = 0; i < frames.length; ++i)

for..of!

> Source/WebInspectorUI/UserInterface/ProbeSetDataTable.js:107
> +        for (var i = 0; i < frames.length; ++i)

for..of!

> Source/WebInspectorUI/UserInterface/ProbeSetDataTable.js:109
> +            if (frames[i][probe.identifier])
> +                delete frames[i][probe.identifier];

No need for the if check here. Just call "delete frame[probe.identifier]".

> Source/WebInspectorUI/UserInterface/ProbeSetObject.js:29
> +WebInspector.ProbeSetObject = function(breakpoint)

How about just calling this ProbeSet or ProbeObjectSet? "ProbeSetObject" sounds weird to me.

> Source/WebInspectorUI/UserInterface/ProbeSetObject.js:42
> +    WebInspector.Frame.addEventListener(WebInspector.Frame.Event.MainResourceDidChange, this._mainResourceChanged, this);
> +    WebInspector.ProbeObject.addEventListener(WebInspector.ProbeObject.Event.SampleAdded, this._sampleCollected, this);
> +    WebInspector.Breakpoint.addEventListener(WebInspector.Breakpoint.Event.ResolvedStateDidChange, this._breakpointResolvedStateDidChange, this);

r- for this. It looks like it will cause leaks of all ProbeSetObjects. "this" should call removeEventListener somewhere for WebInspector.Frame and WebInspector.Breakpoint.

Otherwise this ProbeSetObject will forever live in those singleton event listener lists. The lists are not yet WeakMaps, and that is not possible with the current WebMap APIs.

> Source/WebInspectorUI/UserInterface/ProbeSetObject.js:79
> +        this._breakpoint.probeActions.forEach(this._breakpoint.removeAction.bind(this._breakpoint));

Sounds like the Breakpoint.prototype.clearActions method you just added would be more efficient.

> Source/WebInspectorUI/UserInterface/ProbeSetObject.js:94
> +        // This will fire ProbeManager.Event.ProbeAdded, then ProbeObject.Event.ExpressionChanged.
> +        var newAction = this.breakpoint.createAction(WebInspector.BreakpointAction.Type.Probe);
> +        newAction.data = expression;

I like the comment, but perhaps we can avoid the extra churn and just pass createAction the breakpoint action data!

Just pass something to replace the null inside createAction:

    var newAction = new WebInspector.BreakpointAction(this, type, null);

> Source/WebInspectorUI/UserInterface/ProbeSetObject.js:97
> +    // Protected (called by ProbeManager.js)

Again, this sounds public to me then.

> LayoutTests/inspector-protocol/model/probe-manager-add-remove-actions.html:20
> +    function incrementTick(event)
> +    {
> +        InspectorTest.log("Hit test checkpoint event #" + currentTicks + " with type: " + event.type);
> +        if (++currentTicks === expectedTicks)
> +            InspectorTest.completeTest();
> +    }

r- for this too. I don't see this log in the output. So, the samples are not triggering?

[Blaze Burg]

Comment on attachment 222967 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=222967&action=review

>> Source/WebInspectorUI/UserInterface/BreakpointAction.js:69
>> +    },
> 
> "id" is fine. I usually only spell out identifier when the "I" is capitalized cause "Id" looks weird to me.
> 
> But we can standardize on "identifier", I'm cool with that.

As long as that's the rule, I can follow it. Updated the style guide: https://trac.webkit.org/wiki/WebInspectorCodingStyleGuide

>> Source/WebInspectorUI/UserInterface/DebuggerManager.js:73
>> +        breakpoints.forEach(this.addBreakpoint, this);
> 
> I liked this better as a single for loop. Is there an advantage to using map/forEach over a for..of loop here?

nope, I think I wrote this when for..of didn't work yet

>> Source/WebInspectorUI/UserInterface/ProbeManager.js:61
>> +        this._probeSetsByBreakpoint.forEach(sets.push.bind(this));
> 
> Hmm, I don't think this actually works:
> 
>     > var map = new Map;
>     < undefined
> 
>     > map.set(1,1); map.set(2,2);
>     < Map
> 
>     > arr = []; map.forEach(arr.push.bind(this)); arr
>     < []
> 
>     > arr = []; map.forEach(arr.push.bind(arr)); arr
>     < [1, 1, 2, 2]
> 
> Just do the easy route:
> 
>     > arr = []; for (var x of map.values()) arr.push(x); arr
>     < [1, 2]

i think it should be sets.push.bind(sets) but I agree that's ugly. What about just returning an iterator? i.e.,

get probeSets() { return this._probeSetsByBreakpoint.values(); }

The getter is only ever used to iterate over the sets.

>> Source/WebInspectorUI/UserInterface/ProbeManager.js:118
>> +        var knownProbeIds = this._knownProbeIdsForBreakpoint.get(breakpoint);
> 
> This is a case where I would use "knownProbeIdentifiers".

OK

>> Source/WebInspectorUI/UserInterface/ProbeManager.js:140
>> +        }.bind(this));
> 
> Nit: No need for .bind(this) here. Just use the optional thisObject parameter to forEach.
> 
>     "}.bind(this));" => "}, this);"

I keep forgetting. I added it to the style guide.

>> Source/WebInspectorUI/UserInterface/ProbeManager.js:163
>> +    // This function will create a probe set for the specified breakpoint if it does not exist.
> 
> Not sure if this comment adds value. We have this idiom in a lot of places. There was even a discussion on giving them a consistent naming but nothing came out of it. Also, we don't normally prefix methods with "set" or "get".

OK

>> Source/WebInspectorUI/UserInterface/ProbeSetDataFrame.js:42
>> +});
> 
> Is there an advantage to this over just WebInspector.ProbeSetDataFrame.compare = function() { ... }? Property descriptor differences?

No, just cribbing from some other file that did that. In that case it would be important (on Array or Date)

>> Source/WebInspectorUI/UserInterface/ProbeSetDataFrame.js:74
>> +    },
> 
> A setter named "isSeparator" is weird. How about not giving this a setter and just making it an argument of the constructor (or a separate constructor).

This should have been commented better. This is set on the last data frame collected before a main frame navigation. So you can't know at construction time whether it's a separator (i.e., last record) or not. I'm trying to remember why it had to be at the end instead of beginning, but can't recall at the moment.

>> Source/WebInspectorUI/UserInterface/ProbeSetDataTable.js:41
>> +    WillRemove: "probe-set-data-table-will-remove"
> 
> One is prefixed with "probe-set-data-table", the rest aren't. For consistency they should all have it.
> 
> Also, some of these events are unused (each of the replaced ones). Are there plans to add them or are they stale?

Oops, replaced events are only used with replay integration. I'll re-add them later.

>> Source/WebInspectorUI/UserInterface/ProbeSetObject.js:29
>> +WebInspector.ProbeSetObject = function(breakpoint)
> 
> How about just calling this ProbeSet or ProbeObjectSet? "ProbeSetObject" sounds weird to me.

OK

>> Source/WebInspectorUI/UserInterface/ProbeSetObject.js:42
>> +    WebInspector.Breakpoint.addEventListener(WebInspector.Breakpoint.Event.ResolvedStateDidChange, this._breakpointResolvedStateDidChange, this);
> 
> r- for this. It looks like it will cause leaks of all ProbeSetObjects. "this" should call removeEventListener somewhere for WebInspector.Frame and WebInspector.Breakpoint.
> 
> Otherwise this ProbeSetObject will forever live in those singleton event listener lists. The lists are not yet WeakMaps, and that is not possible with the current WebMap APIs.

Oops, missed it in willRemove. I used to use a helper class that would keep a single list of all listeners and bulk add/remove.

>> Source/WebInspectorUI/UserInterface/ProbeSetObject.js:79
>> +        this._breakpoint.probeActions.forEach(this._breakpoint.removeAction.bind(this._breakpoint));
> 
> Sounds like the Breakpoint.prototype.clearActions method you just added would be more efficient.

We only want to clear probe actions, not any other ones.

>> LayoutTests/inspector-protocol/model/probe-manager-add-remove-actions.html:20
>> +    }
> 
> r- for this too. I don't see this log in the output. So, the samples are not triggering?

Hmm, strange that it finishes but doesn't log. I'll look into it.

[Timothy Hatcher]

Comment on attachment 222967 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=222967&action=review

>>> Source/WebInspectorUI/UserInterface/ProbeManager.js:61
>>> +        this._probeSetsByBreakpoint.forEach(sets.push.bind(this));
>> 
>> Hmm, I don't think this actually works:
>> 
>>     > var map = new Map;
>>     < undefined
>> 
>>     > map.set(1,1); map.set(2,2);
>>     < Map
>> 
>>     > arr = []; map.forEach(arr.push.bind(this)); arr
>>     < []
>> 
>>     > arr = []; map.forEach(arr.push.bind(arr)); arr
>>     < [1, 1, 2, 2]
>> 
>> Just do the easy route:
>> 
>>     > arr = []; for (var x of map.values()) arr.push(x); arr
>>     < [1, 2]
> 
> i think it should be sets.push.bind(sets) but I agree that's ugly. What about just returning an iterator? i.e.,
> 
> get probeSets() { return this._probeSetsByBreakpoint.values(); }
> 
> The getter is only ever used to iterate over the sets.

Returning an iterator can work, but it can give wrong results if reused. Maybe call the getter probeSetsIterator?

[Blaze Burg]

Created attachment 223166 [details]
patch2

[Blaze Burg]

(In reply to comment #6)
> > i think it should be sets.push.bind(sets) but I agree that's ugly. What about just returning an iterator? i.e.,
> > 
> > get probeSets() { return this._probeSetsByBreakpoint.values(); }
> > 
> > The getter is only ever used to iterate over the sets.
> 
> Returning an iterator can work, but it can give wrong results if reused. Maybe call the getter probeSetsIterator?

I decided against returning an iterator after all.

[Timothy Hatcher]

Comment on attachment 223166 [details]
patch2

View in context: https://bugs.webkit.org/attachment.cgi?id=223166&action=review

> Source/WebInspectorUI/UserInterface/DebuggerManager.js:514
> -        console.assert(!breakpoint.id);
> +        console.assert(!breakpoint.identifier);

Can you revert this?

> Source/WebInspectorUI/UserInterface/DebuggerManager.js:517
> -        if (breakpoint.id || breakpoint.disabled)
> +        if (breakpoint.identifier || breakpoint.disabled)

Ditto.

> Source/WebInspectorUI/UserInterface/DebuggerManager.js:527
> -            breakpoint.id = breakpointIdentifier;
> +            breakpoint.identifier = breakpointIdentifier;

Ditto.

> Source/WebInspectorUI/UserInterface/DebuggerManager.js:571
> -        if (!breakpoint.id)
> +        if (!breakpoint.identifier)

Ditto.

> Source/WebInspectorUI/UserInterface/DebuggerManager.js:579
> -            delete this._breakpointIdMap[breakpoint.id];
> +            delete this._breakpointIdMap[breakpoint.identifier];

Ditto.

> Source/WebInspectorUI/UserInterface/DebuggerManager.js:581
> -            breakpoint.id = null;
> +            breakpoint.identifier = null;

Ditto.

> Source/WebInspectorUI/UserInterface/DebuggerManager.js:590
> -        DebuggerAgent.removeBreakpoint(breakpoint.id, didRemoveBreakpoint.bind(this));
> +        DebuggerAgent.removeBreakpoint(breakpoint.identifier, didRemoveBreakpoint.bind(this));

Ditto.

> Source/WebInspectorUI/UserInterface/DebuggerManager.js:599
> -        if (!breakpoint.id || breakpoint.disabled)
> +        if (!breakpoint.identifier || breakpoint.disabled)

Ditto.

[Blaze Burg]

Committed r163477: <http://trac.webkit.org/changeset/163477>