327930 - events retrieved by calICalendar.getItems() have a wrong creation/modification date [setting readonly properties creationDate, lastModifiedTime]

Status: RESOLVED FIXED

(Calendar :: Provider: Local Storage, defect)

Description

[snotling]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; fr-FR; rv:1.8) Gecko/20060110 Debian/1.5.dfsg-4 Firefox/1.5
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8) Gecko/20060217 Mozilla Sunbird/0.3a1+

All the events I get from a getItems() search have a creation and modification date/time equal to the current(!) date/time.

In my case, the calendar was from the storage provider. The creation/modif date fetched from the SQLite DB is correct. However, the SQL timestamp fed throught the CalDateTime setter function "nativeTime" isn't correctly transformed apparently (as done is calStorageCalendar.js, function newDateTime). Thus I guess the bug must come from the CalDateTime class.

Reproducible: Always

Steps to Reproduce:
My code :

var emptyListener = {
  onOperationComplete: function(aCalendar, aStatus, aOperationType, aId, aDetail) {
    debug("emptyListener::onOperationComplete()");
   },
   onGetResult: function(aCalendar, aStatus, aItemType, aDetail, aCount, aItems) {
     debug("emptyListener::onGetResult()");
       for each ( calEvent in aItems ) {
         debug("Event '" + calEvent.title + "' created  " + calEvent.creationDate + ", modified " + calEvent.lastModifiedTime);
       }
     }
   }
};

// aCalendar is a "storage" calendar

var filter = aCalendar.ITEM_FILTER_COMPLETED_ALL | aCalendar.ITEM_FILTER_TYPE_EVENT;
aCalendar.getItems(filter, 0, null, null, emptyListener);

Actual Results:  
OUTPUT, from code executed at 2006/02/20 17:04:58 UTC :

Event 'my event 1' created  2006/02/20 17:04:58 UTC, modified 2006/02/20 17:04:58 UTC
Event 'another event' created  2006/02/20 17:04:58 UTC, modified 2006/02/20 17:04:58 UTC
Event 'hello world' created  2006/02/20 17:04:58 UTC, modified 2006/02/20 17:04:58 UTC
Event 'ding dong' created  2006/02/20 17:04:58 UTC, modified 2006/02/20 17:04:58 UTC

Expected Results:  
The creation and modification time of the events should be correct, identical to the SQL timestamp.

Comment 1

[Joey Minta]

Confirmed.  This might be a reasonable bug to try to implement the dyanmic storage lookup version of calIItemBase/calIEvent/calITodo.

Comment 2

[snotling]

In file calStorageCalendar.js, function getItemBaseFromRow() :

 if (row.time_created) 
   item.creationDate = newDateTime(row.time_created, "UTC"); 
 if (row.last_modified)
   item.lastModifiedTime = newDateTime(row.last_modified, "UTC");

According to the calItemBase IDL, creationDate and lastModifiedTime are read-only. Is this an issue ?
Furthermore, for each attribute of the item which is then filled (id, title, ...), the lastModifiedTime is automatically updated, thus overwritten. So, at the end, the lastModifiedTime is just plain wrong :/

What are the solutions ?

Comment 3

[gekacheka]

These errors cause lots of annoying warnings when using Venkman.


Warning ``item.creationDate is read-only'' [xs] in file ``file:///C:/moz/sunbird/components/calStorageCalendar.js'', line 1228, character 0.

Warning ``item.lastModifiedTime is read-only'' [xs] in file ``file:///C:/moz/sunbird/components/calStorageCalendar.js'', line 1230, character 0.

Comment 4

[Reed Loden [:reed]]

The bugspam monkeys have been set free and are feeding on Calendar :: Provider: Local Storage. Be afraid for your sanity!

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

I don't think this is dataloss that anyone is going to see just yet.  Which is to say, it only happens in the storage calendar, so we won't be corrupting any existing non-Mozilla calendars, since we don't use the storage calendar as a cache yet.  These attributes aren't exposed through any UI, and the only things that might really care about them would be sync code, which isn't written yet.  This means that there's not really any way for the user to be conscious that these are wrong, other than conceivably causing trouble for extension authors.

Comment 7

[Norbert Püschel]

Attachment: Part 1 of fix for modification time bug

This is a necessary change to allow the fix for the modification time bug in calStorageCalendar.js. Without this patch it is impossible to setProperty("LAST-MODIFIED",...) because the call to this.modify() would set mDirty which would in turn cause the modification time to be reset on next access.

