Last Comment Bug 368758 - Remove obsolete workaround pref editor.quotesPreformatted
: Remove obsolete workaround pref editor.quotesPreformatted
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: All All
: -- minor (vote)
: mozilla13
Assigned To: :aceman
:
Mentors:
http://mxr.mozilla.org/comm-central/s...
Depends on:
Blocks: 650787 732691 739912
  Show dependency treegraph
 
Reported: 2007-01-30 15:44 PST by Mike Cowperthwaite
Modified: 2012-03-28 01:57 PDT (History)
10 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch for core (12.91 KB, patch)
2012-03-02 10:51 PST, :aceman
ehsan: review+
Details | Diff | Splinter Review
patch for Thunderbird (2.99 KB, patch)
2012-03-02 10:52 PST, :aceman
mconley: review+
Details | Diff | Splinter Review
patch v2, for core (14.73 KB, patch)
2012-03-02 12:20 PST, :aceman
ehsan: review+
Details | Diff | Splinter Review
patch for Thunderbird, v2 (2.96 KB, patch)
2012-03-27 12:04 PDT, :aceman
acelists: review+
Details | Diff | Splinter Review

Description Mike Cowperthwaite 2007-01-30 15:44:12 PST
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 Ben Bucksch (:BenB) 2007-01-30 15:54:36 PST
Let's make the subject a bit more objective :)
"Cruftectomy: Eradicate quotesPreformatted" ->
"Remove obsolete workaround pref quotesPreformatted"
Comment 2 :aceman 2012-02-29 13:32:44 PST
Is this still valid? I am not sure I see the usage of the pref in those functions.
Comment 3 Mike Cowperthwaite 2012-02-29 20:51:02 PST
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.
Comment 4 :aceman 2012-03-01 02:10:24 PST
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 Ben Bucksch (:BenB) 2012-03-01 02:18:45 PST
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.
Comment 6 :aceman 2012-03-01 02:33:33 PST
Thanks. It seems the patch there does not yet touch nsPlainTextSerializer.cpp .
Comment 7 :aceman 2012-03-01 11:19:19 PST
It seems this actually belongs into Core (it is in /mozilla/*).
Comment 8 :aceman 2012-03-01 11:51:23 PST
(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?
Comment 9 Mike Cowperthwaite 2012-03-01 19:20:37 PST
(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.
Comment 10 :aceman 2012-03-02 10:51:09 PST
Created attachment 602410 [details] [diff] [review]
patch for core

Thanks for your help. Who can review this?
Comment 11 :aceman 2012-03-02 10:52:42 PST
Created attachment 602411 [details] [diff] [review]
patch for Thunderbird
Comment 12 neil@parkwaycc.co.uk 2012-03-02 11:43:38 PST
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.
Comment 13 :aceman 2012-03-02 11:49:16 PST
Assign(LS_LITERAL_STRING())?
Comment 14 :Ehsan Akhgari 2012-03-02 11:53:51 PST
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.  :-)
Comment 15 :Ehsan Akhgari 2012-03-02 11:54:32 PST
(In reply to :aceman from comment #13)
> Assign(LS_LITERAL_STRING())?

No, just use NS_LITERAL_STRING("span") instead of the tag variable.
Comment 16 :aceman 2012-03-02 12:01:48 PST
(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 :Ehsan Akhgari 2012-03-02 12:11:07 PST
(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!
Comment 18 :aceman 2012-03-02 12:20:18 PST
Created attachment 602438 [details] [diff] [review]
patch v2, for core

Done.

(There is a similar unneeded pattern in nsHTMLEditor::InsertAsCitedQuotation).
Comment 19 :aceman 2012-03-02 12:37:03 PST
Thanks.
I'll look around that file in a new bug.
Comment 20 Andrew Sutherland [:asuth] 2012-03-02 12:56:55 PST
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 :)
Comment 21 Ryan VanderMeulen [:RyanVM] 2012-03-02 14:24:02 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fa4f6cd52f5
Comment 22 Mike Cowperthwaite 2012-03-02 18:32:41 PST
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"?
Comment 23 :aceman 2012-03-03 04:42:05 PST
Thanks, I will do that in the followup bug I promised in comment 19.
Comment 24 :aceman 2012-03-03 07:45:22 PST
I'll try newNode as that is InsertAsCitedQuotation uses.
Comment 25 Ed Morley [:emorley] 2012-03-04 04:58:21 PST
https://hg.mozilla.org/mozilla-central/rev/7fa4f6cd52f5
Comment 26 Mike Conley (:mconley) - (Needinfo me!) 2012-03-27 11:26:16 PDT
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.
Comment 27 :aceman 2012-03-27 12:04:04 PDT
Created attachment 609825 [details] [diff] [review]
patch for Thunderbird, v2

Done. r=mconley.
Comment 28 Ryan VanderMeulen [:RyanVM] 2012-03-27 16:03:37 PDT
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.
Comment 29 :aceman 2012-03-28 00:07:58 PDT
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 30 :aceman 2012-03-28 00:16:32 PDT
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:
Comment 31 Mark Banner (:standard8) (afk until 26th July) 2012-03-28 01:45:12 PDT
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.
Comment 32 :aceman 2012-03-28 01:57:52 PDT
Yeah sorry, I missed it was mozilla-aurora, not comm-aurora. Seems the flags are offered depending on product.

Note You need to log in before you can comment on or make changes to this bug.