222539 - [FIX]xml pretty printing is broken [last worked in 10/10 build]

Status: RESOLVED FIXED

(Core :: XML, defect, P1)

Description

[Darin Fisher]

xml pretty printing is broken [last worked in 10/10 build]

Comment 1

[Darin Fisher]

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.

Comment 2

[Darin Fisher]

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 :)

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch

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.

Comment 4

[Peter Van der Beken [:peterv]]

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?

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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?

Comment 6

[Boris Zbarsky [:bzbarsky]]

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?

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 8

[Boris Zbarsky [:bzbarsky]]

Yeah, pretty much (modulo a typo in the OnTransformDone notification that needs
fixing).  Patch coming up tonight.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch to that effect

Comment 10

[Boris Zbarsky [:bzbarsky]]

*** Bug 222669 has been marked as a duplicate of this bug. ***

Comment 11

[Boris Zbarsky [:bzbarsky]]

*** Bug 222727 has been marked as a duplicate of this bug. ***

Comment 12

[Peter Van der Beken [:peterv]]

Comment on attachment 133540 [details] [diff] [review]
Patch to that effect

This seems better, thanks.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 133540 [details] [diff] [review]
Patch to that effect

thanks!

Comment 14

[Boris Zbarsky [:bzbarsky]]

Checked in.