Closed Bug 917685 Opened 12 years ago Closed 12 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
All
defect
Not set
normal

Tracking

(firefox24 unaffected, firefox25+ verified, firefox26+ fixed, firefox27 verified, fennec25+)

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+
Status: ASSIGNED → RESOLVED
Closed: 12 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
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: