Bug 54287

Summary: SubresourceLoader should expose finish time
Product: WebKit Reporter: Bill Budge <bbudge>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, levin, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 53925    
Attachments:
Description Flags
Proposed patch
none
Proposed patch
levin: review-
Proposed patch with testing field filled in.
levin: review-, commit-queue: commit-queue-
Proposed patch
none
Proposed patch
none
Proposed patch, with LFs
levin: review-
Proposed patch
none
Proposed patch
none
Proposed patch
none
Proposed Patch
levin: review-
Proposed Patch none

Description Bill Budge 2011-02-11 08:29:37 PST
The SubresourceLoader should expose the finish time value that it receives from it's internal ResourceHandle. This value will be needed by clients of SubresourceLoader that drive the WebURLLoaderClient interface, which returns this value in the didFinishLoading callback.
Comment 1 Bill Budge 2011-02-11 08:31:59 PST
Created attachment 82137 [details]
Proposed patch
Comment 2 Bill Budge 2011-02-11 08:49:24 PST
Created attachment 82138 [details]
Proposed patch

Add the issue/bug and description to the changelog
Comment 3 David Levin 2011-02-11 10:31:31 PST
Comment on attachment 82138 [details]
Proposed patch

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

Try uploading the patch with the fix for the item I noted and hopefully that will fix the "pink" ews bubbles as well (which indicate that the patch doesn't apply).

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

Either explain why test aren't necessary, impossible, or add a test.

In this case "No new functionality exposed, so no new tests."
Comment 4 Bill Budge 2011-02-11 14:01:14 PST
Created attachment 82171 [details]
Proposed patch with testing field filled in.
Comment 5 David Levin 2011-02-11 14:02:44 PST
Comment on attachment 82171 [details]
Proposed patch with testing field filled in.

fwiw, the functionality isn't exposed (externally to js) as opposed to exposing trivial functionality to js which would need a test :).
Comment 6 WebKit Commit Bot 2011-02-11 14:04:22 PST
Comment on attachment 82171 [details]
Proposed patch with testing field filled in.

Rejecting attachment 82171 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'apply-..." exit_code: 2

Last 500 characters of output:
 u'--reviewer', u'David Levin', u'--force']" exit_code: 2

Parsed 3 diffs from patch file(s).
patching file Source/WebCore/ChangeLog
patch: **** malformed patch at line 20:          Reviewed by Yury Semikhatsky.

patching file Source/WebCore/loader/SubresourceLoader.cpp
patching file Source/WebCore/loader/SubresourceLoader.h
patch unexpectedly ends in middle of line

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'David Levin', u'--force']" exit_code: 2

Full output: http://queues.webkit.org/results/7867761
Comment 7 David Levin 2011-02-11 14:12:41 PST
Comment on attachment 82171 [details]
Proposed patch with testing field filled in.

ok, this doesn't apply but I got to thinking about it more.  I looked at the original bug and then started wondering why we are pushing this down into subresource loader when only one user needs it.

After all, it seems that instead of having purely a "DummyClient", you could fill in the "didFinishLoading" method and make it capture the finishTime. Then we don't add more code into subresource loader (which only one user of it cares about).
Comment 8 Bill Budge 2011-02-12 08:03:26 PST
I think it is needed, because the DummyClient is a  ThreadableLoaderClient, and its didFinishLoading method doesn't receive the finish time.
Comment 9 Bill Budge 2011-02-12 08:18:50 PST
Alternatively, I could catch it in DocumentThreadableLoader (which does receive the finishTime in its callback), and store it as a property which my derived loader could access. It's a little less messy, since the property could be protected. Do you prefer that?
Comment 10 Bill Budge 2011-02-12 08:38:14 PST
OK, now I think I see what you're saying. There is a SubresourceLoaderClient override possible. I'll look into it and revert the change to SubresourceLoader.
Comment 11 Bill Budge 2011-02-14 11:46:44 PST
I looked at it and I don't see a way to get the finishTime out of SubresourceLoader unless we:

A) Store the finish time and add a finishTime() accessor, or...
B) Change SubresourceLoaderClient::didFinishLoading() to add the finishTime value, or...
C) Add a createSubresourceLoader method to DocumentThreadableLoader, so clients can derive a SubresourceLoader class that catches the value.
Comment 12 David Levin 2011-02-14 11:49:05 PST
(In reply to comment #11)
> I looked at it and I don't see a way to get the finishTime out of SubresourceLoader unless we:
> 
> A) Store the finish time and add a finishTime() accessor, or...
> B) Change SubresourceLoaderClient::didFinishLoading() to add the finishTime value, or...
> C) Add a createSubresourceLoader method to DocumentThreadableLoader, so clients can derive a SubresourceLoader class that catches the value.

