Summary: | Switch web intents API to be vendor-prefixed | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Greg Billock <gbillock> | ||||||||||
Component: | New Bugs | Assignee: | Greg Billock <gbillock> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | abarth, dglazkov, fishd, ojan, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 75123 | ||||||||||||
Attachments: |
|
Description
Greg Billock
2012-04-04 09:33:55 PDT
Created attachment 135612 [details]
Patch
Comment on attachment 135612 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=135612&action=review > Source/WebCore/Modules/intents/DOMWindowIntents.idl:33 > + attribute IntentConstructor webkitIntent; nit: should be WebKitIntent note the case. constructors should start with an upper case letter. > LayoutTests/webintents/resources/web-intents-testing.js:21 > + navigator.webkitStartActivity(new webkitIntent("action1", "mime/type1", "test"), onSuccess, onFailure); webkitIntent -> WebKitIntent Created attachment 135655 [details]
Patch
Comment on attachment 135612 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=135612&action=review >> Source/WebCore/Modules/intents/DOMWindowIntents.idl:33 >> + attribute IntentConstructor webkitIntent; > > nit: should be WebKitIntent > > note the case. constructors should start with an upper case letter. Changing (note that we're using this lc style for speech, audio, and IDB). >> LayoutTests/webintents/resources/web-intents-testing.js:21 >> + navigator.webkitStartActivity(new webkitIntent("action1", "mime/type1", "test"), onSuccess, onFailure); > > webkitIntent -> WebKitIntent Done Comment on attachment 135655 [details] Patch Attachment 135655 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12329068 New failing tests: fast/dom/navigator-detached-no-crash.html Created attachment 135676 [details]
Archive of layout-test-results from ec2-cr-linux-02
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
(In reply to comment #6) > Created an attachment (id=135676) [details] > Archive of layout-test-results from ec2-cr-linux-02 > > The attached test failures were seen while running run-webkit-tests on the chromium-ews. > Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick I'm not sure how to read this. The results don't show anything for fast/dom/navigator-detached-no-crash.html, which I'd assume means it passed, but obviously means something else. > I'm not sure how to read this. The results don't show anything for fast/dom/navigator-detached-no-crash.html, which I'd assume means it passed, but obviously means something else.
It might be uploading the wrong zip. Have you tried running the test locally?
Created attachment 135733 [details]
Patch
(In reply to comment #8) > > I'm not sure how to read this. The results don't show anything for fast/dom/navigator-detached-no-crash.html, which I'd assume means it passed, but obviously means something else. > > It might be uploading the wrong zip. Have you tried running the test locally? Hurrah for testing. I'd forgotten that expectations change for platform/chromium, which is now included. Comment on attachment 135733 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=135733&action=review > Source/WebCore/Modules/intents/NavigatorIntents.idl:28 > - void startActivity(in Intent intent, > - in [Callback, Optional] IntentResultCallback successCallback, > - in [Callback, Optional] IntentResultCallback failureCallback) > + void webkitStartActivity(in Intent intent, > + in [Callback, Optional] IntentResultCallback successCallback, > + in [Callback, Optional] IntentResultCallback failureCallback) Your patch follows the traditional style of naming the implementation of vendor-prefixed functions with the vendor prefix, but one thing we've been discussing is whether to use <https://trac.webkit.org/wiki/WebKitIDL#ImplementedAs> to keep the vendor-prefixed names in IDL and use unprefixed names in the implementation. (In reply to comment #11) > (From update of attachment 135733 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=135733&action=review > > > Source/WebCore/Modules/intents/NavigatorIntents.idl:28 > > - void startActivity(in Intent intent, > > - in [Callback, Optional] IntentResultCallback successCallback, > > - in [Callback, Optional] IntentResultCallback failureCallback) > > + void webkitStartActivity(in Intent intent, > > + in [Callback, Optional] IntentResultCallback successCallback, > > + in [Callback, Optional] IntentResultCallback failureCallback) > > Your patch follows the traditional style of naming the implementation of vendor-prefixed functions with the vendor prefix, but one thing we've been discussing is whether to use <https://trac.webkit.org/wiki/WebKitIDL#ImplementedAs> to keep the vendor-prefixed names in IDL and use unprefixed names in the implementation. I'd like to get this checked in soon. Is that discussion settled? Or shall I get this in and then update to follow the agreed convention? Please don't feel blocked on that discussion. I just mentioned it because it occurred to me while reading your patch. Comment on attachment 135733 [details] Patch Clearing flags on attachment: 135733 Committed r113282: <http://trac.webkit.org/changeset/113282> All reviewed patches have been landed. Closing bug. |