Bug 112461 - [GTK] Switch ContextMenu implementation to CROSS_PLATFORM_CONTEXT_MENU
Summary: [GTK] Switch ContextMenu implementation to CROSS_PLATFORM_CONTEXT_MENU
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 111874
  Show dependency treegraph
 
Reported: 2013-03-15 13:15 PDT by Jesus Sanchez-Palencia
Modified: 2017-03-11 10:56 PST (History)
4 users (show)

See Also:


Attachments
WIP patch (missing Changelog) (23.41 KB, patch)
2013-03-15 13:15 PDT, Jesus Sanchez-Palencia
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jesus Sanchez-Palencia 2013-03-15 13:15:50 PDT
Created attachment 193364 [details]
WIP patch (missing Changelog)

GTK could switch to cross platform context menu implementation and reduce Context Menu related code significantly.
Comment 1 Jesus Sanchez-Palencia 2013-03-15 13:28:54 PDT
Comment on attachment 193364 [details]
WIP patch (missing Changelog)

View in context: https://bugs.webkit.org/attachment.cgi?id=193364&action=review

> Source/WebCore/platform/gtk/ContextMenuItemGtk.cpp:146
> +    //if (!gtk_action_get_accel_path(action))

After this patch, on MiniBrowser (WK2) I can see the Context Menu items being rendered properly but when I click on them nothing happens. If I uncomment this early return, though, the check for gtk_action_get_accel_path(action) always fail. Thus, early returning here. I couldn't find what could have broken this "accel_path", since I couldn't find the analog gtk setter function (gtk_action_SET_accel_path) being called anywhere...
Comment 2 Gustavo Noronha (kov) 2013-03-16 08:50:53 PDT
Comment on attachment 193364 [details]
WIP patch (missing Changelog)

View in context: https://bugs.webkit.org/attachment.cgi?id=193364&action=review

>> Source/WebCore/platform/gtk/ContextMenuItemGtk.cpp:146
>> +    //if (!gtk_action_get_accel_path(action))
> 
> After this patch, on MiniBrowser (WK2) I can see the Context Menu items being rendered properly but when I click on them nothing happens. If I uncomment this early return, though, the check for gtk_action_get_accel_path(action) always fail. Thus, early returning here. I couldn't find what could have broken this "accel_path", since I couldn't find the analog gtk setter function (gtk_action_SET_accel_path) being called anywhere...

I have a theory: I believe in the previous implementation the GtkMenuItem object would have been created when you get here, and would have caused the action to get an accel path set.
Comment 3 Jesus Sanchez-Palencia 2013-03-18 07:10:48 PDT
(In reply to comment #2)
> I have a theory: I believe in the previous implementation the GtkMenuItem object would have been created when you get here, and would have caused the action to get an accel path set.

But it would have had its accel path set, right? I can't find any call to gtk_action_set_accel_path(), assuming this was the right function to be called... (I have close to 0 experience with GTK =/)
Comment 4 Gustavo Noronha (kov) 2013-03-18 07:58:01 PDT
Right, my theory is the creation of a widget (like the menu item that gets created without this patch) attached to the action causes the accel path to be set as a side-effect, that means you won't find a set call inside WebKit, it's being done inside GTK+.
Comment 5 Jesus Sanchez-Palencia 2013-03-28 14:10:23 PDT
I have put some more investigation into this. The problem is that after this patch we are not keeping a GtkMenu* anywhere anymore and we should create it on demand. WebKitGTK's design relys too much in WebCore creating and keeping this pointer inside ContextMenu. The same goes to ContextMenuItem and GtkMenuItem*.

I did have some progress there, but I've reached a point that my lack of time to understand Gtk and GObject specificities prevent me of going on. At least not without risking WebKitGTK+'s API implementation. =/

It would be better if someone from the GTK port could look into this when he/she have enough free time for it.

Thanks for helping!