RESOLVED FIXED 49143
[GTK] The WebKitWebView should expose a set of signals encapsulating the behavior of the EditorClient
https://bugs.webkit.org/show_bug.cgi?id=49143
Summary [GTK] The WebKitWebView should expose a set of signals encapsulating the beha...
Martin Robinson
Reported 2010-11-07 08:17:07 PST
This functionality is necesarry to pass many layout tests and eventually it would be a very useful part of the API.
Attachments
Add editing dumps (895.70 KB, patch)
2010-11-07 11:56 PST, Martin Robinson
no flags
Patch with addressing Xan's comments (895.45 KB, patch)
2010-11-20 10:40 PST, Martin Robinson
no flags
Patch using g_signal_accumulator_first_wins and fixing a couple other minor issues (897.90 KB, patch)
2010-11-20 22:57 PST, Martin Robinson
xan.lopez: review+
Martin Robinson
Comment 1 2010-11-07 11:56:46 PST
Created attachment 73198 [details] Add editing dumps
Xan Lopez
Comment 2 2010-11-19 18:57:49 PST
Comment on attachment 73198 [details] Add editing dumps View in context: https://bugs.webkit.org/attachment.cgi?id=73198&action=review Holy huge patch Batman. > WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:367 > { Since you are not actually implementing this (why?) you should not add the name to the parameter. > WebKit/gtk/webkit/webkitwebview.cpp:153 > CONSOLE_MESSAGE, This is an ABI break I believe, don't move the signal enum aronud. > WebKit/gtk/webkit/webkitwebview.cpp:1392 > + Is this something like "the first handler wins" or? > WebKit/gtk/webkit/webkitwebview.cpp:2700 > Hrm, is it really useful for subclasses to be able to override this? > WebKit/gtk/webkit/webkitwebview.h:154 > This would be an ABI break; you have to remove one padding.
Xan Lopez
Comment 3 2010-11-19 19:18:26 PST
Comment on attachment 73198 [details] Add editing dumps View in context: https://bugs.webkit.org/attachment.cgi?id=73198&action=review >> WebKit/gtk/webkit/webkitwebview.cpp:1392 >> + > > Is this something like "the first handler wins" or? I think this is indeed a "first wins" type of deal. Why is it needed here? In any case, a built-in accumulator like this was added to glib recently. >> WebKit/gtk/webkit/webkitwebview.cpp:2700 >> > > Hrm, is it really useful for subclasses to be able to override this? Eh, nevermind, I guess you need at least one handler.
Martin Robinson
Comment 4 2010-11-20 09:24:21 PST
(In reply to comment #3) > > Is this something like "the first handler wins" or? > I think this is indeed a "first wins" type of deal. Why is it needed here? In any case, a built-in accumulator like this was added to glib recently. Thanks for the review! The only built in handler I see in GLib 2.26 is g_signal_accumulator_true_handled, which is a little different. That one is not first-handler-wins, but first handler returning TRUE stops the emission. I'm addressing your other comments now.
Martin Robinson
Comment 5 2010-11-20 10:40:12 PST
Created attachment 74484 [details] Patch with addressing Xan's comments
Xan Lopez
Comment 6 2010-11-20 14:52:39 PST
(In reply to comment #4) > (In reply to comment #3) > > > Is this something like "the first handler wins" or? > > I think this is indeed a "first wins" type of deal. Why is it needed here? In any case, a built-in accumulator like this was added to glib recently. > > Thanks for the review! The only built in handler I see in GLib 2.26 is g_signal_accumulator_true_handled, which is a little different. That one is not first-handler-wins, but first handler returning TRUE stops the emission. I'm addressing your other comments now. I didn't meant that one, there's g_signal_accumulator_first_wins, which returns whatever the first accumulator returns.
Xan Lopez
Comment 7 2010-11-20 15:00:19 PST
(In reply to comment #6) > I didn't meant that one, there's g_signal_accumulator_first_wins, which returns whatever the first accumulator returns. I meant the first handler, of course.
Martin Robinson
Comment 8 2010-11-20 22:57:18 PST
Created attachment 74496 [details] Patch using g_signal_accumulator_first_wins and fixing a couple other minor issues
Xan Lopez
Comment 9 2010-11-23 15:44:11 PST
Comment on attachment 74496 [details] Patch using g_signal_accumulator_first_wins and fixing a couple other minor issues Yay!
Martin Robinson
Comment 10 2010-11-24 08:44:09 PST
WebKit Review Bot
Comment 11 2010-11-24 09:26:20 PST
http://trac.webkit.org/changeset/72675 might have broken GTK Linux 64-bit Debug
Note You need to log in before you can comment on or make changes to this bug.