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

RESOLVED FIXED in 3.9

Status

P1
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Fallen, Assigned: Fallen)

Tracking

Dependency tree / graph

Details

Attachments

(2 attachments)

Created attachment 8500955 [details] [diff] [review]
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.
Created attachment 8500960 [details] [diff] [review]
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.
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+
(Assignee)

Updated

4 years ago
Priority: -- → P1
Pushing strings only in time for the merge to get localization 6 weeks earlier:

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

Updated

4 years ago
Blocks: 1085287
(Assignee)

Updated

4 years ago
Depends on: 1103637
(Assignee)

Updated

4 years ago
No longer blocks: 1085287
Depends on: 1061363, 1085287
(Assignee)

Updated

4 years ago
Depends on: 1082478, 1083934
(Assignee)

Updated

4 years ago
Depends on: 1108597
(Assignee)

Updated

4 years ago
Depends on: 1106047
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Updated

4 years ago
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:
> 
> 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 :).
Added bug 1114504 for this issue. Thanks!
You need to log in before you can comment on or make changes to this bug.