Closed Bug 821604 Opened 8 years ago Closed 6 years ago

Don't start periodic sync until we have a calendar that needs a real sync operation.

Categories

(Firefox OS Graveyard :: Gaia::Calendar, defect, P2)

defect

Tracking

(blocking-b2g:2.0M+, blocking-basecamp:-, tracking-b2g:+, b2g18+, b2g-v2.0 affected, b2g-v2.0M fixed, b2g-v2.1 affected, b2g-v2.1S affected, b2g-v2.2 fixed, b2g-master fixed)

RESOLVED FIXED
2.1 S8 (7Nov)
blocking-b2g 2.0M+
blocking-basecamp -
tracking-b2g +
Tracking Status
b2g18 + ---
b2g-v2.0 --- affected
b2g-v2.0M --- fixed
b2g-v2.1 --- affected
b2g-v2.1S --- affected
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: jlal, Assigned: gaye)

References

Details

(Keywords: perf, verifyme, Whiteboard: [priority][p=8][c= p= s= u=])

Attachments

(4 files)

This has a CPU/battery impact as we wake up the device to attempt to sync when there are no real calendars we need to sync.
Followup from periodic sync there is code that does very similar things in modify event and settings. We basically need to watch for new calendars to be added then turn on/off periodic sync as need.
blocking-basecamp: --- → ?
Keywords: perf
blocking-basecamp: ? → +
Priority: -- → P3
Assignee: nobody → cbrocious
Target Milestone: --- → B2G C3 (12dec-1jan)
James, can you clarify the CPU and battery impact?
Hmm...I'm going to renom. Not sure if this blocks unless there's enough evidence of significant risk here.
blocking-basecamp: + → ?
Without this work even though there are no calendars that _need_ to be synced (like the offline one) we will wake up the calendar (and cpu / wifi) every N minutes (defaults to on and 15).

Obviously we need to do this for users with synced calendars so the cost is not massive but simply excessive for users who don't need this functionality. We essentially drain they battery for no reason. I suspect the drain is minimal but everything adds up.

Given our market it may even make sense to have periodic sync off by default with or without the fix suggested here.

Cody I think has started on this so b- or + we may end up with this functionality as a pending patch.
blocking-basecamp: ? → -
The pull request is at https://github.com/mozilla-b2g/gaia/pull/7134

The test is very simple: add a caldav account and see that periodic sync is enabled, then remove it and see that sync is disabled.
Attachment #694764 - Flags: review?(jlal)
Comment on attachment 694764 [details] [diff] [review]
Implemented functionality and test

Reviewed this awhile ago there are some comments that need to be addressed on GH but the approach makes sense...
Attachment #694764 - Flags: review?(jlal) → review+
Assignee: cody.brocious+bugzilla → jlal
I will pick this up for next release (post Jan 15)
Assignee: jlal → nobody
I believe this bug to be causing calendar app to be started automatically without user intention.
Within 5-10 minutes from starting fresh profile gaia I see calendar in cardview without ever starting it.
Whiteboard: [FFOS_perf]
Probably just needs approval.
blocking-b2g: --- → tef?
Sorry, ignore blocking req until comment 8 is addressed.
blocking-b2g: tef? → ---
Please see comment 8 re qawanted.
Keywords: qawanted
We've already got a r+ patch here...don't think it's worthwhile to do QA investigation. Let's rebase the patch and get it landed.
Keywords: qawanted
James - Can you rebase the patch and get this landed?
Flags: needinfo?(jlal)
Assignee: nobody → jlal
Flags: needinfo?(jlal)
J-lal, is this long ago bitrotterdam'd?
Flags: needinfo?(jlal)
Whiteboard: [FFOS_perf] → [FFOS_perf] c=
Yep- this patch is not viable anymore... I had a conditional r+ before Cody left.

