Closed Bug 329035 Opened 18 years ago Closed 18 years ago

calendar view refresh() operation has re-entrancy problems

Categories

(Calendar :: Calendar Frontend, defect)

Product:
Calendar
Component:
Calendar Frontend
Type:
defect
Priority:
Not set
Severity:
major

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: dmosedale, Assigned: michael.buettner)

References

Details

Attachments

(1 file, 1 obsolete file)

patch v2
6.66 KB, patch
mattwillis
: first-review+
Details | Diff | Splinter Review 
The instance that I've noticed is that in multiday view, calling refresh before an existing refresh request has completed is almost certain to confuse the view.  I suspect other views may have similar issues.
Severity: normal → major
umm, bug 323094 has nothing to do with this bug.
No longer depends on: 323094
Whoops!
Depends on: 329034
The bugspam monkeys have struck again. They are currently chewing on default assignees for Calendar. Be afraid for your sanity!
Assignee: base → nobody
Component: Internal Components → Calendar Views
QA Contact: base → views
taking this one.
Assignee: nobody → michael.buettner
the problem basically is that refresh() implicitly assumes that previous calls already have been finished. this is because refresh() calls relayout() which in turn accesses the internal structure in order to delete exiting event boxes from the view. in case we call refresh() several times with asynchronous providers in place we end up calling relayout() *before* previous operation are finished.

there are two possible solutions to this problem:

1) establish a queue for the requests and deal with them one after another.

2) establish a true sync-mechanism for relayout(). basically onGetResult() receives the list of new items while those items contained in the view can be considered as the old set of item. we could synchronize those sets when receiving a new bunch of items in onGetResult(), i.e. new items, modified items, deleted items. this functionality could be moved into the providers so that every client could use it.

comments are welcome...
Attached patch patch v1 (obsolete) — — Splinter Review 
this patch implements solution 1 as proposed above. i'd still be interested in implementing the second solution, but i'd like to get some feedback before writing the patch.
Attachment #244816 - Flags: first-review?(lilmatt)
Comment on attachment 244816 [details] [diff] [review]
patch v1

It's all style nits, but there's quite a few, so I'd like to see this once more before giving it r+.

>+++ mozilla/calendar/base/content/calendar-month-view.xml
>-      <method name="refresh">
>-        <body><![CDATA[

>+      <method name="popRefreshQueue">
>+        <body>
>+          <![CDATA[
Put the CDATA on the same line as the body tag. That's how most of them in this file appear to be.

>+          if(this.mRefreshPending)
>+            return;
>+
>+          var refreshJob = this.mRefreshQueue.pop();
>+          if(!refreshJob)
>+            return;
>+
For both of these if statements, please add a space before the (, and please wrap the return in curly braces. We ought to have done that originally.

>+      <method name="refresh">
>+        <body>
>+          <![CDATA[
>+          var refreshJob = {};
>+          this.mRefreshQueue.push(refreshJob);
>+          this.popRefreshQueue();
>+        ]]>
>+        </body>
>+      </method>
Put the opening and closing of the CDATA on the same lines as the opening and closing of the body tag. Prevailing style.

>-          onOperationComplete: function(aCalendar, aStatus, aOperationType, aId, aDetail) {},
>+          onOperationComplete: function(aCalendar, aStatus, aOperationType, aId, aDetail) {
>+             // signal that the current operation finished.
>+             this.calView.mRefreshPending = false;
>+         
>+             // immediately start the next job on the queue.
>+             this.calView.popRefreshQueue();
>+          },
Use 2-space indenting here, as it appears to be prevailing style.
In addition, the blank line in the middle has a bunch of trailing spaces.


>+++ mozilla/calendar/base/content/calendar-multiday-view.xml	
 
>           function onOperationComplete(aCalendar, aStatus, aOperationType, 
>-                                       aId, aDetail) {},
>+                                       aId, aDetail) {
>+               // signal that the current operation finished.
>+               this.calView.mRefreshPending = false;
>+               
>+               // immediately start the next job on the queue.
>+               this.calView.popRefreshQueue();
>+          },
This whole block uses 5-space indenting for some reason. Please use 4 to match the (arguable) prevailing style. In addition, the blank line in the middle has a bunch of trailing spaces.

>-      <method name="refresh">
>-        <body><![CDATA[

>+
>+      <method name="popRefreshQueue">
>+        <body>
>+          <![CDATA[
Same deal here as with before. CDATA on same line as body.

>+          if(this.mRefreshPending)
>+            return;
>+          
>+          var refreshJob = this.mRefreshQueue.pop();
>+          if(!refreshJob)
>+            return;
>+
Braces around returns, spaces before (. Indent the returns two more spaces also.
The blank line has trailing spaces.

>+        ]]>
>+        </body>
>+      </method>
Closing CDATA on same line as closing body

>+      <method name="refresh">
>+        <body>
>+          <![CDATA[
CDATA on same line as body.
Attachment #244816 - Flags: first-review?(lilmatt) → first-review-
Attached patch patch v2 — — Splinter Review 
patch with nits addressed...
Attachment #244816 - Attachment is obsolete: true
Attachment #246006 - Flags: first-review?(lilmatt)
Comment on attachment 246006 [details] [diff] [review]
patch v2

Nice. Only one nit this time.

> mozilla/calendar/base/content/calendar-multiday-view.xml
>@@ -2271,18 +2277,30 @@
>                   child = child.nextSibling;
>               }
>           }
>           this.refresh();
>           this.scrollToMinute(firstMinute);          
>         ]]></body>
>       </method>
> 
>-      <method name="refresh">
>+      <field name="mRefreshQueue">[]</field>
>+      <field name="mRefreshPending">false</field>
>+
>+      <method name="popRefreshQueue">
>         <body><![CDATA[
>+          if (this.mRefreshPending) {
>+            return;
>+          }
>+          
This line ^^^^ has trailing spaces
>+          var refreshJob = this.mRefreshQueue.pop();
>+          if (!refreshJob) {
>+            return;
>+          }
>+
>           if (!this.startDate || !this.endDate)
>             return;
> 
>           // recreate our columns
>           this.relayout();
> 
>           if (!this.mCalendar)
>               return;

r=lilmatt with that fixed
Attachment #246006 - Flags: first-review?(lilmatt) → first-review+
Checked in on trunk and MOZILLA_1_8_BRANCH
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: