Bug 49143

Summary: [GTK] The WebKitWebView should expose a set of signals encapsulating the behavior of the EditorClient
Product: WebKit Reporter: Martin Robinson <mrobinson>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, darin, eric, ojan, rniwa, tony, webkit.review.bot, xan.lopez
Priority: P3 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on:    
Bug Blocks: 25526    
Attachments:
Description Flags
Add editing dumps
none
Patch with addressing Xan's comments
none
Patch using g_signal_accumulator_first_wins and fixing a couple other minor issues xan.lopez: review+

Description Martin Robinson 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.
Comment 1 Martin Robinson 2010-11-07 11:56:46 PST
Created attachment 73198 [details]
Add editing dumps
Comment 2 Xan Lopez 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.
Comment 3 Xan Lopez 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.
Comment 4 Martin Robinson 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.
Comment 5 Martin Robinson 2010-11-20 10:40:12 PST
Created attachment 74484 [details]
Patch with addressing Xan's comments
Comment 6 Xan Lopez 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.
Comment 7 Xan Lopez 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.
Comment 8 Martin Robinson 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
Comment 9 Xan Lopez 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!
Comment 10 Martin Robinson 2010-11-24 08:44:09 PST
Committed r72675: <http://trac.webkit.org/changeset/72675>
Comment 11 WebKit Review Bot 2010-11-24 09:26:20 PST
http://trac.webkit.org/changeset/72675 might have broken GTK Linux 64-bit Debug