Bug 54920

Summary: [chromium] WebWorkerBase::openFileSystem confuses clang's -Woverloaded-virtual
Product: WebKit Reporter: Nico Weber <thakis>
Component: WebKit Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, kinuko, levin
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 54367    
Attachments:
Description Flags
Patch
none
Patch jamesr: review+, thakis: commit-queue-

Nico Weber
Reported 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?
Attachments
Patch (7.51 KB, patch)
2011-02-21 18:05 PST, Nico Weber 		no flags
DetailsFormatted DiffDiff
Patch (4.77 KB, patch)
2011-02-22 10:30 PST, Nico Weber 		jamesr: review+
thakis: commit-queue-
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Nico Weber
Comment 1 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.
Nico Weber
Comment 2 2011-02-21 18:05:41 PST
Created attachment 83248 [details] Patch
Nico Weber
Comment 3 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).
Nico Weber
Comment 4 2011-02-21 18:10:19 PST
Dave, are you a good person to review this? If not, can you suggest someone?
Adam Barth
Comment 5 2011-02-21 19:50:40 PST
I wanted to review this, but it looked too complicated.
Kinuko Yasuda
Comment 6 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).
Nico Weber
Comment 7 2011-02-22 10:30:49 PST
Created attachment 83338 [details] Patch
Nico Weber
Comment 8 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
Nico Weber
Comment 9 2011-02-22 13:47:10 PST
Comment on attachment 83338 [details] Patch Landed in r79354.
Note You need to log in before you can comment on or make changes to this bug.