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

RESOLVED FIXED 99697

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

https://bugs.webkit.org/show_bug.cgi?id=99697

Summary [EFL][WK2] Make Ewk_Download_Job members private and remove private C functions

Chris Dumez

Reported 2012-10-18 01:29:46 PDT

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.

Attachments
Patch (21.63 KB, patch) 2012-10-18 01:42 PDT, Chris Dumez	kenneth: review+	Details Formatted Diff Diff
Patch (21.64 KB, patch) 2012-10-18 02:49 PDT, Chris Dumez	no flags	Details Formatted Diff Diff
Patch for landing (21.68 KB, patch) 2012-10-18 03:10 PDT, Chris Dumez	no flags	Details Formatted Diff Diff
Patch (22.01 KB, patch) 2012-10-18 05:15 PDT, Chris Dumez	no flags	Details Formatted Diff Diff
Patch (22.01 KB, patch) 2012-10-18 06:25 PDT, Chris Dumez	no flags	Details Formatted Diff Diff
Pimpl alternative (16.31 KB, patch) 2012-10-18 09:19 PDT, Chris Dumez	no flags	Details Formatted Diff Diff
Pimpl alternative (19.58 KB, patch) 2012-10-18 10:50 PDT, Chris Dumez	no flags	Details Formatted Diff Diff
Pimpl (22.10 KB, patch) 2012-10-19 03:32 PDT, Chris Dumez	no flags	Details Formatted Diff Diff
Pimpl alternative (19.59 KB, patch) 2012-10-19 03:36 PDT, Chris Dumez	no flags	Details Formatted Diff Diff
Show Obsolete (8) View All Add attachment proposed patch, testcase, etc.

Chris Dumez

Comment 1 2012-10-18 01:42:24 PDT

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 2 2012-10-18 01:46:48 PDT

Comment on attachment 169367 [details] Patch Look how nice it is now! informal r+ from me off course :)

Kenneth Rohde Christiansen

Comment 3 2012-10-18 02:30:09 PDT

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

Comment 4 2012-10-18 02:48:09 PDT

(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

Comment 5 2012-10-18 02:49:26 PDT

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

Comment 6 2012-10-18 03:10:05 PDT

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

Comment 7 2012-10-18 05:15:18 PDT

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

Comment 8 2012-10-18 06:25:47 PDT

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

Comment 9 2012-10-18 09:19:36 PDT

Created attachment 169425 [details] Pimpl alternative

Mikhail Pozdnyakov

Comment 10 2012-10-18 09:51:05 PDT

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 11 2012-10-18 09:55:09 PDT

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

Comment 12 2012-10-18 10:50:50 PDT

Created attachment 169436 [details] Pimpl alternative Add changelog.

Kenneth Rohde Christiansen

Comment 13 2012-10-19 01:41:34 PDT

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 14 2012-10-19 01:44:40 PDT

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

Comment 15 2012-10-19 02:09:54 PDT

(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

Comment 16 2012-10-19 02:45:27 PDT

(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

Comment 17 2012-10-19 03:32:43 PDT

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

Comment 18 2012-10-19 03:36:20 PDT

Created attachment 169590 [details] Pimpl alternative Take Kenneth feedback into consideration for alternative to Pimpl: - Use AsciiLiteral()

Chris Dumez

Comment 19 2012-10-19 05:52:12 PDT

Comment on attachment 169588 [details] Pimpl Obsoleting the pimpl patch based on the mailing list discussion.

WebKit Review Bot

Comment 20 2012-10-21 03:36:56 PDT

Comment on attachment 169590 [details] Pimpl alternative Clearing flags on attachment: 169590 Committed r132000: <http://trac.webkit.org/changeset/132000>

WebKit Review Bot

Comment 21 2012-10-21 03:37:02 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component WebKit EFL

Assignee

Chris Dumez

Reported

2012-10-18 01:29 PDT

Modified

2012-10-21 03:37 PDT History

CC List

8 users Show

URL

Keywords

Depends on

Blocks

99696

Dependencies

tree graph