Closed Bug 1437869 Opened 2 years ago Closed 2 years ago

Allow viewing sites in desktop mode

Categories

(GeckoView :: General, enhancement, P1)

enhancement

Tracking

(firefox58 wontfix, firefox59 wontfix, firefox60 fixed)

RESOLVED FIXED
mozilla60
Tracking Status
firefox58 --- wontfix
firefox59 --- wontfix
firefox60 --- fixed

People

(Reporter: snorp, Assigned: esawin)

Details

(Whiteboard: [geckoview:klar])

Attachments

(2 files, 2 obsolete files)

I think this should just be a GeckoSessionSetting flag.
Assignee: nobody → esawin
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.
Attachment #8956991 - Flags: review?(nchen)
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.
Attachment #8956993 - Flags: review?(snorp)
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?
Attachment #8956993 - Flags: review?(snorp) → review+
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.
Attachment #8956991 - Flags: review?(nchen) → review+
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.
Attachment #8956993 - Flags: feedback+
(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.
Attachment #8956993 - Attachment is obsolete: true
Attachment #8957301 - Flags: review?(nchen)
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.
Attachment #8957301 - Flags: review?(nchen) → review+
(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.
Attachment #8957301 - Attachment is obsolete: true
Attachment #8957594 - Flags: review+
Pushed by esawin@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/c59f42a79b5a
https://hg.mozilla.org/mozilla-central/rev/a2a51018aa5f
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 60 → mozilla60
You need to log in before you can comment on or make changes to this bug.