697857 - User Agent Switcher

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P3)

Description

[Brian Nicholson (:bnicholson)]

The stock ICS browser will have a button that switches between desktop/mobile user agents.  Since so many sites have gimped mobile versions, and since we have a fully capable Gecko engine, we might also want to include this feature.

Comment 1

[Brian Nicholson (:bnicholson)]

Attachment: patch

Initial patch included for basic desktop/mobile switching from menu.

Comment 2

[[:fabrice] Fabrice Desré]

What we really need is per-window UA switching. Setting this application wide doesn't really solve the problem which is that some site behave correctly with a mobile UA and some don't.

Comment 3

[Matt Brubeck (:mbrubeck)]

(In reply to Fabrice Desré [:fabrice] from comment #2)
> What we really need is per-window UA switching. Setting this application
> wide doesn't really solve the problem which is that some site behave
> correctly with a mobile UA and some don't.

I agree that it will be much more useful with per-window or per-origin preferences, but this will at least help those users who want to always see the same content as a desktop browser (especially on tablets).

Comment 4

[Madhava Enros [:madhava]]

The experience we really want here is being able to say "Request Desktop Site" when on a site, and have the browser remember that decision for that site until you undo it.

It sounds like we can't do this unless there are platform changes. Can we get those changes?

If we can only vary the UA for the whole application, how close to that experience can we get?

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

The current plan is to use a "http-modify-request" observer to change the UA header sent with a request. We can use the nsILoadContext to link a reguest to a DOMWindow (to a tab).

Comment 6

[Madhava Enros [:madhava]]

(In reply to Mark Finkle (:mfinkle) from comment #5)
> The current plan is to use a "http-modify-request" observer to change the UA
> header sent with a request. We can use the nsILoadContext to link a reguest
> to a DOMWindow (to a tab).

So, if I understand this correctly, the user flow would be:

1. I encounter a site that's sending me a mobile version when I want a full version
2. I go to the menu and select "Request desktop site"
3. The page reloads and I get the appropriate version
4. I can navigate within that site and still get the desktop version
5. If I switch tabs, I'm not requesting a desktop version on that tab
6. If I switch back to the tab where I was using a desktop version, I will continue to get the desktop version
7. When I move to a new site on this tab, I don't get a desktop version, given that I requested it only for the previous site

Ideally, we'd remember that you'd requested a desktop version for the particular site in question, but if we can't do that for now, a "one time" request that lasts as long as you're moving within that site might do.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: alternate WIP

Let's move away from the global preference and start trying to create the "UA mode". This patch is a WIP for a mobile/desktop UA mode. The patch adds an HTTP request observer when a Tab wants to use "desktop" mode. We do a simple check to see if the request is for the current Tab and if the Tab wants "desktop" mode.

It still needs some more features:
* Call Tab.setAgentMode from Java. Add some Java messages to do this.
* Auto revert the tab from "desktop" back to "mobile" when the base domain of the tab changes. Let's store "baseHost" in the Tab. See:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#7903

Comment 8

[Aaron Train [:aaronmt]]

Attachment: ICS Stock Browser

For what it's worth, this is how the ICS stock browser now does it. Tap the favicon and you're presented with: Mobile [✓] Desktop [ ].

Comment 9

[Aaron Train [:aaronmt]]

(In reply to Aaron Train [:aaronmt] from comment #8)
> Created attachment 574306 [details]
> ICS Stock Browser
> 
> For what it's worth, this is how the ICS stock browser now does it. Tap the
> favicon and you're presented with: Mobile [✓] Desktop [ ].

(Nexus S has a hardware menu button - I'm guessing this layout changes on devices without)

Comment 10

[Doug Turner (:dougt)]

There is a large discussion on the newsgroups about UA strings.  Please review that before we add any UA switching feature.

Comment 11

[Madhava Enros [:madhava]]

To confirm on what the UI would be:

-> an item in the menu reading "Request Desktop Site"

When the user picks it, they get the desktop site. We can put a checkbox next to it on ICS.

Comment 12

[Brian Nicholson (:bnicholson)]

Attachment: patch v2

This patch adds support for UA switching between Desktop and Mobile modes.  The back button can cause the UI and current UA state to get out of sync; this can be fixed by including the user agent in the session history with bug 698094.

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 575581 [details] [diff] [review]
patch v2

>diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java

>+            } else if (event.equals("UserAgent:Changed")) {
>+                Tab.UserAgent userAgent = message.getString("userAgent").equals("mobile") ? Tab.UserAgent.MOBILE : Tab.UserAgent.DESKTOP;
>+                int tabId = message.getInt("tabId");
>+                Tab tab = Tabs.getInstance().getTab(tabId);
>+                tab.setUserAgent(userAgent);
>+                if (tab == Tabs.getInstance().getSelectedTab())
>+                    updateUserAgentButton(userAgent);

Normally we check for a valid "tab" object, but I guess comparing to the selected tab will weed out invalid tabs

>+    void updateUserAgentButton(final Tab.UserAgent userAgent) {

updateUserAgentButton -> updateUserAgentMenu

>+        if (sMenu == null)
>+            return;
>+        mMainHandler.post(new Runnable() {

Add a blank line after the return

>+            public void run() {
>+                int strId = userAgent == Tab.UserAgent.MOBILE ? R.string.agent_request_desktop : R.string.agent_request_mobile;
>+                sMenu.findItem(R.id.user_agent).setTitle(getString(strId));
>+            }
>+        });

From below:
I am concerned about setting the agent mode menu in one runnable and selected the tab in another. I think we need to add an isSelectedTab check to updateUserAgentButton since we can't know for sure that by the time the runnable is called, the tab we thought was selected is still selected.

You'll need to pass in a "tab" and then do this in the runnable:
                  if (Tabs.getInstance().isSelectedTab(tab)) {

>     void handleSelectTab(int tabId) {
>         final Tab tab = Tabs.getInstance().selectTab(tabId);
>         if (tab == null)
>             return;
>-
>+        updateUserAgentButton(tab.getUserAgent());

Leave the blank line

>         mMainHandler.post(new Runnable() { 
>             public void run() {
>                 if (Tabs.getInstance().isSelectedTab(tab)) {

I am concerned about setting the agent mode menu in one runnable and selected the tab in another. I think we need to add an isSelectedTab check to updateUserAgentButton since we can't know for sure that by the time the runnable is called, the tab we thought was selected is still selected.

>diff --git a/mobile/android/base/Tab.java b/mobile/android/base/Tab.java

> public class Tab {
>+    public static enum UserAgent { MOBILE, DESKTOP };

>+    private UserAgent mUserAgent = UserAgent.MOBILE;

>+    public void setUserAgent(UserAgent userAgent) {
>+        mUserAgent = userAgent;
>+    }
>+
>+    public UserAgent getUserAgent() {
>+        return mUserAgent;
>+    }

It's nit-picky, I know, but setUserAgent sounds like it does something different. We are only setting a "mode" not the actual user agent. I was thinking we coudl refer to this concept as "AgentMode". Let's think about this and change all the "UserAgen" occurrences to "AgentMode" (or whatever we decide)


>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>     } else if (aTopic == "Tab:Load") {
>       let uri = URIFixup.createFixupURI(aData, Ci.nsIURIFixup.FIXUP_FLAG_ALLOW_KEYWORD_LOOKUP);
>-      browser.loadURI(uri ? uri.spec : aData);
>+      uri = uri ? uri.spec : aData;
>+      browser.loadURI(uri);

Why this change?

> function Tab(aURL, aParams) {
>   this.browser = null;
>   this.id = 0;
>+  this.agentMode = null;

Let's default to "mobile" and let's make consts for "mobile" and "desktop" in the JS

const UA_MODE_MOBILE = "mobile";
const UA_MODE_DESKTOP = "desktop";

>   create: function(aURL, aParams) {

>     this.browser.addProgressListener(this, flags);
>     this.browser.sessionHistory.addSHistoryListener(this);
>+    Services.obs.addObserver(this, "http-on-modify-request", false);

I worry about listening for the notification all the time. I think my WIP patch had the code setup to only listen when we were in desktop mode

>+  setHostFromURL: function (aURL) {

nit: no space between "function" and "("

>+    if (this.lastHost != host) {
>+      this.lastHost = host;
>+      // TODO: remember mobile/desktop selection for each host

File a bug to add this and reference the bug number here

>+  observe: function(aSubject, aTopic, aData) {

>+    let channel = aSubject.QueryInterface(Ci.nsIHttpChannel);
>+    if (!(channel.loadFlags & Ci.nsIChannel.LOAD_DOCUMENT_URI)) {
>+      return;
>+    }

nit: no {}
nit: blank line after return

>+      if (this.agentMode == "desktop") {
>+        channel.setRequestHeader("User-Agent", "Mozilla/5.0 (X11; Linux x86_64; rv:7.0.1) Gecko/20100101 Firefox/7.0.1", false);
>+      }

nit: no {}

Comment 14

[Brian Nicholson (:bnicholson)]

Attachment: patch v3

Comment 15

[Brian Nicholson (:bnicholson)]

(In reply to Mark Finkle (:mfinkle) from comment #13)
> Comment on attachment 575581 [details] [diff] [review] [diff] [details] [review]
> patch v2
> 
> >diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java
> 
> >+            } else if (event.equals("UserAgent:Changed")) {
> >+                Tab.UserAgent userAgent = message.getString("userAgent").equals("mobile") ? Tab.UserAgent.MOBILE : Tab.UserAgent.DESKTOP;
> >+                int tabId = message.getInt("tabId");
> >+                Tab tab = Tabs.getInstance().getTab(tabId);
> >+                tab.setUserAgent(userAgent);
> >+                if (tab == Tabs.getInstance().getSelectedTab())
> >+                    updateUserAgentButton(userAgent);
> 
> Normally we check for a valid "tab" object, but I guess comparing to the
> selected tab will weed out invalid tabs

Good catch - this could still break the tab.setUserAgent() call if we get back an invalid tab.  I've added a null check.

> 
> >diff --git a/mobile/android/base/Tab.java b/mobile/android/base/Tab.java
> 
> > public class Tab {
> >+    public static enum UserAgent { MOBILE, DESKTOP };
> 
> >+    private UserAgent mUserAgent = UserAgent.MOBILE;
> 
> >+    public void setUserAgent(UserAgent userAgent) {
> >+        mUserAgent = userAgent;
> >+    }
> >+
> >+    public UserAgent getUserAgent() {
> >+        return mUserAgent;
> >+    }
> 
> It's nit-picky, I know, but setUserAgent sounds like it does something
> different. We are only setting a "mode" not the actual user agent. I was
> thinking we coudl refer to this concept as "AgentMode". Let's think about
> this and change all the "UserAgen" occurrences to "AgentMode" (or whatever
> we decide)
> 

Sounds like a good argument to me.  I've changed all instances of UserAgent to AgentMode.

> 
> >   create: function(aURL, aParams) {
> 
> >     this.browser.addProgressListener(this, flags);
> >     this.browser.sessionHistory.addSHistoryListener(this);
> >+    Services.obs.addObserver(this, "http-on-modify-request", false);
> 
> I worry about listening for the notification all the time. I think my WIP
> patch had the code setup to only listen when we were in desktop mode

I changed this in anticipation of bug 705840.  Once we save the user agent preferences for each host, we'll need to check if we're navigating to a new host (regardless of whether we're currently on desktop mode).

> 
> >+    if (this.lastHost != host) {
> >+      this.lastHost = host;
> >+      // TODO: remember mobile/desktop selection for each host
> 
> File a bug to add this and reference the bug number here
> 

Filed bug 705840.

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 577340 [details] [diff] [review]
patch v3

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>+  observe: function(aSubject, aTopic, aData) {

>+      if (this.agentMode == UA_MODE_DESKTOP)
>+        channel.setRequestHeader("User-Agent", "Mozilla/5.0 (X11; Linux x86_64; rv:7.0.1) Gecko/20100101 Firefox/7.0.1", false);

Let's file a bug to dynamically generate the "desktop" UA from the current "mobile" UA so we don't need to keep bumping the Firefox version and Gecko date stamp

We'll need to watch the affect of creating a http listener for each tab. Maybe we could get by with a single global listener somehow. Anyway, let's keep an eye on performance.

Comment 17

[Brian Nicholson (:bnicholson)]

Landed http://hg.mozilla.org/projects/birch/rev/7df6863adc7e

Filed bug 706165 for dynamic UA string.
Also filed bug 706181 for fixing the agent mode in the session history.

Comment 18

[Aaron Train [:aaronmt]]

Samsung Nexus S (Android 4.0.1)
20111130040240
http://hg.mozilla.org/projects/birch/rev/4e745f151abd

Comment 19

[Andreea Pod]

I can not see "Request desktop site" option in the menu anymore, was it removed? Should I still create a testcase for this?

Comment 20

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Andreea Pod from comment #19)
> I can not see "Request desktop site" option in the menu anymore, was it
> removed? Should I still create a testcase for this?

It was removed in bug 709888

Comment 21

[Aaron Train [:aaronmt]]

(In reply to Andreea Pod from comment #19)
> I can not see "Request desktop site" option in the menu anymore, was it
> removed? Should I still create a testcase for this?

No.

Comment 22

[Camelia Urian]

Removing flags since this feature was removed on bug 709888