Closed
Bug 222539
Opened 19 years ago
Closed 19 years ago
[FIX]xml pretty printing is broken [last worked in 10/10 build]
Categories
(Core :: XML, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla1.6alpha
People
(Reporter: darin.moz, Assigned: bzbarsky)
References
Details
(Keywords: regression)
Attachments
(1 file, 1 obsolete file)
2.68 KB,
patch
|
sicking
:
review+
peterv
:
superreview+
|
Details | Diff | Splinter Review |
xml pretty printing is broken [last worked in 10/10 build]
Reporter | ||
Comment 1•19 years ago
|
||
i should have added that pretty printing is broken in so far as nothing displays, not even the raw XML text. there aren't too many suspicious looking checkins between the 10th and 11th, and there don't appear to be any interesting assertions or warnings in a debug build. it just displays nothing without much complaint.
Reporter | ||
Comment 2•19 years ago
|
||
ok, this is a regression caused by the patch for bug 220930. i backed that patch out of my local tree, and pretty printing works again. over to bz with love :)
Assignee: bugmail → bzbarsky
![]() |
Assignee | |
Updated•19 years ago
|
Priority: -- → P1
Summary: xml pretty printing is broken [last worked in 10/10 build] → [FIX]xml pretty printing is broken [last worked in 10/10 build]
Target Milestone: --- → mozilla1.6alpha
![]() |
Assignee | |
Comment 3•19 years ago
|
||
The basic issue is that the XML prettyprinter drops the prettyprinting if the DOM is modified... or rather if it gets a document observer notification. So we have to notify before we call MaybePrettyPrint(). Since we now notify for mDocument even if we plan to XSLT it, I've changed the OnTransformDone code to only notify if aResultDocument != mDocument.
![]() |
Assignee | |
Updated•19 years ago
|
Attachment #133466 -
Flags: superreview?(peterv)
Attachment #133466 -
Flags: review?(bugmail)
Comment 4•19 years ago
|
||
Comment on attachment 133466 [details] [diff] [review] Patch Nothing gets confused by getting the two ContentInserted on different documents in the case of XSLT transforms?
Attachment #133466 -
Flags: superreview?(peterv) → superreview+
I'm not comfortable at all with having notifications happening on the source document for XSLT transforms, that seems errorprone since all observers must be aware that XSLT exists and how to deal with it, which i doubt they will do. Also, i have some vauge recolletion of that the XBL code couldn't really deal with the binding being added after the initial reflow occurs. When does the initial reflow occur, on the |ContentInserted| call or on the |StartLayout| call?
![]() |
Assignee | |
Comment 6•19 years ago
|
||
peterv, I can't think of anything that would be registered as the document observer for both of the documents involved.... sicking, the initial reflow occurs on the StartLayout() call. I would be quite happy to remove the notification in OnTransformDone() -- I only added this at peterv's request. The alternative is to move the MaybePrettyPrint() call into the "!mXSLTProcessor" case in DidBuildModel. That seems more correct to me in general, since MaybePrettyPrint should never prettyprint in the presence of an XSLT stylesheet, right? Then if !mXSLTProcessor we could notify and then MaybePrettyPrint, and if mXSLTProcessor we can notify for the resultdocument only in OnTransformDone. Thoughts? I'm not completely sure how some of this hooks together -- eg if XSLT transforms into unstyled XML, do we pretty-print?
> The alternative is to move the MaybePrettyPrint() call into the > "!mXSLTProcessor" case in DidBuildModel. That seems more correct to me in > general, since MaybePrettyPrint should never prettyprint in the presence of an > XSLT stylesheet, right? Yes, I like this much better. And yes, MaybePrettyPrint should be a noop when a XSLT stylesheet is available. > Then if !mXSLTProcessor we could notify and then MaybePrettyPrint, and if > mXSLTProcessor we can notify for the resultdocument only in OnTransformDone. Yep, that sounds like the way to go (and the way things would be if only the MaybePrettyPrint call was moved, right?)
![]() |
Assignee | |
Comment 8•19 years ago
|
||
Yeah, pretty much (modulo a typo in the OnTransformDone notification that needs fixing). Patch coming up tonight.
![]() |
Assignee | |
Comment 9•19 years ago
|
||
Attachment #133466 -
Attachment is obsolete: true
![]() |
Assignee | |
Updated•19 years ago
|
Attachment #133540 -
Flags: superreview?(peterv)
Attachment #133540 -
Flags: review?(bugmail)
![]() |
Assignee | |
Updated•19 years ago
|
Attachment #133466 -
Flags: review?(bugmail)
![]() |
Assignee | |
Comment 10•19 years ago
|
||
*** Bug 222669 has been marked as a duplicate of this bug. ***
![]() |
Assignee | |
Comment 11•19 years ago
|
||
*** Bug 222727 has been marked as a duplicate of this bug. ***
Comment 12•19 years ago
|
||
Comment on attachment 133540 [details] [diff] [review] Patch to that effect This seems better, thanks.
Attachment #133540 -
Flags: superreview?(peterv) → superreview+
Comment on attachment 133540 [details] [diff] [review] Patch to that effect thanks!
Attachment #133540 -
Flags: review?(bugmail) → review+
![]() |
Assignee | |
Comment 14•19 years ago
|
||
Checked in.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•