[webkit-reviews] review denied: [Bug 127117] Web Inspector: add probe manager and model objects to the frontend : [Attachment 222967] patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Brian Burg <bburg at apple.com>'s
request for review:
Bug 127117: Web Inspector: add probe manager and model objects to the frontend
https://bugs.webkit.org/show_bug.cgi?id=127117

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

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
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_Objec
ts/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.MainResourceDidCha
nge, this._mainResourceChanged, this);
> +   
WebInspector.ProbeObject.addEventListener(WebInspector.ProbeObject.Event.Sample
Added, this._sampleCollected, this);
> +   
WebInspector.Breakpoint.addEventListener(WebInspector.Breakpoint.Event.Resolved
StateDidChange, 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?