1437869 - Allow viewing sites in desktop mode

Status: RESOLVED FIXED

(GeckoView :: General, enhancement, P1)

Description

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

I think this should just be a GeckoSessionSetting flag.

Comment 1

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1437869-1.0-Pass-and-handle-session-settings-in-.patch

We should pass the session settings to the content process to allow handling there. This makes the separation clearer and avoids ad-hoc loading of data-URI-based frame scripts.

Comment 2

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1437869-2.0-Add-desktop-mode-session-setting-fla.patch

With patch 1 we provide access to session settings and the onSettingsUpdate callback in content modules. With this we can set the desktop-mode viewport in content while overriding the UA in the parent process.

In a follow up, I would like to expose an UA-overriding API.

Comment 3

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

Comment on attachment 8956993 [details] [diff] [review]
0002-Bug-1437869-2.0-Add-desktop-mode-session-setting-fla.patch

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

::: mobile/android/modules/geckoview/GeckoViewSettings.jsm
@@ +76,5 @@
> +  get desktopUserAgent() {
> +    if (!this._desktopUA) {
> +      this._desktopUA = Cc["@mozilla.org/network/protocol;1?name=http"]
> +                        .getService(Ci.nsIHttpProtocolHandler).userAgent
> +                        .replace(/Android \d.+?; [a-zA-Z]+/, "X11; Linux x86_64")

Wow, this seems fragile. Isn't some builder we can use to create the right string? I see Fennec uses this regex stuff, but maybe we can do something better?

Comment 4

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

Comment on attachment 8956991 [details] [diff] [review]
0001-Bug-1437869-1.0-Pass-and-handle-session-settings-in-.patch

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

::: mobile/android/modules/geckoview/GeckoViewSettings.jsm
@@ +66,5 @@
>  
> +  set useTrackingProtection(aUse) {
> +    if (aUse && !this._isSafeBrowsingInit) {
> +      SafeBrowsing.init();
> +      this._isSafeBrowsingInit = true;

I don't think this prevents multiple GeckoSessions from calling `SafeBrowsing.init()`. Not ideal but probably harmless.

Comment 5

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

Comment on attachment 8956993 [details] [diff] [review]
0002-Bug-1437869-2.0-Add-desktop-mode-session-setting-fla.patch

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

::: mobile/android/modules/geckoview/GeckoViewSettings.jsm
@@ +99,5 @@
> +  set useDesktopMode(aUse) {
> +    if (this.useDesktopMode === aUse) {
> +      return;
> +    }
> +    UserAgentOverrides.addComplexOverride(this.onUserAgentRequest.bind(this));

Drive-by: this will add another override each time desktop mode is turned on right? Also, the last session will replace overrides from previous sessions, so I don't think this works with multiple sessions.

Comment 6

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1437869-2.1-Add-desktop-mode-session-setting-fla.patch

(In reply to Jim Chen [:jchen] [:darchons] from comment #5)
> Comment on attachment 8956993 [details] [diff] [review]
> 0002-Bug-1437869-2.0-Add-desktop-mode-session-setting-fla.patch
> 
> Review of attachment 8956993 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/modules/geckoview/GeckoViewSettings.jsm
> @@ +99,5 @@
> > +  set useDesktopMode(aUse) {
> > +    if (this.useDesktopMode === aUse) {
> > +      return;
> > +    }
> > +    UserAgentOverrides.addComplexOverride(this.onUserAgentRequest.bind(this));
> 
> Drive-by: this will add another override each time desktop mode is turned on
> right? Also, the last session will replace overrides from previous sessions,
> so I don't think this works with multiple sessions.

Great catch, thanks!
For the desktop mode override we can simply override it manually. Eventually we'll need to roll our own UA override manager to support a session-based UA override API.

Comment 7

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

Comment on attachment 8957301 [details] [diff] [review]
0002-Bug-1437869-2.1-Add-desktop-mode-session-setting-fla.patch

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

::: mobile/android/modules/geckoview/GeckoViewSettings.jsm
@@ +74,5 @@
>    }
>  
> +  get desktopUserAgent() {
> +    if (!this._desktopUA) {
> +      this._desktopUA = Cc["@mozilla.org/network/protocol;1?name=http"]

I'd prefer a lazy constant DESKTOP_USER_AGENT using `XPCOMUtils.defineLazyGetter` for this.

@@ +106,5 @@
> +      return;
> +    }
> +    if (aUse) {
> +      Services.obs.addObserver(this.onUserAgentRequest.bind(this),
> +                               "http-on-useragent-request");

I think this can race with UserAgentOverrides.jsm, i.e. the relative order between this module and UserAgentOverrides is not guaranteed, but maybe we don't care about that.

Comment 8

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1437869-2.2-Add-desktop-mode-session-setting-fla.patch

(In reply to Jim Chen [:jchen] [:darchons] from comment #7)
> I'd prefer a lazy constant DESKTOP_USER_AGENT using
> `XPCOMUtils.defineLazyGetter` for this.

Yes, that's much nicer.

> I think this can race with UserAgentOverrides.jsm, i.e. the relative order
> between this module and UserAgentOverrides is not guaranteed, but maybe we
> don't care about that.

I don't think we care about it for GV atm., but that's something we should address when implementing the override API.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c59f42a79b5a
[1.0] Pass and handle session settings in the content process. r=jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2a51018aa5f
[2.2] Add desktop mode session setting flag. r=snorp,jchen

Comment 10

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/c59f42a79b5a
https://hg.mozilla.org/mozilla-central/rev/a2a51018aa5f