[webkit-efl] Use of pimpl idiom for Ewk classes

[Dumez, Christophe]

Hi Thiago,

what you are proposing (make the members private and add getters / setters
for them) is the first thing I have tried.
However, this results in a lot of more changes.

1. With the pimpl way, we can simply move the members to the cpp file in a
POD type (Ok, with some macros). C API implementation remains mostly
unchanged.

2. With the alternative approach, we will need a lot of new methods in the
ewk classes to match the functionality provided by the public EFL/C API.
And our cpp file is going to contain C functions and C++ methods
implementations for the same functionality (the C functions calling the
corresponding C++ methods).

Mikhail said he would give the alternative approach a try for Ewk_Download
so that we can compare the results. Hopefully, we can better see the
difference then.

Performance-wise, I don't see much difference. With pimpl, we have an extra
pointer dereferencing in the C functions and with the alternative, we have
an extra function call.

BTW, Thiago, in your code, the example you are showing is for class methods
but you need to take into consideration the C functions as well to have a
full picture.
Also, to be a fair comparison, your code example for the C++ method should
have been:

uint64_t Ewk_Download_Job::id() const
{
    // return m_downloadProxyId;
    return m_downloadProxy->downloadID();
}

Instead of:

uint64_t Ewk_Download_Job::id() const
{
    // return d->downloadProxyId;
    return d->downloadProxy->downloadID();
}

(You created a new m_downloadProxyId member for one code sample but not the
other ;)

Kr,

On Thu, Oct 18, 2012 at 6:08 PM, Thiago Marcos P. Santos <
tmpsantos at gmail.com> wrote:

> On Thu, Oct 18, 2012 at 11:56 AM, Dumez, Christophe
> <christophe.dumez at intel.com> wrote:
> > Hi,
> >
> > Mikhail and I are currently working on refactoring the Ewk classes to
> make
> > them more C++-like, more convenience to use and less bug-prone.
> > As part of this work, we came out with a proposal: Use pimpl idom for Ewk
> > classes.
> >
> > The Ewk classes are currently defined in the private headers, with all
> their
> > members public. The private headers also usually contain C-style
> functions
> > to interact with the Ewk class from inside WebKit.
> > Now, we would like to get rid of those private C functions and replace
> them
> > with C++ methods in the Ewk class. The issue is that the Ewk class
> members
> > are currently public meaning we can alter the members directly from
> outside
> > the Ewk class implementation and bypass those class methods.
> >
> > At the same time, we don't want to make all the Ewk class members private
> > and create getter / setter methods for all of them. Not only this means a
> > lot of refactoring work and extra code, but also, this means that are
> public
> > C API implementation will need to go through those getters / setters as
> > well, instead of interacting with the class members directly.
> >
> > We therefore thought of using the pimpl idiom so that the class members
> are
> > now stored in a POD struct in the cpp file. This effectively prevents
> access
> > to them from code other than our public C API and Ewk class methods,
> which
> > is what we want. The C API functions can still directly interact with the
> > Ewk class members which is convenient. We can get rid of all C functions
> in
> > the private headers and replace them with Ewk class methods. Therefore,
> the
> > coding style in WK2 EFL port implementation with be more C++ oriented.
> >
> > I created a meta bug for this task:
> > https://bugs.webkit.org/show_bug.cgi?id=99696
> >
> > I also wrote a first patch to show what the result looks like for the
> > Ewk_Download_Job class. The patch also adds convenience macros to
> > ewk_private.h to make pimpl idiom usage simpler and less error-prone:
> > https://bugs.webkit.org/show_bug.cgi?id=99697
> >
> > We hope you like the result as much as we do.
> > If everyone agrees, Mikhail and I are planning to port other Ewk classes
> to
> > this proposal and update the WebKit EFL coding style.
> >
>
> Is it really necessary to make this a Qt-like pimpl?
>
> AFAIK Qt has this opaque d-pointer to keep ABI compatibility across
> releases. Since we are working on private code, I don't see need for
> this. Will just complexity to the code and extra pointers
> dereferences.
>
> Don't get me wrong, the refactoring is indeed necessary (but can be
> done differently). What I'm questioning here is the need of these
> macros and the "d->private" idiom. The change bellow is fine, removed
> C-ism from the code:
>
> -     uint64_t downloadId = ewk_download_job_id_get(ewkDownload);
> +    uint64_t downloadId = ewkDownload->id();
>
> But I would write Ewk_Download_Job::id() like this:
>
> uint64_t Ewk_Download_Job::id() const
> {
>     return m_downloadProxyId;
> }
>
> Instead of:
>
> uint64_t Ewk_Download_Job::id() const
> {
>     return d->downloadProxy->downloadID();
> }
>
> Why not instead of creating a Ewk_Download_Job_Data you store the
> information you need inside Ewk_Download_Job and mark as private
> attributes?
>
> Cheers,
>



-- 
Christophe Dumez
Linux Software Engineer, PhD
Intel Finland Oy - Open Source Technology Center
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-efl/attachments/20121018/93e0e0ef/attachment-0001.html>