Add a UA override for Youtube until Bug 1174784 is fixed

RESOLVED FIXED in Firefox 41

Status

()

Firefox for Android
General
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: miketaylr, Assigned: miketaylr)

Tracking

unspecified
Firefox 41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(p11+, firefox41 fixed, fennec41+)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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.
Assignee: nobody → miket
Summary: UA override for Youtube → Add a UA override for Youtube until Bug 1174784 is fixed
Blocks: 1175305
tracking-fennec: --- → ?
Created attachment 8623342 [details] [diff] [review]
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 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"
Attachment #8623342 - Flags: review?(mbrubeck) → review-
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.
(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.
(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.

Updated

3 years ago
Depends on: 1174784
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

3 years ago
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;"
Thanks Jim and Karl. Luckily that old issue is gone, so let's keep it simple(r).
Created attachment 8623734 [details] [diff] [review]
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.
Attachment #8623342 - Attachment is obsolete: true
Attachment #8623734 - Flags: review?(mbrubeck) → review+
Created attachment 8623787 [details] [diff] [review]
1175301-Update-UA-override-for-Youtube.-r-mbrube.patch

Thanks Matt. Updating commit message to reflect r+.
Attachment #8623734 - Attachment is obsolete: true
Sheriffs, no Try run--just updating ua-update.json.in file here and have tested locally that it builds + works as expected.
Keywords: checkin-needed
Created attachment 8623791 [details] [diff] [review]
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.
Attachment #8623787 - Attachment is obsolete: true

Comment 13

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/df312e4cadef
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/df312e4cadef
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/df312e4cadef
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 41
tracking-fennec: ? → 41+
tracking-p11: --- → +
You need to log in before you can comment on or make changes to this bug.