After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 679366 - Port context menu to WebKit2
Port context menu to WebKit2
Status: RESOLVED FIXED
Product: epiphany
Classification: Core
Component: Backend
git master
Other Linux
: Normal normal
: ---
Assigned To: Xan Lopez
Epiphany Maintainers
Depends on:
Blocks: 678610
 
 
Reported: 2012-07-04 06:50 UTC by Carlos Garcia Campos
Modified: 2012-10-11 10:24 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Postpone the creation of the download object until destination is known (3.02 KB, patch)
2012-07-04 10:33 UTC, Carlos Garcia Campos
reviewed Details | Review
Add update_link_actions_sensitivity() (2.16 KB, patch)
2012-07-04 10:33 UTC, Carlos Garcia Campos
committed Details | Review
Patch (10.90 KB, patch)
2012-07-04 10:33 UTC, Carlos Garcia Campos
reviewed Details | Review
Updated patch (10.87 KB, patch)
2012-09-12 08:16 UTC, Carlos Garcia Campos
committed Details | Review

Description Carlos Garcia Campos 2012-07-04 06:50:02 UTC
Use WebKit2 API to implement the context menu. Using WebKit2 API we could combine custom actions with webkit stock actions and add unicode and input methods submenu items to the context menu for editable content, for example.
Comment 1 Carlos Garcia Campos 2012-07-04 10:33:02 UTC
Created attachment 218002 [details] [review]
Postpone the creation of the download object until destination is known
Comment 2 Carlos Garcia Campos 2012-07-04 10:33:23 UTC
Created attachment 218003 [details] [review]
Add update_link_actions_sensitivity()
Comment 3 Carlos Garcia Campos 2012-07-04 10:33:40 UTC
Created attachment 218004 [details] [review]
Patch
Comment 4 Xan Lopez 2012-08-10 10:09:37 UTC
Comment on attachment 218002 [details] [review]
Postpone the creation of the download object until destination is known

Hrm, as discussed elsewhere (https://bugzilla.gnome.org/show_bug.cgi?id=674291#c3) there are some kinds of bugs with suggested filenames in downloads that can only be fixed if we start downloads even before showing the filechooser at all. This seems to go exactly in the opposite direction?
Comment 5 Xan Lopez 2012-08-10 10:13:40 UTC
Review of attachment 218003 [details] [review]:

Looks good.
Comment 6 Carlos Garcia Campos 2012-08-14 09:07:20 UTC
Smart separators patch landed in webkit already.
Comment 7 Xan Lopez 2012-09-11 16:21:38 UTC
Review of attachment 218004 [details] [review]:

OK, this seems pretty straightforward. My comment/question in comment #4 still applies, fwiw.

::: src/ephy-window.c
@@ +1879,3 @@
+
+		if (webkit_context_menu_item_get_stock_action (item) == action)
+			return g_object_ref (item);

Nitpick, but if get_stock_action returns NULL or whatever for separators I think you can just get rid of the is_separator check.

@@ +1912,3 @@
+	update_edit_actions_sensitivity (window, FALSE);
+
+	if (webkit_hit_test_result_context_is_link (hit_test_result)) {

Braces go in next line in old code style.

::: src/popup-commands.c
@@ +101,3 @@
+
+	event = ephy_window_get_context_event (window);
+	g_assert (event != NULL);

If we have to do anything, let's do g_return_if_fail please.
Comment 8 Carlos Garcia Campos 2012-09-12 07:54:36 UTC
(In reply to comment #7)
> Review of attachment 218004 [details] [review]:
> 
> OK, this seems pretty straightforward. My comment/question in comment #4 still
> applies, fwiw.

Patches don't depend on each other.

> ::: src/ephy-window.c
> @@ +1879,3 @@
> +
> +        if (webkit_context_menu_item_get_stock_action (item) == action)
> +            return g_object_ref (item);
> 
> Nitpick, but if get_stock_action returns NULL or whatever for separators I
> think you can just get rid of the is_separator check.

stock actions are enum values, separators would return WEBKIT_CONTEXT_MENU_ACTION_NO_ACTION, if we assume we won't be interested in looking for NO_ACTION items, or that in such case the first NO_ACTION item will be returned, we could avoid the is_separator() check, but I think it's more clear and harmless.

> @@ +1912,3 @@
> +    update_edit_actions_sensitivity (window, FALSE);
> +
> +    if (webkit_hit_test_result_context_is_link (hit_test_result)) {
> 
> Braces go in next line in old code style.

Ok, I'll fix coding style issues.

> ::: src/popup-commands.c
> @@ +101,3 @@
> +
> +    event = ephy_window_get_context_event (window);
> +    g_assert (event != NULL);
> 
> If we have to do anything, let's do g_return_if_fail please.

I don't think using g_return_if_fail in private methods is a good idea. The method should only be called internally from the context menu callback, so event will be a valid pointer unless something really wrong has happened, and in such case the program is going to crash sooner or later.
Comment 9 Carlos Garcia Campos 2012-09-12 08:12:36 UTC
(In reply to comment #4)
> (From update of attachment 218002 [details] [review])
> Hrm, as discussed elsewhere
> (https://bugzilla.gnome.org/show_bug.cgi?id=674291#c3) there are some kinds of
> bugs with suggested filenames in downloads that can only be fixed if we start
> downloads even before showing the filechooser at all. This seems to go exactly
> in the opposite direction?

In WebKit2, downloads don't start on demand, there's no webkit_download_start. You request the web context to start a download and it returns a download object. So, without this patch, as soon as you select save as, you will see the file chooser and a download already running (well, actually two downloads because of bug #678993) in the downloads bar. The location you choose from the file chooser won't have any effect because the download is already running with the default download destination.
Comment 10 Carlos Garcia Campos 2012-09-12 08:16:53 UTC
Created attachment 224074 [details] [review]
Updated patch
Comment 11 Carlos Garcia Campos 2012-09-24 10:36:19 UTC
ping
Comment 12 Xan Lopez 2012-10-11 10:08:19 UTC
Review of attachment 224074 [details] [review]:

OK.

::: src/popup-commands.c
@@ +101,3 @@
+
+	event = ephy_window_get_context_event (window);
+	g_assert (event != NULL);

If this is really unlikely to fail just remove the g_assert. We don't add asserts all over the place for things that basically should never fail (among other things because they are not really like WebKit's ASSERT).

@@ +104,3 @@
+
+	result = ephy_embed_event_get_hit_test_result (event);
+	if (!webkit_hit_test_result_context_is_link (result))

Missing the braces, I guess.
Comment 13 Carlos Garcia Campos 2012-10-11 10:17:07 UTC
(In reply to comment #12)
> Review of attachment 224074 [details] [review]:
> 
> OK.

Thanks!

> ::: src/popup-commands.c
> @@ +101,3 @@
> +
> +    event = ephy_window_get_context_event (window);
> +    g_assert (event != NULL);
> 
> If this is really unlikely to fail just remove the g_assert. We don't add
> asserts all over the place for things that basically should never fail (among
> other things because they are not really like WebKit's ASSERT).

Sure, this assert comes from the current wk1 code that uses g_return macro. I thought g_assert was more appropriate here than g_return, but I can just remove it.

> @@ +104,3 @@
> +
> +    result = ephy_embed_event_get_hit_test_result (event);
> +    if (!webkit_hit_test_result_context_is_link (result))
> 
> Missing the braces, I guess.

Right.
Comment 14 Carlos Garcia Campos 2012-10-11 10:24:30 UTC
I'm closing this bug, I'll file a new one for the downloads issue.