Bug 86354 - [EFL] Add simple implementation of Web Intents
Summary: [EFL] Add simple implementation of Web Intents
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: Chris Dumez
URL:
Keywords:
Depends on: 84572
Blocks: 85364 86868
  Show dependency treegraph
 
Reported: 2012-05-14 03:59 PDT by Chris Dumez
Modified: 2012-05-20 22:20 PDT (History)
5 users (show)

See Also:


Attachments
Patch (24.76 KB, patch)
2012-05-14 04:32 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (26.89 KB, patch)
2012-05-15 03:39 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (26.89 KB, patch)
2012-05-17 00:18 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (26.89 KB, patch)
2012-05-17 00:52 PDT, Chris Dumez
eric: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (25.47 KB, patch)
2012-05-17 23:23 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch for landing (25.47 KB, patch)
2012-05-17 23:26 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2012-05-14 03:59:45 PDT
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.
Comment 1 Chris Dumez 2012-05-14 04:32:11 PDT
Created attachment 141689 [details]
Patch
Comment 2 Gyuyoung Kim 2012-05-14 17:55:55 PDT
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.
Comment 3 Chris Dumez 2012-05-15 03:39:31 PDT
Created attachment 141909 [details]
Patch

Take feedback into consideration.
Comment 4 Gyuyoung Kim 2012-05-16 18:42:31 PDT
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.
Comment 5 Chris Dumez 2012-05-17 00:18:43 PDT
Created attachment 142429 [details]
Patch
Comment 6 Chris Dumez 2012-05-17 00:28:17 PDT
(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 7 Gyuyoung Kim 2012-05-17 00:49:40 PDT
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.
Comment 8 Chris Dumez 2012-05-17 00:52:06 PDT
Created attachment 142436 [details]
Patch

Fix style nits.
Comment 9 Eric Seidel (no email) 2012-05-17 03:20:36 PDT
Comment on attachment 142436 [details]
Patch

rs=me.
Comment 10 WebKit Review Bot 2012-05-17 04:17:16 PDT
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 11 Adam Barth 2012-05-17 04:50:28 PDT
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.
Comment 12 Chris Dumez 2012-05-17 23:23:45 PDT
Created attachment 142642 [details]
Patch for landing

Could someone please cq+ ?
Comment 13 Chris Dumez 2012-05-17 23:24:37 PDT
Comment on attachment 142642 [details]
Patch for landing

Clearing flags as I spotted minor style issue.
Comment 14 Chris Dumez 2012-05-17 23:26:14 PDT
Created attachment 142643 [details]
Patch for landing
Comment 15 WebKit Review Bot 2012-05-18 00:02:44 PDT
Comment on attachment 142643 [details]
Patch for landing

Clearing flags on attachment: 142643

Committed r117551: <http://trac.webkit.org/changeset/117551>
Comment 16 WebKit Review Bot 2012-05-18 00:02:54 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Raphael Kubo da Costa (:rakuco) 2012-05-18 12:03:23 PDT
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.
Comment 18 Chris Dumez 2012-05-20 22:20:10 PDT
(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.