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

[Dumez, Christophe]

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/b6865b02/attachment.html>