Closed
Bug 368758
Opened 18 years ago
Closed 13 years ago
Remove obsolete workaround pref editor.quotesPreformatted
Categories
(Core :: DOM: Editor, defect)
Core
DOM: Editor
Tracking
()
RESOLVED
FIXED
mozilla13
People
(Reporter: mcow, Assigned: aceman)
References
()
Details
Attachments
(2 files, 2 obsolete files)
(deleted),
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
aceman
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 1•18 years ago
|
||
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
Updated•16 years ago
|
Product: Core → MailNews Core
Is this still valid? I am not sure I see the usage of the pref in those functions.
Reporter | ||
Comment 3•13 years ago
|
||
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?
Comment 5•13 years ago
|
||
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?
Reporter | ||
Comment 9•13 years ago
|
||
(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.
Assignee | ||
Comment 10•13 years ago
|
||
Thanks for your help. Who can review this?
Attachment #602410 -
Flags: review?(ehsan)
Attachment #602410 -
Flags: feedback?(mcow)
Assignee | ||
Comment 11•13 years ago
|
||
Attachment #602411 -
Flags: review?(bugmail)
Status: NEW → ASSIGNED
Summary: Remove obsolete workaround pref quotesPreformatted → Remove obsolete workaround pref editor.quotesPreformatted
Comment 12•13 years ago
|
||
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.
Assignee | ||
Comment 13•13 years ago
|
||
Assign(LS_LITERAL_STRING())?
Comment 14•13 years ago
|
||
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)
Comment 15•13 years ago
|
||
(In reply to :aceman from comment #13)
> Assign(LS_LITERAL_STRING())?
No, just use NS_LITERAL_STRING("span") instead of the tag variable.
Assignee | ||
Comment 16•13 years ago
|
||
(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.
Comment 17•13 years ago
|
||
(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!
Assignee | ||
Comment 18•13 years ago
|
||
Done.
(There is a similar unneeded pattern in nsHTMLEditor::InsertAsCitedQuotation).
Attachment #602410 -
Attachment is obsolete: true
Attachment #602438 -
Flags: review?(ehsan)
Updated•13 years ago
|
Attachment #602438 -
Flags: review?(ehsan) → review+
Assignee | ||
Comment 19•13 years ago
|
||
Thanks.
I'll look around that file in a new bug.
Keywords: checkin-needed
Whiteboard: [checkin the Core patch, leave open for TB]
Comment 20•13 years ago
|
||
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)
Comment 21•13 years ago
|
||
Keywords: checkin-needed
Reporter | ||
Comment 22•13 years ago
|
||
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"?
Assignee | ||
Comment 23•13 years ago
|
||
Thanks, I will do that in the followup bug I promised in comment 19.
Assignee | ||
Comment 24•13 years ago
|
||
I'll try newNode as that is InsertAsCitedQuotation uses.
Comment 25•13 years ago
|
||
Target Milestone: --- → mozilla13
Comment 26•13 years ago
|
||
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+
Assignee | ||
Comment 27•13 years ago
|
||
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]
Comment 28•13 years ago
|
||
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]
Assignee | ||
Comment 29•13 years ago
|
||
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.
Assignee | ||
Comment 30•13 years ago
|
||
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 31•13 years ago
|
||
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?
Assignee | ||
Comment 32•13 years ago
|
||
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.
Description
•