779389 - Remove XPCOM-related code from Readability.js

Status: RESOLVED DUPLICATE of bug 779796

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

Description

[Brian Nicholson (:bnicholson)]

Attachment: Remove XPCOM-related code from Readability

The current plan is to move Readability into it's own Worker thread using a JS DOMParser. Worker threads cannot use any XPCOM-related functions, so we'll have to drop these.

Comment 1

[Lucas Rocha (:lucasr)]

Comment on attachment 647806 [details] [diff] [review]
Remove XPCOM-related code from Readability

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

Move the base uri parsing outside Readability and pass it as part of the uri argument. Let's try to avoid this custom URI parsing.

::: mobile/android/chrome/content/Readability.js
@@ +10,2 @@
>  
>  var Readability = function(uri, doc) {

The input uri is still an nsUri thing, no? Are you going to send a js object following the API instead?

@@ +104,5 @@
>    _fixRelativeUris: function(articleContent) {
> +    let spec = this._uri.spec;
> +    let schemeDelimiterIndex = spec.indexOf("://") + 3;
> +    let scheme = spec.substr(0, schemeDelimiterIndex);
> +    let baseUri = spec.substr(0, spec.lastIndexOf("/"));

I'd prefer to parse the base uri using the nsiUri API and pass it as a property of this._uri or something. Let's try to avoid custom URI parsing as much as possible and use proven code/APIs whenever we can.

@@ +107,5 @@
> +    let scheme = spec.substr(0, schemeDelimiterIndex);
> +    let baseUri = spec.substr(0, spec.lastIndexOf("/"));
> +    let prePathUri = spec.substr(0, spec.indexOf("/", schemeDelimiterIndex));
> +
> +    function toAbsoluteURI(uri) {

Does this cover things like about:home? Doesn't seem so. I'm a bit mixed about this custom parsing here but I can live with this version of toAbsoluteURI().

Comment 2

[Brian Nicholson (:bnicholson)]

(In reply to Lucas Rocha (:lucasr) from comment #1)
> The input uri is still an nsUri thing, no? Are you going to send a js object
> following the API instead?

I've been using JSON.stringify(uri) before sending the uri to the worker thread, which passes in all of the attributes listed here at https://developer.mozilla.org/en/NsIURI#Attributes. Unfortunately, none of those are exactly what we want (except for scheme - I should have used that in the patch).

For example, given "http://www.foo.com:8080/foo/bar/?q=123", we would want:
 * baseURI = "http://www.foo.com:8080/foo/bar/" - this would represent the current directory for standard relative links.
 * prePathURI - "http://www.foo.com:8080/" - this is similar to the nsIURI, except I think we also need to include the port. This is needed for relative links such as "/abc/".

Using regex expressions could be a bit cleaner than what I have in the patch, but I can't really think of a good way to get the baseURI we need using just the nsIURI API.

It's a bit hacky, but something that may work could be:
baseURI = Services.io.newURI("__replaceme__", null, uri).spec.replace("__replaceme__", "")

which we would send to the worker from browser.js.

> 
> @@ +107,5 @@
> > +    let scheme = spec.substr(0, schemeDelimiterIndex);
> > +    let baseUri = spec.substr(0, spec.lastIndexOf("/"));
> > +    let prePathUri = spec.substr(0, spec.indexOf("/", schemeDelimiterIndex));
> > +
> > +    function toAbsoluteURI(uri) {
> 
> Does this cover things like about:home? Doesn't seem so. I'm a bit mixed
> about this custom parsing here but I can live with this version of
> toAbsoluteURI().

Yeah, good point...I'll fix that. I'm not particularly excited about this change either, but we don't have too many options. If we really want to use the ioService, we could post messages between the worker thread and browser.js, but that might get a bit messy/slow.

Comment 3

[Brian Nicholson (:bnicholson)]

> this is similar to the nsIURI

That should have been: "this is similar to the prePath attribute in nsIURI".

Comment 4

[Brian Nicholson (:bnicholson)]

> baseURI = Services.io.newURI("__replaceme__", null, uri).spec.replace("__replaceme__", "")

Not sure what I was thinking last night, but this should be as simple as:

baseURI = Services.io.newURI(".", null, uri).spec

Also, I think it would be better to have this change in bug 779796. Bug 779796 requires it, and it doesn't make much sense as a standalone patch.