Closed Bug 1356517 Opened 3 years ago Closed 3 years ago

Implement a centralized Deep Link handling mechanism

Categories

(Firefox for Android :: General, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: cnevinchen, Assigned: cnevinchen)

References

(Blocks 1 open bug)

Details

(Whiteboard: [LP_M1])

Attachments

(1 file, 4 obsolete files)

We will need deep link features to support marketing requirements.
Since we don't want the logic to be everywhere, we need a centralized Deep Link handling mechanism for easy debugging, testing, updating.
Blocks: 1356386
Attachment #8858270 - Flags: feedback?(walkingice0204)
Attachment #8858270 - Flags: feedback?(topwu.tw)
Attachment #8858270 - Flags: feedback?(max)
Assignee: nobody → cnevinchen
Blocks: 1351571
removing tracking nom as it will be tracked under lean plum meta bug.
tracking-fennec: ? → ---
Attachment #8859519 - Attachment is obsolete: true
Attachment #8858270 - Flags: review?(s.kaspari) → review?(gkruglov)
Attachment #8858270 - Flags: review?(s.kaspari) → review?(gkruglov)
Attachment #8860948 - Flags: review?(s.kaspari) → review?(gkruglov)
Attachment #8860949 - Flags: review?(s.kaspari) → review?(gkruglov)
Comment on attachment 8858270 [details]
Bug 1356517 - Add deep links to handle events.

https://reviewboard.mozilla.org/r/130256/#review135964

I'm missing context necessary to properly review these changes.

What are you trying to achieve here: what are your requirements? What kind of deep links will we need to handle, and what kinds does this patch attempt to handle? Why do you feel this needs to spill significantly outside of LauncherActivity at all? E.g. I feel that most if not all of the complexity here is unnecessary, but I don't quite understand your restrictions either.

