[webkit-reviews] review denied: [Bug 14370] Inspector's timeline should record when certain DOM events fired : [Attachment 40467] Fix

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at hatcher.name> has denied Brian Weinstein
<bweinstein at apple.com>'s request for review:
Bug 14370: Inspector's timeline should record when certain DOM events fired
https://bugs.webkit.org/show_bug.cgi?id=14370

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

------- Additional Comments from Timothy Hatcher <timothy at hatcher.name>

> +	   InspectorController* controller = page()->inspectorController();
> +	   if (controller) {
> +	       if
(controller->isMainResourceLoader(f->loader()->documentLoader(), url())) {
> +		   controller->mainResourceFiredDOMContentEvent();
> +	       }
> +	   }

Better written as:

> +	   InspectorController* controller = page()->inspectorController();
> +	   if (controller &&
controller->isMainResourceLoader(f->loader()->documentLoader(), url()))
> +	       controller->mainResourceFiredDOMContentEvent();

But I think it would be better to not expose isMainResourceLoader and just
always call "firedDOMContentEvent", etc. And let InspectorController decide to
keep it if isMainResourceLoader true. That way the call sites in Document.cpp
and DOMWindow.cpp are as small as possible.

So:

> +	   if (InspectorController* controller = page()->inspectorController())

> +	       controller->firedDOMContentEvent(f->loader()->documentLoader());



> +    bool isMainResourceLoader(DocumentLoader* loader, const KURL&
requestUrl);

See my previous comment about not exposing this a public.

> +	       jsonObject.set("domContentTime", m_domContentEventTime);

This should be "domContentEventTime" to match "loadEventTime".

> +	   
> +	   
> +	   if (this.calculator.startAtZero) {

Remove the extra line above the if.



> +	   if (eventTime !== -1 && !this.startAtZero)
> +	       var percent = ((eventTime - this.minimumBoundary) /
this.boundarySpan) * 100;
> +	   else
> +	       var percent = 0;
> +    
> +	   return percent;

Simplier as:

> +	   if (eventTime !== -1 && !this.startAtZero)
> +	       return ((eventTime - this.minimumBoundary) / this.boundarySpan)
* 100;
> +	   return 0;


> +	   if (payload.loadEventTime) {
> +	       // This loadEventTime is for the main resource, and we want to
show it
> +	       // for all resources on this page. This means we want to set it
as a member
> +	       // of the resource panel instead of the individual resource.
> +	       if (this.panels.resources)
> +		   this.panels.resources.mainResourceLoadTime =
payload.loadEventTime;

> +	   }
> +	   if (payload.domContentTime) {

Add a line break between those lines.

> +	       if (this.panels.resources)
> +		   this.panels.resources.mainResourceDOMContentTime =
payload.domContentTime;

Add a comment referencing the previous comment, so you know the reason this
goes on ResourcesPanel.


> +    InspectorController* controller =
frame()->page()->inspectorController();
> +    if (controller) {
> +	   if
(controller->isMainResourceLoader(frame()->loader()->documentLoader(),
frame()->document()->url())) {
> +	       controller->mainResourceFiredLoadEvent();
> +	   }
> +    }

See my earlier comment about simplifying this call site.