Bug 66088 – Remove finishTime parameter from ThreadableLoaderClient::didFinishLoading()

• Component: WebCore Misc.
• Status: RESOLVED WONTFIX

[Nate Chapin]

This variable is unused in all of the ThreadableLoaderClients except the chromium-specific AssociatedURLLoader, which passes it on to its clients, which ignore it.

We should be able to just hardcode a garbage value in AssociatedURLLoader and remove the parameter from a bunch of didFinishLoading()s.

[Nate Chapin]

Created attachment 103660 [details]
patch

[WebKit Review Bot]

Attachment 103660 [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/loader/WorkerThreadableLoader.cpp:219:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

Comment on attachment 103660 [details]
patch

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

Why was this even added in the first place?

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:168
> +    m_client->didFinishLoading(m_loader, 0);

Can you add a FIXME to the WebKit API to remove the parameter?

[Nate Chapin]

(In reply to comment #3)
> (From update of attachment 103660 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=103660&action=review
> 
> Why was this even added in the first place?
> 
> > Source/WebKit/chromium/src/AssociatedURLLoader.cpp:168
> > +    m_client->didFinishLoading(m_loader, 0);
> 
> Can you add a FIXME to the WebKit API to remove the parameter?

It's because AssociatedURLLoader inherits from WebURLLoader, which has a finishTime parameter on didFinishLoading that is actually in use.

[Adam Barth]

Sounds like we need input from fishd here.  Removing all this plumbing sounds like a good idea, especially b/c it helps you with your later patches.

[Darin Fisher (:fishd, Google)]

Comment on attachment 103660 [details]
patch

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

>>> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:168
>>> +    m_client->didFinishLoading(m_loader, 0);
>> 
>> Can you add a FIXME to the WebKit API to remove the parameter?
> 
> It's because AssociatedURLLoader inherits from WebURLLoader, which has a finishTime parameter on didFinishLoading that is actually in use.

It makes me a bit sad to promise an interface that we cannot support.  It doesn't seem justified to invent WebAssociatedURLLoader{Client} over this one thing though, and I can see that if we currently have no consumers in Chrome that care about this finishTime, that we would want to kill it.  It just seems like one of those warts that someone might trip over in the future (after the beta has been cut).

[Nate Chapin]

Comment on attachment 103660 [details]
patch

The more I think about it, the less I like this solution of removing the finishTime parameter.  I may revive this someday, but I don't think it will be any time soon.