see patch
Created attachment 63879 [details] Patch
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).
(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.
Created attachment 65126 [details] Patch The comment for the ref() will be included in a further patch.
Comment on attachment 65126 [details] Patch r=me
Comment on attachment 65126 [details] Patch Clearing flags on attachment: 65126 Committed r65835: <http://trac.webkit.org/changeset/65835>
All reviewed patches have been landed. Closing bug.