Bug 137993 - Abstractify WebOriginDataManager to support arbitrary ChildProcess supplements.
Summary: Abstractify WebOriginDataManager to support arbitrary ChildProcess supplements.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords: InRadar
Depends on:
Blocks: 137120
  Show dependency treegraph
 
Reported: 2014-10-22 17:47 PDT by Jer Noble
Modified: 2022-08-08 16:07 PDT (History)
5 users (show)

See Also:


Attachments
Patch (28.03 KB, patch)
2014-10-22 18:05 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (28.09 KB, patch)
2014-10-24 14:32 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch for landing (27.86 KB, patch)
2014-10-24 14:54 PDT, Jer Noble
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2014-10-22 17:47:22 PDT
Abstractify WebOriginDataManager to support arbitrary ChildProcess supplements.
Comment 1 Jer Noble 2014-10-22 18:05:57 PDT
Created attachment 240315 [details]
Patch
Comment 2 Jer Noble 2014-10-22 18:07:06 PDT
Abstractify? Genericize.
Comment 3 Brady Eidson 2014-10-24 11:12:29 PDT
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!
Comment 4 Jer Noble 2014-10-24 14:32:21 PDT
Created attachment 240437 [details]
Patch
Comment 5 Brady Eidson 2014-10-24 14:47:31 PDT
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
Comment 6 Jer Noble 2014-10-24 14:51:27 PDT
(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
Comment 7 Jer Noble 2014-10-24 14:54:44 PDT
Created attachment 240439 [details]
Patch for landing
Comment 8 WebKit Commit Bot 2014-10-24 16:01:53 PDT
Comment on attachment 240439 [details]
Patch for landing

Clearing flags on attachment: 240439

Committed r175188: <http://trac.webkit.org/changeset/175188>
Comment 9 Radar WebKit Bug Importer 2014-10-29 17:48:05 PDT
<rdar://problem/18819561>
Comment 10 Ahmad Saleem 2022-08-08 16:07:22 PDT
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!