Comment 8

[Norbert Püschel]

Attachment: Part 2 of fix for modification time bug

1) Changed write accesses to readonly item attributes to use setProperty instead.
2) Filling in of item data in getItem(s) would cause LAST-MODIFIED to be reset by generic code in calItemBase.js. Now will do a setProperty("LAST-MODIFIED",...) at the end of the getItem-Operation to repair the damage.

Comment 9

[Norbert Püschel]

Well, I'm writing synchronisation code for MyPhoneExplorer right now, and therefore need this bug fixed ASAP. As far as I can imagine there is no alternative to make meaningful synchronisation possible at all. So PLEASE, PLEASE apply this patch - if you get it into 0.3.1 this would be great.

Comment 10

[Matthew (lilmatt) Willis]

Norbert:
This is too significant a patch for us to include in 0.3.1, especially this close to release.  However, we'll definitely consider it for 0.5 (which we hope to release fairly soon as well).  Please ask for first-review on your patches, to keep them moving towards inclusion.  I suggest [e-mail] for r1.

Comment 11

[cmtalbert]

Comment on attachment 254525 [details] [diff] [review]
Part 1 of fix for modification time bug

Was asked by email to review these patches, I am adding them to my own review queue.

Comment 12

[cmtalbert]

(In reply to comment #8)
Norbert, it is our practice to create one patch file for all changes unless the patch is extremely large. You can do this from your mozilla/ directory with the command: cvs diff -u8pN calendar > mypatch.diff

This will make a more readable, unified patch.

I can do a first review on these as they are.  But, before second review, you will need to create the above style patch.  

Comment 13

[cmtalbert]

Attachment: Unified diff of these changes (still original patch content)

This is a unified diff that I will use for my r1, so requesting r1 from myself.

Comment 14

[cmtalbert]

Comment on attachment 256461 [details] [diff] [review]
Unified diff of these changes (still original patch content)

Norbert: Thanks for the patch.  You have good ideas, and we need to be able to set these attributes rationally.  However, I think we want a more universal solution to this problem.

Note that I created a universal diff for the patch -- you'll need to do this for your next patch version.  So...comments inline.

>     setProperty: function (aName, aValue) {
>-        this.modify();
>+        if(aName == "LAST-MODIFIED") {
>+            this.mDirty = false;
>+        } else {
>+            this.modify();
>+        }
We usually always include the braces, even on one line if's and they are formatted this way.

I really think that the proper solution to this would be to have an accessor  method for setLastModified time on the calIItemBase interface (defined in the IDL).  We have one for setting the stampTime, and I can't see any reason why LastModified should be any different than the stamp time.  I think that the updateLastModified accessor method can handle this logic with the mDirty attribute and ensure that the proper last modified time is written to the item appropriately.  Restructuring this logic is the reason I have r-'d this patch.  We need to make the code more sane, and having setProperty do x for all attributes and do y for Last_Modified is just not a good way to achieve code sanity.

>Index: calendar/providers/storage/calStorageCalendar.js
>===================================================================
>         if (row.event_end)
>             item.endDate = newDateTime(row.event_end, row.event_end_tz);
>-        if (row.event_stamp)
>-            item.stampTime = newDateTime(row.event_stamp, "UTC");
>         if ((row.flags & CAL_ITEM_FLAG_EVENT_ALLDAY) != 0) {
>             item.startDate.isDate = true;
>             item.endDate.isDate = true;
>         }
> 
>+        if (row.event_stamp)
>+            item.setProperty("DTSTAMP",newDateTime(row.event_stamp, "UTC"));
>+
>+        if (row.last_modified)
>+            item.setProperty("LAST-MODIFIED",newDateTime(row.last_modified, "UTC"));
>+

Honestly, here the item.stampTime = *should* throw an error since this is modifying a readonly attribute on the IDL. We should use item.updateStampTime() for this.  Likewise, I would like to see a similar call: item.updateLastModified instead of the item.setProperty method you are using here.
 
>     getAdditionalDataForItem: function (item, flags) {
>+	var save_last_modified = item.lastModifiedTime;
>+
No tabs ^^ in source code please. Please use spaces.


Please resubmit a patch containing this new method on calIItemBase.idl and use that method in this code. Also, please use the updateStampTime method instead of setting the DTSTAMP property directly. When you create a new patch please run: "cvs diff -u8pN calendar > mypatch.diff" from the mozilla directory to create a unified patch.  I am looking forward to the next review. Thanks!

Comment 15

[Norbert Püschel]

(In reply to comment #14)
> (From update of attachment 256461 [details] [diff] [review])
> Norbert: Thanks for the patch.  You have good ideas, and we need to be able to
> set these attributes rationally.  However, I think we want a more universal
> solution to this problem.
> 

Universal solutions are always good... ;-)

> Note that I created a universal diff for the patch -- you'll need to do this
> for your next patch version.  So...comments inline.
> 
> >     setProperty: function (aName, aValue) {
> >-        this.modify();
> >+        if(aName == "LAST-MODIFIED") {
> >+            this.mDirty = false;
> >+        } else {
> >+            this.modify();
> >+        }
> We usually always include the braces, even on one line if's and they are
> formatted this way.
> 

Thank you, will do that.

> I really think that the proper solution to this would be to have an accessor 
> method for setLastModified time on the calIItemBase interface (defined in the
> IDL).  We have one for setting the stampTime, and I can't see any reason why
> LastModified should be any different than the stamp time.  I think that the
> updateLastModified accessor method can handle this logic with the mDirty
> attribute and ensure that the proper last modified time is written to the item
> appropriately.  Restructuring this logic is the reason I have r-'d this patch. 
> We need to make the code more sane, and having setProperty do x for all
> attributes and do y for Last_Modified is just not a good way to achieve code
> sanity.

Well, being a newbie and all, I respectfully but heartily disagree with you.

1) setProperty at the moment DOES NOT WORK AT ALL for lastModifiedTime - the internal call to modify() will overwrite the value stored by setProperty before the next access, making it completely impossible to set lastModifiedTime at all. So, my patch to setProperty just makes this function behave more consistent, not less.

2) (The following arguments assume the the IDL for calItemBase as seen on:
http://lxr.mozilla.org/mozilla/source/calendar/base/public/calIItemBase.idl
is still valid. If there is a newer version, I don't know how to find it.)

Well, there is no accessor function for stampTime. updateStampTime() does not allow to set an arbitrary time, it sets the current time as stampTime! As I understand the IDL of calItemBase, the setProperty-Function IS the universal acessor function for all attributes. Changing that would mean making all attributes readwrite instead of readonly. That doesn't make sense.

The problem is, there is currently no way for calendar providers to "load items from behind the scene" apart from converting iCal-Strings to items, which the storage calendar provider doesn't do.

A real universal approch to this would be a privileged access to item innards by calendar providers. I'm not deep enough into calendar & javascript to make a suggestion how to do that.

> 
> >Index: calendar/providers/storage/calStorageCalendar.js
> >===================================================================
> >         if (row.event_end)
> >             item.endDate = newDateTime(row.event_end, row.event_end_tz);
> >-        if (row.event_stamp)
> >-            item.stampTime = newDateTime(row.event_stamp, "UTC");
> >         if ((row.flags & CAL_ITEM_FLAG_EVENT_ALLDAY) != 0) {
> >             item.startDate.isDate = true;
> >             item.endDate.isDate = true;
> >         }
> > 
> >+        if (row.event_stamp)
> >+            item.setProperty("DTSTAMP",newDateTime(row.event_stamp, "UTC"));
> >+
> >+        if (row.last_modified)
> >+            item.setProperty("LAST-MODIFIED",newDateTime(row.last_modified, "UTC"));
> >+
> 
> Honestly, here the item.stampTime = *should* throw an error since this is
> modifying a readonly attribute on the IDL. We should use item.updateStampTime()
> for this.  Likewise, I would like to see a similar call:
> item.updateLastModified instead of the item.setProperty method you are using
> here.

Read what I wrote above. calStorageCalender.js is calendar provider code and MUST access item attributes that should not be accessed by others. updateStampTime() is NO solution, the point is to fill the item with the values that are stored in the database, which can be achieved only by setProperty.

> 
> >     getAdditionalDataForItem: function (item, flags) {
> >+	var save_last_modified = item.lastModifiedTime;
> >+
> No tabs ^^ in source code please. Please use spaces.
> 
> 
> Please resubmit a patch containing this new method on calIItemBase.idl and use
> that method in this code. Also, please use the updateStampTime method instead
> of setting the DTSTAMP property directly. When you create a new patch please
> run: "cvs diff -u8pN calendar > mypatch.diff" from the mozilla directory to
> create a unified patch.  I am looking forward to the next review. Thanks!
> 

Sorry, I haven't yet installed cvs yet; I was doing my code surgery on the living object. ;-) (I will install cvs as soon as possible.)

However, I will not make a new patch at the moment (apart from the cosmetic issues you mentioned). Instead, please reconsider your verdict on my patch; I respectfully claim you haven't reviewed thoroughly enough. ;-)

Please, I don't see a major IDL redesign going in for 0.5. My fix is the only way to do it without that. Without that fix, lastModifiedTime is unusable, which makes syncing against storage calendars rather inconvenient.

Comment 16

[cmtalbert]

(In reply to comment #15) 
> Well, being a newbie and all, I respectfully but heartily disagree with you.
> 
Disagreements are always welcome, no worries.

> 1) setProperty at the moment DOES NOT WORK AT ALL for lastModifiedTime - the
> internal call to modify() will overwrite the value stored by setProperty before
> the next access, making it completely impossible to set lastModifiedTime at
> all. So, my patch to setProperty just makes this function behave more
> consistent, not less.
> 
I realized that, but I think I had the wrong mindset about it...keep reading...

> 2) (The following arguments assume the the IDL for calItemBase as seen on:
> http://lxr.mozilla.org/mozilla/source/calendar/base/public/calIItemBase.idl
> is still valid. If there is a newer version, I don't know how to find it.)
> 
> Well, there is no accessor function for stampTime. updateStampTime() does not
> allow to set an arbitrary time, it sets the current time as stampTime! As I
> understand the IDL of calItemBase, the setProperty-Function IS the universal
> acessor function for all attributes. Changing that would mean making all
> attributes readwrite instead of readonly. That doesn't make sense.
> 
You're absolutely right about this.  And it was an oversight on my part about updateStamptime.

> The problem is, there is currently no way for calendar providers to "load items
> from behind the scene" apart from converting iCal-Strings to items, which the
> storage calendar provider doesn't do.
> 
> A real universal approch to this would be a privileged access to item innards
> by calendar providers. I'm not deep enough into calendar & javascript to make a
> suggestion how to do that.
>
I have conferred with one of our provider layer developers and he concurred with you that setProperty is the way to do this "inner" access that the providers require.  I was mistaken, and wasn't thinking about this when I reviewed the patch. Thanks for correcting me. 

I think the proper way to move forward here is to submit a unified patch of your original changes (be sure to address the nit-picky style notes I commented on), and ask for R1 one more time.  This time, please request R1 from one of our provider developers: [e-mail].  I'm sorry for botching your review.

We're looking forward to your next patch.

Comment 17

[Norbert Püschel]

Attachment: New version of the LAST_MODIFIED bug patch

This is the new version of the patch, hopefully conforming to coding standards.

Comment 18

[Norbert Püschel]

So, now I created a new version of the patch correcting the last_modified problem in the storage provider. I'm now asking for a new review.

Thanks,
  Norbert

Comment 19

[Matthew (lilmatt) Willis]

Norbert:
For future reference, to ask for review, in the details associated with your attachment, set the first-review flag to "?" and put the reviewer's email address in the textbox ([e-mail] in this case)

Comment 20

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 256919 [details] [diff] [review]
New version of the LAST_MODIFIED bug patch

>+++ calendar/base/src/calItemBase.js	1 Mar 2007 16:50:01 -0000
>     setProperty: function (aName, aValue) {
>-        this.modify();
>+        if (aName == "LAST-MODIFIED") {
>+            this.mDirty = false;
>+        }
>+        else {
>+            this.modify();
>+        }
Please use |else| as follows:
    if (aName == "LAST-MODIFIED") {
        this.mDirty = false;
    } else {
        this.modify();
    }


>+++ calendar/providers/storage/calStorageCalendar.js	1 Mar 2007 16:50:06 -0000

>@@ -1468,20 +1468,20 @@ calStorageCalendar.prototype = {
>     getItemBaseFromRow: function (row, flags, item) {
>-        if (row.time_created)
>-            item.creationDate = newDateTime(row.time_created, "UTC");
>-        if (row.last_modified)
>-            item.lastModifiedTime = newDateTime(row.last_modified, "UTC");
>+        if (row.time_created) {
>+          item.setProperty("CREATED",newDateTime(row.time_created, "UTC"));
>+        }
>+
This is one of the exceptions where its advisable to not use brackets for one-line if's. Since the many if's are similar, it is better to keep the existing formatting. Also, please remove your extra newlines.

>@@ -1543,23 +1543,29 @@ calStorageCalendar.prototype = {
[...]
>-        if (row.event_stamp)
>-            item.stampTime = newDateTime(row.event_stamp, "UTC");
>         if ((row.flags & CAL_ITEM_FLAG_EVENT_ALLDAY) != 0) {
>             item.startDate.isDate = true;
>             item.endDate.isDate = true;
>         }
> 
>+        if (row.event_stamp) {
>+            item.setProperty("DTSTAMP",newDateTime(row.event_stamp, "UTC"));
>+        }
>+
>+        if (row.last_modified) {
>+            item.setProperty("LAST-MODIFIED",newDateTime(row.last_modified, "UTC"));
>+        }
>+

>@@ -1567,27 +1573,33 @@ calStorageCalendar.prototype = {
[...]
>+        if (row.last_modified) {
>+            item.setProperty("LAST-MODIFIED",newDateTime(row.last_modified, "UTC"));
>+        }
>+

Why are you setting the last_modified time in both getEventFromRow() and getTodoFromRow()? The modification time is a property of the base item, so this should be set in getItemBaseFromRow() where you also removed the similar set of lines. Note also that getItemBaseFromRow() is called in both of the other two functions, so this would save a couple lines of code but still provide the same functionality.


>@@ -1567,27 +1573,33 @@ calStorageCalendar.prototype = {
[...]
>     getAdditionalDataForItem: function (item, flags) {
>+        var save_last_modified = item.lastModifiedTime;
>+
Please add a small comment to why you are doing this in the source. I do realize why, but adding this to the source will make the code more readable.


>@@ -1734,16 +1746,18 @@ calStorageCalendar.prototype = {
[...]
>+
>+        item.setProperty("LAST-MODIFIED",save_last_modified);

Minor style nit: please use spaces between function arguments. i.e:     
         item.setProperty("LAST-MODIFIED", save_last_modified);

With those issues fixed,
r=phil

When you have a new patch, set first-review + directly and request second-review from [e-mail] (mvl is the storage provider module owner).

Comment 21

[Norbert Püschel]

OK, I will do the reformatting tomorrow, I have been away for a few days, sorry for the delay.


> >@@ -1543,23 +1543,29 @@ calStorageCalendar.prototype = {
> [...]
> >-        if (row.event_stamp)
> >-            item.stampTime = newDateTime(row.event_stamp, "UTC");
> >         if ((row.flags & CAL_ITEM_FLAG_EVENT_ALLDAY) != 0) {
> >             item.startDate.isDate = true;
> >             item.endDate.isDate = true;
> >         }
> > 
> >+        if (row.event_stamp) {
> >+            item.setProperty("DTSTAMP",newDateTime(row.event_stamp, "UTC"));
> >+        }
> >+
> >+        if (row.last_modified) {
> >+            item.setProperty("LAST-MODIFIED",newDateTime(row.last_modified, "UTC"));
> >+        }
> >+
> 
> >@@ -1567,27 +1573,33 @@ calStorageCalendar.prototype = {
> [...]
> >+        if (row.last_modified) {
> >+            item.setProperty("LAST-MODIFIED",newDateTime(row.last_modified, "UTC"));
> >+        }
> >+
> 
> Why are you setting the last_modified time in both getEventFromRow() and
> getTodoFromRow()? The modification time is a property of the base item, so this
> should be set in getItemBaseFromRow() where you also removed the similar set of
> lines. Note also that getItemBaseFromRow() is called in both of the other two
> functions, so this would save a couple lines of code but still provide the same
> functionality.

Yes, but it would not work at all. Please note, that it is crucial to set the modification time last of all properties, as the setting of any other property will change the modification time... this is also the reason why I removed the setting of the modification time from getItemBaseFromRow(), because it would be useless. Leaving the setting of the modifcation time in getItemFromBaseRow() would mean to move the call of this function to the end of the other get...()-functions. I will check out, if this is possible.

> 
> 
> >@@ -1567,27 +1573,33 @@ calStorageCalendar.prototype = {
> [...]
> >     getAdditionalDataForItem: function (item, flags) {
> >+        var save_last_modified = item.lastModifiedTime;
> >+
> Please add a small comment to why you are doing this in the source. I do
> realize why, but adding this to the source will make the code more readable.
> 
> 
> >@@ -1734,16 +1746,18 @@ calStorageCalendar.prototype = {
> [...]
> >+
> >+        item.setProperty("LAST-MODIFIED",save_last_modified);
> 
> Minor style nit: please use spaces between function arguments. i.e:     
>          item.setProperty("LAST-MODIFIED", save_last_modified);
> 
> With those issues fixed,
> r=phil
> 
> When you have a new patch, set first-review + directly and request
> second-review from [e-mail] (mvl is the storage provider module owner).
> 

OK, will do that.

Comment 22

[Norbert Püschel]

Attachment: Slightly modified version of patch

OK, this should fix the style issues of the last patch. Also, moved the setting of modification time back to getItemBaseFromRow(), where it should be, but had to move the calls of getItemBaseFromRow() for this to work. So hopefully, this is it.

Comment 23

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 258300 [details] [diff] [review]
Slightly modified version of patch

>Index: calendar/providers/storage/calStorageCalendar.js

>+        if (row.time_created)
>+          item.setProperty("CREATED", newDateTime(row.time_created, "UTC"));

Why must this be moved?

>+
>+        // This must be done last
Explain why it must be last
>+        if (row.last_modified)
>+            item.setProperty("LAST-MODIFIED", newDateTime(row.last_modified, "UTC"));
>     },

>-            item.stampTime = newDateTime(row.event_stamp, "UTC");
>+            item.setProperty("DTSTAMP", newDateTime(row.event_stamp, "UTC"));

Why this change?


>+        var save_last_modified = item.lastModifiedTime;

Use camel caste style, please: var savedLastModified;

Comment 24

[Norbert Püschel]

(In reply to comment #23)
> (From update of attachment 258300 [details] [diff] [review])
> >Index: calendar/providers/storage/calStorageCalendar.js
> 
> >+        if (row.time_created)
> >+          item.setProperty("CREATED", newDateTime(row.time_created, "UTC"));
> 
> Why must this be moved?

It doesn't have to, I just thought it is better style to keep the setting of related data close together.

> 
> >+
> >+        // This must be done last
> Explain why it must be last
> >+        if (row.last_modified)
> >+            item.setProperty("LAST-MODIFIED", newDateTime(row.last_modified, "UTC"));
> >     },

GAAH! Don't you guys ever read the bug history when you do a review? I'm growing sick of explaining all over again!

So, for the last time:

The modification time of an item must be set last because the setting of any other property will change the modification time.

I will add this to the comment in the patch; I thought this should be obvious...

> 
> >-            item.stampTime = newDateTime(row.event_stamp, "UTC");
> >+            item.setProperty("DTSTAMP", newDateTime(row.event_stamp, "UTC"));
> 
> Why this change?

RTF IDL file. stampTime is a readonly property. Note that this is not enough to fix stampTime, but at least it avoids error messages for trying to assign to readonly properties.

> 
> 
> >+        var save_last_modified = item.lastModifiedTime;
> 
> Use camel caste style, please: var savedLastModified;
> 

Will do.

Comment 25

[Norbert Püschel]

Attachment: Reworked Version of the patch

1) Expanded comment to explain why the modification time must always be set last.
2) Changed variable name to conform to naming convention.

Comment 26

[Michiel van Leeuwen (email: mvl+moz@)]

> > Explain why it must be last
> GAAH! Don't you guys ever read the bug history when you do a review? I'm
> growing sick of explaining all over again!

Sorry I was not clear. What I wanted to say:
Extend that comment, explaining why it must be last.
> I will add this to the comment in the patch; I thought this should be
> obvious...

It is obvious now. But it won't be so obvious when somebody else looks at that code a few months later.

Comment 27

[Norbert Püschel]

(In reply to comment #26)

> > I will add this to the comment in the patch; I thought this should be
> > obvious...
> 
> It is obvious now. But it won't be so obvious when somebody else looks at that
> code a few months later.
> 

Well, that is correct, right.

OK, is the new version of the patch now acceptable? If yes, will it get into 0.5?

Comment 28

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 258396 [details] [diff] [review]
Reworked Version of the patch

r2=mvl

Comment 29

[Matthew (lilmatt) Willis]

Patch checked in on MOZILLA_1_8_BRANCH and trunk.

-> FIXED