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

Status: RESOLVED FIXED

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

Description

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

Attachment: All Changes - v1

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.

Comment 1

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

Attachment: Strings and Build System - v1

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.

Comment 2

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

Pushing strings only in time for the merge to get localization 6 weeks earlier:

https://hg.mozilla.org/comm-central/rev/256a13ac5eb5

Comment 3

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

https://hg.mozilla.org/comm-central/rev/a24a53785467

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.

Comment 4

[Merike Sell (:merike)]

(In reply to Philipp Kewisch [:Fallen] from comment #2)
> Pushing strings only in time for the merge to get localization 6 weeks
> earlier:
> 
> https://hg.mozilla.org/comm-central/rev/256a13ac5eb5

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 :).

Comment 5

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

Added bug 1114504 for this issue. Thanks!