1175301 - Add a UA override for Youtube until Bug 1174784 is fixed

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mike Taylor [:miketaylr]]

Before I try to land Bug 1169772, the biggest (known) regression is Youtube video breaks. ;_;

This bug adds an override to hide the Android version number until Google can fix it. Bug 1174784 tracks the Evangelism part--Google is aware but it's unknown how long it will take for them to fix.

Comment 1

[Mike Taylor [:miketaylr]]

Attachment: 1175301-Add-UA-override-for-Youtube-until-Bug-11.patch

Hi Matt, would you mind reviewing this? 

I've tested locally and it does what I expect it to (that is, Youtube videos can play rather than show a "Video can't be played because the file is corrupt message").

Comment 2

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8623342 [details] [diff] [review]
1175301-Add-UA-override-for-Youtube-until-Bug-11.patch

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

::: mobile/android/app/ua-update.json.in
@@ +4,5 @@
>  {
>    // bug 788422, youtube.com
> +  "youtube.com": "Android; Tablet;#Android; Mobile;",
> +  // bug 1174784, youtube.com
> +  "youtube.com": "Android\\s\\d.+?;#Android;",

This regex looks incorrect to me.  It removes only the first two characters of the version number.  Should it be "\\d\\.\\d+" instead?

> "Android 5.1; Mobile".replace(new RegExp("Android\\s\\d.+?", "g"), "Android")

"Android1; Mobile"

Comment 3

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8623342 [details] [diff] [review]
1175301-Add-UA-override-for-Youtube-until-Bug-11.patch

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

::: mobile/android/app/ua-update.json.in
@@ +2,5 @@
>  // Everything after the first // on a line will be removed by the preproccesor.
>  // Send these sites a custom user-agent. Bugs should be included with an entry.
>  {
>    // bug 788422, youtube.com
> +  "youtube.com": "Android; Tablet;#Android; Mobile;",

This override should either be removed or updated, since it no longer matches the default UA.

Comment 4

[Mike Taylor [:miketaylr]]

(In reply to Matt Brubeck (:mbrubeck) from comment #2) 
> This regex looks incorrect to me.  It removes only the first two characters
> of the version number.  Should it be "\\d\\.\\d+" instead?
> 
> > "Android 5.1; Mobile".replace(new RegExp("Android\\s\\d.+?", "g"), "Android")
> 
> "Android1; Mobile"

I think you might have missed the ";" after the non-greedy ".+?", which is saying grab everything until the first ";".

> "Android 5.1; Mobile".replace(new RegExp("Android\\s\\d.+?;", "g"), "Android;")

"Android; Mobile"

> "Android 4.4.4; Mobile".replace(new RegExp("Android\\s\\d.+?;", "g"), "Android;")

"Android; Mobile"


(In reply to Matt Brubeck (:mbrubeck) from comment #3)
> >  {
> >    // bug 788422, youtube.com
> > +  "youtube.com": "Android; Tablet;#Android; Mobile;",
> 
> This override should either be removed or updated, since it no longer
> matches the default UA.

Yeah, good call. Let me see if the problem that this original override existed for still reproduces and I'll either update or remove it.

Comment 5

[Matt Brubeck (:mbrubeck)]

(In reply to Mike Taylor [:miketaylr] from comment #4)
> I think you might have missed the ";" after the non-greedy ".+?", which is
> saying grab everything until the first ";".

Ah yes, you are correct.

Comment 6

[(inactive) Jim Chen [:jchen] [:darchons]]

Drive-by: IIRC we only support one override per domain (and I don't think it's valid JSON to have two of the same property?) So if the old bug still exists, you'll need to combine the two overrides by playing around with the regex (e.g. using capturing groups) or using a fixed UA string instead of a regex.

Comment 7

[Karl Dubost💡 :karlcow]

To fulfill Jim Chen prophecy and regex geekiness (points|insanity)

> > "Android 4.4.4; Mobile".replace(new RegExp("Android\\s?[\\d\\.]*;\\s(Mobile|Tablet)", "g"), "Android; Mobile;")
> < "Android; Mobile;"
> > "Android 4.4.4; Tablet".replace(new RegExp("Android\\s?[\\d\\.]*;\\s(Mobile|Tablet)", "g"), "Android; Mobile;")
> < "Android; Mobile;"
> > "Android; Tablet".replace(new RegExp("Android\\s?[\\d\\.]*;\\s(Mobile|Tablet)", "g"), "Android; Mobile;")
> < "Android; Mobile;"
> > "Android 5.1; Mobile".replace(new RegExp("Android\\s?[\\d\\.]*;\\s(Mobile|Tablet)", "g"), "Android; Mobile;")
> < "Android; Mobile;"

Comment 8

[Mike Taylor [:miketaylr]]

Thanks Jim and Karl. Luckily that old issue is gone, so let's keep it simple(r).

Comment 9

[Mike Taylor [:miketaylr]]

Attachment: 1175301-Update-UA-override-for-Youtube.patch

OK, I've confirmed that the behavior from Bug 786623 no longer reproduces (without the old override), so let's just remove it.

Comment 10

[Mike Taylor [:miketaylr]]

Attachment: 1175301-Update-UA-override-for-Youtube.-r-mbrube.patch

Thanks Matt. Updating commit message to reflect r+.

Comment 11

[Mike Taylor [:miketaylr]]

Sheriffs, no Try run--just updating ua-update.json.in file here and have tested locally that it builds + works as expected.

Comment 12

[Mike Taylor [:miketaylr]]

Attachment: 1175301-Update-UA-override-for-Youtube-2.patch

Actually, that one would have exploded with the trailing comma (post-build). JSON doesn't like that.

Comment 13

[Pulsebot]

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

Comment 14

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

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

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/df312e4cadef