Bug 86354 – [EFL] Add simple implementation of Web Intents

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Chris Dumez]

Ewk classes are needed to wrap WebCore::Intent and WebCore::IntentRequest.

dispatchIntent() needs to be implemented in EFL's FrameLoaderClient to make clients aware of new intent requests.

[Chris Dumez]

Created attachment 141689 [details]
Patch

[Gyuyoung Kim]

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.

[Chris Dumez]

Created attachment 141909 [details]
Patch

Take feedback into consideration.

[Gyuyoung Kim]

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.

[Chris Dumez]

Created attachment 142429 [details]
Patch

[Chris Dumez]

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

[Gyuyoung Kim]

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.

[Chris Dumez]

Created attachment 142436 [details]
Patch

Fix style nits.

[Eric Seidel (no email)]

Comment on attachment 142436 [details]
Patch

rs=me.

[WebKit Review Bot]

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

[Adam Barth]

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.

[Chris Dumez]

Created attachment 142642 [details]
Patch for landing

Could someone please cq+ ?

[Chris Dumez]

Comment on attachment 142642 [details]
Patch for landing

Clearing flags as I spotted minor style issue.

[Chris Dumez]

Created attachment 142643 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 142643 [details]
Patch for landing

Clearing flags on attachment: 142643

Committed r117551: <http://trac.webkit.org/changeset/117551>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Raphael Kubo da Costa (:rakuco)]

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.

[Chris Dumez]

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