Bug 89275 - [WK2] Add C API to inspect a Web Intent
Summary: [WK2] Add C API to inspect a Web Intent
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on:
Blocks: 88303 89749
  Show dependency tree / graph
 
Reported: 2012-06-16 00:26 PDT by Chris Dumez
Modified: 2012-06-22 01:43 PDT (History)
12 users (show)

See Also:

Attachments
Patch (11.53 KB, patch)
2012-06-16 09:09 PDT, Chris Dumez 		buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch (11.53 KB, patch)
2012-06-16 10:07 PDT, Chris Dumez 		buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch (16.92 KB, patch)
2012-06-16 11:44 PDT, Chris Dumez 		buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch (17.23 KB, patch)
2012-06-16 12:00 PDT, Chris Dumez 		buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch (11.02 KB, patch)
2012-06-16 13:01 PDT, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (10.99 KB, patch)
2012-06-16 13:03 PDT, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (9.95 KB, patch)
2012-06-17 23:56 PDT, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (11.29 KB, patch)
2012-06-17 23:58 PDT, Chris Dumez 		andersca: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (11.31 KB, patch)
2012-06-21 10:05 PDT, Chris Dumez 		no flags Details | Formatted Diff | Diff
Show Obsolete (8) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2012-06-16 00:26:47 PDT 
We need a C API for WKIntentData so that it can be queried on client side.
Comment 1 Chris Dumez 2012-06-16 09:09:56 PDT 
Created attachment 147971 [details]
Patch
Comment 2 Build Bot 2012-06-16 09:52:03 PDT 
Comment on attachment 147971 [details]
Patch

Attachment 147971 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12966690
Comment 3 Chris Dumez 2012-06-16 10:07:34 PDT 
Created attachment 147975 [details]
Patch

Try to make ews-mac happy.
Comment 4 Build Bot 2012-06-16 10:25:46 PDT 
Comment on attachment 147975 [details]
Patch

Attachment 147975 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12970254
Comment 5 Build Bot 2012-06-16 10:46:10 PDT 
Comment on attachment 147975 [details]
Patch

Attachment 147975 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12974016
Comment 6 Chris Dumez 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.
Comment 7 Build Bot 2012-06-16 11:51:20 PDT 
Comment on attachment 147979 [details]
Patch

Attachment 147979 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12961897
Comment 8 Chris Dumez 2012-06-16 12:00:04 PDT 
Created attachment 147980 [details]
Patch
Comment 9 Build Bot 2012-06-16 12:37:33 PDT 
Comment on attachment 147980 [details]
Patch

Attachment 147980 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12973058
Comment 10 Build Bot 2012-06-16 12:40:21 PDT 
Comment on attachment 147980 [details]
Patch

Attachment 147980 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12973059
Comment 11 Chris Dumez 2012-06-16 13:01:00 PDT 
Created attachment 147986 [details]
Patch
Comment 12 Chris Dumez 2012-06-16 13:03:07 PDT 
Created attachment 147987 [details]
Patch
Comment 13 Gyuyoung Kim 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.
Comment 14 Chris Dumez 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.
Comment 15 Gyuyoung Kim 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.
Comment 16 Chris Dumez 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.
Comment 17 Chris Dumez 2012-06-17 23:56:11 PDT 
Created attachment 148061 [details]
Patch

Take Gyuyoung's feedback into consideration.
Comment 18 Chris Dumez 2012-06-17 23:58:40 PDT 
Created attachment 148063 [details]
Patch
Comment 19 Gyuyoung Kim 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.
Comment 20 WebKit Review Bot 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
Comment 21 Chris Dumez 2012-06-21 10:05:31 PDT 
Created attachment 148828 [details]
Patch for landing

Could someone please cq+ ?
Comment 22 WebKit Review Bot 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
Comment 23 Chris Dumez 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+?
Comment 24 WebKit Review Bot 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>
Comment 25 WebKit Review Bot 2012-06-21 14:23:54 PDT 
All reviewed patches have been landed.  Closing bug.