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

Status: RESOLVED FIXED

(GeckoView :: General, enhancement)

Description

[Emily Kager [:ekager]]

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.

Comment 1

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1466640-1.0-Keep-desktop-mode-observer-function-.patch

We never remove the desktop mode observer because bind returns a new function object.

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

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.

Comment 3

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1466640-2.0-Add-desktop-mode-test.-r-jchen.patch

Does this test look sane to you? It fails in the desktop mode case.

Comment 4

[(inactive) Jim Chen [:jchen] [:darchons]]

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.

Comment 5

[James Willcox (:snorp) ([e-mail]) (he/him)]

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.

Comment 6

[Eugen Sawin [:esawin]]

(In reply to James Willcox (:snorp) ([e-mail]) 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.

Comment 7

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1466640-1.1-Keep-desktop-mode-observer-function-.patch

Addressed comments.

Comment 8

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1466640-2.1-Add-desktop-mode-test.-r-snorp.patch

Switched test URL to "https://example.com" to make sure request headers are actually set.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
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

Comment 10

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/39b29d5990d3
https://hg.mozilla.org/mozilla-central/rev/b4ed04cef602