[webkit-reviews] review granted: [Bug 51818] GeolocationPositionCache should not use ScriptExecutionContext::Task : [Attachment 77836] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Steve Block <steveblock at google.com>'s
request for review:
Bug 51818: GeolocationPositionCache should not use ScriptExecutionContext::Task
https://bugs.webkit.org/show_bug.cgi?id=51818

Attachment 77836: Patch
https://bugs.webkit.org/attachment.cgi?id=77836&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.