707123 - Refactor GeckoProfiles to allow for a profiles ContentProvider to be used by Sync

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Data Providers, defect)

Description

[Lucas Rocha (:lucasr)]

With special queries for:
- Fetching list of available profiles in Fennec
- Fetching which database a certain profile is using (Android or Local)

Or maybe this could be in the BrowserProvider itself. A separate content provider would reduce bloat in BrowserProvider though.

Comment 1

[Lucas Rocha (:lucasr)]

rnewman, does this bug still make sense?

Comment 2

[Richard Newman [:rnewman]]

If we plan to offer multiple profiles on Fennec, then yes, we'll need this (and Sync changes) before it lands. If not, then we can WONTFIX this.

Comment 3

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

Multiple profiles is still on the table

Comment 4

[Richard Newman [:rnewman]]

(In reply to Mark Finkle (:mfinkle) from comment #3)
> Multiple profiles is still on the table

Is there a meta bug that this can block?

Comment 5

[Richard Newman [:rnewman]]

This is gonna be relevant for Sync telemetry/health recording, so picking this up.

Comment 6

[Richard Newman [:rnewman]]

Attachment: Part 1: refactor GeckoProfileDirectories out of GeckoProfile.

Comment 7

[Richard Newman [:rnewman]]

Attachment: Part 2: content provider for Fennec profiles.

Comment 8

[Richard Newman [:rnewman]]

Comment on attachment 8377932 [details] [diff] [review]
Part 1: refactor GeckoProfileDirectories out of GeckoProfile.

Only asking for feedback at this stage, because I haven't tested these.

Comment 9

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

Comment on attachment 8377932 [details] [diff] [review]
Part 1: refactor GeckoProfileDirectories out of GeckoProfile.

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

>+    static INIParser getProfilesINI(File mozillaDir) {
>+        return new INIParser(new File(mozillaDir, "profiles.ini"));
>+    }

We end up calling getProfilesINI from more than a few places. Each time creates a INIPareser and opens a file. I had a patch somewhere that made the parser static. I'm not sure it would be of a lot of benefit or not:
https://bug886925.bugzilla.mozilla.org/attachment.cgi?id=778806

Comment 10

[Richard Newman [:rnewman]]

Next step in this process is to acknowledge that GPD is static, and perhaps the sole arbiter of the contents of this file, and optimize accordingly. Will fork that into a separate bug!

(This is why I love refactoring. Makes stuff so apparent.)

Comment 11

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8377932 [details] [diff] [review]
Part 1: refactor GeckoProfileDirectories out of GeckoProfile.

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

Even with nits and my pedantry, this is an improvement.  SHIP IT

::: mobile/android/base/BrowserApp.java
@@ +2587,5 @@
>      public int getLayout() { return R.layout.gecko_app; }
>  
>      @Override
>      protected String getDefaultProfileName() {
> +        String profile = GeckoProfile.getDefaultProfileName(this);

So, another lame-duck null check.  I see an @Override here, so perhaps maintaining the method (and not inlining GeckoProfile) still has value?

::: mobile/android/base/GeckoProfile.java
@@ +57,5 @@
>          boolean isGeckoApp = false;
>          try {
>              isGeckoApp = context instanceof GeckoApp;
>          } catch (NoClassDefFoundError ex) {}
>          

nit: ws.

@@ +105,5 @@
>  
>          // if no profile was passed in, look for the default profile listed in profiles.ini
>          // if that doesn't exist, look for a profile called 'default'
>          if (TextUtils.isEmpty(profileName) && profileDir == null) {
> +            profileName = GeckoProfile.getDefaultProfileName(context);

Is there ever a time when we'd want |getDefaultProfileName| to return null?  What does this signify other than "use DEFAULT_PROFILE"?

@@ +419,5 @@
>              if (mProfileDir == null) {
>                  return false;
>              }
>  
> +            INIParser parser = GeckoProfileDirectories.getProfilesINI(mozillaDir);

Presumably this is WIP toward managing |profiles.ini| outside of this class?

::: mobile/android/base/GeckoProfileDirectories.java
@@ +13,5 @@
> +import org.mozilla.gecko.util.INISection;
> +
> +import android.content.Context;
> +
> +public class GeckoProfileDirectories {

javadoc explaining role of class, plz.

@@ +20,5 @@
> +    static INIParser getProfilesINI(File mozillaDir) {
> +        return new INIParser(new File(mozillaDir, "profiles.ini"));
> +    }
> +
> +    static String saltProfileName(String name) {

nit: always annotate, or comment about package private.  /me hates package private.

@@ +31,5 @@
> +        salt.append(name);
> +        return salt.toString();
> +    }
> +
> +    public static File getMozillaDirectory(Context context) {

I'm sure this is follow-up, but I find this whole idea of surfacing the "mozilla" directory to be misguided.  What consumer cares?  Should care?

@@ +37,5 @@
> +    }
> +
> +    /**
> +     * Discover the default profile name by examining profiles.ini.
> +     * 

nit: ws throughout.

@@ +54,5 @@
> +
> +      return null;
> +    }
> +
> +    public static Map<String, String> getAllProfiles(final File mozillaDir) {

javadoc, plz.  Also, why emphasize the name -> path mapping?  I'd much rather this be name -> rich struct.  Is this follow-up?

@@ +86,5 @@
> +                return new File(section.getStringProperty("Path"));
> +            }
> +        }
> +
> +        return null; 

Not loving this |null| return.  Investigate how much pain throwing a checked exception here causes.

Comment 12

[Nick Alexander :nalexander [he/him]]

(In reply to Richard Newman [:rnewman] from comment #8)
> Comment on attachment 8377932 [details] [diff] [review]
> Part 1: refactor GeckoProfileDirectories out of GeckoProfile.
> 
> Only asking for feedback at this stage, because I haven't tested these.

So, I guess you would write JUnit 3 tests for this if we landed a JUnit 3 test suite, right?

Comment 13

[Richard Newman [:rnewman]]

(In reply to Nick Alexander :nalexander from comment #12)

> So, I guess you would write JUnit 3 tests for this if we landed a JUnit 3
> test suite, right?

Yessir.

Overall response to review comments: this was as minimal a refactor as I could get away with and have the output look broadly sane. Didn't want it to morph into a rewrite and dramatically change the logic. Yet.

Comment 14

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8377933 [details] [diff] [review]
Part 2: content provider for Fennec profiles.

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

This looks pretty good to me, but of course it needs tests :)  FYI, you can build JUnit 3 tests that run against Fennec by using the BackgroundInstrumentationTests Eclipse package and pretending that the source doesn't come from ABS.

::: mobile/android/base/GeckoProfileProvider.java
@@ +17,5 @@
> +import android.database.MatrixCursor;
> +import android.net.Uri;
> +
> +/**
> + * This is not a per-profile provider. This provider allows read-only,

Maybe call it GeckoProfile*s*Provider, or GeckoProfileSetProvider?

@@ +21,5 @@
> + * This is not a per-profile provider. This provider allows read-only,
> + * restricted access to certain attributes of Fennec profiles.
> + */
> +public class GeckoProfileProvider extends ContentProvider {
> +    static final UriMatcher URI_MATCHER = new UriMatcher(UriMatcher.NO_MATCH);

private?

@@ +104,5 @@
> +        addRowForProfile(cursor, args.length, nameIndex, pathIndex, name, path);
> +        return cursor;
> +    }
> +
> +    protected Cursor getCursorForProfiles(final String[] args, Map<String, String> profiles) {

Why do we have two very similar functions?  Can you javadoc to explain the difference?

@@ +118,5 @@
> +        }
> +
> +        final MatrixCursor cursor = new MatrixCursor(args);
> +        for (Entry<String, String> entry : profiles.entrySet()) {
> +            addRowForProfile(cursor, args.length, nameIndex, pathIndex, entry.getKey(), entry.getValue());

It's unclear to me, in 30 seconds of thinking, what happens if nameIndex and pathIndex are both -1.  Can this not happen?  If not, why not?

@@ +128,5 @@
> +    public Cursor query(Uri uri, String[] projection, String selection,
> +                        String[] selectionArgs, String sortOrder) {
> +
> +        final String[] args = (projection == null) ? DEFAULT_ARGS : projection;
> +

nit: ws, and trailing whitespace throughout.

@@ +166,5 @@
> +
> +    @Override
> +    public int update(Uri uri, ContentValues values, String selection,
> +                      String[] selectionArgs) {
> +        return 0;

throw, or otherwise complain louder.

Comment 15

[Richard Newman [:rnewman]]

(In reply to Nick Alexander :nalexander from comment #11)

> Is there ever a time when we'd want |getDefaultProfileName| to return null? 
> What does this signify other than "use DEFAULT_PROFILE"?

WebAppImpl#getDefaultProfileName returns null to signify that the intent passed in was malformed. But this whole cascade is kind of a mess, and probably buggy (e.g., GeckoProfile#get doesn't include the null check, and so will try to look up `null` in a Map…).

I'll file a follow-up to fix GeckoProfile and this delegate pattern.


> > +            INIParser parser = GeckoProfileDirectories.getProfilesINI(mozillaDir);
> 
> Presumably this is WIP toward managing |profiles.ini| outside of this class?

Yeah. For now, GPD is read-only; this violates encapsulation, but buys us a read/write separation of concerns.

Filed Bug 975212 and commented on the package-scoped methods accordingly.

> > +    static String saltProfileName(String name) {
> 
> nit: always annotate, or comment about package private.  /me hates package
> private.

I made this public, documented it, and made some minor improvements.

> > +    public static File getMozillaDirectory(Context context) {
> 
> I'm sure this is follow-up, but I find this whole idea of surfacing the
> "mozilla" directory to be misguided.  What consumer cares?  Should care?

GeckoProfile, of course; the two classes are still very coupled. It only needs to do so to ensure that this directory exists. I'll move that into GeckoProfileDirectories.


> > +    public static Map<String, String> getAllProfiles(final File mozillaDir) {
> 
> javadoc, plz.  Also, why emphasize the name -> path mapping?  I'd much
> rather this be name -> rich struct.  Is this follow-up?

Yeah. For now we only ever care about getting the path.


> Not loving this |null| return.  Investigate how much pain throwing a checked
> exception here causes.

Actually introduces much joy, so done.

Comment 16

[Richard Newman [:rnewman]]

Attachment: Part 1b: review comments and extracting Mozilla directory handling and exceptions.

This includes all review comments, fixing a bunch of nits (e.g., static members named mFoo => sFoo), and rejigging the handling of the Mozilla directory to make a little more sense. That includes introducing explicit error handling (even if that error handling is just "throw a descriptive error").

Comment 17

[Richard Newman [:rnewman]]

Attachment: Part 1c: make getDefaultProfileName return the default profile name.

Decided to just address this one -- refactoring made it easy.

Comment 18

[Richard Newman [:rnewman]]

Attachment: Part 2: content provider for Fennec profiles.

Review comments addressed.

Comment 19

[Richard Newman [:rnewman]]

Robocop looks as cheerful as ever:

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

Comment 20

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8379430 [details] [diff] [review]
Part 1b: review comments and extracting Mozilla directory handling and exceptions.

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

lgtm, but fold before landing.

::: mobile/android/base/GeckoApp.java
@@ +4,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  package org.mozilla.gecko;
>  
> +import java.io.BufferedOutputStream;

Hahaha... Eclipse for the loss.  I'm personally for cleaning import lists aggressively, but I think we should probably discuss a bulk clean-up and ongoing import rules with the list before we get to zealous.

@@ +123,2 @@
>  public abstract class GeckoApp
>      extends GeckoActivity 

ws.

@@ +1162,5 @@
> +                        try {
> +                            profileName = getDefaultProfileName();
> +                        } catch (NoMozillaDirectoryException e) {
> +                            Log.wtf(LOGTAG, "Unable to fetch default profile name!", e);
> +                            // There's nothing at all we can do now. If the Mozilla directory

Is this true?  What creates the directory on first run?

::: mobile/android/base/GeckoProfile.java
@@ +33,5 @@
>  
>      private static HashMap<String, GeckoProfile> sProfileCache = new HashMap<String, GeckoProfile>();
>      private static String sDefaultProfileName = null;
>  
> +    public static boolean sIsUsingCustomProfile = false;

public?

@@ +85,5 @@
> +                defaultProfileName = geckoApp.getDefaultProfileName();
> +            } catch (NoMozillaDirectoryException e) {
> +                // If this failed, we're screwed. But there are so many callers that
> +                // we'll just throw a RuntimeException.
> +                Log.wtf(LOGTAG, "Unable to get default profile name.", e);

Ah, the rare terrible failure in the wild.  Nice to see.

::: mobile/android/base/GeckoProfileDirectories.java
@@ +24,4 @@
>  public class GeckoProfileDirectories {
> +    public static class NoMozillaDirectoryException extends Exception {
> +        public NoMozillaDirectoryException(Throwable cause) {
> +            super(cause);

nit: trailing ws.

@@ +34,5 @@
> +        public NoMozillaDirectoryException(String reason, Throwable cause) {
> +            super(reason, cause);
> +        }
> +    }
> +    public static class NoSuchProfileException extends Exception {

nit: newline between.

@@ +37,5 @@
> +    }
> +    public static class NoSuchProfileException extends Exception {
> +        public final String profileName;
> +        public NoSuchProfileException(final String profile) {
> +            this.profileName = profile;

If you must; but rich exceptions are rarely used.  And not allowing for |cause| is cause for me causing you pain, 'cuz it screws up test tracebacks.  So just include the profile name in the |detailMessage| and be done with it.

@@ +51,5 @@
>          return new INIParser(new File(mozillaDir, "profiles.ini"));
>      }
>  
> +    /**
> +     * Utility method to compute a salted profile name: eight alphanumeric

eight random alphanumeric

Comment 21

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8379431 [details] [diff] [review]
Part 1c: make getDefaultProfileName return the default profile name.

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

r=me with substantive issue clarified.

::: mobile/android/base/GeckoProfile.java
@@ +484,5 @@
>  
>      /**
> +     * @return the default profile name for this application, or
> +     *         {@link GeckoProfile#DEFAULT_PROFILE} if none could be found.
> +     * 

nit: trailing ws.

@@ +499,5 @@
>          }
>  
>          final String profileName = GeckoProfileDirectories.findDefaultProfileName(context);
>          if (profileName == null) {
> +            // Note that we don't persist this.

I don't understand "don't persist this".  I see a new write to |sDefaultProfileName| in the updated code; is that an error?  If it's not, drop the final, set |profileName| in the if, and lose the unneeded return.

Comment 22

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8379461 [details] [diff] [review]
Part 2: content provider for Fennec profiles.

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

Java: so fat.  Have you tested this from ABS?  If not, don't land the Provider itself.  If so, r=me.  I do see the green try for the GPD part.

::: mobile/android/base/GeckoProfileDirectories.java
@@ +40,5 @@
>          public NoSuchProfileException(final String profile) {
>              this.profileName = profile;
>          }
>      }
> +    private interface INISectionPredicate {

nit: newlines before and after, plz.  Oh Java, you so heavy.

@@ +41,5 @@
>              this.profileName = profile;
>          }
>      }
> +    private interface INISectionPredicate {
> +        public boolean evaluate(INISection section);

matches?

@@ +145,5 @@
> +    static Map<String, String> getDefaultProfile(final File mozillaDir) {
> +        return getMatchingProfiles(mozillaDir, sectionIsDefault, true);
> +    }
> +
> +    static Map<String, String> getProfileNamed(final File mozillaDir, final String name) {

nit: based on the type, I'd expect this to be "getProfile_s_Named"?  But we have |stopOnSuccess = true| below.  I don't really care, but I'm not convinced a map with 0 or 1 keys is the right way to say the named profile does not or does exist.

@@ +185,5 @@
>          final INIParser parser = GeckoProfileDirectories.getProfilesINI(mozillaDir);
>  
>          for (Enumeration<INISection> e = parser.getSections().elements(); e.hasMoreElements();) {
>              final INISection section = e.nextElement();
> +            if (predicate.evaluate(section)) {

nit: can we guard for null predicate here?  |!predicate || predicate.matches...|

::: mobile/android/base/GeckoProfilesProvider.java
@@ +49,5 @@
> +    }
> +
> +    @Override
> +    public boolean onCreate() {
> +        return false;

Perhaps say what false means here?

@@ +122,5 @@
> +            break;
> +        case PROFILES_NAME:
> +            // Return data about the specified profile.
> +            final String name = uri.getLastPathSegment();
> +            matchingProfiles = GeckoProfileDirectories.getProfileNamed(mozillaDir,

So, what happens if this throws?  Oh, it returns an empty Map of appropriate type if the profile doesn't exist, right?  WFM.

Comment 23

[Richard Newman [:rnewman]]

(In reply to Nick Alexander :nalexander from comment #21)

> >          final String profileName = GeckoProfileDirectories.findDefaultProfileName(context);
> >          if (profileName == null) {
> > +            // Note that we don't persist this.
> 
> I don't understand "don't persist this".  I see a new write to
> |sDefaultProfileName| in the updated code; is that an error?  If it's not,
> drop the final, set |profileName| in the if, and lose the unneeded return.

It means we don't persist it back to profiles.ini. We hit this case if profiles.ini doesn't include a default profile entry. I'll adjust the comment.

(In reply to Nick Alexander :nalexander from comment #20)

> Hahaha... Eclipse for the loss.  I'm personally for cleaning import lists
> aggressively, but I think we should probably discuss a bulk clean-up and
> ongoing import rules with the list before we get to zealous.

I filed Bug 975774, with 25 parts of import fixes, and rebased all of my current work on top of that.
 

> > +                        try {
> > +                            profileName = getDefaultProfileName();
> > +                        } catch (NoMozillaDirectoryException e) {
> > +                            Log.wtf(LOGTAG, "Unable to fetch default profile name!", e);
> > +                            // There's nothing at all we can do now. If the Mozilla directory
> 
> Is this true?  What creates the directory on first run?

NoMozillaDirectoryException is raised only when the directory doesn't exist and couldn't be created.

getDefaultProfileName ends up calling getMozillaDirectory, which calls mkdirs:

     * @throws NoMozillaDirectoryException
     *             if the directory did not exist and could not be created.
     */
    static File getMozillaDirectory(Context context) throws NoMozillaDirectoryException {
        final File mozillaDir = new File(context.getFilesDir(), MOZILLA_DIR_NAME);
        if (mozillaDir.exists() || mozillaDir.mkdirs()) {
            return mozillaDir;
        }

Comment 24

[Richard Newman [:rnewman]]

Part 1:

https://hg.mozilla.org/integration/fx-team/rev/b7e8c61b4d2e

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b7e8c61b4d2e

Comment 26

[Richard Newman [:rnewman]]

I've split out Bug 980187 to track the actual ContentProvider part, 'cos I need to spend the time to write tests for it.

Comment 27

[Richard Newman [:rnewman]]

I landed Bug 980187 with the wrong commit message:

https://hg.mozilla.org/integration/fx-team/rev/6f60d60ea58d

and backed it out for that reason.

https://hg.mozilla.org/integration/fx-team/rev/bece3008ec82