Add support for KeyboardEvent.location attribute

Add support for KeyboardEvent.location attribute

Add support for KeyboardEvent.location attribute as per the latest specification:
http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html#events-KeyboardEvent

IE10 and Firefox 22 both support KeyboardEvent.location already.

KeyboardEvent.keyLocation is now marked as deprecated so as to not break backward
compatibility and so that we can track its usage.

The V8 bindings generator was extended to better support [ImplementedAs] and
[DeprecateAs] extended attribute for attributes that have
[InitializedByEventConstructor] extended attribute.

Note that several other attributes of KeyboardEvent still don't match the
specification.

BUG=263308
R=arv@chromium.org, darin@chromium.org, garykac@chromium.org, haraken@chromium.org, ojan@chromium.org, pfeldman@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=155170

[haraken, 2013-07-23 09:21:29]

https://codereview.chromium.org/20034002/diff/1/Source/bindings/scripts/depre...
File Source/bindings/scripts/deprecated_code_generator_v8.pm (right):

https://codereview.chromium.org/20034002/diff/1/Source/bindings/scripts/depre...
Source/bindings/scripts/deprecated_code_generator_v8.pm:2715: $code .= "    " .
GenerateDeprecationNotification($deprecation);

How about generating the following code? Then 'found' is not needed, which will
simplify the code generator.

  if (options.get("attr", eventInit.attr))
    GenerateDeprecationNotification(...)

https://codereview.chromium.org/20034002/diff/1/Source/core/dom/KeyboardEvent...
File Source/core/dom/KeyboardEvent.idl (right):

https://codereview.chromium.org/20034002/diff/1/Source/core/dom/KeyboardEvent...
Source/core/dom/KeyboardEvent.idl:34: [ImplementedAs=location,
DeprecateAs=KeyboardEventKeyLocation, InitializedByEventConstructor] readonly
attribute unsigned long keyLocation;

Nit: Shall we write this just below the 'location' attribute?

[do-not-use, 2013-07-23 10:12:03]

https://codereview.chromium.org/20034002/diff/1/Source/bindings/scripts/depre...
File Source/bindings/scripts/deprecated_code_generator_v8.pm (right):

https://codereview.chromium.org/20034002/diff/1/Source/bindings/scripts/depre...
Source/bindings/scripts/deprecated_code_generator_v8.pm:2715: $code .= "    " .
GenerateDeprecationNotification($deprecation);
On 2013/07/23 09:21:30, haraken wrote:
> 
> How about generating the following code? Then 'found' is not needed, which
will
> simplify the code generator.
> 
>   if (options.get("attr", eventInit.attr))
>     GenerateDeprecationNotification(...)

OK. Will do.

https://codereview.chromium.org/20034002/diff/1/Source/core/dom/KeyboardEvent...
File Source/core/dom/KeyboardEvent.idl (right):

https://codereview.chromium.org/20034002/diff/1/Source/core/dom/KeyboardEvent...
Source/core/dom/KeyboardEvent.idl:34: [ImplementedAs=location,
DeprecateAs=KeyboardEventKeyLocation, InitializedByEventConstructor] readonly
attribute unsigned long keyLocation;
On 2013/07/23 09:21:30, haraken wrote:
> 
> Nit: Shall we write this just below the 'location' attribute?

Ok, if you prefer. I created a "deprecated" section because others are likely to
be added soon. As mentioned in the changelog, several other attributes don't
match the spec. However, keeping new and deprecated version together is also
fine.

[do-not-use, 2013-07-23 10:15:45]

On 2013/07/23 09:21:29, haraken wrote:
>
https://codereview.chromium.org/20034002/diff/1/Source/bindings/scripts/depre...
> File Source/bindings/scripts/deprecated_code_generator_v8.pm (right):
> 
>
https://codereview.chromium.org/20034002/diff/1/Source/bindings/scripts/depre...
> Source/bindings/scripts/deprecated_code_generator_v8.pm:2715: $code .= "    "
.
> GenerateDeprecationNotification($deprecation);
> 
> How about generating the following code? Then 'found' is not needed, which
will
> simplify the code generator.
> 
>   if (options.get("attr", eventInit.attr))
>     GenerateDeprecationNotification(...)
> 
>
https://codereview.chromium.org/20034002/diff/1/Source/core/dom/KeyboardEvent...
> File Source/core/dom/KeyboardEvent.idl (right):
> 
>
https://codereview.chromium.org/20034002/diff/1/Source/core/dom/KeyboardEvent...
> Source/core/dom/KeyboardEvent.idl:34: [ImplementedAs=location,
> DeprecateAs=KeyboardEventKeyLocation, InitializedByEventConstructor] readonly
> attribute unsigned long keyLocation;
> 
> Nit: Shall we write this just below the 'location' attribute?

Done in Patch Set 2.

