Closed Bug 917685 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox for Android :: General, defect)

ARM
All
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 27
Tracking Status
firefox24 --- unaffected
firefox25 + verified
firefox26 + fixed
firefox27 --- verified
fennec 25+ ---

People

(Reporter: AdrianT, Assigned: jchen)

References

Details

(Keywords: regression, reproducible)

Attachments

(2 files, 1 obsolete file)

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)
Summary: "Request desktop site" does not work on youtube.com → [TABLET] "Request desktop site" does not work on youtube.com
I am only able to reproduce this issue on tablets.
(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.
I think it's due to the override pref added in bug 788422
Blocks: 788422
Assignee: nobody → nchen
tracking-fennec: ? → 25+
Status: NEW → ASSIGNED
Whiteboard: regression
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.
Attachment #817239 - Flags: review?(mark.finkle)
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.
Attachment #817241 - Flags: review?(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 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 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
(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.
Attachment #817239 - Attachment is obsolete: true
Attachment #817239 - Flags: review?(mark.finkle)
Attachment #817343 - Flags: review?(mark.finkle)
(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.
Attachment #817241 - Flags: review?(dao) → review+
Attachment #817343 - Flags: review?(mark.finkle) → review+
Duplicate of this bug: 927167
https://hg.mozilla.org/mozilla-central/rev/0f25523048c3
https://hg.mozilla.org/mozilla-central/rev/60185e9edfd4
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
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
Attachment #817241 - Flags: approval-mozilla-beta?
Attachment #817241 - Flags: approval-mozilla-aurora?
The other patch is only fixing an error message, so I don't think we have to uplift that.
Flags: needinfo?(adrian.tamas)
Keywords: verifyme
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.
Flags: needinfo?(adrian.tamas)
Keywords: verifyme
Bugs of this nature need to be on the tracking-Firefox25+ list, not the engineering tracking flag.
Keywords: verifyme
Attachment #817241 - Flags: approval-mozilla-beta?
Attachment #817241 - Flags: approval-mozilla-beta+
Attachment #817241 - Flags: approval-mozilla-aurora?
Attachment #817241 - Flags: approval-mozilla-aurora+
Verified as fixed on Samsung Galaxy Tab (Android 4.0.4), on Firefox RC 25 build 3.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.