Bug 38844

Summary: [GTK] Add support for DOM events in the GObject DOM bindings
Product: WebKit Reporter: Xan Lopez <xan.lopez>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: tevaum, tonikitoo, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 38576    
Attachments:
Description Flags
events.diff
xan.lopez: commit-queue-
domevents.diff
none
signalnames.diff
none
testsignals.diff
none
eventobject.diff none

Description Xan Lopez 2010-05-10 07:02:50 PDT
SSIA
Comment 1 Xan Lopez 2010-05-10 07:48:06 PDT
Created attachment 55553 [details]
events.diff

First patch, expose the DOM events as GObject signals.
Comment 2 WebKit Review Bot 2010-05-10 07:52:06 PDT
Attachment 55553 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:764:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:766:  Use 0 instead of NULL.  [readability/null] [5]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:767:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/bindings/gobject/GObjectEventListener.cpp:39:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/bindings/gobject/GObjectEventListener.h:25:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestInterface.cpp:124:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestInterface.cpp:126:  Use 0 instead of NULL.  [readability/null] [5]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestInterface.cpp:127:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestCallback.cpp:171:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestCallback.cpp:173:  Use 0 instead of NULL.  [readability/null] [5]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestCallback.cpp:174:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 11 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Xan Lopez 2010-05-11 03:44:16 PDT
Created attachment 55687 [details]
domevents.diff

This fixes the style issues that are possible to fix.
Comment 4 WebKit Review Bot 2010-05-11 03:47:49 PDT
Attachment 55687 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:766:  Use 0 instead of NULL.  [readability/null] [5]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestInterface.cpp:126:  Use 0 instead of NULL.  [readability/null] [5]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestCallback.cpp:173:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 3 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Xan Lopez 2010-05-11 05:31:53 PDT
Created attachment 55690 [details]
signalnames.diff

GObject-ify signal names for DOM events.
Comment 6 Xan Lopez 2010-05-11 06:43:48 PDT
Created attachment 55698 [details]
testsignals.diff

Add the infrastructure to test the DOM event signals and test the one signal in DOMWindow which does not require interactivity.
Comment 7 Xan Lopez 2010-05-25 07:44:35 PDT
Created attachment 57017 [details]
eventobject.diff

This patch creates a WebKitDOMEvent class to go with every event signal, and the WebKitDOMEventTarget interface to be implemented by all classes that can be the target of an event. Still no support for the event subclasses in the DOM spec, or for userCapture events, those will come in future patches.
Comment 8 Gustavo Noronha (kov) 2010-05-25 21:38:23 PDT
Comment on attachment 55687 [details]
domevents.diff

WebCore/bindings/gobject/GObjectEventListener.h:31
 +          static PassRefPtr<GObjectEventListener> create(GObject* object, const char* signalName) { return adoptRef(new GObjectEventListener(object, signalName)); }
Indentation looks weird here.

WebCore/bindings/gobject/GObjectEventListener.h:49
 +      virtual void handleEvent(ScriptExecutionContext*, Event*); /*  */
What's the /*  */ for? =)

WebCore/bindings/scripts/test/GObject/WebKitDOMTestCallback.cpp:184
 +  
One too many empty lines here, for my taste.

A good first step IMO. And good improvement on style conformance, too.
Comment 9 Gustavo Noronha (kov) 2010-05-25 21:41:53 PDT
Comment on attachment 55690 [details]
signalnames.diff

WebCore/bindings/scripts/CodeGeneratorGObject.pm:494
 +      my ${listenerName} =  $domSignalName . "Listener";
there's an additional space that seems unnecessary here

Clever idea, the breakWords one.
Comment 10 Gustavo Noronha (kov) 2010-05-25 21:48:20 PDT
Comment on attachment 55698 [details]
testsignals.diff

 56     g_idle_add((GSourceFunc)finish_loading, fixture);
 57     g_main_loop_run(fixture->loop);

Why are you doing this here? I would expect the string to be loaded synchronously, as it is now. Also, I think this test is lacking an assertion that fixture->counter has been incremented at all. I don't think this test is really testing what it's supposed to test, so I'll r-.
Comment 11 Gustavo Noronha (kov) 2010-05-25 21:56:13 PDT
Comment on attachment 57017 [details]
eventobject.diff

Looks good to me. And this is more like it, test-wise. I recommend bringing the creation of the test to this patch, with the changes this one contains already in place, instead of cooking up a separate patch for the test. r=me with this change
Comment 12 Xan Lopez 2010-05-26 03:52:50 PDT
Comment on attachment 55687 [details]
domevents.diff

Pushed with the suggested changes in r60225.
Comment 13 Xan Lopez 2010-05-26 03:53:19 PDT
Comment on attachment 55690 [details]
signalnames.diff

Pushed with the suggested change in r60226.
Comment 14 Xan Lopez 2010-05-26 03:53:41 PDT
Comment on attachment 55698 [details]
testsignals.diff

This is an ex-patch.
Comment 15 Xan Lopez 2010-05-26 03:54:10 PDT
Comment on attachment 57017 [details]
eventobject.diff

Pushed together with previous patch as r60229.
Comment 16 Xan Lopez 2010-05-26 03:54:30 PDT
All patches committed, closing bug.