Bug 199999 - Multiple context menu actions broken for YouTube videos
Summary: Multiple context menu actions broken for YouTube videos
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-07-22 07:42 PDT by Michael Catanzaro
Modified: 2019-07-29 02:07 PDT (History)
11 users (show)

See Also:


Attachments
Patch (8.31 KB, patch)
2019-07-25 02:41 PDT, Carlos Garcia Campos
eric.carlson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2019-07-22 07:42:53 PDT
The following context menu actions are broken on youtube.com videos:

WEBKIT_CONTEXT_MENU_ACTION_OPEN_VIDEO_IN_NEW_WINDOW
WEBKIT_CONTEXT_MENU_ACTION_DOWNLOAD_VIDEO_TO_DISK

The former tries to load a blob: URI in a new tab (impossible) and the later just does nothing (probably because of MSE). I'm really not sure what to do about either of these, other than that leaving them broken isn't OK. My only idea is to change WebKitHitTestResult to report that these videos are not videos, but that's... meh.

Not tested, but also suspect:

WEBKIT_CONTEXT_MENU_ACTION_OPEN_AUDIO_IN_NEW_WINDOW
WEBKIT_CONTEXT_MENU_ACTION_DOWNLOAD_AUDIO_TO_DISK
Comment 1 Philippe Normand 2019-07-22 08:02:57 PDT
Broken how?
How is this related with GStreamer?
Comment 2 Philippe Normand 2019-07-22 08:06:01 PDT
MediaPlayerPrivateGStreamer::canSaveMediaData() returns false on Youtube/MSE videos.
Comment 3 Carlos Garcia Campos 2019-07-23 03:51:03 PDT
WPE doesn't support context menus
Comment 4 Carlos Garcia Campos 2019-07-23 06:03:15 PDT
I'm getting a custom context menu from youtube when right clicking a video in youtube.
Comment 5 Michael Catanzaro 2019-07-23 06:42:12 PDT
(In reply to Carlos Garcia Campos from comment #4)
> I'm getting a custom context menu from youtube when right clicking a video
> in youtube.

First right-click brings up the website's custom context menu. Then right-click a second time to get Epiphany's.
Comment 6 Carlos Garcia Campos 2019-07-25 00:34:03 PDT
Those actions are disabled in ff and chromium context menus.
Comment 7 Carlos Garcia Campos 2019-07-25 00:39:58 PDT
Safari shows options to copy video url and open video in new window, but both options are broken too.
Comment 8 Carlos Garcia Campos 2019-07-25 02:41:51 PDT
Created attachment 374884 [details]
Patch

Michael, we can fix ephy without having to depend on this patach, since download option was already excluded.
Comment 9 Michael Catanzaro 2019-07-25 06:26:38 PDT
Comment on attachment 374884 [details]
Patch

Nice, but I wonder if it's semantically right. E.g. a video with a blob: URI should actually be downloadable if it's not using MSE, but copy media link would result in a meaningless URI and open in new window wouldn't work. Right?
Comment 10 Carlos Garcia Campos 2019-07-25 06:29:56 PDT
This is not disabled because it's a blob uri, but because it's a live stream. MediaPlayer::canSaveMediaData() returns False.
Comment 11 Michael Catanzaro 2019-07-25 06:41:26 PDT
(In reply to Carlos Garcia Campos from comment #10)
> This is not disabled because it's a blob uri, but because it's a live
> stream. MediaPlayer::canSaveMediaData() returns False.

OK, but that just proves my point: this will fail if you have a normal video as a blob URI, right?
Comment 12 Michael Catanzaro 2019-07-25 06:57:05 PDT
Comment on attachment 374884 [details]
Patch

I guess this is necessary regardless, though, because if the media is not downloadable then surely it doesn't have a usable link... is that always true?
Comment 13 Philippe Normand 2019-07-25 07:11:46 PDT
You could make canSaveMediaData() return false for "mediasourceblob" URIs and true for "blob" URIs.
Comment 14 Carlos Garcia Campos 2019-07-25 07:30:42 PDT
If the media is not downloadable the url is useless I think, and it can't be opened in a new tab/window either. This is what ff and chromium does. In the case of chromium, it allows to copy the link url, but I'm not sure if that's on purpose or just a bug. Firefox has all the options disabled. In chromium the have this:

    case IDC_CONTENT_CONTEXT_OPENAVNEWTAB:
      // Currently, a media element can be opened in a new tab iff it can                                                                                                                     
      // be saved. So rather than duplicating the MediaCanSave flag, we rely                                                                                                                  
      // on that here.                                                                                                                                                                        
      return !!(params_.media_flags & WebContextMenuData::kMediaCanSave);
Comment 15 Michael Catanzaro 2019-07-25 09:37:25 PDT
OK from me then, but it needs an owner.
Comment 16 Carlos Garcia Campos 2019-07-26 00:48:31 PDT
This doesn't require an owner because it's a change in WebCore, not WebKit2. But still, I want to make sure Apple is also ok with this change before landing.
Comment 17 Radar WebKit Bug Importer 2019-07-26 07:04:33 PDT
<rdar://problem/53585686>
Comment 18 Carlos Garcia Campos 2019-07-29 02:07:33 PDT
Committed r247901: <https://trac.webkit.org/changeset/247901>