Summary: | [GTK] The WebKitWebView should expose a set of signals encapsulating the behavior of the EditorClient | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Martin Robinson <mrobinson> | ||||||||
Component: | WebKitGTK | Assignee: | 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
Martin Robinson
2010-11-07 08:17:07 PST
Created attachment 73198 [details]
Add editing dumps
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 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. (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. Created attachment 74484 [details]
Patch with addressing Xan's comments
(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. (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. Created attachment 74496 [details]
Patch using g_signal_accumulator_first_wins and fixing a couple other minor issues
Comment on attachment 74496 [details]
Patch using g_signal_accumulator_first_wins and fixing a couple other minor issues
Yay!
Committed r72675: <http://trac.webkit.org/changeset/72675> http://trac.webkit.org/changeset/72675 might have broken GTK Linux 64-bit Debug |