So that users can easily add items to their Firefox reading list from other apps. Thoughts?

+1 In the Twitter app, you can share a tweet to Pocket, and it finds the URL in the tweet and adds that to your Pocket list. It could be cool to do something like that as well (probably in a separate bug).

+1 I agree with Margaret that that would be ideal. I also feel that the reading list should be added to desktop Firefox.

Let's remember to get a security review for this too. I am guessing about what flags to set...

Please only accept "web" schemes, http/https and maybe ftp if you really need it. Not javascript, file, resource, chrome, or other unknown schemes that might be supported by an addon later.

Here's a patch for this that sort-of works - it has the annoying property that when invoked the browser appears, loads the page, then goes away again having added the page to the reading list (Unless the browser was already running) This seems at least in part pointful - the page would need to be loaded at least to some degree (title+favicon) in order to collect all the needed info to make the entry to go on the reading list (Indeed, Fennec goes away as soon as the js reports to the Java that this has all been done and the entry added to the list). It would be optimal, in my opinion (And presumably the intention..?) if Fennec would handle this intent silently in the background, and just pop up a Toast notification letting the user know the result of this intent (While the app dispatching the intent retains focus). My attempts to implement that in the case where Fennec is inactive when the Intent is received have been unsuccessful - when I invoke moveTaskToBack(true); in onCreate of BrowserApp Fennec crashes during startup with the following stack trace: 07-05 02:28:41.041: ERROR/GeckoGLController(26388): Unable to create window surface java.lang.IllegalArgumentException: Make sure the SurfaceView or associated SurfaceHolder has a valid Surface at com.google.android.gles_jni.EGLImpl._eglCreateWindowSurface(Native Method) at com.google.android.gles_jni.EGLImpl.eglCreateWindowSurface(EGLImpl.java:92) at org.mozilla.gecko.gfx.GLController$1.run(GLController.java:145) at android.os.Handler.handleCallback(Handler.java:725) at android.os.Handler.dispatchMessage(Handler.java:92) at android.os.Looper.loop(Looper.java:137) at android.app.ActivityThread.main(ActivityThread.java:5041) at java.lang.reflect.Method.invokeNative(Native Method) at java.lang.reflect.Method.invoke(Method.java:511) at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:793) at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:560) at dalvik.system.NativeStart.main(Native Method) My attempts to debug this problem all ended up extremely kludge-esque - I would appreciate input from someone familiar with this part of the code on how to proceed. (Or should I just be figuring this out for myself? Not entirely sure what the protocol is here..)

Comment on attachment 771588 [details] [diff] [review] An imperfect first attempt at this.. Review of attachment 771588 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/AndroidManifest.xml.in @@ +89,5 @@ > <intent-filter> > <action android:name="org.mozilla.gecko.ACTION_ALERT_CALLBACK" /> > </intent-filter> > + <intent-filter> > + <action android:name="org.mozilla.gecko.ADD_TO_READING_LIST" /> I think what we want here is for "Firefox Reading List" or something like that to appear in the list of applications that get listed when another app tries to launch a share intent. Poking around on the internet, I think we need to register to handle android.intent.action.SEND. In fact, we actually already do this for sync: http://mxr.mozilla.org/mozilla-central/source/mobile/android/services/manifests/SyncAndroidManifest_activities.xml.in#89 So we may want to talk to rnewman about how they came up with that implementation. We should test this by trying to share a link from Twitter or some other app like that, since I think that's the use case we're trying to address here. @@ +92,5 @@ > + <intent-filter> > + <action android:name="org.mozilla.gecko.ADD_TO_READING_LIST" /> > + <category android:name="android.intent.category.DEFAULT" /> > + <data android:scheme="http"/> > + <data android:scheme="https"/> Looking at the sync manifest, we just check the mimetype, not the scheme, which may be a security-friendly way to handle this. ::: mobile/android/base/BrowserApp.java @@ +398,5 @@ > + //moveTaskToBack(true); //We don't actually want to show them to UI. Just the toast about success. > + //Problem: Doing this causes GLController to become upset. But we don't really want to show the page load > + //in the foreground.. Hmph. > + processAddToReadingList(launchIntent); > + } I think we should come up with a solution that doesn't involve adding anything to onCreate. You're probably getting that crash because you're doing something that you shouldn't too early during startup. Ideally, we'd be able to create a different Activity in the manifest that just handles adding the page to the reading list in the background. I'm worried this may be hard since the reading list logic is currently in JS. However, things like the preferences are currently implemented as separate activities, so maybe it isn't so hard to communicate to gecko JS from a separate activity. In any case, I think refactoring some of the reading list code to handle this use case would be preferable to this currently implementation. @@ +1119,5 @@ > final String url = message.getString("url"); > handleReaderAdded(result, title, url); > + try { > + if(message.getBoolean("term")) { > + moveTaskToBack(true); //We're only here to add something to a reading list... I don't like this, we need to find a real solution that doesn't require this.

