Bug 163176 – Update KeyboardEvent to stop using legacy [ConstructorTemplate=Event]

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Chris Dumez]

Update KeyboardEvent to stop using legacy [ConstructorTemplate=Event] and use a proper constructor instead, like in the specification:
- https://www.w3.org/TR/uievents/#interface-keyboardevent

[Chris Dumez]

Created attachment 291040 [details]
Patch

[WebKit Commit Bot]

Attachment 291040 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/MouseRelatedEvent.cpp:64:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/dom/MouseRelatedEvent.cpp:64:  Wrong number of spaces before statement. (expected: 41)  [whitespace/indent] [4]
Total errors found: 2 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 291040 [details]
Patch

Attachment 291040 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2248029

New failing tests:
fast/events/constructors/keyboard-event-constructor.html

[Build Bot]

Created attachment 291041 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Chris Dumez]

Created attachment 291047 [details]
Patch

[WebKit Commit Bot]

Attachment 291047 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/MouseRelatedEvent.cpp:64:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/dom/MouseRelatedEvent.cpp:64:  Wrong number of spaces before statement. (expected: 41)  [whitespace/indent] [4]
Total errors found: 2 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 291047 [details]
Patch

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

> Source/WebCore/dom/KeyboardEvent.idl:59
> +    unsigned long location;

Why doesn’t this have a default? Is there a distinct “no location provided” state that is different from 0?

> Source/WebCore/dom/KeyboardEvent.idl:63
> +    // legacy.

Strange formatting for this comment. I don’t think these comments that say "this is legacy" are specific enough. I think the comments should state *why* we are keeping the things that are not in the specification.

> Source/WebCore/dom/UIEventWithKeyState.h:54
> +    UIEventWithKeyState(const AtomicString& type, bool canBubble, bool cancelable, double timestamp, DOMWindow* view,
> +        int detail, bool ctrlKey, bool altKey, bool shiftKey, bool metaKey, bool altGraphKey, bool capsLockKey)

Functions like this with long lists of arguments all of the same type are easy to call with incorrect values in code that looks correct; is there an alternate way for us to do this in the future?

> LayoutTests/fast/events/constructors/keyboard-event-constructor-expected.txt:29
> -PASS new KeyboardEvent('eventType', { view: testObject }).view threw exception TypeError: Dictionary member is not of type Window.
> -PASS new KeyboardEvent('eventType', { view: document }).view threw exception TypeError: Dictionary member is not of type Window.
> +PASS new KeyboardEvent('eventType', { view: testObject }).view threw exception TypeError: Type error.
> +PASS new KeyboardEvent('eventType', { view: document }).view threw exception TypeError: Type error.

Are these changes for the better? The old errors seemed to have better messages.

[Chris Dumez]

Comment on attachment 291047 [details]
Patch

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

> Source/WebCore/dom/KeyboardEvent.h:64
> +                , location(location ? *location : keyLocation.valueOr(DOM_KEY_LOCATION_STANDARD))

See the Optional<> trick here for supporting both location and keyLocation as input.

>> Source/WebCore/dom/KeyboardEvent.idl:59
>> +    unsigned long location;
> 
> Why doesn’t this have a default? Is there a distinct “no location provided” state that is different from 0?

As per the specification, the default should be 0. However, we currently support both location (current spec) and keyLocation (old spec) and they both map to the same native member.
I am not specifying a default value so that I can use an Optional<unsigned> for both location and keyLocation on native side. This way I can use location if it was passed and then fallback to using keyLocation.

>> Source/WebCore/dom/KeyboardEvent.idl:63
>> +    // legacy.
> 
> Strange formatting for this comment. I don’t think these comments that say "this is legacy" are specific enough. I think the comments should state *why* we are keeping the things that are not in the specification.

Ok, I will clarify.

>> Source/WebCore/dom/UIEventWithKeyState.h:54
>> +        int detail, bool ctrlKey, bool altKey, bool shiftKey, bool metaKey, bool altGraphKey, bool capsLockKey)
> 
> Functions like this with long lists of arguments all of the same type are easy to call with incorrect values in code that looks correct; is there an alternate way for us to do this in the future?

We need to give this some thought. The reason this is currently needed is because on inheritance and because the generated bindings code initializes like so:
return { a, b, c, d };

A nice way to avoid this would be to generate the bindings code so that initialization is done like so:
UIEventWithKeyState init;
init.a = a;
init.b = b;
init.c = c;
init.d = d;

