Created attachment 284325 [details] [review]
popup-commands: add include guards

Also add useless G_BEGIN_DECLS and G_END_DECLS since it's pretty
standard to use these everywhere.

Created attachment 284326 [details] [review]
Add context menu entries for media elements

This commit adds the following new context menu entries for video and
audio elements:

* Save Media As...
* Copy Media Address
* Play Media

These context menu entries intentionally mirror the entries we use for
images.

It would be nicer to use the labels "Save Video As...", "Save Audio
As..." etc. but I didn't spot an obivous way to determine the type of
media.

Review of attachment 284325 [details] [review]:

G_BEGIN_DECLS and G_END_DECLS are not useless, it's true they are not needed in this case, because this is not a public header that can be included by a C++ file, so I would reword the commit message.

::: src/popup-commands.h
@@ +19,3 @@
 
+#if !defined (__EPHY_EPIPHANY_H_INSIDE__) && !defined (EPIPHANY_COMPILATION)
+#error "Only <epiphany/epiphany.h> can be included directly."

I don't think we need this at all, this is not a public header.

But we don't have any public headers, so that boilerplate isn't needed anywhere. I guess this is a remnant from when we used to have extensions?  Are we purposefully keeping a distinction between headers that might in the future be public if we were to have extensions again?  I notice the makefiles have separate sections for installed and uninstalled headers (none of which get installed) which I was thinking about cleaning up....

(In reply to comment #2)
> Created an attachment (id=284326) [details] [review]
> Add context menu entries for media elements
> 
> This commit adds the following new context menu entries for video and
> audio elements:
> 
> * Save Media As...
> * Copy Media Address
> * Play Media

How can you play media if we don't expose API for that?

> These context menu entries intentionally mirror the entries we use for
> images.

I think open audio/video in new tab/window would also make sense, more similar to a link than to an image.

> It would be nicer to use the labels "Save Video As...", "Save Audio
> As..." etc. but I didn't spot an obivous way to determine the type of
> media.

Right, we should probably add webkit_hit_test_result_get_media_type() or something like that, but even in that case, we would need to use the items from the default context menu for actions not exposed in the API (play, pause, mute, fullscreen, toggle media controls, etc). So, for now we can look for video items in the default context menu to figure out whether it's a video element or not.

Created attachment 284499 [details] [review]
A different approach to add media actions to context menu

This is currently broken for non English locales due to a bug in WebKit, the only problem is that you would see Audio instead of Video in the context menu entries, see https://bugs.webkit.org/show_bug.cgi?id=136249

Review of attachment 284499 [details] [review]:

Yay, this is better than my approach.

::: src/ephy-window.c
@@ +1786,3 @@
 						    webkit_context_menu_item_new_separator ());
+		add_item_to_context_menu (context_menu, input_methods_item);
+		add_item_to_context_menu (context_menu, unicode_item);

The WebKitContextMenuItem in webkit_context_menu_append() is not nullable, so either this is wrong or else the conditional above if (input_methods_item || unicode_item) is also unnecessary.

(It also looks unrelated.)

(In reply to comment #4)
> But we don't have any public headers, so that boilerplate isn't needed
> anywhere. I guess this is a remnant from when we used to have extensions?  Are
> we purposefully keeping a distinction between headers that might in the future
> be public if we were to have extensions again?  I notice the makefiles have
> separate sections for installed and uninstalled headers (none of which get
> installed) which I was thinking about cleaning up....

Carlos, what do you think about this? ^

(In reply to comment #5)
> I think open audio/video in new tab/window would also make sense, more similar
> to a link than to an image.

That's what my "Play Media" entry does, which is really silly. It was intended to parallel "View Image" but I realized "View Media" doesn't work for audio.  Your approach is better, but we should also rename "View Image" to "Open Image in New Tab." I'll do that in another bug.

(In reply to comment #7)
> Review of attachment 284499 [details] [review]:
> 
> Yay, this is better than my approach.
> 
> ::: src/ephy-window.c
> @@ +1786,3 @@
>                              webkit_context_menu_item_new_separator ());
> +        add_item_to_context_menu (context_menu, input_methods_item);
> +        add_item_to_context_menu (context_menu, unicode_item);
> 
> The WebKitContextMenuItem in webkit_context_menu_append() is not nullable, so
> either this is wrong or else the conditional above if (input_methods_item ||
> unicode_item) is also unnecessary.

add_item_to_context_menu returns early if the item is NULL.

> (It also looks unrelated.)

It uses a new method added in this commit, I can split it, but it's related.

(In reply to comment #8)
> (In reply to comment #4)
> > But we don't have any public headers, so that boilerplate isn't needed
> > anywhere. I guess this is a remnant from when we used to have extensions?  Are
> > we purposefully keeping a distinction between headers that might in the future
> > be public if we were to have extensions again?  I notice the makefiles have
> > separate sections for installed and uninstalled headers (none of which get
> > installed) which I was thinking about cleaning up....
> 
> Carlos, what do you think about this? ^

Remove the single header check and leave the begin/end macros.

> (In reply to comment #5)
> > I think open audio/video in new tab/window would also make sense, more similar
> > to a link than to an image.
> 
> That's what my "Play Media" entry does, which is really silly. It was intended
> to parallel "View Image" but I realized "View Media" doesn't work for audio. 
> Your approach is better, but we should also rename "View Image" to "Open Image
> in New Tab." I'll do that in another bug.

Ok.

(In reply to comment #9)
> add_item_to_context_menu returns early if the item is NULL.

Oops, yes.

Comment on attachment 284325 [details] [review]
popup-commands: add include guards

Attachment 284325 [details] pushed as fb97f9f - popup-commands: add include guards