Cc'ing rnewman in case he has any advice about the share intent handling.

> ::: mobile/android/base/AndroidManifest.xml.in > @@ +89,5 @@ > > <intent-filter> > > <action android:name="org.mozilla.gecko.ACTION_ALERT_CALLBACK" /> > > </intent-filter> > > + <intent-filter> > > + <action android:name="org.mozilla.gecko.ADD_TO_READING_LIST" /> > > I think what we want here is for "Firefox Reading List" or something like > that to appear in the list of applications that get listed when another app > tries to launch a share intent. Poking around on the internet, I think we > need to register to handle android.intent.action.SEND. In fact, we actually > already do this for sync: > http://mxr.mozilla.org/mozilla-central/source/mobile/android/services/ > manifests/SyncAndroidManifest_activities.xml.in#89 > > So we may want to talk to rnewman about how they came up with that > implementation. > > We should test this by trying to share a link from Twitter or some other app > like that, since I think that's the use case we're trying to address here. Ah. No problem. The way the bug is called "Add an android intent" had me thinking I'd be adding a new intent, instead of handling one of the system ones. Makes somewhat more sense this way. > @@ +92,5 @@ > > + <intent-filter> > > + <action android:name="org.mozilla.gecko.ADD_TO_READING_LIST" /> > > + <category android:name="android.intent.category.DEFAULT" /> > > + <data android:scheme="http"/> > > + <data android:scheme="https"/> > > Looking at the sync manifest, we just check the mimetype, not the scheme, > which may be a security-friendly way to handle this. Okay, but we should check the scheme, too, as per comment 4, right? > ::: mobile/android/base/BrowserApp.java > @@ +398,5 @@ > > + //moveTaskToBack(true); //We don't actually want to show them to UI. Just the toast about success. > > + //Problem: Doing this causes GLController to become upset. But we don't really want to show the page load > > + //in the foreground.. Hmph. > > + processAddToReadingList(launchIntent); > > + } > > I think we should come up with a solution that doesn't involve adding > anything to onCreate. You're probably getting that crash because you're > doing something that you shouldn't too early during startup. > > Ideally, we'd be able to create a different Activity in the manifest that > just handles adding the page to the reading list in the background. I'm > worried this may be hard since the reading list logic is currently in JS. > However, things like the preferences are currently implemented as separate > activities, so maybe it isn't so hard to communicate to gecko JS from a > separate activity. > > In any case, I think refactoring some of the reading list code to handle > this use case would be preferable to this currently implementation. Yes, it would be ideal if we could do this from another activity but, as you say, the reading list stuff is all hidden away in JS, meaning communicating with it before doing the (Rather large task) of starting Gecko and suchlike seems rather nonsimple. I shall have to investigate how this is done elsewhere.. > @@ +1119,5 @@ > > final String url = message.getString("url"); > > handleReaderAdded(result, title, url); > > + try { > > + if(message.getBoolean("term")) { > > + moveTaskToBack(true); //We're only here to add something to a reading list... > > I don't like this, we need to find a real solution that doesn't require this. The alternative seems to be a dedicated activity for the purpose of adding to the reading list. This does not appear to be simple, since the startup process is quite complicated.

