Bug 43453 - [chromium] The chromium API needs a WebMouseEvent class
Summary: [chromium] The chromium API needs a WebMouseEvent class
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-08-03 18:16 PDT by Jay Civelli
Modified: 2010-08-18 15:06 PDT (History)
3 users (show)

See Also:


Attachments
Initial patch (8.23 KB, patch)
2010-08-03 18:21 PDT, Jay Civelli
no flags Details | Formatted Diff | Diff
Fixed style issues (8.17 KB, patch)
2010-08-03 18:38 PDT, Jay Civelli
fishd: review-
Details | Formatted Diff | Diff
Renamed WebEvent to WebDOMEvent (41.85 KB, patch)
2010-08-13 18:39 PDT, Jay Civelli
no flags Details | Formatted Diff | Diff
Fixed style. (41.51 KB, patch)
2010-08-13 23:04 PDT, Jay Civelli
no flags Details | Formatted Diff | Diff
Fixed style. (take 2) (16.66 KB, patch)
2010-08-13 23:52 PDT, Jay Civelli
no flags Details | Formatted Diff | Diff
Adding missing files (50.42 KB, patch)
2010-08-14 00:07 PDT, Jay Civelli
fishd: review-
Details | Formatted Diff | Diff
Applied fishd suggested changes (50.14 KB, patch)
2010-08-17 14:43 PDT, Jay Civelli
no flags Details | Formatted Diff | Diff
Fixing EOL (sigh...) (72.05 KB, patch)
2010-08-17 15:03 PDT, Jay Civelli
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jay Civelli 2010-08-03 18:16:17 PDT
The WebKit::WebNode event listener API only has a WebMutationEvent.
For autofill related functionalities, we now need a WebMouseEvent class.
Comment 1 Jay Civelli 2010-08-03 18:21:19 PDT
Created attachment 63398 [details]
Initial patch
Comment 2 Jay Civelli 2010-08-03 18:22:53 PDT
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.
Comment 3 WebKit Review Bot 2010-08-03 18:27:52 PDT
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.
Comment 4 Jay Civelli 2010-08-03 18:38:25 PDT
Created attachment 63402 [details]
Fixed style issues
Comment 5 Darin Fisher (:fishd, Google) 2010-08-04 01:04:17 PDT
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...
Comment 6 Darin Fisher (:fishd, Google) 2010-08-04 01:05:08 PDT
You can see this union approach in action here:
http://code.google.com/p/ppapi/source/browse/trunk/c/pp_event.h
Comment 7 Jay Civelli 2010-08-12 14:07:20 PDT
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?
Comment 8 Jay Civelli 2010-08-13 18:39:42 PDT
Created attachment 64397 [details]
Renamed WebEvent to WebDOMEvent
Comment 9 Jay Civelli 2010-08-13 22:30:40 PDT
Comment on attachment 64397 [details]
Renamed WebEvent to WebDOMEvent

Renaming WebEvent to WebDOMEvent.
Comment 10 WebKit Review Bot 2010-08-13 22:34:41 PDT
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.
Comment 11 Jay Civelli 2010-08-13 23:04:28 PDT
Created attachment 64408 [details]
Fixed style.
Comment 12 Jay Civelli 2010-08-13 23:52:51 PDT
Created attachment 64410 [details]
Fixed style. (take 2)
Comment 13 Jay Civelli 2010-08-14 00:07:40 PDT
Created attachment 64411 [details]
Adding missing files
Comment 14 Darin Fisher (:fishd, Google) 2010-08-17 11:12:52 PDT
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
Comment 15 Jay Civelli 2010-08-17 14:43:54 PDT
Created attachment 64635 [details]
Applied fishd suggested changes
Comment 16 WebKit Review Bot 2010-08-17 14:46:22 PDT
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.
Comment 17 Jay Civelli 2010-08-17 15:03:14 PDT
Created attachment 64638 [details]
Fixing EOL (sigh...)
Comment 18 WebKit Commit Bot 2010-08-18 11:21:52 PDT
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
Comment 19 WebKit Commit Bot 2010-08-18 15:06:30 PDT
Comment on attachment 64638 [details]
Fixing EOL (sigh...)

Clearing flags on attachment: 64638

Committed r65633: <http://trac.webkit.org/changeset/65633>
Comment 20 WebKit Commit Bot 2010-08-18 15:06:37 PDT
All reviewed patches have been landed.  Closing bug.