::: mobile/android/base/java/org/mozilla/gecko/LauncherActivity.java:153
(Diff revision 6)
> +        if (deepLink == null) {
> +            return false;
> +        }
> +
> +        if (deepLink.equalsIgnoreCase(DeepLinkAction.TYPE.DEFAULT_BROWSER.name())) {
> +            DeepLinkStore.dispatch(new DeepLinkAction(DeepLinkAction.TYPE.DEFAULT_BROWSER, deepLink));

Don't do this, please. Split this into two methods, one to check if something is a "deep link", and one to handle it - like the rest of this file does.

Otherwise, your method name is wrong, something that is supposed to be a simple pure function now has a huge side-effect, making it impossible to write a test for, the `if` statemenets using it are entirely misleading, etc.

::: mobile/android/base/java/org/mozilla/gecko/deeplink/DeepLinkStore.java:13
(Diff revision 6)
> +
> +
> +import java.util.ArrayList;
> +import java.util.HashMap;
> +
> +final public class DeepLinkStore {

Can you explain to me why this is necessary at all?

You add an incoming deeplinkaction to a store, and handle this right away.

To me a deep link is something that is processed on one-by-one basis as soon as one comes in. Since they're processed right away, we can simply dispatch it. There is no sensible "dispatch failed, so we'll try again in the future" scenario here.
Attachment #8858270 - Flags: review?(gkruglov) → review-
Sorry I didn't make it clear.

I'm adding butch of complexity for the deep link is mainly for Lean Plum integration.
For this bug, it's only the first step.

We'll add deep links that need to wait for other events or states.

Possible items are...
==========
Encourage users to set Firefox as Default browser
Remind users of saved bookmarks
Save as PDF
Reader mode feature is broken,  so below can't be done
Remind users of saved articles
Reader mode
Add to homescreen
Sync Promotion - Passwords
*Sync Promotion - Bookmarks
Tab Queues
*Private Browsing
PWA
Share feature
Inform user to turn off tab queue
How to use tab queue
*Add-Ons promotion
Ad blocker
Video downloader
Focus promotion
Perform surveys
Send users to Google Play for Reviews

e.g. If I want to open BrowserApp first,  then SettingsAcitivity, then open the second level page (e.g. Privacy Setting)  , then I'll need to pass those arguments around in BrowserApp , wait for "Settings:Show" event from Gecko, open GeckoPreferences, then shows the Settings...

I want to make all the state change only happens in the Store, not in BrowserApp or  GeckoPreferences , or components. They should just response to whatever they should execute.
Flags: needinfo?(gkruglov)
Attachment #8860778 - Attachment is obsolete: true
Attachment #8860778 - Flags: review?(gkruglov)
Attachment #8860948 - Attachment is obsolete: true
Attachment #8860948 - Flags: review?(gkruglov)
Attachment #8860949 - Attachment is obsolete: true
Attachment #8860949 - Flags: review?(gkruglov)
Hi Grisha
I've summited another patch for simpler implementation for below events
default_browser
save_as_pdf
bookmark_list
history_list
sign_up
preferences
preferences_privacy
preferences_search
preferences_notifications
preferences_accessibility


Since we need to land the basic deep link features as soon as possible (for LeanPlum integration), I'm proposing to use this simpler approach first and work on the complex architecure for more complex requirement later.
Comment on attachment 8858270 [details]
Bug 1356517 - Add deep links to handle events.

https://reviewboard.mozilla.org/r/130256/#review136350

::: mobile/android/base/AndroidManifest.xml.in:101
(Diff revision 7)
>                  <category android:name="android.intent.category.BROWSABLE" />
>                  <data android:scheme="http" />
>                  <data android:scheme="https" />
>                  <data android:scheme="about" />
>                  <data android:scheme="javascript" />
> +                <data android:scheme="firefox" />

You probably want to at least wrap this in a IS_NIGHTLY conditional, so that it doesn't ride the trains by accident.

::: mobile/android/base/java/org/mozilla/gecko/LauncherActivity.java:40
(Diff revision 7)
>          GeckoAppShell.ensureCrashHandling();
>  
>          final SafeIntent safeIntent = new SafeIntent(getIntent());
>  
>          // Is this web app?
> -        if (isWebAppIntent(safeIntent)) {
> +        if (isDeepLink(safeIntent)) {

This sounds like a great thing to setup a switchboard experiment for, so that you have remote control over the feature in case you'll want to disable it or enable only for a subset of users.

::: mobile/android/base/java/org/mozilla/gecko/LauncherActivity.java:90
(Diff revision 7)
>          filterFlags(intent);
>  
>          startActivity(intent);
>      }
>  
> +    private void dispatchUtlIntent(String url) {

What's a Utl Intent? Did you mean to say url intent?

::: mobile/android/base/java/org/mozilla/gecko/LauncherActivity.java:92
(Diff revision 7)
>          startActivity(intent);
>      }
>  
> +    private void dispatchUtlIntent(String url) {
> +        Intent intent = new Intent(getIntent());
> +        if (url != null) {

Is there a legit case of passing a null url? Prefer marking the parameter as @NonNull

::: mobile/android/base/java/org/mozilla/gecko/LauncherActivity.java:150
(Diff revision 7)
>      private boolean isCustomTabsEnabled() {
>          return GeckoSharedPrefs.forApp(this).getBoolean(GeckoPreferences.PREFS_CUSTOM_TABS, false);
>      }
> +
> +    private boolean isDeepLink(SafeIntent intent) {
> +        if (intent == null || intent.getData() == null || intent.getData().getScheme() == null) {

You probably also want to check for intent's action, e.g. `Intent.ACTION_VIEW.equals(safeIntent.getAction())`

::: mobile/android/base/java/org/mozilla/gecko/LauncherActivity.java:157
(Diff revision 7)
> +        }
> +        return intent.getData().getScheme().equalsIgnoreCase(getString(R.string.deeplink_scheme));
> +    }
> +
> +    private void dispatchDeepLink(SafeIntent intent) {
> +        if (intent == null || intent.getData() == null || intent.getData().getHost() == null) {

I think it's safe to assume that your intent should be fine to act upon without the added checks.

::: mobile/android/base/java/org/mozilla/gecko/LauncherActivity.java:178
(Diff revision 7)
> +                    url = getString(R.string.pref_default_browser_link);
> +                    dispatchUtlIntent(url);
> +                }
> +                break;
> +            case "save_as_pdf":
> +                EventDispatcher.getInstance().dispatch("SaveAs:PDF", null);

Who's listening for this event? There's probably a constant you can use so that this won't be hardcoded.

::: mobile/android/base/java/org/mozilla/gecko/LauncherActivity.java:189
(Diff revision 7)
> +            case "history_list":
> +                url = AboutPages.getURLForBuiltinPanelType(HomeConfig.PanelType.COMBINED_HISTORY);
> +                dispatchUtlIntent(url);
> +                break;
> +            case "sign_up":
> +                url = "about:accounts?action=signup";

Don't hardcode this. We have constants in AboutPages.ACCOUNTS

::: mobile/android/base/java/org/mozilla/gecko/LauncherActivity.java:193
(Diff revision 7)
> +            case "sign_up":
> +                url = "about:accounts?action=signup";
> +                dispatchUtlIntent(url);
> +                break;
> +            case "preferences":
> +                Intent settingsIntent = new Intent(this, GeckoPreferences.class);

Seems cleaner to take the declaration out of the switch statement.

::: mobile/android/base/java/org/mozilla/gecko/LauncherActivity.java:207
(Diff revision 7)
> +            case "preferences_accessibility":
> +                settingsIntent = new Intent(this, GeckoPreferences.class);
> +                GeckoPreferences.setResourceToOpen(settingsIntent, host);
> +                startActivityForResult(settingsIntent, ACTIVITY_REQUEST_PREFERENCES);
> +                break;
> +

Add a default statement and at the very least log unrecognized deep links.

::: mobile/android/base/strings.xml.in:326
(Diff revision 7)
>    <string name="content_notification_action_read_now">&content_notification_action_read_now;</string>
>    <string name="content_notification_updated_on">&content_notification_updated_on;</string>
>  
>    <string name="pref_default_browser">&pref_default_browser;</string>
>    <string name="pref_default_browser_mozilla_support_tablet">&pref_default_browser_mozilla_support_tablet;</string>
> +  <string name="deeplink_scheme">firefox</string>

Please move this out of this file. A constant in LauncherActivity will be fine.

::: mobile/android/base/strings.xml.in:327
(Diff revision 7)
>    <string name="content_notification_updated_on">&content_notification_updated_on;</string>
>  
>    <string name="pref_default_browser">&pref_default_browser;</string>
>    <string name="pref_default_browser_mozilla_support_tablet">&pref_default_browser_mozilla_support_tablet;</string>
> +  <string name="deeplink_scheme">firefox</string>
> +  <string name="pref_default_browser_link">https://support.mozilla.org/kb/make-firefox-default-browser-android?utm_source=inproduct&amp;utm_medium=settings&amp;utm_campaign=mobileandroid</string>

I don't think this link is correct. Please see how other SUMO urls are formattted - to allow for localization, etc.
Attachment #8858270 - Flags: review?(gkruglov) → review+
Comment on attachment 8858270 [details]
Bug 1356517 - Add deep links to handle events.

https://reviewboard.mozilla.org/r/130256/#review137508

::: mobile/android/base/AndroidManifest.xml.in:101
(Diff revision 7)
>                  <category android:name="android.intent.category.BROWSABLE" />
>                  <data android:scheme="http" />
>                  <data android:scheme="https" />
>                  <data android:scheme="about" />
>                  <data android:scheme="javascript" />
> +                <data android:scheme="firefox" />

We want this feature to work on release channel as well. So I just keep it as is. But please feel free to raise any concern.

::: mobile/android/base/java/org/mozilla/gecko/LauncherActivity.java:40
(Diff revision 7)
>          GeckoAppShell.ensureCrashHandling();
>  
>          final SafeIntent safeIntent = new SafeIntent(getIntent());
>  
>          // Is this web app?
> -        if (isWebAppIntent(safeIntent)) {
> +        if (isDeepLink(safeIntent)) {

Using switchboard is great. But if other feature will depend on this that'll be a problem. I haven't think of a good solution to this. Maybe I'll have different experiements for different deep links instead of using one experiement to switch on/off all deep links.

I'll create another bug for it.

::: mobile/android/base/java/org/mozilla/gecko/LauncherActivity.java:90
(Diff revision 7)
>          filterFlags(intent);
>  
>          startActivity(intent);
>      }
>  
> +    private void dispatchUtlIntent(String url) {

yes :)

::: mobile/android/base/strings.xml.in:327
(Diff revision 7)
>    <string name="content_notification_updated_on">&content_notification_updated_on;</string>
>  
>    <string name="pref_default_browser">&pref_default_browser;</string>
>    <string name="pref_default_browser_mozilla_support_tablet">&pref_default_browser_mozilla_support_tablet;</string>
> +  <string name="deeplink_scheme">firefox</string>
> +  <string name="pref_default_browser_link">https://support.mozilla.org/kb/make-firefox-default-browser-android?utm_source=inproduct&amp;utm_medium=settings&amp;utm_campaign=mobileandroid</string>

This link is sepcial.. normrally it has os/locale/version format. But this looks different. I copied it from our code actually.
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/340b02a5a42c
Add deep links to handle events. r=Grisha
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/340b02a5a42c
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Priority: -- → P1
Flags: needinfo?(gkruglov)
Whiteboard: [LP_M2]
Whiteboard: [LP_M2] → [LP_M1]
You need to log in before you can comment on or make changes to this bug.