Closed
Bug 766406
Opened 12 years ago
Closed 12 years ago
Implement "Request Desktop Site"
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox15 verified, firefox16 verified)
VERIFIED
FIXED
Firefox 16
People
(Reporter: bnicholson, Assigned: bnicholson)
References
Details
Attachments
(3 files, 4 obsolete files)
10.04 KB,
patch
|
mfinkle
:
review-
|
Details | Diff | Splinter Review |
20.40 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
33.28 KB,
patch
|
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
We previously had the ability to switch to a desktop site (bug 697857), but removed it mainly because of redirection issues (bug 706264). Here's the problem: 1) A user goes to a site, say http://www.yahoo.com 2) The user is automatically redirected to a mobile version of the site, http://m.yahoo.com 3) The user clicks Request Desktop Site 4) The UA is changed and the site reloads, but the user is still at http://m.yahoo.com Switching to desktop mode was technically working, but it left users looking at a mobile page with a desktop UA. This made it appear as if UA switching wasn't working at all. To worsen this problem, we wanted site-specific UA settings. Sites were partitioned by host (m.yahoo.com or www.yahoo.com). So after landing on http://m.yahoo.com, if the user tries going to http://www.yahoo.com, the UA mode is reset to mobile since the host changed. Chrome implements this feature, but the UA setting is tab-specific, so the page does at least update if the user manually re-navigates to the page. Unfortunately, there aren't any great solutions for translating a mobile URL to its desktop equivalent. If we kept track of past redirects (a solution proposed in https://bugzilla.mozilla.org/show_bug.cgi?id=706264#c1), the pre-redirected URL becomes stale as the user clicks links to navigate the site. Rather than reloading the current URL with the desktop UA, we could just go directly to that site's root URL, removing the path. This will take the user off the page they were looking at, but if they want to see the page in desktop mode, they generally have to do this anyway, as mentioned above. It's not perfect, but it does consistently load desktop versions of sites.
Assignee | ||
Comment 1•12 years ago
|
||
This is a heavily modified version of the patch from bug 697857 with the following changes: - the "root URL" is loaded when Request Desktop Site is clicked, as mentioned in comment 0 - the desktop UA is automatically generated - the UA is saved in localStorage for each site I've tried it on several sites (google.com, yahoo.com, mozilla.org), and it seems to work pretty well.
Attachment #634679 -
Flags: review?(mark.finkle)
Assignee | ||
Comment 2•12 years ago
|
||
Here's a build containing this change: http://dl.dropbox.com/u/35559547/fennec-request-desktop-site.apk
Comment 3•12 years ago
|
||
Why is so much of this in Java? Can't we use our own menu apis?
Assignee | ||
Comment 4•12 years ago
|
||
(In reply to Wesley Johnston (:wesj) from comment #3) > Why is so much of this in Java? Can't we use our own menu apis? We probably could, though I think we'd have to add some things, like the ability to change a menu item's label and disable/enable an item. I'm also not sure how we could refresh the menu state in onPrepareOptionsMenu() without maintaining some agentMode state on the Java side.
Comment 5•12 years ago
|
||
re comment #0, do we know what stock browser does?
Updated•12 years ago
|
Flags: in-moztrap?(fennec)
Assignee | ||
Comment 6•12 years ago
|
||
(In reply to Aaron Train [:aaronmt] from comment #5) > re comment #0, do we know what stock browser does? It looks like the stock browser and Chrome keep track of the original pre-redirected URLs. When switching the UA, it loads these URLs. We could do this too since it does work pretty well in a number of cases (clicking Google results, news items on yahoo.com, etc). We'll have to make the assumption that the redirection is from a desktop URL -> mobile URL, which isn't always true, but should be in most cases. Combined with the implementation from comment 0, I think this can work pretty well.
Assignee | ||
Comment 7•12 years ago
|
||
Implementing this as a persistent site-specific setting introduces too many problems, so this just makes it a non-persistent, tab-specific setting. This keeps track of the original URL, as described in comment 6, so that switching modes keeps the user on the same page when possible.
Attachment #634679 -
Attachment is obsolete: true
Attachment #634679 -
Flags: review?(mark.finkle)
Attachment #635527 -
Flags: review?(mark.finkle)
Assignee | ||
Comment 8•12 years ago
|
||
forgot to refresh some changes
Attachment #635527 -
Attachment is obsolete: true
Attachment #635527 -
Flags: review?(mark.finkle)
Attachment #635528 -
Flags: review?(mark.finkle)
Comment 9•12 years ago
|
||
Hey brian, you mind filing bugs for adding those menu features you mentioned?
Assignee | ||
Comment 10•12 years ago
|
||
(In reply to Wesley Johnston (:wesj) from comment #9) > Hey brian, you mind filing bugs for adding those menu features you mentioned? Filed bug 767213.
Assignee | ||
Comment 11•12 years ago
|
||
Since we don't save desktop settings for each site, this setting could be useful to some users (particularly tablet users).
Attachment #635539 -
Flags: review?(mark.finkle)
Assignee | ||
Comment 12•12 years ago
|
||
wrong patch
Attachment #635539 -
Attachment is obsolete: true
Attachment #635539 -
Flags: review?(mark.finkle)
Attachment #635542 -
Flags: review?(mark.finkle)
Comment 13•12 years ago
|
||
Comment on attachment 635528 [details] [diff] [review] Implement "Request Desktop Site", v2.1 >diff --git a/mobile/android/base/BrowserApp.java b/mobile/android/base/BrowserApp.java >+ desktopMode.setEnabled(true); >+ desktopMode.setCheckable(true); Why not set these two in the XML? >diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java >+ case R.id.desktop_mode: >+ Tab selectedTab = Tabs.getInstance().getSelectedTab(); >+ if (selectedTab == null) >+ return true; >+ JSONObject args = new JSONObject(); >+ try { >+ args.put("desktopMode", !item.isChecked()); >+ args.put("tabId", selectedTab.getId()); >+ } catch (JSONException e) { >+ Log.e(LOGTAG, "error building json arguments"); >+ } >+ GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("DesktopMode:Change", args.toString())); >+ return true; It kinda dawns on me that this code should probably be in BrowserApp somehow. Later patch... >diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js > var UserAgent = { > observe: function ua_observe(aSubject, aTopic, aData) { >+ if (channel.loadFlags & Ci.nsIChannel.LOAD_DOCUMENT_URI) { >+ // Remember original URL for UA changes on redirected pages >+ tab.originalURL = channel.originalURI.spec; Normally I favor holding the URI >+ // Send desktop UA if "Request Desktop Site" is enabled >+ if (tab.desktopModeEnabled) Can we rename to "desktopMode"? The "Enabled" suffix makes it too wordy for me and "desktopMode" works as a boolean. >+ reloadDesktopMode: function (aDesktopMode) { OCD nit: rename to reloadWithMode ? >+ let uri = this.browser.currentURI; it's nice to know uri is currentURI, so let's rename the local var >+ let flags = Ci.nsIWebNavigation.LOAD_FLAGS_BYPASS_CACHE; >+ if (this.originalURL && this.originalURL != url) { If we change originalURL to originalURI the comparison would be | this.originalURI.equals(currentURI) | >+ // Many sites use mobile-specific URLs, such as: >+ // http://m.yahoo.com >+ // http://www.google.com/m >+ // If the user clicks "Request Desktop Site" while on a mobile site, it >+ // will appear to do nothing since the mobile URL is still being >+ // requested. To address this, we do the following: >+ // 1) Remove the path from the URL (http://www.google.com/m?q=query -> http://www.google.com) >+ // 2) If a host subdomain is "m", remove it (http://en.m.wikipedia.org -> http://en.wikipedia.org) >+ // This means the user is sent to site's home page, but this is better >+ // than the setting having no effect at all. >+ if (aDesktopMode) { >+ url = Services.io.newURI(this.browser.currentURI.spec, null, null) >+ .prePath.replace(/([\/\.])m\./g, "$1"); one line is OK and this is borderline hacky, but I am OK to try it out. r- just to see a new patch
Attachment #635528 -
Flags: review?(mark.finkle) → review-
Comment 14•12 years ago
|
||
Comment on attachment 635542 [details] [diff] [review] Add preference to request desktop site by default I don't think we want this. In the long run, the tablet UA is different than the phone, and we want websites to use the UA to direct. You can get UX to weigh in too, but I don't think we want to do this part.
Attachment #635542 -
Flags: review?(mark.finkle) → review-
Assignee | ||
Comment 15•12 years ago
|
||
with suggested changes
Attachment #635528 -
Attachment is obsolete: true
Attachment #635641 -
Flags: review?(mark.finkle)
Comment 16•12 years ago
|
||
Comment on attachment 635641 [details] [diff] [review] Implement "Request Desktop Site", v3 \o/
Attachment #635641 -
Flags: review?(mark.finkle) → review+
Comment 17•12 years ago
|
||
I just thought of something: Do we need this for tablets? They should (hopefully) be getting desktop sites. Well, we can wait and see.
Assignee | ||
Comment 18•12 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/aaff24d3411a
Comment 19•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/aaff24d3411a
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 20•12 years ago
|
||
Verified Fixed, Nightly (06.24) Need an owner for this new feature on MozTrap.
status-firefox16:
--- → verified
Assignee | ||
Comment 21•12 years ago
|
||
[Approval Request Comment] Bug caused by (feature/regressing bug #): User impact if declined: users cannot switch between mobile/desktop UA Testing completed (on m-c, etc.): m-c Risk to taking this patch (and alternatives if risky): medium-low risk; fairly isolated feature, but hasn't been thoroughly tested String or UUID changes made by this patch: menu label for "Request Desktop Site"
Attachment #638557 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•12 years ago
|
status-firefox15:
--- → affected
Updated•12 years ago
|
Attachment #638557 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 22•12 years ago
|
||
http://hg.mozilla.org/releases/mozilla-aurora/rev/65e39bd76d91
Assignee | ||
Updated•12 years ago
|
Updated•12 years ago
|
Flags: in-moztrap?(fennec) → in-moztrap?(xwei)
QA Contact: xwei
Updated•12 years ago
|
Flags: in-moztrap?(xwei) → in-moztrap+
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•