917685 - [TABLET] "Request desktop site" does not work on youtube.com

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Adrian Tamas (:AdrianT)]

Firefox Mobile 25 beta 1/ Nightly 26.0a1 2013-09-17
Samsung Galaxy Tab 2 (Android 4.1)

Steps to reproduce:
1) Go to youtube.com
2) Open the Custom Menu and choose "Request desktop site"

Expected results:
The user is redirected to the youtube desktop website

Actual results:
The mobile version is displayed

Notes:
"Request desktop site" works correctly on other websites like espn.com for e.g.
The issue is not reproducible on Firefox Mobile 24 (Google play release)

Comment 1

[u421692]

I am only able to reproduce this issue on tablets.

Comment 2

[u421692]

(In reply to Pop Mihai from comment #1)
> I am only able to reproduce this issue on tablets.

I am only able to reproduce this issue only on tablets, not able to reproduce it on any phone.

Comment 3

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

I think it's due to the override pref added in bug 788422

Comment 4

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

Attachment: Catch error thrown by nsILoadContext.associatedWindow (v1)

Brad has seen errors in the log thrown by nsILoadContext.associatedWindow. I think it's due to background connections that don't have associated windows.

Comment 5

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

Attachment: Let complex overrides take precedence over pref overrides (v1)

The bug is caused by the Fennec "Request Desktop" override having lower precedence than YouTube's pref override. I think it makes sense to make complex overrides have higher precedence.

Comment 6

[Dão Gottwald [::dao]]

Comment on attachment 817239 [details] [diff] [review]
Catch error thrown by nsILoadContext.associatedWindow (v1)

>+    try {
>+      let loadContext = this._getRequestLoadContext(aRequest);
>+      if (loadContext)
>+        return loadContext.associatedWindow;
>+    } catch (e) {
>+      // loadContext.associatedWindow can throw when there's no window
>+    }

If your comment about what you intend to catch here is to be believed, this code should look like this instead:

    let loadContext = this._getRequestLoadContext(aRequest);
    if (loadContext) {
      try {
        return loadContext.associatedWindow;
      } catch (e) {
        // loadContext.associatedWindow can throw when there's no window
      }
    }

Comment 7

[Dão Gottwald [::dao]]

Comment on attachment 817241 [details] [diff] [review]
Let complex overrides take precedence over pref overrides (v1)

>   addComplexOverride: function uao_addComplexOverride(callback) {
>-    gOverrideFunctions.push(callback);
>+    // Add to front of array so complex overrides have precedence
>+    gOverrideFunctions.unshift(callback);
>   },

When adding multiple overrides via addComplexOverride, this reverses the order of those overrides ... which seems unexpected?

Comment 8

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

Comment on attachment 817239 [details] [diff] [review]
Catch error thrown by nsILoadContext.associatedWindow (v1)

Can you use dao's code snippet? That way we don't ignore exceptions that might happen in _getRequestLoadContext

Comment 9

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

Attachment: Catch error thrown by nsILoadContext.associatedWindow (v2)

(In reply to Mark Finkle (:mfinkle) from comment #8)
> Comment on attachment 817239 [details] [diff] [review]
> Catch error thrown by nsILoadContext.associatedWindow (v1)
> 
> Can you use dao's code snippet? That way we don't ignore exceptions that
> might happen in _getRequestLoadContext

Okay done.

Comment 10

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

(In reply to Dão Gottwald [:dao] from comment #7)
> Comment on attachment 817241 [details] [diff] [review]
> Let complex overrides take precedence over pref overrides (v1)
> 
> >   addComplexOverride: function uao_addComplexOverride(callback) {
> >-    gOverrideFunctions.push(callback);
> >+    // Add to front of array so complex overrides have precedence
> >+    gOverrideFunctions.unshift(callback);
> >   },
> 
> When adding multiple overrides via addComplexOverride, this reverses the
> order of those overrides ... which seems unexpected?

I think it can be argued either way.

Personally I think it makes sense for callbacks added later to "override" earlier callbacks. For example, this enables an add-on to register a second override that reverses the effect of a built-in override.

Comment 12

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/0f25523048c3
https://hg.mozilla.org/integration/mozilla-inbound/rev/60185e9edfd4

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0f25523048c3
https://hg.mozilla.org/mozilla-central/rev/60185e9edfd4

Comment 14

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

Comment on attachment 817241 [details] [diff] [review]
Let complex overrides take precedence over pref overrides (v1)

Requesting uplift for Aurora and Beta since this bug is tracking 25.


[Approval Request Comment]

Bug caused by (feature/regressing bug #): possibly bug 788422

User impact if declined: "request desktop site" feature won't work for youtube

Testing completed (on m-c, etc.): locally, m-c

Risk to taking this patch (and alternatives if risky): very small; only youtube is affected

String or IDL/UUID changes made by this patch: none

Comment 15

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

The other patch is only fixing an error message, so I don't think we have to uplift that.

Comment 16

[Adrian Tamas (:AdrianT)]

Samsung Galaxy Tab (Android 4.0.4)
Nightly 27.0a1 2013-10-17

Request desktop site works for youtube.com. Marking as fixed for Nightly and awaiting the uplift for aurora and beta to check.

Comment 17

[Alex Keybl [:akeybl]]

Bugs of this nature need to be on the tracking-Firefox25+ list, not the engineering tracking flag.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a93bf8a2d137
https://hg.mozilla.org/releases/mozilla-aurora/rev/420a47d418ec

https://hg.mozilla.org/releases/mozilla-beta/rev/ef14c7c4eb39
https://hg.mozilla.org/releases/mozilla-beta/rev/26f496f6c216

Comment 19

[u421692]

Verified as fixed on Samsung Galaxy Tab (Android 4.0.4), on Firefox RC 25 build 3.