Bug 49143 – [GTK] The WebKitWebView should expose a set of signals encapsulating the behavior of the EditorClient

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Martin Robinson]

This functionality is necesarry to pass many layout tests and eventually it would be a very useful part of the API.

[Martin Robinson]

Created attachment 73198 [details]
Add editing dumps

[Xan Lopez]

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 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]

(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]

Created attachment 74484 [details]
Patch with addressing Xan's comments

[Xan Lopez]

(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]

(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]

Created attachment 74496 [details]
Patch using g_signal_accumulator_first_wins and fixing a couple other minor issues

[Xan Lopez]

Comment on attachment 74496 [details]
Patch using g_signal_accumulator_first_wins and fixing a couple other minor issues

Yay!

[Martin Robinson]

Committed r72675: <http://trac.webkit.org/changeset/72675>

[WebKit Review Bot]

http://trac.webkit.org/changeset/72675 might have broken GTK Linux 64-bit Debug