451065 - Replace invalid comments in suite's dtds with valid ones

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Rimas Kudelis [use needinfo to ping]]

Attachment: Patch (checked in)

This patch should make compare-locales happier. It replaces two invalid comments in messenger.dtd with valid ones.

Comment 1

[Karsten Düsterloh]

Comment on attachment 334307 [details] [diff] [review]
Patch (checked in)

>-<!--Tooltips-->
>+<!-- Tooltips -->

>-<!---SearchBar-->
>+<!-- SearchBar -->

I'm all for qualifying these comments as ugly, but they're valid XML as per <http://www.w3.org/TR/xml/#sec-comments>:

  [15] Comment ::= '<!--' ((Char - '-') | ('-' (Char - '-')))* '-->'

Let's make a deal, Rimas:

I'll r+ this as a deuglification, if you file a bug (and link it here) against compare-locales, because it's violating the XML spec. ;-)

Comment 2

[Rimas Kudelis [use needinfo to ping]]

I'm not good at reading that syntax. And I'm not sure what product/component to file a bug on either. CC'ing Pike, hopefully he'll read that when he's done with what he's doing now.

Comment 3

[Caio Tiago Oliveira (asrail)]

Karsten,

they are not valid per your quote.

It requires "<!--" + space + ...

"<!--x" or whatever non-space is wrong.

Also "<!-- text -- oooops -->" is not valid per those productions, since it won't generate any double dash.


There are other invalid xml comments around. Should I file a new bug or attach a new patch here?

Comment 4

[Karsten Düsterloh]

> It requires "<!--" + space + ...

Wrong.

  [2] Char ::= #x9 | #xA | #xD | [#x20-#xD7FF] | [#xE000-#xFFFD] 
                   | [#x10000-#x10FFFF]	
      /* any Unicode character, excluding the surrogate blocks,
         FFFE, and FFFF. */

Thus space is NOT a special character here and since there is no explicit space in [15], a comment "<!--x-->" is perfectly legal. 

Mind: The * in [15] means that you can leave out everything and still have a valid comment "<!---->"!

> Also "<!-- text -- oooops -->" is not valid per those productions, since it
> won't generate any double dash.

Correct.

> There are other invalid xml comments around. Should I file a new bug or 
> attach a new patch here?

If they're really invalid, you can put patches here, as long as it's open.

Comment 5

[Robert Kaiser]

(In reply to comment #1)
> I'll r+ this as a deuglification, if you file a bug (and link it here) against
> compare-locales, because it's violating the XML spec. ;-)

Filed as bug 452923 - does this patch need checkin?

Comment 6

[Caio Tiago Oliveira (asrail)]

(In reply to comment #5)
> (In reply to comment #1)
> > I'll r+ this as a deuglification, if you file a bug (and link it here) against
> > compare-locales, because it's violating the XML spec. ;-)
> 
> Filed as bug 452923 - does this patch need checkin?

Per comment #4, this patch won't be required if we fix bug 452923.
But I wouldn't mark this bug as invalid right now.

Comment 7

[Caio Tiago Oliveira (asrail)]

Fixing that bug will save us from a lot of "<!--LOCALIZATION" comment changes.

We actually won't require the patch here.

I have a WIP for bug 452923 that I should finish in a few hours.

Comment 8

[Robert Kaiser]

Comment on attachment 334307 [details] [diff] [review]
Patch (checked in)

I checked this in as beautification.

Comment 9

[Robert Kaiser]

I there anything still to fix here or is this resolved?

Comment 10

[Robert Kaiser]

Actually, re-reading this, it was about getting compare-locales to work and it does now, we are on dashboard even ;-)