Closed Bug 1079189 Opened 10 years ago Closed 10 years ago

Code Review for bug 493389 - Provider for Google Calendar cannot sync tasks


(Calendar :: Provider: GData, defect, P1)



(Not tracked)



(Reporter: Fallen, Assigned: Fallen)




(2 files)

Attached patch All Changes - v1 β€” β€” Splinter Review
bug 493389 already has a lot of comments, therefore I'm doing the code review in here. If you are reading this, please don't use this bug for error reports or feature requests and head over to bug 493389 instead.

The code changes are pretty massive and it will probably take a long time to review at a normal pace. Therefore I propose to only review string changes and the build system. I've split these out into a separate patch, but note the full patch also contains these changes.

Note that I had to import OAuth2.jsm again to be backwards compatible with TB24. I guess we could remove this when pushing to comm-central, but I'd rather just push this in and then gradually remove backwards compatibility in new bugs.
Mark, I need you to review the mail/testsuite-targets change. If you have a few minutes more feel free to look at the other build system parts in this patch to make sure things are sane.

There is a pretty extensive unit test that I'd like to run during builds, but for that to happen the gdata provider obviously needs to be packaged with the tests. The existing code that copies extensions from the test package to the test profile should do the rest.

Mohit and Matt, please take a look at the remaining changes and r+ when you are done. If you'd also like to look at the final provider code, please use f+ on the other patch.
Attachment #8500960 - Flags: review?(standard8)
Attachment #8500960 - Flags: review?(mohit.kanwal)
Attachment #8500960 - Flags: review?(matthew.mecca)
Attachment #8500960 - Flags: review?(standard8) → review+
Attachment #8500960 - Flags: review?(mohit.kanwal) → review+
Attachment #8500960 - Flags: review?(matthew.mecca) → review+
Priority: -- → P1
Pushing strings only in time for the merge to get localization 6 weeks earlier:
Depends on: 1103637
No longer blocks: 1085287
Depends on: 1061363, 1085287
Depends on: 1108597
Depends on: 1106047

Not sure what the best way is to go about backporting. I'd be fine with just keeping it on c-c, but some strings are in aurora and since I'd like Lightning 3.1 -> Provider 1.0 I would have to backport to ESR. This doesn't work because of strings.

I could modify the version creation script to do Lightning 3.9 -> Provider 1.0, or backport to aurora (minus new strings) and use 3.8 -> 1.0.
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.9
(In reply to Philipp Kewisch [:Fallen] from comment #2)
> Pushing strings only in time for the merge to get localization 6 weeks
> earlier:

For better localizations busyTitle and quotaExceeded should have a comment about what %1$S is substituted with. Not every localizer can go and check the code to find that out :).
Added bug 1114504 for this issue. Thanks!
You need to log in before you can comment on or make changes to this bug.