Share overlay request backend: stage 1

RESOLVED FIXED in Firefox 34

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ckitching, Assigned: ckitching)

Tracking

(Depends on: 1 bug, Blocks: 3 bugs)

unspecified
Firefox 34
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec34+)

Details

Attachments

(3 attachments, 14 obsolete attachments)

4.04 KB, patch
nalexander
: review+
Details | Diff | Splinter Review
1.76 KB, patch
nalexander
: review+
Details | Diff | Splinter Review
38.65 KB, patch
rnewman
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
Stage 1 of the overlay backend, as described in Bug 1044791. Provides a handy target for marking as dependencies the ensuing fallout.

Patches to follow shortlyish...
(Assignee)

Updated

5 years ago
Blocks: 1040967
(Assignee)

Updated

5 years ago
See Also: → bug 1044940
(Assignee)

Updated

5 years ago
Summary: Stage 1 backend overlay request backend → Stage 1 overlay request backend
(Assignee)

Updated

5 years ago
Blocks: 1044948
(Assignee)

Comment 1

5 years ago
Attachment #8467512 - Flags: review?(rnewman)
(Assignee)

Comment 3

5 years ago
Attachment #8467514 - Flags: review?(rnewman)
(Assignee)

Comment 4

5 years ago
Attachment #8467515 - Flags: review?(rnewman)
(Assignee)

Comment 5

5 years ago
Attachment #8467516 - Flags: review?(rnewman)
(Assignee)

Comment 6

5 years ago
Attachment #8467517 - Flags: review?(rnewman)
(Assignee)

Comment 7

5 years ago
These, plus the one from Bug 1044947 seem to provide a complete share overlay, sans graphics, Bug 1048645, and adding to the reading list.
Comment on attachment 8467512 [details] [diff] [review]
Part 1/6: Delete Android Sync share handler

No, for several reasons.

1. This needs to land behind a build flag. That flag will first turn this on, then turn this on and Send Tab off.

2. We probably want a migration step here, so that quickshare etc. are preserved (either only in Fennec, or system-wide).

3. This would need to be landed in a-s first.
Attachment #8467512 - Flags: review?(rnewman) → review-
Attachment #8467513 - Flags: review?(rnewman) → review+
Comment on attachment 8467514 [details] [diff] [review]
Part 3/6: Make BrowserDB safe to multiply-initialise

Don't use BrowserDB. Use BrowserProvider directly.
Attachment #8467514 - Flags: review?(rnewman) → review-
Comment on attachment 8467515 [details] [diff] [review]
Part 4/6: Phase 1 overlay backend service

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

::: mobile/android/base/AndroidManifest.xml.in
@@ +384,5 @@
>              </intent-filter>
>          </activity>
>  
> +
> +        <service

#ifdef MOZ_ANDROID_SHARE_OVERLAY

See http://www.ncalexander.net/blog/2014/07/09/how-to-land-a-fennec-feature-behind-a-build-flag/

@@ +387,5 @@
> +
> +        <service
> +                 android:name="org.mozilla.gecko.overlays.OverlayIntentHandler"
> +                 android:label="Firefox Sharing Service"
> +                 android:permission="" />

sec-review-needed

::: mobile/android/base/overlays/OverlayIntentHandler.java
@@ +3,5 @@
>  import android.app.Service;
>  import android.content.Intent;
>  import android.os.IBinder;
> +import org.mozilla.gecko.GeckoProfile;
> +import org.mozilla.gecko.db.BrowserDB;

As previously mentioned: use BrowserProvider. See how Sync does it.

@@ +20,5 @@
> + * Open tab in background**
> + *
> + * * Neither of these incur a page fetch on the service... yet. That will require headless Gecko,
> + *   something we're yet to have. Refactoring Gecko as a service itself and restructing the rest of
> + *   the app to talk to it seems like the way to go there.

Bug numbers.

@@ +40,5 @@
> +        // Perform Fennec startup actions here. Initialise Singletons we care about etc.
> +        // This work is not repeated when Fennec eventually starts for real, provided the service
> +        // isn't killed in the meantime.
> +
> +        startDatabase();

Not needed.

@@ +44,5 @@
> +        startDatabase();
> +
> +
> +        // Dispatch intent to appropriate method according to its action.
> +        String action = intent.getAction();

intent can be null. action might be able to be null. See Bug 1025937.

