1284548 - Audio file provides Share Video option

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mike Kaply [:mkaply]]

If you go to:

http://spe.mobilephone.net/wit/objects/ylwsub_9s_32kbps_mn.mp3

In Fennec and do a long hold, you'll be presented with a "Share Video" option at the top.

I believe this should be "Share Audio"

The save option correctly says "Save Audio"

Comment 1

[Mike Kaply [:mkaply]]

Our generic sharing item is video:

https://dxr.mozilla.org/mozilla-central/source/mobile/android/locales/en-US/chrome/browser.properties#274

While our saving items are audio/video:

https://dxr.mozilla.org/mozilla-central/source/mobile/android/locales/en-US/chrome/browser.properties#278

Comment 2

[Ian Moody [:Kwan] (UTC+1)]

Attachment: Bug 1284548 - Use 'Share Audio' label for sharing audio played in <video>. ?

Vary the 'Share [...]' label depending on the actual use of the <video> tag, as
is done for the 'Save [...]' label.

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

Comment 3

[Ian Moody [:Kwan] (UTC+1)]

Attachment: Bug 1284548 - Put logic classifying <video> element based on content in a function and reuse it. ?

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

Comment 4

[Ian Moody [:Kwan] (UTC+1)]

This bugged me, so thanks for saving me creating a bug mkaply!

The reason this happens is two fold:
1) Firefox uses a <video> when loading bare media files, regardless of the type of file.
2) The Save context item varies its label based on what is known about the file in the media tag: if nothing is known because it's not loaded it uses "Media", if the file's videoWidth and Height are 0 it uses "Audio", and uses "Video" otherwise.  The Share item has none of this logic.
(interestingly, files in an <audio> get no Share action at all, fodder for another bug perhaps)

I have a patch that duplicates this logic so that the Share label is customised the same way, and another patch that de-dupes this logic into a helper function, since with my Share patch it's used three separate times. The reviewer may wish me to re-order the patches so the de-duping one comes first, which I'm more than happy to do.
I also don't currently change the generic mime type from video/* to audio/*, which might be worth doing.

(In my first de-dupe patch the function took the strings to return in an object, but I decided that was too verbose, so return hardcoded ones now, with the only downside being needing to sync the string ID suffixes)

snorp, would you be willing to give some feedback on this/re-direct to someone who can please?

Comment 5

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8934266 [details]
Bug 1284548 - Use 'Share Audio' label for sharing audio played in <video>. ?

https://reviewboard.mozilla.org/r/205182/#review219716

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8934267 [details]
Bug 1284548 - Put logic classifying <video> element based on content in a function and reuse it. ?

https://reviewboard.mozilla.org/r/205184/#review219718

Looks good with one nit

::: mobile/android/locales/en-US/chrome/browser.properties:318
(Diff revision 1)
>  # is working while "Paste" / "Add as search engine" triggers the bug. See bug 1262098 for more details.
>  # Manual testing the scenario described in bug 1262098 is highly recommended.
>  contextmenu.addSearchEngine3=Add Search Engine
>  contextmenu.playMedia=Play
>  contextmenu.pauseMedia=Pause
> -contextmenu.shareMedia2=Share Media
> +contextmenu.shareMedia1=Share Media

This probably needs to be contextmenu.shareMedia3, since the current name indicates there was already a contextmenu.shareMedia1.

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

Sorry this took so long, Ian! I didnn't realize this was a review.

Comment 8

[Ian Moody [:Kwan] (UTC+1)]

Comment on attachment 8934267 [details]
Bug 1284548 - Put logic classifying <video> element based on content in a function and reuse it. ?

https://reviewboard.mozilla.org/r/205184/#review219804

(In reply to James Willcox (:snorp) ([e-mail]) from comment #7)
> Sorry this took so long, Ian! I didnn't realize this was a review.

No problem! Not exactly urgent, and I thought it was probably more at the feedback stage anyway.

::: mobile/android/chrome/content/browser.js:811
(Diff revision 1)
>          let url = (aElement.currentSrc || aElement.src);
>          let title = aElement.textContent || aElement.title;
>          return {
>            title: title,
>            uri: url,
>            type: "video/*",

What do you think about making this "audio/*" when need be as well?

::: mobile/android/locales/en-US/chrome/browser.properties:318
(Diff revision 1)
>  # is working while "Paste" / "Add as search engine" triggers the bug. See bug 1262098 for more details.
>  # Manual testing the scenario described in bug 1262098 is highly recommended.
>  contextmenu.addSearchEngine3=Add Search Engine
>  contextmenu.playMedia=Play
>  contextmenu.pauseMedia=Pause
> -contextmenu.shareMedia2=Share Media
> +contextmenu.shareMedia1=Share Media

I'll fix this by just making the other two end with "2" as well.  I don't think 1 endings are ever used anyway.

Comment 9

[Ian Moody [:Kwan] (UTC+1)]

Comment on attachment 8934267 [details]
Bug 1284548 - Put logic classifying <video> element based on content in a function and reuse it. ?

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/205184/diff/1-2/

Comment 10

[Ian Moody [:Kwan] (UTC+1)]

Comment on attachment 8934266 [details]
Bug 1284548 - Use 'Share Audio' label for sharing audio played in <video>. ?

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/205182/diff/1-2/

Comment 11

[Ian Moody [:Kwan] (UTC+1)]

Comment on attachment 8934266 [details]
Bug 1284548 - Use 'Share Audio' label for sharing audio played in <video>. ?

Hey James, I fixed the string numbering issues (mainly by reordering the patches), and also fixed a small pre-existing typo in a moved comment I hadn't noticed before.

Just need you to check the MIME change please:
https://reviewboard.mozilla.org/r/205180/diff/1-2/

(not 100% sure why the r+s got cleared from bugzilla, but reviewboard still has them, so you don't actually need to do change anything)

Comment 12

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8934267 [details]
Bug 1284548 - Put logic classifying <video> element based on content in a function and reuse it. ?

https://reviewboard.mozilla.org/r/205180/#review220334

lgtm

Comment 13

[Ian Moody [:Kwan] (UTC+1)]

Try (sorry about the unnecessary reftests):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c63c068aef9d463c9b9b390bbcf1b7d3090fa44f&filter-resultStatus=success&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=running&filter-resultStatus=pending&filter-resultStatus=runnable

(I know bugzilla doesn't think one patch has review, but reviewboard knows it does, so autoland should be fine)

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0c4dfacc6645
Put logic classifying <video> element based on content in a function and reuse it. r=snorp?
https://hg.mozilla.org/integration/autoland/rev/de83b569b39f
Use 'Share Audio' label for sharing audio played in <video>. r=snorp?

Comment 15

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/0c4dfacc6645
https://hg.mozilla.org/mozilla-central/rev/de83b569b39f

Comment 16

[Oana Horvath [:ohorvath]]

Verified as fixed on Nightly 60.0a1 (2018-02-08).
Device:Huawei Honor 5X (Android 5.1.1)