715644 - Content provider for storing "Tabs from other devices"

Status: VERIFIED FIXED

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

Description

[Richard Newman [:rnewman]]

In order to support Fennec 10 parity for tabs on other computers, we need a content provider to which we can write tab records.

That means a collection of records, each of which is:

  * Client name
  * Open tabs

with each tab being:

  * title
  * an array of history URLs (position 0 being the current page)
  * an icon (URL, I presume)
  * a last used timestamp

These collections must not be mutated by Fennec, only by Sync.

Fennec's own current tabs are the second part of this. In order to support syncing of Fennec's open tabs, we need a content provider to:

  * Retrieve the current set of open tabs, ideally by modified time, so as to avoid uploading *all the tabs* on every sync
  * For each tab, retrieve the four values above.

You get extra points if you design this in a way that's flexible for future extension (Panorama groups, for example).

Comment 1

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

Sriram - let's figure out what needs to happen with this feature. Talk to Richard to make sure we provide what Sync needs. Comment 0 is a good start.

If we need to split the work up, I would focus on pulling in desktop tabs first, then work on pushing out the currently loaded tabs second.

Comment 2

[Richard Newman [:rnewman]]

CCing ibarlow so he knows what data's available.

Comment 3

[Mike Connor [:mconnor]]

Should there be separate bugs for the local store this content provider will talk to, the UI hookups to update that local store when tabs change, and the UI for remote tabs that will read from that local store?

Comment 4

[Richard Newman [:rnewman]]