@@ +62,5 @@
> +
> +    /**
> +     * Start the BrowserDB so we can write to it. Currently always uses the default profile.
> +     *
> +     * TODO: Facilitate selection of desired profile here. (How? Why? Do we even want that?)

Pass the 'profile' argument to BrowserProvider.
Attachment #8467515 - Flags: review?(rnewman) → feedback+
Comment on attachment 8467516 [details] [diff] [review]
Part 5/6: Add-bookmark support for overlay backend

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

::: mobile/android/base/overlays/OverlayIntentHandler.java
@@ +83,5 @@
>          // TODO
>      }
>  
> +    /**
> +     * Add the given url/title combination to the database. Title may not be provided, and should be

s/url/URL

@param url May not be null.
@param title May be null.

@@ +87,5 @@
> +     * Add the given url/title combination to the database. Title may not be provided, and should be
> +     * considered provisional (as it comes from a share intent, so may be rubbish).
> +     */
> +    private void addBookmark(String url, String title) {
> +        BrowserDB.addBookmark(getContentResolver(), title, url);

Validate your inputs:

* Empty or null.
* Too long.
* Invalid URLs -- what do we consider acceptable for bookmarking?
Attachment #8467516 - Flags: review?(rnewman) → review-
Comment on attachment 8467517 [details] [diff] [review]
Part 6/6: Send-tab support for overlay backend

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

Did you intend to label this "TEMPORARY PLACEHOLDER"? I thought by the time we got here we'd know the destination, no?
Attachment #8467517 - Flags: review?(rnewman)
tracking-fennec: --- → 34+
Hardware: ARM → All
Summary: Stage 1 overlay request backend → Share overlay request backend: stage 1
Depends on: 1048901
(Assignee)

Comment 13

5 years ago
(In reply to Richard Newman [:rnewman] from comment #9)
> Comment on attachment 8467514 [details] [diff] [review]
> Part 3/6: Make BrowserDB safe to multiply-initialise
> 
> Don't use BrowserDB. Use BrowserProvider directly.

So the biggest problem this produces is that this is too low-level an abstraction. BrowserProvider gives me low-level access to the SQL: which is sufficient to do what I want, but not elegantly.

Consider the logic in LocalBrowserDB that's used for adding a bookmark:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/LocalBrowserDB.java#974

You really want me to duplicate all that? The current approach I can see for directly using BrowserProvider involves copy-pasting that entire function. I'm not happy about it. Did I miss something?
What's the concrete benefit of using the ContentProvider over LocalBrowserDB, given that I prevented the need to initialise it again when Fennec is started? At the moment all I can see are downsides (a lot of copy-pasted code).


(In reply to Richard Newman [:rnewman] from comment #12)
> Comment on attachment 8467517 [details] [diff] [review]
> Part 6/6: Send-tab support for overlay backend
> 
> Review of attachment 8467517 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Did you intend to label this "TEMPORARY PLACEHOLDER"? I thought by the time
> we got here we'd know the destination, no?

The destination device, you mean?
Yes, probably, subject to figuring out enough about how Sync works in time. The frontend would need to fire up and get from Sync a list of devices before it is displayed but after the share intent hits it. 

We will need it to do exactly as it does in this patch if the user isn't logged in to sync (so it sends them to the "set up sync" thing), or if there's some reason why we can't synchronously find the aforementioned device list to show in the UI (in which case we cache the result in a service and have it available for every share intent that lands until Android kills the service).
Hm, you might be right.

My main concerns with using BrowserDB are around carrying excess state. But it looks like it's shrunk a lot since I formed that opinion; now it's much closer to being a simple wrapper, which is what we want. Rock on.


> Yes, probably, subject to figuring out enough about how Sync works in time.
> The frontend would need to fire up and get from Sync a list of devices
> before it is displayed but after the share intent hits it. 

Solvable problem, and see SendTabActivity.


> We will need it to do exactly as it does in this patch if the user isn't
> logged in to sync (so it sends them to the "set up sync" thing)

We should detect that state and have this directly trigger FxAccountGetStartedActivity. See SendTabActivity and SyncPreference.
(Assignee)

Comment 15

5 years ago
(In reply to Richard Newman [:rnewman] from comment #14)
> Hm, you might be right.
> 
> My main concerns with using BrowserDB are around carrying excess state. But
> it looks like it's shrunk a lot since I formed that opinion; now it's much
> closer to being a simple wrapper, which is what we want. Rock on.

I'd also suggest that carrying around extra state (that's then useful when we start Fennec later) is less problematic than duplicating much of LocalBrowserDB in overlay-land. We can always reduce the memory footprint by refactoring (so that state we don't want goes away). That said, since the problem appears not to exist: hooray!

Something that bothers me about BrowserDB is that, like LocalBrowserDB (and others), it doesn't have a comment at the top that states what it *is*. This means that different people will come along and infer different things about what this class is for, causing its purpose may mutate unpleasantly over time. I feel quite strongly that the author of each class should write absolutely no code without first stating what it's supposed to do.. How else will they know when to stop writing code in this clasS?

This also makes it much harder to explore the codebase. "What the hell is a DoorHanger?", "Does this class do <thing I want to do that its name suggest it probably does but I don't especially want to read all the code to figure out if it really does that>?", etc.

I mean, I still don't actually know: what is BrowserDB? What's it for? How is it different from LocalBrowserDB? Where's the line between the two drawn (and to what end?)
It looks like BrowserDB is an attempt to provide an abstract interface to the database so we could potentially swap out LocalBrowserDB for something with a different backend without vast hassle... But then it also has a bunch of other sort of random things sellotaped onto it. What's the goal of this class? "BrowserDB is..."?

> > Yes, probably, subject to figuring out enough about how Sync works in time.
> > The frontend would need to fire up and get from Sync a list of devices
> > before it is displayed but after the share intent hits it. 
> 
> Solvable problem, and see SendTabActivity.

Absolutely, I was just fishing for class names so I know where to look. :D

> > We will need it to do exactly as it does in this patch if the user isn't
> > logged in to sync (so it sends them to the "set up sync" thing)
> 
> We should detect that state and have this directly trigger
> FxAccountGetStartedActivity. See SendTabActivity and SyncPreference.

Will do.
(In reply to Chris Kitching [:ckitching] from comment #15)
> We can always reduce the memory footprint by
> refactoring (so that state we don't want goes away).

I'm less concerned about memory, and more concerned about bugs. For example, I'm not quite sure how Part 3 and Part 4 are supposed to work, especially in the case of multiple profiles. E.g., what's the value of that static profile string when Fennec is in guest mode, and you share something?

> Something that bothers me about BrowserDB is that, like LocalBrowserDB (and
> others), it doesn't have a comment at the top that states what it *is*.

BrowserDB ought to not exist. We used to be able to switch between the Fennec DBs ("local") and Android's.
(Assignee)

Comment 17

5 years ago
Posted patch Draft complete backend patch (obsolete) — Splinter Review
Righty, here's a draft patch providing a "complete" backend. This supports bookmarks/reading list/send tab.

The reading list support turned out vastly simpler than expected: just sticking the url/title in the DB works correctlyish.
If the page you added isn't compatible with reader-mode, you get stuck on the loading screen indefinitely when you attempt to load the page. Extra work is needed to have reader mode just display an ordinary web view in such situations (or maybe something else...?)

The send-tab support works correctly. It is largely cannabilised from the old (now defunct, but not removed by this patch) SendTabActivity.


The backend consists of a service, OverlayIntentHandler, to which clients of the share service (the overlays, the future "bubbles" thing, whatever else we write) send requests to share things.
The intent protocol used to do that is documented in some detail in OverlayConstants.java.

The service has two main roles:

* To allow processing necessary to perform a share to occur in the background while the user is using a different app (after sending us the share intent)
* To allow the results of expensive one-time startup code that must be run prior to a share to be cached. We don't want to have to perform expensive database lookups every time if we can avoid it, particularly if the results of these queries are necessary for the correct display of the UI.

A ShareMethod is a singleton representing a way to handle a shared url/title combination. "add to reading list", "send tab", are ShareMethods.
The ShareMethod object provides a handle method which takes the specific actions necessary to perform that sort of share.
ShareMethods use the LocalBroadcastManager to broadcast events relating to changes in their state that a UI might care about. For example, when the set of available sync clients has been loaded, a broadcast is made so any connected UI can display the new list.
A ShareMethod must be initialised before it is used. The init() method is provided for this purpose: it is here that expensive startup tasks should be performed.
In the event a state change occurs elsewhere in the application that invalidates some state in a ShareMethod, the service provides an API for selectively re-initialising specified ShareMethods. This will be invoked, for example, from the broadcast listeners that respond to sync accounts being removed (causing the send tab ShareMethod to enter a state causing any attempt to use it to launch the "set up sync" activity instead).

This seems a fairly tidy way of laying things out and, importantly, it *works* (when combined with the frontend patch I haven't uploaded yet, that is).

Any thoughts?
Attachment #8467514 - Attachment is obsolete: true
Attachment #8467515 - Attachment is obsolete: true
Attachment #8467516 - Attachment is obsolete: true
Attachment #8467517 - Attachment is obsolete: true
Attachment #8476381 - Flags: feedback?(wjohnston)
Attachment #8476381 - Flags: feedback?(nalexander)
Comment on attachment 8476381 [details] [diff] [review]
Draft complete backend patch

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

Overall I like. I'm not in love with the initialization though. i.e. I'd like to not initialize a share provider until we're going to use it. Maybe we have to do it this way?

It could be that doesn't work for something like Sync, but I think with Sync it would actually be neat if we could Offer to set it up if you click on it and its not already set up. Maybe that's what happens here and I'm mis-reading. Right now it looked like we just popped it up right away if you tried to share and sync wasn't set up (even though reading list doesn't need it).

There are some nits here too, but I quit my "review" mode since this is just a f?

I also think you should split this up and start landing things as soon as you can. Its split up well so that you can do that. Little unfinished pieces are fine, especially if they're behind build flags or set to nightly-only.

::: mobile/android/base/AndroidManifest.xml.in
@@ +380,5 @@
>          </activity>
> +
> +        <!-- Service to handle requests from overlays. -->
> +        <service android:name="org.mozilla.gecko.overlays.service.OverlayIntentHandler"
> +                 android:label="Firefox Sharing Service" />

You'll need to localize this. I think I'd just use "Firefox Share" too. Or just "Firefox"

::: mobile/android/base/overlays/OverlayConstants.java
@@ +51,5 @@
> +
> +        public static final Creator<ShareType> CREATOR = new Creator<ShareType>() {
> +            @Override
> +            public ShareType createFromParcel(final Parcel source) {
> +                return ShareType.values()[source.readInt()];

You are probably safer to serialize using the string name. i.e. then if the order of options in this file changes, things should be ok.

@@ +146,5 @@
> +
> +    // Keys used for parceling ClientRecords.
> +    public static final String CLIENT_RECORD_NAME = "N";
> +    public static final String CLIENT_RECORD_TYPE = "T";
> +    public static final String CLIENT_RECORD_GUID = "G";

Can you keep these consts with the SyncShare client code? I'd also probably use full names. i.e. = "NAME";

::: mobile/android/base/overlays/service/OverlayIntentHandler.java
@@ +67,5 @@
> +            case ACTION_SERVICE_RE_INIT:
> +                // Re-initialise the given set of ShareMethods. Useful when the state has changed
> +                // in ways that invalidates the state cached in the ShareMethods.
> +                reinitialiseMethods((ShareType[]) intent.getParcelableArrayExtra(SHARE_METHOD));
> +            break;

I'm a little confused about why we have to initialize these. I guess we're trying to answer "Is sync setup"?

::: mobile/android/base/overlays/service/sharemethods/AddBookmark.java
@@ +6,5 @@
> +import org.mozilla.gecko.R;
> +import org.mozilla.gecko.db.LocalBrowserDB;
> +
> +public class AddBookmark extends ShareMethod {
> +    private static final String LOGTAG = "AddBookmark";

We always prefix these with Gecko so they are easy to grep for.

@@ +24,5 @@
> +    protected void init() {
> +        resolver = context.getContentResolver();
> +
> +        // Acquire LocalBrowserDB instance for the default profile.
> +        browserDB = new LocalBrowserDB(GeckoProfile.DEFAULT_PROFILE);

Is there some reason to cache resolver or browserDB in init()?

::: mobile/android/base/overlays/service/sharemethods/AddToReadingList.java
@@ +21,5 @@
> +        values.put(Bookmarks.URL, url);
> +
> +        browserDB.addReadingListItem(resolver, values);
> +
> +        showToast(context.getResources().getString(org.mozilla.gecko.R.string.reading_list_added));

A bit sad we have to duplicate this toast code everywhere, but maybe its our only option.

@@ +32,5 @@
> +        // Acquire LocalBrowserDB instance for the default profile.
> +        browserDB = new LocalBrowserDB(GeckoProfile.DEFAULT_PROFILE);
> +    }
> +
> +    protected static AddToReadingList instance;

Move fields to the top of the file.

::: mobile/android/base/overlays/service/sharemethods/SendTab.java
@@ +68,5 @@
> +            return;
> +        }
> +
> +        // Queue up the share commands for each destination device.
> +        // Remember that ShareMethod.handle is always run on the background thread, so the database

You could use ThreadUtils.assertOnBackgroundThread to ensure that's true.

@@ +87,5 @@
> +    @Override
> +    protected void init() {
> +        // Reinitialise the UI state intent...
> +        uiStateIntent = new Intent(OverlayConstants.SHARE_METHOD_UI_EVENT);
> +        uiStateIntent.putExtra(OverlayConstants.SHARE_METHOD, (Parcelable) OverlayConstants.ShareType.SEND_TAB);

Everyone who uses this fires it right away. i.e. it doesn't seem to accumulate information. Is there some reason its a field?

@@ +101,5 @@
> +                // We have a Firefox Account, but it's definitely not able to send a tab
> +                // right now. Redirect to the status activity.
> +                Logger.warn(LOGTAG, "Firefox Account named like " + fxAccount.getObfuscatedEmail() +
> +                                             " needs action before it can send a tab; redirecting to status activity.");
> +                redirectToNewTask(FxAccountStatusActivity.class);

I think we might be better to redirect in handle. i.e. we let you "Send to sync", and then if sync isn't set up, we offer to set it up for you.

@@ +131,5 @@
> +
> +    }
> +
> +    private void updateClientList(TabSender tabSender) {
> +        Collection<ClientRecord> otherClients = getOtherClients(tabSender);

final

::: mobile/android/base/overlays/service/sharemethods/ShareMethod.java
@@ +46,5 @@
> +    }
> +
> +    /**
> +     * Show a fancy toast (which may contain useful first-run tutorial diagrams) when the user
> +     * completes a share interaction.

My UX eyes are itching that we should be careful to use normal toast layouts unless we've got something special to show.

@@ +52,5 @@
> +     * @param s Message to display in the toast.
> +     */
> +    protected void showToast(String s) {
> +        LayoutInflater inflater = LayoutInflater.from(context);
> +        View layout = inflater.inflate(org.mozilla.gecko.R.layout.overlay_share_toast, null);

Is this file in a different patch? I would move this inflation down by the setView call, and in the end probably wrap it (and the gravity bit?) in an if() { } so that in the normal case we just use the normal layouts.
Attachment #8476381 - Flags: feedback?(wjohnston) → feedback+
(Assignee)

Updated

5 years ago
Attachment #8476381 - Attachment is obsolete: true
Attachment #8476381 - Flags: feedback?(nalexander)
(Assignee)

Comment 19

5 years ago
Perhaps the dullest patch of all: add a build flag that doesn't do anything.
Attachment #8467512 - Attachment is obsolete: true
Attachment #8478048 - Flags: review?(nalexander)
(Assignee)

Comment 20

5 years ago
We probably never want *both* share handlers to be registered (causes two "Send to Firefox" thingies to appear in the share menu).
So, a fairly dull patch to omit Sync's old share handler when the new one is turned on.
Attachment #8478050 - Flags: review?(rnewman)
(Assignee)

Updated

5 years ago
Attachment #8467513 - Attachment description: Part 2/6: Delete a semantically null static initialiser from BrowserDB → Part 0: Delete a semantically null static initialiser from BrowserDB
(Assignee)

Updated

5 years ago
Attachment #8478050 - Attachment is obsolete: true
Attachment #8478050 - Flags: review?(rnewman)
(Assignee)

Comment 22

5 years ago
(In reply to Wesley Johnston (:wesj) from comment #18)
> Overall I like. I'm not in love with the initialization though. i.e. I'd
> like to not initialize a share provider until we're going to use it. Maybe
> we have to do it this way?

It is necessary to read the database to determine Sync's status so you can correctly show the UI (device list, maybe the button should actually show you "set up sync" instead of sharing? etc.)
In the future, this backend may be shared by a VIEW intent handler that is to support "open tab in background". That will likely want to launch headless Gecko during init, so this sort of lifecycle seemed like a handy thing to have around.
The initialisation mechanism is completely overkill for bookmarks/reading list adding ShareMethods, but I don't think we gain anything much by special-casing them.

This mechanism has the advantage of caching the results of the expensive startup work in the service, allowing the UI to load in the correct state immediately for future shares (no need to do expensive database work the second time round).

> It could be that doesn't work for something like Sync, but I think with Sync
> it would actually be neat if we could Offer to set it up if you click on it
> and its not already set up. Maybe that's what happens here and I'm
> mis-reading. Right now it looked like we just popped it up right away if you
> tried to share and sync wasn't set up (even though reading list doesn't need
> it).
> 
> There are some nits here too, but I quit my "review" mode since this is just
> a f?

Richard's reappearing soon: I was just exploiting you to reduce the amount of iterations needed with him so we can land faster.

> I also think you should split this up and start landing things as soon as
> you can. Its split up well so that you can do that. Little unfinished pieces
> are fine, especially if they're behind build flags or set to nightly-only.

Definitely an excellent idea.

> ::: mobile/android/base/AndroidManifest.xml.in
> @@ +380,5 @@
> >          </activity>
> > +
> > +        <!-- Service to handle requests from overlays. -->
> > +        <service android:name="org.mozilla.gecko.overlays.service.OverlayIntentHandler"
> > +                 android:label="Firefox Sharing Service" />
> 
> You'll need to localize this. I think I'd just use "Firefox Share" too. Or
> just "Firefox"

Removed: It doesn't serve a useful purpose to the user anyway.

> ::: mobile/android/base/overlays/OverlayConstants.java
> @@ +51,5 @@
> > +
> > +        public static final Creator<ShareType> CREATOR = new Creator<ShareType>() {
> > +            @Override
> > +            public ShareType createFromParcel(final Parcel source) {
> > +                return ShareType.values()[source.readInt()];
> 
> You are probably safer to serialize using the string name. i.e. then if the
> order of options in this file changes, things should be ok.

Enum ordinals are set at compile-time. The ordinal of an enum value is determined by its position in the enum declaration in the source.

Since this enum is used for message-passing, we are never going to encounter an enum that was serialised by an older version of Firefox. If we do, its ordinal will only differ if we've added a new enum constant earlier than existing ones in the source.
I don't think it's worth sacrificing the elegance of this approach to handle an edgecase that is both impossible to reproduce, and not useful to resolve even if it somehow manages to happen.

> @@ +146,5 @@
> > +
> > +    // Keys used for parceling ClientRecords.
> > +    public static final String CLIENT_RECORD_NAME = "N";
> > +    public static final String CLIENT_RECORD_TYPE = "T";
> > +    public static final String CLIENT_RECORD_GUID = "G";
> 
> Can you keep these consts with the SyncShare client code? I'd also probably
> use full names. i.e. = "NAME";

*shrug*. They're arbitrary keys. Their value doesn't matter as long as it's unique (though longer uses *slightly* more memory, of course).

> ::: mobile/android/base/overlays/service/OverlayIntentHandler.java
> @@ +67,5 @@
> > +            case ACTION_SERVICE_RE_INIT:
> > +                // Re-initialise the given set of ShareMethods. Useful when the state has changed
> > +                // in ways that invalidates the state cached in the ShareMethods.
> > +                reinitialiseMethods((ShareType[]) intent.getParcelableArrayExtra(SHARE_METHOD));
> > +            break;
> 
> I'm a little confused about why we have to initialize these. I guess we're
> trying to answer "Is sync setup"?

This has since been refactored somewhat. The idea here is that we want an API for re-initialising ShareMethods when state on which they depend has changed.
What if you delete your sync account, for example? We want the ShareMethod to once again point us to the "set up sync" activity if we try and use it, so this mechanism is used to re-run the initialiser code.

> ::: mobile/android/base/overlays/service/sharemethods/AddBookmark.java
> @@ +6,5 @@
> > +import org.mozilla.gecko.R;
> > +import org.mozilla.gecko.db.LocalBrowserDB;
> > +
> > +public class AddBookmark extends ShareMethod {
> > +    private static final String LOGTAG = "AddBookmark";
> 
> We always prefix these with Gecko so they are easy to grep for.

Good plan. IDE templates changed. :P

> @@ +24,5 @@
> > +    protected void init() {
> > +        resolver = context.getContentResolver();
> > +
> > +        // Acquire LocalBrowserDB instance for the default profile.
> > +        browserDB = new LocalBrowserDB(GeckoProfile.DEFAULT_PROFILE);
> 
> Is there some reason to cache resolver or browserDB in init()?

I guess we could just as well lazily-initialise it in handle() for these two boring handlers (reading list and bookmarks).
Might also be worth sharing it between all ShareMethod.

> @@ +87,5 @@
> > +    @Override
> > +    protected void init() {
> > +        // Reinitialise the UI state intent...
> > +        uiStateIntent = new Intent(OverlayConstants.SHARE_METHOD_UI_EVENT);
> > +        uiStateIntent.putExtra(OverlayConstants.SHARE_METHOD, (Parcelable) OverlayConstants.ShareType.SEND_TAB);
> 
> Everyone who uses this fires it right away. i.e. it doesn't seem to
> accumulate information. Is there some reason its a field?

We want to update it and re-dispatch it with the complete UI state information whenever something interesting happens.
When it's a field, we get to do "Write the new interesting thing. Dispatch the intent (which we know also has all the other state in it)".
Otherwise, we'd have to do "Build from scratch a new intent with the several fields we care about in it". This would be longer, slightly more irritating, and slightly more expensive.

> @@ +101,5 @@
> > +                // We have a Firefox Account, but it's definitely not able to send a tab
> > +                // right now. Redirect to the status activity.
> > +                Logger.warn(LOGTAG, "Firefox Account named like " + fxAccount.getObfuscatedEmail() +
> > +                                             " needs action before it can send a tab; redirecting to status activity.");
> > +                redirectToNewTask(FxAccountStatusActivity.class);
> 
> I think we might be better to redirect in handle. i.e. we let you "Send to
> sync", and then if sync isn't set up, we offer to set it up for you.

Looks like I need to rename this method. All it does is writes an intent to the UI state intent so that UIs connected to the service know that when the user asks to use this ShareMethod we should really fire this intent.

> 
> @@ +131,5 @@
> > +
> > +    }
> > +
> > +    private void updateClientList(TabSender tabSender) {
> > +        Collection<ClientRecord> otherClients = getOtherClients(tabSender);
> 
> final

What for?

Making local variables final is of little use unless it significantly clarifies code (or removes a footgun, such as if you're about to synchronise on it).
From a performance standpoint, having marked it as final does make the JIT's job very slightly easier, so you may see slight performance gains in some situations. But, Proguard uses static analysis to mark as final everything it can prove is only written once. As a result, you only gain performance from doing it in the source in places where the dataflow situation is sufficiently complicated that Proguard can't see through it.

In this case, declaring otherClients final doesn't seem to lend any clarity. Indeed, the collection remains mutable even if we do mark it final, so one might argue that declaring it final may lull a tired developer into misreading it to mean the collection is immutable when it's not. Certainly, I'm not seeing why otherClients being final is any more useful than doing so for any of the (many) other local variables that are effectively-final. Including that there parameter.
Perhaps I missed something?

> ::: mobile/android/base/overlays/service/sharemethods/ShareMethod.java
> @@ +46,5 @@
> > +    }
> > +
> > +    /**
> > +     * Show a fancy toast (which may contain useful first-run tutorial diagrams) when the user
> > +     * completes a share interaction.
> 
> My UX eyes are itching that we should be careful to use normal toast layouts
> unless we've got something special to show.

We do: yuan wants fancy first-run tutorial image thingies: Bug 1048645

> @@ +52,5 @@
> > +     * @param s Message to display in the toast.
> > +     */
> > +    protected void showToast(String s) {
> > +        LayoutInflater inflater = LayoutInflater.from(context);
> > +        View layout = inflater.inflate(org.mozilla.gecko.R.layout.overlay_share_toast, null);
> 
> Is this file in a different patch? I would move this inflation down by the
> setView call, and in the end probably wrap it (and the gravity bit?) in an
> if() { } so that in the normal case we just use the normal layouts.

Which "normal case" are you referring to? We never want a vanilla toast - see Yuan's pretty pictures: http://delivery.yuuuan.com/du9yadac
(Assignee)

Comment 23

5 years ago
(In reply to Wesley Johnston (:wesj) from comment #18)
> ::: mobile/android/base/overlays/OverlayConstants.java
> @@ +51,5 @@
> > +
> > +        public static final Creator<ShareType> CREATOR = new Creator<ShareType>() {
> > +            @Override
> > +            public ShareType createFromParcel(final Parcel source) {
> > +                return ShareType.values()[source.readInt()];
> 
> You are probably safer to serialize using the string name. i.e. then if the
> order of options in this file changes, things should be ok.

Oh, I almost forgot. 

"Parcel is not a general-purpose serialization mechanism. This class (and the corresponding Parcelable API for placing arbitrary objects into a Parcel) is designed as a high-performance IPC transport. As such, it is not appropriate to place any Parcel data in to persistent storage: changes in the underlying implementation of any of the data in the Parcel can render older data unreadable."

http://developer.android.com/reference/android/os/Parcel.html

Android already breaks the promise of future-decodability. That's one of the reasons Parcelable is disjoint from Serializable. There is, then, no point going to extra effort to try and keep the promise on our side.
(Assignee)

Comment 24

5 years ago
Same thing as Wes looked at earlier, but refactored to be nicer, with a few bugs fixed, and with the review comments addresses.
Attachment #8478194 - Flags: review?(rnewman)
(Assignee)

Comment 25

5 years ago
The hooks into Sync-land to re-initialise the ShareMethod (if the service is running) when the set of available ClientRecords changes.

I dislike the way I ended up repeating myself in this patch. Is there a better way to do "Make this happen whenever the list of ClientRecords changes" what what I've got, rnewman?
Attachment #8478196 - Flags: review?(rnewman)
(Assignee)

Updated

5 years ago
Attachment #8478196 - Attachment description: Part 4:Sync bindings for the Send Tab ShareMethod → Part 4: Sync bindings for the Send Tab ShareMethod
(Assignee)

Comment 26

5 years ago
... Now with more qref.
Attachment #8478194 - Attachment is obsolete: true
Attachment #8478194 - Flags: review?(rnewman)
Attachment #8478205 - Flags: review?(rnewman)
Comment on attachment 8478048 [details] [diff] [review]
Part 1: Add build flag for overlays stuff.

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

lgtm.  Blog posting for the win!
Attachment #8478048 - Flags: review?(nalexander) → review+
Comment on attachment 8478053 [details] [diff] [review]
Part 2: Conditionalise sync's share handler on the new build flag (V2)

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

Sure.
Attachment #8478053 - Flags: review?(rnewman) → review+
Comment on attachment 8478196 [details] [diff] [review]
Part 4: Sync bindings for the Send Tab ShareMethod

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

This couples some of the android-sync internals quite tightly to the new share handler work.  But I'd like to see us take a different approach here.

I just landed new AccountsLoader logic in Bug 1026005; I'll need to see what the share overlay UI really looks like to figure out what the right way to inform the overlay handler of account state changes is, but it might be that.  It might be Android system level, or local, broadcasts -- I think we already have these in place for account deletion.  We could add these per-Sync-stage.

Thanks for separating this out so clearly, so that we can iterate on this little piece separately.
Attachment #8478196 - Flags: feedback-
Comment on attachment 8478205 [details] [diff] [review]
Part 3: A service for handling share intents. (V2)

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

Partial review only, but:

In general, I found this well documented and easy to read.  Big thumbs up for the work that went into that.

My over-arching question is: why is this centralized intent service better than just having a bunch of bespoke intent services for each share method?  I was led to this question by trying to understand why we need this initialise/re-initialise boilerplate.  I re-read your comment about "expensive DB initialization" and fetching state, and I don't think I buy it.  This stuff happens at "human speed"; I'm struggling to understand what exactly is so expensive here.  Are we displaying bookmark state in the UI?  (How is initializing the DB going to help with that query?)  Are we reflecting the Sync account state?  That's not much more expensive to query than to maintain over time.

My only significant concrete code comment is that we should use IntentService; it makes background processing better.  I'm not clear on why you haven't, but perhaps I'm missing something.

Again, thanks for the clear commenting throughout, and for the explanation of what you're trying to achieve with the design.

::: mobile/android/base/overlays/OverlayConstants.java
@@ +17,5 @@
> + * The intent API used by the service is defined herein.
> + */
> +public class OverlayConstants {
> +    /**
> +     * Enum representing the types of share operation supported by the share overlay system.

This is all seems fancy, probably unecessary, and rather brittle.  It ties the creation of intents to the creation of handlers via the singletons, and makes registering new share handlers a static/code thing rather than a dynamic registry thing.  All ShareMethods are going to require an Android context; it's weird to have a static singleton encapsulate a context.  (I may be wrong on how the singletons are initialized; will keep reading.)

My preference would be to drop the fancy intent handling stuff (JSON strings for the simple win!) and to register more dynamically.

@@ +94,5 @@
> +     * Action for sharing a page.
> +     *
> +     * Intent parameters:
> +     *
> +     * $URL: URL of page to share.    (required)

Is this $ notation standard?  In general, Javadoc uses embedded HTML, doesn't it?

@@ +139,5 @@
> +    /*
> +     * ShareMethod-specific constants.
> +     */
> +    // Key used in the extras Bundle in the share intent used for a send tab ShareMethod.
> +    public static final String SEND_TAB_TARGET_DEVICES = "SEND_TAB_TARGET_DEVICES";

I have a mild preference to have these in the relevant ShareMethod classes.

::: mobile/android/base/overlays/service/OverlayIntentHandler.java
@@ +38,5 @@
> + * * Neither of these incur a page fetch on the service... yet. That will require headless Gecko,
> + *   something we're yet to have. Refactoring Gecko as a service itself and restructing the rest of
> + *   the app to talk to it seems like the way to go there.
> + */
> +public class OverlayIntentHandler extends Service {

nit: s/Handler/Service/.  Handler means something in Android.

Explain why this isn't an IntentService.  What are you doing that requires non-trivial service lifecycle handling?

@@ +82,5 @@
> +
> +    @Override
> +    public int onStartCommand(Intent intent, int flags, int startId) {
> +        // Dispatch intent to appropriate method according to its action.
> +        String action = intent.getAction();

intent and action can be null.

@@ +96,5 @@
> +            default:
> +                throw new IllegalArgumentException("Unsupported intent action: " + action);
> +        }
> +
> +        // Register a broadcast listener to handle partial service reinit.

I think this should be in onCreate.  If not, this needs a comment to explain why you'll never get two onStartCommand calls.  Or is it okay to registerReceivers multiple time?  Even if so, it's really strange looking.

@@ +119,5 @@
> +                Parcelable extra = extras.getParcelable(OverlayConstants.EXTRA);
> +
> +                Object shareMethodTypes = extras.get(SHARE_METHOD);
> +
> +                // Deduce the list of ShareMethods to use. SHARE_METHOD may be either one method or

Why?  This kind of type-plurality is not idiomatic Java.

::: mobile/android/base/overlays/service/sharemethods/ShareMethod.java
@@ +11,5 @@
> +
> +import java.util.concurrent.atomic.AtomicBoolean;
> +
> +/**
> + * Represents of sharing a url/title. Add a bookmark? Send to a device? Add to reading list?

nit: grammar.  "Represents a method of sharing..., such as "Adding a bookmark", etc."

@@ +16,5 @@
> + *
> + * A ShareMethod consists of a handle() method, to perform the actual sharing operation, as well as
> + * an init() method to perform any preparatory work that must occur before handle() can function
> + * correctly.
> + *

nit: blank comment lines?

@@ +22,5 @@
> + */
> +public abstract class ShareMethod {
> +    protected Context context;
> +
> +    AtomicBoolean isInitialised = new AtomicBoolean(false);

nit: final?

@@ +35,5 @@
> +     * Perform a share for the given title/url combination. Called on the background thread by the
> +     * handler service when a request is made. The "extra" parameter is provided should a ShareMethod
> +     * desire to handle the share differently based on some additional parameters.
> +     */
> +    public abstract void handle(String url, String title, Parcelable extra);

nit: (but a big nit): we're pretty consistent about title/url, in that order (like the comment), throughout the Fennec codebase.  It's very rare to *display* URL before title.  I'd prefer to standardize on (title, URL) early.

@@ +45,5 @@
> +        handle(url, title, null);
> +    }
> +
> +    /**
> +     * When called, should broadcast information useful for a UI representing this ShareMethod to

I prefer just "Broadcast this ShareMethod's state/definition."  Nothing happens until things are called; anything broadcast is information by definition; it's hard to say what is useful; and of course it's broadcasting to listeners :)

@@ +62,5 @@
> +     * @param s Message to display in the toast.
> +     */
> +    protected void showToast(String s) {
> +        LayoutInflater inflater = LayoutInflater.from(context);
> +        View layout = inflater.inflate(org.mozilla.gecko.R.layout.overlay_share_toast, null);

I'd like to see this be just a regular toast at first; we can "progressively enhance" after the initial landing.

@@ +95,5 @@
> +        isInitialised.set(false);
> +    }
> +
> +    public void reinitialise() {
> +        uninitialise();

Is this necessary?  And this whole approach with AtomicBoolean doesn't stop you racing, right?  (Your call to uninitialise could land between the ensureInitialised compareAndSet and the actual init() call, allowing a second ensureInitialised to go through.)  Am I missing something?

@@ +99,5 @@
> +        uninitialise();
> +        ensureInitialised(context);
> +    }
> +
> +    protected ShareMethod() {}

Does this do something?  You're already abstract, so you can't instantiate this; and this is protected, so any package can extend ShareMethod...

::: mobile/android/base/overlays/service/sharemethods/sendtab/SendTab.java
@@ +230,5 @@
> +        if (ourGUID == null) {
> +            return all.values();
> +        }
> +
> +        final ArrayList<ClientRecord> out = new ArrayList<>(all.size());

This isn't OK until we land 1.7.  Let's try to break this dep.
(Assignee)

Comment 31

5 years ago
(In reply to Nick Alexander :nalexander from comment #29)
> This couples some of the android-sync internals quite tightly to the new
> share handler work.  But I'd like to see us take a different approach here.

Indeed, this is sort of unpleasant.
What I really want is some way to add an event listener that's fired when a sync completes: this mess is just an attempt to figure out when the list of available clients has changed for some reason.

> I just landed new AccountsLoader logic in Bug 1026005; I'll need to see what
> the share overlay UI really looks like to figure out what the right way to
> inform the overlay handler of account state changes is, but it might be
> that.  It might be Android system level, or local, broadcasts -- I think we
> already have these in place for account deletion.  We could add these
> per-Sync-stage.

That sounds like it'd do the trick...


(In reply to Nick Alexander :nalexander from comment #30)
> My over-arching question is: why is this centralized intent service better
> than just having a bunch of bespoke intent services for each share method?

It makes it easier to share state, such as LocalBrowserDB instances (though it occurs to me that that's something I've so far omitted to do).
It also reduces space overheads associated with running more services.

> I was led to this question by trying to understand why we need this
> initialise/re-initialise boilerplate.  I re-read your comment about
> "expensive DB initialization" and fetching state, and I don't think I buy
> it.  This stuff happens at "human speed"; I'm struggling to understand what
> exactly is so expensive here.  Are we displaying bookmark state in the UI? 
> (How is initializing the DB going to help with that query?)  Are we
> reflecting the Sync account state?  That's not much more expensive to query
> than to maintain over time.

This mechanism doesn't really gain us anything at all for the bookmarks/reading list ShareMethods.
We need to fetch Sync's state to present a list of devices the user may wish to share to. If we poll that when the overlay is opened there's an unpleasant flicker (and on a slow device a quite noticeable pause) before the buttons become available. That seems like it'd get frustrating if you're trying to send a bunch of tabs to a device.

The other thing to consider is where future work here is going. This backend is to be shared by the future work that handles VIEW intents. One of the things that's supposed to be able to do is open tabs in the background. Eventually, that'll be implemented by starting Gecko headlessly from the service (and the headless Gecko instance could then be used to do things like fill in missing titles/favicons in the background).
Since starting Gecko definitely counts as "expensive", and something we'd want to share between all ShareMethods, I decided to over-engineer a little a this stage, in an attempt to make future work go more smoothly.

Lastly, note that if the service is not running, the cost of maintaining the state over time is just the cost of an unreceived broadcast. It may prove sensible to have some heuristic for shutting down the service if it goes unused for a long time, but at the moment I preferred to cache the state for as long as Android will let us.

> My only significant concrete code comment is that we should use
> IntentService; it makes background processing better.  I'm not clear on why
> you haven't, but perhaps I'm missing something.

I think the main reason is probably incompetence. I'll refactor as an IntentService and let you know if a better reason to not do so emerges.

> ::: mobile/android/base/overlays/OverlayConstants.java
> @@ +17,5 @@
> > + * The intent API used by the service is defined herein.
> > + */
> > +public class OverlayConstants {
> > +    /**
> > +     * Enum representing the types of share operation supported by the share overlay system.
> 
> This is all seems fancy, probably unecessary, and rather brittle.  It ties
> the creation of intents to the creation of handlers via the singletons, and
> makes registering new share handlers a static/code thing rather than a
> dynamic registry thing. 

I thought about that, but adding new ones didn't seem a useful thing to be able to do at runtime.

That said, maybe it is: this seems like something the addons API might want to hook up to in the future, after all...

> All ShareMethods are going to require an Android
> context; it's weird to have a static singleton encapsulate a context.  (I
> may be wrong on how the singletons are initialized; will keep reading.)

If not as singletons, how else should a ShareMethod be represented? We only want one instance of the service and it only wants one instance of each method, so it sort of makes sense.
Perhaps I've missed the point that makes you feel this is "weird"?

> 
> My preference would be to drop the fancy intent handling stuff (JSON strings
> for the simple win!) and to register more dynamically.
> 
> @@ +94,5 @@
> > +     * Action for sharing a page.
> > +     *
> > +     * Intent parameters:
> > +     *
> > +     * $URL: URL of page to share.    (required)
> 
> Is this $ notation standard?  In general, Javadoc uses embedded HTML,
> doesn't it?

Unclear. I was optimising for readability in code, since we still don't appear to be generating javadocs from our javadoc comments (so, yes, while you can jam fancy HTML formatting and hyperlinks into the javadoc comments they're not particularly useful since we never parse it)

> @@ +96,5 @@
> > +            default:
> > +                throw new IllegalArgumentException("Unsupported intent action: " + action);
> > +        }
> > +
> > +        // Register a broadcast listener to handle partial service reinit.
> 
> I think this should be in onCreate.  If not, this needs a comment to explain
> why you'll never get two onStartCommand calls.  Or is it okay to
> registerReceivers multiple time?  Even if so, it's really strange looking.

Yes, you're probably right about the risk of doing this twice.

> @@ +119,5 @@
> > +                Parcelable extra = extras.getParcelable(OverlayConstants.EXTRA);
> > +
> > +                Object shareMethodTypes = extras.get(SHARE_METHOD);
> > +
> > +                // Deduce the list of ShareMethods to use. SHARE_METHOD may be either one method or
> 
> Why?  This kind of type-plurality is not idiomatic Java.

Trying to make a nice API. On second thoughts this is actually a pretty stupid idea: I'll get rid of it.

> @@ +35,5 @@
> > +     * Perform a share for the given title/url combination. Called on the background thread by the
> > +     * handler service when a request is made. The "extra" parameter is provided should a ShareMethod
> > +     * desire to handle the share differently based on some additional parameters.
> > +     */
> > +    public abstract void handle(String url, String title, Parcelable extra);
> 
> nit: (but a big nit): we're pretty consistent about title/url, in that order
> (like the comment), throughout the Fennec codebase.  It's very rare to
> *display* URL before title.  I'd prefer to standardize on (title, URL) early.

Ah.
I'd been trying to keep my code consistently doing url, title. I'll turn them round.

Oh how much I love IDEA's "change method signature" button. :P

> @@ +62,5 @@
> > +     * @param s Message to display in the toast.
> > +     */
> > +    protected void showToast(String s) {
> > +        LayoutInflater inflater = LayoutInflater.from(context);
> > +        View layout = inflater.inflate(org.mozilla.gecko.R.layout.overlay_share_toast, null);
> 
> I'd like to see this be just a regular toast at first; we can "progressively
> enhance" after the initial landing.

I went this way simply because it's what Yuan drew:
http://delivery.yuuuan.com/du9yadac

> @@ +95,5 @@
> > +        isInitialised.set(false);
> > +    }
> > +
> > +    public void reinitialise() {
> > +        uninitialise();
> 
> Is this necessary?  And this whole approach with AtomicBoolean doesn't stop
> you racing, right?  (Your call to uninitialise could land between the
> ensureInitialised compareAndSet and the actual init() call, allowing a
> second ensureInitialised to go through.)  Am I missing something?

Uh, nope. That needs to be fixed.

> 
> @@ +99,5 @@
> > +        uninitialise();
> > +        ensureInitialised(context);
> > +    }
> > +
> > +    protected ShareMethod() {}
> 
> Does this do something?  You're already abstract, so you can't instantiate
> this; and this is protected, so any package can extend ShareMethod...

Also pointless, yes.

> 
> ::: mobile/android/base/overlays/service/sharemethods/sendtab/SendTab.java
> @@ +230,5 @@
> > +        if (ourGUID == null) {
> > +            return all.values();
> > +        }
> > +
> > +        final ArrayList<ClientRecord> out = new ArrayList<>(all.size());
> 
> This isn't OK until we land 1.7.  Let's try to break this dep.

Yes, doing so is completely trivial, but this code makes lots of use of things like strings-in-switch which get quite ugly if I end up having to push the "switch to source level 6" button.
It seems like this code will spend long enough in review hell for 1.7 to finish breaking the world. If not, I'll make it untidy before landing it so attempts to back out 1.7 don't mandate backing out this stuff.
(Assignee)

Comment 32

5 years ago
(In reply to Nick Alexander :nalexander from comment #30)
> My only significant concrete code comment is that we should use
> IntentService; it makes background processing better.  I'm not clear on why
> you haven't, but perhaps I'm missing something.

I remembered why:
IntentServices are designed to run while they have work in their queue. Once they run out, they stop.

Part of the goal of my service is to keep state around that's useful for handling requests. If we're ultimately going to want to be running Gecko in the background to load tabs and things, we're not going to want to shut that down and restart it each time the user sends us a share request.
Similarly, it's slightly advantageous to be able to avoid needing to hit the Sync database before we can show the UI properly each time a user performs a share.

On the other hand, I can get both by just calling onStartCommand on an IntentService so it doesn't die as soon as the work queue runs out. That introduces another thread, though, which doesn't seem to be necessary (we can just run the work we want to do on the existing background thread). I'm slightly afraid that some of the large amount of places where we assume things are on The Background Thread will break if the overlay service starts doing things in its own thread.
(In reply to Chris Kitching [:ckitching] from comment #31)

> What I really want is some way to add an event listener that's fired when a
> sync completes: this mess is just an attempt to figure out when the list of
> available clients has changed for some reason.

You probably want a dataset observer on the clients table. And then to be smart about it, because the clients stage tends to delete everything then insert everything, so you'll want to wait for an insert, then compare it to your cached version.


> > My over-arching question is: why is this centralized intent service better
> > than just having a bunch of bespoke intent services for each share method?
>
> It makes it easier to share state, such as LocalBrowserDB instances (though
> it occurs to me that that's something I've so far omitted to do).
> It also reduces space overheads associated with running more services.

I strongly encourage the simplest model here. There's precisely one piece of dynamism: the list of sync clients. Cache that and observe the table so that your list is up to date. We don't care all that much if it's stale, either, so you could even just refresh your cache *after* displaying the stale data, and do away with the observer.

Every other action is hard-coded (we even know which profile we'll be writing to!), and won't be growing without design and interaction work. So hard-code it, even using switch statements. It'll end up small and simple.

You don't even need to cache LocalBrowserDB instances: they're relatively cheap. We should literally be instantiating UI to pick a thing, and then running code to do the right thing with the right DB. Nothing more. YAGNI.
 

> The other thing to consider is where future work here is going. This backend
> is to be shared by the future work that handles VIEW intents. One of the
> things that's supposed to be able to do is open tabs in the background.
> Eventually, that'll be implemented by starting Gecko headlessly from the
> service (and the headless Gecko instance could then be used to do things
> like fill in missing titles/favicons in the background).

But that Gecko *will be* Fennec's Gecko. That won't be owned by the share handler; the share handler will have a communication channel with (and a keepalive reference to) Fennec's Gecko thread, probably 'steering' an invisible window, and Fennec will sit on top as needed.


> That said, maybe it is: this seems like something the addons API might want
> to hook up to in the future, after all...

Not in scope. I want this to land ASAP.


> If not as singletons, how else should a ShareMethod be represented? 

How about as an enum, or an integer, or a string? After all, the only thing being done here is picking what to do with a URL and a title, then immediately doing that thing. There's very little need, IMO, to abstract this.
Oh, and in favor of a simple transient service: the clients table has a CP, which means Android already maintains a single connection to it. It's a small DB, so it's probably entirely held in memory. Queries against it will take 50 milliseconds on average. If that takes too long, *have the ContentProvider cache values in-memory*. Voila: a stable cache, and your service doesn't need to do the work.
Mark, could you and Chris find time to chat about this briefly?
Flags: needinfo?(mgoodwin)
(In reply to Richard Newman [:rnewman] from comment #35)
> Mark, could you and Chris find time to chat about this briefly?

Sure
Flags: needinfo?(mgoodwin)
Comment on attachment 8478205 [details] [diff] [review]
Part 3: A service for handling share intents. (V2)

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

Clearing flags until (a) simplification and Nick's comments are addressed, and (b) TODOs are addressed.
Attachment #8478205 - Flags: review?(rnewman)
Comment on attachment 8478196 [details] [diff] [review]
Part 4: Sync bindings for the Send Tab ShareMethod

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

Clearing flags until Nick's comments are addressed.
Attachment #8478196 - Flags: review?(rnewman)
Hey!  This is going to be a little hard to read, but I'll try to
synthesize my responses with rnewman's.

(In reply to Chris Kitching [:ckitching] from comment #31)
> (In reply to Nick Alexander :nalexander from comment #29)
> > This couples some of the android-sync internals quite tightly to the new
> > share handler work.  But I'd like to see us take a different approach here.
> 
> Indeed, this is sort of unpleasant.
> What I really want is some way to add an event listener that's fired when a
> sync completes: this mess is just an attempt to figure out when the list of
> available clients has changed for some reason.
> 
> > I just landed new AccountsLoader logic in Bug 1026005; I'll need to see what
> > the share overlay UI really looks like to figure out what the right way to
> > inform the overlay handler of account state changes is, but it might be
> > that.  It might be Android system level, or local, broadcasts -- I think we
> > already have these in place for account deletion.  We could add these
> > per-Sync-stage.
> 
> That sounds like it'd do the trick...

Per rnewman, I think the right thing to do here is to condition on
Fennec's TabsProvider's clients table.  I think it's reasonable to
CursorLoad that as part of your UI display, filter out local clients,
and hide the Send Tab option if there are none.  This is not quite as
authoritative as querying the Firefox Account and Sync state, but post
Bug 830270, it's close.  In the rare situations where you have an
Account but can't Send Tab due to being in a bad state, the immediate
sync that happens during a Send Tab should pop up a "busted!!!"
notification.

> (In reply to Nick Alexander :nalexander from comment #30)
> > My over-arching question is: why is this centralized intent service better
> > than just having a bunch of bespoke intent services for each share method?
> 
> It makes it easier to share state, such as LocalBrowserDB instances (though
> it occurs to me that that's something I've so far omitted to do).
> It also reduces space overheads associated with running more services.
> 
> > I was led to this question by trying to understand why we need this
> > initialise/re-initialise boilerplate.  I re-read your comment about
> > "expensive DB initialization" and fetching state, and I don't think I buy
> > it.  This stuff happens at "human speed"; I'm struggling to understand what
> > exactly is so expensive here.  Are we displaying bookmark state in the UI? 
> > (How is initializing the DB going to help with that query?)  Are we
> > reflecting the Sync account state?  That's not much more expensive to query
> > than to maintain over time.
> 
> This mechanism doesn't really gain us anything at all for the
> bookmarks/reading list ShareMethods.
> We need to fetch Sync's state to present a list of devices the user may wish
> to share to. If we poll that when the overlay is opened there's an
> unpleasant flicker (and on a slow device a quite noticeable pause) before
> the buttons become available. That seems like it'd get frustrating if you're
> trying to send a bunch of tabs to a device.
> 
> The other thing to consider is where future work here is going. This backend
> is to be shared by the future work that handles VIEW intents. One of the
> things that's supposed to be able to do is open tabs in the background.
> Eventually, that'll be implemented by starting Gecko headlessly from the
> service (and the headless Gecko instance could then be used to do things
> like fill in missing titles/favicons in the background).
> Since starting Gecko definitely counts as "expensive", and something we'd
> want to share between all ShareMethods, I decided to over-engineer a little
> a this stage, in an attempt to make future work go more smoothly.
> 
> Lastly, note that if the service is not running, the cost of maintaining the
> state over time is just the cost of an unreceived broadcast. It may prove
> sensible to have some heuristic for shutting down the service if it goes
> unused for a long time, but at the moment I preferred to cache the state for
> as long as Android will let us.

I think we should err on the side of not maintaining any state, at least
for the initial cut.  Let's see what it feels like to do this as part of
the UI initialization; we can investigate caching in the future.

> > My only significant concrete code comment is that we should use
> > IntentService; it makes background processing better.  I'm not clear on why
> > you haven't, but perhaps I'm missing something.
> 
> I think the main reason is probably incompetence. I'll refactor as an
> IntentService and let you know if a better reason to not do so emerges.
> 
> > ::: mobile/android/base/overlays/OverlayConstants.java
> > @@ +17,5 @@
> > > + * The intent API used by the service is defined herein.
> > > + */
> > > +public class OverlayConstants {
> > > +    /**
> > > +     * Enum representing the types of share operation supported by the share overlay system.
> > 
> > This is all seems fancy, probably unecessary, and rather brittle.  It ties
> > the creation of intents to the creation of handlers via the singletons, and
> > makes registering new share handlers a static/code thing rather than a
> > dynamic registry thing. 
> 
> I thought about that, but adding new ones didn't seem a useful thing to be
> able to do at runtime.

I agree.  I still find the singletons a little brittle, but I withdraw
the thoughts on runtime registration.   Roll on.

> That said, maybe it is: this seems like something the addons API might want
> to hook up to in the future, after all...

I agree with rnewman here: future.

> > All ShareMethods are going to require an Android
> > context; it's weird to have a static singleton encapsulate a context.  (I
> > may be wrong on how the singletons are initialized; will keep reading.)
> 
> If not as singletons, how else should a ShareMethod be represented? We only
> want one instance of the service and it only wants one instance of each
> method, so it sort of makes sense.
> Perhaps I've missed the point that makes you feel this is "weird"?

How would you feel about straight enums and switch statements?

> > 
> > My preference would be to drop the fancy intent handling stuff (JSON strings
> > for the simple win!) and to register more dynamically.
> > 
> > @@ +94,5 @@
> > > +     * Action for sharing a page.
> > > +     *
> > > +     * Intent parameters:
> > > +     *
> > > +     * $URL: URL of page to share.    (required)
> > 
> > Is this $ notation standard?  In general, Javadoc uses embedded HTML,
> > doesn't it?
> 
> Unclear. I was optimising for readability in code, since we still don't
> appear to be generating javadocs from our javadoc comments (so, yes, while
> you can jam fancy HTML formatting and hyperlinks into the javadoc comments
> they're not particularly useful since we never parse it)

Sure.  I personally don't mind parsing HTML visually, but I won't
require others to do so.  Your call.

> > @@ +96,5 @@
> > > +            default:
> > > +                throw new IllegalArgumentException("Unsupported intent action: " + action);
> > > +        }
> > > +
> > > +        // Register a broadcast listener to handle partial service reinit.
> > 
> > I think this should be in onCreate.  If not, this needs a comment to explain
> > why you'll never get two onStartCommand calls.  Or is it okay to
> > registerReceivers multiple time?  Even if so, it's really strange looking.
> 
> Yes, you're probably right about the risk of doing this twice.

If we end up with this, let's put the reg/dereg in onCreate/onDestroy, then.

> > @@ +119,5 @@
> > > +                Parcelable extra = extras.getParcelable(OverlayConstants.EXTRA);
> > > +
> > > +                Object shareMethodTypes = extras.get(SHARE_METHOD);
> > > +
> > > +                // Deduce the list of ShareMethods to use. SHARE_METHOD may be either one method or
> > 
> > Why?  This kind of type-plurality is not idiomatic Java.
> 
> Trying to make a nice API. On second thoughts this is actually a pretty
> stupid idea: I'll get rid of it.

Not stupid at all!  But I think ShareMethod[] will be plenty.

> > @@ +35,5 @@
> > > +     * Perform a share for the given title/url combination. Called on the background thread by the
> > > +     * handler service when a request is made. The "extra" parameter is provided should a ShareMethod
> > > +     * desire to handle the share differently based on some additional parameters.
> > > +     */
> > > +    public abstract void handle(String url, String title, Parcelable extra);
> > 
> > nit: (but a big nit): we're pretty consistent about title/url, in that order
> > (like the comment), throughout the Fennec codebase.  It's very rare to
> > *display* URL before title.  I'd prefer to standardize on (title, URL) early.
> 
> Ah.
> I'd been trying to keep my code consistently doing url, title. I'll turn
> them round.
> 
> Oh how much I love IDEA's "change method signature" button. :P
> 
> > @@ +62,5 @@
> > > +     * @param s Message to display in the toast.
> > > +     */
> > > +    protected void showToast(String s) {
> > > +        LayoutInflater inflater = LayoutInflater.from(context);
> > > +        View layout = inflater.inflate(org.mozilla.gecko.R.layout.overlay_share_toast, null);
> > 
> > I'd like to see this be just a regular toast at first; we can "progressively
> > enhance" after the initial landing.
> 
> I went this way simply because it's what Yuan drew:
> http://delivery.yuuuan.com/du9yadac

OK, roger this.  This is a link between the front and back end; can it
be moved to the front end code for landing?  For example, it could be a
static helper method.

> > @@ +95,5 @@
> > > +        isInitialised.set(false);
> > > +    }
> > > +
> > > +    public void reinitialise() {
> > > +        uninitialise();
> > 
> > Is this necessary?  And this whole approach with AtomicBoolean doesn't stop
> > you racing, right?  (Your call to uninitialise could land between the
> > ensureInitialised compareAndSet and the actual init() call, allowing a
> > second ensureInitialised to go through.)  Am I missing something?
> 
> Uh, nope. That needs to be fixed.

I'm hoping we can remove the mechanism entirely.  Try it on for size?

> > 
> > @@ +99,5 @@
> > > +        uninitialise();
> > > +        ensureInitialised(context);
> > > +    }
> > > +
> > > +    protected ShareMethod() {}
> > 
> > Does this do something?  You're already abstract, so you can't instantiate
> > this; and this is protected, so any package can extend ShareMethod...
> 
> Also pointless, yes.
> 
> > 
> > ::: mobile/android/base/overlays/service/sharemethods/sendtab/SendTab.java
> > @@ +230,5 @@
> > > +        if (ourGUID == null) {
> > > +            return all.values();
> > > +        }
> > > +
> > > +        final ArrayList<ClientRecord> out = new ArrayList<>(all.size());
> > 
> > This isn't OK until we land 1.7.  Let's try to break this dep.
> 
> Yes, doing so is completely trivial, but this code makes lots of use of
> things like strings-in-switch which get quite ugly if I end up having to
> push the "switch to source level 6" button.
> It seems like this code will spend long enough in review hell for 1.7 to
> finish breaking the world. If not, I'll make it untidy before landing it so
> attempts to back out 1.7 don't mandate backing out this stuff.

In light of the recent 1.7 back out (sorry, that was my oversight!) I
think de-coupling sounds good.  In general, I prefer switching on closed
data types anyway.

Next steps
==========

Overall, I'm really pretty happy with these patches.  I don't think we
should land in this form, but I don't think a simpler form is far away.
I'd like to see two things:

1) a fresh version, with the small changes I suggested here and as much
of the simplifications that rnewman and I recommend as you think wise;

2) the front-end patches, to get a better understanding of how this
stuff will be used, and to really nail down the questions about
initialization and caching.
(Assignee)

Comment 40

5 years ago
Comment on attachment 8478196 [details] [diff] [review]
Part 4: Sync bindings for the Send Tab ShareMethod

Don't need this insanity any more, then...
Attachment #8478196 - Attachment is obsolete: true
(Assignee)

Comment 41

5 years ago
The massively over-engineered lifecycle management system is no more.

A static helper class is now used to create toasts (which have had a "retry-on-failure" feature added as per yuan's latest work). The toast-creation stuff has been omitted from this patch (if this one lands first I'll drop in some stubs).

So, currently, the overlay opens, sends a "prepare" intent to the service (which informs it of the need to do any work required to display the UI properly and broadcast it). It responds by initialising ShareMethods, each of which does the bare minimum work necessary to show the user a correct UI. At the moment, that's just SendTab loading the Sync clients. This will hopefully complete and throw a broadcast back to the UI before the 500ms introductory animation has concluded. The crazy partial-reinitialisation mechanism is no longer necessary, as we simply hit the DB every single time. That does rather simplify matters.

(In reply to Nick Alexander :nalexander from comment #39)
> Per rnewman, I think the right thing to do here is to condition on
> Fennec's TabsProvider's clients table.  I think it's reasonable to
> CursorLoad that as part of your UI display, filter out local clients,
> and hide the Send Tab option if there are none.  This is not quite as
> authoritative as querying the Firefox Account and Sync state, but post
> Bug 830270, it's close.  In the rare situations where you have an
> Account but can't Send Tab due to being in a bad state, the immediate
> sync that happens during a Send Tab should pop up a "busted!!!"
> notification.

By just hitting the DB every time in much the same way as the old SendTabActivity did, that's no longer necessary.

As an added bonus, such work allows us to handle broken Sync accounts elegantly (without the slightly-confusing interaction you describe).

> How would you feel about straight enums and switch statements?

That's largely what I now have: as I mention above, I'm slightly keen to keep the logic for the various methods separated to avoid spaghettification.

(In reply to Richard Newman [:rnewman] from comment #33)
> I strongly encourage the simplest model here. There's precisely one piece of
> dynamism: the list of sync clients. Cache that and observe the table so that
> your list is up to date. We don't care all that much if it's stale, either,
> so you could even just refresh your cache *after* displaying the stale data,
> and do away with the observer.
> 
> Every other action is hard-coded (we even know which profile we'll be
> writing to!), and won't be growing without design and interaction work. So
> hard-code it, even using switch statements. It'll end up small and simple.
> 
> You don't even need to cache LocalBrowserDB instances: they're relatively
> cheap. We should literally be instantiating UI to pick a thing, and then
> running code to do the right thing with the right DB. Nothing more. YAGNI.

This is now done.

> How about as an enum, or an integer, or a string? After all, the only thing
> being done here is picking what to do with a URL and a title, then
> immediately doing that thing. There's very little need, IMO, to abstract
> this.

The only slight subtlety arises when the results of the ShareMethod initialisation need to be reflected back into the UI, as for Sync. The removal of the crazy lifecycle management scheme seems to make this considerably less vomit-inducing.

(In reply to Nick Alexander :nalexander from comment #30)
> This is all seems fancy, probably unecessary, and rather brittle.  It ties
> the creation of intents to the creation of handlers via the singletons, and
> makes registering new share handlers a static/code thing rather than a
> dynamic registry thing.  All ShareMethods are going to require an Android
> context; it's weird to have a static singleton encapsulate a context.  (I
> may be wrong on how the singletons are initialized; will keep reading.)

While we're not going the way of a dynamic registry just yet, you're right that it's probably deeply unwise to initialise singletons in Enum preparation code. Not my brightest moment. :P


> @@ +82,5 @@
> > +
> > +    @Override
> > +    public int onStartCommand(Intent intent, int flags, int startId) {
> > +        // Dispatch intent to appropriate method according to its action.
> > +        String action = intent.getAction();
> 
> intent and action can be null.

So Richard mentioned this before, citing Bug 1025937.

Bug 1025937 applied to an IntentService. IntentServices can produce null intents, as documented:
http://developer.android.com/reference/android/app/IntentService.html#onStartCommand(android.content.Intent, int, int)

This stems from the general behaviour of services when onStartCommand returns the START_STICKY flag: It causes Android to reload your service as soon as it can after it has to kill your service. When it does so, you're started with a null intent and expected to figure out what's going in.
My service returns START_NOT_STICKY (do not resuscitate), so this situation cannot occur.

A null action seems like it can only happen if that's what a caller puts in the intent. That's the result of programmer error.

I've added assertions for both, but don't think there's much value in gracefully handling either case at runtime: they are insanity conditions that should never occur, and that we cannot usefully recover from. If we respond by just silently dying in these cases, future work attempting to call the service may encounter a difficult to debug problem when their accidentially-null action is ignored...
Attachment #8478205 - Attachment is obsolete: true
Attachment #8480297 - Flags: review?(rnewman)
Attachment #8480297 - Flags: feedback?(nalexander)
I'll get to this tonight; got pulled away by a stream of questions for most of the day.
Comment on attachment 8480297 [details] [diff] [review]
Part 3: A service for handling share intents. (V3)

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

Make sure SendTab.java reflects the changes in Bug 993885.

r+ with comments addressed. Feel free to request re-review if they're substantial.

::: mobile/android/base/overlays/OverlayConstants.java
@@ +42,5 @@
> +     */
> +
> +    // The URL/title of the page being shared
> +    public static final String URL = "URL";
> +    public static final String TITLE = "TITLE";

Name the constants EXTRA_URL, EXTRA_TITLE. The value doesn't have to change.

@@ +45,5 @@
> +    public static final String URL = "URL";
> +    public static final String TITLE = "TITLE";
> +
> +    // The optional extra Parcelable parameters for a ShareMethod.
> +    public static final String EXTRA = "EXTRA";

EXTRA_PARAMETERS.

@@ +48,5 @@
> +    // The optional extra Parcelable parameters for a ShareMethod.
> +    public static final String EXTRA = "EXTRA";
> +
> +    // The extra field key used for holding one or more share method names (See above).
> +    public static final String SHARE_METHOD = "SHARE_METHOD";

EXTRA_SHARE_METHOD.

@@ +63,5 @@
> +     *
> +     * $SHARE_METHOD: The ShareType to which this event relates.
> +     * ... ShareType-specific parameters as desired... (optional)
> +     */
> +    public static final String SHARE_METHOD_UI_EVENT = "org.mozilla.gecko.overlays.SHARE_METHOD_UI_EVENT";

ACTION_SH…

::: mobile/android/base/overlays/service/OverlayIntentService.java
@@ +34,5 @@
> + *
> + * Add bookmark*
> + * Add to reading list*
> + * Send tab (delegates to Sync's existing handler)
> + * TODO: Load page in background.

s/TODO/Future:

@@ +40,5 @@
> + * * Neither of these incur a page fetch on the service... yet. That will require headless Gecko,
> + *   something we're yet to have. Refactoring Gecko as a service itself and restructing the rest of
> + *   the app to talk to it seems like the way to go there.
> + */
> +public class OverlayIntentService extends Service {

Don't call it an IntentService unless it's an IntentService.

@@ +44,5 @@
> +public class OverlayIntentService extends Service {
> +    private static final String LOGTAG = "GeckoOverlayService";
> +
> +    // Map used for selecting the appropriate helper object when handling a share.
> +    private EnumMap<ShareMethod.Type, ShareMethod> shareTypes = new EnumMap<>(ShareMethod.Type.class);

final.

@@ +100,5 @@
> +
> +                // Fish the parameters out of the Intent.
> +                String url = extras.getString(OverlayConstants.URL);
> +                String title = extras.getString(OverlayConstants.TITLE);
> +                Parcelable extra = extras.getParcelable(OverlayConstants.EXTRA);

If you're not going to assign to them (deliberately), prefer `final`.

@@ +104,5 @@
> +                Parcelable extra = extras.getParcelable(OverlayConstants.EXTRA);
> +
> +                Assert.isTrue(url != null);
> +
> +                ShareMethod.Type shareMethodType = (ShareMethod.Type) extras.get(SHARE_METHOD);

I'd probably do this first, and die if it's missing, rather than the Assert.isTrue(false) below.

@@ +107,5 @@
> +
> +                ShareMethod.Type shareMethodType = (ShareMethod.Type) extras.get(SHARE_METHOD);
> +                ShareMethod shareMethod = shareTypes.get(shareMethodType);
> +
> +                // Dispatch the share to the targeted ShareMethod;

s/;/.

::: mobile/android/base/overlays/service/sharemethods/AddBookmark.java
@@ +1,1 @@
> +package org.mozilla.gecko.overlays.service.sharemethods;

License.

@@ +12,5 @@
> + * Acquires a LocalBrowserDB instance in init and performs a simple insert to the bookmarks table in
> + * handle.
> + */
> +public class AddBookmark extends ShareMethod {
> +    private static final String LOGTAG = "AddBookmark";

Gecko

@@ +21,5 @@
> +
> +        // Acquire LocalBrowserDB instance for the default profile.
> +        LocalBrowserDB browserDB = new LocalBrowserDB(GeckoProfile.DEFAULT_PROFILE);
> +
> +        // Write to bookmarks table.

These two comments are redundant.

@@ +28,5 @@
> +        return Result.SUCCESS;
> +    }
> +
> +    public String getSuccessMesssage() {
> +        return context.getResources().getString(R.string.bookmark_added);

The strings loaded by these methods will be wrong unless the locale has been corrected in the activity. And they might *still* be wrong. Verify: switch locales inside the app in a multi-locale build, kill the app, and then run through the sharing flow. The UI for the flow and the confirmation messages should be in your selected locale, not the system locale.

::: mobile/android/base/overlays/service/sharemethods/AddToReadingList.java
@@ +1,1 @@
> +package org.mozilla.gecko.overlays.service.sharemethods;

License.

@@ +5,5 @@
> +import android.os.Parcelable;
> +import org.mozilla.gecko.GeckoProfile;
> +import org.mozilla.gecko.db.LocalBrowserDB;
> +
> +import static org.mozilla.gecko.db.BrowserContract.*;

Is that really necessary?

@@ +14,5 @@
> + * Inserts the given url/title pair into the reading list database.
> + * TODO: In the event the page turns out not to be reader-mode-compatible,
> + */
> +public class AddToReadingList extends ShareMethod {
> +    private static final String LOGTAG = "AddToReadingList";

Gecko

@@ +20,5 @@
> +    @Override
> +    public Result handle(String title, String url, Parcelable unused) {
> +        ContentResolver resolver = context.getContentResolver();
> +
> +        // Acquire LocalBrowserDB instance for the default profile.

add 1 to x

::: mobile/android/base/overlays/service/sharemethods/ParcelableClientRecord.java
@@ +1,1 @@
> +package org.mozilla.gecko.overlays.service.sharemethods;

License.

@@ +4,5 @@
> +import android.os.Parcelable;
> +import org.mozilla.gecko.sync.repositories.domain.ClientRecord;
> +
> +/**
> + * A representation of a Sync ClientRecord suitable for Parceling, storing only name, guid, type.

An immutable

@@ +16,5 @@
> +    public final String name;
> +    public final String type;
> +    public final String guid;
> +
> +    public ParcelableClientRecord(String aName, String aType, String aGUID) {

Consider making this private.

@@ +26,5 @@
> +    /**
> +     * Create a ParcelableClientRecord from a vanilla ClientRecord.
> +     */
> +    public static ParcelableClientRecord fromClientRecord(ClientRecord record) {
> +        return new ParcelableClientRecord(record.name, record.type, record.guid);

Validate inputs at some point -- either here or in the constructor.

@@ +48,5 @@
> +            String name = source.readString();
> +            String type = source.readString();
> +            String guid = source.readString();
> +
> +            return new ParcelableClientRecord(name, type, guid);

Similarly.

::: mobile/android/base/overlays/service/sharemethods/SendTab.java
@@ +1,1 @@
> +package org.mozilla.gecko.overlays.service.sharemethods;

License.

@@ +9,5 @@
> +import android.os.Bundle;
> +import android.os.Parcelable;
> +import android.support.v4.content.LocalBroadcastManager;
> +import org.mozilla.gecko.R;
> +import org.mozilla.gecko.background.common.log.Logger;

If you're going to use Logger via copypasta from SendTabActivity, make sure you init it.

@@ +49,5 @@
> +    // Key used in the extras Bundle in the share intent used for a send tab ShareMethod.
> +    public static final String SEND_TAB_TARGET_DEVICES = "SEND_TAB_TARGET_DEVICES";
> +
> +    // Key used in broadcast intent from SendTab ShareMethod specifying available ClientRecords.
> +    public static final String CLIENT_RECORDS_BUNDLE = "RECORDS";

Similar point to earlier.

@@ +74,5 @@
> +
> +        // Ensure all target GUIDs are devices we actually know about...
> +        if (!validGUIDs.containsAll(Arrays.asList(targetGUIDs))) {
> +            Logger.error(LOGTAG, "Not all provided GUIDs are real devices...");
> +            return Result.PERMANENT_FAILURE;

This seems like an unfriendly approach. Why not intersect the two?

@@ +95,5 @@
> +
> +        // Queue up the share commands for each destination device.
> +        // Remember that ShareMethod.handle is always run on the background thread, so the database
> +        // access here is of no concern.
> +        for (int i = 0; i < targetGUIDs.length; i++) {

for (String targetGUID : targetGUIDs) {
  …

@@ +109,5 @@
> +    }
> +
> +
> +    /**
> +     * Get an Intent suitable for broadacasting the UI state of this ShareMethod.

broadcasting

@@ +112,5 @@
> +    /**
> +     * Get an Intent suitable for broadacasting the UI state of this ShareMethod.
> +     * The caller shall populate the intent with the actual state.
> +     */
> +    private Intent getUiStateIntent() {

UI

@@ +145,5 @@
> +                // right now. Redirect to the status activity.
> +                Logger.warn(LOGTAG, "Firefox Account named like " + fxAccount.getObfuscatedEmail() +
> +                                    " needs action before it can send a tab; redirecting to status activity.");
> +
> +                setOverrideIntent(FxAccountStatusActivity.class);

Doing this -- or, indeed, setting up Sync -- will lose this sent URL. Please file a follow-up to rectify that situation.

::: mobile/android/base/overlays/service/sharemethods/ShareMethod.java
@@ +1,1 @@
> +package org.mozilla.gecko.overlays.service.sharemethods;

License.

@@ +7,5 @@
> +/**
> + * Represents a method of sharing a url/title. Add a bookmark? Send to a device? Add to reading list?
> + *
> + * A ShareMethod consists of a handle() method, to perform the actual sharing operation, as well as
> + * an init() method to perform any preparatory work that must occur before handle() can function

Correct this comment.
Attachment #8480297 - Flags: review?(rnewman) → review+
> for (String targetGUID : targetGUIDs) {

Also, yes, I know, the i = approach is probably quicker. If you're really motivated, do that instead.
(Assignee)

Updated

5 years ago
See Also: → bug 1060882
(Assignee)

Updated

5 years ago
See Also: → bug 1060664
(Assignee)

Comment 45

5 years ago
Huh. Seems like my IDE is misconfigured >.>

(In reply to Richard Newman [:rnewman] from comment #43)
> Make sure SendTab.java reflects the changes in Bug 993885.

I should perhaps have mentioned earlier that I've already done so. Most of the work there isn't useful here, as we don't have activity lifecycle issues running around causing race conditions in quite the same way as you did for SendTabActivity. (Also, I was reproducing that bug with absurd levels of regularity when trying to test: I found it when trying to re-file it).

> @@ +104,5 @@
> > +                Parcelable extra = extras.getParcelable(OverlayConstants.EXTRA);
> > +
> > +                Assert.isTrue(url != null);
> > +
> > +                ShareMethod.Type shareMethodType = (ShareMethod.Type) extras.get(SHARE_METHOD);
> 
> I'd probably do this first, and die if it's missing, rather than the
> Assert.isTrue(false) below.

There's two possible failures here: the Type was null or a programmer added a new enum value and didn't handle it properly. Both are possible future bugs, and I wanted to use the minimum possible code to detect stupidity by future programmers. Your suggestion would not handle the "new enum constant" situation.
Admittedly, Assert.isTrue(false) is unsightly. It would be nice if the Assert class had some more fluent API. Bug 1060664 happened.

> @@ +28,5 @@
> > +        return Result.SUCCESS;
> > +    }
> > +
> > +    public String getSuccessMesssage() {
> > +        return context.getResources().getString(R.string.bookmark_added);
> 
> The strings loaded by these methods will be wrong unless the locale has been
> corrected in the activity. And they might *still* be wrong. Verify: switch
> locales inside the app in a multi-locale build, kill the app, and then run
> through the sharing flow. The UI for the flow and the confirmation messages
> should be in your selected locale, not the system locale.

Certainly, the activity needs to set up the locale as you mentioned over in the other bug.

I think the answer here is "The activity needs to inherit from LocaleAwareActivity so it does the right magic that'll most likely mean the application context acquired by the service has the right locale, so we're fine. Probably.". If this turns out not to be true, let's follow-up-ify it. It's'a shipping time.

> ::: mobile/android/base/overlays/service/sharemethods/AddToReadingList.java
> @@ +5,5 @@
> > +import android.os.Parcelable;
> > +import org.mozilla.gecko.GeckoProfile;
> > +import org.mozilla.gecko.db.LocalBrowserDB;
> > +
> > +import static org.mozilla.gecko.db.BrowserContract.*;
> 
> Is that really necessary?

Seems IDEA needs its "Number of imported classes until we give up and use an asterisk" parameter tweaked.

> @@ +16,5 @@
> > +    public final String name;
> > +    public final String type;
> > +    public final String guid;
> > +
> > +    public ParcelableClientRecord(String aName, String aType, String aGUID) {
> 
> Consider making this private.

That'd prevent us from sending ClientRecords the other way. At the moment, we only send them from the service outwards, but should we ever want to send them inward...
That said, I don't think the current frontend patch wants that either, but let's not arbitrarily delete API features we're not using when there's no benefit.

> @@ +145,5 @@
> > +                // right now. Redirect to the status activity.
> > +                Logger.warn(LOGTAG, "Firefox Account named like " + fxAccount.getObfuscatedEmail() +
> > +                                    " needs action before it can send a tab; redirecting to status activity.");
> > +
> > +                setOverrideIntent(FxAccountStatusActivity.class);
> 
> Doing this -- or, indeed, setting up Sync -- will lose this sent URL. Please
> file a follow-up to rectify that situation.

Do we actually care all that much?
I guess that'll boil down to stowing the URL somewhere handy and retrying the share when Sync's all set up.

(In reply to Richard Newman [:rnewman] from comment #44)
> > for (String targetGUID : targetGUIDs) {
> 
> Also, yes, I know, the i = approach is probably quicker. If you're really
> motivated, do that instead.

Congratulations. You've found an instance in which the java compiler *isn't* painfully stupid.

For-each is desugared into either a while loop with an iterator or for-i loop if the argument is an array (naturally, it'd be sort of stupid to use an iterator of an array. Such overhead. Wow.)
I'm so used to Java doing the dumb thing that I'd assumed that's what it was doing. Nope. It does the sane thing. It's like Christmas. You can have your enhanced for loop: it's all the same in bytecode (well, actually, not quite: javac desugars it slightly stupidly and puts in an extra copy for the counter at the start, but Proguard will fix that).
(Assignee)

Comment 46

5 years ago
Implemented review comments, slightly tweaked the way the frontend/backend patches fit together, STOPPED TRANSPOSING URL AND TITLE.

Doesn't warrant a new review, really, but you're welcome to have another look before I land it tomorrow evening. (and it may prove helpful to be able to see the exact version of this patch the new frontend was written to interact with, even if the differences are minor).
Attachment #8480297 - Attachment is obsolete: true
Attachment #8480297 - Flags: feedback?(nalexander)
Attachment #8481915 - Flags: feedback?(rnewman)
Comment on attachment 8481915 [details] [diff] [review]
Part 3: A service for handling share intents. (V4)

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

::: mobile/android/base/overlays/service/OverlayActionService.java
@@ +62,5 @@
> +    }
> +
> +    @Override
> +    public int onStartCommand(Intent intent, int flags, int startId) {
> +        Assert.isTrue(intent != null);

Assert is meant to be used for debug assertions. In this case, if the assertion fails and we've in a release build, we'll walk right into a background NPE. If assertions are turned on, we'll throw.

There's nothing useful we can do if the intent is null, and it shouldn't happen, but there's definitely no point in crashing. So detect it here and return, rather than either doing nothing or throwing an uncaught exception.

@@ +104,5 @@
> +                final String url = extras.getString(OverlayConstants.EXTRA_URL);
> +                final String title = extras.getString(OverlayConstants.EXTRA_TITLE);
> +                final Parcelable extra = extras.getParcelable(OverlayConstants.EXTRA_PARAMETERS);
> +
> +                Assert.isTrue(url != null);

Remember, with assertions turned off this will do nothing, and we'll call `shareMethod.handle()` with a null URL.

@@ +107,5 @@
> +
> +                Assert.isTrue(url != null);
> +
> +                ShareMethod.Type shareMethodType = (ShareMethod.Type) extras.get(EXTRA_SHARE_METHOD);
> +                ShareMethod shareMethod = shareTypes.get(shareMethodType);

Be ready for ClassCastException in the Play console!

@@ +132,5 @@
> +                        // Show a failure toast without a retry button.
> +                        OverlayToastHelper.showFailureToast(getApplicationContext(), shareMethod.getFailureMessage(), false);
> +                        break;
> +                    default:
> +                        Assert.isTrue(false);

Again, I question the value of ever throwing here. A descriptive log message (Log.e(LOGTAG, "Unknown share method: " + sha

::: mobile/android/base/overlays/service/sharemethods/AddBookmark.java
@@ +15,5 @@
> +/**
> + * ShareMethod to add a bookmark.
> + *
> + * Acquires a LocalBrowserDB instance in init and performs a simple insert to the bookmarks table in
> + * handle.

Correct this comment.

::: mobile/android/base/overlays/service/sharemethods/AddToReadingList.java
@@ +17,5 @@
> +
> +/**
> + * ShareMethod to add a page to the reading list.
> + *
> + * Inserts the given url/title pair into the reading list database.

URL/title.

@@ +18,5 @@
> +/**
> + * ShareMethod to add a page to the reading list.
> + *
> + * Inserts the given url/title pair into the reading list database.
> + * TODO: In the event the page turns out not to be reader-mode-compatible,

Fix this comment.

::: mobile/android/base/overlays/service/sharemethods/ParcelableClientRecord.java
@@ +22,5 @@
> +    public final String name;
> +    public final String type;
> +    public final String guid;
> +
> +    public ParcelableClientRecord(String aName, String aType, String aGUID) {

> > Consider making this private.

> That'd prevent us from sending ClientRecords the other way.

(a) you'd still have the static that consumes ClientRecords, and (b) if that's not enough, you'd probably want to rework this class anyway. YAGNI.

@@ +23,5 @@
> +    public final String type;
> +    public final String guid;
> +
> +    public ParcelableClientRecord(String aName, String aType, String aGUID) {
> +        Assert.isTrue(aName != null);

You've gone way overboard on the asserts. There's nothing useful we can do with a null name, so we should always throw.
Attachment #8481915 - Flags: feedback?(rnewman) → review+
(Assignee)

Comment 48

5 years ago
(In reply to Richard Newman [:rnewman] from comment #47)
> ::: mobile/android/base/overlays/service/OverlayActionService.java
> @@ +62,5 @@
> > +    }
> > +
> > +    @Override
> > +    public int onStartCommand(Intent intent, int flags, int startId) {
> > +        Assert.isTrue(intent != null);
> 
> Assert is meant to be used for debug assertions. In this case, if the
> assertion fails and we've in a release build, we'll walk right into a
> background NPE. If assertions are turned on, we'll throw.
> 
> There's nothing useful we can do if the intent is null, and it shouldn't
> happen, but there's definitely no point in crashing. So detect it here and
> return, rather than either doing nothing or throwing an uncaught exception.

Generally, I don't approve of "defensive programming" like this, though since this is an entry point from an API with "character" it's maybe not such a terrible idea.

On the other hand, as I explained earlier, I do not believe your earlier example of "intents can sometimes randomly be null" (Bug 1025937) really shows that we need to check for null intents in all our services. That bug seemed to be falling foul of the documented behaviour of services when START_STICKY is returned from onStartCommand (which is also the behaviour you get when you're an IntentService, as in the other bug), as explained here:
http://stackoverflow.com/questions/8421430/reasons-that-the-passed-intent-would-be-null-in-onstartcommand

Since I'm a vanilla service returning START_NOT_STICKY, the only way we can ever have a null intent is if we explicitly pass one (GIGO) or if there's an Android bug which gives us one unexpectedly (are we really going to start speculatively working around Android bugs we don't have evidence for the existence of? Where do you draw the line with implementing this sort of "should be redundant according to the API specification" check? There seems to be no more compelling reason to check for null intents here than to perform comparable checks on absolutely every input we get from Android land.).

> @@ +104,5 @@
> > +                final String url = extras.getString(OverlayConstants.EXTRA_URL);
> > +                final String title = extras.getString(OverlayConstants.EXTRA_TITLE);
> > +                final Parcelable extra = extras.getParcelable(OverlayConstants.EXTRA_PARAMETERS);
> > +
> > +                Assert.isTrue(url != null);
> 
> Remember, with assertions turned off this will do nothing, and we'll call
> `shareMethod.handle()` with a null URL.

If that happens, we've got a bug: some future programmer made a mistake (or I missed something!). In such cases we want to know about the earliest time things deviate from our intended view of the world. Hiding our precondition violation behind defensive null checks just complicates debugging and clutters our code.
We should not be afraid to have preconditions for our methods. Everything we ever write has a set of assumptions (implicit or otherwise) under which it executes. Aggressively checking at runtime for each of these to hold is folly (and error prone, confusing, slow, and may lead to future developers mutating the definition of our method in ways that subtly break things because they made incorrect inferences about the nature of our implicit assumptions).

Remember that these assertions are enabled whenever Proguard is not. That translates to "prettymuch every developer build".

Since this API does not support null urls and is not intended for public use, we don't need to program defensively to gracefully handle crazy input: we need to not give ourselves crazy input.

The point of assertions is to make explicit (and check) at debug-time that the assumptions you are making hold, be they preconditions or postconditions (did we *really* do what we're trying to?). You appear to be asking me to write code that hides precondition violations, something that makes detection of bugs more challenging. I get that we don't want release builds to crash, but we achieve that by writing code that does not violate our internal assumptions, not by aggressively double-checking ourselves everywhere. All that does is confuses us (and wastes a teensy bit of CPU time).
Lastly, in this particular case, the cost of crashing is negligible to the user. If the service crashes it'll come right back to life the next time the user shares something. Maybe that time they won't give us a null...

> @@ +107,5 @@
> > +
> > +                Assert.isTrue(url != null);
> > +
> > +                ShareMethod.Type shareMethodType = (ShareMethod.Type) extras.get(EXTRA_SHARE_METHOD);
> > +                ShareMethod shareMethod = shareTypes.get(shareMethodType);
> 
> Be ready for ClassCastException in the Play console!

I'm not sure what you want me to do. We can't have strongly-typed objects in the Bundle, can we? I could probably extend Intent to give us a putShareMethodTypeExtra providing such typing, but doing so doesn't seem worthwhile.
This object has to be of that type. It doesn't seem that the type system lets us do so, but at least we fail fast if it's not so.
If it's not, it's a client bug. I could assert this, but you seem to not like it when I do that...

> @@ +132,5 @@
> > +                        // Show a failure toast without a retry button.
> > +                        OverlayToastHelper.showFailureToast(getApplicationContext(), shareMethod.getFailureMessage(), false);
> > +                        break;
> > +                    default:
> > +                        Assert.isTrue(false);
> 
> Again, I question the value of ever throwing here. A descriptive log message
> (Log.e(LOGTAG, "Unknown share method: " + sha

Certainly, a helpful message should be added. Something like "Assert.fail(String message)" would make this prettier.

My reason for preferring asserts in this sort of situation is that this is a "WTF" condition. According to our understanding of the program, this condition can never occur, so it's redundant to store this message and whatnot.
On the other hand, the optimiser might be deleting this branch anyway, but I'm not seeing why a log statement is any more valuable here than an assert (it seems to be slightly less useful for debugging because it allows us to continue past an assumption violation, and slightly less good performance-wise). Asserts are on whenever Proguard isn't, which seems to be the case for the builds developers who like their sanity. Release builds never care about this condition, because you're only going to get here if you're a developer that did something a bit daft.

Since the Assert class is apparently not being used by anybody else, I suggest you either come up with a coherent idea of when it should be used, or file for its deletion and start consistently doing defensive programming (please don't do that).

> mobile/android/base/overlays/service/sharemethods/ParcelableClientRecord.java
> @@ +22,5 @@
> > +    public final String name;
> > +    public final String type;
> > +    public final String guid;
> > +
> > +    public ParcelableClientRecord(String aName, String aType, String aGUID) {
> 
> > > Consider making this private.
>
> > That'd prevent us from sending ClientRecords the other way.
> 
> (a) you'd still have the static that consumes ClientRecords, and (b) if
> that's not enough, you'd probably want to rework this class anyway. YAGNI.

You're basically right, but I've got an evil hack in the ArrayAdapter for dealing with "button-like" states that uses dummy ParcelableClientRecords.
That's sort of dumb: I think what I really want, then, is to separate ParcelableClientRecords (things that represent a real ClientRecord somewhere that are Parcelable) from "objects we show in the ListView". That way, we can stop being dumb here.

I'll land something that sucks less...

> @@ +23,5 @@
> > +    public final String type;
> > +    public final String guid;
> > +
> > +    public ParcelableClientRecord(String aName, String aType, String aGUID) {
> > +        Assert.isTrue(aName != null);
> 
> You've gone way overboard on the asserts. There's nothing useful we can do
> with a null name, so we should always throw.

It does seem a little pointless to put assertions in when the very next thing will NPE out if the assertion doesn't hold, but it does have some documentation value: it makes explicit the assumptions we're making (instead of requiring a reader to work out for themselves that Bad Things will happen if this is null).
Also, when faced with such an NPE a future programmer may be tempted to "solve" the GIGO problem they've encountered by defensively null-checking here instead of fixing the broken consumer class. The presence of an assertion may discourage such behaviour. Redundantly checking your assumptions all over the place is pretty daft.



Since we urgently need to land, I'm going to apply all your contested review comments as you gave them and land this immediately. I really think we need to start using assertions properly (like, at all), and stop our habit of hiding brokenness with defensive programming. It's not just beneficial (and makes our binary and source larger than they need to be).
(Assignee)

Comment 49

5 years ago
(In reply to Richard Newman [:rnewman] from comment #47)
> mobile/android/base/overlays/service/sharemethods/ParcelableClientRecord.java
> @@ +22,5 @@
> > +    public final String name;
> > +    public final String type;
> > +    public final String guid;
> > +
> > +    public ParcelableClientRecord(String aName, String aType, String aGUID) {
> 
> > > Consider making this private.
> 
> > That'd prevent us from sending ClientRecords the other way.
> 
> (a) you'd still have the static that consumes ClientRecords, and (b) if
> that's not enough, you'd probably want to rework this class anyway. YAGNI.

Suitably refactoring the ArrayAdapter in the frontend made this madness go away. Hooray.
(Assignee)

Comment 50

5 years ago
Comments addressed and suchlike. Should be ready to land... But the tree is closed.

I'm also not likely to be anywhere near a computer for most of tomorrow, so you may have the land this in my absence if you want it in 34. The patches in this bug are the latest versions now, so this should be a simple (if slightly tedious) task.
Attachment #8481915 - Attachment is obsolete: true
Attachment #8482224 - Flags: review?(rnewman)
(Assignee)

Comment 51

5 years ago
Refactoring the ToastHelper class in the frontend mandated a tweak in here.
Attachment #8482224 - Attachment is obsolete: true
Attachment #8482224 - Flags: review?(rnewman)
Attachment #8482233 - Flags: review?(rnewman)
Comment on attachment 8482233 [details] [diff] [review]
Part 3: A service for handling share intents. (V6)

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

::: mobile/android/base/overlays/service/sharemethods/AddBookmark.java
@@ +13,5 @@
> +import org.mozilla.gecko.db.LocalBrowserDB;
> +
> +/**
> + * ShareMethod to add a bookmark.
> + */

Remove redundant comment.

::: mobile/android/base/overlays/service/sharemethods/ShareMethod.java
@@ +1,5 @@
> +/*
> + * This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/.
> + */

Newline after comment. (Every file.)

@@ +9,5 @@
> +import android.os.Parcel;
> +import android.os.Parcelable;
> +
> +/**
> + * Represents a method of sharing a url/title. Add a bookmark? Send to a device? Add to reading list?

s/url/URL.

@@ +19,5 @@
> +        context = aContext;
> +    }
> +
> +    /**
> +     * Perform a share for the given title/url combination. Called on the background thread by the

s/url/URL.

@@ +89,5 @@
> +                return new Type[size];
> +            }
> +        };
> +    }
> +

Kill newline.

::: mobile/android/base/overlays/ui/OverlayToastHelper.java
@@ +1,5 @@
> +/*
> + * This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/.
> + */

Newline after comment.
Attachment #8482233 - Flags: review?(rnewman) → review+
(In reply to Chris Kitching [:ckitching] from comment #48)

> On the other hand, as I explained earlier, I do not believe your earlier
> example of "intents can sometimes randomly be null" (Bug 1025937) really
> shows that we need to check for null intents in all our services.

I agree; I'm not asserting that we would routinely receive a null here, as we do in that bug. But two points:

* Things change. Some part of software engineering is about guarding against future bugs that are likely to be introduced by well-meaning but uninformed engineers. If this code morphs into, say, an intent service, this free check will quietly avoid a crash -- not even hiding a bug, just automatically doing the right thing.
* GIGO is a real problem.


> There seems
> to be no more compelling reason to check for null intents here than to
> perform comparable checks on absolutely every input we get from Android
> land.).

Prior experience suggests that we ought to perform comparable checks on absolutely every input we get from Android :)

A good rule of thumb for large-scale software engineering: enforce contracts at module boundaries. Especially when a failure to do so causes a user-visible crash, and the check is effectively free.



> You appear to be asking me to
> write code that hides precondition violations, something that makes
> detection of bugs more challenging. 

No. What I'm suggesting is that we should not have assertions that result in a crash in developer builds, removed in release builds, and then for users to get a NPE due to an unenforced contract. By all means make developer builds crash and release builds not crash!

Remember that, realistically, most code lands without adequate tests (including this one), and most code does not have well-exercised error flows (because most of the error flows are untestable).

You're assuming that an assertion here, experienced by developers, is sufficient to ensure that no user will ever get bad input. If you're wrong, we'll get user-visible crashes. I don't see that as a reasonable tradeoff. Services should always check their inputs.


> Lastly, in this particular case, the cost of crashing is negligible to the
> user. If the service crashes it'll come right back to life the next time the
> user shares something. Maybe that time they won't give us a null...

Experience with service crashes suggests that users see a "Firefox has stopped" box over their frontmost activity.


> Remember that these assertions are enabled whenever Proguard is not.
> That translates to "prettymuch every developer build".

I thought that Proguard was enabled by default? It certainly is for me. Is it on for Try builds? Nightly? Don't rely on particular configurations to catch bugs.


> I really think we need
> to start using assertions properly (like, at all), and stop our habit of
> hiding brokenness with defensive programming. It's not just beneficial (and
> makes our binary and source larger than they need to be).

I agree with using assertions within module boundaries to document contracts. I want rigorous, unconditional, explicit throw-or-muffle semantics at module boundaries or critical junctures (e.g., concurrency).

See e.g.,         ThreadUtils.assertOnThread(getUiThread(), AssertBehavior.THROW);
Comment on attachment 8467513 [details] [diff] [review]
Part 0: Delete a semantically null static initialiser from BrowserDB

This patch no longer needed.
Attachment #8467513 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/ebd17f3519e3
https://hg.mozilla.org/mozilla-central/rev/e3cf9620539c
https://hg.mozilla.org/mozilla-central/rev/2aec90e5aed2
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
You need to log in before you can comment on or make changes to this bug.