Closed Bug 697553 Opened 8 years ago Closed 8 years ago

Adapt to the calIChangeLog interface changes (gdata offline support)

Categories

(Calendar :: Provider: GData, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: Fallen, Assigned: Fallen)

References

Details

(Whiteboard: [needed beta][no l10n impact])

Attachments

(1 file, 1 obsolete file)

Attached patch Fix - v1 (obsolete) β€” β€” Splinter Review
Since offline support has landed, both gdata and wcap need some updating. This bug covers gdata support.

Mohit, I would be delighted if you could take a look at this patch to see if the code is doing the correct thing and also give the patch a test in both uncached and cached mode.
Attachment #569773 - Flags: review?(mohit.kanwal)
Flags: blocking-calendar1.0+
Attached patch Fix - v2 β€” β€” Splinter Review
This works much better. I've tested these cases:

* add/modify/delete while:
  - online with network
  - offline
  - online without network
* playback offline items
  - add/modify/delete working
  - all failing (easy to test with a bogus proxy server set)
  - Higher amount of items changed

* retrieve items:
  - offline / online
  - cached / uncached

* conflict testing (both while online and when coming back online):
  - modified on server, deleted locally
  - modified on server, modified locally
  - deleted on server, modified locally
  - deleted on server, deleted locally (no error!)
Attachment #569773 - Attachment is obsolete: true
Attachment #569773 - Flags: review?(mohit.kanwal)
Attachment #571482 - Flags: review+
Pushed to comm-central changeset 4a14fc769a0d
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Trunk
Backported to releases/comm-aurora changeset f2486981acc2
Target Milestone: Trunk → 1.0b9
Backported to releases/comm-beta changeset 053fae2d6e0f
Target Milestone: 1.0b9 → 1.0b8
Comment on attachment 571482 [details] [diff] [review]
Fix - v2

Mohit, despite this has landed, I'd appreciate if you could look at the code and give it a whirl.
Attachment #571482 - Flags: review+ → review?(mohit.kanwal)
Comment on attachment 571482 [details] [diff] [review]
Fix - v2

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

The patch looks fine. How ever can you tell me about doing !!this.mOfflineStorage in the calls to XXXItemOrUseCache?
I shall test it out, especially the conflict cases. 
Cheers,
Mohit

::: calendar/base/src/calCachedCalendar.js
@@ +496,1 @@
>                              }

Nice! changed the this_.supportsChangeLog to try-catch!

::: calendar/providers/gdata/components/calGoogleCalendar.js
@@ +337,1 @@
>  

Is there a typo in the above line?  !!this.mOfflineStorage
Attachment #571482 - Flags: review?(mohit.kanwal) → review+
Just tested. And it works well. 

+1
Hey Mohit, thanks for the review. Could you elaborate more on what you meant in the review? The splinter review is a bit strange sometimes, you need to click directly in the line where the issue is :-)

About !!this.mOfflineStorage, thats just a way to "cast" a variable to boolean. See the following example:


var foo = { some: "object" };
var firststep = !foo;        // firststep is now "false"
var secondstep = !firststep; // secondstep is now "true"
var in_one_step = !!foo;     // The same thing but in one step

The idea in XXXItem was to use the cache if we have an offline storage set, so that on an uncached call useCache is false.
Ok cool, that explains it then.
Blocks: 362645
You need to log in before you can comment on or make changes to this bug.