Move readability check to web worker

VERIFIED FIXED in Firefox 16

Status

()

VERIFIED FIXED
7 years ago
10 months ago

People

(Reporter: lucasr, Assigned: bnicholson)

Tracking

unspecified
Firefox 17
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox16+ verified)

Details

Attachments

(7 attachments, 3 obsolete attachments)

9.67 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
3.69 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
6.09 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
4.70 KB, patch
lucasr
: review+
mfinkle
: review+
Details | Diff | Splinter Review
10.58 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
27.47 KB, patch
lucasr
: review+
mfinkle
: review+
Details | Diff | Splinter Review
53.03 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

7 years ago
This way we can run the full readability check without blocking Gecko in any way.
(Reporter)

Updated

7 years ago
Blocks: 779797
(Assignee)

Updated

7 years ago
Duplicate of this bug: 779389
(Assignee)

Comment 2

7 years ago
Posted patch Part 1: Implement JSDOMParser (obsolete) — Splinter Review
The DOM cannot be used in web workers, so we need our own implementation to be used by Readability.js. This is a pretty minimal implementation that doesn't have some of the overhead from a complete DOM (such as live NodeLists).
Attachment #649137 - Flags: review?(mfinkle)
Attachment #649137 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Comment 3

7 years ago
Since the JSDOMParser allows us to do relatively quick parses in a separate thread, we can just do a full parse for every page visited. This provides two advantages over the existing check:
1) There will be no more false positives (fixing bug 767956)
2) Parsed pages will be immediately available to read or add to the reading list
Attachment #649139 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Comment 4

7 years ago
grabArticle() manipulates elements in the document as it iterates through the allElements NodeList. Since JSDOMParser doesn't support live NodeLists, we have to manually keep the allElements array updated with changes. It's a bit messier, but faster.
Attachment #649143 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Comment 5

7 years ago
Moves the parsing to a web worker. This includes a check to make sure the URL of the page hasn't changed during the parse, which should fix bug 779219.
Attachment #649144 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Comment 6

7 years ago
Carried over from bug 779389. I'm not particularly excited about the relative URL change, but since workers cannot use XPCOM, we don't have tons of options. An alternative might be to message the main thread to do the relative URL fixup, but that could make things messier and slower.
Attachment #649145 - Flags: review?(mfinkle)
Attachment #649145 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Comment 7

7 years ago
No longer needed since we're in a separate thread.
Attachment #649146 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Comment 8

7 years ago
The JSDOMParser cannot handle improperly formed HTML, so bug 777966 needs to land before or at the same time as this one.
Depends on: 777966
(Assignee)

Updated

7 years ago
Blocks: 779219
(Assignee)

Updated

7 years ago
Blocks: 767956
(Assignee)

Comment 9

7 years ago
We'll want to cache all articles so they're readily available as described in comment 3. I've filed bug 780519 for this.
(Assignee)

Comment 10

7 years ago
Here's a test build containing changes from this bug and bug 777966: http://dl.dropbox.com/u/35559547/fennec-reader-worker.apk
(Reporter)

Updated

7 years ago
Attachment #649139 - Flags: review?(lucasr.at.mozilla) → review+
(Reporter)

Comment 11

7 years ago
Comment on attachment 649146 [details] [diff] [review]
Part 6: Remove article generator function in Readability.js

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

