...
Created attachment 428289 [details] Patch
<rdar://77722187>
Comment on attachment 428289 [details] 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.
Comment on attachment 428289 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=428289&action=review Thanks for the review! >> Source/WebCore/ChangeLog:18 >> + making the map in IDBSerializationContext keye by thread pointer. > > Typo: keye (I guess you meant keyed?) Yes. >> 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. Will add. >> 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. Sure. >> Source/WebCore/Modules/indexeddb/server/IDBSerializationContext.h:51 >> + IDBSerializationContext(Thread&); > > explicit Will add. >> Tools/TestWebKitAPI/Tests/WebKitCocoa/IndexedDBDatabaseProcessKill.mm:45 >> +static RetainPtr<WKScriptMessage> lastScriptMessage; > > Maybe this could be a RetainPtr<NSString> instead. Sure >> 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. Sure, will change.
Created attachment 428323 [details] Patch for landing
Committed r277343 (237601@main): <https://commits.webkit.org/237601@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 428323 [details].