GeolocationPositionCache uses ScriptExecutionContext::Task to coordinate its background thread. GeolocationPositionCache has nothing to do with ScriptExecutionContext so should not use this class. Instead it should use some other generic task infrastructure.
The convenience of using ScriptExecutionTask::Task is that CrossThreadTask provides generic machinery to create a ScriptExecutionContext::Task which simply invokes a supplied method. This machinery is heavily templated and modifying it to support tasks both with and without a ScriptExecutionContext would be messy. The classes that currently use tasks with MessageQueue and which do not use a ScriptExecutionContext are as follows ... FileThread LocalStorageThread DatabaseThread Each of these uses a custom task class which could not be replaced by a simple generic task similar to CrossThreadTask. So the only user of such a generic task would be GeolocationPositionCache. I don't think it's worth adding the complexity of this generic machinery for a single user, though this can be revisited in the future. So I think the correct approach is to add a custom task class for GeolocationPositionCache. We can also add a base class to provide a performTask() method for all of the existing task classes. Patch coming soon.
Created attachment 77826 [details] Patch
Comment on attachment 77826 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77826&action=review It seems a little strange to make a function object and name it Task. But OK. > Source/JavaScriptCore/wtf/Task.h:40 > + virtual ~Task() {} We put spaces between braces in cases like that. > WebCore/fileapi/FileThread.h:58 > - class Task : public Noncopyable { > + class FileTask : public Task { Does this really need to be renamed? You could just say this: class Task : public WTF::Task That would make the patch smaller, but I guess could be a bit confusing. With the name FileTask it seems it could be made a non-member. > WebCore/page/GeolocationPositionCache.cpp:48 > + CacheTask(GeolocationPositionCache* cache) : m_cache(cache) {} We put spaces in those {}
Comment on attachment 77826 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77826&action=review >> WebCore/fileapi/FileThread.h:58 >> + class FileTask : public Task { > > Does this really need to be renamed? You could just say this: > > class Task : public WTF::Task > > That would make the patch smaller, but I guess could be a bit confusing. With the name FileTask it seems it could be made a non-member. OK, I'll leave the name unchanged to minimise the size of the patch.
Created attachment 77836 [details] Patch
Comment on attachment 77836 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77836&action=review > WebCore/page/GeolocationPositionCache.cpp:49 > + CacheTask(GeolocationPositionCache* cache) : m_cache(cache) { } > + GeolocationPositionCache* m_cache; It would be more elegant for m_cache to be private and have a protected accessor function cache(). > WebCore/page/GeolocationPositionCache.cpp:56 > + CacheReadTask(GeolocationPositionCache* cache) : CacheTask(cache) {} In my previous review, I didn’t call out every case of {} that should be changed to { }. This was another. > WebCore/page/GeolocationPositionCache.cpp:64 > + CacheWriteTask(GeolocationPositionCache* cache) : CacheTask(cache) {} In my previous review, I didn’t call out every case of {} that should be changed to { }. This was another. > WebCore/storage/LocalStorageTask.cpp:-56 > -LocalStorageTask::~LocalStorageTask() > -{ > -} This may be a controversial change. In the past I have removed these from DOM classes and then others added them back. > WebCore/storage/LocalStorageTask.h:-44 > - ~LocalStorageTask(); This may be a controversial change. In the past I have removed these from DOM classes and then others added them back.
Comment on attachment 77836 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77836&action=review >> WebCore/storage/LocalStorageTask.h:-44 >> - ~LocalStorageTask(); > > This may be a controversial change. In the past I have removed these from DOM classes and then others added them back. OK, I'll leave these in. Will upload another version so that levin, who suggested the change, can see the final version.
Created attachment 77844 [details] Patch
Comment on attachment 77844 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77844&action=review > WebCore/page/GeolocationPositionCache.cpp:46 > +class CacheTask : public Task { I don't think you need this stuff. Instead FileThreadTask.h should only rely on Task now and could be moved to wtf with Task.
(In reply to comment #1) > The convenience of using ScriptExecutionTask::Task is that CrossThreadTask provides generic machinery to create a ScriptExecutionContext::Task which simply invokes a supplied method. This machinery is heavily templated and modifying it to support tasks both with and without a ScriptExecutionContext would be messy. > > The classes that currently use tasks with MessageQueue and which do not use a ScriptExecutionContext are as follows ... > FileThread > LocalStorageThread > DatabaseThread > > Each of these uses a custom task class which could not be replaced by a simple generic task similar to CrossThreadTask. So the only user of such a generic task would be GeolocationPositionCache. I don't think it's worth adding the complexity of this generic machinery for a single user, though this can be revisited in the future. If you are considering this larger thing, it would be worthwhile to talk to Kinuko first (who has been thinking about this).
> Instead FileThreadTask.h should only rely on Task now and could be moved to > wtf with Task. OK, but this will require moving CrossThreadCopier and CrossThreadTaskTraits to WTF too. If FileThreadTask is to become a generic utility, any preference on naming? SimpleTask? CallbackTask?
(In reply to comment #11) > > Instead FileThreadTask.h should only rely on Task now and could be moved to > > wtf with Task. > OK, but this will require moving CrossThreadCopier and CrossThreadTaskTraits to WTF too. > > If FileThreadTask is to become a generic utility, any preference on naming? SimpleTask? CallbackTask? On second hand, I don't think that works because it is creating a FileTask. Sorry :( At this point, I don't understand why this patch is creating a wtf task because it doesn't seem to benefit the patch at all. (I wouldn't do this just because it seems like there are these common methods unless there is something more.) I do think that getting rid of the ScriptExecutingContext::Task from here is good simply because it is really hackish to use it in this way. I will admit that you wished to leave it as is for now that could last for a bit longer as I believe a general clean up of this stuff will be in progress soon and it can take on this case too.) Thanks for working on this. Sorry that it is a pain.
> On second hand, I don't think that works because it is creating a FileTask. There's nothing specific to files in FileTask, so I think this would work. > I do think that getting rid of the ScriptExecutingContext::Task from here is > good simply because it is really hackish to use it in this way. I will admit > that you wished to leave it as is for now that could last for a bit longer as > I believe a general clean up of this stuff will be in progress soon and it > can take on this case too.) OK, I'm happy to postpone the cleanup if you are. Do you have a bug I can point to for this more general cleanup?
I created a bug and made this dependent on it. I hope that 51814 isn't blocked anymore.
GeolocationPositionCache was removed in Bug 65289