Closed Bug 1466640 Opened 2 years ago Closed 2 years ago

When setting USE_DESKTOP_MODE to true/false page will always use desktop mode once set

Categories

(GeckoView :: General, enhancement)

enhancement
Not set

Tracking

(firefox62 fixed)

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: ekager, Assigned: esawin)

Details

Attachments

(2 files, 2 obsolete files)

I’m using `geckoSession.getSettings().setBoolean(GeckoSessionSettings.USE_DESKTOP_MODE, true/false)` to set desktop mode, but it doesn’t seem to play well with toggling true/false. Once I request desktop mode and set this to true, you can't get back to the mobile site by setting it to false. I have to create a new GV by opening a new tab or trashing the Klar session and then you can see mobile sites again.
We never remove the desktop mode observer because bind returns a new function object.
Assignee: nobody → esawin
Attachment #8983181 - Flags: review?(nchen)
Comment on attachment 8983181 [details] [diff] [review]
0001-Bug-1466640-1.0-Keep-desktop-mode-observer-function-.patch

Review of attachment 8983181 [details] [diff] [review]:
-----------------------------------------------------------------

Please write a test while you're here!

You can look at 'window.navigator.userAgent' via RDP.
Does this test look sane to you? It fails in the desktop mode case.
Attachment #8983218 - Flags: review?(snorp)
Comment on attachment 8983181 [details] [diff] [review]
0001-Bug-1466640-1.0-Keep-desktop-mode-observer-function-.patch

Review of attachment 8983181 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/modules/geckoview/GeckoViewSettings.jsm
@@ +101,2 @@
>                                    "http-on-useragent-request");
> +      delete this._userAgentObserver;

I think `delete` is not recommended for performance reasons. Use `this._userAgentObserver = null` or `this._userAgentObserver = undefined` instead.
Attachment #8983181 - Flags: review?(nchen) → review+
Comment on attachment 8983218 [details] [diff] [review]
0002-Bug-1466640-2.0-Add-desktop-mode-test.-r-jchen.patch

Review of attachment 8983218 [details] [diff] [review]:
-----------------------------------------------------------------

Test looks good to me. It fails even with your fix? I guess either the fix or test is broken then, but the test seems fine.
Attachment #8983218 - Flags: review?(snorp) → review+
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #5)
> Test looks good to me. It fails even with your fix? I guess either the fix
> or test is broken then, but the test seems fine.

The fix seems fine based on local testing. I'm going to land the fix and try to figure out what's wrong with the test in the meanwhile.
Addressed comments.
Attachment #8983181 - Attachment is obsolete: true
Attachment #8983775 - Flags: review+
Switched test URL to "https://example.com" to make sure request headers are actually set.
Attachment #8983218 - Attachment is obsolete: true
Attachment #8983776 - Flags: review+
Pushed by esawin@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/39b29d5990d3
[1.1] Keep desktop mode observer function around for its removal. r=jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4ed04cef602
[2.1] Add desktop mode test. r=snorp
https://hg.mozilla.org/mozilla-central/rev/39b29d5990d3
https://hg.mozilla.org/mozilla-central/rev/b4ed04cef602
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 62 → mozilla62
You need to log in before you can comment on or make changes to this bug.