Bug 54920 - [chromium] WebWorkerBase::openFileSystem confuses clang's -Woverloaded-virtual
Summary: [chromium] WebWorkerBase::openFileSystem confuses clang's -Woverloaded-virtual
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 54367
  Show dependency treegraph
 
Reported: 2011-02-21 17:47 PST by Nico Weber
Modified: 2011-02-22 13:47 PST (History)
3 users (show)

See Also:


Attachments
Patch (7.51 KB, patch)
2011-02-21 18:05 PST, Nico Weber
no flags Details | Formatted Diff | Diff
Patch (4.77 KB, patch)
2011-02-22 10:30 PST, Nico Weber
jamesr: review+
thakis: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nico Weber 2011-02-21 17:47:58 PST
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?
Comment 1 Nico Weber 2011-02-21 17:57:11 PST
Hm, for 2., the override can just be removed, since it's no different from the base class version.
Comment 2 Nico Weber 2011-02-21 18:05:41 PST
Created attachment 83248 [details]
Patch
Comment 3 Nico Weber 2011-02-21 18:10:02 PST
This is important so that we can turn on -Woverloaded-virtual, which finds unsuccessful override attempts (a very common source of real bugs).
Comment 4 Nico Weber 2011-02-21 18:10:19 PST
Dave, are you a good person to review this? If not, can you suggest someone?
Comment 5 Adam Barth 2011-02-21 19:50:40 PST
I wanted to review this, but it looked too complicated.
Comment 6 Kinuko Yasuda 2011-02-22 02:17:49 PST
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).
Comment 7 Nico Weber 2011-02-22 10:30:49 PST
Created attachment 83338 [details]
Patch
Comment 8 Nico Weber 2011-02-22 10:32:04 PST
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 9 Nico Weber 2011-02-22 13:47:10 PST
Comment on attachment 83338 [details]
Patch

Landed in r79354.