Closed
Bug 917685
Opened 8 years ago
Closed 8 years ago
[TABLET] "Request desktop site" does not work on youtube.com
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox24 unaffected, firefox25+ verified, firefox26+ fixed, firefox27 verified, fennec25+)
VERIFIED
FIXED
Firefox 27
People
(Reporter: AdrianT, Assigned: jchen)
References
Details
(Keywords: regression, reproducible)
Attachments
(2 files, 1 obsolete file)
957 bytes,
patch
|
dao
:
review+
akeybl
:
approval-mozilla-aurora+
akeybl
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
1.07 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
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
(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.
Assignee | ||
Comment 3•8 years ago
|
||
I think it's due to the override pref added in bug 788422
Blocks: 788422
Updated•8 years ago
|
Assignee: nobody → nchen
tracking-fennec: ? → 25+
Updated•8 years ago
|
Status: NEW → ASSIGNED
status-firefox27:
--- → affected
Keywords: regression,
reproducible
Whiteboard: regression
Assignee | ||
Comment 4•8 years ago
|
||
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)
Assignee | ||
Comment 5•8 years ago
|
||
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 6•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
||
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
Assignee | ||
Comment 9•8 years ago
|
||
(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)
Assignee | ||
Comment 10•8 years ago
|
||
(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.
Updated•8 years ago
|
Attachment #817241 -
Flags: review?(dao) → review+
Updated•8 years ago
|
Attachment #817343 -
Flags: review?(mark.finkle) → review+
Assignee | ||
Comment 12•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f25523048c3 https://hg.mozilla.org/integration/mozilla-inbound/rev/60185e9edfd4
Comment 13•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0f25523048c3 https://hg.mozilla.org/mozilla-central/rev/60185e9edfd4
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
Assignee | ||
Comment 14•8 years ago
|
||
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?
Assignee | ||
Comment 15•8 years ago
|
||
The other patch is only fixing an error message, so I don't think we have to uplift that.
Reporter | ||
Comment 16•8 years ago
|
||
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)
Comment 17•8 years ago
|
||
Bugs of this nature need to be on the tracking-Firefox25+ list, not the engineering tracking flag.
Updated•8 years ago
|
Attachment #817241 -
Flags: approval-mozilla-beta?
Attachment #817241 -
Flags: approval-mozilla-beta+
Attachment #817241 -
Flags: approval-mozilla-aurora?
Attachment #817241 -
Flags: approval-mozilla-aurora+
Comment 18•8 years ago
|
||
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•8 years ago
|
||
Verified as fixed on Samsung Galaxy Tab (Android 4.0.4), on Firefox RC 25 build 3.
Updated•3 months ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•