Bug 54287

Created attachment 82137 [details] Proposed patch

Created attachment 82138 [details] Proposed patch Add the issue/bug and description to the changelog

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

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

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

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

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?

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.

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.

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

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

Created attachment 82379 [details] Proposed patch Revert change to SubresourceLoader that exposed finishTime accessor. Change SubresourceLoaderClient::didFinishLoading to pass finishTime as a parameter.

Created attachment 82380 [details] Proposed patch Simplifying patch to only fix finishTime issue.

Created attachment 82383 [details] Proposed patch, with LFs Fixed line endings. Bear with me, I'm trying to do this from Windows machine.

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=

Created attachment 82386 [details] Proposed patch One more time

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=

Created attachment 82388 [details] Proposed patch

Created attachment 82394 [details] Proposed patch Prepared on my Mac, hopefully I'll have better luck.

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.

Created attachment 82397 [details] Proposed Patch Argggh.

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.

Created attachment 82401 [details] Proposed Patch Addressed parameter issues.

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 on attachment 82401 [details] Proposed Patch Clearing flags on attachment: 82401 Committed r78558: <http://trac.webkit.org/changeset/78558>

All reviewed patches have been landed. Closing bug.