595280 - Support HTML fragments from the AMO 1.5 API

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dave Townsend [:mossop]]

This looks ugly right now so we should make it pretty where we can.

Comment 1

[Ben Bucksch (:BenB)]

Options:
1. strip tags
2. downconvert to plaintext
3. try to render HTML (not recommended)

Re 2:
As said in bug 571547 comment 12:
There is a HTML to TXT converter in Gecko - it's called
nsPlainTextSerializer. It's used both for Thunderbird mail send as well as
copy&paste in Firefox. You could use it, but its use is not entirely trivial.
You can see its use in
http://mxr.mozilla.org/comm-central/source/mozilla/parser/htmlparser/tests/outsinks/Convert.cpp#111
(function HTML2text())
I don't know of any JS user in the tree, off-hand.

Comment 2

[Dave Townsend [:mossop]]

Attachment: patch rev 1

Fairly straightforward patch with tests. Only niggle is that the API data isn't straight HTML, it also includes linebreaks that really should be displayed as such so those get converted to <br> before the HTML conversion otherwise they are lost.

The converted data is stored in the database as opposed to the raw API data. This is by far simpler to do and if we find a problem with the conversion and update it it will only a take a day for the change to take effect anyway.

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 474176 [details] [diff] [review]
patch rev 1

>+function convertHTMLToTXT(html) {

Nit: convertHTMLToText - plz use full words kthx.

Comment 4

[Ben Bucksch (:BenB)]

FWIW, TXT refers to the "plaintext" format. HTML is "text", too.

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

convertHTMLToPlainText also works - I just don't like "TXT".

Comment 6

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/mozilla-central/rev/fd5ff14bc45a

Comment 7

[(no longer active)]

I backed this out with Mossop's blessing because of orange on Windows:

http://hg.mozilla.org/mozilla-central/rev/029d7a594ce7

Sample log:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1284410917.1284413686.15584.gz

Comment 8

[Dave Townsend [:mossop]]

Attachment: follow-up patch

This is a patch to be combined with the previously reviewed patch to fix the bustage. It normalizes the generated plain text which on windows used \r\n for newlines, also a small check that some text was there to be converted in the first place.

Comment 9

[Dave Townsend [:mossop]]

Attachment: follow-up patch for realz

Try again

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 475298 [details] [diff] [review]
follow-up patch for realz

Stupid Windows newlines...

Comment 11

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/mozilla-central/rev/3836139042c7

Comment 12

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

This breaks AddonRepository on Android:

WARN addons.manager: Exception calling callback:
Cc['@mozilla.org/widget/htmlformatconverter;1'] is undefined

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #12)
> This breaks AddonRepository on Android:
> 
> WARN addons.manager: Exception calling callback:
> Cc['@mozilla.org/widget/htmlformatconverter;1'] is undefined

Do we wanna handle that in a follow-up bug?

Also will we be able now to fix bug 571547?

Comment 14

[Dave Townsend [:mossop]]

(In reply to comment #13)
> (In reply to comment #12)
> > This breaks AddonRepository on Android:
> > 
> > WARN addons.manager: Exception calling callback:
> > Cc['@mozilla.org/widget/htmlformatconverter;1'] is undefined
> 
> Do we wanna handle that in a follow-up bug?

It was fixed in bug 596983

> Also will we be able now to fix bug 571547?

That bug has gotten messed up and I'm not sure what its purpose is now, it is probably redundant.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #14)
> > Also will we be able now to fix bug 571547?
> 
> That bug has gotten messed up and I'm not sure what its purpose is now, it is
> probably redundant.

We still don't show the add-on description correctly in the details pane. The HTML code is still visible. Shouldn't that be fixed by this implementation?

Comment 16

[Dave Townsend [:mossop]]

(In reply to comment #15)
> (In reply to comment #14)
> > > Also will we be able now to fix bug 571547?
> > 
> > That bug has gotten messed up and I'm not sure what its purpose is now, it is
> > probably redundant.
> 
> We still don't show the add-on description correctly in the details pane. The
> HTML code is still visible. Shouldn't that be fixed by this implementation?

For extensions and themes yes, unless they specify it in their install.rdf.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This looks good so far with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b8pre) Gecko/20101029 Firefox/4.0b8pre

Would there be a way to detect links and make them clickable? As it looks like we do not support a homepage for Personas. So some of them, i.e. Firefoxcup putting the url into the description itself.