Bug 83172 - Switch web intents API to be vendor-prefixed
: Switch web intents API to be vendor-prefixed
Status: RESOLVED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
: 75123
  Show dependency treegraph
 
Reported: 2012-04-04 09:33 PST by
Modified: 2012-05-07 10:47 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-04-04 09:33:55 PST
Switch web intents API to be vendor-prefixed
------- Comment #1 From 2012-04-04 09:35:52 PST -------
Created an attachment (id=135612) [details]
Patch
------- Comment #2 From 2012-04-04 10:24:25 PST -------
(From update of attachment 135612 [details])
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 From 2012-04-04 12:53:35 PST -------
Created an attachment (id=135655) [details]
Patch
------- Comment #4 From 2012-04-04 12:56:40 PST -------
(From update of attachment 135612 [details])
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 From 2012-04-04 14:09:14 PST -------
(From update of attachment 135655 [details])
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 From 2012-04-04 14:09:20 PST -------
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
------- Comment #7 From 2012-04-04 17:48:10 PST -------
(In reply to comment #6)
> Created an attachment (id=135676) [details] [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 From 2012-04-04 17:52:57 PST -------
> 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 From 2012-04-04 18:10:17 PST -------
Created an attachment (id=135733) [details]
Patch
------- Comment #10 From 2012-04-04 18:12:38 PST -------
(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 From 2012-04-04 18:20:07 PST -------
(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.
------- Comment #12 From 2012-04-04 18:58:52 PST -------
(In reply to comment #11)
> (From update of attachment 135733 [details] [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 From 2012-04-04 19:07:29 PST -------
Please don't feel blocked on that discussion.  I just mentioned it because it occurred to me while reading your patch.
------- Comment #14 From 2012-04-04 20:36:28 PST -------
(From update of attachment 135733 [details])
Clearing flags on attachment: 135733

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