715307 - Add ability to read profiles.ini

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P3)

Description

[Wesley Johnston (:wesj)]

There are a few cases where we need the ability to read profiles.ini in Java. For instance, to detect the correct profile to use as default when using a local database. We should add support for reading the ini files and modifying them from java.

Comment 1

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

Attachment: WIP 1

This patch adds:
* code to support reading the default profile from profiles.ini
* explicitly initializes the local DB so we can pass the profile name to the DB

Comment 2

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

I have not added any code for Sync to get the default profile yet.

Comment 3

[Lucas Rocha (:lucasr)]

Bug 707123 tracks the ability to expose profile information to Sync.

Comment 4

[Richard Newman [:rnewman]]

By the time Fennec supports multiple profiles, Sync will/should be explicitly naming the profile on every CP request.

So we'll only need to know the set of profile names during setup ("which profile do you want to use with this Sync account?"), and to have the current profile name passed in if setup is launched from inside Fennec (to avoid that picker).

Comment 5

[Wesley Johnston (:wesj)]

Comment on attachment 619043 [details] [diff] [review]
WIP 1

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

Seems like a good start, but I worry this will negatively impact startup. Would be good to measure numbers locally as much as possible and know what we're getting into before we land.

::: mobile/android/base/GeckoProfile.java
@@ +68,5 @@
> +                }
> +                bufferedReader.close();
> +                Log.i(LOGTAG, "***** " + profile);
> +                if (foundDefault)
> +                    profileName = profile;

This scares me. I guess we just hope that Default always follows the Name row in profiles.ini? Since we write it, that's probably the case.

I'd rather we had a dedicated reader class (INIReader.java), and maybe a way for to hold a really lightweight list (name and File) of uninitialized GeckoProfiles that we could store. Overkill....? We'll need something like that for profile switching anyway....

@@ +79,5 @@
>              GeckoProfile profile = sProfileCache.get(profileName);
>              if (profile == null) {
>                  profile = new GeckoProfile(context, profileName);
>                  sProfileCache.put(profileName, profile);
>              }

Can we cache these based on the Context passed in somehow? Maybe we do? I don't want to do this lookup in the ini file everytime someone asks for the profile. Ignoring changes to profiles.ini while Gecko is running seems reasonable to me (at least until we can flip profiles without restarting Gecko). Then again, with ContentProviders, these may remain cached even when Gecko is closed...

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: slightly updated patch to m-c

slightly updated patch; needed for my patch on bug 740586.

Comment 7

[Wesley Johnston (:wesj)]

Attachment: Patch v1

This creates an INIParser/INISection classes to read and write ini files. It basically contains two hash tables.

1.) properties <String, Object> (in both a Section and the parser classes)
2.) sections<String, INISection> (only in the parser class)

Then we just use them to read and write the defaults + new profiles + some of the work mfinkle did originally to set up the default better.

I'm not hugely attached to these classes and it feels like a lot of code to do a little. Maybe the other approach is better...

Comment 8

[Wesley Johnston (:wesj)]

Comment on attachment 629904 [details] [diff] [review]
Patch v1

Grr.. last minute change broke this. Fixings...

Comment 9

[Wesley Johnston (:wesj)]

Attachment: Patch v2

Still a bit worried about this doing IO at startup. We can do something more lo-fi mabye, but wanted to put this up.

Comment 10

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

Comment on attachment 630279 [details] [diff] [review]
Patch v2

>diff --git a/mobile/android/base/INIParser.java b/mobile/android/base/INIParser.java

I think we need to work on our folder structure a bit. We are using "base" as a kitchen sink. That's a new bug though.

>diff --git a/mobile/android/base/INISection.java b/mobile/android/base/INISection.java
>\ No newline at end of file

Add a newline

My first reaction to the INI classes is "too much code for too little value", but it's not that much code and there is no harm in making a convenient helper for INI parsing.

