Closed Bug 368758 Opened 18 years ago Closed 13 years ago

Remove obsolete workaround pref editor.quotesPreformatted

Categories

(Core :: DOM: Editor, defect)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: mcow, Assigned: aceman)

References

()

Details

Attachments

(2 files, 2 obsolete files)

The pref editor.quotesPreformatted was introduced as a "temporary" hack for bug 69638. It's used in two places: * nsHTMLEditor::InsertAsPlaintextQuotation() if somehow set True, puts reply-quotes inside a <pre> rather than a <span> in the plain-text editor. No visible effect in the compose window, but requires an extra rule in HTML.CSS to colorize the pre[_moz_quote]. * nsPlainTextSerializer::Write() if True, may cause a bypass of the "no intelligent wrapping" section of the DOM-to-Text conversion of the plain-text editor's buffer into a message, thereby preventing quotes from being wrapped -- the same behavior seen when the pref is turned off and the quote is contained within a <span>. The pref is set to False by default; I seriously doubt anyone still uses this for anything. I gather it was desirable when there was some other, harder-to-deal-with problem in the editor's behavior within a <pre>, but that's obsolete now. This pref and the code supporting it is all cruft, and should be removed. xref bug 233705 -- the pref discussed at that bug also "bypasses the 'no intelligent wrapping'" DOM-to-text conversion.
Let's make the subject a bit more objective :) "Cruftectomy: Eradicate quotesPreformatted" -> "Remove obsolete workaround pref quotesPreformatted"
Summary: Cruftectomy: Eradicate quotesPreformatted → Remove obsolete workaround pref quotesPreformatted
Product: Core → MailNews Core
Is this still valid? I am not sure I see the usage of the pref in those functions.
You mean you don't see that it's used at all or you don't understand what it's doing? I don't really understand it, but my understanding was you could just remove the 'true' cases testing on that pref and nobody would notice. Besides the cases I listed when I opened this bug, MXR shows a third reference in nsHTMLDataTransfer.cpp, and a comment in nsComposeTxtSrvFilter.h which may clarify things a bit.
Yes, I couldn't see it at all. If you say it is still used in current code then I must look more closely :) I see it now, it is fetched in other function (e.g. nsPlainTextSerializer::Init) and passed in a variable. I do not understand it either. I can just do the work and remove it as you decide:) So I should just assume its value is FALSE and remove code that would run if it would be TRUE?
Just as a heads-up: nsPlainTextSerializer is currently undergoing a bigger change in bug 650787. Just a technical change, not changing the logic, but I'm mentioning it so that you can beware of source-level conflicts.
Thanks. It seems the patch there does not yet touch nsPlainTextSerializer.cpp .
Assignee: nobody → acelists
Blocks: 650787
It seems this actually belongs into Core (it is in /mozilla/*).
Component: Composition → Editor
Product: MailNews Core → Core
QA Contact: composition → editor
(In reply to Mike Cowperthwaite from comment #3) > Besides the cases I listed when I opened this bug, MXR shows a third > reference in nsHTMLDataTransfer.cpp, and a comment in > nsComposeTxtSrvFilter.h which may clarify things a bit. I am not sure what to do from that comment. Should mPreAtom be removed as there will be no more <pre> tags created in nsHTMLEditor::InsertAsPlaintextQuotation?
(In reply to :aceman from comment #8) > I am not sure what to do from that comment. Should mPreAtom be removed as > there will be no more <pre> tags created in > nsHTMLEditor::InsertAsPlaintextQuotation? Yes, I think that is the right thing to do.
Attached patch patch for core (obsolete) (deleted) — Splinter Review
Thanks for your help. Who can review this?
Attachment #602410 - Flags: review?(ehsan)
Attachment #602410 - Flags: feedback?(mcow)
Attached patch patch for Thunderbird (obsolete) (deleted) — Splinter Review
Attachment #602411 - Flags: review?(bugmail)
Status: NEW → ASSIGNED
Summary: Remove obsolete workaround pref quotesPreformatted → Remove obsolete workaround pref editor.quotesPreformatted
Comment on attachment 602410 [details] [diff] [review] patch for core Not intended to be anything resembling a real review. >+ || (((mSpanLevel > 0) || mDontWrapAnyQuotes) Nit: the ()s around mSpanLevel > 0 can probably go. > // Wrap the inserted quote in a <pre> so it won't be wrapped: s/pre/span/? > nsAutoString tag; >- if (quotesInPre) >- tag.AssignLiteral("pre"); >- else >- tag.AssignLiteral("span"); >+ tag.AssignLiteral("span"); > > rv = DeleteSelectionAndCreateNode(tag, getter_AddRefs(preNode)); This could probably use NS_LITERAL_STRING("span") anyway.
Assign(LS_LITERAL_STRING())?
Comment on attachment 602410 [details] [diff] [review] patch for core Review of attachment 602410 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, thanks! ::: editor/libeditor/html/nsHTMLDataTransfer.cpp @@ +1978,1 @@ > nsAutoEditBatch beginBatching(this); I'm assuming that you will be fixing the indentation in this function. :-)
Attachment #602410 - Flags: review?(ehsan)
Attachment #602410 - Flags: review+
Attachment #602410 - Flags: feedback?(mcow)
(In reply to :aceman from comment #13) > Assign(LS_LITERAL_STRING())? No, just use NS_LITERAL_STRING("span") instead of the tag variable.
(In reply to Ehsan Akhgari [:ehsan] from comment #14) > I'm assuming that you will be fixing the indentation in this function. :-) Yes, if the change to save one level of indentation is OK, I can do it.
(In reply to :aceman from comment #16) > (In reply to Ehsan Akhgari [:ehsan] from comment #14) > > I'm assuming that you will be fixing the indentation in this function. :-) > Yes, if the change to save one level of indentation is OK, I can do it. Yes, please do that. Thanks!
Attached patch patch v2, for core (deleted) — Splinter Review
Done. (There is a similar unneeded pattern in nsHTMLEditor::InsertAsCitedQuotation).
Attachment #602410 - Attachment is obsolete: true
Attachment #602438 - Flags: review?(ehsan)
Attachment #602438 - Flags: review?(ehsan) → review+
Thanks. I'll look around that file in a new bug.
Keywords: checkin-needed
Whiteboard: [checkin the Core patch, leave open for TB]
Comment on attachment 602411 [details] [diff] [review] patch for Thunderbird I'm not familiar with the composition process; deferring to blake because he has a tool that can figure out who the best reviewer is :)
Attachment #602411 - Flags: review?(bugmail) → review?(bwinton)
That core patch seems to do everything I expect, and a lot more updating to techniques that I have to assume are correct. I wonder if you want to change the name of 'preNode' since it's no longer a <pre>. Maybe "quoteNode"?
Thanks, I will do that in the followup bug I promised in comment 19.
Blocks: 732691
I'll try newNode as that is InsertAsCitedQuotation uses.
Comment on attachment 602411 [details] [diff] [review] patch for Thunderbird Review of attachment 602411 [details] [diff] [review]: ----------------------------------------------------------------- Besides the indentation nit I found, this looks good, and attachment reminders still seem to function properly. r=me with the indentation fix. ::: mail/components/compose/content/MsgComposeCommands.js @@ +1473,5 @@ > let bucket = document.getElementById("attachmentBucket"); > let warn = getPref("mail.compose.attachment_reminder"); > if (warn && !bucket.itemCount) { > + let keywordsInCsv = Services.prefs.getComplexValue( > + "mail.compose.attachment_reminder_keywords", The indentation of lines 1477 and 1478 should be two spaces in from the above line's indentation.
Attachment #602411 - Flags: review?(bwinton) → review+
Attached patch patch for Thunderbird, v2 (deleted) — Splinter Review
Done. r=mconley.
Attachment #602411 - Attachment is obsolete: true
Attachment #609825 - Flags: review+
Keywords: checkin-needed
Whiteboard: [checkin the Core patch, leave open for TB] → [checkin the Thunderbird patch]
http://hg.mozilla.org/comm-central/rev/4a2ef1b4bc27 Not sure if you want to request comm-aurora approval for this since the mozilla-central patch landed for Firefox 13.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [checkin the Thunderbird patch]
Good hint. The pref was removed from all.js in 13 but the patch removes getPref("editor.quotesPreformatted") from TB only in 14. I was told fetches of undefined prefs could throw an exception.
Comment on attachment 609825 [details] [diff] [review] patch for Thunderbird, v2 [Approval Request Comment] Regression caused by (bug #): User impact if declined: getPref("editor.quotesPreformatted") on non-existent pref could throw. Not sure if getPref() function catches exceptions. Testing completed (on m-c, etc.): Risk to taking this patch (and alternatives if risky): String changes made by this patch:
Attachment #609825 - Flags: approval-mozilla-aurora?
Comment on attachment 609825 [details] [diff] [review] patch for Thunderbird, v2 approval-mozilla-aurora is the wrong thing here. You generally want approval-comm-aurora for c-c patches, which is why I generally like to keep c-c parts in c-c specific bugs, as it helps tracking the changes which affect TB. Could you clone this bug to mailnews core/composition and attach the patch there, then I can approve it? Thanks.
Attachment #609825 - Flags: approval-mozilla-aurora?
Blocks: 739912
Yeah sorry, I missed it was mozilla-aurora, not comm-aurora. Seems the flags are offered depending on product.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: