Bug 72158 – Mouse Lock: MouseEvent IDL

• Component: New Bugs
• Status: RESOLVED FIXED

[Vincent Scheib]

Mouse Lock: MouseEvent IDL

[Vincent Scheib]

Created attachment 114740 [details]
Patch

[Vincent Scheib]

http://dvcs.w3.org/hg/webevents/raw-file/default/mouse-lock.html

[Dimitri Glazkov (Google)]

Comment on attachment 114740 [details]
Patch

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

please fix the bug title before landing.

> Source/WebCore/ChangeLog:3
> +        Mouse Lock: MouseEvent IDL

This is not IDL, right?

[Vincent Scheib]

Comment on attachment 114740 [details]
Patch

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

>> Source/WebCore/ChangeLog:3
>> +        Mouse Lock: MouseEvent IDL
> 
> This is not IDL, right?

The intent of the patch is to stub in the minimal C++ to support the IDL change also in this patch: Source/WebCore/dom/MouseEvent.idl.

Still think I should rename?

[Dimitri Glazkov (Google)]

(In reply to comment #4)
> (From update of attachment 114740 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114740&action=review
> 
> >> Source/WebCore/ChangeLog:3
> >> +        Mouse Lock: MouseEvent IDL
> > 
> > This is not IDL, right?
> 
> The intent of the patch is to stub in the minimal C++ to support the IDL change also in this patch: Source/WebCore/dom/MouseEvent.idl.
> 
> Still think I should rename?

Doh. Sorry, I misread.

[Alexey Proskuryakov]

Why do you keep posting patches for Mouse Lock? You did not get support for adding Mouse Lock it its current form on webkit-dev or elsewhere, as far as I can tell.

The IDL changes in particular are wrong. One cannot add optional arguments in the middle of a function prototype without breaking existing callers.

[Dimitri Glazkov (Google)]

Comment on attachment 114740 [details]
Patch

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

> Source/WebCore/dom/MouseEvent.idl:51
> +#if defined(ENABLE_MOUSE_LOCK_API) && ENABLE_MOUSE_LOCK_API
> +                                           in [Optional=CallWithDefaultValue] long movementX,
> +                                           in [Optional=CallWithDefaultValue] long movementY,
> +#endif

Alexey is right -- this is wrong. Can we remove this for now?

[Vincent Scheib]

Created attachment 114772 [details]
Patch

[Vincent Scheib]

Committed r100024: <http://trac.webkit.org/changeset/100024>

[Vincent Scheib]

Reverted r100024 for reason:

Broke chromium build

Committed r100027: <http://trac.webkit.org/changeset/100027>

[Vincent Scheib]

Created attachment 114859 [details]
Patch

[Vincent Scheib]

Committed r100093: <http://trac.webkit.org/changeset/100093>

[Adam Barth]

Comment on attachment 114859 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Tests for movementX/Y pending mock mouse lock test infrastructure.

Frowns.

> Source/WebCore/dom/MouseEvent.idl:31
> +#if defined(ENABLE_MOUSE_LOCK_API) && ENABLE_MOUSE_LOCK_API
> +        readonly attribute long             movementX;
> +        readonly attribute long             movementY;
> +#endif

Can we use [Conditional=MOUSE_LOCK_API] ?

Also, why ENABLE(MOUSE_LOCK_API) and not just ENABLE(MOUSE_LOCK) ?  Almost everything in ENABLE is an API.  :)

[Vincent Scheib]

(In reply to comment #13)
> (From update of attachment 114859 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114859&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Tests for movementX/Y pending mock mouse lock test infrastructure.
> 
> Frowns.
> 
> > Source/WebCore/dom/MouseEvent.idl:31
> > +#if defined(ENABLE_MOUSE_LOCK_API) && ENABLE_MOUSE_LOCK_API
> > +        readonly attribute long             movementX;
> > +        readonly attribute long             movementY;
> > +#endif
> 
> Can we use [Conditional=MOUSE_LOCK_API] ?
> 
> Also, why ENABLE(MOUSE_LOCK_API) and not just ENABLE(MOUSE_LOCK) ?  Almost everything in ENABLE is an API.  :)

Sure, will follow up with a polish patch.

[Adam Barth]

> Sure, will follow up with a polish patch.

Thanks!