Bug 72158 - Mouse Lock: MouseEvent IDL
Summary: Mouse Lock: MouseEvent IDL
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Vincent Scheib
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-11 11:17 PST by Vincent Scheib
Modified: 2011-11-14 10:09 PST (History)
4 users (show)

See Also:


Attachments
Patch (6.79 KB, patch)
2011-11-11 11:22 PST, Vincent Scheib
no flags Details | Formatted Diff | Diff
Patch (5.94 KB, patch)
2011-11-11 14:26 PST, Vincent Scheib
no flags Details | Formatted Diff | Diff
Patch (3.15 KB, patch)
2011-11-13 09:58 PST, Vincent Scheib
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vincent Scheib 2011-11-11 11:17:44 PST
Mouse Lock: MouseEvent IDL
Comment 1 Vincent Scheib 2011-11-11 11:22:10 PST
Created attachment 114740 [details]
Patch
Comment 3 Dimitri Glazkov (Google) 2011-11-11 14:07:18 PST
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?
Comment 4 Vincent Scheib 2011-11-11 14:11:04 PST
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?
Comment 5 Dimitri Glazkov (Google) 2011-11-11 14:12:19 PST
(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.
Comment 6 Alexey Proskuryakov 2011-11-11 14:13:40 PST
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.
Comment 7 Dimitri Glazkov (Google) 2011-11-11 14:16:48 PST
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?
Comment 8 Vincent Scheib 2011-11-11 14:26:27 PST
Created attachment 114772 [details]
Patch
Comment 9 Vincent Scheib 2011-11-11 14:30:10 PST
Committed r100024: <http://trac.webkit.org/changeset/100024>
Comment 10 Vincent Scheib 2011-11-11 14:37:21 PST
Reverted r100024 for reason:

Broke chromium build

Committed r100027: <http://trac.webkit.org/changeset/100027>
Comment 11 Vincent Scheib 2011-11-13 09:58:47 PST
Created attachment 114859 [details]
Patch
Comment 12 Vincent Scheib 2011-11-13 13:05:46 PST
Committed r100093: <http://trac.webkit.org/changeset/100093>
Comment 13 Adam Barth 2011-11-13 22:54:36 PST
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.  :)
Comment 14 Vincent Scheib 2011-11-14 09:53:00 PST
(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.
Comment 15 Adam Barth 2011-11-14 10:09:20 PST
> Sure, will follow up with a polish patch.

Thanks!