218919 - [FIX]Composer reverses order of attributes when reading files/source

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect, P1)

Description

[[e-mail]]

When creating new objects in a document their attributes serialize in the order
that they are set in Composer. But bz(?) decided to reverse the order that the
parset sets attributes which means that the attributes serialize in reverse
order. Apparently some spiders expect meta attributes in a certain order. Also
switching all the attributes around having made a "minor" change looks odd.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Two possible fixes here:

1)  Composer serializes in the opposite order
2)  We make SetAttr prepend to the linked list instead of appending.

sicking, how goes your work on clobbering all the attribute code anyway?  Should
we wait on that?

In general, relying on the order of attributes on a content node is dangerous,
since attribute order is explicitly undefined and we may end up wanting to do
some optimizations in the way we store attributes that change the order (eg
storing more-often-accessed attrs first in the list).

Comment 2

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

no, don't wait on me. I agree that reversing the order in composer is a bad
thing and we should try to fix that (but I must admit I have a hard time caring
about spiders that are so broken that they care about the order of attributes,
unless it's a very important spider like google or some such).

is the reversal of attributes during parsin a recent change? Was there a good
reason for it, like improved speed? If so then I think the best fix would be to
make the serializer output the attributes in reversed order.

Comment 3

[Johnny Stenback (:jst)]

Yeah, the change in attribute order was recent, and there was a good reason for
it. See bug 213347.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Again, it may be possible to simply have SetAttr prepend instead of appending. 
As far as I can tell, for HTML this would involve reversing the indexing on
mAttrNames in nsHTMLAttributes (so that index 0 for GetAttrNameAt means "last
attr in the array").  What about the non-HTML content classes, though?

Comment 5

[Johnny Stenback (:jst)]

Or we could just make the serializer walk backwards over the list of attributes
too. That may be the faster way to solve this...

Comment 6

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

Yeah, I like that way better

Comment 7

[Boris Zbarsky [:bzbarsky]]

Until we add XHTML-as-XML functionality to Composer...

Comment 8

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

> > Or we could just make the serializer walk backwards over the list of
> > attributes too. That may be the faster way to solve this...
>
> Yeah, I like that way better

Me too.

Comment 9

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

What i suggest is that the HTML-serializer takes attributes in reversed order
and the XML serializer does not. That'll take care of the problem even when
composer does XML

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch to that effect

Comment 11

[Boris Zbarsky [:bzbarsky]]

Taking.

Comment 12

[Kathleen :Brade]

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

I'm ok with this patch but I might suggest that the comment also include this
bug # or the bug # that reversed the attributes in the first place (or both).
r=brade

Comment 13

[Christopher Aillon (sabbatical, not receiving bugmail)]

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

>+  for (index = count; index > 0; ) {
>+    --index;

What about
    for (index = count; index-- > 0; ) {
or
    for (index = count; --index >= 0; ) {

I think I prefer the former (post-increment) version rather than the
pre-increment one...

Comment 14

[Christopher Aillon (sabbatical, not receiving bugmail)]

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

Also I think |count| can be removed now in favor of just a direct assignment of
GetAttrCount() into |index| in the for-loop's initialize statement.

Comment 15

[[e-mail]]

caillon wrote:
> for (index = count; --index >= 0; )
Did you not read this comment on the previous line?
// index is unsigned, hence index >= 0 is always true.

I like your idea of (for index = aContent->GetAttrCount(); index-- > 0; )
although purists will object to decrementing index when it is zero.

Comment 16

[Christopher Aillon (sabbatical, not receiving bugmail)]

No I didn't read it until after I posted.  Wasn't that obvious?

Comment 17

[Boris Zbarsky [:bzbarsky]]

I can point to bug 213347 in the comment, I guess... this bug will be pointed to
in the CVS commit comment.

Comment 18

[Boris Zbarsky [:bzbarsky]]

This bug is present on the 1.5 branch, btw....  The patch above won't apply to
branch, of course, but the branch version is pretty much the same, modulo the
nsIContent api change.

Comment 19

[Johnny Stenback (:jst)]

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

sr=jst

Comment 20

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

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

hey! don't move my circles

Comment 21

[Boris Zbarsky [:bzbarsky]]

Fix checked in, with the comment change requested by brade.

Comment 22

[Boris Zbarsky [:bzbarsky]]

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

Is there still time for this to make 1.5?  This is a very very low-risk patch,
and fixes a rather serious editor regression....

Comment 23

[Asa Dotzler [:asa]]

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

1.5 shipped. removing obsolete request.