698935 - Error in errStrayStartTag key of dom/chrome/layout/htmlparser.properties

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[RickieES aka Ricardo Palomares]

Attachment: End vs. Start switch

errStrayStartTag key of htmlparser.properties is:

Stray end tag “%1$S”.

instead of:

Stray start tag “%1$S”.

So, "end" -> "start".

Patch provided.

Comment 1

[Henri Sivonen (:hsivonen)]

Comment on attachment 571178 [details] [diff] [review]
End vs. Start switch

Good catch. Thanks.

Comment 2

[Henri Sivonen (:hsivonen)]

Ricardo, do you have access to land this patch yourself? If not, is it OK to use your bugzilla email in the patch author field?

Comment 3

[RickieES aka Ricardo Palomares]

(In reply to Henri Sivonen (:hsivonen) from comment #2)
> Ricardo, do you have access to land this patch yourself? If not, is it OK to
> use your bugzilla email in the patch author field?


No, I don't have access, so I've set the checkin-needed keyword. Of course, it is OK to use my e-mail, so everybody can blame me if the Universe crash down after landing the patch. :-)

Comment 4

[Henri Sivonen (:hsivonen)]

Thanks for the fix. Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/df8fb0f7eb07

Comment 5

[Henri Sivonen (:hsivonen)]

Attachment: Rename the key

It has been suggested to me that the string key should be renamed even though the erroneous message had been for less than 24 hours on m-c. Axel, should the key be renamed? Here's a patch for renaming the key in case it needs to be renamed.

Comment 6

[Axel Hecht [:Pike]]

Comment on attachment 571320 [details] [diff] [review]
Rename the key

Hrm. errNoElementToCloseButEndTagSeen is also wrong, having %1$S in there twice, I guess we want to fix that, too.

I'm unsure, does the suffix Bis carry any meaning?

Comment 7

[Henri Sivonen (:hsivonen)]

(In reply to Axel Hecht [:Pike] from comment #6)
> Comment on attachment 571320 [details] [diff] [review] [diff] [details] [review]
> Rename the key
> 
> Hrm. errNoElementToCloseButEndTagSeen is also wrong, having %1$S in there
> twice, I guess we want to fix that, too.

It's intentional that the message contains the same tag name twice.

> I'm unsure, does the suffix Bis carry any meaning?

Not beyond the first meaning listed at http://en.wikipedia.org/wiki/Bis

I didn't want to use time to find out if digits are allowed in keys, so I didn't use errStartStrayTag2.

Comment 8

[Axel Hecht [:Pike]]

Comment on attachment 571320 [details] [diff] [review]
Rename the key

Review of attachment 571320 [details] [diff] [review]:
-----------------------------------------------------------------

Our printf code suprises me often, is there a test that verifies that the multiple %1$S works? Just haven't seen that done before, so I'm cautious.

Re the Bis, just use 2 as suffix, that is allowed and folks are used to it.

Comment 9

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/df8fb0f7eb07
leaving open for the l10n fix

Comment 10

[Axel Hecht [:Pike]]

I don't think that landing patches in the middle of the solution is helpful, tbh.

Now you're exposing the right string the wrong way.

Comment 11

[Henri Sivonen (:hsivonen)]

Attachment: Rename the key, v2

Comment 12

[Axel Hecht [:Pike]]

Comment on attachment 571918 [details] [diff] [review]
Rename the key, v2

Review of attachment 571918 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, thanks.

Comment 13

[Henri Sivonen (:hsivonen)]

Thanks.
https://hg.mozilla.org/integration/mozilla-inbound/rev/936c243bf7fc

Comment 14

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/936c243bf7fc