Bug 51818 – GeolocationPositionCache should not use ScriptExecutionContext::Task

• Status: RESOLVED
• Resolution: INVALID
• Component: WebCore Misc.

[Steve Block]

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.

[Steve Block]

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.

[Steve Block]

Created attachment 77826 [details]
Patch

[Darin Adler]

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 {}

[Steve Block]

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.

[Steve Block]

Created attachment 77836 [details]
Patch

[Darin Adler]

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.

[Steve Block]

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.

[Steve Block]

Created attachment 77844 [details]
Patch

[David Levin]

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.

[David Levin]

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

[Steve Block]

> 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?

[David Levin]

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

[Steve Block]

> 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?

[David Levin]

I created a bug and made this dependent on it.

I hope that 51814 isn't blocked anymore.

[Steve Block]

GeolocationPositionCache was removed in Bug 65289