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

[Kangil Han]

Hi Chris,
 
> With the alternative to pimpl, there is one annoying issue as I said. The
public C functions can no longer access the class members directly and they
have to call the object methods instead.
AFAIK, This is what we call encapsulation in OOP.
 
> You can already see it in my patch for the destination() getter. It has to
return a "const char*" instead of a String because it is used by the C API.
IMHO, external object(s) should treat Ewk classes as C++ as much as possible
in this case.
Therefore, we don't have to make functions that return char*.
Rather, caller can use utilization function to change String class to char*.
 
--- Ewk class
String destination() { . }
--- Utilization function
char* StoCP(String s)
{
    char* cp = s.utf8().data();
    return cp;
}
--- C layer
void i_am_c_public_api()
{
    char* cp = StoCP(destination);
}
 
As you mentioned, if performance drawback is not so considerable, why don't
we use OOP majority here?
Reducing complexity pay much especially on sizable project like WebKit. :)
 
Kind regards,
Kangil
 
From: Dumez, Christophe [mailto:christophe.dumez at intel.com] 
Sent: Friday, October 19, 2012 7:11 PM
To: kangil.han at samsung.com
Cc: Thiago Marcos P. Santos; webkit-efl at lists.webkit.org; Kenneth R
Christiansen; cpgs at samsung.com
Subject: Re: FW: [webkit-efl] Use of pimpl idiom for Ewk classes
 
Hi,

With the alternative to pimpl, there is one annoying issue as I said. The
public C functions can no longer access the class members directly and they
have to call the object methods instead.
Not only this forces us to decide one C++ method for each public C function,
but also this means that we inherit some C requirements on the C++ methods
(for example due to string sharing).

You can already see it in my patch for the destination() getter. It has to
return a "const char*" instead of a String because it is used by the C API.
We have a WKEinaSharedString m_destination member and we simply return it as
a const char* so that the browser can use it and benefit from string
sharing. The annoying part is that the C++ method is used from private C++
implementation as well and the result is:
String destination = String("file://") +
String::fromUTF8(download->destination());

instead of

String destination = String("file://") + download->destination();

It is bug prone because the C++ called needs to use String::fromUTF8().
Implicit conversion to a String type would treat the string as ASCII, which
would be wrong here.

We cannot return a String with this approach because then the code would
look like:

const char* ewk_download_job_destination_get(const Ewk_Download_Job*
download)
{
   String destination = download->destination(); // returns a String
   return eina_stringshare_add(destination.utf8().data()); // Sadly we copy
the char array and the client has to call eina_stringshare_del() explicitly
after use.
}

With pimpl, the public C function code looks like:
const char* ewk_download_job_destination_get(const Ewk_Download_Job*
download)
{
   EWK_D(Ewk_Download_Job, download);
   return d->destination; // The client does not have to call
eina_stringshare_del() explicitely.
}

and we can have a C++ getter for private code that returns a C++ type
(String):
String Ewk_Download_Job::destination() const
{
  return String::fromUTF8(d->destination);
}

One solution would be to have 2 C++ methods:
const char* destination() const; // For public C API use
String destinationString() const; // For private C++ code
but it starts to look ugly and duplicates code.

The pimpl approach:
- Allows for more flexibility because the C API can access class members
directly and therefore we don't have to design the C++ methods to fit their
need.
- We add C++ methods only to replace the existing private C functions so it
leads to less code.
- Performance-wise: it don't think there is a big difference but as Kenneth
said, it is likely that pointer dereferencing (as in pimpl) is faster than
calling a method on the object
- Shorter compilation time: the private headers contain less information
with Pimpl and therefore it leads to faster compile time

The drawbacks I see with Pimpl:
- With have a POD struct in the cpp file in addition to the C++ class in the
private header
- We need to use a couple of macros to make our life easier (Same for Qt BTW
and they are living fine with it)
- The pimpl pattern is slightly more complex (but it is popular / famous)

Kr,
--
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/20121019/e3b9d299/attachment-0001.html>