Bug 84572 - [EFL] Split ewk_private.h file to multiple private files.
Summary: [EFL] Split ewk_private.h file to multiple private files.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 85178 86354 86456
  Show dependency treegraph
 
Reported: 2012-04-23 00:51 PDT by Tomasz Morawski
Modified: 2012-05-15 02:48 PDT (History)
12 users (show)

See Also:


Attachments
Move private function from ewk_private.h to corresponding private header files. (86.19 KB, patch)
2012-05-11 01:03 PDT, Tomasz Morawski
no flags Details | Formatted Diff | Diff
Fixed (86.32 KB, patch)
2012-05-11 07:48 PDT, Tomasz Morawski
no flags Details | Formatted Diff | Diff
Fixed comments (87.38 KB, patch)
2012-05-14 00:03 PDT, Tomasz Morawski
tonikitoo: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Rebased (87.88 KB, patch)
2012-05-15 00:30 PDT, Tomasz Morawski
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tomasz Morawski 2012-04-23 00:51:05 PDT
This is a master bug to track changes related to introduce of multiple private files in scope of changes:
- Move private function from ewk_private.h to corresponding private header files.
- Move structures form cpp files to corresponding private header files

This change was discused:  https://lists.webkit.org/pipermail/webkit-efl/2012-February/000132.html
Comment 1 Tomasz Morawski 2012-05-11 01:03:31 PDT
Created attachment 141347 [details]
Move private function from ewk_private.h to corresponding private header files.

Patch change: move private function from ewk_private.h to corresponding private header files.
Comment 2 Gyuyoung Kim 2012-05-11 01:55:06 PDT
Comment on attachment 141347 [details]
Move private function from ewk_private.h to corresponding private header files.

Though it is difficult to review because of huge patch, I like this refactoring. Looks fine.
Comment 3 Grzegorz Czajkowski 2012-05-11 02:02:09 PDT
LGTM too.
Comment 4 Thiago Marcos P. Santos 2012-05-11 03:56:04 PDT
Comment on attachment 141347 [details]
Move private function from ewk_private.h to corresponding private header files.

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

General comments:

When generating the docs, I'm getting documentation for a lot of private headers. I think the idea of private headers is exactly the opposite: hide from the users so we don't make any commitment on these APIs.

I just realized that we are not enabling any kind of warning since security origin in not including the private header and I don't get any message. 

I tried a build here disabling all the possible things that we support disabling and it just worked. :)

Thanks for the patch.

> Source/WebKit/efl/ewk/ewk_auth_soup_private.h:61
> + */

Docs of private things should be moved to .cpp files and marked as @internal

