1260304 - mediasource: prepended to the context menu of MSE videos

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Audio/Video, defect)

Description

[Kevin Brosnan [Ex-Mozilla]]

* Open https://m.youtube.com/watch?v=mOp4szS2lsw
* Start the video playing
* Long press on the video
* mediasource:https://m.youtube.com... is shown

Comment 1

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Video controls issue on Android.

Comment 2

[Blake Wu [:bwu][:blakewu]]

Tim, 
Need your help here!

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Ray, is this something you could investigate? Don't worry if it's out of our scope, but it's never hurt trying.

Comment 4

[Ray Lin[:ralin]]

No problem! I think I found out where the string comes from.

Before the call site Prompt(CtxMenu) showing up, title will be assigned accordingly by these functions[1][2]. In this case, <video> on youtube[3] has only "src" can be took as title, and the "src" is prepended with "mediasource". I'll go deep into whether it is caused by us or youtube next Mon. Thanks!!

[1] https://dxr.mozilla.org/mozilla-central/rev/21bf1af375c1fa8565ae3bb2e89bd1a0809363d4/mobile/android/chrome/content/browser.js#2698-2706
[2] https://dxr.mozilla.org/mozilla-central/rev/21bf1af375c1fa8565ae3bb2e89bd1a0809363d4/mobile/android/chrome/content/browser.js#2628-2653
[3] https://www.dropbox.com/s/n07pnvgjzlqzt3i/Screenshot%202016-04-15%2022.02.00.png?dl=0

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

What's the expected result here? Comment 0 did not specify. Should we show anything other than mediasource: URL there for mediasource: <video> tags like the one on YouTube?

:antlam, could you give us a quick feedback here? Thanks!

Comment 6

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #5)
> What's the expected result here? Comment 0 did not specify. Should we show
> anything other than mediasource: URL there for mediasource: <video> tags
> like the one on YouTube?
> 
> :antlam, could you give us a quick feedback here? Thanks!

We should just show the URL. and remove "mediasource:"

So, if the user long-presses on a video, it should have the corresponding link there only.

Comment 7

[Ray Lin[:ralin]]

Updates:

Same problem presents in the web page[0], and the url prepended with "mediasource" is created by:
> video.src = window.URL.createObjectURL(ms);
It seems to be not caused by browser, but website which leverage new MediaSource API[1].

To deal with this, we can simply strip the string at [2] though I'm not sure it is appropriate.
Margaret, do you think it is a good idea to do so? Thanks!

[0] http://html5-demos.appspot.com/static/media-source.html
[1] https://www.w3.org/TR/media-source/#introduction
[2] https://dxr.mozilla.org/mozilla-central/rev/21bf1af375c1fa8565ae3bb2e89bd1a0809363d4/mobile/android/chrome/content/browser.js#2649

Comment 8

[Ray Lin[:ralin]]

Attachment: Bug-1260304-WIP-Remove-mediasource-string-from-conte.patch

WIP 

Perhaps a more legitimate place to trim possible blob or mediasource scheme off url than comment 7.

Comment 9

[:Margaret Leibovic]

Comment on attachment 8742221 [details] [diff] [review]
Bug-1260304-WIP-Remove-mediasource-string-from-conte.patch

Review of attachment 8742221 [details] [diff] [review]:
-----------------------------------------------------------------

For future patches, you should learn to use MozReview:
http://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview.html

That will make it easier to quickly get context around a patch that's posted for review.

::: mobile/android/chrome/content/browser.js
@@ +2789,4 @@
>        Haptic.performSimpleAction(Haptic.LongPress);
>  
>        // spin through the tree looking for a title for this context menu
> +      let title = this._findTitle(target).replace(/^(?:blob|mediasource):/, '');

I tried looking into how desktop deals with this, but the UI there is different, so this isn't a problem for them. Given that, I think doing this replace is probably our best bet, even if it feels a bit hacky.

However, I think this logic would be better inside of the `_getUrl` method that `_findTitle` eventually ends up calling.

Comment 10

[Ray Lin[:ralin]]

Attachment: MozReview Request: Bug 1260304 - Remove mediasource string from context menu on Android. r?margaret

Review commit: https://reviewboard.mozilla.org/r/47347/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47347/

Comment 11

[:Margaret Leibovic]

Comment on attachment 8742608 [details]
MozReview Request: Bug 1260304 - Remove mediasource string from context menu on Android. r?margaret

https://reviewboard.mozilla.org/r/47347/#review44219

Nice, this is a good improvement over the last patch.

Comment 12

[Ray Lin[:ralin]]

Comment on attachment 8742608 [details]
MozReview Request: Bug 1260304 - Remove mediasource string from context menu on Android. r?margaret

https://reviewboard.mozilla.org/r/47347/#review45471

Comment 13

[Tim Guan-tin Chien [:timdream] (please needinfo)]

What stops this from being landed?

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/49b7c8123960

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/fx-team/rev/49b7c8123960

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/49b7c8123960