Bug 89275 – [WK2] Add C API to inspect a Web Intent

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Chris Dumez]

We need a C API for WKIntentData so that it can be queried on client side.

[Chris Dumez]

Created attachment 147971 [details]
Patch

[Build Bot]

Comment on attachment 147971 [details]
Patch

Attachment 147971 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12966690

[Chris Dumez]

Created attachment 147975 [details]
Patch

Try to make ews-mac happy.

[Build Bot]

Comment on attachment 147975 [details]
Patch

Attachment 147975 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12970254

[Build Bot]

Comment on attachment 147975 [details]
Patch

Attachment 147975 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12974016

[Chris Dumez]

Created attachment 147979 [details]
Patch

Add new files to xcode and win project files. This should hopefully make the ews happy.

[Build Bot]

Comment on attachment 147979 [details]
Patch

Attachment 147979 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12961897

[Chris Dumez]

Created attachment 147980 [details]
Patch

[Build Bot]

Comment on attachment 147980 [details]
Patch

Attachment 147980 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12973058

[Build Bot]

Comment on attachment 147980 [details]
Patch

Attachment 147980 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12973059

[Chris Dumez]

Created attachment 147986 [details]
Patch

[Chris Dumez]

Created attachment 147987 [details]
Patch

[Gyuyoung Kim]

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]

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

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

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

Created attachment 148061 [details]
Patch

Take Gyuyoung's feedback into consideration.

[Chris Dumez]

Created attachment 148063 [details]
Patch

[Gyuyoung Kim]

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 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]

Created attachment 148828 [details]
Patch for landing

Could someone please cq+ ?

[WebKit Review Bot]

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 on attachment 148828 [details]
Patch for landing

The chromium problem seems unrelated. Setting cq? again. Could someone please cq+?

[WebKit Review Bot]

Comment on attachment 148828 [details]
Patch for landing

Clearing flags on attachment: 148828

Committed r120963: <http://trac.webkit.org/changeset/120963>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.