[webkit-reviews] review denied: [Bug 91528] Web Inspector: Embeddable Web Inspector : [Attachment 154992] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Gabriel Peal
<gpeal at google.com>'s request for review:
Bug 91528: Web Inspector: Embeddable Web Inspector
https://bugs.webkit.org/show_bug.cgi?id=91528

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

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


> Source/WebCore/ChangeLog:8
> +	   Additional information of the change such as approach, rationale.
Please add per-function descriptions below (OOPS!).

You should only have one ChangeLog entry. It should explain what changes as why
here.

> Source/WebCore/inspector/front-end/InspectorView.js:76
> +	   if (typeof x === "number")

x should be a panel. We should annotate it with @param {WebInspector.Panel} x.

> Source/WebCore/inspector/front-end/TimelineModel.js:172
> +    loadRecords: function(records)

Since this is private to this file, should be _loadRecords

> Source/WebCore/inspector/front-end/TimelineModel.js:211
> +    loadFromUrl: function(url)

WebKit uses all caps for abbreviations (loadFromURL)

> Source/WebCore/inspector/front-end/TimelineModel.js:214
> +	   var xhr = new XMLHttpRequest();

You should call InspectorFrontendHost.loadResourceSynchronously here and move
this implementation into the InspectorFrontendHostStub.

> Source/WebCore/inspector/front-end/TimelineModel.js:216
> +	   xhr.onreadystatechange = dataReceived;

We don't use self = this method, we bind callbacks to this instead. i.e.
xhr.onreadystatechange = dataReceived.bind(this);

> Source/WebCore/inspector/front-end/TimelineModel.js:221
> +	       if (xhr.status === 200) {

Prefer guard expressions and early returns.

> Source/WebCore/inspector/front-end/TimelinePanel.js:776
> +    loadFromUrl: function(url)

loadFromURL

> Source/WebCore/inspector/front-end/inspector.js:47
> +	   var hiddenPanelsString = InspectorFrontendHost.hiddenPanels() || "";


Move this code into InspectorFrontendHostStub's hiddenPanels method.

> Source/WebCore/inspector/front-end/inspector.js:417
> +    } else if (InspectorFrontendHost.isStub) {

WebKit does not put "else" after a branch with "return". Just start with if
(... on the next line.

> Source/WebCore/inspector/front-end/inspector.js:419
> +	   WebInspector.inspectorView.setCurrentPanel(0);

What if there are no panels? Instead of using 0, you should expose panels:
function() on InspectorView that would return this._panelOrder.slice(). Then
you would be able to fetch panels from here and select the first one if any.

> Source/WebCore/inspector/front-end/inspector.js:420
> +	   if (typeof WebInspector.queryParamsObject["timelineUrl"] !==
"undefined")

There should be no special casing for timeline in here. I would put the code
below into the InspectorFrontendAPI::loadTimelineFromURL and would dispatch
query parameters into calls on InspectorFrontendAPI in some generic manner. In
this case, it would be:

inspector.html?callAPI=loadTimelineURL&param=<url> or similarly.