Bug 54920 – [chromium] WebWorkerBase::openFileSystem confuses clang's -Woverloaded-virtual

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Nico Weber]

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?

[Nico Weber]

Hm, for 2., the override can just be removed, since it's no different from the base class version.

[Nico Weber]

Created attachment 83248 [details]
Patch

[Nico Weber]

This is important so that we can turn on -Woverloaded-virtual, which finds unsuccessful override attempts (a very common source of real bugs).

[Nico Weber]

Dave, are you a good person to review this? If not, can you suggest someone?

[Adam Barth]

I wanted to review this, but it looked too complicated.

[Kinuko Yasuda]

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

[Nico Weber]

Created attachment 83338 [details]
Patch

[Nico Weber]

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

[Nico Weber]

Comment on attachment 83338 [details]
Patch

Landed in r79354.