[webkit-reviews] review granted: [Bug 31130] Web Inspector: Add basic support for resource events and marks. Couple of drive-by fixes. Enabling the panel! : [Attachment 42495] [PATCH]

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at hatcher.name> has granted Pavel Feldman
<pfeldman at chromium.org>'s request for review:
Bug 31130: Web Inspector: Add basic support for resource events and marks.
Couple of drive-by fixes. Enabling the panel!
https://bugs.webkit.org/show_bug.cgi?id=31130

Attachment 42495: [PATCH]
https://bugs.webkit.org/attachment.cgi?id=42495&action=review

------- Additional Comments from Timothy Hatcher <timothy at hatcher.name>
> +    this._resourceUrls = {};

Should be _resourceURLs


> +	       return record.data.x + ", " + record.data.y + ", " +
record.data.width + ", " + record.data.height;

Maybe we should just show width x height. I am not sure location will be much
use. That detail coudl be in the popup we plan to add. width and height is more
usefual since it gives you an idea how big.


> +	   case WebInspector.TimelineAgent.RecordType.ResourceSendRequest:
> +	       this._resourceUrls[record.data.identifier] = record.data.url;
>	       return record.data.url;
> +	   case WebInspector.TimelineAgent.RecordType.ResourceReceiveResponse:
> +	   case WebInspector.TimelineAgent.RecordType.ResourceFinish:
> +	       return this._resourceUrls[record.data.identifier];

These two returns should use WebInspector.displayNameForURL().


> +	   case WebInspector.TimelineAgent.RecordType.MarkTimeline:
> +	       return record.data.message;

Hmm I am not sure the whole message should be in the sidebar, but I guess it is
their fault of it is long.


> +	       dataElement.title = this._record.details;

What is .details?