We do need tests for the INIParser itself.

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>         DOMApplicationRegistry.getManifestFor(data.origin, (function(aManifest) {
>-	   if (!aManifest)
>-	     return;
>+          if (!aManifest)
>+            return;

>         DOMApplicationRegistry.getManifestFor(data.origin, (function(aManifest) {
>-	   if (!aManifest)
>-	     return;
>+          if (!aManifest)
>+            return;

Extra cruft?

r+, but let's get some tests for INIParser and do some Try builds to see if startup is affected.

Comment 11

[Wesley Johnston (:wesj)]

Pushed to try without: https://tbpl.mozilla.org/?tree=Try&rev=e7ada2f80ecd
and with patch: https://tbpl.mozilla.org/?tree=Try&rev=ed42840ebfd9

Should post back here when done...

Comment 12

[Wesley Johnston (:wesj)]

Test - Result w/ patch - Result w/o patch
dh   - 889.79  - 895.65
rck  - 1497.58 - 814.22
rck2 - 3889.41 - 3065.16
sp   - 72.42   - 73.36
s    - 7587.5  - 7673.41
tpn  - 642.05  - 636.2
ts   - 2631.05 - 2662.95

i.e. I'm mainly worried about ts and it looks the same with this patch. I'll land.

Comment 13

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/802fbaed034c

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Sorry, but I backed this out due to rpr not having a green run since this landed on inbound yesterday (looks like the Try run in comment 11 may have had the same issue). I will re-land it if things don't go green.
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd714857c954

Comment 15

[Wesley Johnston (:wesj)]

Good call. Thanks!

Comment 16

[Ryan VanderMeulen [:RyanVM]]

FYI, troboprovider did indeed go green after backing out.

Comment 17

[Wesley Johnston (:wesj)]

Attachment: Test fix

I think this fixes the test that failed with this test. Maybe its better to put this in the ContentProvider base class?

Looked good on try (once, as opposed to the constant failure we saw before0, but I forgot I left some mochitest changes in that I needed to test this locally. Will push again...

https://tbpl.mozilla.org/?tree=Try&rev=8f937262aa59

Comment 18

[Lucas Rocha (:lucasr)]

Comment on attachment 633366 [details] [diff] [review]
Test fix

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

I assume no further changes are necessary on BrowserProvider stuff.

Comment 19

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/469d69e2b822
https://hg.mozilla.org/integration/mozilla-inbound/rev/76a114d3f4d1

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/76a114d3f4d1
https://hg.mozilla.org/mozilla-central/rev/469d69e2b822

Comment 21

[Kartikaya Gupta (email:[e-mail])]

On startup i now see "WESJ - SET DEFAULT" in my logcat :)

Comment 22

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

Comment on attachment 630279 [details] [diff] [review]
Patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): native rewrite
User impact if declined: limits ability of add-ons to manipulate profiles. add-ons could start experimenting with 'guest mode' and 'private browsing' ideas.
Testing completed (on m-c, etc.): it's been on m-c for a while
Risk to taking this patch (and alternatives if risky): given the testing, i think the risk is low
String or UUID changes made by this patch: none

Comment 23

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

Comment on attachment 633366 [details] [diff] [review]
Test fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: needed for fixing the tests if we upload the main patch
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none

Comment 24

[Alex Keybl [:akeybl]]

(In reply to Mark Finkle (:mfinkle) from comment #22)
> User impact if declined: limits ability of add-ons to manipulate profiles.
> add-ons could start experimenting with 'guest mode' and 'private browsing'
> ideas.

How functional would an add-on guest mode based upon this work be? If this doesn't enable a compelling add-on to be created in FF15, it's unclear why we would approve. If this is just a matter of experimenting, I assume that could occur on nightly.

Comment 25

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

(In reply to Alex Keybl [:akeybl] from comment #24)
> (In reply to Mark Finkle (:mfinkle) from comment #22)
> > User impact if declined: limits ability of add-ons to manipulate profiles.
> > add-ons could start experimenting with 'guest mode' and 'private browsing'
> > ideas.
> 
> How functional would an add-on guest mode based upon this work be? If this
> doesn't enable a compelling add-on to be created in FF15, it's unclear why
> we would approve. If this is just a matter of experimenting, I assume that
> could occur on nightly.

With this patch uploaded, I could make a simple guest mode add-on that creates a temp profile, restarts Firefox and starts the user browsing in the guest profile. When finished, it would revert back to the last profile and remove the temp guest profile.

I have also ported my "Mobile Profiles" add-on to Native Fennec using this patch. That add-on allows users to create and switch between several profiles.

Comment 26

[Alex Keybl [:akeybl]]

Comment on attachment 630279 [details] [diff] [review]
Patch v2

[Triage Comment]
Thanks - sounds like a big win with little engineering risk and no l10n impact. Approved for Aurora 15.

Comment 27

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/b0a1b1e07bae
https://hg.mozilla.org/releases/mozilla-aurora/rev/3f34a60b6577