Support HTML fragments from the AMO 1.5 API

VERIFIED FIXED in mozilla2.0b7

Status

()

defect
VERIFIED FIXED
9 years ago
9 years ago

People

(Reporter: mossop, Assigned: mossop)

Tracking

Trunk
mozilla2.0b7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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

Updated

9 years ago
blocking2.0: --- → betaN+
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.
(Assignee)

Comment 2

9 years ago
Posted patch patch rev 1Splinter Review
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.
Attachment #474176 - Flags: review?(bmcbride)
(Assignee)

Updated

9 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

9 years ago
Version: 1.9.2 Branch → Trunk
Comment on attachment 474176 [details] [diff] [review]
patch rev 1

>+function convertHTMLToTXT(html) {

Nit: convertHTMLToText - plz use full words kthx.
Attachment #474176 - Flags: review?(bmcbride) → review+
FWIW, TXT refers to the "plaintext" format. HTML is "text", too.
convertHTMLToPlainText also works - I just don't like "TXT".
(Assignee)

Comment 6

9 years ago
Landed: http://hg.mozilla.org/mozilla-central/rev/fd5ff14bc45a
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b6

Comment 7

9 years ago
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 8

9 years ago
Posted patch follow-up patch (obsolete) — Splinter Review
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.
Attachment #475289 - Flags: review?(bmcbride)
(Assignee)

Comment 9

9 years ago
Try again
Attachment #475289 - Attachment is obsolete: true
Attachment #475298 - Flags: review?(bmcbride)
Attachment #475289 - Flags: review?(bmcbride)
Comment on attachment 475298 [details] [diff] [review]
follow-up patch for realz

Stupid Windows newlines...
Attachment #475298 - Flags: review?(bmcbride) → review+
(Assignee)

Comment 11

9 years ago
Landed: http://hg.mozilla.org/mozilla-central/rev/3836139042c7
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
This breaks AddonRepository on Android:

WARN addons.manager: Exception calling callback:
Cc['@mozilla.org/widget/htmlformatconverter;1'] is undefined
Blocks: 596983
(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?
Blocks: 571547
(Assignee)

Comment 14

9 years ago
(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.
(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?
(Assignee)

Comment 16

9 years ago
(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.
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.