Closed Bug 1462481 Opened 7 years ago Closed 7 years ago

HTML Sanitizer class cleanup

Categories

(MailNews Core :: MIME, enhancement)

enhancement
Not set
normal

Tracking

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

RESOLVED FIXED
Thunderbird 62.0
Tracking Status
thunderbird_esr52 --- fixed
thunderbird_esr60 --- fixed
thunderbird60 --- fixed
thunderbird61 --- wontfix
thunderbird62 --- fixed

People

(Reporter: BenB, Assigned: BenB)

References

Details

Attachments

(1 file, 3 obsolete files)

Attached patch efail-sanitizer-cleanup-1.diff (obsolete) — Splinter Review
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+
Attached patch efail-sanitizer-cleanup-2.diff (obsolete) — Splinter 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.
Attached patch efail-sanitizer-cleanup-3.diff (obsolete) — Splinter Review
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: 7 years ago
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.

Attachment

General

Created:
Updated:
Size: