[webkit-reviews] review denied: [Bug 31204] WebInspector: RR for unit test of timeline data : [Attachment 42667] WebInspector: Adds a unit test for Timeline records

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Eric Ayers
<zundel at google.com>'s request for review:
Bug 31204: WebInspector: RR for unit test of timeline data
https://bugs.webkit.org/show_bug.cgi?id=31204

Attachment 42667: WebInspector: Adds a unit test for Timeline records
https://bugs.webkit.org/attachment.cgi?id=42667&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
> -    setTimeout(function() {
> -	   if (window.layoutTestController)
> -	       layoutTestController.showWebInspector();
> -	   window.location.href += "?reload";
> -    }, 0);
> +    if (window.layoutTestController)
> +	   layoutTestController.showWebInspector();

Sync opening of web inspector will fail on windows as far as i remember.

> +	       }, 100); // give other scripts a chance to do work before the
reload

100ms is not going to work on bots. What 'other' scripts and 'work' do you
refer to here?

> -    }
> -    evaluateInWebInspector(toInject.join("\n"), doit);
> +    doit();

It is important that doit is called after evaluateInWebInspector invoked its
callback.

> +if (window.layoutTestController) {
> +    layoutTestController.dumpAsText();
> +    layoutTestController.waitUntilDone();
> +}

You have it in inspector-test.js already.

> +	   }
> +	   markCurrentRecordAsOverhead("onload" + (ignoreLoad ? ":ignoreLoad":
""));
> +    }, 1000);  // Give timer fires created in script a chance to run.  Is 1
second too long to delay?
> +}

Again, time won't help. What is the goal here?

> +// We ignore initial load of the page, enable inspector and initiate reload.

> +// This allows inspector controller to capture events that happen during the

> +// initial page load, and startup the Timeline to capture all the records
from
> +// loading the second page.

I'd like to keep reload logic in one place. We can extend inspector-test to
make it call into
frontend_setUp early in the cycle.