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

RESOLVED FIXED in Firefox 45

Status

()

Core
Serializers
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: Jorg K (GMT+2), Assigned: Jorg K (GMT+2))

Tracking

Trunk
mozilla45
Points:
---

Firefox Tracking Flags

(firefox44 affected, firefox45 fixed, thunderbird_esr38 affected)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

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

Comment 1

2 years ago
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.
Attachment #8673302 - Flags: review?(roc)
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.
(Assignee)

Comment 3

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

Comment 4

2 years ago
One more ;-) - https://hg.mozilla.org/mozilla-central/rev/fa443367d637
Attachment #8673302 - Flags: review?(roc) → review+
(Assignee)

Updated

2 years ago
Assignee: nobody → mozilla
(Assignee)

Comment 5

2 years ago
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.
Keywords: checkin-needed

Comment 6

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3d28572da60
Keywords: checkin-needed

Updated

2 years ago
status-thunderbird_esr38: --- → affected
tracking-thunderbird_esr38: --- → +
https://hg.mozilla.org/mozilla-central/rev/b3d28572da60
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45

Comment 8

2 years ago
This patch busted c-c (nsRefPtr no longer exists).
Flags: needinfo?(mozilla)
(Assignee)

Comment 9

2 years ago
Dear Sheriff, please back this out:
https://hg.mozilla.org/mozilla-central/rev/b3d28572da60
Backed out https://hg.mozilla.org/mozilla-central/rev/7ec0d2929561
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 11

2 years ago
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.
Flags: needinfo?(mozilla)
https://hg.mozilla.org/mozilla-central/rev/42627d5369b3
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
(Assignee)

Comment 13

2 years ago
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.
Keywords: checkin-needed

Updated

2 years ago
Keywords: checkin-needed
(Assignee)

Comment 14

2 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 15

2 years ago
Created attachment 8687676 [details] [diff] [review]
Restoring nsDocumentEncoder.cpp to its true original condition.
Attachment #8687676 - Flags: review?(roc)
(Assignee)

Updated

2 years ago
Attachment #8673302 - Attachment description: Proposed solution. → Proposed solution. NOT CORRECT.
(Assignee)

Updated

2 years ago
Attachment #8684369 - Attachment description: Same patch, only nsRefPtr renamed to RefPtr → Same patch, only nsRefPtr renamed to RefPtr. NOT CORRECT (but landed).

Comment 16

2 years ago
(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? ;)
(Assignee)

Comment 17

2 years ago
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.
Attachment #8687676 - Attachment is obsolete: true
Attachment #8687676 - Flags: review?(roc)
Attachment #8687694 - Flags: review?(roc)
(Assignee)

Comment 18

2 years ago
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.
Flags: needinfo?(ehsan)
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.
Attachment #8687694 - Flags: review?(roc) → review-
Flags: needinfo?(ehsan)
(Assignee)

Comment 20

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.
Attachment #8673302 - Attachment is obsolete: true
Attachment #8684369 - Attachment is obsolete: true
Attachment #8687694 - Attachment is obsolete: true
Attachment #8692592 - Flags: review?(ehsan)
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
Attachment #8692592 - Flags: review?(ehsan) → review+
(Assignee)

Comment 22

2 years ago
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!
Keywords: checkin-needed

Comment 23

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e925caf848fa
Keywords: checkin-needed

Comment 24

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

Comment 25

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c9bc8f46ac3e
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED

Updated

2 years ago
Depends on: 1229518

Updated

2 years ago
No longer depends on: 1229518
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.
(Assignee)

Comment 27

a year ago
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.

Updated

a year ago
tracking-thunderbird_esr38: + → -
Pushed for Thunderbird 38.8

http://hg.mozilla.org/releases/mozilla-esr38/rev/8cc8fa60f0f6
http://hg.mozilla.org/releases/mozilla-esr38/rev/a0390f2afafd
status-thunderbird_esr38: affected → fixed
tracking-thunderbird_esr38: - → 44+
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
status-thunderbird_esr38: fixed → affected
tracking-thunderbird_esr38: 44+ → ---
(Assignee)

Comment 30

a year ago
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.
(Assignee)

Comment 32

a year ago
Sure, but why did you start in the first place ;-)
You need to log in before you can comment on or make changes to this bug.