What about B? (which is what I was thinking but didn't state clearly)
Comment 13 Bill Budge 2011-02-14 12:06:41 PST
I was hesitant to change an interface, but if that's OK, it would be the cleanest solution. I'll put together a patch.
Comment 14 Bill Budge 2011-02-14 16:15:34 PST
Created attachment 82379 [details]
Proposed patch

Revert change to SubresourceLoader that exposed finishTime accessor. Change SubresourceLoaderClient::didFinishLoading to pass finishTime as a parameter.
Comment 15 Bill Budge 2011-02-14 16:31:06 PST
Created attachment 82380 [details]
Proposed patch

Simplifying patch to only fix finishTime issue.
Comment 16 Bill Budge 2011-02-14 16:37:25 PST
Created attachment 82383 [details]
Proposed patch, with LFs

Fixed line endings. Bear with me, I'm trying to do this from Windows machine.
Comment 17 David Levin 2011-02-14 16:41:07 PST
Comment on attachment 82383 [details]
Proposed patch, with LFs

You need to fix the overloads in IconLoader.h and CachedResourceRequest.h
(I found these using code search.)
http://www.google.com/codesearch?as_q=SubresourceLoaderClient&btnG=Search+Code&hl=en&vert=chromium&filesuggest=&as_lang=&as_filename=&as_class=&as_function=&as_case=
Comment 18 Bill Budge 2011-02-14 16:43:59 PST
Created attachment 82386 [details]
Proposed patch

One more time
Comment 19 Bill Budge 2011-02-14 16:44:55 PST
Ah, thanks. VS is no help here.

(In reply to comment #17)
> (From update of attachment 82383 [details])
> You need to fix the overloads in IconLoader.h and CachedResourceRequest.h
> (I found these using code search.)
> http://www.google.com/codesearch?as_q=SubresourceLoaderClient&btnG=Search+Code&hl=en&vert=chromium&filesuggest=&as_lang=&as_filename=&as_class=&as_function=&as_case=
Comment 20 Bill Budge 2011-02-14 16:54:34 PST
Created attachment 82388 [details]
Proposed patch
Comment 21 Bill Budge 2011-02-14 17:40:08 PST
Created attachment 82394 [details]
Proposed patch

Prepared on my Mac, hopefully I'll have better luck.
Comment 22 WebKit Review Bot 2011-02-14 17:41:23 PST
Attachment 82394 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 23 Bill Budge 2011-02-14 17:45:02 PST
Created attachment 82397 [details]
Proposed Patch

Argggh.
Comment 24 David Levin 2011-02-14 17:52:13 PST
Comment on attachment 82397 [details]
Proposed Patch

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

Just some minor nits to address and then this one will be done.

> Source/WebCore/loader/DocumentThreadableLoader.cpp:234
> +void DocumentThreadableLoader::didFinishLoading(SubresourceLoader* loader, double finishTime)

I would leave out the parameter name "finishTime". Since it isn't used in this function, I believe it will cause a compile error (with some builds like the WebKit build on OSX) if you include it.

> Source/WebCore/loader/SubresourceLoaderClient.h:51
> +    virtual void didFinishLoading(SubresourceLoader*, double) { }

It would be good to include the name here /*finishTime*/ like other places so that one can look at this header and tell what this parameter is about.

> Source/WebCore/loader/cache/CachedResourceRequest.cpp:140
> +void CachedResourceRequest::didFinishLoading(SubresourceLoader* loader, double finishTime)

I would leave out the parameter name "finishTime" here. Same reasoning as before.

> Source/WebCore/loader/icon/IconLoader.cpp:134
> +void IconLoader::didFinishLoading(SubresourceLoader* resourceLoader, double finishTime)

I would leave out the parameter name "finishTime" here. Same reasoning as before.
Comment 25 Bill Budge 2011-02-14 18:02:12 PST
Created attachment 82401 [details]
Proposed Patch

Addressed parameter issues.
Comment 26 WebKit Commit Bot 2011-02-15 07:27:21 PST
The commit-queue encountered the following flaky tests while processing attachment 82401 [details]:

http/tests/websocket/tests/multiple-connections.html bug 53825 (author: abarth@webkit.org)
The commit-queue is continuing to process your patch.
Comment 27 WebKit Commit Bot 2011-02-15 07:29:12 PST
Comment on attachment 82401 [details]
Proposed Patch

Clearing flags on attachment: 82401

Committed r78558: <http://trac.webkit.org/changeset/78558>
Comment 28 WebKit Commit Bot 2011-02-15 07:29:18 PST
All reviewed patches have been landed.  Closing bug.