Summary: | [chromium] The chromium API needs a WebMouseEvent class | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jay Civelli <jcivelli> | ||||||||||||||||||
Component: | WebKit Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | commit-queue, fishd, webkit.review.bot | ||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | PC | ||||||||||||||||||||
OS: | OS X 10.5 | ||||||||||||||||||||
Attachments: |
|
Description
Jay Civelli
2010-08-03 18:16:17 PDT
Created attachment 63398 [details]
Initial patch
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. 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.
Created attachment 63402 [details]
Fixed style issues
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...
You can see this union approach in action here: http://code.google.com/p/ppapi/source/browse/trunk/c/pp_event.h 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? Created attachment 64397 [details]
Renamed WebEvent to WebDOMEvent
Comment on attachment 64397 [details]
Renamed WebEvent to WebDOMEvent
Renaming WebEvent to WebDOMEvent.
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.
Created attachment 64408 [details]
Fixed style.
Created attachment 64410 [details]
Fixed style. (take 2)
Created attachment 64411 [details]
Adding missing files
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
Created attachment 64635 [details]
Applied fishd suggested changes
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.
Created attachment 64638 [details]
Fixing EOL (sigh...)
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 on attachment 64638 [details] Fixing EOL (sigh...) Clearing flags on attachment: 64638 Committed r65633: <http://trac.webkit.org/changeset/65633> All reviewed patches have been landed. Closing bug. |