Bug 83172 – Switch web intents API to be vendor-prefixed

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Greg Billock]

Switch web intents API to be vendor-prefixed

[Greg Billock]

Created attachment 135612 [details]
Patch

[Darin Fisher (:fishd, Google)]

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

[Greg Billock]

Created attachment 135655 [details]
Patch

[Greg Billock]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Greg Billock]

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

[Adam Barth]

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

[Greg Billock]

Created attachment 135733 [details]
Patch

[Greg Billock]

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

[Adam Barth]

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.

[Greg Billock]

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

[Adam Barth]

Please don't feel blocked on that discussion.  I just mentioned it because it occurred to me while reading your patch.

[WebKit Review Bot]

Comment on attachment 135733 [details]
Patch

Clearing flags on attachment: 135733

Committed r113282: <http://trac.webkit.org/changeset/113282>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.