852828 - Add basic support for subscribing to feeds (RSS/Atom)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

This bug is more simplistic than bug 477714. In this bug we are only adding the minimal code and UI to be able to add feeds found on webpages to web-based feed services.

This bug does _not_ include many features. Those will come later, if at all:
* No support for navigator.registerContentHandler(...) [separate bug]
  Only the built-in providers will function in this bug
* No support for previewing the feed before subscribing
* No support for local feed reader apps. I didn't find any way to get those to subscribe anyway. I expected to be able to use some Android Intents, but found nothing supported. Maybe some exist.

Comment 1

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

Attachment: WIP 1

This patch adds enough code and UI to get basic feed subscription working:
* DOMLinkAdded listens for rel="alternate" and allows rss and atom types
  * I copied the desktop approach, but not the code (at least not verbatim)
* Tabs listens for "Link:Feeds" and enables feeds on a tab
  * Renamed "DOMLinkAdded" to "Link:Favicon"
* Tab has enough code to know a feed exists for the page, but nothing more. All real work is done in JS
* Added a "Subscribe" menu to the URL context menu if a feed exists on the page
* Fires a "Feeds:Subscribe" message to JS, where we
  * Build a prompt for the feed, if more than one exists
  * Build a prompt for the web handler
  * Open a new tab using the "add feed" URL for the handler

Things that could improve here:
* DOMLinkAdded handler is getting a bit large. Could pull favicon out into a function and feeds into FeedHandler
* Double check the prefs I added to see what's not needed
* Determine how much of Firefox Desktop we want to port/fork. I did very little porting and no forking. I don't think we need to fork anything for this patch. Adding support for navigator.registerContentHandler will require a component, but it can be gutted a lot too.

Looking for feedback on the approach, the structure, the use of Prompts and whatever else you see.

Comment 2

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

Find desktop bits and pieces here:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js?rev=83cb7a59803f#3335
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/utilityOverlay.js?rev=6396d766a611#633
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-feeds.js#41
http://mxr.mozilla.org/mozilla-central/source/browser/components/feeds/src/FeedWriter.js#1214
http://mxr.mozilla.org/mozilla-central/source/browser/components/feeds/src/WebContentConverter.js#805

Comment 3

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

Test build here:
http://people.mozilla.com/~mfinkle/fennec/fennec-22.0a1-feeds.apk

Comment 4

[Brian Nicholson (:bnicholson)]

Comment on attachment 727047 [details] [diff] [review]
WIP 1

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

::: mobile/android/chrome/content/browser.js
@@ +3615,5 @@
> +            rel: list.join(" "),
> +            size: maxSize
> +          };
> +        } else if (list.indexOf("[alternate]") != -1) {
> +          let type = target.type.toLowerCase().replace(/^\s+|\s*(?:;.*)?$/g, "");

Since this regex took a few minutes me to understand, I'll make a note of it here. It removes any whitespace at the beginning of the string, and removes any whitespace from the end of the tag along with any following text beginning with ";" (I'm not sure why a semicolon would be in the type attribute though).

So:
"  application/rss+xml   " => "application/rss+xml"
"  application/rss+xml;abcxyz   " => "application/rss+xml"
"  application/rss+xml;   abcxyz" => "application/rss+xml"

@@ +3625,5 @@
> +              ContentAreaUtils.urlSecurityCheck(target.href, target.ownerDocument.nodePrincipal, Ci.nsIScriptSecurityManager.DISALLOW_INHERIT_PRINCIPAL);
> +
> +              if (!this.browser.feeds)
> +                this.browser.feeds = [];
> +              this.browser.feeds.push({ href: target.href, title: target.title, type: type });

Since the feeds array is set on the tab's browser object, I think this will result in a collection of old feed URLs that accumulate as the user browses through different pages. Maybe you could reset the feeds array after each location change? Also, why attach the feeds array to the tab's browser instead of the tab itself?

Comment 5

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