There is one issue with this though, the struct then needs a default constructor. In most cases this is not really an issue. However, if one of the members is a non-nullable wrapper type, we will currently pass it as a C++ reference, which would prevent having a default constructor. Therefore, if we choose this initialization pattern, we would have to pass non-nullable wrapper types some other way. The easy way would be a raw pointer but sadly, this does not communicate to the implementation that the pointer is non-null and that the bindings code already did a null check.

Let me know if you have thoughts on this.

>> LayoutTests/fast/events/constructors/keyboard-event-constructor-expected.txt:29
>> +PASS new KeyboardEvent('eventType', { view: document }).view threw exception TypeError: Type error.
> 
> Are these changes for the better? The old errors seemed to have better messages.

Right, the convertWrapperType() in JSDOMConvert() does not currently provide a nice exception message. I need to look into this.

[Chris Dumez]

Committed r206971: <http://trac.webkit.org/changeset/206971>

[Chris Dumez]

(In reply to comment #8)
> Comment on attachment 291047 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=291047&action=review
> 
> > Source/WebCore/dom/KeyboardEvent.h:64
> > +                , location(location ? *location : keyLocation.valueOr(DOM_KEY_LOCATION_STANDARD))
> 
> See the Optional<> trick here for supporting both location and keyLocation
> as input.
> 
> >> Source/WebCore/dom/KeyboardEvent.idl:59
> >> +    unsigned long location;
> > 
> > Why doesn’t this have a default? Is there a distinct “no location provided” state that is different from 0?
> 
> As per the specification, the default should be 0. However, we currently
> support both location (current spec) and keyLocation (old spec) and they
> both map to the same native member.
> I am not specifying a default value so that I can use an Optional<unsigned>
> for both location and keyLocation on native side. This way I can use
> location if it was passed and then fallback to using keyLocation.
> 
> >> Source/WebCore/dom/KeyboardEvent.idl:63
> >> +    // legacy.
> > 
> > Strange formatting for this comment. I don’t think these comments that say "this is legacy" are specific enough. I think the comments should state *why* we are keeping the things that are not in the specification.
> 
> Ok, I will clarify.
> 
> >> Source/WebCore/dom/UIEventWithKeyState.h:54
> >> +        int detail, bool ctrlKey, bool altKey, bool shiftKey, bool metaKey, bool altGraphKey, bool capsLockKey)
> > 
> > Functions like this with long lists of arguments all of the same type are easy to call with incorrect values in code that looks correct; is there an alternate way for us to do this in the future?
> 
> We need to give this some thought. The reason this is currently needed is
> because on inheritance and because the generated bindings code initializes
> like so:
> return { a, b, c, d };
> 
> A nice way to avoid this would be to generate the bindings code so that
> initialization is done like so:
> UIEventWithKeyState init;
> init.a = a;
> init.b = b;
> init.c = c;
> init.d = d;
> 
> There is one issue with this though, the struct then needs a default
> constructor. In most cases this is not really an issue. However, if one of
> the members is a non-nullable wrapper type, we will currently pass it as a
> C++ reference, which would prevent having a default constructor. Therefore,
> if we choose this initialization pattern, we would have to pass non-nullable
> wrapper types some other way. The easy way would be a raw pointer but sadly,
> this does not communicate to the implementation that the pointer is non-null
> and that the bindings code already did a null check.
> 
> Let me know if you have thoughts on this.

I uploaded a patch to do so in https://bugs.webkit.org/show_bug.cgi?id=163188.
There is currently only one place (In internal) where we pass a wrapper type by reference.

[Darin Adler]

(In reply to comment #10)
> (In reply to comment #8)
> > There is one issue with this though, the struct then needs a default
> > constructor. In most cases this is not really an issue. However, if one of
> > the members is a non-nullable wrapper type, we will currently pass it as a
> > C++ reference, which would prevent having a default constructor. Therefore,
> > if we choose this initialization pattern, we would have to pass non-nullable
> > wrapper types some other way. The easy way would be a raw pointer but sadly,
> > this does not communicate to the implementation that the pointer is non-null
> > and that the bindings code already did a null check.

Maybe we could use std::reference_wrapper for that?

[Chris Dumez]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #8)
> > > There is one issue with this though, the struct then needs a default
> > > constructor. In most cases this is not really an issue. However, if one of
> > > the members is a non-nullable wrapper type, we will currently pass it as a
> > > C++ reference, which would prevent having a default constructor. Therefore,
> > > if we choose this initialization pattern, we would have to pass non-nullable
> > > wrapper types some other way. The easy way would be a raw pointer but sadly,
> > > this does not communicate to the implementation that the pointer is non-null
> > > and that the bindings code already did a null check.
> 
> Maybe we could use std::reference_wrapper for that?

I thought about this but reference_wrapper has no default constructor. It merely enables copying of references.