Abstractify WebOriginDataManager to support arbitrary ChildProcess supplements.
Created attachment 240315 [details] Patch
Abstractify? Genericize.
Comment on attachment 240315 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=240315&action=review > Source/WebKit2/ChangeLog:48 > + to fall under the heel of the new regieme. DatabaseProcess will subclass spelling - regime > Source/WebKit2/Shared/AsyncTask.h:40 > + AsyncTask(const std::function<void()> taskFunction) > + : m_taskFunction(taskFunction) > + { > + } ASSERT m_taskFunction plz > Source/WebKit2/Shared/AsyncTask.h:45 > + if (m_taskFunction) > + m_taskFunction(); No null check, but ASSERT > Source/WebKit2/WebProcess/OriginData/WebOriginDataManager.cpp:57 > + ChildProcess* childProcess = m_childProcess; Can be a ref instead of a pointer (see below) > Source/WebKit2/WebProcess/OriginData/WebOriginDataManager.cpp:65 > + ChildProcess* childProcess = m_childProcess; Can be a ref instead of a pointer (see below) > Source/WebKit2/WebProcess/OriginData/WebOriginDataManager.cpp:73 > + ChildProcess* childProcess = m_childProcess; Can be a ref instead of a pointer (see below) > Source/WebKit2/WebProcess/OriginData/WebOriginDataManager.cpp:81 > + ChildProcess* childProcess = m_childProcess; Can be a ref instead of a pointer (see below) > Source/WebKit2/WebProcess/OriginData/WebOriginDataManager.h:44 > + WebOriginDataManager(ChildProcess*, WebOriginDataManagerSupplement*); Good time to make these references instead of pointers. > Source/WebKit2/WebProcess/OriginData/WebOriginDataManager.h:56 > ChildProcess* m_childProcess; > + WebOriginDataManagerSupplement* m_supplement; References instead of pointers. > Source/WebKit2/WebProcess/OriginData/WebOriginDataManagerSupplement.h:46 > + virtual void getOrigins(WKOriginDataTypes, std::function<void(const Vector<SecurityOriginData>&)> completion) > + { > + completion(Vector<SecurityOriginData>()); > + } > + virtual void deleteEntriesForOrigin(WKOriginDataTypes, const SecurityOriginData&, std::function<void()> completion) { completion(); } > + virtual void deleteEntriesModifiedBetweenDates(WKOriginDataTypes, double startDate, double endDate, std::function<void()> completion) { completion(); } > + virtual void deleteAllEntries(WKOriginDataTypes, std::function<void()> completion) { completion(); } Pure virtuals please!
Created attachment 240437 [details] Patch
Comment on attachment 240437 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=240437&action=review > Source/WebKit2/DatabaseProcess/DatabaseProcess.cpp:57 > + , m_webOriginDataManager(std::make_unique<WebOriginDataManager>(*this, *this)) This is silly. I wonder if childprocess will always ever be the only supplement, in which case this is redundant. Oh well. > Source/WebKit2/WebProcess/OriginData/WebOriginDataManager.cpp:59 > + ChildProcess& childProcess = m_childProcess; > + m_supplement.getOrigins(types, [&childProcess, callbackID](const Vector<SecurityOriginData>& results) { > + childProcess.send(Messages::WebOriginDataManagerProxy::DidGetOrigins(results, callbackID), 0); > + }); There's actually no need for the temporary ChildProcess if you just capture `this` in the lamba instead, and then access m_childProcess in the function body. There's no lifetime concern for either object anyways. > Source/WebKit2/WebProcess/OriginData/WebOriginDataManager.cpp:67 > + ChildProcess& childProcess = m_childProcess; > + m_supplement.deleteEntriesForOrigin(types, originData, [&childProcess, callbackID] { > + childProcess.send(Messages::WebOriginDataManagerProxy::DidDeleteEntries(callbackID), 0); > + }); Ditto > Source/WebKit2/WebProcess/OriginData/WebOriginDataManager.cpp:75 > + ChildProcess& childProcess = m_childProcess; > + m_supplement.deleteEntriesModifiedBetweenDates(types, startTime, endTime, [&childProcess, callbackID] { > + childProcess.send(Messages::WebOriginDataManagerProxy::DidDeleteEntries(callbackID), 0); > + }); Ditto > Source/WebKit2/WebProcess/OriginData/WebOriginDataManager.cpp:83 > + ChildProcess& childProcess = m_childProcess; > + m_supplement.deleteAllEntries(types, [&childProcess, callbackID] { > + childProcess.send(Messages::WebOriginDataManagerProxy::DidDeleteAllEntries(callbackID), 0); > + }); Ditto
(In reply to comment #5) > Comment on attachment 240437 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=240437&action=review > > > Source/WebKit2/DatabaseProcess/DatabaseProcess.cpp:57 > > + , m_webOriginDataManager(std::make_unique<WebOriginDataManager>(*this, *this)) > > This is silly. I wonder if childprocess will always ever be the only > supplement, in which case this is redundant. Oh well. Maybe? But this way, a ChildProcess can choose to be a WebOriginDataManager supplement, rather than it being built into ChildProcess. > > Source/WebKit2/WebProcess/OriginData/WebOriginDataManager.cpp:59 > > + ChildProcess& childProcess = m_childProcess; > > + m_supplement.getOrigins(types, [&childProcess, callbackID](const Vector<SecurityOriginData>& results) { > > + childProcess.send(Messages::WebOriginDataManagerProxy::DidGetOrigins(results, callbackID), 0); > > + }); > > There's actually no need for the temporary ChildProcess if you just capture > `this` in the lamba instead, and then access m_childProcess in the function > body. > > There's no lifetime concern for either object anyways. > > > Source/WebKit2/WebProcess/OriginData/WebOriginDataManager.cpp:67 > > + ChildProcess& childProcess = m_childProcess; > > + m_supplement.deleteEntriesForOrigin(types, originData, [&childProcess, callbackID] { > > + childProcess.send(Messages::WebOriginDataManagerProxy::DidDeleteEntries(callbackID), 0); > > + }); > > Ditto > > > Source/WebKit2/WebProcess/OriginData/WebOriginDataManager.cpp:75 > > + ChildProcess& childProcess = m_childProcess; > > + m_supplement.deleteEntriesModifiedBetweenDates(types, startTime, endTime, [&childProcess, callbackID] { > > + childProcess.send(Messages::WebOriginDataManagerProxy::DidDeleteEntries(callbackID), 0); > > + }); > > Ditto > > > Source/WebKit2/WebProcess/OriginData/WebOriginDataManager.cpp:83 > > + ChildProcess& childProcess = m_childProcess; > > + m_supplement.deleteAllEntries(types, [&childProcess, callbackID] { > > + childProcess.send(Messages::WebOriginDataManagerProxy::DidDeleteAllEntries(callbackID), 0); > > + }); > > Ditto
Created attachment 240439 [details] Patch for landing
Comment on attachment 240439 [details] Patch for landing Clearing flags on attachment: 240439 Committed r175188: <http://trac.webkit.org/changeset/175188>
<rdar://problem/18819561>
From Comment 08, I can see this landing and I confirmed from Github that it was not backed out (checking with Bug Number). Commit - https://github.com/WebKit/WebKit/commit/b71994375f44e9c5dc18885ecad9bdce0ba8e368 I am going to mark this as "RESOLVED FIXED". Thanks!