1089838 - 'Request desktop site' is confusing

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: General, defect)

Description

[Mike Taylor [:miketaylr]]

It shouldn't be possible to "Request Desktop Site" from "about:home" and similar non-webby URLs. I see that Bug 711595 fixed this a while ago, but it seems to have regressed at some point.

Comment 1

[Mike Taylor [:miketaylr]]

Attachment: 1089838.patch

Simple fix + a robocop test to make sure this doesn't regress in the future. 

Margaret, r? because you fixed this once before. :)

Comment 2

[Mike Taylor [:miketaylr]]

Just to note:

> desktopMode.setEnabled(StringUtils.isShareableUrl(tab.getURL()));

It feels a little funny to use "isShareableUrl" as a test for webfacing-ness. It's got the right semantics though. Dunno if we should rename that to something more generic (no idea what that would be) and update other code that uses isShareableUrl.

Comment 3

[:Margaret Leibovic]

Comment on attachment 8512224 [details] [diff] [review]
1089838.patch

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

Good catch! Sucks that regressed :/ Thanks for making a test.

::: mobile/android/base/BrowserApp.java
@@ +2605,5 @@
>          bookmark.setIcon(tab.isBookmark() ? R.drawable.ic_menu_bookmark_remove : R.drawable.ic_menu_bookmark_add);
>  
>          back.setEnabled(tab.canDoBack());
>          forward.setEnabled(tab.canDoForward());
> +        // Enable desktop mode menuitem only for web-facing URLs

Nit: Add an empty line above this comment line.

::: mobile/android/base/tests/components/AppMenuComponent.java
@@ +104,5 @@
> +    public void assertMenuItemIsEnabledAndVisible(MenuItem menuItem) {
> +        openAppMenu();
> +
> +        final View menuItemView = findAppMenuItemView(menuItem.getString(mSolo));
> +        fAssertTrue("The page menu item is enabled", menuItemView.isEnabled());

Update these comments to be more generic, since these methods aren't specifically testing the page menu item.

Comment 4

[Brian Nicholson (:bnicholson)]

Hm, I thought this came up before and we decided that RDS should still be enabled since RDS is a per-tab setting, not per-site setting. That is, I might want to switch RDS on before going to a site.

Comment 5

[:Margaret Leibovic]

(In reply to Brian Nicholson (:bnicholson) from comment #4)
> Hm, I thought this came up before and we decided that RDS should still be
> enabled since RDS is a per-tab setting, not per-site setting. That is, I
> might want to switch RDS on before going to a site.

Oh, yeah, better memory than me. We should try to dig up the bug where we actually removed this.

Comment 6

[Richard Newman [:rnewman]]

(In reply to Mike Taylor [:miketaylr] from comment #2)
> Just to note:
> 
> > desktopMode.setEnabled(StringUtils.isShareableUrl(tab.getURL()));
> 
> It feels a little funny to use "isShareableUrl" as a test for
> webfacing-ness. It's got the right semantics though. Dunno if we should
> rename that to something more generic (no idea what that would be) and
> update other code that uses isShareableUrl.

We do have AboutPages.isAboutPage for this kind of thing.

Comment 7

[Mike Taylor [:miketaylr]]

(In reply to Richard Newman [:rnewman] from comment #6)
> We do have AboutPages.isAboutPage for this kind of thing.

That's cool. But doesn't cover chrome:// file:// and resource://.(In reply to :Margaret Leibovic from comment #5)

> (In reply to Brian Nicholson (:bnicholson) from comment #4)
> > Hm, I thought this came up before and we decided that RDS should still be
> > enabled since RDS is a per-tab setting, not per-site setting. That is, I
> > might want to switch RDS on before going to a site.
> 
> Oh, yeah, better memory than me. We should try to dig up the bug where we
> actually removed this.

Seems like a good topic for the mailing list. I'll start a thread tomorrow morning (My feelings won't be hurt either way--I noticed this because right now requesting desktop content prompts the user to report a site issue. Obviously that makes no sense for about:home et al.)

Comment 8

[Mike Taylor [:miketaylr]]

Eh, actually I don't know if you guys hash out stuff like this on the mailing list--or if discussion is better here in bugs.

Comment 9

[Mike Taylor [:miketaylr]]

It looks like the discussion happened in https://bugzilla.mozilla.org/show_bug.cgi?id=790954#c15 and https://bugzilla.mozilla.org/show_bug.cgi?id=790954#c16. I get the reasoning, so this wasn't really a regression.

Since https://bugzilla.mozilla.org/show_bug.cgi?id=767795 was WONTFIX'd, I'll go ahead and WONTFIX this one myself. Should have done some better searching, sorry!

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

It doesn't make sense to me to "request desktop site" for a protocol where we don't send a request. That includes about:, chrome: and file:. The behavior might be per-tab, but the user is taking the action in the context of a tab.

Comment 11

[Brian Nicholson (:bnicholson)]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #10)
> The behavior might be per-tab, but the user is taking the action in the
> context of a tab.

What does "context of a tab" mean? Is that not "per-tab"?

Comment 12

[Gian-Carlo Pascutto [:gcp]]

I would suggest a tiny rewording: "Request desktop site" -> "Request desktop sites".

Comment 13

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

A request was made to look at for a better wording for this menu.

Comment 14

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.