> Source/WebKit/efl/ewk/ewk_logging_private.h:1
> +/*

I think the logging macros can stay on the "global" ewk_private.h

> Source/WebKit/efl/ewk/ewk_private.h:28
>  

The docs of this file needs to be updated. It says "Private methods of WebKit-EFL." and now has only some declarations.

> Source/WebKit/efl/ewk/ewk_security_origin_private.h:29
> +// forward declarations

Nitpicking but I don't thing this kind of comments are necessary.
Comment 5 Grzegorz Czajkowski 2012-05-11 04:55:22 PDT
(In reply to comment #4)
> (From update of attachment 141347 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141347&action=review
> 
> General comments:
> 
> When generating the docs, I'm getting documentation for a lot of private headers. I think the idea of private headers is exactly the opposite: hide from the users so we don't make any commitment on these APIs. 

In my opinion private stuff should be documented as well. Application developer has only public headers that are installed to WebKit package so he doesn't see any docs from private file.

In case of generating docs based on Ewk sources I think using doxgen tag 'internal' for each structure, function should satisfy us. It ensures that documentation won't be attached to the html files (it can be changed by edit doxygen config).
We can consider removing lines @file and @brief from private files. Then those files won't be included to final documentation.
Comment 6 Thiago Marcos P. Santos 2012-05-11 05:03:44 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 141347 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=141347&action=review
> > 
> > General comments:
> > 
> > When generating the docs, I'm getting documentation for a lot of private headers. I think the idea of private headers is exactly the opposite: hide from the users so we don't make any commitment on these APIs. 
> 
> In my opinion private stuff should be documented as well. Application developer has only public headers that are installed to WebKit package so he doesn't see any docs from private file.
> 
> In case of generating docs based on Ewk sources I think using doxgen tag 'internal' for each structure, function should satisfy us. It ensures that documentation won't be attached to the html files (it can be changed by edit doxygen config).
> We can consider removing lines @file and @brief from private files. Then those files won't be included to final documentation.

Yes, I agree that it should be documented. But _private.h things should not go to the generated HTML docs. We are in the same page.
Comment 7 Tomasz Morawski 2012-05-11 07:48:44 PDT
Created attachment 141418 [details]
Fixed
Comment 8 Thiago Marcos P. Santos 2012-05-11 09:38:28 PDT
Comment on attachment 141418 [details]
Fixed

Hmmm, just noticed that ewk_logging.h wasn't removed from Source/WebKit/PlatformEfl.cmake and will probably break a "make install".

Same for ewk_js.cpp which still includes "ewk_logging.h" and is broken. Try building with:
Tools/Scripts/build-webkit --efl --debug --cmakearg="-DSHARED_CORE=ON" --netscape-plugin-api --device-orientation --touch-events --vibration --web-audio

Doxygen is still generating HTML for ewk_private.h
Comment 9 Tomasz Morawski 2012-05-14 00:03:14 PDT
Created attachment 141656 [details]
Fixed comments
Comment 10 Thiago Marcos P. Santos 2012-05-14 01:54:14 PDT
LGTM. Thanks!
Comment 11 Ryuan Choi 2012-05-14 08:42:56 PDT
Comment on attachment 141656 [details]
Fixed comments

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

Looks good to me.

I commented nit.

> Source/WebKit/efl/ewk/ewk_tiled_backing_store_private.h:24
> +#include "EWebKit.h"

MIO, ewk_view.h looks better.
Comment 12 Raphael Kubo da Costa (:rakuco) 2012-05-14 10:39:08 PDT
The patch itself does seem fine -- even though I'm OK with keeping things as is, it looks like more people favor this approach; since nothing's going to change to the users, I'm fine with it.

But I would _really_ prefer if this bug served as a master one and we committed separate small changes instead of a bug 90kb patch -- for example, the ewk_logging changes can be committed first, then some header fixes etc.
Comment 13 Thiago Marcos P. Santos 2012-05-14 14:42:39 PDT
(In reply to comment #12)
> The patch itself does seem fine -- even though I'm OK with keeping things as is, it looks like more people favor this approach; since nothing's going to change to the users, I'm fine with it.
> 
> But I would _really_ prefer if this bug served as a master one and we committed separate small changes instead of a bug 90kb patch -- for example, the ewk_logging changes can be committed first, then some header fixes etc.

IMO is better if this commit lands atomically, since it can potentially cause conflicts. In essence, it's a refactoring and shall not introduce any regressions.

I'm personally waiting for this one to contribute the Database API.
Comment 14 WebKit Review Bot 2012-05-14 16:21:18 PDT
Comment on attachment 141656 [details]
Fixed comments

Rejecting attachment 141656 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
wk/ewk_view_single.cpp
patching file Source/WebKit/efl/ewk/ewk_view_tiled.cpp
patching file Source/WebKit/efl/ewk/ewk_window_features.cpp
patching file Source/WebKit/efl/ewk/ewk_window_features_private.h
patching file Tools/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Antonio Go..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/12685474
Comment 15 Gyuyoung Kim 2012-05-14 17:51:50 PDT
It looks you need to rebase.
Comment 16 Tomasz Morawski 2012-05-15 00:30:17 PDT
Created attachment 141877 [details]
Rebased
Comment 17 Tomasz Morawski 2012-05-15 00:36:19 PDT
(In reply to comment #12)
> The patch itself does seem fine -- even though I'm OK with keeping things as is, it looks like more people favor this approach; since nothing's going to change to the users, I'm fine with it.
> 
> But I would _really_ prefer if this bug served as a master one and we committed separate small changes instead of a bug 90kb patch -- for example, the ewk_logging changes can be committed first, then some header fixes etc.

I was care about your opinion especially, thank you for your review and understand of change background.
Comment 18 Gyuyoung Kim 2012-05-15 00:46:12 PDT
Comment on attachment 141877 [details]
Rebased

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

There are many style nits in this patch. This style nits weren't fixed before. Can you fix style nits within no big changes?

http://trac.webkit.org/wiki/EFLWebKitCodingStyle

> Source/WebKit/efl/ewk/ewk_auth_soup_private.h:36
> +    GObject parent_instance;

Do not use _ in variable name.

> Source/WebKit/efl/ewk/ewk_auth_soup_private.h:43
> +GType ewk_auth_soup_dialog_get_type(void);

WebKit EFL adheres webkit coding style and C++ coding style except for public APIs. So, I think you need to remove void keyword in parameter.

> Source/WebKit/efl/ewk/ewk_auth_soup_private.h:50
> +void ewk_auth_soup_credentials_set(const char *username, const char *password, void *data);

Move '*' to data type side.

> Source/WebKit/efl/ewk/ewk_frame_private.h:82
> +WebCore::Frame *coreFrame(const Evas_Object *ewkFrame);

ditto.

> Source/WebKit/efl/ewk/ewk_history_private.h:31
> +Ewk_History_Item *ewk_history_item_new_from_core(WebCore::HistoryItem *core);

ditto.

> Source/WebKit/efl/ewk/ewk_history_private.h:36
> +WebCore::HistoryItem *coreHistoryItem(const Ewk_History_Item *ewkHistoryItem);

ditto.

> Source/WebKit/efl/ewk/ewk_tiled_backing_store_private.h:63
> +    Eina_Tiler *updates;    /**< updated/dirty areas */

ditto.

> Source/WebKit/efl/ewk/ewk_tiled_backing_store_private.h:77
> +    Evas_Object *image;           /**< Evas Image, the tile to be rendered */

ditto.
Comment 19 Chris Dumez 2012-05-15 01:34:43 PDT
Please commit this one as soon as possible since it blocks other patches.
Comment 20 Gyuyoung Kim 2012-05-15 01:43:13 PDT
Comment on attachment 141877 [details]
Rebased

Though I wanna fix style nits as well, this style nit doesn't come from this patch. Let's discuss it further.
Comment 21 WebKit Review Bot 2012-05-15 02:48:40 PDT
Comment on attachment 141877 [details]
Rebased

Clearing flags on attachment: 141877

Committed r117046: <http://trac.webkit.org/changeset/117046>
Comment 22 WebKit Review Bot 2012-05-15 02:48:47 PDT
All reviewed patches have been landed.  Closing bug.