1214377 - Restore nsDocumentEncoder.cpp to working order for Thunderbird while waiting for bug 1174452 to get fixed.

Status: RESOLVED FIXED

(Core :: DOM: Serializers, defect)

Description

[Jorg K (CEST = GMT+2)]

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.

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: 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.

Comment 2

[Wayne Mery (:wsmwk)]

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.

Comment 3

[Jorg K (CEST = GMT+2)]

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)

Comment 4

[Jorg K (CEST = GMT+2)]

One more ;-) - https://hg.mozilla.org/mozilla-central/rev/fa443367d637

Comment 5

[Jorg K (CEST = GMT+2)]

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.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b3d28572da60

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b3d28572da60

Comment 8

[aleth [:aleth]]

This patch busted c-c (nsRefPtr no longer exists).

Comment 9

[Jorg K (CEST = GMT+2)]

Dear Sheriff, please back this out:
https://hg.mozilla.org/mozilla-central/rev/b3d28572da60

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out https://hg.mozilla.org/mozilla-central/rev/7ec0d2929561

Comment 11

[Jorg K (CEST = GMT+2)]

Attachment: 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.

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/42627d5369b3

Comment 13

[Jorg K (CEST = GMT+2)]

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.

Comment 14

[Jorg K (CEST = GMT+2)]

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.

Comment 15

[Jorg K (CEST = GMT+2)]

Attachment: Restoring nsDocumentEncoder.cpp to its true original condition.

Comment 16

[aleth [:aleth]]

(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? ;)

Comment 17

[Jorg K (CEST = GMT+2)]

Attachment: 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.

Comment 18

[Jorg K (CEST = GMT+2)]

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 19

[(no longer active)]

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.

Comment 20

[Jorg K (CEST = GMT+2)]

Attachment: 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 21

[(no longer active)]

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

Comment 22

[Jorg K (CEST = GMT+2)]

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!

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e925caf848fa

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/abc01b1f82ba (backout)
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9bc8f46ac3e

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c9bc8f46ac3e

Comment 26

[Kent James (:rkent)]

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.

Comment 27

[Jorg K (CEST = GMT+2)]

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.

Comment 28

[Kent James (:rkent)]

Pushed for Thunderbird 38.8

http://hg.mozilla.org/releases/mozilla-esr38/rev/8cc8fa60f0f6
http://hg.mozilla.org/releases/mozilla-esr38/rev/a0390f2afafd

Comment 29

[Kent James (:rkent)]

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

Comment 30

[Jorg K (CEST = GMT+2)]

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.

Comment 31

[Kent James (:rkent)]

38.8 will have very limited distribution, so I don't think it is worth the effort to try to get it to work.

Comment 32

[Jorg K (CEST = GMT+2)]

Sure, but why did you start in the first place ;-)