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)

x86
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla1.6alpha

People

(Reporter: darin.moz, Assigned: bzbarsky)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

xml pretty printing is broken [last worked in 10/10 build]
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.
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
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
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #133466 - Flags: superreview?(peterv)
Attachment #133466 - Flags: review?(bugmail)
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?
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?)
Yeah, pretty much (modulo a typo in the OnTransformDone notification that needs
fixing).  Patch coming up tonight.
Attachment #133466 - Attachment is obsolete: true
Attachment #133540 - Flags: superreview?(peterv)
Attachment #133540 - Flags: review?(bugmail)
Attachment #133466 - Flags: review?(bugmail)
*** Bug 222669 has been marked as a duplicate of this bug. ***
*** Bug 222727 has been marked as a duplicate of this bug. ***
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+
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.