Bug 46873 - Rename @webkitspeech to @x-webkit-speech to follow HTML5 convention
Summary: Rename @webkitspeech to @x-webkit-speech to follow HTML5 convention
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
Depends on:
Blocks: 39485
  Show dependency treegraph
Reported: 2010-09-29 21:49 PDT by Maciej Stachowiak
Modified: 2010-10-05 02:03 PDT (History)
5 users (show)

See Also:

Patch (9.70 KB, patch)
2010-09-30 07:27 PDT, Satish Sampath
no flags Details | Formatted Diff | Diff
Patch (9.53 KB, patch)
2010-10-04 10:08 PDT, Satish Sampath
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Maciej Stachowiak 2010-09-29 21:49:56 PDT
HTML5 recommends the pattern x-vendor-attribute pattern for vendor extension attributes. Please apply this pattern and rename @webkitspeech to @x-webkit-speech to follow HTML5 convention.
Comment 1 Jeremy Orlow 2010-09-30 02:18:32 PDT
Thanks for pointing this out, Maciej!
Comment 2 Jeremy Orlow 2010-09-30 02:21:54 PDT

I don't see anything about event handlers, so I assume what we have is OK?
Comment 3 Satish Sampath 2010-09-30 02:29:16 PDT
I believe Jeremy is asking about the planned addition of a new event 'onspeechchange', this is not implemented yet. From the spec above I wonder if the event handler is treated the same way attributes are, so the event handler would become x-webkit-onspeechchange in the markup and webkitonspeechchange - though it doesn't seem right for an event handler to not have the 'on' prefix.
Comment 4 Satish Sampath 2010-09-30 02:40:29 PDT
Looking at existing features, I see the patterns such as 'onwebkitfullscreenchange' and the same name is used both in the markup as the attribute name and in the IDL. So looks like for speech input we can have the new event handler named as 'onwebkitspeechchange' for both the markup and IDL. Maciej, can you please confirm if my understanding is correct?
Comment 5 Maciej Stachowiak 2010-09-30 02:48:55 PDT
I think if the event name is prefixed, then t's probably better to use the normal onfoo attributes. The event name itself should not contain "on", that's only for the attribute. So I'd suggest:

event name: webkitspeechchange
event handler attribute: onwebkitspeechchange

Maybe it would be good for HTML5 to suggest that for extended event handler attributes too.
Comment 6 Satish Sampath 2010-09-30 03:24:16 PDT
Looks like if I add x-webkit-speech to HTMLAttributeNames.in, the bindings code generator emits the c++ code for attribute named "x_webkit_speechAttr". But adding an IDL attribute named 'webkitSpeech' expects the attribute to be named as 'webkitSpeechAttr', which doesn't match the above generated code. Perhaps this requires an update to the bindings generator? I'm looking at that now.. please let me know if this is not the right track.
Comment 7 Satish Sampath 2010-09-30 07:27:51 PDT
Created attachment 69334 [details]
Comment 8 Satish Sampath 2010-09-30 07:29:49 PDT
I've uploaded a patch to rename the attribute, it required some additions to the dom/make_names.pl script as well. Please take a look and suggest if any other webkit reviewer should have a look as well.
Comment 9 Jeremy Orlow 2010-10-04 09:25:26 PDT
Comment on attachment 69334 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=69334&action=review

> WebCore/dom/make_names.pl:187
> +    if ($attr =~ m/^x-webkit/) {

Maybe do m/^x-webkit-(.*)/ and then use the $1 as the attribute?

> WebCore/dom/make_names.pl:193
> +        $attr =~ s/-/_/g;

You probably want this in both code paths, right?
Comment 10 Satish Sampath 2010-10-04 10:08:10 PDT
Created attachment 69646 [details]

Addressed both comments, please take another look.
Comment 11 Jeremy Orlow 2010-10-04 10:17:52 PDT
seems fine to me
Comment 12 WebKit Commit Bot 2010-10-04 23:49:11 PDT
Comment on attachment 69646 [details]

Rejecting patch 69646 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive']" exit_code: 2
Last 500 characters of output:
Files=14, Tests=304,  1 wallclock secs ( 0.63 cusr +  0.13 csys =  0.76 CPU)
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/abarth/git/webkit-queue/LayoutTests
Testing 21454 test cases.
http/tests/xmlhttprequest/basic-auth.html -> failed

Exiting early after 1 failures. 21300 tests run.
532.30s total testing time

21299 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
34 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/4142077
Comment 13 Satish Sampath 2010-10-05 01:34:22 PDT
Comment on attachment 69646 [details]

The test failure looks unrelated to this change, so trying the commit queue one more time..
Comment 14 WebKit Commit Bot 2010-10-05 02:03:49 PDT
Comment on attachment 69646 [details]

Clearing flags on attachment: 69646

Committed r69099: <http://trac.webkit.org/changeset/69099>
Comment 15 WebKit Commit Bot 2010-10-05 02:03:55 PDT
All reviewed patches have been landed.  Closing bug.