[webkit-reviews] review granted: [Bug 36890] Web Inspector: Timeline Events are not propagated to frontend. : [Attachment 52291] [patch] Second iteration.

[bugzilla-daemon at webkit.org]

Yury Semikhatsky <yurys at chromium.org> has granted Ilya Tikhonovsky
<loislo at chromium.org>'s request for review:
Bug 36890: Web Inspector: Timeline Events are not propagated to frontend.
https://bugs.webkit.org/show_bug.cgi?id=36890

Attachment 52291: [patch] Second iteration.
https://bugs.webkit.org/attachment.cgi?id=52291&action=review

------- Additional Comments from Yury Semikhatsky <yurys at chromium.org>
>	       if (timelineAgent) {
>		   v8::ScriptOrigin origin = function->GetScriptOrigin();
>		   if (!origin.ResourceName().IsEmpty())
> -		      
timelineAgent->willCallFunction(v8ValueToWebCoreString(origin.ResourceName()),
function->GetScriptLineNumber() + 1);
> +		      
timelineAgent->willCallFunction(toWebCoreString(origin.ResourceName()),
function->GetScriptLineNumber() + 1);
It may be useful to see function calls even if resource name is empty.

> +	   if (inspectedPage) {
> +	       InspectorTimelineAgent* timelineAgent =
inspectedPage->inspectorTimelineAgent();
> +	       if (timelineAgent)
> +		   timelineAgent->didCallFunction();
> +	   }
If you move timelineAgent declaration out of the if before the function call
above if (inspectedPage) can be removed.

> +    if (inspectedPage) {
> +	   InspectorTimelineAgent* timelineAgent =
inspectedPage->inspectorTimelineAgent();
> +	   if (timelineAgent)
> +	       timelineAgent->didDispatchEvent();
> +    }
Ditto.


> +    if (inspectedPage) {
> +	   InspectorTimelineAgent* timelineAgent =
inspectedPage->inspectorTimelineAgent();
> +	   if (timelineAgent)
>	       timelineAgent->didDispatchEvent();
>      }
Just checking if (timelineAgentIsActive) here would make the code more clear.


Could we have a test for the case when frame is detached?