[Webkit-unassigned] [Bug 50825] GeolocationPositionCache should do database access on background thread

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=50825


Jeremy Orlow <jorlow at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #76878|review?                     |review-
               Flag|                            |




--- Comment #2 from Jeremy Orlow <jorlow at chromium.org>  2010-12-20 04:25:09 PST ---
(From update of attachment 76878)
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...

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.