Created attachment 218002 [details] [review]
Postpone the creation of the download object until destination is known

Created attachment 218003 [details] [review]
Add update_link_actions_sensitivity()

Created attachment 218004 [details] [review]
Patch

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?

Review of attachment 218003 [details] [review]:

Looks good.

Smart separators patch landed in webkit already.

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.

(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.

(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.

Created attachment 224074 [details] [review]
Updated patch

ping

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.

(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.

I'm closing this bug, I'll file a new one for the downloads issue.