Use UserAgentOverrides.jsm as designed to fix YouTube on tablets

RESOLVED FIXED in Firefox 25

Status

()

defect
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: dao, Assigned: dao)

Tracking

({perf})

Trunk
Firefox 25
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Posted patch patch (obsolete) — Splinter Review
disclaimer: my tablet is broken, so I can't test this easily
Attachment #658404 - Flags: review?(mark.finkle)
Attachment #658404 - Flags: review?(mbrubeck)
Comment on attachment 658404 [details] [diff] [review]
patch

Passing review to Brian.
Attachment #658404 - Flags: review?(mbrubeck) → review?(bnicholson)
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.
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().
(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.
Can you elaborate on why the mere existence of additional http-on-modify-request observers (disregarding what they do) would add meaningful overhead?
(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.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
Assignee: dao → nobody
Due to comment #6, I will close this bug as verified wontfix.
Status: RESOLVED → VERIFIED
Attachment #658404 - Flags: review?(mark.finkle)
Attachment #658404 - Flags: review?(bnicholson)
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.
Assignee: nobody → dao
Status: VERIFIED → REOPENED
Resolution: WONTFIX → ---
Summary: Use UserAgentOverrides.jsm to fix YouTube on tablets → Use UserAgentOverrides.jsm as designed to fix YouTube on tablets
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
Depends on: 896382
Keywords: perf
Posted patch patch (obsolete) — Splinter Review
Attachment #658404 - Attachment is obsolete: true
Posted patch patch (obsolete) — Splinter Review
the previous patch contained unrelated changes...
Attachment #781568 - Attachment is obsolete: true
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.
Posted patch patchSplinter Review
with the change from comment 12
Attachment #781572 - Attachment is obsolete: true
(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.
(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 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.
Attachment #781574 - Flags: review?(bnicholson)
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 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;" ?
Attachment #781574 - Flags: review?(bnicholson) → review+
(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?
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.
(In reply to Dão Gottwald [:dao] from comment #19)
> Do you want Tablet in the resulting UA or not?

In short, no.
(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
https://hg.mozilla.org/mozilla-central/rev/d12b8094728e
Status: REOPENED → RESOLVED
Closed: 7 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
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.
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.
(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.
Flags: needinfo?(dao)
Ah, OK. Thanks Keven and Dão. I'll test the redirect loop on my Nexus before changing anything.
You need to log in before you can comment on or make changes to this bug.