Bug 43453 – [chromium] The chromium API needs a WebMouseEvent class

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Jay Civelli]

The WebKit::WebNode event listener API only has a WebMutationEvent.
For autofill related functionalities, we now need a WebMouseEvent class.

[Jay Civelli]

Created attachment 63398 [details]
Initial patch

[Jay Civelli]

There is already a WebMouseEvent class defined in WebInputEvent.h used for different purposes.
Not sure what would be a good name for this one, used the really lame WebMouseEvent2 in the first patch :-(
Suggestions welcome.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/chromium/src/WebMouseEvent2.cpp:102:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKit/chromium/public/WebMouseEvent2.h:1:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
Total errors found: 63 in 5 files


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

[Jay Civelli]

Created attachment 63402 [details]
Fixed style issues

[Darin Fisher (:fishd, Google)]

Comment on attachment 63402 [details]
Fixed style issues

Yeah, WebMouseEvent2 is obviously not going to cut it.  We probably should
rename the subclasses of WebInputEvent.  That way we don't have to change
our convention of using WebFoo for DOM thingies named Foo.  MouseEvent
should map to WebMouseEvent given that convention.

Maybe inner classes?  WebInputEvent::{Generic,Mouse,Wheel,Keyboard}?  The
downside to such a solution is that it forces the inclusion of WebInputEvent.h
in places that could previously just forward declare it.  We could also
try to do something using a union:

  struct WebInputEvent {
    enum {...} type;
    ... other common fields ...
    union {
      struct {
        ...
      } mouse;
      struct {
        ...
      } wheel;
      struct {
        ...
      } keyboard;
    } data;
  };

Then it would still be valid to forward declare 'struct WebInputEvent'.

I need to think about this more, but those are some ideas...

[Darin Fisher (:fishd, Google)]

You can see this union approach in action here:
http://code.google.com/p/ppapi/source/browse/trunk/c/pp_event.h

[Jay Civelli]

I don't think we can do the union, as the WebTouchEvent contains a WebTouchPoint which has a constructor and that I don't think it is OK for a union to have a member which has a non default constructor.
I guess we could make WebTouchPoint a struct with no constructor and make sure it is initialized properly were it is used.

May be going with the inner-class would be easier (although it prevents us from forward declaring the events)?

What do you think?

[Jay Civelli]

Created attachment 64397 [details]
Renamed WebEvent to WebDOMEvent

[Jay Civelli]

Comment on attachment 64397 [details]
Renamed WebEvent to WebDOMEvent

Renaming WebEvent to WebDOMEvent.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/chromium/src/WebDOMEventListenerPrivate.h:1:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
WebKit/chromium/src/EventListenerWrapper.h:44:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
WebKit/chromium/src/WebDOMEventListener.cpp:1:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
WebKit/chromium/src/WebDOMEventListenerPrivate.cpp:1:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
WebKit/chromium/src/EventListenerWrapper.cpp:37:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
Total errors found: 297 in 16 files


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

[Jay Civelli]

Created attachment 64408 [details]
Fixed style.

[Jay Civelli]

Created attachment 64410 [details]
Fixed style. (take 2)

[Jay Civelli]

Created attachment 64411 [details]
Adding missing files

[Darin Fisher (:fishd, Google)]

Comment on attachment 64411 [details]
Adding missing files

WebKit/chromium/public/WebNode.h:102
 +      WEBKIT_API void addDOMEventListener(const WebString& eventType, WebDOMEventListener* listener, bool useCapture);
nit: these should just be named add/removeEventListener to match
how they are named in the DOM specification.

WebKit/chromium/src/EventListenerWrapper.h:50
 +  class EventListenerWrapperDOM : public EventListener {
EventListenerWrapperDOM is an awkward name.  can we rename the other
one DeprecatedEventListenerWrapper, and then use EventListenerWrapper?

otherwise, LGTM

[Jay Civelli]

Created attachment 64635 [details]
Applied fishd suggested changes

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/chromium/src/WebDOMEventListenerPrivate.cpp:1:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
WebKit/chromium/src/WebDOMEventListenerPrivate.h:1:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
WebKit/chromium/src/WebEventListenerPrivate.h:46:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
WebKit/chromium/src/EventListenerWrapper.h:44:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
WebKit/chromium/src/WebEventListener.cpp:49:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
WebKit/chromium/src/WebDOMEventListener.cpp:1:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
WebKit/chromium/src/EventListenerWrapper.cpp:37:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
WebKit/chromium/src/WebEventListenerPrivate.cpp:48:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
Total errors found: 317 in 20 files


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

[Jay Civelli]

Created attachment 64638 [details]
Fixing EOL (sigh...)

[WebKit Commit Bot]

Comment on attachment 64638 [details]
Fixing EOL (sigh...)

Rejecting patch 64638 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--wait-for-httpd', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1
Last 500 characters of output:
CommitQueue/LayoutTests
Testing 20868 test cases.
media/video-volume.html -> timed out
Sampling process 52835 for 10 seconds with 10 milliseconds of run time between samples
Sampling completed, processing symbols...
Sample analysis of process 52835 written to file /Users/eseidel/Library/Logs/DumpRenderTree/HangReport.txt

Exiting early after 1 failures. 17265 tests run.
902.79s total testing time

17264 test cases (99%) succeeded
1 test case (<1%) timed out
40 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/3742346

[WebKit Commit Bot]

Comment on attachment 64638 [details]
Fixing EOL (sigh...)

Clearing flags on attachment: 64638

Committed r65633: <http://trac.webkit.org/changeset/65633>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.