HelperApps.getAppsForUri should ignore well-known internal schemes

RESOLVED FIXED in Firefox 38

Status

()

Firefox for Android
General
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mfinkle, Assigned: mfinkle)

Tracking

({perf})

unspecified
Firefox 38
ARM
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

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.
Created attachment 8548622 [details] [diff] [review]
helper-cleanup v0.1 (part 0)

Simple cleanup and formatting changes
Assignee: nobody → mark.finkle
Attachment #8548622 - Flags: review?(wjohnston)
Created attachment 8548623 [details] [diff] [review]
helper-ignore-schemes v0.1

Adds a simple check for "about:" and "chrome:" and returns empty [] either directly or via the callback.
Attachment #8548623 - Flags: review?(wjohnston)
(Assignee)

Updated

4 years ago
Keywords: perf
OS: Mac OS X → Android
Hardware: x86 → ARM
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 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.
Attachment #8548623 - Flags: review?(wjohnston) → review+
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.
Attachment #8548622 - Flags: review?(wjohnston) → review+
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/7b853be9e118
https://hg.mozilla.org/mozilla-central/rev/c379987d0266
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
You need to log in before you can comment on or make changes to this bug.