Bug 51368 – [Web Timing] requestStart and responseStart should be available even if the document is still loading

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[James Simonsen]

The referenced W3C test is flaky with Chrome. Sometimes requestStart and responseStart are 0. However, they should always be set, because it's an inline script and we must've already started receiving the document.

[James Simonsen]

Created attachment 77062 [details]
Patch

[Tony Gentilcore]

LGTM

Nice find, I'm glad this turned out to be easy to fix.

Fishing for an r+...

[Darin Fisher (:fishd, Google)]

Comment on attachment 77062 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=77062&action=review

Otherwise, LGTM

> WebCore/page/PerformanceTiming.cpp:36
> +#include "CurrentTime.h"

I think it is more conventional to include files from WTF using the notation:

 #include <wtf/CurrentTime.h>

At least, that's what I see being done in WebCore.

[James Simonsen]

Created attachment 78031 [details]
Patch

[WebKit Review Bot]

Comment on attachment 78031 [details]
Patch

Rejecting attachment 78031 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Commit Bot]

Comment on attachment 78031 [details]
Patch

Clearing flags on attachment: 78031

Committed r75120: <http://trac.webkit.org/changeset/75120>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/75120 might have broken Qt Linux Release
The following tests are not passing:
http/tests/misc/webtiming-slow-load.php

[Eric Seidel (no email)]

I don't understand why this test is PHP.

<?php
    echo str_repeat(" ", 100000);
    flush();
    ob_flush();
    sleep(1);
?>

Should not be needed at all.

in line <script> tags run synchronously, so the test itself will have run long before we hit that php block.

Seems we should convert this test back to html and move it out of the http directory.

[James Simonsen]

(In reply to comment #9)
> I don't understand why this test is PHP.
> 
> <?php
>     echo str_repeat(" ", 100000);
>     flush();
>     ob_flush();
>     sleep(1);
> ?>
> 
> Should not be needed at all.
> 
> in line <script> tags run synchronously, so the test itself will have run long before we hit that php block.
> 
> Seems we should convert this test back to html and move it out of the http directory.

It's important that the script run before the main document has finished loading. didReceiveResponse() and didReceiveData() need to be called separately to expose the bug. We use HTTP, pad the size of the document, and use PHP's flush to reliably get the behavior we want.

[Eric Seidel (no email)]

(In reply to comment #10)
> It's important that the script run before the main document has finished loading. didReceiveResponse() and didReceiveData() need to be called separately to expose the bug. We use HTTP, pad the size of the document, and use PHP's flush to reliably get the behavior we want.

I see.  So you need the script to run before the browser actually has cached the entire main resource.  Is that correct?

If so, do we need to mark the test as uncachable to make it repeatable?

[James Simonsen]

(In reply to comment #11)
> (In reply to comment #10)
> > It's important that the script run before the main document has finished loading. didReceiveResponse() and didReceiveData() need to be called separately to expose the bug. We use HTTP, pad the size of the document, and use PHP's flush to reliably get the behavior we want.
> 
> I see.  So you need the script to run before the browser actually has cached the entire main resource.  Is that correct?

Yeah, that would cause the test to break.

> If so, do we need to mark the test as uncachable to make it repeatable?

That sounds good, but it might not be needed in practice. I just checked with Chrome and it looks like it already knows to skip the cache on this test.