229639 - Various invalid XBL documents due to extraneous text

Status: VERIFIED FIXED

(Core :: XUL, defect)

Description

[Bob Clary [:bc] (inactive)]

While attempting to validate XBL documents in the tree I ran across several
which contain #text in inappropriate locations. Rather than attach a patch for
each individual file, I am attaching a patch for all of them. The changes are
trivial and involve either surrounding the text in comments or removing
extraneous characters. I am not sure where is the appropriate place to file this
so I am filing it under XP Miscellany. Please reassign as appropriate.

Comment 1

[Bob Clary [:bc] (inactive)]

Attachment: patch v0

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 138106 [details] [diff] [review]
patch v0

sr=bzbarsky.  Damn the preprocessor shit that causes people to write invalid
XBL.

Comment 3

[Brendan Eich [:brendan]]

Don't damn the XUL preprocessor, save it -- or rather, someone who owns it
(who?) ought to make it barf on perl/sh-style comments that aren't nested in XML
or SGML (or whatever the right term is for <!-- ... -->) comments.

/be

Comment 4

[Hixie (not reading bugmail)]

Remove the <!-- --> additions. These files are not XBL, they are source files
that need to be preprocessed into XBL. If you want to check for validity of
these files, you need to run them throught the preprocessor first.

There's no reason for the preprocessor to barf on such cases. The whole point is
that those comments get removed before being shipped.

Regarding some of the other changes, maybe XBL should be changed to allow text
nodes anywhere as comments? Maybe not. The stray quotemark should definitely be
removed.

Comment 5

[Bob Clary [:bc] (inactive)]

This comment is just my 2 cents worth. Ignore as you please. It just seems that
since these are marked as XML they should be well formed. The text in the prolog
causes them to not be well-formed xml. It would be nice if these files aren't
intended to be well formed XML that they not be named as XML. Allowing text
elsewhere isn't that big a deal. In an ideal world though, it would be nice if
these were well formed XML and that the preprocessor syntax used XML instead of
C style preprocessor stuff.

Comment 6

[Brendan Eich [:brendan]]

Hixie, I don't want to gild the lilly here, but if these files are not XBL, do
we want their suffixes to be other than .xml?  Formal purity of bodily fluids
demands it, etc.

/be

Comment 7

[Bob Clary [:bc] (inactive)]

Attachment: patch v1 for xpfe/global/resources/content/bindings/editor.xml only

Comment 8

[Hixie (not reading bugmail)]

JS is presumably not "valid" JS when it has preprocessor directives in the
middle either, should we rename all our preprocessed JS files to something else
for the same reason? Seems like a lot of work for little gain to me. :-)

Anyway, that's not my problem, speak to bryner/ben/blake et all, they're the
main people who would be affected.

r=hixie on the editor only patch.

Comment 9

[Brendan Eich [:brendan]]

Actually, good makefile practice would use distinct suffixes for distinct
content types, so preprocessing could be done by common suffix-based inference
rules.

How do we run the preprocessor currently?  I mean, how do we decide what files
need it?

/be

Comment 10

[Bob Clary [:bc] (inactive)]

I guess with bz's sr on the v0 patch and hixie's r on the editor only v1 patch,
the v1 version could be checked in. I don't have cvs permission for the Mozilla
source tree, so someone else will have to do that.

I am just a newbie on this so take this for what it is worth, but I would like
to explain some of the potential benefits I see from using valid content where
possible. 

This bug started because of a crash in some XBL someone else wrote to allow
DocBook to be displayed using CSS/XBL. Looking through bugzilla I found Bug
223799 (the whack-a-mole XBL bug ;-).  The big issues there were: a) invalid XBL
caused crashes and b) no one knows what is valid XBL. Starting with Hixie's XBL
spec, I tried to create a relaxNG grammar that I and others could use to author
and test valid XBL. This bug (and some others) are a result of trying to apply
that grammar to the existing content in the tree. Using well-formed and
potentially valid XBL has the benefit of: making widely available tools
developed for standard content available for us to use, allowing existing errors
to be found easily, making sure new content is valid, educating XBL authors and
potentially stopping the "whack-a-mole" strategy of dealing with invalid XBL. 

This is a win for everyone even if preprocessed XBL is left in it's current
state. If preprocessed XBL had a different extension it would be easier to
separate real validity errors from those caused by unpreprocessed "XBL raw
source". It might be a bigger win if we could do this for all XBL. I intend to
improve the grammar for XBL and to develop one for XUL that would hopefully
provide some of the same benefits.  It is something to consider even if it is
not something that is practical at the moment.

Comment 11

[Hixie (not reading bugmail)]

It definitely would be cool if to-be-preprocessed XML files (XBL, XUL, XHTML)
had their own extension (say, .pre-xml) and as part of the build process we ran
them through the pre-processor *and checked them with a validator*. We should
probably do the same with our JS and CSS files, in fact.

That's probably another bug though.

Comment 12

[[e-mail]]

Comment on attachment 138106 [details] [diff] [review]
patch v0

I don't do fb reviews... r=me too on the editor.xml only changes though.

Comment 13

[Bob Clary [:bc] (inactive)]

Comment on attachment 138106 [details] [diff] [review]
patch v0

I will file a different bug on changing pre-processed files to a different
extension.

Comment 14

[Bob Clary [:bc] (inactive)]

Comment on attachment 138122 [details] [diff] [review]
patch v1 for xpfe/global/resources/content/bindings/editor.xml only

setting the review and superreview requests. I can't set them to + though.

Comment 15

[Bob Clary [:bc] (inactive)]

Filed Bug 229683 on the pre-processed content file extensions.

Comment 16

[Boris Zbarsky [:bzbarsky]]

For what it's worth, I think that not having validity checking of XBL/XUL/JS at
compile time (or worst-case as a Tinderbox test) is the single biggest waste of
my review time (in followup bugs, strict JS warning fixes), and up there for my
debugging time until I started carefully ignoring UI bugs.  So anything we can
do to facilitate such validity checking I would welcome with open arms.

We need this even more for preprocessed XBL/XUL/JS, since now it's possible to
write XBL/XUL/JS that's valid on some platforms but not others.  In fact, if it
were up to me I would have made having such tinderbox tests an absolute
requirement for using the preprocessor.  I guess that's why it's not up to me.  ;)

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 138122 [details] [diff] [review]
patch v1 for xpfe/global/resources/content/bindings/editor.xml only

sr=bzbarsky

Comment 18

[Stephen Donner [:stephend] Not actively reading bugmail]

C:\moz_src\mozilla\xpfe\global\resources\content\bindings>cvs commit -m "Bug 229
639.  Various invalid XBL documents due to extraneous text (editor.xml portion).
  Patch by Bob Clary <[e-mail]> r=neil, sr=bz" editor.xml
Checking in editor.xml;
/cvsroot/mozilla/xpfe/global/resources/content/bindings/editor.xml,v  <--  edito
r.xml
new revision: 1.3; previous revision: 1.2
done

I don't know if you need this bug open for other files, but I'm guessing so due
to the subject, so leaving open for Bob to resolve how he sees fit.

Comment 19

[Bob Clary [:bc] (inactive)]

Thanks everybody! 

Comment 20

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using LXR.