(In reply to Brian Nicholson (:bnicholson) from comment #4)

> @@ +3625,5 @@
> > +              ContentAreaUtils.urlSecurityCheck(target.href, target.ownerDocument.nodePrincipal, Ci.nsIScriptSecurityManager.DISALLOW_INHERIT_PRINCIPAL);
> > +
> > +              if (!this.browser.feeds)
> > +                this.browser.feeds = [];
> > +              this.browser.feeds.push({ href: target.href, title: target.title, type: type });
> 
> Since the feeds array is set on the tab's browser object, I think this will
> result in a collection of old feed URLs that accumulate as the user browses
> through different pages. Maybe you could reset the feeds array after each
> location change? Also, why attach the feeds array to the tab's browser
> instead of the tab itself?

browser.xml already seem to "free" the feeds on pagehide:
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/browser.xml#557

But I think explicitly doing it in our browser.js somewhere is a good idea.

Comment 6

[:Margaret Leibovic]

Comment on attachment 727047 [details] [diff] [review]
WIP 1

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

This is looking good. You should turn this into a lazy-loaded script with your patch from bug 854107!

::: mobile/android/base/GeckoApp.java
@@ +2615,5 @@
>              }
> +            case R.id.subscribe: {
> +                Tab tab = Tabs.getInstance().getSelectedTab();
> +                if (tab != null) {
> +                    if (tab.getFeedsEnabled()) {

Nit: You could combine these checks on one line to avoid an extra level of indentation.

::: mobile/android/chrome/content/browser.js
@@ +8753,5 @@
> +          listitems: []
> +        };
> +
> +        // Build the list of feeds
> +        for (let i=0; i<feeds.length; i++) {

Nit: Put spaces in here.

@@ +8770,5 @@
> +        feedIndex = 0;
> +      }
> +
> +      if (feedIndex == -1)
> +        return;

When would this happen? If the user cancels the select dialog?

@@ +8785,5 @@
> +          listitems: []
> +        };
> +
> +        // Build the list of handlers
> +        for (var i = 0; i < handlers.length; ++i) {

You probably just copied this, but this could be a let instead of var.

Comment 7

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

(In reply to :Margaret Leibovic from comment #6)

> This is looking good. You should turn this into a lazy-loaded script with
> your patch from bug 854107!

Done

> > +                if (tab != null) {
> > +                    if (tab.getFeedsEnabled()) {
> 
> Nit: You could combine these checks on one line to avoid an extra level of
> indentation.

Done

> > +        // Build the list of feeds
> > +        for (let i=0; i<feeds.length; i++) {
> 
> Nit: Put spaces in here.

Done

> > +      if (feedIndex == -1)
> > +        return;
> 
> When would this happen? If the user cancels the select dialog?

Right

> > +        // Build the list of handlers
> > +        for (var i = 0; i < handlers.length; ++i) {
> 
> You probably just copied this, but this could be a let instead of var.

Done

Comment 8

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

Attachment: patch

* Made changes from feedback reviews.
* Removed Netblur entry until we verify it works well

Comment 9

[:Margaret Leibovic]

Comment on attachment 732056 [details] [diff] [review]
patch

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

There appear to be unused things that are probably just leftover from copy/pasting. Push back if you think we'll need them for some future use, but I'm a fan of keeping it simple until we need things.

::: mobile/android/chrome/content/FeedHandler.js
@@ +8,5 @@
> +  TYPE_MAYBE_FEED: "application/vnd.mozilla.maybe.feed",
> +
> +  _contentTypes: {},
> +
> +  getContentHandlers: function fh_getContentHandlers(contentType) {

Is there a reason to make this take a type parameter? You only ever pass in TYPE_MAYBE_FEED. And if you are ever only checking for that one type, you could simplify loadContentHandlers to only store TYPE_MAYBE_FEED types.

@@ +9,5 @@
> +
> +  _contentTypes: {},
> +
> +  getContentHandlers: function fh_getContentHandlers(contentType) {
> +    if (!this.contentHandlers)

I don't see this.contentHandlers get set anywhere. Should this be checking this._contentTypes? And if you do cache the content handlers properly, can we assume that they'll never change while the browser is running?

@@ +14,5 @@
> +      this.loadContentHandlers();
> +
> +    if (!(contentType in this._contentTypes))
> +      return [];
> +    

Nit: whitespace

@@ +17,5 @@
> +      return [];
> +    
> +    return this._contentTypes[contentType];
> +  },
> +  

Nit: whitespace

@@ +20,5 @@
> +  },
> +  
> +  loadContentHandlers: function fh_loadContentHandlers() {
> +    if (this._contentTypes)
> +      this._contentTypes = {};

This if condition is always true, so you can get rid of it and just always create the new empty object.

@@ +43,5 @@
> +      let branch = Services.prefs.getBranch(this.PREF_CONTENTHANDLERS_BRANCH + nums[i] + ".");
> +      let vals = branch.getChildList("");
> +      if (vals.length == 0)
> +        return;
> +  

Nit: whitespace

@@ +51,5 @@
> +        let title = branch.getComplexValue("title", Ci.nsIPrefLocalizedString).data;
> +
> +        if (!(type in this._contentTypes))
> +          this._contentTypes[type] = [];
> +        this._contentTypes[type].push({ contentType: type, uri: uri, name: title });

You never use this contentType property.

@@ +103,5 @@
> +      let feedURL = feeds[feedIndex].href;
> +
> +      // Next, we decide on which service to send the feed
> +      let handlers = this.getContentHandlers(this.TYPE_MAYBE_FEED);
> +      if (handlers.length != 0) {

You can save a level of indentation if you just return early if handlers.length == 0.

::: mobile/android/chrome/content/browser.js
@@ +3036,5 @@
> +        let json;
> +        if (list.indexOf("[icon]") != -1) {
> +          // We want to get the largest icon size possible for our UI.
> +          let maxSize = 0;
> +  

Nit: whitespace

@@ +3041,5 @@
> +          // We use the sizes attribute if available
> +          // see http://www.whatwg.org/specs/web-apps/current-work/multipage/links.html#rel-icon
> +          if (target.hasAttribute("sizes")) {
> +            let sizes = target.getAttribute("sizes").toLowerCase();
> +  

Nit: whitespace

@@ +3044,5 @@
> +            let sizes = target.getAttribute("sizes").toLowerCase();
> +  
> +            if (sizes == "any") {
> +              // Since Java expects an integer, use -1 to represent icons with sizes="any"
> +              maxSize = -1; 

Nit: whitespace

@@ +3054,5 @@
> +                maxSize = Math.max(maxSize, Math.max(w, h));
> +              });
> +            }
> +          }
> +  

Nit: whitespace

@@ +3068,5 @@
> +        } else if (list.indexOf("[alternate]") != -1) {
> +          let type = target.type.toLowerCase().replace(/^\s+|\s*(?:;.*)?$/g, "");
> +          let isFeed = (type == "application/rss+xml" || type == "application/atom+xml");
> +
> +          if (isFeed) {

You could save some indentation and just return early if (!isFeed).

@@ +3075,5 @@
> +              ContentAreaUtils.urlSecurityCheck(target.href, target.ownerDocument.nodePrincipal, Ci.nsIScriptSecurityManager.DISALLOW_INHERIT_PRINCIPAL);
> +
> +              if (!this.browser.feeds)
> +                this.browser.feeds = [];
> +              this.browser.feeds.push({ href: target.href, title: target.title, type: type });

This type property is also unused.

@@ +3076,5 @@
> +
> +              if (!this.browser.feeds)
> +                this.browser.feeds = [];
> +              this.browser.feeds.push({ href: target.href, title: target.title, type: type });
> +  

Nit: whitespace

@@ +3086,5 @@
>            }
>          }
>  
> +        if (json)
> +          sendMessageToJava(json);

I think it might be nicer to just call sendMessageToJava where you make the JSON blobs, instead of storing this json variable.

Comment 10

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

(In reply to :Margaret Leibovic from comment #9)

> > +  getContentHandlers: function fh_getContentHandlers(contentType) {
> 
> Is there a reason to make this take a type parameter? You only ever pass in
> TYPE_MAYBE_FEED. And if you are ever only checking for that one type, you
> could simplify loadContentHandlers to only store TYPE_MAYBE_FEED types.

I'd rather keep it like this until we're sure we don't need any other types. It doesn't add complexity and it would be easier to refactor like this, instead of hard coding to a specific type.

> > +  getContentHandlers: function fh_getContentHandlers(contentType) {
> > +    if (!this.contentHandlers)
> 
> I don't see this.contentHandlers get set anywhere. Should this be checking
> this._contentTypes? And if you do cache the content handlers properly, can
> we assume that they'll never change while the browser is running?

Yes, _contentTypes. Fixed.
For now the list is fixed, by the values we store in prefs. However, to support registerContentHandler, we'll need to be more dynamic. That's a followup bug.

> > +  loadContentHandlers: function fh_loadContentHandlers() {
> > +    if (this._contentTypes)
> > +      this._contentTypes = {};
> 
> This if condition is always true, so you can get rid of it and just always
> create the new empty object.

Changed.

> > +        if (!(type in this._contentTypes))
> > +          this._contentTypes[type] = [];
> > +        this._contentTypes[type].push({ contentType: type, uri: uri, name: title });
> 
> You never use this contentType property.

We might need this for the registerContentHandler code. Prefer leaving for now.

> > +      if (handlers.length != 0) {
> 
> You can save a level of indentation if you just return early if
> handlers.length == 0.

Done

> > +          if (isFeed) {
> 
> You could save some indentation and just return early if (!isFeed).

Done
> > +                this.browser.feeds = [];
> > +              this.browser.feeds.push({ href: target.href, title: target.title, type: type });
> 
> This type property is also unused.

I was keeping this in case we could use Android intents based on mime types. We can, but I found no apps configured for that yet. Still, using local apps is a followup, so I'd like to keep it.

> > +        if (json)
> > +          sendMessageToJava(json);
> 
> I think it might be nicer to just call sendMessageToJava where you make the
> JSON blobs, instead of storing this json variable.

OK, done.

Comment 11

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

Attachment: patch v2

Updated based on review comments

Comment 12

[:Margaret Leibovic]

Comment on attachment 732651 [details] [diff] [review]
patch v2

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

::: mobile/android/chrome/content/FeedHandler.js
@@ +9,5 @@
> +
> +  _contentTypes: {},
> +
> +  getContentHandlers: function fh_getContentHandlers(contentType) {
> +    if (!this._contentHandlers)

Did you mean this._contentTypes? If so, this would still be a bad check, since !{} is false, so you'd probably want to initialize _contentTypes to null.

Comment 13

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

(In reply to :Margaret Leibovic from comment #12)
> Comment on attachment 732651 [details] [diff] [review]
> patch v2
> 
> Review of attachment 732651 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/chrome/content/FeedHandler.js
> @@ +9,5 @@
> > +
> > +  _contentTypes: {},
> > +
> > +  getContentHandlers: function fh_getContentHandlers(contentType) {
> > +    if (!this._contentHandlers)
> 
> Did you mean this._contentTypes? If so, this would still be a bad check,
> since !{} is false, so you'd probably want to initialize _contentTypes to
> null.

ARRRGH. Yes, exactly.

Comment 14

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

Attachment: patch v3

I <3 _contentXxx

Comment 15

[:Margaret Leibovic]

Comment on attachment 732871 [details] [diff] [review]
patch v3

Hopefully there aren't any more typos I didn't catch!

Comment 16

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/5cea17cdb469

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5cea17cdb469

Comment 18

[Aaron Train [:aaronmt]]

STR for QA

i) Visit a site with a detected feed; e.g, http://www.schneier.com/ or http://blog.mozilla.com
ii) Long-tap on the address-bar and select 'Susbcribe'
iii) Select either of the current managers such as Yahoo! or Google (a new tab opens)
iv) Log in to either, and make sure the subscription got carried over to them for addition

Comment 19

[Alexander L. Slovesnik]

Why Google has been added in default feed handlers for mobile? Google Reader are being discontinued and it's been removed from desktop in Bug 851010
cc'ing Pike cause return of feed support in mobile impacts l10n

Comment 20

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

(In reply to Alexander L. Slovesnik from comment #19)
> Why Google has been added in default feed handlers for mobile? Google Reader
> are being discontinued and it's been removed from desktop in Bug 851010
> cc'ing Pike cause return of feed support in mobile impacts l10n

Because we need something to test. I'll be happy to remove it before we do the final release, but in the mean time Google Reader still works and we need the testing.

I am hoping we can add new services too.

Comment 21

[Teodora Vermesan (:TeoVermesan)]

Test Case added:
https://moztrap.mozilla.org/manage/cases/_detail/43800/

Comment 22

[Lukas Blakk [:lsblakk] use ?needinfo]

This has been noted in the Aurora 23 release notes:

http://www.mozilla.org/en-US/mobile/23.0a2/auroranotes/

If you would like to make any changes or have questions/concerns please contact me directly.