Summary: | [EFL][WK2] Make Ewk_Download_Job members private and remove private C functions | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||||||||||||
Component: | WebKit EFL | Assignee: | Chris Dumez <cdumez> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | gyuyoung.kim, kenneth, laszlo.gombos, lucas.de.marchi, mikhail.pozdnyakov, rakuco, tonikitoo, webkit.review.bot | ||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||
Bug Blocks: | 99696 | ||||||||||||||||||||||
Attachments: |
|
Description
Chris Dumez
2012-10-18 01:29:46 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.
Comment on attachment 169367 [details]
Patch
Look how nice it is now! informal r+ from me off course :)
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) :-) (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. 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.
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.
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.
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.
Created attachment 169425 [details]
Pimpl alternative
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 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 :( Created attachment 169436 [details]
Pimpl alternative
Add changelog.
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 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. (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. (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? :-) 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()
Created attachment 169590 [details]
Pimpl alternative
Take Kenneth feedback into consideration for alternative to Pimpl:
- Use AsciiLiteral()
Comment on attachment 169588 [details]
Pimpl
Obsoleting the pimpl patch based on the mailing list discussion.
Comment on attachment 169590 [details] Pimpl alternative Clearing flags on attachment: 169590 Committed r132000: <http://trac.webkit.org/changeset/132000> All reviewed patches have been landed. Closing bug. |