[webkit-reviews] review denied: [Bug 50825] GeolocationPositionCache should do database access on background thread : [Attachment 76878] Patch

[bugzilla-daemon at webkit.org]

Jeremy Orlow <jorlow at chromium.org> has denied Steve Block
<steveblock at google.com>'s request for review:
Bug 50825: GeolocationPositionCache should do database access on background
thread
https://bugs.webkit.org/show_bug.cgi?id=50825

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

------- Additional Comments from Jeremy Orlow <jorlow at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=76878&action=review

> WebCore/page/GeolocationPositionCache.cpp:38
>  

no newline

> WebCore/page/GeolocationPositionCache.cpp:64
> +    if (!(numUsers++) && !m_threadId) {

This one line is doing a lot.  Maybe the ++numUsers should stay outside of the
if statement?

> WebCore/page/GeolocationPositionCache.cpp:75
>      if (!(--numUsers) && m_cachedPosition)

ditto

> WebCore/page/GeolocationPositionCache.cpp:106
> +    m_threadId = createThread(threadEntryPoint, this, "WebCore:
GeolocationPositionCache");

Is there any other thread we can share?  Or will this be used for anything
else?

> WebCore/page/GeolocationPositionCache.cpp:118
> +	   task->performTask(0 /* ScriptExecutionContext */);

I don't think we typically use /* style comments.  Maybe instead of saying it's
the SEC (which anyone can look up) you should explain why it's fine to pass
0...i.e. it's unused on the other side.  Or just delete.

> WebCore/page/GeolocationPositionCache.cpp:133
> +    MutexLocker lock(m_cachedPositionMutex);

Everything in this can block the main thread.

The next line is just an optimization anyway (if you do a final check before
setting m_cachedPostion below).  In non-testing, things will only be
created/set in this thread.  So I don't think you need the lock until my
comment below.

> WebCore/page/GeolocationPositionCache.cpp:173
>      m_cachedPosition = Geoposition::create(coordinates.release(),
statement.getColumnInt64(7)); // timestamp

Here you should take the lock...and do an if statement to verify it hasn't been
set.

> WebCore/page/GeolocationPositionCache.cpp:188
> +    MutexLocker lock(m_cachedPositionMutex);

Only lock to get a snapshot.  Don't hold it.  Otherwise, there's no point...