932208 - BrowserToolbar and HomeFragment context menu 'Share' option should be disabled for these protocols: about:// chrome:// file://

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Adrian Tamas (:AdrianT)]

Firefox Mobile 26 beta 1
Samsung Galaxy Tab 2 7.0(Android 4.1)

Steps to reproduce:
1) Load any unsupported share protocols (e.g. about pages, local files etc)
2) Long tap on the url bar and from the context menu choose share

Expected results:
Just like for the Custom menu the share option is grayed out

Actual results:
The url can be shared

Comment 1

[Aaron Train [:aaronmt]]

Probably need a new updated patch akin to bug 703630, but I sense a Finkle wontfix.

Comment 2

[u421692]

Since the bug 958517 , the share option has been removed from the URL context menu. However, I'm still able to reproduce this from the context menu triggered in top sites/bookmarks panels. Should we keep tracking this issue?

Comment 3

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

Looks like BrowserToolbar and HomeFragment need access to a allowShare style method:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/toolbar/BrowserToolbar.java#275
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/HomeFragment.java#106

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#2234

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Mihai Pop from comment #2)
> Since the bug 958517 , the share option has been removed from the URL
> context menu. However, I'm still able to reproduce this from the context
> menu triggered in top sites/bookmarks panels.

Which context menu is this? When you long-click a top sites item or a bookmark? That seems correct to me.

Comment 5

[u421692]

(In reply to Michael Comella (:mcomella) from comment #4)
> (In reply to Mihai Pop from comment #2)
> > Since the bug 958517 , the share option has been removed from the URL
> > context menu. However, I'm still able to reproduce this from the context
> > menu triggered in top sites/bookmarks panels.
> 
> Which context menu is this? When you long-click a top sites item or a
> bookmark? That seems correct to me.

I think I was not clear enough in my previous comment. I was referring to about:pages in bookmark panel, and pinned top sites such as "about:firefox" or "about:config".

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 1: Remove Share from url bar context menu on pre-ICS.

As per discussion with Ian via IRC.

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 2: Do not allow non-shareable links to be shared.

Comment 8

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

Comment on attachment 8417698 [details] [diff] [review]
Part 2: Do not allow non-shareable links to be shared.

We should be able to remove the contextmenu handler too:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#783

The mainmenu could still be using this one:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#2385

I'd like to see a new patch just we we verify the code is dead now.

Comment 9

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

Comment on attachment 8417697 [details] [diff] [review]
Part 1: Remove Share from url bar context menu on pre-ICS.

Damn. Added comment to wrong patch.

Comment 10

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

Comment on attachment 8417698 [details] [diff] [review]
Part 2: Do not allow non-shareable links to be shared.

>-        menu.findItem(R.id.home_share).setVisible(!GeckoProfile.get(getActivity()).inGuestMode());
>+        if (!StringUtils.isShareableUrl(info.url, GeckoProfile.get(getActivity()).inGuestMode())) {
>+            menu.findItem(R.id.home_share).setVisible(false);
>+        }

I am not a fan of this pattern. I'd rather separate the shareable URL concept and the guestmode concept. Can you switch to use this style:

         if (!StringUtils.isShareableUrl(info.url) || GeckoProfile.get(getActivity()).inGuestMode()) {

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 1: Remove Share from url bar context menu on pre-ICS.

Removed the contextmenu handler as per comment 8.

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 2: Do not allow non-shareable links to be shared.

Adjusted for comment 10.

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

remote:   https://hg.mozilla.org/integration/fx-team/rev/09caceb50f73
remote:   https://hg.mozilla.org/integration/fx-team/rev/aa4dc866e852

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 3: Fix testBookmarksPanel.

Fixing fx-team bustage: https://tbpl.mozilla.org/?tree=Fx-Team&rev=aa4dc866e852

Comment 15

[Chenxia Liu [:liuche]]

Comment on attachment 8438055 [details] [diff] [review]
Part 3: Fix testBookmarksPanel.

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

::: mobile/android/base/tests/testBookmarksPanel.java
@@ +18,5 @@
>          for (String url : StringHelper.DEFAULT_BOOKMARKS_URLS) {
>              isBookmarkDisplayed(url);
>          }
>  
> +        assertAllContextMenuOptionsArePresent(StringHelper.DEFAULT_BOOKMARKS_URLS[1],

I would add a comment either here or above the method, about checking both shareable and unshareable links.

@@ +55,5 @@
>  
> +    private void assertAllContextMenuOptionsArePresent(final String shareableURL,
> +            final String nonShareableURL) {
> +        mAsserter.ok(StringUtils.isShareableUrl(shareableURL), "Ensuring url is shareable", "");
> +        mAsserter.ok(!StringUtils.isShareableUrl(nonShareableURL), "Ensuring url is shareable", "");

(you mentioned this already) "not shareable".

@@ +69,5 @@
> +        mActions.sendSpecialKey(Actions.SpecialKey.BACK);
> +
> +        openBookmarkContextMenu(nonShareableURL);
> +        for (String contextMenuOption : StringHelper.BOOKMARK_CONTEXT_MENU_ITEMS) {
> +            if ("Share".equals(contextMenuOption)) {

Add a comment here about skipping this item.

Comment 16

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 3: Fix testBookmarksPanel.

Fixed nits.

Comment 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I'll save you, fx-team! https://hg.mozilla.org/integration/fx-team/rev/d1e57e3cbba1

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Well, about that...

Backed out in https://hg.mozilla.org/integration/fx-team/rev/b72560289748 for further failures in robocop-1: https://tbpl.mozilla.org/php/getParsedLog.php?id=41489414&tree=Fx-Team

Comment 19

[Wesley Johnston (:wesj)]

It would be kinda neat to actually make these just "share" Firefox. i.e. send a link to the marketplace with some sort of "Firefox for Android rocks!" message :)

Comment 20

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: imported patch _4-null

Oh, it was a real bug. :D

try: https://tbpl.mozilla.org/?tree=Try&rev=ddb416f9cf5c

Later rebased part 3 (not in this try run); will upload.

Comment 21

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 3: Fix testBookmarksPanel.

Rebase.

Comment 22

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 4: Do not compare against null in isShareableUrl.

Updated description.

Comment 23

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Take two:
remote:   https://hg.mozilla.org/integration/fx-team/rev/74efce73d13e
remote:   https://hg.mozilla.org/integration/fx-team/rev/afe2fb8b3c92
remote:   https://hg.mozilla.org/integration/fx-team/rev/baf23870f2ea
remote:   https://hg.mozilla.org/integration/fx-team/rev/963ce9889301

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/74efce73d13e
https://hg.mozilla.org/mozilla-central/rev/afe2fb8b3c92
https://hg.mozilla.org/mozilla-central/rev/baf23870f2ea
https://hg.mozilla.org/mozilla-central/rev/963ce9889301

Comment 25

[u421692]

Verified as fixed on Firefox 33 Beta 4.