Support adding links to homescreen

VERIFIED FIXED

Status

()

Firefox for Android
General
P4
normal
VERIFIED FIXED
6 years ago
a year ago

People

(Reporter: wesj, Assigned: wesj)

Tracking

unspecified
All
Android
Points:
---
Bug Flags:
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [QA+])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
XUL Fennec supported adding bookmarks and links to your homescreen. Native should too!
(Assignee)

Comment 1

6 years ago
Created attachment 582742 [details] [diff] [review]
Patch v1

Patch adds "Open in new tab" and a "Add to homescreen" for shortcuts on the awesomescreen.
Assignee: nobody → wjohnston
Attachment #582742 - Flags: review?(mark.finkle)
I guess I can | hg qdel | my half done patch :)
Comment on attachment 582742 [details] [diff] [review]
Patch v1

> 
>+    private Cursor mContextMenuCursor = null;
>+    @Override
>+    public void onCreateContextMenu(ContextMenu menu, View view, ContextMenuInfo menuInfo) {

Add a blank line to separate mmContextMenuCursor from @Override

>+        super.onCreateContextMenu(menu, view, menuInfo);
>+
>+        AdapterView.AdapterContextMenuInfo info = (AdapterView.AdapterContextMenuInfo) menuInfo;
>+        ListView list = (ListView) view;
>+        Object selecteditem = list.getItemAtPosition(info.position);
>+
>+        if (!(selecteditem instanceof Cursor)) {

Can we reset mContextMenuCursor to null here? In case we return early.

>+    @Override
>+    public boolean onContextItemSelected(MenuItem item) {
>+        AdapterContextMenuInfo info = (AdapterContextMenuInfo) item.getMenuInfo();
>+

Check mContextMenuCursor and if it's null, bail

>+        switch (item.getItemId()) {
>+            case R.id.open: {

R.id.contextmenu_open_new_tab

>+                String url = mContextMenuCursor.getString(mContextMenuCursor.getColumnIndexOrThrow(URLColumns.URL));
>+                GeckoApp.mAppContext.loadUrl(url, AwesomeBar.Type.ADD);

Should we be opening in the background? Can we do that yet? Matt might have a patch to do it. (just thinking out loud)

>+            case R.id.addtolauncher: {

R.id.contextmenu_add_to_launcher

>+                Bitmap bitmap = null;
>+                if (b != null) {
>+                    bitmap = BitmapFactory.decodeByteArray(b, 0, b.length);
>+                }

No {} needed in this file (I think)


Madhava would also like to add "Share" (R.id.contextmenu_share) which will send the title and URL to GeckoAppShell helper for sharing.

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

>+    public static void createShortcut(String aTitle, String aURI, Bitmap aIcon, String aType) {
>+        new CreateShortcutTask().execute(new Shortcut(aTitle, aURI, aIcon, aType));
>+    }
>+
>+    private static class CreateShortcutTask extends AsyncTask<Shortcut, Void, Void> {

AsyncTask is probably wrong for two reasons:
* Since you don't need to impl the onPostExecute, you could probably just use GeckoAppShell.mHandler.post( Runnable );
* We use GeckoAsyncTask to make sure everything is on the right thread. If you do need AsyncTask, use GeckoAsyncTask instead.

>+    static private Bitmap getIcon(Bitmap aSource) {

>+        paint.setColor(Color.HSVToColor(hsv));
>+        canvas.drawRoundRect(new RectF(kOffset,
>+                                       kOffset,
>+                                       kIconSize-kOffset,
>+                                       kIconSize-kOffset), 5, 5, paint);

kIconSize - kOffset

What are the | 5 | about? Margins? Add a constant?

>+        // draw the bitmap
>+        if (aSource == null) {
>+            aSource = BitmapFactory.decodeResource(GeckoApp.mAppContext.getResources(), R.drawable.home_star);
>+        }

Drop the {} in this file (I think)

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

>+            if (colors[h] > colors[maxH]) {
>+              maxH = h;
>+            }
>+            if (sat[s] > sat[maxS]) {
>+              maxS = s;
>+            }
>+            if (val[v] > val[maxV]) {
>+              maxV = v;
>+            }

Drop {}

>+            Log.i("BitmapUtils", "h = " + maxH + ", s = " + maxS + ", v = " + maxV);

Add a LOGTAG constant to the class (See other java files for example)

>diff --git a/mobile/android/base/locales/en-US/android_strings.dtd b/mobile/android/base/locales/en-US/android_strings.dtd

>+<!ENTITY contextmenu_open "Open in new tab">

contextmenu_open_new_tab "Open in New Tab"

>+<!ENTITY contextmenu_addtolauncher "Add to homescreen">

contextmenu_add_to_launcher "Add to Home Screen" (what we used in XUL)

>diff --git a/mobile/android/base/resources/menu/awesomebar_contextmenu.xml b/mobile/android/base/resources/menu/awesomebar_contextmenu.xml

>+    <item android:id="@+id/open"
>+          android:title="@string/contextmenu_open"/>
>+
>+    <item android:id="@+id/addtolauncher"
>+          android:title="@string/contextmenu_addtolauncher"/>
>+

* Add "Share" (Don't reuse the existing id and string entity since l10n might have a different context)
* Use new id and string entity names

>diff --git a/mobile/android/base/strings.xml.in b/mobile/android/base/strings.xml.in

>+  <string name="contextmenu_open">&contextmenu_open;</string>
>+  <string name="contextmenu_addtolauncher">&contextmenu_addtolauncher;</string>

Here too

r-, but close
Attachment #582742 - Flags: review?(mark.finkle) → review-
(Assignee)

Comment 4

6 years ago
Created attachment 582974 [details] [diff] [review]
Patch v2

Fixed up and tweaked in a few bits. Screenshot (arranged in a pretty rainbow) at:

http://dl.dropbox.com/u/72157/icons.png
Attachment #582742 - Attachment is obsolete: true
Attachment #582974 - Flags: review?(mark.finkle)
Priority: -- → P4
Comment on attachment 582974 [details] [diff] [review]
Patch v2

>diff --git a/mobile/android/base/AwesomeBar.java b/mobile/android/base/AwesomeBar.java
>+    public boolean onContextItemSelected(MenuItem item) {
>+        if (mContextMenuCursor == null) {
>+            return false;
>+        }
>+        AdapterContextMenuInfo info = (AdapterContextMenuInfo) item.getMenuInfo();

Drop {} but leave a blank line after | return false; |

>+            case R.id.share: {
>+                String url = mContextMenuCursor.getString(mContextMenuCursor.getColumnIndexOrThrow(URLColumns.URL));
>+                String title = mContextMenuCursor.getString(mContextMenuCursor.getColumnIndexOrThrow(URLColumns.TITLE));
>+                GeckoAppShell.openUriExternal(url, "text/plain", "", "",
>+                                              Intent.ACTION_SEND, title);

Does this need wrapped?

>diff --git a/mobile/android/base/GeckoAppShell.java b/mobile/android/base/GeckoAppShell.java
>+    static private Bitmap getIcon(Bitmap aSource) {

Since this is a method on GeckoAppShell, getIcon sounds too gneral - how about getLauncherIcon ?

>diff --git a/mobile/android/base/locales/en-US/android_strings.dtd b/mobile/android/base/locales/en-US/android_strings.dtd

>+<!ENTITY contextmenu_open_new_tab "Open in new tab">

"Open in New Tab"

r+ with the nits fixed
Attachment #582974 - Flags: review?(mark.finkle) → review+
Made nits and pushed
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2bb564589fc
https://hg.mozilla.org/mozilla-central/rev/d2bb564589fc
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Depends on: 712370

Updated

6 years ago
Depends on: 714092

Updated

6 years ago
Flags: in-litmus+
Whiteboard: [QA+]
No longer depends on: 714092
Aurora nom?
(Assignee)

Comment 9

6 years ago
Comment on attachment 582974 [details] [diff] [review]
Patch v2

I think we should probably take this on Aurora. It a feature regression from XUL Fennc. Has been on nightlies for awhile now. All known regression bugs have been cleaned up (will have to also land the patch in bug 712627 and bug 714975, I'll nom them as well).
Fennec only. Low risk.
Attachment #582974 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 10

6 years ago
Comment on attachment 582974 [details] [diff] [review]
Patch v2

Oh wait. This already is on Aurora. Heh.
Attachment #582974 - Flags: approval-mozilla-aurora?

Comment 11

6 years ago
Verified with:
12.0a1 (2012-01-11) Device: LG Optimus 2X Android(2.2)
11.0a2 (2012-01-11) Device: LG Optimus 2X Android(2.2)

Links can be added to home screen from Awesome menu. (After long tap on the link, a context menu with "Add to Home Screen" option is displayed.)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.