Bug 46873 – Rename @webkitspeech to @x-webkit-speech to follow HTML5 convention

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Maciej Stachowiak]

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.

[Jeremy Orlow]

Thanks for pointing this out, Maciej!

[Jeremy Orlow]

http://dev.w3.org/html5/spec/Overview.html#extensibility

I don't see anything about event handlers, so I assume what we have is OK?

[Satish Sampath]

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.

[Satish Sampath]

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?

[Maciej Stachowiak]

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.

[Satish Sampath]

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.

[Satish Sampath]

Created attachment 69334 [details]
Patch

[Satish Sampath]

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.

[Jeremy Orlow]

Comment on attachment 69334 [details]
Patch

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?

[Satish Sampath]

Created attachment 69646 [details]
Patch

Addressed both comments, please take another look.

[Jeremy Orlow]

seems fine to me

[WebKit Commit Bot]

Comment on attachment 69646 [details]
Patch

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:
ful.
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

[Satish Sampath]

Comment on attachment 69646 [details]
Patch

The test failure looks unrelated to this change, so trying the commit queue one more time..

[WebKit Commit Bot]

Comment on attachment 69646 [details]
Patch

Clearing flags on attachment: 69646

Committed r69099: <http://trac.webkit.org/changeset/69099>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.