Bug 86354

Created attachment 141689 [details] Patch

Comment on attachment 141689 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141689&action=review > Source/WebKit/efl/ewk/ewk_intent_request.h:66 > + * Unneeded line. > Source/WebKit/efl/ewk/ewk_intent_request.h:77 > + * ditto. > Source/WebKit/efl/ewk/ewk_private.h:206 > +#if ENABLE(WEB_INTENTS) Bug 84572 splits ewk_private.h into each file's private file. So, it seems to me this internal functions need to be moved to ewk_intent_private.h.

Created attachment 141909 [details] Patch Take feedback into consideration.

Comment on attachment 141909 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141909&action=review Looks make sense except for a style nit. Although I almost understand how to work this feature, could you explain how application's able to set intent data via ewk_intent_xxx public APIs ? > Source/WebKit/efl/ewk/ewk_intent_private.h:31 > +void ewk_intent_free(Ewk_Intent *intent); Style nit: Move * to data type side.

Created attachment 142429 [details] Patch

(In reply to comment #4) > (From update of attachment 141909 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=141909&action=review > > Looks make sense except for a style nit. Although I almost understand how to work this feature, could you explain how application's able to set intent data via ewk_intent_xxx public APIs ? Intent data is set on JS side so there is not setter in the ewk wrapper. What the application can do is post the result or failure for an intent request via ewk_intent_request_result_post() and ewk_intent_request_failure_post().

Comment on attachment 142429 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142429&action=review Looks make sense. Please fix trivial style nit again. > Source/WebKit/efl/ewk/ewk_intent_request.h:67 > +EAPI void ewk_intent_request_result_post(Ewk_Intent_Request* request, const char *result); Move '*' to variable side. > Source/WebKit/efl/ewk/ewk_intent_request.h:77 > +EAPI void ewk_intent_request_failure_post(Ewk_Intent_Request* request, const char *failure); ditto.

Created attachment 142436 [details] Patch Fix style nits.

Comment on attachment 142436 [details] Patch rs=me.

Comment on attachment 142436 [details] Patch Rejecting attachment 142436 [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: ommit-queue/Source/WebKit/chromium/third_party/snappy/src --revision 37 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' 44>At revision 37. ________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' Updating webkit projects from gyp files... Full output: http://queues.webkit.org/results/12720495

Comment on attachment 142436 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142436&action=review > Source/WebKit/efl/ChangeLog:49 > + Reviewed by NOBODY (OOPS!). The following ChangeLog files contain OOPS: trunk/Source/WebKit/efl/ChangeLog Please don't ever say "OOPS" in a ChangeLog file. The problem is that you've got two ChangeLog entries in this file.

Created attachment 142642 [details] Patch for landing Could someone please cq+ ?

Comment on attachment 142642 [details] Patch for landing Clearing flags as I spotted minor style issue.

Created attachment 142643 [details] Patch for landing

Comment on attachment 142643 [details] Patch for landing Clearing flags on attachment: 142643 Committed r117551: <http://trac.webkit.org/changeset/117551>

All reviewed patches have been landed. Closing bug.

Comment on attachment 142643 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=142643&action=review > Source/WebKit/CMakeLists.txt:5 > "${WEBCORE_DIR}/Modules/webdatabase" > + "${WEBCORE_DIR}/Modules/intents" Post-mortem nitpick: This list should be alphabetically sorted. > Source/WebKit/PlatformEfl.cmake:123 > + efl/ewk/ewk_intent.cpp > + efl/ewk/ewk_intent_request.cpp Ditto. > Source/WebKit/PlatformEfl.cmake:273 > + ${CMAKE_CURRENT_SOURCE_DIR}/efl/ewk/ewk_intent.h > + ${CMAKE_CURRENT_SOURCE_DIR}/efl/ewk/ewk_intent_request.h Ditto. > Source/WebKit/efl/ChangeLog:31 > + * ewk/ewk_intent.h: Added. Post-mortem nitpick: Shouldn't you install this header and add it to EWebKit.h? > Source/WebKit/efl/ChangeLog:42 > + * ewk/ewk_intent_request.h: Added. Ditto. > Source/WebKit/efl/ewk/ewk_frame.h:43 > + * - "intent,new", EwkIntentRequest*: reports new intent. Post-mortem nitpick: missing underscores in Ewk_Intent_Request. > Source/cmake/OptionsEfl.cmake:84 > +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_WEB_INTENTS, ON) Post-mortem nitpick: extra comma.

(In reply to comment #17) > (From update of attachment 142643 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=142643&action=review > > > Source/WebKit/CMakeLists.txt:5 > > "${WEBCORE_DIR}/Modules/webdatabase" > > + "${WEBCORE_DIR}/Modules/intents" > > Post-mortem nitpick: This list should be alphabetically sorted. > > > Source/WebKit/PlatformEfl.cmake:123 > > + efl/ewk/ewk_intent.cpp > > + efl/ewk/ewk_intent_request.cpp > > Ditto. > > > Source/WebKit/PlatformEfl.cmake:273 > > + ${CMAKE_CURRENT_SOURCE_DIR}/efl/ewk/ewk_intent.h > > + ${CMAKE_CURRENT_SOURCE_DIR}/efl/ewk/ewk_intent_request.h > > Ditto. > > > Source/WebKit/efl/ChangeLog:31 > > + * ewk/ewk_intent.h: Added. > > Post-mortem nitpick: Shouldn't you install this header and add it to EWebKit.h? > > > Source/WebKit/efl/ChangeLog:42 > > + * ewk/ewk_intent_request.h: Added. > > Ditto. > > > Source/WebKit/efl/ewk/ewk_frame.h:43 > > + * - "intent,new", EwkIntentRequest*: reports new intent. > > Post-mortem nitpick: missing underscores in Ewk_Intent_Request. > > > Source/cmake/OptionsEfl.cmake:84 > > +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_WEB_INTENTS, ON) > > Post-mortem nitpick: extra comma. Thanks rakuco, I will fix those style nits in Bug 86986.