950610 - Add "Share" and "Add to Home Screen" to pinned sites context menus

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[u421692]

Since "Share" and "Add to Home Screen" are present for every list entry in about:home(top sites, bookmarks, reading list, history), "Share" and "Add to Home Screen" should also be added to pinned sites context menus.

Comment 1

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

Ian - Do we want to add one or both of these? "Add to Home Screen" might be useful. "Share" might be less needed.

Comment 2

[Ian Barlow (:ibarlow)]

Yeah, and I would suggest adding both for consistency's sake. I don't feel too strongly about Share, but it seems kind of arbitrary to not have it for thumbnails if you have it elsewhere.

Comment 3

[Sola Ogunsakin [:sola]]

Attachment: x.patch

Added "Share" and "Add to Home Screen" to pinned sites context menus.

Test:
- Was able to "Share" and "Add to Home Screen" on pinned and unpinned items

Comment 4

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

We may want a Try server run here, to check that adding the menu items does not break an existing test. If we have not tests for the presence of the menu items, maybe we should add one.

Comment 5

[Chenxia Liu [:liuche]]

Try: https://tbpl.mozilla.org/?tree=Try&rev=509aa8b9d696

Comment 6

[Chenxia Liu [:liuche]]

Comment on attachment 8356380 [details] [diff] [review]
x.patch

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

This is a good start! However, I started noticing that there is a lot of duplicated code between the HomeFragment contextmenu and the top sites contextmenu. Let's combine them, and use visibility to display the relevant menu items.

::: mobile/android/base/home/TopSitesPage.java
@@ +22,5 @@
>  import org.mozilla.gecko.home.PinSiteDialog.OnSiteSelectedListener;
>  import org.mozilla.gecko.home.TopSitesGridView.OnEditPinnedSiteListener;
>  import org.mozilla.gecko.home.TopSitesGridView.TopSitesGridContextMenuInfo;
>  import org.mozilla.gecko.util.ThreadUtils;
> +import org.mozilla.gecko.GeckoAppShell;

Nit: move this up to alphabetize imports

