959297 - Get description and approx. reading time for reading list items

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Reader View, defect)

Description

[Sola Ogunsakin [:sola]]

Extract description/preview and approx. reading time from reading list articles

Comment 1

[Sola Ogunsakin [:sola]]

Attachment: bug-959297-fix.patch

-pulling description of article from <meta name="description"/> tags. if the tags not there resort to facebook open-graph description <meta name="og:description"/> or twitter cards description <meta name="twiter:description"/>.

-estimating time to read article with its wordCount. this article says average person reads at 250 - 300 words per minutes http://en.wikipedia.org/wiki/Words_per_minute. Figure time to read in minutes should be wordCount / 275.

Comment 2

[Richard Newman [:rnewman]]

Comment on attachment 8360237 [details] [diff] [review]
bug-959297-fix.patch

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

Drive-by.

::: mobile/android/base/db/LocalBrowserDB.java
@@ +669,5 @@
>          cr.delete(contentUri, urlEquals, urlArgs);
>      }
>  
>      @Override
> +    public void addReadingListItem(ContentResolver cr, String title, String uri, String description, int wordCount) {

I have my doubts about shaping the API in this way. Did you consider other approaches?

::: mobile/android/chrome/content/Readability.js
@@ +1402,5 @@
> +       metaElements        = this._doc.getElementsByTagName('meta'),
> +       descriptionElements = {};
> +
> +
> +  //find description tags

// Find description tags.

@@ +1405,5 @@
> +
> +  //find description tags
> +   for (let i = 0; i< metaElements.length; i++) {
> +      let element = metaElements[i];
> +      let propertyName = element.getAttribute('name');

Always double quotes.

@@ +1408,5 @@
> +      let element = metaElements[i];
> +      let propertyName = element.getAttribute('name');
> +      if(pattern.test(propertyName)) {
> +        let content = element.getAttribute('content');
> +        if(content) {

Always spaces before open parens.

@@ +1415,5 @@
> +        }
> +      }
> +   }
> +
> +   if('description' in descriptionElements) {

Coding style:

  if ("description" in descriptionElements) {
    return descriptionElements["description"];
  }

  if (…)

@@ +1479,5 @@
>      // }
>  
> +    //count the # of white spaces to figure out word count
> +    let r = /\s+/g;
> +    let wordCount = articleContent.textContent.match(r).length;

This strikes me as memory-hungry (and perhaps slow) on larger content. Break this out into a separate method so you can write a little test harness for this, and determine what the right choice is for small and large content.

But more broadly: why not get a general estimate of content length by using a less-precise method, like character count?

Comment 3

[Sola Ogunsakin [:sola]]

Comment on attachment 8360237 [details] [diff] [review]
bug-959297-fix.patch

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

::: mobile/android/base/db/LocalBrowserDB.java
@@ +669,5 @@
>          cr.delete(contentUri, urlEquals, urlArgs);
>      }
>  
>      @Override
> +    public void addReadingListItem(ContentResolver cr, String title, String uri, String description, int wordCount) {

considered passing in a ContentValues object or HashMap containing the fields. would that be better?

Comment 4

[:Margaret Leibovic]

Comment on attachment 8360237 [details] [diff] [review]
bug-959297-fix.patch

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

Nice work!

::: mobile/android/base/db/LocalBrowserDB.java
@@ +669,5 @@
>          cr.delete(contentUri, urlEquals, urlArgs);
>      }
>  
>      @Override
> +    public void addReadingListItem(ContentResolver cr, String title, String uri, String description, int wordCount) {

+1 This method is growing to have a lot of parameters, and we may add more fields in the future, so it would be nice to have a forward-thinking API.

::: mobile/android/chrome/content/Readability.js
@@ +1395,5 @@
> +   * Get description of the article
> +   *
> +   * @return String
> +  **/
> +  _getDescription: function(cleanDoc) {

Since this actually might just return the first paragraph from the article, maybe it should be called _getPreview (or whatever we end up calling this blurb of text), since its real purpose is just to get that text for the UI.

@@ +1396,5 @@
> +   *
> +   * @return String
> +  **/
> +  _getDescription: function(cleanDoc) {
> +   let pattern             = /^((twitter|og):)?description$/gi,

Add a comment explaining what this pattern is supposed to match.

@@ +1410,5 @@
> +      if(pattern.test(propertyName)) {
> +        let content = element.getAttribute('content');
> +        if(content) {
> +          propertyName = propertyName.trim().toLowerCase();
> +          descriptionElements[propertyName] = content;

The name descriptionElements is somewhat confusing here, since this object isn't actually storing elements. Maybe just descriptions? Or descriptionValues?

@@ +1416,5 @@
> +      }
> +   }
> +
> +   if('description' in descriptionElements) {
> +      description = descriptionElements['description'];

Rather that assigning this description local variable, I think it would be clearer if you just made these into return statements.

@@ +1425,5 @@
> +      //use twitter cards description
> +      description = descriptionElements['twitter:description'];
> +   } else {
> +    //no description meta tag, use the article's first paragraph
> +    let paragraphs = cleanDoc.getElementsByTagName('p');

Lazy reviewing: what's the difference between cleanDoc and this._doc?

@@ +1428,5 @@
> +    //no description meta tag, use the article's first paragraph
> +    let paragraphs = cleanDoc.getElementsByTagName('p');
> +    if(paragraphs.length > 0) {
> +      description = paragraphs[0].textContent;
> +    }

Nit: Indentation is off here.

@@ +1479,5 @@
>      // }
>  
> +    //count the # of white spaces to figure out word count
> +    let r = /\s+/g;
> +    let wordCount = articleContent.textContent.match(r).length;

+1 this sounds like a good optimization.

Also, are we actually displaying the word count anywhere in the UI? If this is just going to be used to estimate the amount of time it will take to read the article, something like character count is probably good enough.

Comment 5

[Sola Ogunsakin [:sola]]

Attachment: bug-959297-fix.patch

-Made code styling changes
-add comments
-refactor addReadingListItem() to take in a ContentValues object and validate it for required fields
-Use character count to estimate time to read article using fact that average word is 5.1 characters long. 

@margarat: cleanDoc is the root element for the processed doc that will be shown to the user. this._doc is the original web page.

Comment 6

[Richard Newman [:rnewman]]

Comment on attachment 8360783 [details] [diff] [review]
bug-959297-fix.patch

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

Chenxia and Margaret have this under control.

Comment 7

[:Margaret Leibovic]

Comment on attachment 8360783 [details] [diff] [review]
bug-959297-fix.patch

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

Sorry for the slow response, this is looking good!

::: mobile/android/base/BrowserApp.java
@@ +1196,5 @@
> +                final String excerpt = message.optString("excerpt");
> +                final ContentValues values = new ContentValues();
> +                // Don't have SQL columns names for "excerpt" and "length"
> +                // Only store title and url for now, will add rest when bug#959290 is done
> +                values.put(Bookmarks.URL, url);

Instead of declaring a local variable, I think it would be fine to just put message.getString("url") directly in here (and likewise for the other things you're going to put in the ContentValues).

::: mobile/android/base/db/LocalBrowserDB.java
@@ +676,5 @@
> +        for (String field: REQUIRED_FIELDS) {
> +        	if (!values.containsKey(field)) {
> +        		throw new IllegalArgumentException("Did not specify field: " + field);
> +        	}
> +        }

4-space indentation, please.

@@ +684,1 @@
>          addBookmarkItem(cr, title, uri, Bookmarks.FIXED_READING_LIST_ID);

Is the plan to replace this addBookmarkItem call with a method that inserts an item into the new reading list content provider? If so, is the plan to have that method take a ContentValues as a parameter. If not, I think this use of ContentValues isn't actually that helpful, and it would probably be better to just pass the parameters directly. Sorry for the churn on that :(

::: mobile/android/chrome/content/Readability.js
@@ +1394,5 @@
>    /**
> +   * Get excerpt of the article
> +   *
> +   * @param Element
> +   * @return String

If you're going to put these docs here, you should add some more to explain what these values represent.

@@ +1404,5 @@
> +    // Match description, og:description or twitter:description
> +    let pattern = /^((twitter|og):)?description$/gi;
> +
> +    // Find description tags
> +    for (let i = 0; i < metaElements.length; i++) {

You should be able to replace this with metaElements.forEach(function(element) { ...

@@ +1407,5 @@
> +    // Find description tags
> +    for (let i = 0; i < metaElements.length; i++) {
> +      let element = metaElements[i];
> +      let propertyName = element.getAttribute("name");
> +      if(pattern.test(propertyName)) {

Nit: Space between the if and the (

@@ +1418,5 @@
> +    }
> +
> +    if ("description" in values) {
> +      return values["description"];
> +    } else if ("og:description" in values) {

No need for an else statement after a return.

Comment 8

[Chenxia Liu [:liuche]]

Comment on attachment 8360783 [details] [diff] [review]
bug-959297-fix.patch

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

::: mobile/android/base/db/LocalBrowserDB.java
@@ +670,5 @@
>      }
>  
>      @Override
> +    public void addReadingListItem(ContentResolver cr, ContentValues values) {
> +        // Will add excerpt and length fields when bug#959290 is finished

Since there are a lot of things that need to be done, consider using // TODO so these stand out from regular comments.

::: mobile/android/chrome/content/Readability.js
@@ +1393,5 @@
>  
>    /**
> +   * Get excerpt of the article
> +   *
> +   * @param Element

Be sure to remember to fill out this javadoc a little more.

@@ +1397,5 @@
> +   * @param Element
> +   * @return String
> +  **/
> +  _getExcerpt: function(cleanDoc) {
> +    let values       = {},

What's going on here? Did you mean to use a ; instead?

@@ +1478,5 @@
>      //     this._appendNextPage(nextPageLink);
>      //   }).bind(this), 500);
>      // }
>  
> +    let excerpt = this._getExcerpt(articleContent);

Maybe rename this to aExcerpt.

Comment 9

[Sola Ogunsakin [:sola]]

Attachment: bug-959297-fix.patch

Comment 10

[Sola Ogunsakin [:sola]]

Attachment: bug-959297-fix.patch

Comment 11

[Chenxia Liu [:liuche]]

Comment on attachment 8385613 [details] [diff] [review]
bug-959297-fix.patch

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

Looks good, I'd like to have Margaret have a look at the Readability.js part as well - it seems mostly okay to me, but I'd like to see what her suggestions are for the js, and if there is a better way to do it.

Also, leave the whitespace changes out of this patch, but feel free to make another ws-only patch! Changing whitespace messes with looking through the commit history, especially with changes on lines with content.

::: mobile/android/base/BrowserApp.java
@@ +1175,5 @@
>                  handleReaderListStatusRequest(message.getString("url"));
>              } else if (event.equals("Reader:Added")) {
>                  final int result = message.getInt("result");
> +                final ContentValues values = new ContentValues();
> +                values.put(ReadingListItems.URL, message.optString("url"));

Abstract setting the ContentValues to a separate function.

::: mobile/android/base/db/LocalBrowserDB.java
@@ +702,5 @@
> +    public void addReadingListItem(ContentResolver cr, final ContentValues values) {
> +        // Check that required fields are present.
> +        for (String field: ReadingListItems.REQUIRED_FIELDS) {
> +            if (!values.containsKey(field)) {
> +                throw new IllegalArgumentException("Did not specify field: " + field);

Make this more informative: "Missing required field: " or something of the like.

@@ +706,5 @@
> +                throw new IllegalArgumentException("Did not specify field: " + field);
> +            }
> +        }
> +
> +        // Clear delete flag if necessary

This comment is confusing - you're definitely setting the flag.

::: mobile/android/chrome/content/Readability.js
@@ +1228,5 @@
>      }).bind(this)(nextPageLink, articlePage);
>    },
>  
>    /**
> +   * Get an elements class/id weight. Uses regular expressions to tell if this

Let's leave this line unchanged in this patch, since there's actually content. Feel free to split this (along with the other ws changes) out into another "Part 0: Whitespace" patch if you want, though.

@@ +1391,5 @@
>      this._flags = this._flags & ~flag;
>    },
>  
>    /**
> +   * Get excerpt of the article. Attempts to get the

Clean up this comment a bit - this is redundant/conflicting. Just use the "Attempts to..." part, and mention what it does as a fallback (empty string).

@@ +1392,5 @@
>    },
>  
>    /**
> +   * Get excerpt of the article. Attempts to get the
> +   * excerpt from the following sources (listed in order):

"from these sources in the following order:" looks cleaner (no paren text).

@@ +1393,5 @@
>  
>    /**
> +   * Get excerpt of the article. Attempts to get the
> +   * excerpt from the following sources (listed in order):
> +   * -> meta description tag

Don't use ->, just use -, or even number them.

@@ +1399,5 @@
> +   * -> twitter cards description
> +   * -> article's first paragraph
> +   *
> +   * @param Element
> +   * @return String

Fill these out if you're going to include them.

@@ +1401,5 @@
> +   *
> +   * @param Element
> +   * @return String
> +  **/
> +  _getExcerpt: function(cleanDoc) {

I would move this function up next to _grabArticle, because it seems kind of out of place down here.

@@ +1405,5 @@
> +  _getExcerpt: function(cleanDoc) {
> +    let values = {};
> +    let metaElements = this._doc.getElementsByTagName("meta");
> +
> +    // Match description, og:description or twitter:description

Again, be more descriptive in this comment, esp since og isn't obvious - maybe: Facebook's og:description (Open Graph) or Twitter's twitter:description (Cards).

@@ +1407,5 @@
> +    let metaElements = this._doc.getElementsByTagName("meta");
> +
> +    // Match description, og:description or twitter:description
> +    // in name attribute.
> +    let pattern = /^\s*((twitter)\s*:\s*)?description\s*$/gi;

More descriptive var name (the other var is fbPattern, so use consistent naming style).

@@ +1410,5 @@
> +    // in name attribute.
> +    let pattern = /^\s*((twitter)\s*:\s*)?description\s*$/gi;
> +
> +    // Match og:description in property attribute.
> +    let fbPattern = /^\s*og\s*:\s*description\s*$/gi;

Maybe you want to combine these so you only have one pattern?

@@ +1412,5 @@
> +
> +    // Match og:description in property attribute.
> +    let fbPattern = /^\s*og\s*:\s*description\s*$/gi;
> +
> +    // Find description tags

Nit: period

::: mobile/android/chrome/content/browser.js
@@ +1816,5 @@
>        if (!aOptions)
>          return;
>  
>        sendMessageToJava({
> +        type: "Menu:Update",

Unrelated + has content, either split into a WS patch or leave it.

@@ +1842,5 @@
>     *        timeout:     A time in milliseconds. The notification will not
>     *                     automatically dismiss before this time.
>     *        checkbox:    A string to appear next to a checkbox under the notification
>     *                     message. The button callback functions will be called with
> +   *                     the checked state as an argument.

Unrelated + has content, either split into a WS patch or leave it.

@@ +2958,5 @@
>                                  reflozTimeout,
>                                  viewportWidth - 15);
>    },
>  
> +  /**

ws

@@ +3588,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;

ws

@@ +6261,5 @@
>      if (!aElement || !(aElement instanceof Ci.nsIDOMNSEditableElement))
>        return;
>      let target = aElement.QueryInterface(Ci.nsIDOMNSEditableElement);
>      target.editor.paste(Ci.nsIClipboard.kGlobalClipboard);
> +    target.focus();

ws

@@ +6515,5 @@
>      this._lastStatus = aBrowser.securityUI
>                                 .QueryInterface(Components.interfaces.nsISSLStatusProvider)
>                                 .SSLStatus;
>  
> +    // Don't pass in the actual location object, since it can cause us to

ws

@@ +7412,5 @@
>          } else {
>            throw new Error("Reader:Add requires a tabID or an URL as argument");
>          }
>  
> +        let sendResult = function(result, article) {

Maybe prefix these with a* to make it clear they're args (like aResult, aArticle).

@@ +7414,5 @@
>          }
>  
> +        let sendResult = function(result, article) {
> +          article = article || {};
> +          dump("Reader:Add success=" + result + ", url=" + article.url + ", title=" + article.title + ", excerpt=" + article.excerpt);

Probably want to use this.log instead.

@@ +7455,5 @@
>          break;
>        }
>  
>        case "Reader:Remove": {
> +        // Name change for clarity.

Don't leave this comment in, it won't make sense out of context.

@@ +7457,5 @@
>  
>        case "Reader:Remove": {
> +        // Name change for clarity.
> +        let url = aData;
> +        this.removeArticleFromCache(url, function(success) {

a* prefix here too.

Comment 12

[Sola Ogunsakin [:sola]]

(In reply to Chenxia Liu [:liuche] from comment #11)
> Comment on attachment 8385613 [details] [diff] [review]
> bug-959297-fix.patch
> 
> Review of attachment 8385613 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +706,5 @@
> > +                throw new IllegalArgumentException("Did not specify field: " + field);
> > +            }
> > +        }
> > +
> > +        // Clear delete flag if necessary
> 
> This comment is confusing - you're definitely setting the flag.
> 
I'm setting the flag to 0
> @@ +1410,5 @@
> > +    // in name attribute.
> > +    let pattern = /^\s*((twitter)\s*:\s*)?description\s*$/gi;
> > +
> > +    // Match og:description in property attribute.
> > +    let fbPattern = /^\s*og\s*:\s*description\s*$/gi;
> 
> Maybe you want to combine these so you only have one pattern?
> 
One of the patterns tests the "name" attribute. The other pattern tests the "property" attribute. Combining them could match invalid elements. I've updated the variable naming to be more descriptive.

Comment 13

[Sola Ogunsakin [:sola]]

Attachment: bug-889351.patch

Comment 14

[Sola Ogunsakin [:sola]]

Attachment: bug-959297.patch

Comment 15

[:Margaret Leibovic]

Comment on attachment 8389892 [details] [diff] [review]
bug-959297.patch

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

I want to think about this a bit more, but I have enough feedback that I'd like to see a new version anyway.

::: mobile/android/base/BrowserApp.java
@@ +396,5 @@
>              }
>          });
>      }
>  
> +    void handleReaderAdded(int result, final ContentValues values) {

You should make this private, while you're changing this signature.

@@ +1233,5 @@
>              Log.e(LOGTAG, "Exception handling message \"" + event + "\":", e);
>          }
>      }
>  
> +    public ContentValues messageToReadingListContentValues(JSONObject message) {

Make this private. Also, I would put it up next to handleReaderAdded.

::: mobile/android/base/db/LocalBrowserDB.java
@@ +698,5 @@
>          cr.delete(contentUri, urlEquals, urlArgs);
>      }
>  
>      @Override
> +    public void addReadingListItem(ContentResolver cr, final ContentValues values) {

Why the final?

@@ +702,5 @@
> +    public void addReadingListItem(ContentResolver cr, final ContentValues values) {
> +        // Check that required fields are present.
> +        for (String field: ReadingListItems.REQUIRED_FIELDS) {
> +            if (!values.containsKey(field)) {
> +                throw new IllegalArgumentException("Missing required field: " + field);

Make this exception mention that it's about adding a reading list item, such as "Missing required field for reading list item: " + ...

::: mobile/android/chrome/content/Readability.js
@@ +730,5 @@
> +   *
> +   * @param Element - root element of the processed version page
> +   * @return String - excerpt of the article
> +  **/
> +  _getExcerpt: function(cleanDoc) {

I would rename this parameter articleContent, to match the variable that's passed it.

@@ +743,5 @@
> +    let propertyPattern = /^\s*og\s*:\s*description\s*$/gi;
> +
> +    // Find description tags.
> +    for (let i = 0; i < metaElements.length; i++) {
> +      let name = null;

Nit: declare `name` variable right above the if block where it might be set. You also don't need to set it to null, you can just leave it undefined.

@@ +767,5 @@
> +    }
> +
> +    if ("description" in values) {
> +      return values["description"];
> +    } else if ("og:description" in values) {

No need for else statements if you're returning within the previous if. So this would look like:

if (foo) {
  return bar;
}
if (anotherFoo) {
  return anotherBar;
}

That also saves you from needing to make the last else statement.

@@ +782,5 @@
> +      }
> +    }
> +
> +    return "";
> +  },

I would love to see some unit tests for this! I bet we can use JavascriptTest to run a simple xpcshell test that loads Readability.js, parses various testcases, then checks the excerpts we get back from them. Can you file a follow-up for this?

@@ +1500,5 @@
>      //     this._appendNextPage(nextPageLink);
>      //   }).bind(this), 500);
>      // }
>  
> +    let aExcerpt = this._getExcerpt(articleContent);

Rename this `excerpt`. We only use the "a" prefix convention for parameters to functions (and it's not a convention used in this file anyway).

::: mobile/android/chrome/content/browser.js
@@ +7414,5 @@
>          } else {
>            throw new Error("Reader:Add requires a tabID or an URL as argument");
>          }
>  
> +        let sendResult = function(aResult, aArticle) {

Don't update this to use the "a" prefix.

@@ +7430,5 @@
>          }.bind(this);
>  
>          let handleArticle = function(article) {
>            if (!article) {
> +            sendResult(this.READER_ADD_FAILED, null);

Why is this changed from an empty string to null?

@@ +7458,5 @@
>        }
>  
>        case "Reader:Remove": {
> +        let url = aData;
> +        this.removeArticleFromCache(url, function(aSuccess) {

No "a" prefix.

Comment 16

[Sola Ogunsakin [:sola]]

(In reply to :Margaret Leibovic from comment #15)
> I would love to see some unit tests for this! I bet we can use
> JavascriptTest to run a simple xpcshell test that loads Readability.js,
> parses various testcases, then checks the excerpts we get back from them.
> Can you file a follow-up for this?
sure
> @@ +7430,5 @@
> >          }.bind(this);
> >  
> >          let handleArticle = function(article) {
> >            if (!article) {
> > +            sendResult(this.READER_ADD_FAILED, null);
> 
> Why is this changed from an empty string to null?
Changed the second argument of sendResult to an article object instead of the title string.

Comment 17

[Sola Ogunsakin [:sola]]

Attachment: bug-959297.patch

Comment 18

[:Margaret Leibovic]

Comment on attachment 8389968 [details] [diff] [review]
bug-959297.patch

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

Almost there! Next version will be an r+.

::: mobile/android/base/db/BrowserDB.java
@@ +271,5 @@
>      public static void updateBookmark(ContentResolver cr, int id, String uri, String title, String keyword) {
>          sDb.updateBookmark(cr, id, uri, title, keyword);
>      }
>  
> +    public static void addReadingListItem(ContentResolver cr, final ContentValues values) {

I didn't notice this one last time, also remove the final here.

::: mobile/android/chrome/content/browser.js
@@ +7422,5 @@
>            sendMessageToJava({
>              type: "Reader:Added",
>              result: result,
> +            title: article.title,
> +            url: article.url,

You're changing the behavior here... previously this would still send the url, even if an empty string was passed in as the title for the article. So I think you should still directly pass the url, in case an article isn't passed in.

Alternately, instead of passing in null below, you can just pass in an object with a url, and then you could remove the line where you assign article to an empty object.

Comment 19

[Sola Ogunsakin [:sola]]

(In reply to :Margaret Leibovic from comment #18)
> ::: mobile/android/chrome/content/browser.js
> @@ +7422,5 @@
> >            sendMessageToJava({
> >              type: "Reader:Added",
> >              result: result,
> > +            title: article.title,
> > +            url: article.url,
> 
> You're changing the behavior here... previously this would still send the
> url, even if an empty string was passed in as the title for the article. So
> I think you should still directly pass the url, in case an article isn't
> passed in.
> 
> Alternately, instead of passing in null below, you can just pass in an
> object with a url, and then you could remove the line where you assign
> article to an empty object.

Alright, since "url" is globally scoped I'll leave it the way it was.

Comment 20

[Sola Ogunsakin [:sola]]

Attachment: bug-959297.patch

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/21c0c5f02e83

Comment 23

[[github robot]]

Commit pushed to master at https://github.com/mozilla/webmaker.org

https://github.com/mozilla/webmaker.org/commit/11db0c0e338bfba1c12a1a76de5a496fec9b9808
Fix [bug959297v2] - Added CSP into webmaker.org

Comment 25

[Cristina Madaras, QA [:CristinaM]]

Verified as fixed in:
Build: Firefox for Android 36.0a1 (2014-10-22)
Device: Nexus 4 (Android 4.4.4)