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)
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•12 years ago
|
||
I think it's due to the override pref added in bug 788422
Blocks: 788422
Updated•12 years ago
|
Assignee: nobody → nchen
tracking-fennec: ? → 25+
Updated•12 years ago
|
Status: NEW → ASSIGNED
status-firefox27:
--- → affected
Keywords: regression,
reproducible
Whiteboard: regression
Assignee | ||
Comment 4•12 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•12 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•12 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•12 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•12 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•12 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•12 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•12 years ago
|
Attachment #817241 -
Flags: review?(dao) → review+
Updated•12 years ago
|
Attachment #817343 -
Flags: review?(mark.finkle) → review+
Assignee | ||
Comment 12•12 years ago
|
||
Comment 13•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0f25523048c3
https://hg.mozilla.org/mozilla-central/rev/60185e9edfd4
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
Assignee | ||
Comment 14•12 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•12 years ago
|
||
The other patch is only fixing an error message, so I don't think we have to uplift that.
Reporter | ||
Comment 16•12 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•12 years ago
|
||
Bugs of this nature need to be on the tracking-Firefox25+ list, not the engineering tracking flag.
Updated•12 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•12 years ago
|
||
Comment 19•12 years ago
|
||
Verified as fixed on Samsung Galaxy Tab (Android 4.0.4), on Firefox RC 25 build 3.
Updated•5 years 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
•