(In reply to Mike Connor [:mconnor] from comment #3)
> Should there be separate bugs for the local store this content provider will
> talk to, the UI hookups to update that local store when tabs change, and the
> UI for remote tabs that will read from that local store?

Filed a second part for retrieving tabs from Fennec: Bug 730039.

I'll let Sriram and crew file bugs for the work items needed to address each of these two projects.

Comment 5

[Richard Newman [:rnewman]]

CP Input and output (i.e., schema):
* Client name
* Client GUID
* Denormalized:
  * Tab title
  * Tab URLs
  * Tab URL (the topmost URL, stored for convenience because the rest will be JSON)
  * Favicon URL
  * "Last used" timestamp
  * Some kind of position index, if you want

We don't need a per-tab identifier; they're all smushed into one record when we upload.

So long as you correctly bump the last used timestamp whenever the tab is viewed or the URL changes, you don't need a modified time.

We don't care about individual tab deletions; we will always query something like

  SELECT title, URLs, faviconURL, lastUsed
  WHERE clientGUID = <fennec's client GUID>
  ORDER BY position

and completely replace what's on the server.

Comment 6

[Richard Newman [:rnewman]]

Oh, and if you're ever planning to support Private Browsing of some kind, I suggest you factor that in here and now!

Comment 7

[Sriram Ramasubramanian [:sriram]]

Attachment: WIP: Initial code

This WIP supports storing data from sync to device. The ContentProvider also supports querying.

There are quite a few TODO with questions attached to them. @rnewman, If you can answer them, I can update my patch based on them.

TODO:
1. Support URIs from Fennec to store its own tabs (tracked by another bug)
2. Support caching of client-guid for easy lookup
   -- Does this have take profile into account?

Comment 8

[Richard Newman [:rnewman]]

Comment on attachment 600457 [details] [diff] [review]
WIP: Initial code

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

Really fast skim review. More for the next rev.

::: mobile/android/base/db/BrowserContract.java.in
@@ +207,5 @@
>  
>          public static final String TIME_DELETED = "timeDeleted";
>      }
> +
> +    public static final class Tabs implements CommonColumns {

Do you need all of the CommonColumns? If you're sharing CLIENT_GUID between your two tables…

@@ +216,5 @@
> +        public static final String CONTENT_TYPE = "vnd.android.cursor.dir/tab";
> +
> +        public static final String CONTENT_ITEM_TYPE = "vnd.android.cursor.item/tab";
> +
> +        public static final String CLIENT_NAME = "client_name";

Perhaps split these fields into groups and comment?

::: mobile/android/base/db/TabsProvider.java.in
@@ +1,3 @@
> +/* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*-
> + * ***** BEGIN LICENSE BLOCK *****
> + * Version: MPL 1.1/GPL 2.0/LGPL 2.1

MPL 2.0.

@@ +80,5 @@
> +    static final int TABS_ID = 101;
> +    static final int TABS_CLIENTS = 102;
> +    static final int TABS_CLIENTS_ID = 103;
> +
> +    // TODO: What is the default sort order?

Doesn't matter to Sync. You probably want to sort by position.

@@ +96,5 @@
> +        URI_MATCHER.addURI(BrowserContract.TABS_AUTHORITY, "tabs/#", TABS_ID);
> +        URI_MATCHER.addURI(BrowserContract.TABS_AUTHORITY, "tabs/clients", TABS_CLIENTS);
> +        URI_MATCHER.addURI(BrowserContract.TABS_AUTHORITY, "tabs/clients/#", TABS_CLIENTS_ID);
> +
> +        map = TABS_PROJECTION_MAP;

I think you've got this pattern inverted. Usually one does this:

  static final Map<String, String> TABS_PROJECTION_MAP;
  static {
    HashMap<String, String> map = new HashMap<String, String>();

    map.put(...);
    ...
    TABS_PROJECTION_MAP = Collections.unmodifiableMap(map);
  }

which gives you an immutable, final, statically constructed Map.

@@ +146,5 @@
> +        }
> +
> +        @Override
> +        public void onUpgrade(SQLiteDatabase db, int oldVersion, int newVersion) {
> +            Log.d(LOGTAG, "Upgrading tabs.db: " + db.getPath() + " from " +

All of this logging should respect isLoggable; see BrowserProvider.java.in.

@@ +252,5 @@
> +
> +        return getDatabaseHelperForProfile(profile).getWritableDatabase();
> +    }
> +
> +    private boolean isCallerSync(Uri uri) {

Not needed. We don't need to track individual deletions.

@@ +270,5 @@
> +                } catch (Exception ex) {
> +                    Log.e(LOGTAG, "Error getting profile dir", ex);
> +                }
> +            }
> +        });

Why GeckoAppShell here? This is a ContentProvider, it shouldn't be importing the app at all. Shouldn't this logic simply be part of GeckoDirProvider?

@@ +343,5 @@
> +                selectionArgs = DBUtils.appendSelectionArgs(selectionArgs,
> +                        new String[] { Long.toString(ContentUris.parseId(uri)) });
> +                // fall through
> +            case TABS_CLIENTS:
> +                Log.v(LOGTAG, "Delete all clients: " + uri);

Wha?

The TABS_CLIENTS_ID branch seems to use the URI identifier to select a particular client. Then you delete using `selection`, ignoring the URI ID. That can't be right, surely?

@@ +357,5 @@
> +                        new String[] { Long.toString(ContentUris.parseId(uri)) });
> +                // fall through
> +            case TABS:
> +                Log.v(LOGTAG, "Deleting all tabs: " + uri);
> +                // TODO: Do we need this?

Yes.

@@ +413,5 @@
> +                // TODO: Caching of client-guid mappings
> +                ContentValues clientValues = new ContentValues();
> +        
> +                // TODO: Should I throw an error?
> +                if (!values.containsKey(Tabs.CLIENT_NAME)) {

If there's no name, don't worry about it.

@@ +424,5 @@
> +        
> +                if (values.containsKey(Tabs.CLIENT_GUID))
> +                    clientValues.put(Tabs.CLIENT_GUID, values.getAsString(Tabs.CLIENT_GUID));
> +        
> +                db.insertOrThrow(TABLE_CLIENTS, Tabs.CLIENT_GUID, clientValues); 

Let's simplify this. If there's an entry in TABLE_CLIENTS for the provided GUID, do nothing.

That will allow Sync to insert a client entry with a specified modified time. That means you need to add a

  case TABS_CLIENTS:

below.

@@ +491,5 @@
> +                selection = DBUtils.concatenateWhere(selection, selectColumn(TABLE_CLIENTS, Tabs.CLIENT_GUID));
> +                selectionArgs = DBUtils.appendSelectionArgs(selectionArgs,
> +                        new String[] { Long.toString(ContentUris.parseId(uri)) });
> +                updated = updateValues(uri, values, selection, selectionArgs, false);
> +                break;

You need to implement cases for TABS_CLIENTS, too. Sync uses selection args exclusively; we don't give one jot about Android's stupid ID URI approach.

@@ +543,5 @@
> +                break;
> +
> +            case TABS_CLIENTS_ID:
> +                // TODO: To see if it exists
> +                break;

Need TABS_CLIENTS. We care about this so we can fetch the modified times of each client.

@@ +566,5 @@
> +        final SQLiteDatabase db = getWritableDatabase(uri);
> +
> +        if (!isCallerSync(uri)) {
> +            // notify sync
> +        }

Hmm. Not sure if this is necessary; we'll query when we want them, and Fennec won't be modifying the non-Fennec tabs anyway.

Comment 9

[Richard Newman [:rnewman]]

Comment on attachment 600457 [details] [diff] [review]
WIP: Initial code

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

Another couple of things; see inline.


For the record, here are the operations that Sync will perform, in this order:

  // Fetch the modified times for each client.
  query(clients, [client_guid, modified], null, null)

  // For the ones that are missing on the server now: delete them.
  delete(clients, "where client_guid = ?", [someguid])

  // Also delete tabs if we have to do it explicitly:
  delete(clients/tabs, "where client_guid = ?", [someguid])

  // For the ones that are still present: update the client...
  update(clients, {name: "Foo", modified: 4567890123}, "where client_guid = ?", [otherguid])

  // For new ones: insert.
  insert(clients, {name: "Bar", client_guid: "bazguid", modified: 1234568})

  // Then put tabs in, using one or more bulkInserts. Remove existing tabs first.
  delete(clients/tabs, "where client_guid = ?", [otherguid])
  bulkInsert(clients/tabs, [{uri: ..., client_guid: otherguid}, ...])
  bulkInsert(clients/tabs, [{uri: ..., client_guid: bazguid}, ...])
  


Note that:

* We never query using an ID in the URI; always using selection/selectionArgs.
* We want to write a modified time into the clients table. We can take full responsibility for updating the clients table if it'll make your life simpler.
* We'll regularly be deleting by GUID. You definitely want an index on that column.


For the other bug, retrieving Fennec's tabs, we'll just be issuing a query like this:

  query(clients/tabs, *, "where client_guid = NULL", [], "order by position")

::: mobile/android/base/db/TabsProvider.java.in
@@ +73,5 @@
> +
> +    static final int DATABASE_VERSION = 1;
> +
> +    static final String TABLE_TABS = "tabs";
> +    static final String TABLE_CLIENTS = "tab-clients";

Note that there's a choice of perspective here. Tabs can't exist without clients, but clients can exist without tabs, so really clients are the primary entity. I'm not suggesting that you go renaming stuff, but I think it's important to be able to flip your brain between the two ways of thinking of the domain!

@@ +94,5 @@
> +        // Tabs
> +        URI_MATCHER.addURI(BrowserContract.TABS_AUTHORITY, "tabs", TABS);
> +        URI_MATCHER.addURI(BrowserContract.TABS_AUTHORITY, "tabs/#", TABS_ID);
> +        URI_MATCHER.addURI(BrowserContract.TABS_AUTHORITY, "tabs/clients", TABS_CLIENTS);
> +        URI_MATCHER.addURI(BrowserContract.TABS_AUTHORITY, "tabs/clients/#", TABS_CLIENTS_ID);

And thus on the subject: the URL space for this should really be:

  clients              => TABS_CLIENTS
  clients/tabs         => TABS
  clients/#            => TABS_CLIENTS_ID
  clients/#/tabs       => if we were to support tabs by numeric reference of client.

@@ +140,5 @@
> +
> +            // Table for client's name-guid mapping
> +            db.execSQL("CREATE TABLE " + TABLE_TABS + "(" +
> +                    Tabs.CLIENT_GUID + " TEXT PRIMARY KEY," +
> +                    Tabs.CLIENT_NAME + " TEXT" +

Also need a modified time here, set by the caller.

Comment 10

[Sriram Ramasubramanian [:sriram]]

> > +    public static final class Tabs implements CommonColumns {
> 
> Do you need all of the CommonColumns? If you're sharing CLIENT_GUID between
> your two tables…

CommonColumns just has an _ID field

> > +        public static final String CLIENT_NAME = "client_name";
> 
> Perhaps split these fields into groups and comment?

Would like to leave the way the file is.

> MPL 2.0.

Copy paste problem :D BrowserProvider and other files are still MPL 1.1

> > +        map = TABS_PROJECTION_MAP;
> 
> I think you've got this pattern inverted. Usually one does this:

BrowserProvider does this. I don't know why though.

> > +    private boolean isCallerSync(Uri uri) {
> 
> Not needed. We don't need to track individual deletions.

I'll use ContentProvider for Fennec tabs. So this will be helpful.

> Why GeckoAppShell here? This is a ContentProvider, it shouldn't be importing
> the app at all. Shouldn't this logic simply be part of GeckoDirProvider?

Pointing fingers at BrowserProvider again :D

> 
> @@ +343,5 @@
> > +                selectionArgs = DBUtils.appendSelectionArgs(selectionArgs,
> > +                        new String[] { Long.toString(ContentUris.parseId(uri)) });
> > +                // fall through
> > +            case TABS_CLIENTS:
> > +                Log.v(LOGTAG, "Delete all clients: " + uri);
> 
> Wha?
> 
> The TABS_CLIENTS_ID branch seems to use the URI identifier to select a
> particular client. Then you delete using `selection`, ignoring the URI ID.
> That can't be right, surely?

I am not ignoring the id. It is resigned with the id back to selection. This is how BrowserProvider works.

> > +        
> > +                db.insertOrThrow(TABLE_CLIENTS, Tabs.CLIENT_GUID, clientValues); 
> 
> Let's simplify this. If there's an entry in TABLE_CLIENTS for the provided
> GUID, do nothing.

Yup. When I implement caching (mapping), this will be proper.

Comment 11

[Richard Newman [:rnewman]]

> > Perhaps split these fields into groups and comment?
> 
> Would like to leave the way the file is.

At the very least you have to explain what the columns are, which tables use which, and how they relate. We don't have a data dictionary for this product, and we also don't have a good way to easily introspect the databases, so having a clue about what each column is for would be really helpful.

Remember, I have to write queries over these things :D


> Copy paste problem :D BrowserProvider and other files are still MPL 1.1
...
> BrowserProvider does this. I don't know why though.

It should be corrected there.

Filed Bug 730526 with a patch for both issues.


> > > +    private boolean isCallerSync(Uri uri) {
> > 
> > Not needed. We don't need to track individual deletions.
> 
> I'll use ContentProvider for Fennec tabs. So this will be helpful.

"In what way will the two callers behave differently?" is my point. They shouldn't, so you should need to differentiate.

The only reason for distinguishing Sync with a URL parameter is to control whether deletions are flagged or actually deleted from the database. Tab records don't need that, so we shouldn't need to check this at all.


> Pointing fingers at BrowserProvider again :D

Heh. No harm in fixing BrowserProvider as you go! And of course lifting out shared code…


> I am not ignoring the id. It is resigned with the id back to selection. This
> is how BrowserProvider works.

Ah yes, so you are. (This is what I get for doing a code review from the passenger seat of my truck.)

In this case, I merely suggest that the log statement is misleading :D

Comment 12

[Sriram Ramasubramanian [:sriram]]

Attachment: WIP: Revised version 2.0

This patch has required changes from previous reviews. Hopefully this works :D

Comment 13

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

Forgot to check for compilation erros in previous patch. This patch compiles fine.

Comment 14

[Sriram Ramasubramanian [:sriram]]

Will "client/#" path work? Android expects "#" to be a number to be parsed back as long. I don't think we can pass a String there (as GUID is a string). How do we go about this?

Comment 15

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

Added few more changes to remove "clients/#" as it cannot be supported.
Also fix a startup crash that would happen when it tries to create 2 tables with "same name".

Comment 16

[Richard Newman [:rnewman]]

Comment on attachment 601125 [details] [diff] [review]
Patch

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

Nearly!

Commit message should be "Bug 715644 - Content provider for storing 'Tabs from other devices'". It doesn't add support for remote tabs in Sync :)

I find it really depressing that Android requires six hundred lines of code for "update a trivial database". :/

::: mobile/android/base/db/BrowserContract.java.in
@@ +219,5 @@
> +
> +        public static final String CONTENT_ITEM_TYPE = "vnd.android.cursor.item/tab";
> +
> +        // Columns for "clients" table.
> +        // Name of the client computer

"Client-provided name string. Could conceivably be null."

@@ +222,5 @@
> +        // Columns for "clients" table.
> +        // Name of the client computer
> +        public static final String CLIENT_NAME = "client_name";
> +
> +        // Universal ID for the client computer

Better: "Sync-assigned GUID for client device. NULL for local tabs."

@@ +225,5 @@
> +
> +        // Universal ID for the client computer
> +        public static final String CLIENT_GUID = "client_guid";
> +
> +        // Last modified time on the tabs for the client

"Last modified time for the client's tab record. For remote records, a server timestamp provided by Sync during insertion."

@@ +230,5 @@
> +        public static final String CLIENT_LAST_MODIFIED = "client_last_modified";
> +
> +
> +        // Columns for "tabs" table.
> +        // Title of the tab

I think we can skip this column's comment :)

@@ +233,5 @@
> +        // Columns for "tabs" table.
> +        // Title of the tab
> +        public static final String TITLE = "title";
> +        
> +        // Topmost url - for easy lookup

Capitalize URL, add punctuation:

// Topmost URL from the history array. Allows processing of this tab without parsing that array.

@@ +236,5 @@
> +        
> +        // Topmost url - for easy lookup
> +        public static final String URL = "url";
> +
> +        // Blob/Json array of history urls

// JSON-encoded array of history URL strings, from most recent to least recent.

@@ +239,5 @@
> +
> +        // Blob/Json array of history urls
> +        public static final String HISTORY = "history";
> +
> +        // Favicon for the tab

// Favicon URL for the tab's topmost history entry.

@@ +242,5 @@
> +
> +        // Favicon for the tab
> +        public static final String FAVICON = "favicon";
> +
> +        // Last used time of the tab

Punctuation.

@@ +245,5 @@
> +
> +        // Last used time of the tab
> +        public static final String LAST_USED = "last_used";
> +
> +        // Position of the tab. 1 - foreground.

Why not 0 = foreground? (Or do you mean "1 minus foreground"? Hyphens are evil!)

::: mobile/android/base/db/TabsProvider.java.in
@@ +124,5 @@
> +                    Tabs.HISTORY + " TEXT," +
> +                    Tabs.FAVICON + " TEXT," +
> +                    Tabs.LAST_USED + " INTEGER," +
> +                    Tabs.POSITION + " INTEGER" +
> +                    ");");

You very likely want indices on CLIENT_GUID and POSITION.

@@ +133,5 @@
> +            db.execSQL("CREATE TABLE " + TABLE_CLIENTS + "(" +
> +                    Tabs.CLIENT_GUID + " TEXT PRIMARY KEY," +
> +                    Tabs.CLIENT_NAME + " TEXT," +
> +                    Tabs.CLIENT_LAST_MODIFIED + " INTEGER" +
> +                    ");");

Index on GUID.

@@ +331,5 @@
> +                selectionArgs = DBUtils.appendSelectionArgs(selectionArgs,
> +                        new String[] { Long.toString(ContentUris.parseId(uri)) });
> +                // fall through
> +            case TABS:
> +                trace("Deleting all tabs: " + uri);

s/all //

In fact, better to just mimic the other logs:

  trace("Delete on TABS: " + uri);

@@ +398,5 @@
> +
> +        debug("Inserted ID in database: " + id);
> +
> +        if (id >= 0)
> +            return ContentUris.withAppendedId(uri, id);

So if the clients table doesn't have an ID column, I don't see any advantage in returning the sqlite row ID here in both cases. Lift this debug output and the withAppendedId chunk into the TABS_ID section, and just return null from TABS_CLIENTS. The `switch` is complete, so you don't need any code after it at all.

@@ +452,5 @@
> +                selectionArgs = DBUtils.appendSelectionArgs(selectionArgs,
> +                        new String[] { Long.toString(ContentUris.parseId(uri)) });
> +                updated = updateValues(uri, values, selection, selectionArgs, TABLE_TABS);
> +                break;
> +

Are you deliberately not supporting update on TABS, or was that an omission?

Either say you don't in a comment, or file a follow-up and put the bug number here.

@@ +479,5 @@
> +                if (match == TABS_ID) {
> +                    selection = DBUtils.concatenateWhere(selection, selectColumn(TABLE_TABS, Tabs._ID));
> +                    selectionArgs = DBUtils.appendSelectionArgs(selectionArgs,
> +                            new String[] { Long.toString(ContentUris.parseId(uri)) });
> +                }

Move this clause to TABS_ID and fall through?

Comment 17

[Sriram Ramasubramanian [:sriram]]

> @@ +398,5 @@
> > +
> > +        debug("Inserted ID in database: " + id);
> > +
> > +        if (id >= 0)
> > +            return ContentUris.withAppendedId(uri, id);
> 
> So if the clients table doesn't have an ID column, I don't see any advantage
> in returning the sqlite row ID here in both cases. Lift this debug output
> and the withAppendedId chunk into the TABS_ID section, and just return null
> from TABS_CLIENTS. The `switch` is complete, so you don't need any code
> after it at all.

SQL would return an ID for the newly inserted row. I prefer returning back the same in the URL -- though clients wouldn't use it. This is better than return "null", because we can know if the row was inserted or not.

> Are you deliberately not supporting update on TABS, or was that an omission?
> 
> Either say you don't in a comment, or file a follow-up and put the bug
> number here.

That was an omission ;) Added it now.

Comment 18

[Richard Newman [:rnewman]]

(In reply to Sriram Ramasubramanian [:sriram] from comment #17)

> SQL would return an ID for the newly inserted row. I prefer returning back
> the same in the URL -- though clients wouldn't use it. This is better than
> return "null", because we can know if the row was inserted or not.

Except that returning a URI-with-ID is Android's way of saying "and you can now perform operations on this URI that will affect only that record", and that's not the case.

Better to return nothing or just about anything else!

Comment 19

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This has the required changes from previous review comments.
This adds "CLIENTS_ID" to deal with clients table using rowid.

Comment 20

[Richard Newman [:rnewman]]

Comment on attachment 601347 [details] [diff] [review]
Patch

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

Missing closing quote in commit message.

Otherwise, just nits and the Tabs/Clients split we discussed on IRC.

On to the next one!

::: mobile/android/base/db/TabsProvider.java.in
@@ +58,5 @@
> +    static final Map<String, String> TABS_PROJECTION_MAP;
> +    static final Map<String, String> CLIENTS_PROJECTION_MAP;
> +
> +    static {
> +        // Tabs

Redundant, lonely comment.

@@ +128,5 @@
> +                    Tabs.LAST_USED + " INTEGER," +
> +                    Tabs.POSITION + " INTEGER" +
> +                    ");");
> +
> +            // Indices on CLIENT_GUID and POSITION

Nit: period at end.

@@ +132,5 @@
> +            // Indices on CLIENT_GUID and POSITION
> +            db.execSQL("CREATE INDEX tabs_guid_index ON " + TABLE_TABS + "("
> +                    + Tabs.CLIENT_GUID + ")");
> +
> +            db.execSQL("CREATE INDEX tabs_position_index ON " + TABLE_TABS + "("

Nit: extract index names.

@@ +137,5 @@
> +                    + Tabs.POSITION + ")");
> +
> +            debug("Creating " + TABLE_CLIENTS + " table");
> +
> +            // Table for client's name-guid mapping

.

@@ +144,5 @@
> +                    Tabs.CLIENT_NAME + " TEXT," +
> +                    Tabs.CLIENT_LAST_MODIFIED + " INTEGER" +
> +                    ");");
> +
> +            // Index on GUID

.

@@ +345,5 @@
> +                        new String[] { Long.toString(ContentUris.parseId(uri)) });
> +                // fall through
> +            case CLIENTS:
> +                trace("Delete on CLIENTS: " + uri);
> +                // delete from both TABLE_TABS and TABLE_CLIENTS

Capitalization and punctuation.

Comment 21

[Sriram Ramasubramanian [:sriram]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/6e1d2d8e101e 
Pushed with required nits fixed.

Comment 22

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

looks like this was missed after a merge:
http://hg.mozilla.org/mozilla-central/rev/6e1d2d8e101e

Comment 23

[Cristian Nicolae (:xti)]

Everything works as expected by design on the latest Nightly and Aurora builds.
Closing bug as verified fixed on:

Firefox 15.0a1 (2012-05-28)
Firefox 14.0a2 (2012-05-28)

Device: Galaxy Nexus
OS: Android 4.0.2