HTML Sanitizer class cleanup

RESOLVED FIXED in Thunderbird 62.0

Status

enhancement
RESOLVED FIXED
Last year
Last year

People

(Reporter: BenB, Assigned: BenB)

Tracking

Thunderbird 62.0
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird_esr52 fixed, thunderbird_esr60 fixed, thunderbird60 fixed, thunderbird61 wontfix, thunderbird62 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

The attached patch is a fallout from my work in security bug 1419417. None of the changes in this patch are important for security, they are just cleaning up the code from old debug code etc.

Note:
The purpose of this ticket is only to get that patch in. The purpose is expressively *not* to make a general cleanup, or fix more than I already did there.

Please note that libmime has a different code style than the rest of Thunderbird, and I do not intent to change that in this in this bug. If you want wider style changes, they should happen in all of libmime at once. Like we sometimes have spellcheck or warning fix pr whitespace change patches across a whole module.

The purpose of this bug is only to get this patch in. It should be a massive improvement over status quo. We can make more changes later.
Comment on attachment 8976717 [details] [diff] [review]
efail-sanitizer-cleanup-1.diff

Carrying over positive review from Magnus from bug 1419417.
Attachment #8976717 - Flags: review+
This is against 52.

Will need update for trunk.
Should also remove the AllowStyles and pref change, as that depends on another mozilla-central change.
Remove changing the name and effect of the pref.
This removes the functional changes of the patch.

The patch mostly removes debug code and fixes a few comments.
Attachment #8976718 - Attachment is obsolete: true
Comment on attachment 8976725 [details] [diff] [review]
efail-sanitizer-cleanup-3.diff

To review, it's easiest to compare with patch v1, which was already r+.
Attachment #8976725 - Flags: review?(mkmelin+mozilla)
Status: NEW → ASSIGNED
Comment on attachment 8976725 [details] [diff] [review]
efail-sanitizer-cleanup-3.diff

Thanks again for the clean-up. It's a good idea to move this to this separate bug.

Please rebase the patch against trunk. Right now I get:
pplying efail-sanitizer-cleanup-3.diff
patching file mailnews/mime/src/mimemoz2.cpp
Hunk #1 FAILED at 2154
1 out of 1 hunks FAILED -- saving rejects to file mailnews/mime/src/mimemoz2.cpp.rej
patching file mailnews/mime/src/mimethsa.cpp
Hunk #1 FAILED at 1
1 out of 3 hunks FAILED -- saving rejects to file mailnews/mime/src/mimethsa.cpp.rej
patching file mailnews/mime/src/mimethsa.h
Hunk #1 FAILED at 0
1 out of 1 hunks FAILED -- saving rejects to file mailnews/mime/src/mimethsa.h.rej
patch failed, unable to continue (try -v)
Attachment #8976725 - Flags: review+
Comment on attachment 8976725 [details] [diff] [review]
efail-sanitizer-cleanup-3.diff

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

Looks good. Need it to apply to trunk though
Attachment #8976725 - Flags: review?(mkmelin+mozilla) → review+
Rebased to trunk and added commit message. Also removed superfluous parenthesis in |x = y ? z : t| assignment.
Attachment #8976717 - Attachment is obsolete: true
Attachment #8976725 - Attachment is obsolete: true
Attachment #8979053 - Flags: review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/36e3c7e4c9b9
clean up MIME's HTML sanitizer class. r=mkmelin,jorgk
https://hg.mozilla.org/comm-central/rev/9c9d0d5d6c82
fix white-space issues in mimethsa.cpp. rs=white-space-only
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
I took the liberty to fix white-space issues while we where here:
- No space between function names and opening parenthesis
- no space used for casts.

Some MIME files have already been cleaned up that way, for example the worst offender here:
https://hg.mozilla.org/comm-central/rev/5a8396d48e9e
Target Milestone: Thunderbird 61.0 → Thunderbird 62.0
Attachment #8979053 - Flags: approval-comm-esr60+
Attachment #8979053 - Flags: approval-comm-beta+
In bug 1419417 Comment 144, Jörg K wrote:

> I promise a very quick review in bug 1462895 and immediate landing if there are no test failures.

Given that our tests in many cases hardcode the exact output, and we're going to change the output here, this is pretty much guaranteed to "break" the tests, with a false positive.
https://hg.mozilla.org/comm-central/rev/1dd08f49b8dd52533738f6471621097c3638e5ab
Follow-up: fix "Context-Type", fix typo in comment and remove comment referencing future code. r=me
You need to log in before you can comment on or make changes to this bug.