Bug 83172 - Switch web intents API to be vendor-prefixed
: Switch web intents API to be vendor-prefixed
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Greg Billock
:
Depends on:
Blocks: 75123
  Show dependency treegraph
 
Reported: 2012-04-04 09:33 PDT by Greg Billock
Modified: 2012-05-07 10:47 PDT (History)
5 users (show)

See Also:


Attachments
Patch (12.92 KB, patch)
2012-04-04 09:35 PDT, Greg Billock
no flags Details | Formatted Diff | Diff
Patch (12.92 KB, patch)
2012-04-04 12:53 PDT, Greg Billock
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (6.45 MB, application/zip)
2012-04-04 14:09 PDT, WebKit Review Bot
no flags Details
Patch (14.33 KB, patch)
2012-04-04 18:10 PDT, Greg Billock
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Greg Billock 2012-04-04 09:33:55 PDT
Switch web intents API to be vendor-prefixed
Comment 1 Greg Billock 2012-04-04 09:35:52 PDT
Created attachment 135612 [details]
Patch
Comment 2 Darin Fisher (:fishd, Google) 2012-04-04 10:24:25 PDT
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
Comment 3 Greg Billock 2012-04-04 12:53:35 PDT
Created attachment 135655 [details]
Patch
Comment 4 Greg Billock 2012-04-04 12:56:40 PDT
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 5 WebKit Review Bot 2012-04-04 14:09:14 PDT
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
Comment 6 WebKit Review Bot 2012-04-04 14:09:20 PDT
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
Comment 7 Greg Billock 2012-04-04 17:48:10 PDT
(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.
Comment 8 Adam Barth 2012-04-04 17:52:57 PDT
> 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?
Comment 9 Greg Billock 2012-04-04 18:10:17 PDT
Created attachment 135733 [details]
Patch
Comment 10 Greg Billock 2012-04-04 18:12:38 PDT
(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 11 Adam Barth 2012-04-04 18:20:07 PDT
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.
Comment 12 Greg Billock 2012-04-04 18:58:52 PDT
(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?
Comment 13 Adam Barth 2012-04-04 19:07:29 PDT
Please don't feel blocked on that discussion.  I just mentioned it because it occurred to me while reading your patch.
Comment 14 WebKit Review Bot 2012-04-04 20:36:28 PDT
Comment on attachment 135733 [details]
Patch

Clearing flags on attachment: 135733

Committed r113282: <http://trac.webkit.org/changeset/113282>
Comment 15 WebKit Review Bot 2012-04-04 20:36:41 PDT
All reviewed patches have been landed.  Closing bug.