Last Comment Bug 715307 - Add ability to read profiles.ini
: Add ability to read profiles.ini
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All All
: P3 normal (vote)
: Firefox 16
Assigned To: Wesley Johnston (:wesj)
:
: Sebastian Kaspari (:sebastian)
Mentors:
Depends on:
Blocks: 749072 740586
  Show dependency treegraph
 
Reported: 2012-01-04 13:09 PST by Wesley Johnston (:wesj)
Modified: 2012-07-03 14:15 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
11+


Attachments
WIP 1 (10.70 KB, patch)
2012-04-27 08:15 PDT, Mark Finkle (:mfinkle) (use needinfo?)
wjohnston2000: feedback+
Details | Diff | Splinter Review
slightly updated patch to m-c (6.50 KB, patch)
2012-05-29 10:13 PDT, Vladimir Vukicevic [:vlad] [:vladv]
no flags Details | Diff | Splinter Review
Patch v1 (26.81 KB, patch)
2012-06-04 13:32 PDT, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
Patch v2 (28.36 KB, patch)
2012-06-05 12:38 PDT, Wesley Johnston (:wesj)
mark.finkle: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Test fix (1.96 KB, patch)
2012-06-14 19:22 PDT, Wesley Johnston (:wesj)
lucasr.at.mozilla: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Wesley Johnston (:wesj) 2012-01-04 13:09:22 PST
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?) 2012-04-27 08:15:30 PDT
Created attachment 619043 [details] [diff] [review]
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?) 2012-04-27 08:16:37 PDT
I have not added any code for Sync to get the default profile yet.
Comment 3 Lucas Rocha (:lucasr) 2012-04-27 10:07:31 PDT
Bug 707123 tracks the ability to expose profile information to Sync.
Comment 4 Richard Newman [:rnewman] 2012-04-27 10:12:04 PDT
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) 2012-05-01 15:44:03 PDT
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] 2012-05-29 10:13:05 PDT
Created attachment 628007 [details] [diff] [review]
slightly updated patch to m-c

slightly updated patch; needed for my patch on bug 740586.
Comment 7 Wesley Johnston (:wesj) 2012-06-04 13:32:27 PDT
Created attachment 629904 [details] [diff] [review]
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) 2012-06-04 13:46:24 PDT
Comment on attachment 629904 [details] [diff] [review]
Patch v1

Grr.. last minute change broke this. Fixings...
Comment 9 Wesley Johnston (:wesj) 2012-06-05 12:38:22 PDT
Created attachment 630279 [details] [diff] [review]
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?) 2012-06-07 07:04:04 PDT
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) 2012-06-07 10:30:32 PDT
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) 2012-06-07 15:11:42 PDT
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 14 Ryan VanderMeulen [:RyanVM] 2012-06-09 11:37:16 PDT
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) 2012-06-09 11:45:37 PDT
Good call. Thanks!
Comment 16 Ryan VanderMeulen [:RyanVM] 2012-06-09 14:38:06 PDT
FYI, troboprovider did indeed go green after backing out.
Comment 17 Wesley Johnston (:wesj) 2012-06-14 19:22:41 PDT
Created attachment 633366 [details] [diff] [review]
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) 2012-06-15 03:31:27 PDT
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 21 Kartikaya Gupta (email:kats@mozilla.com) 2012-06-22 10:10:37 PDT
On startup i now see "WESJ - SET DEFAULT" in my logcat :)
Comment 22 Mark Finkle (:mfinkle) (use needinfo?) 2012-06-28 07:45:09 PDT
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?) 2012-06-28 07:45:51 PDT
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] 2012-07-02 16:26:52 PDT
(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?) 2012-07-02 19:17:05 PDT
(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] 2012-07-03 11:15:06 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.