Bug 137993 – Abstractify WebOriginDataManager to support arbitrary ChildProcess supplements.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

Abstractify WebOriginDataManager to support arbitrary ChildProcess supplements.

[Jer Noble]

Created attachment 240315 [details]
Patch

[Jer Noble]

Abstractify? Genericize.

[Brady Eidson]

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!

[Jer Noble]

Created attachment 240437 [details]
Patch

[Brady Eidson]

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

[Jer Noble]

(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

[Jer Noble]

Created attachment 240439 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 240439 [details]
Patch for landing

Clearing flags on attachment: 240439

Committed r175188: <http://trac.webkit.org/changeset/175188>

[Radar WebKit Bug Importer]

<rdar://problem/18819561>

[Ahmad Saleem]

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!