Bug 54287 – SubresourceLoader should expose finish time

• Component: Platform
• Status: RESOLVED FIXED

[Bill Budge]

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.

[Bill Budge]

Created attachment 82137 [details]
Proposed patch

[Bill Budge]

Created attachment 82138 [details]
Proposed patch

Add the issue/bug and description to the changelog

[David Levin]

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."

[Bill Budge]

Created attachment 82171 [details]
Proposed patch with testing field filled in.

[David Levin]

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 :).

[WebKit Commit Bot]

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

[David Levin]

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).

[Bill Budge]

I think it is needed, because the DummyClient is a  ThreadableLoaderClient, and its didFinishLoading method doesn't receive the finish time.

[Bill Budge]

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?

[Bill Budge]

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.

[Bill Budge]

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.

[David Levin]

(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)

[Bill Budge]

I was hesitant to change an interface, but if that's OK, it would be the cleanest solution. I'll put together a patch.

[Bill Budge]

Created attachment 82379 [details]
Proposed patch

Revert change to SubresourceLoader that exposed finishTime accessor. Change SubresourceLoaderClient::didFinishLoading to pass finishTime as a parameter.

[Bill Budge]

Created attachment 82380 [details]
Proposed patch

Simplifying patch to only fix finishTime issue.

[Bill Budge]

Created attachment 82383 [details]
Proposed patch, with LFs

Fixed line endings. Bear with me, I'm trying to do this from Windows machine.

[David Levin]

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=

[Bill Budge]

Created attachment 82386 [details]
Proposed patch

One more time

[Bill Budge]

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=

[Bill Budge]

Created attachment 82388 [details]
Proposed patch

[Bill Budge]

Created attachment 82394 [details]
Proposed patch

Prepared on my Mac, hopefully I'll have better luck.

[WebKit Review Bot]

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.

[Bill Budge]

Created attachment 82397 [details]
Proposed Patch

Argggh.

[David Levin]

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.

[Bill Budge]

Created attachment 82401 [details]
Proposed Patch

Addressed parameter issues.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 82401 [details]:

http/tests/websocket/tests/multiple-connections.html bug 53825 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 82401 [details]
Proposed Patch

Clearing flags on attachment: 82401

Committed r78558: <http://trac.webkit.org/changeset/78558>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.