Bug 99697 – [EFL][WK2] Make Ewk_Download_Job members private and remove private C functions

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Chris Dumez]

As part of Bug 99696, this bug is for using pimpl idiom for Ewk_Download_Job class and serve as example for the proposed approach.

[Chris Dumez]

Created attachment 169367 [details]
Patch

I'll send a mail to the webkit-efl mailing list to announce / explain the proposal but I think it is nice to have a patch to show how the result looks like.

Thanks to Mikhail for brainstorming with me on this proposal.

[Mikhail Pozdnyakov]

Comment on attachment 169367 [details]
Patch

Look how nice it is now! informal r+ from me off course :)

[Kenneth Rohde Christiansen]

Comment on attachment 169367 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169367&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_download_job.cpp:47
> +    uint64_t downloaded; /**< length already downloaded */

why not just use C++ comments

> Source/WebKit2/UIProcess/API/efl/ewk_private.h:45
> +// Macros used for Pimpl idiom.
> +#define EWK_DECLARE_DATA(ClassName) struct Data; OwnPtr<Data> d
> +#define EWK_D(ClassName, obj) ClassName::Data* d = obj->d.get()

Qt allows to get the owner as well, (d upside down, ie. q) :-)

[Chris Dumez]

(In reply to comment #3)
> (From update of attachment 169367 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169367&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_download_job.cpp:47
> > +    uint64_t downloaded; /**< length already downloaded */
> 
> why not just use C++ comments

I'll fix this, thanks.

> > Source/WebKit2/UIProcess/API/efl/ewk_private.h:45
> > +// Macros used for Pimpl idiom.
> > +#define EWK_DECLARE_DATA(ClassName) struct Data; OwnPtr<Data> d
> > +#define EWK_D(ClassName, obj) ClassName::Data* d = obj->d.get()
> 
> Qt allows to get the owner as well, (d upside down, ie. q) :-)

We don't really need to get the owner in our case. Unlike with Qt, our private implementation is a simple POD struct so it does not need to get its owner.

[Chris Dumez]

Created attachment 169377 [details]
Patch

Fix remaining C-style comment. Not setting cq flag yet as I'm waiting for a bit more feedback on the mailing list.

[Chris Dumez]

Created attachment 169380 [details]
Patch for landing

Use WTF_MAKE_NONCOPYABLE macro for Ewk_Download_Job to make sure no one ever tries to copy it.

[Chris Dumez]

Created attachment 169395 [details]
Patch

Mikhail and I made another iteration of the patch so we are requesting r? again.

The improvement is that the pimpl data pointer is now private (it used to be public). The drawback is that we need an additional EWK_DECLARE_DATA_OWNER() macro for the Data type.

[Chris Dumez]

Created attachment 169402 [details]
Patch

use
const OwnPtr<struct Download_Job_Data> d;
instead of
OwnPtr<struct Download_Job_Data> d;

just to be safe.

[Chris Dumez]

Created attachment 169425 [details]
Pimpl alternative

[Mikhail Pozdnyakov]

Comment on attachment 169425 [details]
Pimpl alternative

View in context: https://bugs.webkit.org/attachment.cgi?id=169425&action=review

I've been doing the same patch :) this approach looks more attractive to me

> Source/WebKit2/UIProcess/API/efl/ewk_download_job_private.h:51
> +    uint64_t id() const;

I would put the body of simple getters right here in header file

> Source/WebKit2/UIProcess/API/efl/ewk_download_job_private.h:58
> +    Ewk_Url_Response* response() const;

the two methods above are not const I guess

[Chris Dumez]

Comment on attachment 169425 [details]
Pimpl alternative

View in context: https://bugs.webkit.org/attachment.cgi?id=169425&action=review

>> Source/WebKit2/UIProcess/API/efl/ewk_download_job_private.h:51
>> +    uint64_t id() const;
> 
> I would put the body of simple getters right here in header file

I guess it is a matter of taste. I'm trying to keep the headers as small as possible to reduce compile time.

>> Source/WebKit2/UIProcess/API/efl/ewk_download_job_private.h:58
>> +    Ewk_Url_Response* response() const;
> 
> the two methods above are not const I guess

Well, I did that at first but then you have to const_cast in the C functions :(

[Chris Dumez]

Created attachment 169436 [details]
Pimpl alternative

Add changelog.

[Kenneth Rohde Christiansen]

Comment on attachment 169425 [details]
Pimpl alternative

View in context: https://bugs.webkit.org/attachment.cgi?id=169425&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_context_download_client.cpp:63
> +    String destination = String("file://") + String::fromUTF8(download->destination());

ASCIIString pls

[Kenneth Rohde Christiansen]

Comment on attachment 169402 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169402&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_private.h:44
> +#define EWK_DECLARE_DATA(ClassName) const OwnPtr<struct ClassName##_Data> d; friend struct ClassName##_Data

Why _Data now instead of ::Data? pro/cons? Also isn't Private a more descriptive name? ie. it makes it clear how it should be used.

[Mikhail Pozdnyakov]

(In reply to comment #14)
> (From update of attachment 169402 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169402&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_private.h:44
> > +#define EWK_DECLARE_DATA(ClassName) const OwnPtr<struct ClassName##_Data> d; friend struct ClassName##_Data
> 
> Why _Data now instead of ::Data? pro/cons? 

Private nested type is inaccessible from C API functions, that's why we had to make it like this.. 
In Qt by the way they also do not use nested types.

[Mikhail Pozdnyakov]

(In reply to comment #15)
> (In reply to comment #14)
> > (From update of attachment 169402 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=169402&action=review
> > 
> > > Source/WebKit2/UIProcess/API/efl/ewk_private.h:44
> > > +#define EWK_DECLARE_DATA(ClassName) const OwnPtr<struct ClassName##_Data> d; friend struct ClassName##_Data
> > 
> > Why _Data now instead of ::Data? pro/cons? 
> 
> Private nested type is inaccessible from C API functions, that's why we had to make it like this.. 
> In Qt by the way they also do not use nested types.

BTW, do you prefer pimpl approach? :-)

[Chris Dumez]

Created attachment 169588 [details]
Pimpl

Take Kenneth feedback into consideration for Pimpl patch:
- Rename private struct to _Private instead of _Data
- Rename macros to EWK_DECLARE_PRIVATE() / EWK_DECLARE_PUBLIC()
- Use AsciiLiteral()

[Chris Dumez]

Created attachment 169590 [details]
Pimpl alternative

Take Kenneth feedback into consideration for alternative to Pimpl:
- Use AsciiLiteral()

[Chris Dumez]

Comment on attachment 169588 [details]
Pimpl

Obsoleting the pimpl patch based on the mailing list discussion.

[WebKit Review Bot]

Comment on attachment 169590 [details]
Pimpl alternative

Clearing flags on attachment: 169590

Committed r132000: <http://trac.webkit.org/changeset/132000>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.