mediasource: prepended to the context menu of MSE videos

RESOLVED FIXED in Firefox 49

Status

()

Firefox for Android
Audio/Video
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: kbrosnan, Assigned: ralin)

Tracking

unspecified
Firefox 49
All
Android
Points:
---

Firefox Tracking Flags

(firefox49 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

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

Updated

2 years ago
OS: Unspecified → Android
Hardware: Unspecified → All
Video controls issue on Android.
Flags: needinfo?(bwu)
Tim, 
Need your help here!
Flags: needinfo?(bwu) → needinfo?(timdream)
Ray, is this something you could investigate? Don't worry if it's out of our scope, but it's never hurt trying.
Flags: needinfo?(timdream) → needinfo?(ralin)
(Assignee)

Comment 4

2 years ago
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
Flags: needinfo?(ralin)
Assignee: nobody → ralin
Status: NEW → ASSIGNED
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!
Flags: needinfo?(alam)
(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.
Flags: needinfo?(alam)
(Assignee)

Comment 7

2 years ago
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
Flags: needinfo?(margaret.leibovic)
(Assignee)

Comment 8

2 years ago
Created attachment 8742221 [details] [diff] [review]
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

2 years ago
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.

Updated

2 years ago
Flags: needinfo?(margaret.leibovic)
(Assignee)

Comment 10

2 years ago
Created attachment 8742608 [details]
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/
Attachment #8742608 - Flags: review?(margaret.leibovic)

Updated

2 years ago
Attachment #8742608 - Flags: review?(margaret.leibovic) → review+

Comment 11

2 years ago
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.
(Assignee)

Comment 12

2 years ago
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
Attachment #8742608 - Flags: review+
What stops this from being landed?
Flags: needinfo?(ralin)
(Assignee)

Updated

2 years ago
Flags: needinfo?(ralin)
Keywords: checkin-needed

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/49b7c8123960
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
You need to log in before you can comment on or make changes to this bug.