Closed
Bug 1214377
Opened 9 years ago
Closed 9 years ago
Restore nsDocumentEncoder.cpp to working order for Thunderbird while waiting for bug 1174452 to get fixed.
Categories
(Core :: DOM: Serializers, defect)
Core
DOM: Serializers
Tracking
()
RESOLVED
FIXED
mozilla45
People
(Reporter: jorgk-bmo, Assigned: jorgk-bmo)
Details
Attachments
(1 file, 4 obsolete files)
(deleted),
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
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•9 years ago
|
||
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)
Comment 2•9 years ago
|
||
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•9 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•9 years ago
|
||
One more ;-) - https://hg.mozilla.org/mozilla-central/rev/fa443367d637
Attachment #8673302 -
Flags: review?(roc) → review+
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → mozilla
Assignee | ||
Comment 5•9 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
Keywords: checkin-needed
Updated•9 years ago
|
status-thunderbird_esr38:
--- → affected
tracking-thunderbird_esr38:
--- → +
Comment 7•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Assignee | ||
Comment 9•9 years ago
|
||
Dear Sheriff, please back this out:
https://hg.mozilla.org/mozilla-central/rev/b3d28572da60
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 11•9 years ago
|
||
Can you please land this.
Note: No testing required, the change is in an
#ifdef MOZ_THUNDERBIRD
#endif block.
Flags: needinfo?(mozilla)
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 13•9 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•9 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 14•9 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•9 years ago
|
||
Attachment #8687676 -
Flags: review?(roc)
Assignee | ||
Updated•9 years ago
|
Attachment #8673302 -
Attachment description: Proposed solution. → Proposed solution. NOT CORRECT.
Assignee | ||
Updated•9 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•9 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•9 years ago
|
||
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•9 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 19•9 years ago
|
||
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-
Updated•9 years ago
|
Flags: needinfo?(ehsan)
Assignee | ||
Comment 20•9 years ago
|
||
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 21•9 years ago
|
||
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•9 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•9 years ago
|
||
Keywords: checkin-needed
Comment 24•9 years ago
|
||
Comment 25•9 years ago
|
||
bugherder |
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
Comment 26•9 years ago
|
||
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•9 years 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•9 years ago
|
Comment 28•9 years ago
|
||
Comment 29•9 years ago
|
||
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
Assignee | ||
Comment 30•9 years 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.
Comment 31•9 years ago
|
||
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•9 years 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.
Description
•