Closed
Bug 1462481
Opened 7 years ago
Closed 7 years ago
HTML Sanitizer class cleanup
Categories
(MailNews Core :: MIME, enhancement)
Tracking
(thunderbird_esr52 fixed, thunderbird_esr60 fixed, thunderbird60 fixed, thunderbird61 wontfix, thunderbird62 fixed)
RESOLVED
FIXED
Thunderbird 62.0
People
(Reporter: BenB, Assigned: BenB)
References
Details
Attachments
(1 file, 3 obsolete files)
14.66 KB,
patch
|
jorgk-bmo
:
review+
jorgk-bmo
:
approval-comm-beta+
jorgk-bmo
:
approval-comm-esr60+
|
Details | Diff | 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.
Assignee | ||
Comment 1•7 years ago
|
||
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+
Assignee | ||
Comment 2•7 years ago
|
||
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.
Assignee | ||
Comment 3•7 years ago
|
||
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
Assignee | ||
Comment 4•7 years ago
|
||
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)
Assignee | ||
Updated•7 years ago
|
Status: NEW → ASSIGNED
Comment 5•7 years ago
|
||
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 6•7 years ago
|
||
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+
Comment 7•7 years ago
|
||
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
Comment 9•7 years ago
|
||
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
Updated•7 years ago
|
Attachment #8979053 -
Flags: approval-comm-esr60+
Attachment #8979053 -
Flags: approval-comm-beta+
Assignee | ||
Comment 10•7 years ago
|
||
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.
Comment 11•7 years ago
|
||
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
Comment 12•7 years ago
|
||
TB 52.8.1 ESR:
https://hg.mozilla.org/releases/comm-esr52/rev/cc471639dc646fc346579875a78196ee4e14ea0b
https://hg.mozilla.org/releases/comm-esr52/rev/fd71eff2a0046cf8488d03f7a0e4413201fbfb69
https://hg.mozilla.org/releases/comm-esr52/rev/4c8746e63c32a6389f501d052b6c2e63b2765192
status-thunderbird61:
--- → affected
status-thunderbird62:
--- → fixed
status-thunderbird_esr52:
--- → fixed
status-thunderbird_esr60:
--- → affected
Comment 13•7 years ago
|
||
TB 60 beta 7 (BETA_60_CONTINUATION branch):
https://hg.mozilla.org/releases/comm-beta/rev/1e39ffad84f5
https://hg.mozilla.org/releases/comm-beta/rev/3aa1b46747a2
https://hg.mozilla.org/releases/comm-beta/rev/82893c29e1cb
status-thunderbird60:
--- → fixed
Comment 14•7 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•