RESOLVED FIXED 89275
[WK2] Add C API to inspect a Web Intent
https://bugs.webkit.org/show_bug.cgi?id=89275
Summary [WK2] Add C API to inspect a Web Intent
Chris Dumez
Reported 2012-06-16 00:26:47 PDT
We need a C API for WKIntentData so that it can be queried on client side.
Attachments
Patch (11.53 KB, patch)
2012-06-16 09:09 PDT, Chris Dumez
buildbot: commit-queue-
Patch (11.53 KB, patch)
2012-06-16 10:07 PDT, Chris Dumez
buildbot: commit-queue-
Patch (16.92 KB, patch)
2012-06-16 11:44 PDT, Chris Dumez
buildbot: commit-queue-
Patch (17.23 KB, patch)
2012-06-16 12:00 PDT, Chris Dumez
buildbot: commit-queue-
Patch (11.02 KB, patch)
2012-06-16 13:01 PDT, Chris Dumez
no flags
Patch (10.99 KB, patch)
2012-06-16 13:03 PDT, Chris Dumez
no flags
Patch (9.95 KB, patch)
2012-06-17 23:56 PDT, Chris Dumez
no flags
Patch (11.29 KB, patch)
2012-06-17 23:58 PDT, Chris Dumez
andersca: review+
webkit.review.bot: commit-queue-
Patch for landing (11.31 KB, patch)
2012-06-21 10:05 PDT, Chris Dumez
no flags
Chris Dumez
Comment 1 2012-06-16 09:09:56 PDT
Build Bot
Comment 2 2012-06-16 09:52:03 PDT
Chris Dumez
Comment 3 2012-06-16 10:07:34 PDT
Created attachment 147975 [details] Patch Try to make ews-mac happy.
Build Bot
Comment 4 2012-06-16 10:25:46 PDT
Build Bot
Comment 5 2012-06-16 10:46:10 PDT
Chris Dumez
Comment 6 2012-06-16 11:44:11 PDT
Created attachment 147979 [details] Patch Add new files to xcode and win project files. This should hopefully make the ews happy.
Build Bot
Comment 7 2012-06-16 11:51:20 PDT
Chris Dumez
Comment 8 2012-06-16 12:00:04 PDT
Build Bot
Comment 9 2012-06-16 12:37:33 PDT
Build Bot
Comment 10 2012-06-16 12:40:21 PDT
Chris Dumez
Comment 11 2012-06-16 13:01:00 PDT
Chris Dumez
Comment 12 2012-06-16 13:03:07 PDT
Gyuyoung Kim
Comment 13 2012-06-17 17:46:31 PDT
Comment on attachment 147987 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147987&action=review > Source/WebKit2/UIProcess/API/C/WKIntentData.h:31 > +#if ENABLE(WEB_INTENTS) As far as I know, generally, we don't use ENABLE(...) macro in public header file. Because, application need to know ENABLE macro defined in Platform.h. I wonder whether WebKit2 can use ENABLE(...) macro in header file.
Chris Dumez
Comment 14 2012-06-17 21:49:02 PDT
(In reply to comment #13) > (From update of attachment 147987 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=147987&action=review > > > Source/WebKit2/UIProcess/API/C/WKIntentData.h:31 > > +#if ENABLE(WEB_INTENTS) > > As far as I know, generally, we don't use ENABLE(...) macro in public header file. Because, application need to know ENABLE macro defined in Platform.h. I wonder whether WebKit2 can use ENABLE(...) macro in header file. The exact same thing is done in WKInspector.h. Also, I don't believe this is a public header anyway. The public header will be ewk_intent.h, which will use this WKIntentData API internally.
Gyuyoung Kim
Comment 15 2012-06-17 23:22:04 PDT
(In reply to comment #14) > The exact same thing is done in WKInspector.h. Also, I don't believe this is a public header anyway. > The public header will be ewk_intent.h, which will use this WKIntentData API internally. If WKXXX is not public APIs, I agree to use ENABLE() macro in header file. Of course, I already checked INSPECTOR is used in that files. However, as you can see, MiniBrowser is still using WKXXX functions. So, in my opinion, it is good to not use ENABLE() in WK header files. And also, WK functions can be used by other ports. If other ports don't support port specific APIs, they may want to use WK functions directly.
Chris Dumez
Comment 16 2012-06-17 23:24:33 PDT
(In reply to comment #15) > (In reply to comment #14) > > > The exact same thing is done in WKInspector.h. Also, I don't believe this is a public header anyway. > > The public header will be ewk_intent.h, which will use this WKIntentData API internally. > > If WKXXX is not public APIs, I agree to use ENABLE() macro in header file. Of course, I already checked INSPECTOR is used in that files. However, as you can see, MiniBrowser is still using WKXXX functions. So, in my opinion, it is good to not use ENABLE() in WK header files. > > And also, WK functions can be used by other ports. If other ports don't support port specific APIs, they may want to use WK functions directly. I agree with you. I'll check if I can get rid of it in the header without causing compilation issues on Mac.
Chris Dumez
Comment 17 2012-06-17 23:56:11 PDT
Created attachment 148061 [details] Patch Take Gyuyoung's feedback into consideration.
Chris Dumez
Comment 18 2012-06-17 23:58:40 PDT
Gyuyoung Kim
Comment 19 2012-06-18 00:15:03 PDT
Comment on attachment 148063 [details] Patch Looks good to me. But, I'm not sure whether WK2 reviewers like this macro process. Anyway, thank you for your update.
WebKit Review Bot
Comment 20 2012-06-21 09:55:45 PDT
Comment on attachment 148063 [details] Patch Rejecting attachment 148063 [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: urce/WebKit2/UIProcess/API/C/WKIntentData.cpp patching file Source/WebKit2/UIProcess/API/C/WKIntentData.h patching file Source/WebKit2/UIProcess/WebIntentData.cpp patching file Source/WebKit2/UIProcess/WebIntentData.h Hunk #1 FAILED at 50. 1 out of 1 hunk FAILED -- saving rejects to file Source/WebKit2/UIProcess/WebIntentData.h.rej Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Anders Car..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/ Full output: http://queues.webkit.org/results/13028175
Chris Dumez
Comment 21 2012-06-21 10:05:31 PDT
Created attachment 148828 [details] Patch for landing Could someone please cq+ ?
WebKit Review Bot
Comment 22 2012-06-21 12:08:35 PDT
Comment on attachment 148828 [details] Patch for landing Attachment 148828 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13026187
Chris Dumez
Comment 23 2012-06-21 12:11:43 PDT
Comment on attachment 148828 [details] Patch for landing The chromium problem seems unrelated. Setting cq? again. Could someone please cq+?
WebKit Review Bot
Comment 24 2012-06-21 14:23:45 PDT
Comment on attachment 148828 [details] Patch for landing Clearing flags on attachment: 148828 Committed r120963: <http://trac.webkit.org/changeset/120963>
WebKit Review Bot
Comment 25 2012-06-21 14:23:54 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.