Bug 43714 – Move filehandling into fileLoadTimer callback

• Component: Platform
• Status: RESOLVED FIXED

[Patrick R. Gansterer]

see patch

[Patrick R. Gansterer]

Created attachment 63879 [details]
Patch

[Adam Roben (:aroben)]

Comment on attachment 63879 [details]
Patch

> -void ResourceHandle::fileLoadTimer(Timer<ResourceHandle>* timer)
> +void ResourceHandle::fileLoadTimer(Timer<ResourceHandle>*)
>  {
> +    String path = firstRequest().url().path();
> +    // Windows does not enjoy a leading slash on paths.
> +    if (path[0] == '/')
> +        path = path.substring(1);
> +
> +    int queryPos = path.find('?');
> +    if (queryPos != -1)
> +        path.remove(queryPos);

I'm surprised that KURL::path would return a string that contains a query component. That seems like a bad bug. Is that really happening?

It seems like it would be better to use KURL::fileSystemPath anyway.

> +    if (fileHandle == INVALID_HANDLE_VALUE) {
> +        client()->didFail(this, ResourceError());
> +        deref();
> +        return;
> +    }

Where is the ref() that this deref() is balancing? I don't see it in this patch. Was it a bug that we weren't dereffing before? We usually include a comment at both the ref() and deref() callsites mentioning where the balancing call is.

> +    int dotPos = path.reverseFind('.');
> +    int slashPos = path.reverseFind('/');
> +
> +    if (slashPos < dotPos && dotPos != -1) {
> +        String ext = path.substring(dotPos + 1);
> +        response.setMimeType(MIMETypeRegistry::getMIMETypeForExtension(ext));
> +    }

This code seems entirely new and doesn't match the ChangeLog (which claims this patch just moves code).

[Patrick R. Gansterer]

(In reply to comment #2)
> (From update of attachment 63879 [details])
> > -void ResourceHandle::fileLoadTimer(Timer<ResourceHandle>* timer)
> > +void ResourceHandle::fileLoadTimer(Timer<ResourceHandle>*)
> >  {
> > +    String path = firstRequest().url().path();
> > +    // Windows does not enjoy a leading slash on paths.
> > +    if (path[0] == '/')
> > +        path = path.substring(1);
> > +
> > +    int queryPos = path.find('?');
> > +    if (queryPos != -1)
> > +        path.remove(queryPos);
> 
> I'm surprised that KURL::path would return a string that contains a query component. That seems like a bad bug. Is that really happening?
I had this problem at least once, so i added this.

> It seems like it would be better to use KURL::fileSystemPath anyway.
Didn't see that function. ;-)

> 
> > +    if (fileHandle == INVALID_HANDLE_VALUE) {
> > +        client()->didFail(this, ResourceError());
> > +        deref();
> > +        return;
> > +    }
> 
> Where is the ref() that this deref() is balancing? I don't see it in this patch. Was it a bug that we weren't dereffing before? We usually include a comment at both the ref() and deref() callsites mentioning where the balancing call is.
The ref() is directly at the start(). At the moment the ResourceHandleWin does a "delete this"!!! I'll add comments.

> 
> > +    int dotPos = path.reverseFind('.');
> > +    int slashPos = path.reverseFind('/');
> > +
> > +    if (slashPos < dotPos && dotPos != -1) {
> > +        String ext = path.substring(dotPos + 1);
> > +        response.setMimeType(MIMETypeRegistry::getMIMETypeForExtension(ext));
> > +    }
> 
> This code seems entirely new and doesn't match the ChangeLog (which claims this patch just moves code).
Ok, I'll update the ChangeLog.

[Patrick R. Gansterer]

Created attachment 65126 [details]
Patch

The comment for the ref() will be included in a further patch.

[Adam Roben (:aroben)]

Comment on attachment 65126 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 65126 [details]
Patch

Clearing flags on attachment: 65126

Committed r65835: <http://trac.webkit.org/changeset/65835>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.