Bug 49143 - [GTK] The WebKitWebView should expose a set of signals encapsulating the behavior of the EditorClient
Summary: [GTK] The WebKitWebView should expose a set of signals encapsulating the beha...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P3 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks: 25526
  Show dependency treegraph
 
Reported: 2010-11-07 08:17 PST by Martin Robinson
Modified: 2010-11-24 09:26 PST (History)
8 users (show)

See Also:


Attachments
Add editing dumps (895.70 KB, patch)
2010-11-07 11:56 PST, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch with addressing Xan's comments (895.45 KB, patch)
2010-11-20 10:40 PST, Martin Robinson
no flags Details | Formatted Diff | Diff
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+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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