Bug 51818 - GeolocationPositionCache should not use ScriptExecutionContext::Task
Summary: GeolocationPositionCache should not use ScriptExecutionContext::Task
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Steve Block
URL:
Keywords:
Depends on: 51857
Blocks: 51814
  Show dependency treegraph
 
Reported: 2011-01-03 06:02 PST by Steve Block
Modified: 2011-07-29 06:11 PDT (History)
3 users (show)

See Also:


Attachments
Patch (22.76 KB, patch)
2011-01-03 11:13 PST, Steve Block
no flags Details | Formatted Diff | Diff
Patch (17.38 KB, patch)
2011-01-03 12:54 PST, Steve Block
no flags Details | Formatted Diff | Diff
Patch (16.72 KB, patch)
2011-01-03 13:50 PST, Steve Block
levin: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Steve Block 2011-01-03 06:02:07 PST
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.
Comment 1 Steve Block 2011-01-03 06:23:42 PST
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.
Comment 2 Steve Block 2011-01-03 11:13:43 PST
Created attachment 77826 [details]
Patch
Comment 3 Darin Adler 2011-01-03 11:30:59 PST
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 4 Steve Block 2011-01-03 12:51:00 PST
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.
Comment 5 Steve Block 2011-01-03 12:54:00 PST
Created attachment 77836 [details]
Patch
Comment 6 Darin Adler 2011-01-03 13:00:37 PST
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 7 Steve Block 2011-01-03 13:37:02 PST
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.
Comment 8 Steve Block 2011-01-03 13:50:31 PST
Created attachment 77844 [details]
Patch
Comment 9 David Levin 2011-01-03 14:00:28 PST
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.
Comment 10 David Levin 2011-01-03 14:06:15 PST
(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).
Comment 11 Steve Block 2011-01-03 14:29:07 PST
> 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?
Comment 12 David Levin 2011-01-03 16:10:34 PST
(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.
Comment 13 Steve Block 2011-01-03 16:20:41 PST
> 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?
Comment 14 David Levin 2011-01-03 17:51:15 PST
I created a bug and made this dependent on it.

I hope that 51814 isn't blocked anymore.
Comment 15 Steve Block 2011-07-29 06:11:08 PDT
GeolocationPositionCache was removed in Bug 65289