[webkit-reviews] review denied: [Bug 40655] Database access in worker threads results in WebKit SPI notifications being posted from worker threads : [Attachment 59356] patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Dumitru Daniliuc
<dumi at chromium.org>'s request for review:
Bug 40655: Database access in worker threads results in WebKit SPI
notifications being posted from worker threads
https://bugs.webkit.org/show_bug.cgi?id=40655

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

------- Additional Comments from David Levin <levin at chromium.org>
I wonder how the concept of isMainThread maps to the work being done in WebKit2
with respect to multiple threads. If someone chimes in on that, maybe it can be
addressed (but I'm guessing that it may need a more systematic pass later).


> Index: WebKit/mac/Storage/WebDatabaseTrackerClient.mm
> +struct OriginModifiedInfo {

I can't parse this name (but my attempt was comical).
...
Now that I read lower, I get it. I personally like DidModifyOriginData better.
What do you think? (I chose Data instead of Info to avoid an abbreviation per
WebKit style while keeping it short.)

> +    OriginModifiedInfo(WebDatabaseTrackerClient* client, SecurityOrigin*
origin)
> +	   : client(client), origin(origin) { }

Ideally on separate lines.


> +    WebDatabaseTrackerClient* client;

Is WebDatabaseTrackerClient threadsafe? (Not many things in WebKit are.)

> +    SecurityOrigin* origin;

Definitely not threadsafe.

> +};
> +
> +static void dispatchDidModifyOriginOnMainThread(void* context)
> +{
> +    OriginModifiedInfo* info = static_cast<OriginModifiedInfo*>(context);
> +    info->client->dispatchDidModifyOrigin(info->origin);
> +    delete info;
> +}
> +
>  void WebDatabaseTrackerClient::dispatchDidModifyOrigin(SecurityOrigin*
origin)
>  {
> -	RetainPtr<WebSecurityOrigin> webSecurityOrigin(AdoptNS,
[[WebSecurityOrigin alloc] _initWithWebCoreSecurityOrigin:origin]);
> +    if (!isMainThread()) {
> +	   OriginModifiedInfo* context = new OriginModifiedInfo(this, origin);

Use origin->threadsafeCopy(); to get something appropriate for another thread
(and change the member variable to be a RefPtr).


> +	   callOnMainThread(dispatchDidModifyOriginOnMainThread,
static_cast<void*>(context));

I would just make "dispatchDidModifyOriginOnMainThread" a static method on the
class above and then make the variables private.

Taking it a step further you could just have the class have just one exposed
static method: DidModifyOriginData::dispatchToMainThread which does the new and
then the callOnMainThread. (It also makes it clear who should do the copies --
the class itself since it is doing the call to the main thread.)

> +	   return;
> +    }
> +
> +    RetainPtr<WebSecurityOrigin> webSecurityOrigin(AdoptNS,
[[WebSecurityOrigin alloc] _initWithWebCoreSecurityOrigin:origin]);
>  
>      [[NSNotificationCenter defaultCenter]
postNotificationName:WebDatabaseDidModifyOriginNotification 
>							  
object:webSecurityOrigin.get()];
>  }
>  
> +struct DatabaseModifiedInfo {
> +    DatabaseModifiedInfo(WebDatabaseTrackerClient* client, SecurityOrigin*
origin, const String& databaseName)
> +	   : client(client), origin(origin),
databaseIdentifier(databaseIdentifier) { }

Ideally separate lines.

> +    WebDatabaseTrackerClient* client;
> +    SecurityOrigin* origin;

Same as before.

> +    String databaseIdentifier;

Use databaseIdentifier->crossThreadString(); (I'm sorry about the name. It
doesn't provide a string that can be used on multiple threads at the same time.
It does provide a string to be given to another thread. Maybe otherThreadString
would be a better name?)


>  void WebDatabaseTrackerClient::dispatchDidModifyDatabase(SecurityOrigin*
origin, const String& databaseIdentifier)
>  {
> +    if (!isMainThread()) {
> +	   DatabaseModifiedInfo* context = new DatabaseModifiedInfo(this,
origin, databaseIdentifier);

Same comments as before.


> Index: WebKit/win/WebDatabaseManager.cpp


It's déjà vu all over again. Is there anyway to share code here?