Remove obsolete workaround pref editor.quotesPreformatted

RESOLVED FIXED in mozilla13

Status

()

Core
Editor
--
minor
RESOLVED FIXED
11 years ago
5 years ago

People

(Reporter: Mike Cowperthwaite, Assigned: aceman)

Tracking

Trunk
mozilla13
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

11 years ago
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

11 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
Product: Core → MailNews Core
(Assignee)

Comment 2

5 years ago
Is this still valid? I am not sure I see the usage of the pref in those functions.
(Reporter)

Comment 3

5 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.
(Assignee)

Comment 4

5 years ago
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

5 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.
(Assignee)

Comment 6

5 years ago
Thanks. It seems the patch there does not yet touch nsPlainTextSerializer.cpp .
Assignee: nobody → acelists
Blocks: 650787
(Assignee)

Comment 7

5 years ago
It seems this actually belongs into Core (it is in /mozilla/*).
Component: Composition → Editor
Product: MailNews Core → Core
QA Contact: composition → editor
(Assignee)

Comment 8

5 years ago
(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

5 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

5 years ago
Created attachment 602410 [details] [diff] [review]
patch for core

Thanks for your help. Who can review this?
Attachment #602410 - Flags: review?(ehsan)
Attachment #602410 - Flags: feedback?(mcow)
(Assignee)

Comment 11

5 years ago
Created attachment 602411 [details] [diff] [review]
patch for Thunderbird
Attachment #602411 - Flags: review?(bugmail)
(Assignee)

Updated

5 years ago
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.
(Assignee)

Comment 13

5 years ago
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.
(Assignee)

Comment 16

5 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.
(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

5 years ago
Created attachment 602438 [details] [diff] [review]
patch v2, for core

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+
(Assignee)

Comment 19

5 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 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)
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fa4f6cd52f5
Keywords: checkin-needed
(Reporter)

Comment 22

5 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

5 years ago
Thanks, I will do that in the followup bug I promised in comment 19.
(Assignee)

Updated

5 years ago
Blocks: 732691
(Assignee)

Comment 24

5 years ago
I'll try newNode as that is InsertAsCitedQuotation uses.
https://hg.mozilla.org/mozilla-central/rev/7fa4f6cd52f5
Target Milestone: --- → mozilla13
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

5 years ago
Created attachment 609825 [details] [diff] [review]
patch for Thunderbird, v2

Done. r=mconley.
Attachment #602411 - Attachment is obsolete: true
Attachment #609825 - Flags: review+
(Assignee)

Updated

5 years ago
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
Last Resolved: 5 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [checkin the Thunderbird patch]
(Assignee)

Comment 29

5 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

5 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 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)

Updated

5 years ago
Blocks: 739912
(Assignee)

Comment 32

5 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.