| Summary: | [Web Timing] requestStart and responseStart should be available even if the document is still loading | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | James Simonsen <simonjam> | ||||||
| Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | abarth, commit-queue, eric, fishd, tonyg, webkit.review.bot | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | PC | ||||||||
| OS: | OS X 10.5 | ||||||||
| URL: | http://test.w3.org/webperf/tests/submission/Microsoft/NavigationTiming/test_timing_attributes_ordering_simple.htm | ||||||||
| Attachments: |
|
||||||||
|
Description
James Simonsen
2010-12-20 18:45:34 PST
Created attachment 77062 [details]
Patch
LGTM Nice find, I'm glad this turned out to be easy to fix. Fishing for an r+... 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. Created attachment 78031 [details]
Patch
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 on attachment 78031 [details] Patch Clearing flags on attachment: 78031 Committed r75120: <http://trac.webkit.org/changeset/75120> All reviewed patches have been landed. Closing bug. 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 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.
(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. (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? (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. |