788422 - Use UserAgentOverrides.jsm as designed to fix YouTube on tablets

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Dão Gottwald [:dao]]

Attachment: patch

disclaimer: my tablet is broken, so I can't test this easily

Comment 1

[Matt Brubeck (:mbrubeck)]

Comment on attachment 658404 [details] [diff] [review]
patch

Passing review to Brian.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 658404 [details] [diff] [review]
patch

The patch looks OK to me, but I am wondering if there is enough value to need this patch. UserAgentOverrides is a handy module, but since we already have an "http-on-modify-request" code path and we want to remove this override as soon as possible, this might be too much overhead to be worthwhile.

Comment 3

[Dão Gottwald [:dao]]

This shouldn't add more overhead than is implied by the code I'm removing. For instance, UserAgentOverrides doesn't use a regular expression for matching the host and it doesn't construct the UA string on the fly.

If you don't need it anymore, you can just comment out UserAgentOverrides.init().

Comment 4

[Brian Nicholson (:bnicholson)]

(In reply to Dão Gottwald [:dao] from comment #3)
> This shouldn't add more overhead than is implied by the code I'm removing.
> For instance, UserAgentOverrides doesn't use a regular expression for
> matching the host and it doesn't construct the UA string on the fly.
> 
The UserAgentOverrides would be more efficient in the UA replacement since it doesn't create the UA on-the-fly like you said, but this only matters when visiting youtube.com.

This patch creates an additional "http-on-modify-request" observer. Considering the "http-on-modify-request" event is fired for every HTTP resource loaded, it might best to minimize these observers.

Comment 5

[Dão Gottwald [:dao]]

Can you elaborate on why the mere existence of additional http-on-modify-request observers (disregarding what they do) would add meaningful overhead?

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Dão Gottwald [:dao] from comment #5)
> Can you elaborate on why the mere existence of additional
> http-on-modify-request observers (disregarding what they do) would add
> meaningful overhead?

I think Brian is referring to the small hit we would take calling from C++ to JS for the "observe" call. It crazy, I know, but it is actually measurable on many mobile devices. In fact, we vetoed using a progressbar for page loads for the same reason.

It might seem heavy handed, but I think we should pass on this patch. The existing "http-on-modify-request" is good enough for this short term hack. Thanks for taking the time to make a patch and clue us into UserAgentOverrides.

Comment 7

[Cristian Nicolae (:xti)]

Due to comment #6, I will close this bug as verified wontfix.

Comment 8

[Dão Gottwald [:dao]]

So now you actually load UserAgentOverrides.jsm, but you use custom code rather than a pref to fix youtube, so you don't benefit from bug 896382.

Comment 9

[Wesley Johnston (:wesj)]

I think this patch would have broken our SiteSpecificAgent fix that ensures navigator.UserAgent matches what we send with http requests:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/SiteSpecificUserAgent.js#23

Comment 10

[Dão Gottwald [:dao]]

Attachment: patch

Comment 11

[Dão Gottwald [:dao]]

Attachment: patch

the previous patch contained unrelated changes...

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 781572 [details] [diff] [review]
patch

>--- a/mobile/android/components/SiteSpecificUserAgent.js
>+++ b/mobile/android/components/SiteSpecificUserAgent.js
>@@ -3,30 +3,30 @@
>  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> const Cu = Components.utils;
> const Cc = Components.classes;
> const Ci = Components.interfaces;
> 
> Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> Cu.import("resource://gre/modules/UserAgentOverrides.jsm");
>-Cu.import("resource://gre/modules/Services.jsm");
> 
> const DEFAULT_UA = Cc["@mozilla.org/network/protocol;1?name=http"]
>                      .getService(Ci.nsIHttpProtocolHandler)
>                      .userAgent;
> 
> function SiteSpecificUserAgent() {}
> 
> SiteSpecificUserAgent.prototype = {
>   getUserAgentForURIAndWindow: function ssua_getUserAgentForURIAndWindow(aURI, aWindow) {
>+    let UA;
>     let win = Cc["@mozilla.org/appshell/window-mediator;1"].getService(Ci.nsIWindowMediator).getMostRecentWindow("navigator:browser");

Hm, I removed the unused Services.jsm while I was at it, but I guess I could just start using it while I'm at it.

Comment 13

[Dão Gottwald [:dao]]

Attachment: patch

with the change from comment 12

Comment 14

[Dão Gottwald [:dao]]

(In reply to Wesley Johnston (:wesj) from comment #9)
> I think this patch would have broken our SiteSpecificAgent fix that ensures
> navigator.UserAgent matches what we send with http requests:
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/
> SiteSpecificUserAgent.js#23

That's older than my original patch, so my patch wouldn't have broken your SiteSpecificAgent fix but the other way around. ;) I'm taking care of it in my current patch.

Comment 15

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #14)
> (In reply to Wesley Johnston (:wesj) from comment #9)
> > I think this patch would have broken our SiteSpecificAgent fix that ensures
> > navigator.UserAgent matches what we send with http requests:
> > 
> > http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/
> > SiteSpecificUserAgent.js#23
> 
> That's older than my original patch,

I meant: my original patch is older.

Anyway, who wants to review this?

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 781574 [details] [diff] [review]
patch

LGTM, but let's make sure the bug 858828, and those it blocks, are not regressed.

Brian can do the review.

Comment 17

[Dão Gottwald [:dao]]

Comment on attachment 781574 [details] [diff] [review]
patch

>--- a/netwerk/protocol/http/UserAgentOverrides.jsm
>+++ b/netwerk/protocol/http/UserAgentOverrides.jsm
>@@ -111,17 +111,19 @@ function buildOverrides() {
>       if (search && replace) {
>         userAgent = DEFAULT_UA.replace(new RegExp(search, "g"), replace);
>       } else {
>         userAgent = override;
>       }
>       builtUAs.set(override, userAgent);
>     }
> 
>-    gOverrides.set(domain, userAgent);
>+    if (userAgent != DEFAULT_UA) {
>+      gOverrides.set(domain, userAgent);
>+    }
>   }
> }
> 
> function HTTP_on_modify_request(aSubject, aTopic, aData) {
>   let channel = aSubject.QueryInterface(Ci.nsIHttpChannel);
> 
>   for (let callback of gOverrideFunctions) {
>     let modifiedUA = callback(channel, DEFAULT_UA);

If this part of the patch doesn't apply, that's because it builds upon bug 891968, which hasn't been merged to mozilla-central yet.

Comment 18

[Brian Nicholson (:bnicholson)]

Comment on attachment 781574 [details] [diff] [review]
patch

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

Looks good to me with comment addressed.

::: mobile/android/app/mobile.js
@@ +777,5 @@
>  // Enable hardware-accelerated Skia canvas
>  pref("gfx.canvas.azure.backends", "skia");
>  pref("gfx.canvas.azure.accelerated", true);
> +
> +pref("general.useragent.override.youtube.com", "Android; Tablet;#Android; Mobile; Tablet;");

Shouldn't this be "Android; Tablet;#Android; Mobile;" and not "Android; Tablet;#Android; Mobile; Tablet;" ?

Comment 19

[Dão Gottwald [:dao]]

(In reply to Brian Nicholson (:bnicholson) from comment #18)
> Comment on attachment 781574 [details] [diff] [review]
> patch
> 
> Review of attachment 781574 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me with comment addressed.
> 
> ::: mobile/android/app/mobile.js
> @@ +777,5 @@
> >  // Enable hardware-accelerated Skia canvas
> >  pref("gfx.canvas.azure.backends", "skia");
> >  pref("gfx.canvas.azure.accelerated", true);
> > +
> > +pref("general.useragent.override.youtube.com", "Android; Tablet;#Android; Mobile; Tablet;");
> 
> Shouldn't this be "Android; Tablet;#Android; Mobile;" and not "Android;
> Tablet;#Android; Mobile; Tablet;" ?

That's how the override worked at the time I filed the bug, but apparently it changed in the meantime. Do you want Tablet in the resulting UA or not?

Comment 20

[Brian Nicholson (:bnicholson)]

If the current UA is using "Android; Mobile; Tablet;", that seems like a bug. After talking to Mark and Wes, they agree that we want either "Android; Mobile;" or "Android; Tablet;" -- not both.

Comment 21

[Brian Nicholson (:bnicholson)]

(In reply to Dão Gottwald [:dao] from comment #19)
> Do you want Tablet in the resulting UA or not?

In short, no.

Comment 22

[Dão Gottwald [:dao]]

(In reply to Brian Nicholson (:bnicholson) from comment #21)
> (In reply to Dão Gottwald [:dao] from comment #19)
> > Do you want Tablet in the resulting UA or not?
> 
> In short, no.

I removed it.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d12b8094728e

Comment 23

[Ryan VanderMeulen [:RyanVM]]

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

Comment 24

[Mike Taylor [:miketaylr]]

Hey Dão (or anyone else), do you remember what the purpose of this override was? 

Over in https://bugzilla.mozilla.org/show_bug.cgi?id=1175301#c3 I'm either going to update it or remove it, but I'm not really sure what it was doing. 

AFAICT, the Tablet and Mobile UAs get the same site, at least on Fennec.

Comment 25

[Kevin Brosnan [Ex-Mozilla]]

If I recall correctly. YouTube gave Firefox with Tablet UA the desktop site for the main page. Though individual videos received a mobile interface that would blow up with a redirect loop on video play see bug 786623. You should confirm that this is no longer the case with the UA hack removed.

Comment 26

[Dão Gottwald [:dao]]

(In reply to Mike Taylor [:miketaylr] from comment #24)
> Hey Dão (or anyone else), do you remember what the purpose of this override
> was?

I could only speculate. The override was refactored in this bug, but originally it was added elsewhere.

Comment 27

[Mike Taylor [:miketaylr]]

Ah, OK. Thanks Keven and Dão. I'll test the redirect loop on my Nexus before changing anything.