(In reply to :Margaret Leibovic from comment #6) > I think what we want here is for "Firefox Reading List" or something like > that to appear in the list of applications that get listed when another app > tries to launch a share intent. Poking around on the internet, I think we > need to register to handle android.intent.action.SEND. In fact, we actually > already do this for sync: Correct. > Ideally, we'd be able to create a different Activity in the manifest that > just handles adding the page to the reading list in the background. I'm > worried this may be hard since the reading list logic is currently in JS. > However, things like the preferences are currently implemented as separate > activities, so maybe it isn't so hard to communicate to gecko JS from a > separate activity. You'll need to have a separate activity (i.e., not BrowserApp) if you want to control its appearance and name in the Share dialog (to show something other than "Firefox"). In Sync-land we have: https://github.com/mozilla-services/android-sync/blob/develop/manifests/SyncAndroidManifest_activities.xml.in#L78 https://github.com/mozilla-services/android-sync/blob/develop/src/main/java/org/mozilla/gecko/sync/setup/activities/SendTabActivity.java Take a look at that activity as a guideline (along with some of the fun stuff you might want to handle, like rotation). Note, however, that if you're wanting to do what Pocket does -- maybe show a little toast, but basically work in the background -- then you want a Service, not an Activity. (You can do UI work from a service: <http://stackoverflow.com/questions/3955410/create-toast-from-intentservice>) My suggestion is to define a service that watches for action.SEND. The Service can buffer items which can be written into shared storage later, if you can't access Gecko storage directly from the service. You can always do a circular OrderedBroadcast later if you want to hand it off to a running Firefox.

(In reply to Chris Kitching [:ckitching] from comment #8) > Yes, it would be ideal if we could do this from another activity but, as you > say, the reading list stuff is all hidden away in JS, meaning communicating > with it before doing the (Rather large task) of starting Gecko and suchlike > seems rather nonsimple. I shall have to investigate how this is done > elsewhere.. > ... > The alternative seems to be a dedicated activity for the purpose of adding > to the reading list. This does not appear to be simple, since the startup > process is quite complicated. Just noticed that you're dwelling on this point, so let me be less brief. I recommend that the Service (or Activity!) *not* block on Gecko for this. You'll impact usability, and you don't want to spend 3 seconds of heavy IO and CPU just to save a reading list item for a user. Make this pure Java. Store the new item somewhere (SharedPreferences, for example). Then send an OrderedBroadcast with signature permissions. If Firefox is running, it can catch this and say "gotcha"; drop it from SharedPreferences. If Firefox _isn't_ running, the OB will come back around unmolested, and you know to keep the new item in SharedPreferences. Fx can check for new items on startup. This is basically a dropbox system. The alternative is polling -- whenever Fx shows the reading list UI, check the dropbox to see whether anything was queued up. These approaches will be seamless to the user, but allow you to catch the intent without tripping up on Gecko.

The Service could just save the item to the Reading List via the ContentProvider, right?

(In reply to Mark Finkle (:mfinkle) from comment #11) > The Service could just save the item to the Reading List via the > ContentProvider, right? The ContentProvider can make a reading list bookmark, but don't we do something in JS to parse/cache the article? I haven't looked into it closely, but this was my assumption.

I had assumed that we were using indexeddb for all storage. If items themselves are stored in pure Java, then hell yes, write into the CP. Margaret, can we just add a cache-populating phase after startup?

(In reply to :Margaret Leibovic from comment #12) > (In reply to Mark Finkle (:mfinkle) from comment #11) > > The Service could just save the item to the Reading List via the > > ContentProvider, right? > > The ContentProvider can make a reading list bookmark, but don't we do > something in JS to parse/cache the article? I haven't looked into it > closely, but this was my assumption. Yes, we need to parse the given page to decide whether the page is an article (i.e. reading list material) or not. We should only add URLs to our database if the page is an article.

(In reply to Lucas Rocha (:lucasr) from comment #14) > Yes, we need to parse the given page to decide whether the page is an > article (i.e. reading list material) or not. Can we do that when we try to display it, or in a pre-fetching phase to save content for offline reading? > We should only add URLs to our > database if the page is an article. Maybe now's the time to relax that restriction? That is, would users be better served by letting them put *anything* in Reading List, even if we can't turn on Reader Mode for the page? (Pocket lets you store anything, which is a feature that seems to be widely used -- they have a "top videos" chart, for example. This restriction on content is one reason why I don't use Fennec's Reader functionality at all, instead sharing to Pocket from Quickshare and switching apps.)

(In reply to Lucas Rocha (:lucasr) from comment #14) > (In reply to :Margaret Leibovic from comment #12) > > (In reply to Mark Finkle (:mfinkle) from comment #11) > > > The Service could just save the item to the Reading List via the > > > ContentProvider, right? > > > > The ContentProvider can make a reading list bookmark, but don't we do > > something in JS to parse/cache the article? I haven't looked into it > > closely, but this was my assumption. > > Yes, we need to parse the given page to decide whether the page is an > article (i.e. reading list material) or not. We should only add URLs to our > database if the page is an article. This might not be feasible. I'll be open to verifying an "article" even as late as when the user attempts to view it.

(In reply to Mark Finkle (:mfinkle) from comment #16) > This might not be feasible. I'll be open to verifying an "article" even as > late as when the user attempts to view it. Well, that would break the assumption that stuff in reading list are always available offline (which is a core part of the reading list concept in Fennec).

Why not, in the event the user adds an article that can't be reformatted, just show it as usual (Instead of the fancy reader-mode reformatted edition) We could possibly address the possibility of making more pages reformattable later, but why not allow absolutely anything on the reading list? A user could want to read later something that isn't formatted in just the right way, after all/

(In reply to Richard Newman [:rnewman] from comment #15) > (In reply to Lucas Rocha (:lucasr) from comment #14) > > > Yes, we need to parse the given page to decide whether the page is an > > article (i.e. reading list material) or not. > > Can we do that when we try to display it, or in a pre-fetching phase to save > content for offline reading? Yes, we can implement a pre-fetching procedure to cache reading list items offline in batches or something. For instance, that's what Pocket does. > > We should only add URLs to our > > database if the page is an article. > > Maybe now's the time to relax that restriction? That is, would users be > better served by letting them put *anything* in Reading List, even if we > can't turn on Reader Mode for the page? > > (Pocket lets you store anything, which is a feature that seems to be widely > used -- they have a "top videos" chart, for example. This restriction on > content is one reason why I don't use Fennec's Reader functionality at all, > instead sharing to Pocket from Quickshare and switching apps.) I'm definitely open to re-discuss the types of content that can go in the readability. However, right now, there's an assumption that once an article is added to reading list, it will be available offline. I really don't want us to lose that. Pocket has an Android service that caches everything to be accessible offline that runs every time the device gets online. We could do something similar. My point here is that just adding an entry in the Content Provider is not enough. We'll still need a reliable way to get pages cached for offline usage (even if the readability parsing fails). For example, Pocket caches the whole original page if the readability parsing fails.

I kind of agree with the idea of relaxing the rule of "only show parseable items in the Reading List". Since we can't know before a page is visited if it can be launched in Reader Mode. What if we: - Let users add any link to the reading list - Try to download everything in the Reading List for offline viewing, and create some kind of error screen like I described in bug 782698 for cases where the either the page wasn't downloaded or can't be viewed in Reader Mode. This would really simplify the interaction of saving stuff for later, and make it easier to save a page for later without even visiting it, say, by long pressing a link and adding the page to the reading list with a context menu. Or from an intent.

^ This is basically what Pocket does, btw.

(In reply to Chris Kitching [:ckitching] from comment #18) > Why not, in the event the user adds an article that can't be reformatted, > just show it as usual (Instead of the fancy reader-mode reformatted edition) > We could possibly address the possibility of making more pages reformattable > later, but why not allow absolutely anything on the reading list? A user > could want to read later something that isn't formatted in just the right > way, after all/ I'm ok with allowing users to add anything to the reading list. But we should ensure that the reading list is available offline, at least for readable content. The same assumption might not apply to things like videos and photos though. We should not require an internet connection to access your reading list content (again, at least for the readable stuff). The offline access is a key aspect of this feature, especially on mobile devices.

(In reply to Lucas Rocha (:lucasr) from comment #22) > I'm ok with allowing users to add anything to the reading list. But we > should ensure that the reading list is available offline, at least for > readable content. The same assumption might not apply to things like videos > and photos though. > > We should not require an internet connection to access your reading list > content (again, at least for the readable stuff). The offline access is a > key aspect of this feature, especially on mobile devices. This makes sense to me. If we can start this with a simple "download content in background" service then we should file a bug for just that part and start listing the service's requirements. Given that the download service will be in Java, we'll need to save the content somewhere that allows the Gecko side to easily access it and begin the Reader parse, when needed.

Not working on this at the mo. Unassigning to enable easier theft.

This is relevant to Chris's current work on share handling. We'll need a way to drop a link into Reading List that doesn't require pre-loading.

We do this in the "Add to Firefox" share overlay. Can we close out this bug?