[webkit-reviews] review granted: [Bug 225658] Use one VM per thread for IDB serialization work : [Attachment 428289] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has granted Sihui Liu <sihui_liu at apple.com>'s
request for review:
Bug 225658: Use one VM per thread for IDB serialization work
https://bugs.webkit.org/show_bug.cgi?id=225658

Attachment 428289: Patch

https://bugs.webkit.org/attachment.cgi?id=428289&action=review




--- Comment #3 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 428289
  --> https://bugs.webkit.org/attachment.cgi?id=428289
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=428289&action=review

r=me with suggestions.

> Source/WebCore/ChangeLog:11
> +	   does not stay around, and WebIDBServer will be destroyed after it
finishes schedueld tasks on the background 

Typo: schedueld

> Source/WebCore/ChangeLog:12
> +	   thread.Then, it's possible that while a WebIDBServer for some
session is removed and finishing last tasks, 

Typo: .Then

> Source/WebCore/ChangeLog:18
> +	   making the map in IDBSerializationContext keye by thread pointer.

Typo: keye (I guess you meant keyed?)

> Source/WebCore/Modules/indexeddb/server/IDBSerializationContext.cpp:39
> +static HashMap<Thread*, IDBSerializationContext*>& serializationContextMap()

It would look safer if this function took a Locker<Lock>& as parameter, since
the caller needs to grab the lock.

> Source/WebCore/Modules/indexeddb/server/IDBSerializationContext.h:43
> +    static Ref<IDBSerializationContext>
getOrCreateIDBSerializationContext();

Maybe we could rename this to getOrCreateForCurrentThread() for clarity, given
the new behavior? (note that I don't think we need to repeat
"IDBSerializationContext" in the function name since it is already in the
IDBSerializationContext class scope.

> Source/WebCore/Modules/indexeddb/server/IDBSerializationContext.h:51
> +    IDBSerializationContext(Thread&);

explicit

> Tools/TestWebKitAPI/Tests/WebKitCocoa/IndexedDBDatabaseProcessKill.mm:45
> +static RetainPtr<WKScriptMessage> lastScriptMessage;

Maybe this could be a RetainPtr<NSString> instead.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/IndexedDBDatabaseProcessKill.mm:76
> +    lastScriptMessage = message;

Maybe we could store [message body] here. Doesn't seem we really want the whole
message.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/IndexedDBDatabaseProcessKill.mm:112
> +    RetainPtr<DatabaseProcessKillMessageHandler> handler =
adoptNS([[DatabaseProcessKillMessageHandler alloc] init]);

auto

> Tools/TestWebKitAPI/Tests/WebKitCocoa/IndexedDBDatabaseProcessKill.mm:113
> +    RetainPtr<WKWebViewConfiguration> configuration =
adoptNS([[WKWebViewConfiguration alloc] init]);

auto

> Tools/TestWebKitAPI/Tests/WebKitCocoa/IndexedDBDatabaseProcessKill.mm:116
> +    auto ephemeralStore = [WKWebsiteDataStore nonPersistentDataStore];

This local variable is not needed.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/IndexedDBDatabaseProcessKill.mm:125
> +	       var request = window.indexedDB.open('testDB'); \

can omit "window."

> Tools/TestWebKitAPI/Tests/WebKitCocoa/IndexedDBDatabaseProcessKill.mm:129
> +		   for (let i = 0; i < 10000; i ++) \

extra space between i and ++

> Tools/TestWebKitAPI/Tests/WebKitCocoa/IndexedDBDatabaseProcessKill.mm:131
> +		  
window.webkit.messageHandlers.testHandler.postMessage('Opened');\

can omit "window."

> Tools/TestWebKitAPI/Tests/WebKitCocoa/IndexedDBDatabaseProcessKill.mm:140
> +    [webView evaluateJavaScript:@"openDatabase()" completionHandler: nil];

no space after completionHandler:

> Tools/TestWebKitAPI/Tests/WebKitCocoa/IndexedDBDatabaseProcessKill.mm:142
> +    RetainPtr<NSString> string = (NSString *)[lastScriptMessage body];

Local variable would not be needed with my suggestion above.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/IndexedDBDatabaseProcessKill.mm:147
> +    [secondWebView evaluateJavaScript:@"openDatabase()" completionHandler:
nil];

no space after completionHandler:

> Tools/TestWebKitAPI/Tests/WebKitCocoa/IndexedDBDatabaseProcessKill.mm:148
> +    receivedScriptMessage = false;

It would look safer if we did this before the evaluateJavaScript call (although
it is safe because async).

> Tools/TestWebKitAPI/Tests/WebKitCocoa/IndexedDBDatabaseProcessKill.mm:150
> +    string = (NSString *)[lastScriptMessage body];

Local variable would not be needed with my suggestion above.