I have another patch pending review (bug 832258) which has the majority of the work needed for this.
Flags: needinfo?(jlal)
So, close this as a dup of bug 832258?
James(In reply to James Lal [:lightsofapollo] from comment #15)
> Yep- this patch is not viable anymore... I had a conditional r+ before Cody
> left.
> 
> I have another patch pending review (bug 832258) which has the majority of
> the work needed for this.

So close this as a dup of bug 832258?
Flags: needinfo?(jlal)
Whiteboard: [FFOS_perf] c= → [c= ]
Ah no- this still needs some glue cleanup now that that bug has landed.
Assignee: jlal → nobody
Flags: needinfo?(jlal)
Priority: P3 → P2
Target Milestone: B2G C3 (12dec-1jan) → ---
Looks like we are waiting on bug 832258
Depends on: 832258
Dylan,

Are these Calendar app improvements still wanted by the Productivity team?

FxOS Perf Triage
Flags: needinfo?(doliver)
Whiteboard: [c= ] → [c= p= s= u=]
Gareth, can you comment on what we want to do with this bug and bug 832258?
Flags: needinfo?(doliver) → needinfo?(gaye)
I just landed James' patch for 832258 on gaia master. We can go ahead and work on this now with help from some of that code. Perhaps this is the sort of thing we might want to triage?
Flags: needinfo?(gaye)
blocking-b2g: --- → backlog
Whiteboard: [c= p= s= u=] → [priority][p=8]
Whiteboard: [priority][p=8] → [priority][p=8][c= p= s= u=]
Assignee: nobody → gaye
Target Milestone: --- → 1.4 S1 (14feb)
Target Milestone: 1.4 S1 (14feb) → ---
Assignee: gaye → nobody
(In reply to James Lal [:lightsofapollo] Less responsive until Jan 1 (please email for urgent issues) from comment #18)
> Ah no- this still needs some glue cleanup now that that bug has landed.

Hi James,

Sorry, I'm confused with this bug.
After Bug 832258 is landed, what should we do for the sync things?
Excuse me, do what cleanup?

Currently, the Calendar doesn't not wake up automatically in the gaia 02ba36a5 (2014-03-07) build.
It means that this issue is not existed.

So we could just close this bug, right?
Or anything I missed?

Thanks.
Flags: needinfo?(jlal)
Assignee: nobody → evanxd
Assignee: evanxd → nobody
Assignee: nobody → gaye
Status: NEW → ASSIGNED
Flags: needinfo?(jlal)
[priority] --> tracking-b2g:+ conversion
tracking-b2g: --- → +
Haven't tested yet, but pretty sure this is what we want :)
Attachment #8509701 - Flags: review?(mmedeiros)
Comment on attachment 8509701 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/25420

just a few nits but LGTM! nice work.
Attachment #8509701 - Flags: review?(mmedeiros) → review+
https://github.com/mozilla-b2g/gaia/commit/3b98c6a903e79ea16a51832fa4be23f817ab5829 landed on master
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S8 (7Nov)
https://github.com/mozilla-b2g/gaia/commit/a07994714f0552f89801d6097982308d8b0a1ee1 relanded on master and thanks Wes for catching.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Flags: needinfo?(gaye)
Resolution: --- → FIXED
Duplicate of this bug: 1113549
Duplicate of this bug: 1129278
blocking-b2g: backlog → 2.0M+
Hi Gary,
This bugs has also found by partner in bug 1129278. Could you check whether we can pick it back to 2.0M?
Thanks!
Flags: needinfo?(gchen)
Attached file PR for v2.0m
Hi Josh and Kai-Zhen,
   I've prepared the patch for v2.0m, please help to land in v2.0m.
   Thanks.
Flags: needinfo?(kli)
Flags: needinfo?(jocheng)
Flags: needinfo?(gchen)
Hi Gary,
Thanks!
Flags: needinfo?(jocheng)
Keywords: verifyme
QA Whiteboard: QAExclude
Flags: needinfo?(jmercado)
Flags: needinfo?(jmercado)
You need to log in before you can comment on or make changes to this bug.