::: mobile/android/chrome/content/Readability.js
@@ +386,5 @@
>     *
>     * @param page a document to run upon. Needs to be a full document, complete with body.
>     * @return Element
>    **/
> +  _grabArticle: function (page, callback) {

grabArticle() doesn't need to be async anymore, does it? You should probably return the result directly instead of using a callback then.
Attachment #649146 - Flags: review?(lucasr.at.mozilla) → review-
(Reporter)

Comment 12

7 years ago
Comment on attachment 649145 [details] [diff] [review]
Part 5: Remove XPCOM-related code from Readability.js

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

Nice one. Just want to see a new cleaned up version of this patch.

::: mobile/android/chrome/content/Readability.js
@@ +98,5 @@
>     * @param Element
>     * @return void
>     */
>    _fixRelativeUris: function(articleContent) {
> +    let spec = this._uri.spec;

You don't use spec below, do you?

@@ +101,5 @@
>    _fixRelativeUris: function(articleContent) {
> +    let spec = this._uri.spec;
> +    let scheme = this._uri.scheme;
> +    let prePath = this._uri.prePath;
> +    let pathBase = this._uri.pathBase;

Seem like you should bail (or use "#" as the link) if pathBase is undefined? Error building pathBase in browser.js simply logs the exception but still runs the readability parsing.

@@ +117,5 @@
> +    }
> +
> +    function toAbsoluteURI(uri) {
> +      // If this is already an absolute URI, return it.
> +      if (/^[a-zA-Z]+:/.test(uri))

Isn't it safer if you make the uri doesn't start with "."? Something like "/^[^\.]/?

@@ +122,5 @@
> +        return uri;
> +
> +      // If this page isn't one of the whitelisted schemes, don't resolve its
> +      // relative URLs.
> +      if (!shouldResolveRelativeURIs)

This check should probably come first.
Attachment #649145 - Flags: review?(lucasr.at.mozilla) → review-
(Assignee)

Comment 13

7 years ago
(In reply to Lucas Rocha (:lucasr) from comment #12)
> Comment on attachment 649145 [details] [diff] [review]
> Part 5: Remove XPCOM-related code from Readability.js
> 
> Review of attachment 649145 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice one. Just want to see a new cleaned up version of this patch.
> 
> ::: mobile/android/chrome/content/Readability.js
> @@ +98,5 @@
> >     * @param Element
> >     * @return void
> >     */
> >    _fixRelativeUris: function(articleContent) {
> > +    let spec = this._uri.spec;
> 
> You don't use spec below, do you?

Nope, thanks.

> @@ +101,5 @@
> >    _fixRelativeUris: function(articleContent) {
> > +    let spec = this._uri.spec;
> > +    let scheme = this._uri.scheme;
> > +    let prePath = this._uri.prePath;
> > +    let pathBase = this._uri.pathBase;
> 
> Seem like you should bail (or use "#" as the link) if pathBase is undefined?
> Error building pathBase in browser.js simply logs the exception but still
> runs the readability parsing.

I think the only way getting the pathBase can fail is if we're looking at an about: page. We shouldn't even be running the worker on about: pages since we do schemeIs checks in parseDocumentFromTab(). Since getting the pathBase shouldn't ever fail, perhaps we should just return immediately if the exception is caught in browser.js? In fact, we can probably just wrap all of parseInWorker() in a try/catch, and return null for any errors.

> @@ +117,5 @@
> > +    }
> > +
> > +    function toAbsoluteURI(uri) {
> > +      // If this is already an absolute URI, return it.
> > +      if (/^[a-zA-Z]+:/.test(uri))
> 
> Isn't it safer if you make the uri doesn't start with "."? Something like
> "/^[^\.]/?

I don't follow what you mean. This regex is trying to detect an absolute URL (i.e., whether or not the URL string starts with a scheme). If the URL is "foo", that should be relative; if the URL is "http://foo", that should be absolute. Why would look for a "."?

With that said, this regex can be hardened a bit more to look for unusual schemes. Looking at section 3.1 of http://www.ietf.org/rfc/rfc2396.txt, I think the complete regex should be:

/^[a-zA-z][a-zA-Z0-9\+\-\.]*:/

> @@ +122,5 @@
> > +        return uri;
> > +
> > +      // If this page isn't one of the whitelisted schemes, don't resolve its
> > +      // relative URLs.
> > +      if (!shouldResolveRelativeURIs)
> 
> This check should probably come first.

I'm wondering if we even want/need this check. Again, we shouldn't even be running the worker on about: (or any other non-whitelisted scheme) pages since we check for them before creating the worker.
(Assignee)

Comment 14

7 years ago
Attachment #649145 - Attachment is obsolete: true
Attachment #649145 - Flags: review?(mfinkle)
Attachment #649374 - Flags: review?(mfinkle)
Attachment #649374 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Comment 15

7 years ago
Makes Readability.js synchronous again.
Attachment #649146 - Attachment is obsolete: true
Attachment #649392 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Comment 16

7 years ago
Fixed to make getAttribute() return the attribute string, and not the Attribute object itself.
Attachment #649137 - Attachment is obsolete: true
Attachment #649137 - Flags: review?(mfinkle)
Attachment #649137 - Flags: review?(lucasr.at.mozilla)
Attachment #649397 - Flags: review?(mfinkle)
Attachment #649397 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Comment 17

7 years ago
> /^[a-zA-z][a-zA-Z0-9\+\-\.]*:/

I locally fixed the typo here where "A-z" should be "A-Z".

Updated

7 years ago
status-firefox16: --- → affected
(Reporter)

Comment 18

7 years ago
(In reply to Brian Nicholson (:bnicholson) from comment #13)
> > @@ +101,5 @@
> > >    _fixRelativeUris: function(articleContent) {
> > > +    let spec = this._uri.spec;
> > > +    let scheme = this._uri.scheme;
> > > +    let prePath = this._uri.prePath;
> > > +    let pathBase = this._uri.pathBase;
> > 
> > Seem like you should bail (or use "#" as the link) if pathBase is undefined?
> > Error building pathBase in browser.js simply logs the exception but still
> > runs the readability parsing.
> 
> I think the only way getting the pathBase can fail is if we're looking at an
> about: page. We shouldn't even be running the worker on about: pages since
> we do schemeIs checks in parseDocumentFromTab(). Since getting the pathBase
> shouldn't ever fail, perhaps we should just return immediately if the
> exception is caught in browser.js? In fact, we can probably just wrap all of
> parseInWorker() in a try/catch, and return null for any errors.

Wrapping all parseInWorker in a try/catch block makes sense.

> > @@ +117,5 @@
> > > +    }
> > > +
> > > +    function toAbsoluteURI(uri) {
> > > +      // If this is already an absolute URI, return it.
> > > +      if (/^[a-zA-Z]+:/.test(uri))
> > 
> > Isn't it safer if you make the uri doesn't start with "."? Something like
> > "/^[^\.]/?
> 
> I don't follow what you mean. This regex is trying to detect an absolute URL
> (i.e., whether or not the URL string starts with a scheme). If the URL is
> "foo", that should be relative; if the URL is "http://foo", that should be
> absolute. Why would look for a "."?
> 
> With that said, this regex can be hardened a bit more to look for unusual
> schemes. Looking at section 3.1 of http://www.ietf.org/rfc/rfc2396.txt, I
> think the complete regex should be:
> 
> /^[a-zA-z][a-zA-Z0-9\+\-\.]*:/

The regex you used just looked too naive for something involving URIs :-) Use this complete regex then.

> > @@ +122,5 @@
> > > +        return uri;
> > > +
> > > +      // If this page isn't one of the whitelisted schemes, don't resolve its
> > > +      // relative URLs.
> > > +      if (!shouldResolveRelativeURIs)
> > 
> > This check should probably come first.
> 
> I'm wondering if we even want/need this check. Again, we shouldn't even be
> running the worker on about: (or any other non-whitelisted scheme) pages
> since we check for them before creating the worker.

I like the fact that you're being explicit about the protocols we support in reader. Actually, I'd probably only run the readability check on http and https pages.
(Reporter)

Comment 19

7 years ago
Comment on attachment 649374 [details] [diff] [review]
Part 5: Remove XPCOM-related code from Readability.js, v2

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

Patch looks good but the order of the patches seems wrong. This patch should probably come before the patch to make move readability to a web worker—otherwise you'll break bisecting because readability.js should have no XPCOM code once it's moved to web worker. So, this should actually be patch #4.
Attachment #649374 - Flags: review?(lucasr.at.mozilla) → review+
(Reporter)

Comment 20

7 years ago
Comment on attachment 649392 [details] [diff] [review]
Part 6: Remove article generator function in Readability.js, v2

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

::: mobile/android/chrome/content/Readability.js
@@ -467,5 @@
>        let doc = this._doc;
>        let stripUnlikelyCandidates = this._flagIsActive(this.FLAG_STRIP_UNLIKELYS);
>        let isPaging = (page !== null ? true: false);
>  
> -      page = page ? page : this._doc.body;

How is this change related? This line should probably be kept here.

::: mobile/android/chrome/content/readerWorker.js
@@ +7,5 @@
>  self.onmessage = function (msg) {
>    let uri = JSON.parse(msg.data.uri);
>    let doc = new JSDOMParser().parse(msg.data.doc);
> +  let article = new Readability(uri, doc).parse();
> +  postMessage(article);

I assume there's no problem in posting a null message here (in case the parse fails).
Attachment #649392 - Flags: review?(lucasr.at.mozilla) → review+
Tracking this bug instead of bug 767956 since this is where the action is on fixing the issue.
tracking-firefox16: --- → +
(Assignee)

Comment 22

7 years ago
Flagging for security review - in particular, Patch 1, which includes a homebrew DOMParser. I don't think there should be any problems since bug 778582 puts the result in an iframe, but it'd be nice to get some feedback in case we're overlooking something.
Keywords: sec-review-needed
Comment on attachment 649374 [details] [diff] [review]
Part 5: Remove XPCOM-related code from Readability.js, v2

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>+    let serializedDoc;
>+    let uriJSON;
>+    try {
>+      serializedDoc = new XMLSerializer().serializeToString(doc);
>+      uriJSON = JSON.stringify({

technically, uriJSON is really a string here, just like the serializedDoc. Just name these stringified variables like: _uri and _doc, kinda dumb, I know, but sometimes simple is less confusing.
Attachment #649374 - Flags: review?(mark.finkle) → review+
Comment on attachment 649397 [details] [diff] [review]
Part 1: Implement JSDOMParser, v2

>diff --git a/mobile/android/chrome/content/JSDOMParser.js b/mobile/android/chrome/content/JSDOMParser.js

>+  function error(m) {
>+    dump("JSDOMParser error: " + m);
>+  }

how is dump supported?


We need to plan for some tests on this code. Currently, if something bad happens, we just won't get a completed readability check, which isn't too bad. If we start using this code for more than a check, it could be worse. We should also consider profiling this code too.
Attachment #649397 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 25

7 years ago
(In reply to Mark Finkle (:mfinkle) from comment #24)
> Comment on attachment 649397 [details] [diff] [review]
> Part 1: Implement JSDOMParser, v2
> 
> >diff --git a/mobile/android/chrome/content/JSDOMParser.js b/mobile/android/chrome/content/JSDOMParser.js
> 
> >+  function error(m) {
> >+    dump("JSDOMParser error: " + m);
> >+  }
> 
> how is dump supported?

dump() is one of the few DOM-related functions available to workers: https://developer.mozilla.org/en-US/docs/DOM/Worker/Functions_available_to_workers
(Reporter)

Comment 26

7 years ago
Comment on attachment 649144 [details] [diff] [review]
Part 4: Move Readability parsing to a web worker

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

Nice.
Attachment #649144 - Flags: review?(lucasr.at.mozilla) → review+
(Reporter)

Comment 27

7 years ago
Comment on attachment 649143 [details] [diff] [review]
Part 3: Update Readability.js to be compatible with JSDOMParser

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

Giving r+ but please answer my questions.

::: mobile/android/chrome/content/Readability.js
@@ +431,5 @@
> +       * JSDOMParser returns static node lists, not live ones. When we remove
> +       * an element from the document, we need to manually remove it - and all
> +       * of its children - from the allElements array.
> +       */
> +      function purgeChildren(node) {

Maybe purgeNode() is a more appropriate name?

@@ -430,5 @@
>              unlikelyMatchString.search(this.REGEXPS.okMaybeItsACandidate) === -1 &&
>              node.tagName !== "BODY") {
>              dump("Removing unlikely candidate - " + unlikelyMatchString);
>              node.parentNode.removeChild(node);
> -            nodeIndex -= 1;

Why isn't this assignment needed anymore?

@@ +469,3 @@
>              node.parentNode.replaceChild(newNode, node);
> +            allElements[nodeIndex] = newNode;
> +            purgeChildren(node);

I wonder if you should factor out this index-mangling code into inner function (just like purgeChildren()). Would probably make this code a bit easier to read.

@@ +479,5 @@
>                  continue;
>  
>                if (childNode.nodeType === 3) { // Node.TEXT_NODE
>                  let p = doc.createElement('p');
> +                p.innerHTML = childNode.textContent;

How is this related to this patch?
Attachment #649143 - Flags: review?(lucasr.at.mozilla) → review+
(Reporter)

Comment 28

7 years ago
Comment on attachment 649397 [details] [diff] [review]
Part 1: Implement JSDOMParser, v2

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

This code is begging for unit tests as soon as possible. I don't want to block on this but please file a follow up. We'll definitely need test cases for this code otherwise it will be a pain to spot regressions and determine the cause of certain readability bugs (JSDOMParser, Readability, AboutReader, etc). Giving r+ but please implement the suggestions (and maybe the nits) where applicable.

::: mobile/android/chrome/content/JSDOMParser.js
@@ +289,5 @@
> +    },
> +
> +    get nextSibling() {
> +      if (this.parentNode) {
> +        let childNodes = this.parentNode.childNodes;

Why not simply using childNodes.indexOf(this) here?

@@ +302,5 @@
> +
> +    appendChild: function (child) {
> +      if (child.parentNode) {
> +        child.parentNode.removeChild(child);
> +      }

nit: add empty line here.

@@ +308,5 @@
> +      child.parentNode = this;
> +    },
> +
> +    removeChild: function (child) {
> +      let childNodes = this.childNodes;

Same here: you should probably just use childNodes.indexOf(child) here.

@@ +320,5 @@
> +
> +    replaceChild: function (newNode, oldNode) {
> +      let childNodes = this.childNodes;
> +      for (let i = childNodes.length; --i >= 0;) {
> +        if (childNodes[i] == oldNode) {

Use childNodes.indexOf(oldNode) to get the index.

@@ +345,5 @@
> +    this.childNodes = [];
> +  };
> +
> +  Comment.prototype = {
> +    __proto__: new Node(),

Shouldn't this be more like "__proto__: Node.prototype"? Same applies to all other cases.

@@ +486,5 @@
> +    },
> +
> +    set innerHTML(html) {
> +      let parser = new JSDOMParser();
> +      let node = parser.parse(html);

Should probably do "let length = this.childNotes.length" in a use it in the loop below.

@@ +490,5 @@
> +      let node = parser.parse(html);
> +      for (let i = this.childNodes.length; --i >= 0;) {
> +        this.childNodes[i].parentNode = null;
> +      }
> +      this.childNodes = node.childNodes;

Same here.

@@ +559,5 @@
> +  Style.prototype = {
> +    getStyle: function (styleName) {
> +      let attr = this.node.getAttribute("style");
> +      if (!attr)
> +        return undefined;

nit: add empty line here.

@@ +566,5 @@
> +        let style = styles[i].split(":");
> +        let name = style[0].trim();
> +        if (name == styleName)
> +          return style[1].trim();
> +      }

nit: add empty line here.

@@ +583,5 @@
> +          value = value.substr(0, index).trim() + (next ? " " + value.substr(next).trim() : "");
> +          break;
> +        }
> +        index = next;
> +      } while (index);

nit: add empty line here.

@@ +756,5 @@
> +
> +    /**
> +     * Reads child nodes for the given node.
> +     */
> +    readChildren: function (node) {

nit: I tend to prefer prefixing private methods with '_'. Not a big deal though as we're not doing this consistently in, say, browser.js.

@@ +775,5 @@
> +     * @returns the node
> +     */
> +    readNode: function () {
> +      let c = this.nextChar();
> +      

Remove tab or trailing spaces here.

@@ +795,5 @@
> +        return node;
> +      }
> +
> +      c = this.peekNext();
> +      

Remove tab or trailing spaces here.
Attachment #649397 - Flags: review?(lucasr.at.mozilla) → review+
(Assignee)

Comment 29

7 years ago
(In reply to Lucas Rocha (:lucasr) from comment #28)
> Comment on attachment 649397 [details] [diff] [review]
> Part 1: Implement JSDOMParser, v2
> 
> Review of attachment 649397 [details] [diff] [review]:
> 
> ::: mobile/android/chrome/content/JSDOMParser.js
> @@ +289,5 @@
> > +    },
> > +
> > +    get nextSibling() {
> > +      if (this.parentNode) {
> > +        let childNodes = this.parentNode.childNodes;
> 
> Why not simply using childNodes.indexOf(this) here?

Thanks - I forgot that method existed in JS. This change could improve performance a bit.

> @@ +345,5 @@
> > +    this.childNodes = [];
> > +  };
> > +
> > +  Comment.prototype = {
> > +    __proto__: new Node(),
> 
> Shouldn't this be more like "__proto__: Node.prototype"? Same applies to all
> other cases.

I think inheritance in JS is typically done by setting the prototype to an instance of the super class so that the super constructor is called. But since the Node constructor is empty, I guess we can get away with just setting it to Node.prototype in this case.

> @@ +486,5 @@
> > +    },
> > +
> > +    set innerHTML(html) {
> > +      let parser = new JSDOMParser();
> > +      let node = parser.parse(html);
> 
> Should probably do "let length = this.childNotes.length" in a use it in the
> loop below.

Not sure I understand this change; this.childNodes.length only gets used once, when it initializes i in the loop. Why put it into another variable?
(Assignee)

Comment 30

7 years ago
(In reply to Lucas Rocha (:lucasr) from comment #27)
> Comment on attachment 649143 [details] [diff] [review]
> Part 3: Update Readability.js to be compatible with JSDOMParser
> 
> Review of attachment 649143 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Giving r+ but please answer my questions.
> 
> Maybe purgeNode() is a more appropriate name?

Indeed it is.

> 
> @@ -430,5 @@
> >              unlikelyMatchString.search(this.REGEXPS.okMaybeItsACandidate) === -1 &&
> >              node.tagName !== "BODY") {
> >              dump("Removing unlikely candidate - " + unlikelyMatchString);
> >              node.parentNode.removeChild(node);
> > -            nodeIndex -= 1;
> 
> Why isn't this assignment needed anymore?

With live NodeLists, removing the child here would "shift" the NodeList such that this index refers to the next element, so decrementing nodeIndex allows us to revisit the new node at the same index. allElements is just an Array in JSDOMParser, though, so when we remove/purge this node, this element at this index will just be undefined if we try to look at it again.

> @@ +479,5 @@
> >                  continue;
> >  
> >                if (childNode.nodeType === 3) { // Node.TEXT_NODE
> >                  let p = doc.createElement('p');
> > +                p.innerHTML = childNode.textContent;
> 
> How is this related to this patch?

I could've added a getter in JSDOMParser for nodeValue that just returned textContent, but since this is the only place nodeValue is used in Readability.js, I figured it could just use textContent directly. I think this should also be more efficient since this loop is executed a lot in the code, and getters are more costly than properties.
(Assignee)

Updated

7 years ago
Duplicate of this bug: 767956
(Assignee)

Updated

7 years ago
Duplicate of this bug: 779219
Depends on: 782285
No longer depends on: 782285
Flags: sec-review?(dveditz)
Keywords: sec-review-needed
(Assignee)

Updated

7 years ago
Depends on: 784126
(Assignee)

Comment 35

7 years ago
[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: worker is run on ui thread which can slow things down, reader icon will be shown for more invalid pages, parsed article won't be available instantly
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: none
Attachment #653561 - Flags: approval-mozilla-aurora?
Attachment #653561 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Status: RESOLVED → VERIFIED
status-firefox16: fixed → verified
Depends on: 880913
Depends on: 958735

Updated

4 years ago
Depends on: 1144351
Flags: sec-review?(dveditz)
You need to log in before you can comment on or make changes to this bug.