[haraken, 2013-07-23 10:20:56]

LGTM, but we should wait for an approval from an API owner.

https://codereview.chromium.org/20034002/diff/8001/Source/bindings/scripts/de...
File Source/bindings/scripts/deprecated_code_generator_v8.pm (right):

https://codereview.chromium.org/20034002/diff/8001/Source/bindings/scripts/de...
Source/bindings/scripts/deprecated_code_generator_v8.pm:2697: my
$declaredFoundBoolean = 0;

Nit: This is unused.

https://codereview.chromium.org/20034002/diff/8001/Source/bindings/scripts/de...
Source/bindings/scripts/deprecated_code_generator_v8.pm:2705: if ($deprecation)
{

Nit: You can just say $attribute->extendedAttributes->{"DeprecateAs"}.

[arv (Not doing code reviews), 2013-07-23 17:12:55]

Adding Ojan who has looked at these things before

[arv (Not doing code reviews), 2013-07-23 17:33:22]

Can you add a test that accesses keyLocation so that we are testing the
deprecation too.

https://codereview.chromium.org/20034002/diff/14001/Source/devtools/front_end...
File Source/devtools/front_end/ExtensionAPI.js (right):

https://codereview.chromium.org/20034002/diff/14001/Source/devtools/front_end...
Source/devtools/front_end/ExtensionAPI.js:741: location: event.location
Maybe add:

get keyLocation() {
  return this.keyLocation;
}

Then we do not hit the deprecation warning until actually used

[do-not-use, 2013-07-23 19:42:02]

On 2013/07/23 17:33:22, arv wrote:
> Can you add a test that accesses keyLocation so that we are testing the
> deprecation too.

I already added such test in
LayoutTests/fast/events/constructors/keyboard-event-constructor.html

[do-not-use, 2013-07-23 19:44:02]

https://codereview.chromium.org/20034002/diff/14001/Source/devtools/front_end...
File Source/devtools/front_end/ExtensionAPI.js (right):

https://codereview.chromium.org/20034002/diff/14001/Source/devtools/front_end...
Source/devtools/front_end/ExtensionAPI.js:741: location: event.location
On 2013/07/23 17:33:22, arv wrote:
> Maybe add:
> 
> get keyLocation() {
>   return this.keyLocation;
> }
> 
> Then we do not hit the deprecation warning until actually used

I don't understand this comment, sorry. Since I am renaming to 'location', we
should not hit the deprecation warning already.

[arv (Not doing code reviews), 2013-07-23 20:07:16]

LGTM

https://codereview.chromium.org/20034002/diff/14001/Source/devtools/front_end...
File Source/devtools/front_end/ExtensionAPI.js (right):

https://codereview.chromium.org/20034002/diff/14001/Source/devtools/front_end...
Source/devtools/front_end/ExtensionAPI.js:741: location: event.location
On 2013/07/23 19:44:03, Christophe Dumez wrote:
> On 2013/07/23 17:33:22, arv wrote:
> > Maybe add:
> > 
> > get keyLocation() {
> >   return this.keyLocation;
> > }
> > 
> > Then we do not hit the deprecation warning until actually used
> 
> I don't understand this comment, sorry. Since I am renaming to 'location', we
> should not hit the deprecation warning already.

Ignore me. I thought this was part of something public.

[ojan, 2013-07-24 03:43:30]

This needs to go through the blink launch process before being committed.
Specifically, since this is not behind a flag, it needs LGTMs from 3 API owners
on the "Intent to implement and ship" thread.

http://www.chromium.org/blink#launch-process

This feature seems redundant with the DOM4 code property, but since we're
already shipping it under a different name, and this brings us in accord with IE
and FF, I think it's fine.

https://codereview.chromium.org/20034002/diff/14001/Source/bindings/tests/idl...
File Source/bindings/tests/idls/TestExtendedEvent.idl (right):

https://codereview.chromium.org/20034002/diff/14001/Source/bindings/tests/idl...
Source/bindings/tests/idls/TestExtendedEvent.idl:35:
[InitializedByEventConstructor] readonly attribute unsigned long    location;
extra spaces?

[do-not-use, 2013-07-24 08:58:56]

On 2013/07/24 03:43:30, ojan wrote:
> This needs to go through the blink launch process before being committed.
> Specifically, since this is not behind a flag, it needs LGTMs from 3 API
owners
> on the "Intent to implement and ship" thread.
> 
> http://www.chromium.org/blink#launch-process
> 
> This feature seems redundant with the DOM4 code property, but since we're
> already shipping it under a different name, and this brings us in accord with
IE
> and FF, I think it's fine.

Ok, I sent the intent to implement/ship email as advised.

[do-not-use, 2013-07-25 06:34:15]

On 2013/07/24 08:58:56, Christophe Dumez wrote:
> On 2013/07/24 03:43:30, ojan wrote:
> > This needs to go through the blink launch process before being committed.
> > Specifically, since this is not behind a flag, it needs LGTMs from 3 API
> owners
> > on the "Intent to implement and ship" thread.
> > 
> > http://www.chromium.org/blink#launch-process
> > 
> > This feature seems redundant with the DOM4 code property, but since we're
> > already shipping it under a different name, and this brings us in accord
with
> IE
> > and FF, I think it's fine.
> 
> Ok, I sent the intent to implement/ship email as advised.

I believe the intent to implement/ship received enough support (3 API owners:
Adam Barth, Dimitri Glazkov and Darin Fisher).

Could an API owner please review this patch? (ojan only LGTM'd by mistake).

[ojan, 2013-07-25 17:38:56]

I'd like wez or garykac to sign off on all key event related patches and then
I'm happy to lgtm properly. They are much more aware of the technical and spec
issues involved than anyone else.

[garykac, 2013-07-25 22:35:52]

https://codereview.chromium.org/20034002/diff/24001/LayoutTests/fast/events/k...
File LayoutTests/fast/events/keydown-leftright-keys-expected.txt (right):

https://codereview.chromium.org/20034002/diff/24001/LayoutTests/fast/events/k...
LayoutTests/fast/events/keydown-leftright-keys-expected.txt:9: PASS
lastKeyboardEvent.location is KEY_LOCATION_LEFT
This should be DOM_KEY_LOCATION_LEFT (and RIGHT)

I'm confused by some of these 'xxx-expected.txt' files - some use numeric values
while others use descriptive strings.

I think I prefer the descriptive strings (since 'DOM_KEY_LOCATION_NUMPAD' is
better than '3'), but (frankly) I'm not very familiar with these tests.

And a side comment: I don't see any tests for DOM_KEY_LOCATION_MOBILE or
DOM_KEY_LOCATION_JOYSTICK. It seems outside the scope of this CL, but do you
know if there are plans to include support and/or tests for the values?

[do-not-use, 2013-07-26 07:12:07]

On 2013/07/25 22:35:52, garykac wrote:
>
https://codereview.chromium.org/20034002/diff/24001/LayoutTests/fast/events/k...
> File LayoutTests/fast/events/keydown-leftright-keys-expected.txt (right):
> 
>
https://codereview.chromium.org/20034002/diff/24001/LayoutTests/fast/events/k...
> LayoutTests/fast/events/keydown-leftright-keys-expected.txt:9: PASS
> lastKeyboardEvent.location is KEY_LOCATION_LEFT
> This should be DOM_KEY_LOCATION_LEFT (and RIGHT)
> 
> I'm confused by some of these 'xxx-expected.txt' files - some use numeric
values
> while others use descriptive strings.
> 
> I think I prefer the descriptive strings (since 'DOM_KEY_LOCATION_NUMPAD' is
> better than '3'), but (frankly) I'm not very familiar with these tests.
> 
> And a side comment: I don't see any tests for DOM_KEY_LOCATION_MOBILE or
> DOM_KEY_LOCATION_JOYSTICK. It seems outside the scope of this CL, but do you
> know if there are plans to include support and/or tests for the values?

Our KeyboardEvent interface does not yet expose the DOM_KEY_LOCATION_*
constants. I am planning to add those in a follow-up patch.
This CL is merely about renaming the 'keyLocation' attribute to 'location' to
match the spec.

I think it makes little sense to fix the tests in this CL to display
"DOM_KEY_LOCATION_*" in the results because this will anyway change when I
introduce the "DOM_KEY_LOCATION_*" constants. Right now, these constants are
hard-coded in the tests.

[garykac, 2013-07-26 15:39:15]

lgtm

[do-not-use, 2013-07-26 16:21:19]

On 2013/07/26 15:39:15, garykac wrote:
> lgtm

Thanks garykac@. Could an API owner please give the green light?

[darin (slow to review), 2013-07-26 21:54:43]

LGTM

[commit-bot: I haz the power, 2013-07-27 07:22:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@sisa.samsung.com/20034002/33001

[commit-bot: I haz the power, 2013-07-27 07:34:14]

Retried try job too often on blink_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...

[do-not-use, 2013-07-27 07:50:43]

On 2013/07/27 07:34:14, I haz the power (commit-bot) wrote:
> Retried try job too often on blink_presubmit for step(s) presubmit
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...

pfeldman@ or yurys@, could you please take a look at the changes under
Source/devtools ?
I am missing lgtm for that folder. TIA.

[pfeldman, 2013-07-30 12:30:15]

devtools lgtm

[commit-bot: I haz the power, 2013-07-30 12:55:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@sisa.samsung.com/20034002/46001

[do-not-use, 2013-07-30 17:53:55]

Committed patchset #6 manually as r155170 (presubmit successful).