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

RESOLVED FIXED in mozilla1.6alpha

Status

()

P1
normal
RESOLVED FIXED
15 years ago
15 years ago

People

(Reporter: neil, Assigned: bzbarsky)

Tracking

({regression})

Trunk
mozilla1.6alpha
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

15 years ago
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.
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).
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.
Yeah, the change in attribute order was recent, and there was a good reason for
it. See bug 213347.
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?
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
Until we add XHTML-as-XML functionality to Composer...
> > 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.
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
Attachment #132308 - Flags: superreview?(jst)
Attachment #132308 - Flags: review?(bugmail)
Taking.
Assignee: harishd → bzbarsky
Severity: trivial → normal
OS: Windows 2000 → All
Priority: -- → P1
Hardware: PC → All
Summary: Composer reverses order of attributes when reading files/source → [FIX]Composer reverses order of attributes when reading files/source
Target Milestone: --- → mozilla1.6alpha

Comment 12

15 years ago
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 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 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.
(Reporter)

Comment 15

15 years ago
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.
No I didn't read it until after I posted.  Wasn't that obvious?
I can point to bug 213347 in the comment, I guess... this bug will be pointed to
in the CVS commit comment.
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.
Flags: blocking1.5?
Comment on attachment 132308 [details] [diff] [review]
Patch to that effect

sr=jst
Attachment #132308 - Flags: superreview?(jst)
Attachment #132308 - Flags: superreview+
Attachment #132308 - Flags: review?(bugmail)
Attachment #132308 - Flags: review+
Comment on attachment 132308 [details] [diff] [review]
Patch to that effect

hey! don't move my circles
Attachment #132308 - Flags: review?(bugmail) → review+
Fix checked in, with the comment change requested by brade.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
Flags: blocking1.5?
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....
Attachment #132308 - Flags: approval1.5?

Comment 23

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

1.5 shipped. removing obsolete request.
Attachment #132308 - Flags: approval1.5?
You need to log in before you can comment on or make changes to this bug.