@@ +361,5 @@
>          }
>  
> +        if (itemId == R.id.home_share) {
> +            if (info.url == null) {
> +                Log.e(LOGTAG, "Can't share because URL is null");

Nit: let's make this message a little clearer out of context, maybe something like "Share not enabled for context menu because URL is null." No need to make it an Error level log, warn should be sufficient.

I noticed that there is actually a lot of duplicated code now, between HomeFragment and TopSites - it looks like the context menu resources are almost exactly the same now, so why don't we actually clean those up and unify them?

@@ +370,5 @@
> +        }
> +
> +        if (itemId == R.id.home_add_to_launcher) {
> +            if (info.url == null) {
> +                Log.e(LOGTAG, "Can't add to home screen because URL is null");

Same as above comment for the message.

::: mobile/android/base/resources/menu/top_sites_contextmenu.xml
@@ +17,5 @@
>      <item android:id="@+id/top_sites_pin"
>            android:title="@string/contextmenu_top_sites_pin"/>
>  
>      <item android:id="@+id/top_sites_unpin"
>            android:title="@string/contextmenu_top_sites_unpin"/>

Nit: newline between items.

@@ +20,5 @@
>      <item android:id="@+id/top_sites_unpin"
>            android:title="@string/contextmenu_top_sites_unpin"/>
> +    <item android:id="@+id/home_share"
> +          android:title="@string/contextmenu_share"/>
> +          

Nit: no trailing whitespace.

Comment 7

[Chenxia Liu [:liuche]]

Specifically:

We can combine resources/menu/home_contextmenu.xml and resources/menu/top_sites_contextmenu.xml.

Then take a look at how we hide/show specific contextmenu items, like [1].

Basically, we should at least combine HomeFragment.onCreateContextMenu and TopSitesPage.onCreateContextMenu, and consider combining onContextItemSelected (by calling through super.onContextItemSelected).

[1] http://hg.mozilla.org/mozilla-central/file/ce3dd14c1840/mobile/android/base/home/HomeFragment.java#l90

Comment 8

[Chenxia Liu [:liuche]]

Actually, going through the code some more, doing a refactor will be a little more complicated and is kind of out of the scope of this "good-first-bug," so let's eat the code duplication a little longer and think about whether the refactor is worth it.

Comment 9

[Sola Ogunsakin [:sola]]

Attachment: bug-950610-fix-v2.patch

Comment 10

[Sola Ogunsakin [:sola]]

Attachment: bug-950610-fix-v2.patch

Comment 11

[Chenxia Liu [:liuche]]

Comment on attachment 8356926 [details] [diff] [review]
bug-950610-fix-v2.patch

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

This looks good, except it seems to be a separate patch on top of your original patch. You'll want to address the comments and then combine the two patches. You can use hg qfold to do this:
Leave only the first patch applied (pop any other ones). Then 'hg qfold <second-patchname>' to fold the second patch into the first. Then upload the combined patches.

Almost there!

::: mobile/android/base/resources/menu/top_sites_contextmenu.xml
@@ +18,5 @@
>            android:title="@string/contextmenu_top_sites_pin"/>
>  
>      <item android:id="@+id/top_sites_unpin"
>            android:title="@string/contextmenu_top_sites_unpin"/>
> +          

Nit: it looks like your editor might be adding in spaces automatically - there are still leading/trailing spaces that should be removed.

@@ +23,3 @@
>      <item android:id="@+id/home_share"
>            android:title="@string/contextmenu_share"/>
>            

Same: extra spaces.

Comment 12

[Sola Ogunsakin [:sola]]

Attachment: bug-950610-fix3.patch

Comment 13

[Chenxia Liu [:liuche]]

Comment on attachment 8357202 [details] [diff] [review]
bug-950610-fix3.patch

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

Two last little things, and then this is good to go!

Also, update the patch comment to terminate with a . instead of ;, and add r=liuche to the end. (Bug 950610 - Add "Share" and "Add to Home Screen" to pinned sites context menus. r=liuche)

When you're done making the changes, add checkin-needed to the whiteboard.

::: mobile/android/base/home/TopSitesPage.java
@@ +28,5 @@
>  import android.app.Activity;
>  import android.content.ContentResolver;
>  import android.content.Context;
>  import android.content.res.Configuration;
> +import android.content.Intent;

Tiny nit: alphabetize this after android.content.Context

::: mobile/android/base/resources/menu/top_sites_contextmenu.xml
@@ +19,5 @@
>  
>      <item android:id="@+id/top_sites_unpin"
>            android:title="@string/contextmenu_top_sites_unpin"/>
>  
> +    <item android:id="@+id/home_share"

Move this to be above Edit, so it's consistent with the other menus.

Comment 14

[Sola Ogunsakin [:sola]]

Attachment: bug-950610-fix4.patch

Comment 15

[Chenxia Liu [:liuche]]

Comment on attachment 8357302 [details] [diff] [review]
bug-950610-fix4.patch

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

One last whitespace error. Also, tiny tiny nit, but a space between the . and r= in the comment.

Sorry to be gating an so many small insignificant-sounding things; in general, large projects benefit from consistent code style for maintenance and readability reasons, because the code might be around for a long time and be touched by many people.

::: mobile/android/base/resources/menu/top_sites_contextmenu.xml
@@ +12,5 @@
>            android:title="@string/contextmenu_open_private_tab"/>
>  
> +    <item android:id="@+id/home_share"
> +          android:title="@string/contextmenu_share"/>
> +        

Whoops, looks like you introduced another trailing whitespace.

Comment 16

[:Margaret Leibovic]

Comment on attachment 8357302 [details] [diff] [review]
bug-950610-fix4.patch

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

::: mobile/android/base/home/TopSitesPage.java
@@ +365,5 @@
> +                Log.w(LOGTAG, "Share not enabled for context menu because URL is null.");
> +            } else {
> +                GeckoAppShell.openUriExternal(info.url, SHARE_MIME_TYPE, "", "",
> +                                              Intent.ACTION_SEND, info.getDisplayTitle());
> +            }

Drive-by: Shouldn't you return true in here?

Comment 17

[Chenxia Liu [:liuche]]

Comment on attachment 8357302 [details] [diff] [review]
bug-950610-fix4.patch

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

::: mobile/android/base/home/TopSitesPage.java
@@ +365,5 @@
> +                Log.w(LOGTAG, "Share not enabled for context menu because URL is null.");
> +            } else {
> +                GeckoAppShell.openUriExternal(info.url, SHARE_MIME_TYPE, "", "",
> +                                              Intent.ACTION_SEND, info.getDisplayTitle());
> +            }

Huh, that's true. Sola, can you return true here, and also add it to the share code in HomeFragment? It should return true there as well.

Comment 18

[Sola Ogunsakin [:sola]]

Attachment: bug-950610-fix5.patch

Comment 19

[Chenxia Liu [:liuche]]

Comment on attachment 8357442 [details] [diff] [review]
bug-950610-fix5.patch

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

Just small changes, then r+! (Do be aware of whitespace in the future though.)

::: mobile/android/base/home/TopSitesPage.java
@@ +372,5 @@
> +        }
> +
> +        if (itemId == R.id.home_add_to_launcher) {
> +            if (info.url == null) {
> +                Log.w(LOGTAG, "Cannot add to home page for context menu because URL is null.");

Nit: this comment is a little confusing, maybe something like "Not enabling 'Add to home page' because URL is null."

::: mobile/android/base/resources/menu/top_sites_contextmenu.xml
@@ +12,5 @@
>            android:title="@string/contextmenu_open_private_tab"/>
>  
> +    <item android:id="@+id/home_share"
> +          android:title="@string/contextmenu_share"/>
> +        

Extra whitespace.

Comment 20

[Sola Ogunsakin [:sola]]

Attachment: bug-950610-fix6.patch

Comment 21

[Chenxia Liu [:liuche]]

Comment on attachment 8357504 [details] [diff] [review]
bug-950610-fix6.patch

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

Looks good!

Now just add 'checkin-needed' into the whiteboard field of the bug, and a sheriff (or someone else) will land it. In general, don't set checkin flags or whiteboard until the patch is completely ready to land, because otherwise it might actually get checked in!

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/ad1fec1c3678

Comment 23

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

https://hg.mozilla.org/mozilla-central/rev/ad1fec1c3678

Comment 24

[Ioana Chiorean]

Build: Firefox Nightly 29.0a1 2014-01-10
Device: Galaxy Nexus 
OS: Android 4.3 

The two options are present for the pinned top sites. Setting the according flag to verified. 
Will this be uplifted to Aurora/Beta?

Comment 25

[Ioana Chiorean]

TC updated in MozTrap for Mobile Fx 29 and Mobile Fx 29 tablet versions: https://moztrap.mozilla.org/manage/case/6030/