Thunderbird has suffered from a few bugs and their regressions (bug 116083 and bug 1151873) and is not waiting for bug 1174452 to get fixed properly. In bug 1125956 a Thunderbird-specific hack was implemented. There is with a follow-up to watch this hack, bug 1141786. Since there is already conditional compilation in nsDocumentEncoder.cpp added by Ehsan in bug 1125956, I suggest to use a fixed version of the conditional compilation which delivers more useful results. In the proposed patch we copy elements with style "pre-wrap" as plain text. I also made sure, that bug 1125956 does not regress.
Created attachment 8673302 [details] [diff] [review] Proposed solution. NOT CORRECT. Sorry, this is messy. Ehsan is against conditional compilation, but since he added some himself in bug 1125956, we might as well make it useful.
relative to bug 1193153 comment 25, my opinion copy/paste regressions simply shouldn't be permitted to stand. Unfortunately Thunderbird is much more impacted than Firefox by copy/paste/format issues in general, and frustratingly so.
Note to the reviewer: The proposed patch re-establishes the (ugly) pre-bug 116083 state, but only for Thunderbird. Please refer to the changeset landed for 116083: https://hg.mozilla.org/mozilla-central/rev/d5ef728a519d Further changes in that area: https://hg.mozilla.org/mozilla-central/rev/1748c99efd92 (bug 1125956, conditional compilation) https://hg.mozilla.org/mozilla-central/rev/47d62ded4e5f (bug 1151873, complete removal of "pre*" treatment)
One more ;-) - https://hg.mozilla.org/mozilla-central/rev/fa443367d637
Robert, thanks for approving this. It's quite ugly and I hope we can kick it out soon when we rework the serializers. ==== Dear Sheriff, in this patch some code it changed that is only conditionally compiled for Thunderbird. That's the #ifdef MOZ_THUNDERBIRD. So save server resources, please land this together with a bunch of other stuff, since it won't have any impact on testing results. Also, there is absolutely no rush, so please wait until you've got a suitable set of patches.
This patch busted c-c (nsRefPtr no longer exists).
Dear Sheriff, please back this out: https://hg.mozilla.org/mozilla-central/rev/b3d28572da60
Created attachment 8684369 [details] [diff] [review] Same patch, only nsRefPtr renamed to RefPtr. NOT CORRECT (but landed). Can you please land this. Note: No testing required, the change is in an #ifdef MOZ_THUNDERBIRD #endif block.
Note: nsRefPtr was renamed to RefPtr in bug 1207245 while this patch was waiting for review. Since the code is not compiled for Firefox, no one noticed until it got landed and busted Thunderbird.
I'm so sorry, the code I added for Thunderbird was not correct. It made bug 1125956 regress. I thought it didn't, but I hadn't tested it hard enough. We need indeed to return to the original code can can be seen here: https://hg.mozilla.org/mozilla-central/rev/d5ef728a519d I'll attach a patch that restores this code.
Created attachment 8687676 [details] [diff] [review] Restoring nsDocumentEncoder.cpp to its true original condition.
(In reply to Jorg K (GMT+1) from comment #14) > I'm so sorry, the code I added for Thunderbird was not correct. It made bug > 1125956 regress. I thought it didn't, but I hadn't tested it hard enough. Are you saying this needs better tests? ;)
Created attachment 8687694 [details] [diff] [review] Restoring nsDocumentEncoder.cpp to its true original condition (take 2). I took another look at this and decided that this needs a BIG comment. Ehsan dislikes bug numbers in the code, but I think here they are warranted. They can be removed when the bugs get fixed.
Ehsan, would you be able to rubber stamp this? Looks like Roc is not getting to it and you're very familiar with the issue. I've just returned the code to it's initial state, see https://hg.mozilla.org/mozilla-central/rev/d5ef728a519d and added a *BIG* comment.
Comment on attachment 8687694 [details] [diff] [review] Restoring nsDocumentEncoder.cpp to its true original condition (take 2). Review of attachment 8687694 [details] [diff] [review]: ----------------------------------------------------------------- Just for the record, this is clearly the wrong thing to do. The next time that someone touches the encoder code to fix anything related to this, this change _will_ break in unexpected ways. But if you still want to do this, please fix the comment below. ::: dom/base/nsDocumentEncoder.cpp @@ +1470,5 @@ > + // bug 1141786. > + // For case 2: > + // Wait for Firefox to get fixed to detect pre-formatting correctly, > + // bug 1174452. > + nsCOMPtr<nsIDOMElement> bodyElem = do_QueryInterface(selContent); Please don't use nsIDOMElement. Use IsHTMLElement() to make sure you're dealing with a body element and GetAttr() to get the style attribute.
2 years ago
Created attachment 8692592 [details] [diff] [review] Restoring nsDocumentEncoder.cpp to its true original condition (take 2, v2). Thank you very much for the quick review. I am really sorry about this, but Thunderbird users have suffered a lot from modifications of these 10 lines of code. It is the only way to fix it for them until bug 1174452 and bug 1141786 get fixed. This could be a while since Core::Serializers is unowned and Firefox users apparently don't do so much copying. I agree that this is the wrong way to fix it, however, this will carry a huge comment, so hopefully the next person along will notice. I will of course always be there to assist with Thunderbird integration in this area. Allow me one more comment: As you know, Firefox and Thunderbird were both born out of Netscape Navigator. When I was looking at serialiser code recently to fix the CJK problem, I was made aware of that once again. Mail was very much a part of the system, just try: grep -i mail dom/base/nsIDocumentEncoder.idl | wc -l grep -i mail dom/base/nsPlainTextSerializer.cpp | wc -l The most hilarious one I've found was: Preferences::GetBool("mail.compose.wrap_to_window_width", ...). Much/some of this code only exists for the purpose of serialising DOM to e-mail.
Comment on attachment 8692592 [details] [diff] [review] Restoring nsDocumentEncoder.cpp to its true original condition (take 2, v2). Review of attachment 8692592 [details] [diff] [review]: ----------------------------------------------------------------- rs=me
Dear Sheriff, This needs no testing since only code inside a #ifdef MOZ_THUNDERBIRD #endif block has changed. This does not affect Firefox and running the test suite on this is unnecessary. The change can be committed straight to mozilla-central. If, however, you don't trust me, I suggest you bundle this up with some other patches for landing, for example my minor change in bug 1225864. Thanking you in advance. P.S.: And thanks, Ehsan!
Is there any interest in trying to get this fixed in Thunderbird 38? If so, we would want to get it into a beta first.
This is not the place to discuss the inclusion in TB. We have bug 1193153 for that. Actually, in bug 1193153 comment #35 I suggested to get this uplifted into TB 38 more than two months(!!) ago, but there was no interest. Now it's in TB 45 (beta), so I think we can forget it for TB 38.
Pushed for Thunderbird 38.8 http://hg.mozilla.org/releases/mozilla-esr38/rev/8cc8fa60f0f6 http://hg.mozilla.org/releases/mozilla-esr38/rev/a0390f2afafd
Builds failed with these patches on esr38, not that important so backing them out (THUNDERBIRD_38_VERBRANCH) https://hg.mozilla.org/releases/mozilla-esr38/rev/78e117e69589 https://hg.mozilla.org/releases/mozilla-esr38/rev/ccb59fd997af
Yes, this would fail on this line: RefPtr<nsStyleContext> styleContext = Did the compiler tell you that? In the olden days, that used to be nsRefPtr. It would have been a small fix to get this to work.
38.8 will have very limited distribution, so I don't think it is worth the effort to try to get it to work.
Sure, but why did you start in the first place ;-)