Bug 51368 - [Web Timing] requestStart and responseStart should be available even if the document is still loading
Summary: [Web Timing] requestStart and responseStart should be available even if the d...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL: http://test.w3.org/webperf/tests/subm...
Keywords:
Depends on:
Blocks:
 
Reported: 2010-12-20 18:45 PST by James Simonsen
Modified: 2011-02-24 17:52 PST (History)
6 users (show)

See Also:


Attachments
Patch (4.81 KB, patch)
2010-12-20 18:50 PST, James Simonsen
no flags Details | Formatted Diff | Diff
Patch (6.42 KB, patch)
2011-01-05 13:12 PST, James Simonsen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Simonsen 2010-12-20 18:45:34 PST
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.
Comment 1 James Simonsen 2010-12-20 18:50:12 PST
Created attachment 77062 [details]
Patch
Comment 2 Tony Gentilcore 2010-12-29 14:22:36 PST
LGTM

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

Fishing for an r+...
Comment 3 Darin Fisher (:fishd, Google) 2011-01-03 13:45:23 PST
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.
Comment 4 James Simonsen 2011-01-05 13:12:56 PST
Created attachment 78031 [details]
Patch
Comment 5 WebKit Review Bot 2011-01-05 13:22:21 PST
Comment on attachment 78031 [details]
Patch

Rejecting attachment 78031 [details] from commit-queue.

simonjam@chromium.org 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.
Comment 6 WebKit Commit Bot 2011-01-05 17:22:58 PST
Comment on attachment 78031 [details]
Patch

Clearing flags on attachment: 78031

Committed r75120: <http://trac.webkit.org/changeset/75120>
Comment 7 WebKit Commit Bot 2011-01-05 17:23:03 PST
All reviewed patches have been landed.  Closing bug.
Comment 8 WebKit Review Bot 2011-01-06 03:35:50 PST
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
Comment 9 Eric Seidel (no email) 2011-02-24 13:57:00 PST
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.
Comment 10 James Simonsen 2011-02-24 15:50:51 PST
(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.
Comment 11 Eric Seidel (no email) 2011-02-24 15:52:45 PST
(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?
Comment 12 James Simonsen 2011-02-24 17:52:23 PST
(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.