Bug 84572

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 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.

LGTM too.

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.

(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.

(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.

Created attachment 141418 [details] Fixed

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

Created attachment 141656 [details] Fixed comments

LGTM. Thanks!

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.

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.

(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 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

It looks you need to rebase.

Created attachment 141877 [details] Rebased

(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 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.

Please commit this one as soon as possible since it blocks other patches.

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 on attachment 141877 [details] Rebased Clearing flags on attachment: 141877 Committed r117046: <http://trac.webkit.org/changeset/117046>

All reviewed patches have been landed. Closing bug.