1244071 - Restrictions: Do only one disk read

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

Observation from bug 1239376: We are reading the application restrictions twice: Once in Restrictions.java to determine if we are a restricted profile and a second time to create the cache in RestrictedProfileConfiguration. As every call to UserManager.getApplicationRestrictions() reads and parses an XML file and those reads happen on the UI thread we should try to optimize this to just do one read.

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: MozReview Request: Bug 1244071 - Move reading of restrictions to dedicated cache class. r?mcomella

Review commit: https://reviewboard.mozilla.org/r/33093/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33093/

Comment 2

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

Comment on attachment 8714463 [details]
MozReview Request: Bug 1244071 - Move reading of restrictions to dedicated cache class. r?mcomella

https://reviewboard.mozilla.org/r/33093/#review29889

The cache invalidation if we're not on the Ui thread seems questionable to me, otherwise looks good – nice cleanup.

If you decide there's a better way to invalidate than that, I'd like to see it before you land.

::: mobile/android/base/java/org/mozilla/gecko/restrictions/RestrictionCache.java:20
(Diff revision 1)
> +public class RestrictionCache {

I feel like we should move `Restrictions.java` to the `restriction` package and make this package protected.

::: mobile/android/base/java/org/mozilla/gecko/restrictions/RestrictionCache.java:52
(Diff revision 1)
> +        if (isCacheInvalid || !ThreadUtils.isOnUiThread()) {

Why do we want to always update the cache if we're not the UI thread?

::: mobile/android/base/java/org/mozilla/gecko/restrictions/RestrictionCache.java:62
(Diff revision 1)
> +        StrictMode.ThreadPolicy policy = StrictMode.allowThreadDiskReads();

nit: `final`

I generally prefer to push the strictmode calls off to the caller so they're aware of the ui thread impacts of their call. However, since we're caching this, it doesn't matter so much. Might be worth adding a comment explaining this.

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

https://reviewboard.mozilla.org/r/33093/#review29889

> I feel like we should move `Restrictions.java` to the `restriction` package and make this package protected.

Yeah, that's something I wanted to do too (package protected). The reason I did not move Restrictions so far is that it has some JNI entry points (@WrapForJNI) and this is a bit more "costly" than just moving the class. But I could file a bug to do this (bug 1245044).

> Why do we want to always update the cache if we're not the UI thread?

This is the same behavior like before! The reason is that we wouldn't cache any restrictions at all if the implementation on Android's side would be a bit smarter - like SharedPreferences. The primary goal was to avoid having stale values as much as possible and not block the UI thread if we can. On a background thread the read is negligible. See bug 1189347.

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8714463 [details]
MozReview Request: Bug 1244071 - Move reading of restrictions to dedicated cache class. r?mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33093/diff/1-2/

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=135c9af98202

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

https://hg.mozilla.org/integration/fx-team/rev/7f9f6cc8991de19a0bc932cdb9d717a7df444f27
Bug 1244071 - Move reading of restrictions to dedicated cache class. r=mcomella

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7f9f6cc8991d