Bug 83172

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.