840574 - Browser.EXTRA_APPLICATION_ID not handled

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sven Dawitz]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.17 (KHTML, like Gecko) Chrome/24.0.1312.68 Safari/537.17

Steps to reproduce:

When starting a browser from another app, you might want to force it to use a single tab for all actions from this app, so your users don't get into tab-hell.

Android knows a flag for this: intent.putExtra(Browser.EXTRA_APPLICATION_ID, context.getPackageName());

This flag was introduced in Android 1.5.


Actual results:

For each startActivity(intent) a new tab is opened.


Expected results:

Like in chrome or android stock browser, the same tab should be used, if Browser.EXTRA_APPLICATION_ID is provided.

Android documentation about this: http://developer.android.com/reference/android/provider/Browser.html#EXTRA_APPLICATION_ID
Stackoverflow about this: http://stackoverflow.com/questions/5489086/open-an-url-in-android-browser-avoid-multiple-tabs

Comment 1

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

Note: we should be careful how the TabQueueDispatcher might affect this flow.

Comment 2

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

Created a test Android application for testing Browser.EXTRA_APPLICATION_ID:

  https://github.com/mcomella/ExtraApplicationId

Comment 3

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

From using my test application (assuming it's correct), Chrome on L (i.e. tabs are in the recent apps switcher) does not implement this behavior while both the stock browser and Chrome on my Galaxy Nexus (4.2?) implement the behavior. Notes:
  * Links without EXTRA_APPLICATION_ID are created in a new tab
  * Links with EXTRA_APPLICATION_ID are created in the same tab (I didn't try with different EXTRA_APPLICATION_IDs)
  * If the user presses a link in the EXTRA_APPLICATION_ID tab, ^ still applies. If the user navigates to a new page via the URL bar, ^ no longer applies (i.e. a new tab is created for the given EXTRA_APPLICATION_ID).
  * The same EXTRA_APPLICATION_ID tab will be used when bookmark or history items are loaded (which seems incorrect to me).

Comment 4

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

Attachment: MozReview Request: bz://840574/mcomella

/r/7911 - Bug 840574 - Add applicationId to tabs; use it to open new urls in the correct tabs. r=margaret

Pull down this commit:

hg pull -r 7b769f74c010d228b542b5b57644c0015b04184e https://reviewboard-hg.mozilla.org/gecko/

Comment 5

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

Please note that the push in comment 4 requires more changesets to fix this issue - see the extended commit summary for the remaining TODOs.

Comment 6

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

Comment on attachment 8599605 [details]
MozReview Request: bz://840574/mcomella

/r/7911 - Bug 840574 - Add applicationId to tabs; use it to open new urls in the correct tabs. r=margaret

Pull down this commit:

hg pull -r 51e8d181abff4d9f248bc06d0b84416f8eb8efeb https://reviewboard-hg.mozilla.org/gecko/

Comment 7

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

I just wanted to point out that we support a form of this "tab pinning" already. When a webpage is "Add to the Home Screen" it creates a shortcut that will open the webpage in Firefox when launched. When opening, we "pin" the new tab to the origin URL of the shortcut. Any subsequent launches of the shortcut will just select the existing tab, not create a new tab.

Bug 1132591 is about adding support for "unpinning" the tab if the user navigates away from the domain of the origin URL.

So, can/should we integrate this new feature into that framework?

Comment 8

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

(In reply to Mark Finkle (:mfinkle) from comment #7)
> I just wanted to point out that we support a form of this "tab pinning"
> already.
> 
> So, can/should we integrate this new feature into that framework?

It seems the primary logic for adding the "appOrigin" tag is in addTab [1]:

1117     let pinned = "pinned" in aParams ? aParams.pinned : false;
1118     if (pinned) {
1119       let ss = Cc["@mozilla.org/browser/sessionstore;1"].getService(Ci.nsISessionStore);
1120       ss.setTabValue(newTab, "appOrigin", aURI);
1121     }

While the restoration logic is in _getBrowser [2]:

3243     if (aURI && aWhere == Ci.nsIBrowserDOMWindow.OPEN_SWITCHTAB) {
3244       pinned = true;
3245       let spec = aURI.spec;
3246       let tabs = BrowserApp.tabs;
3247       for (let i = 0; i < tabs.length; i++) {
3248         let appOrigin = ss.getTabValue(tabs[i], "appOrigin");
3249         if (appOrigin == spec) {
3250           let tab = tabs[i];
3251           BrowserApp.selectTab(tab);
3252           return tab.browser;
3253         }
3254       }
3255     }

A notable difference is that when the "appOrigin" tag is matched, we don't load the new URL passed to _getBrowser but rather just switch to the tab. Assuming we don't take the fragile approach of parsing the stored "appOrigin" to see if it's a java package (i.e. an android app), it'd make the most sense to pass another flag (e.g. appOriginLoadNewUrl) from Java to be read alongside appOrigin and load the new url if it's received. A bit messy, but probably better than adding duplicate logic (with room for new bugs!) in Java.

However, clearing the application ID only makes sense when the user types in a new location change here, while bug 1132591 seems better suited to navigating away from the appOrigin (i.e. domain) of the homescreen shortcut. These use cases seem different enough that managing it with a flag seems tedious and might be better to just having another stored value (e.g. "appOrigin" and "applicationId").

To be fair, I'm unclear where the loadURI and loadURIWithFlags (which call _getBrowser) are called from and why - are they called on all URI loads?

I also have to do some research into how this would interact with the remaining edge cases (relevant info copied from the extended commit description):
 * ACTION_LOAD Intents
 * Tab queue
 * Tab restoration

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js?rev=4b9b12c248dc#1120
[2]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js?rev=4b9b12c248dc#3248

Comment 9

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

After discussing w/ Finkle & co. on IRC, the benefits of JS would be easier access to SessionStore, potentially less code duplication with appOrigin, and clearer integration/separation between appOrigin and applicationId. 

However, since the appOrigin scheme doesn't precisely fit into applicationId (see comment 8), the latter two points are less relevant and Java seems the better choice - the applicationId should be cleared due to explicit user interaction - which Java handles - and can avoid the complications of entangling the applicationId (an Android concept) with Gecko.

Comment 10

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

Comment on attachment 8599605 [details]
MozReview Request: bz://840574/mcomella

/r/7911 - Bug 840574 - Add applicationId to tabs; use it to open new urls in the correct tabs. r=margaret
/r/8069 - Bug 840574 - Clear the selected tab's applicationId on user action. r=margaret

Pull down these commits:

hg pull -r f8375595b09b7dc934db12ce483dd10e67c4168b https://reviewboard-hg.mozilla.org/gecko/

Comment 11

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

I don't need to handle ACTION_LOAD - it appears to be used to handle Youtube URLs from internal events (I filed bug 1160692 because I think that should be changed to ACTION_VIEW). Since we're handling external URLs only (i.e. this intent extra only comes from external Android apps), it should not effect this flow.

Comment 12

[:Margaret Leibovic]

https://reviewboard.mozilla.org/r/7911/#review6903

This code is so gnarly... this looks good to me. I like that this minimizes the scope to only adding this new functionality, but this logic could probably use some auditing/clean-up. Do you think we could write a Robocop test for any of this? Can we pass intents to the activity we're testing? Or maybe we can just write a bunch of unit tests for Tabs#loadUrl, passing it all sorts of different combinations of parameters.

Comment 13

[:Margaret Leibovic]

https://reviewboard.mozilla.org/r/8069/#review6905

Your commit message talks about delayLoad and selected, but I don't see any changes in here that do anything with that. Are you missing some changes?

This seems fine to me, but let's make sure we're thinking through all the cases where we should be resetting this.

::: mobile/android/base/BrowserApp.java:2209
(Diff revision 1)
> +        clearSelectedTabApplicationId();

Is commitEditingMode really the only place we need to reset this? Does that get called if the user selects an item from one of the home panels? Or what about link navigation?

Comment 14

[:Margaret Leibovic]

Comment on attachment 8599605 [details]
MozReview Request: bz://840574/mcomella

https://reviewboard.mozilla.org/r/7909/#review6907

Ship It!

Comment 15

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

https://reviewboard.mozilla.org/r/8069/#review6917

> Your commit message talks about delayLoad and selected, but I don't see any changes in here that do anything with that. Are you missing some changes?

This change got moved to the first commit and I forgot to update the message.

> Is commitEditingMode really the only place we need to reset this? Does that get called if the user selects an item from one of the home panels? Or what about link navigation?

> Is commitEditingMode really the only place we need to reset this?

`clearSelectedTabApplicationId` is also called in onUrlOpen which handles...
> Does that get called if the user selects an item from one of the home panels?

Yep.
> Or what about link navigation?

The stock browser does not clear the ID when clicking links so I followed that paradigm. The stock browser only clears when entering new urls (i.e. not when clicking history/bookmarks). However, I thought it makes sense for the value to be cleared when any url is explicitly chosen by the user (i.e. not selecting links), especially because all of this context is available via the awesomescreen, so I implemented that. To be explicit, any action from the awesomescreen besides cancel should clear the application ID and other actions should not cancel the selected tab.

Comment 16

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

https://reviewboard.mozilla.org/r/7911/#review6919

> Do you think we could write a Robocop test for any of this?

I'll follow a follow-up.

Comment 17

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

https://hg.mozilla.org/integration/fx-team/rev/e665a4452667
https://hg.mozilla.org/integration/fx-team/rev/95a7cebd3385

Comment 18

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

Landed the initial implementation. If this works as expected, we can work on the followups (will file):
  * Testing to ensure applicationId works as expected (Robocop - either unit test loadUrl or a full robocop test)
  * Handling tab queues
  * Handling tab restoration

Comment 19

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

https://hg.mozilla.org/mozilla-central/rev/e665a4452667
https://hg.mozilla.org/mozilla-central/rev/95a7cebd3385

Comment 21

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

Attachment: MozReview Request: Bug 840574 - Clear the selected tab's applicationId on user action. r=margaret

Comment 22

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

Attachment: MozReview Request: Bug 840574 - Add applicationId to tabs; use it to open new urls in the correct tabs. r=margaret

Comment 23

[Sylvestre Ledru [:Sylvestre]]

Ditto :)

Release Note Request (optional, but appreciated)
[Why is this notable]:
[Suggested wording]:
[Links (documentation, blog post, etc)]:

Comment 24

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

Leave Margaret's NI to verify.

Release Note Request (optional, but appreciated)
[Why is this notable]:
  Applications that open the browser and specify EXTRA_APPLICATION_ID will attempt to open any links with this ID in the same tab - this can help make the application -> web browser experience more seamless. Chrome and the stock browser support this functionality.

[Suggested wording]:
  Open links from Android applications in the same tab via [EXTRA_APPLICATION_ID](http://developer.android.com/reference/android/provider/Browser.html#EXTRA_APPLICATION_ID)

[Links (documentation, blog post, etc)]:
  http://developer.android.com/reference/android/provider/Browser.html#EXTRA_APPLICATION_ID

Comment 25

[:Margaret Leibovic]

Sounds good to me :)

Comment 26

[Sylvestre Ledru [:Sylvestre]]

Added to the 40 release notes with Michael wording. Thanks!

Comment 27

[laur.catalin]

Still not working on Android 6.0.1 with the latest version of Firefox browser 51.0.3. Any updates of this issue?

Comment 28

[Kevin Brosnan [Ex-Mozilla]]

The reporter is not a useful person to needinfo in this case. It is possible that this regressed. If you believe it did then file a new bug https://bugzilla.mozilla.org/enter_bug.cgi?product=Firefox%20for%20Android

Comment 29

[aditi24.bhatnagar]

Hello,

I am still seeing this issue in the latest version of Firefox on Android, can anyone please guide how can I re-open this bug?

Thanks

Comment 30

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

This is not something we will address in Fennec right now. Would you mind giving Firefox Preview (Fenix) a try and let us know if that application works as expected?