937253 - Android home screen shortcuts should switch-to-tab if the page is already open

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

STR:
Bookmark a page and add it to the Home Screen
Click on bookmark on Home Screen
Note the current tab count
Go back to the home screen
Click on same bookmark on Home Screen

ER:
The tab count remains the same and the selected tab is the previously opened bookmarked page. It could be reloaded or just shown as usual.

AR:
A new tab is created at the URL of the bookmarked page.

(Tested with Firefox Nightly 11/11/2013 on LG Nexus 5)

Comment 1

[Lucas Rocha (:lucasr)]

This will be covered by bug 933811. We need design for the switch-to-tab functionality in about:home's thumbnails.

Comment 2

[:Margaret Leibovic]

I thought this bug was about the "Add to home screen" functionality, like the shortcuts we create on your Android launch screen. Bug 933811 is just about the about:home thumbnails.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to :Margaret Leibovic from comment #2)
> I thought this bug was about the "Add to home screen" functionality, like
> the shortcuts we create on your Android launch screen. Bug 933811 is just
> about the about:home thumbnails.

This is correct. I am talking about the "Add to home screen", as in the Android home screen, not the Firefox home screen. Should this bug be reopened?

Comment 4

[:Margaret Leibovic]

(In reply to Jared Wein [:jaws] from comment #3)
> (In reply to :Margaret Leibovic from comment #2)
> > I thought this bug was about the "Add to home screen" functionality, like
> > the shortcuts we create on your Android launch screen. Bug 933811 is just
> > about the about:home thumbnails.
> 
> This is correct. I am talking about the "Add to home screen", as in the
> Android home screen, not the Firefox home screen. Should this bug be
> reopened?

Yep.

Comment 5

[Aaron Train [:aaronmt]]

Can this be a mentor bug?

Comment 6

[Lucas Rocha (:lucasr)]

(In reply to Aaron Train [:aaronmt] from comment #5)
> Can this be a mentor bug?

I think so.

Comment 8

[Phil]

Attachment: bug937253v1.patch

Hi,

I'd like to know what you think about this patch. I wasn't positive about where the logic for this should go, but from reading through the code GeckoApp.java seemed like the best place.

Phil

Comment 9

[Lucas Rocha (:lucasr)]

Comment on attachment 8358405 [details] [diff] [review]
bug937253v1.patch

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

Hi Phil, first of all, thanks for the patch. Your patch changes the code for when you add a page to Android's home screen. But this bug is about when you tap on the bookmark in Android's home screen. Here some extra information to help you find your way in the code that needs to be changed:

When you tap on a bookmark that was added to Android's home screen, we'll use a special type of intent (ACTION_BOOKMARK) that is defined when you add the shortcut:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoAppShell.java#809

So, when you tap on a shortcut from Android's home screen, we'll handle it here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java#1911

Which will make Gecko use a special -bookmark flag when loading the URL:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoEvent.java#673

Which will then be used by gecko to figure out how to load the page in here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/BrowserCLH.js#69

But note how we don't really do anything with the bookmark flag in there. The actual call that triggers the page load is here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/BrowserCLH.js#94

Note how we unconditionally use OPEN_NEWTAB in there.

A very first thing to try is to:
- Check if the -bookmark flag was passed around here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/BrowserCLH.js#69
- If the -bookmark flag was passed, use OPEN_SWITCHTAB (instead of OPEN_NEWTAB)

For more context, these flags are defined here:
https://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#2624

And used in this _getBrowser() method here:
https://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#2610

There's some old cruft there that uses appOrigin to handle OPEN_SWITCHTAB but we should just match based on tab's URL instead:
https://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#2615

I realize this is a lot to grasp at once :-) Ping us on IRC if you have any questions :-)

Comment 10

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

Curiously, this is not an issue on new tablet.

NI to investigate and maybe mentor.

Comment 11

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

Curiously, for the default bookmarks, this appears to work for "Firefox Marketplace" on all devices, but not for "Firefox: Customize with add-ons".

This sounds like it requires some digging but I'm definitely not knowledgeable enough on this part of the code to mentor.

Margaret, can you take a look and mentor if possible?

Also, I wonder how relevant comment 2 still is - can you speak to that?

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Michael Comella (:mcomella) from comment #11)

Please don't change the direction of this bug. As comment #3 states, this is not for about:home, but the Android home screen. I think you're looking for bug 933811.

Comment 13

[Richard Newman [:rnewman]]

Let's avoid further confusion in the bug summary -- after all, home screen shortcuts don't need to be bookmarks.


(In reply to Michael Comella (:mcomella) from comment #11)

> Curiously, for the default bookmarks, this appears to work for "Firefox
> Marketplace" on all devices, but not for "Firefox: Customize with add-ons".

That's because the page redirects from the bookmark:

  https://addons.mozilla.org/android/

to the locale-specific page:

  https://addons.mozilla.org/en-US/android/

so from Fennec's perspective it's not the same page, and it won't offer switch-to-tab.

The same will be true in some circumstances for home screen shortcuts, and anywhere else we might offer switch-to-tab.

Comment 14

[:Margaret Leibovic]

We don't even offer switch-to-tab for home screen shortcuts, which is why this bug was filed.

Most of lucasr's mxr links from comment 9 are out of date, but I don't think this logic has changed, so his general advice should still remain true.

Comment 15

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

I started looking into this bug. It was annoying me too much to let it alone.

I added the concept of "pinned" sites [1] back in the old days when WebApps were just Homescreen shortcuts, like Bookmarks. WebApps went an entirely different (better) direction with Native APK wrappers and never use "pinned" anymore.

Most of the "pinned" code paths are still in place and almost work just fine. I will add some patches to:
1. Remove some vestiges of the old WebApps-as-shortcuts code
2. Updated the "pinned" code to work for Bookmarks and handle the new Tab stubs

Comment 16

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

(In reply to Mark Finkle (:mfinkle) from comment #15)

> I added the concept of "pinned" sites [1] back in the old days when WebApps

Where [1] is:
http://hg.mozilla.org/mozilla-central/rev/2ce1f809b765

Comment 17

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

Attachment: remove-old-webapps v0.1

This patch removes some old code that handled the original WEBAPP intent coming in from a homescreen shortcut. The new WebApp approach never uses this code path, I verified.

Also note that "pinned" doesn't need to be sent to browser.js startup anymore. Tab stubs are used, which mean the startup code never knows the URL being opened anymore.

Comment 18

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

Attachment: make-pinned-work v0.1

This patch makes sure the "pinned" idea works for HOMESCREEN_SHORTCUT intents (which used to be BOOKMARK intents). The only real code change here is updating loadStartupTab to take flags. We need to be able to tell Gecko whether or not the External URL is supposed to be pinned or not.

Comment 19

[Brian Nicholson (:bnicholson)]

Comment on attachment 8543356 [details] [diff] [review]
make-pinned-work v0.1

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

::: mobile/android/base/webapp/WebappImpl.java
@@ +183,3 @@
>          // Load a tab so it's available for any code that assumes a tab
>          // before the app tab itself is loaded in BrowserApp._loadWebapp.
> +        flags = Tabs.LOADURL_NEW_TAB | Tabs.LOADURL_USER_ENTERED | Tabs.LOADURL_EXTERNAL;

Might want to use "flags |=" instead so any additional flags passed in don't get wiped out and ignored.

Comment 20

[Wesley Johnston (:wesj)]

Comment on attachment 8543355 [details] [diff] [review]
remove-old-webapps v0.1

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

::: mobile/android/chrome/content/browser.js
@@ +464,3 @@
>      // The order that context menu items are added is important
>      // Make sure the "Open in App" context menu item appears at the bottom of the list
> +    this.initContextMenu();

This will make all our context menu actions work in webapps. I don't mind that, but note that we intentionally disabled it before (and apparently disabled it in SearchEngine startup too?)

Comment 21

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

(In reply to Wesley Johnston (:wesj) from comment #20)
> Comment on attachment 8543355 [details] [diff] [review]
> remove-old-webapps v0.1
> 
> Review of attachment 8543355 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/chrome/content/browser.js
> @@ +464,3 @@
> >      // The order that context menu items are added is important
> >      // Make sure the "Open in App" context menu item appears at the bottom of the list
> > +    this.initContextMenu();
> 
> This will make all our context menu actions work in webapps. I don't mind
> that, but note that we intentionally disabled it before (and apparently
> disabled it in SearchEngine startup too?)

Not really. I verified that "pinned" is never true in BrowserApp.startup for webapps, or anything. We must disable context menus some other way. Wait, actually that's bug 1097261.

Comment 22

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

remote:   https://hg.mozilla.org/integration/fx-team/rev/66b114f7c038
remote:   https://hg.mozilla.org/integration/fx-team/rev/5419d08cbe98

Comment 23

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

(In reply to Brian Nicholson (:bnicholson) from comment #19)
> Comment on attachment 8543356 [details] [diff] [review]
> make-pinned-work v0.1
> 
> Review of attachment 8543356 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/webapp/WebappImpl.java
> @@ +183,3 @@
> >          // Load a tab so it's available for any code that assumes a tab
> >          // before the app tab itself is loaded in BrowserApp._loadWebapp.
> > +        flags = Tabs.LOADURL_NEW_TAB | Tabs.LOADURL_USER_ENTERED | Tabs.LOADURL_EXTERNAL;
> 
> Might want to use "flags |=" instead so any additional flags passed in don't
> get wiped out and ignored.

We totally ignore whatever is passed into WebappImpl.loadStartupTab, so we don't want the flags. I could have improved the comment to make the more clear.

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/66b114f7c038
https://hg.mozilla.org/mozilla-central/rev/5419d08cbe98

Comment 25

[Cristina Madaras, QA [:CristinaM]]

Verified as fixed in Firefox for Android 37.0a1 (2015-01-11)
Device: Samsung Galaxy S4 (Android 4.4.2)

Comment 27

[Lawrence Mandel [:lmandel] (use needinfo)]

This is a nice change. relnoted as "Changed: Android home screen shortcut now opens existing tab instead of new tab"