1121318 - HelperApps.getAppsForUri should ignore well-known internal schemes

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

I think we can ignore "about:" and "chrome:" URI schemes and just return empty arrays. This saves ~60ms in Gecko and two Messaging.sendRequest in Java calls during a about:home startup.

Comment 1

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

Attachment: helper-cleanup v0.1 (part 0)

Simple cleanup and formatting changes

Comment 2

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

Attachment: helper-ignore-schemes v0.1

Adds a simple check for "about:" and "chrome:" and returns empty [] either directly or via the callback.

Comment 3

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

I am also considering creating a simple cache to avoid Java side lookups for URIs, since many times pages with the same domain (https://twitter.com or http://google.com) are loaded and we could fast path the return of handlers.

This would assume that we don't need to hold the entire URI spec in the cache. We'd need to look at the ways Android Manifests allow URI based matching. If only the scheme and domain are needed, we could get a nice perf bump for little cache size.

Comment 4

[Richard Newman [:rnewman]]

Comment on attachment 8548623 [details] [diff] [review]
helper-ignore-schemes v0.1

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

(Assuming Wes isn't working late, and I am.)

::: mobile/android/modules/HelperApps.jsm
@@ +103,5 @@
>    },
>  
>    getAppsForUri: function getAppsForUri(uri, flags = { }, callback) {
> +    // Return early for well-known internal schemes
> +    if (uri.schemeIs("about") || uri.schemeIs("chrome")) {

In an abundance of caution, because not every call site checks: add a null check to this clause.

Comment 5

[Richard Newman [:rnewman]]

Comment on attachment 8548622 [details] [diff] [review]
helper-cleanup v0.1 (part 0)

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

::: mobile/android/modules/HelperApps.jsm
@@ +177,1 @@
>        

Might as well kill this trailing whitespace if you're doing cleanup.

Comment 6

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

With comments addressed:
remote:   https://hg.mozilla.org/integration/fx-team/rev/7b853be9e118
remote:   https://hg.mozilla.org/integration/fx-team/rev/c379987d0266

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7b853be9e118
https://hg.mozilla.org/mozilla-central/rev/c379987d0266