The default bug view has changed. See this FAQ.

Add ability to read profiles.ini

RESOLVED FIXED in Firefox 15

Status

()

Firefox for Android
General
P3
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: wesj, Assigned: wesj)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 16
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox15 fixed, firefox16 fixed, fennec11+)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

5 years ago
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.
Assignee: nobody → wjohnston
Priority: -- → P3
tracking-fennec: --- → 11+
Blocks: 749072
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
Assignee: wjohnston → mark.finkle
Attachment #619043 - Flags: feedback?(wjohnston)
I have not added any code for Sync to get the default profile yet.
Bug 707123 tracks the ability to expose profile information to Sync.
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).
(Assignee)

Comment 5

5 years ago
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...
Attachment #619043 - Flags: feedback?(wjohnston) → feedback+
Created attachment 628007 [details] [diff] [review]
slightly updated patch to m-c

slightly updated patch; needed for my patch on bug 740586.
Assignee: mark.finkle → wjohnston
Blocks: 740586
(Assignee)

Comment 7

5 years ago
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...
Attachment #619043 - Attachment is obsolete: true
Attachment #628007 - Attachment is obsolete: true
Attachment #629904 - Flags: review?(mark.finkle)
(Assignee)

Comment 8

5 years ago
Comment on attachment 629904 [details] [diff] [review]
Patch v1

Grr.. last minute change broke this. Fixings...
Attachment #629904 - Flags: review?(mark.finkle)
(Assignee)

Comment 9

5 years ago
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.
Attachment #629904 - Attachment is obsolete: true
Attachment #630279 - Flags: review?(mark.finkle)
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.
Attachment #630279 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 11

5 years ago
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...
(Assignee)

Comment 12

5 years ago
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.
(Assignee)

Comment 13

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/802fbaed034c
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
(Assignee)

Comment 15

5 years ago
Good call. Thanks!
FYI, troboprovider did indeed go green after backing out.
(Assignee)

Comment 17

5 years ago
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
Attachment #633366 - Flags: review?(lucasr.at.mozilla)
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.
Attachment #633366 - Flags: review?(lucasr.at.mozilla) → review+
(Assignee)

Comment 19

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/469d69e2b822
https://hg.mozilla.org/integration/mozilla-inbound/rev/76a114d3f4d1
https://hg.mozilla.org/mozilla-central/rev/76a114d3f4d1
https://hg.mozilla.org/mozilla-central/rev/469d69e2b822
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16
On startup i now see "WESJ - SET DEFAULT" in my logcat :)
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
Attachment #630279 - Flags: approval-mozilla-aurora?
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
Attachment #633366 - Flags: approval-mozilla-aurora?
(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.
(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 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.
Attachment #630279 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Attachment #633366 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/b0a1b1e07bae
https://hg.mozilla.org/releases/mozilla-aurora/rev/3f34a60b6577
status-firefox15: --- → fixed
status-firefox16: --- → fixed
You need to log in before you can comment on or make changes to this bug.