There are multiple issues here. 1.) WebWorkerBase::openFileSystem() has a comment that points out that this method is not an implementation of the superclass's WebFrameClient::openFileSystem(). Instead of the comment, the method should have a different name, to make this obvious. I propose openFileSystemForWorker(). 2.) WebWorkerClientImpl::openFileSystem() tries to override one of these 2 methods, but fails, since the signature matches neither. I think it tries to override the 1st (i.e. the soon-to-be openFileSystemForWorker) 3.) WebCommonWorkerClient has an old implementation of openFileSystemForWorker() that's missing the |create| parameter with a comment that this needs to go away once it's no longer used by chrome (left-over from a 2-sided patch). kinuko, does this sound about right?
Hm, for 2., the override can just be removed, since it's no different from the base class version.
Created attachment 83248 [details] Patch
This is important so that we can turn on -Woverloaded-virtual, which finds unsuccessful override attempts (a very common source of real bugs).
Dave, are you a good person to review this? If not, can you suggest someone?
I wanted to review this, but it looked too complicated.
Comment on attachment 83248 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=83248&action=review Thanks for cleaning up the confusing overloads and messed-up half-way two sided patches. > Source/WebKit/chromium/public/WebCommonWorkerClient.h:96 > + virtual void openFileSystemForWorker(WebFileSystem::Type type, long long size, bool create, WebFileSystemCallbacks* callbacks) Hmm I think actually what we want to leave here is the one WITHOUT the bool create flag. The flag was added to WebFrameClient::openFileSystem mainly for devtools support, and we don't need such a flag in worker cases. So can we just rename the method but leave the parameters as is? (Or if you think the naming is ok we can just leave it as is, as I believe for WebCommonWorkerClient there're no worries for confused overloads.) Again, thanks for cleaning up this and sorry for the mess. > Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:220 > + commonClient->openFileSystemForWorker(type, size, /*create=*/false, MainThreadFileSystemCallbacks::createLeakedPtr(bridge, mode)); As I commented above can we not add the 'create' flag? (And if we add it it's got to be true).
Created attachment 83338 [details] Patch
kinuko, thank you for your comments. I understand the design much better now. The new patch is a lot simpler. It does 2 things: 1.) Rename just WebWorkerBase::openFileSystem() to openFileSystemForWorker() and change the one caller. 2.) Remove an unsuccessful override attempt from WebWorkerClientImpl
Comment on attachment 83338 [details] Patch Landed in r79354.