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

Status: RESOLVED FIXED

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

Description

[James Lal [:lightsofapollo]]

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.

Comment 1

[James Lal [:lightsofapollo]]

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.

Comment 2

[Josh Carpenter [:jcarpenter]]

James, can you clarify the CPU and battery impact?

Comment 3

[Jason Smith [:jsmith]]

Hmm...I'm going to renom. Not sure if this blocks unless there's enough evidence of significant risk here.

Comment 4

[James Lal [:lightsofapollo]]

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.

Comment 5

[Cody Brocious [:Daeken]]

Attachment: Implemented functionality and test

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.

Comment 6

[James Lal [:lightsofapollo]]

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

Comment 7

[James Lal [:lightsofapollo]]

I will pick this up for next release (post Jan 15)

Comment 8

[Zibi Braniecki [:zbraniecki][:gandalf]]

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.

Comment 9

[Lucas Adamski [:ladamski]]

Probably just needs approval.

Comment 10

[Lucas Adamski [:ladamski]]

Sorry, ignore blocking req until comment 8 is addressed.

Comment 11

[Lucas Adamski [:ladamski]]

Please see comment 8 re qawanted.

Comment 12

[Jason Smith [:jsmith]]

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.

Comment 13

[Jason Smith [:jsmith]]

James - Can you rebase the patch and get this landed?

Comment 14

[Dietrich Ayala (:dietrich)]

J-lal, is this long ago bitrotterdam'd?

Comment 15

[James Lal [:lightsofapollo]]

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.

Comment 16

[Mike Lee [:mlee]]

So, close this as a dup of bug 832258?

Comment 17

[Mike Lee [:mlee]]

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?

Comment 18

[James Lal [:lightsofapollo]]

Ah no- this still needs some glue cleanup now that that bug has landed.

Comment 19

[Hubert Figuiere [:hub]]

Looks like we are waiting on bug 832258

Comment 20

[Mike Lee [:mlee]]

Dylan,

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

FxOS Perf Triage

Comment 21

[Dylan Oliver [:doliver]]

Gareth, can you comment on what we want to do with this bug and bug 832258?

Comment 22

[Gareth Aye [:gaye] (back from PTO)]

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?

Comment 23

[Evan Tseng [:evanxd]]

(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.

Comment 24

[Dylan Oliver [:doliver]]

[priority] --> tracking-b2g:+ conversion

Comment 25

[Gareth Aye [:gaye] (back from PTO)]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/25420

Haven't tested yet, but pretty sure this is what we want :)

Comment 26

[Miller Medeiros [:millermedeiros]]

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.

Comment 27

[Gareth Aye [:gaye] (back from PTO)]

https://github.com/mozilla-b2g/gaia/commit/3b98c6a903e79ea16a51832fa4be23f817ab5829 landed on master

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to revert this in https://github.com/mozilla-b2g/gaia/commit/eb8a140841f2fb2d8f28f45cbfd0bb053d8aa187 for Gu test failures:

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=730707&repo=b2g-inbound

Comment 29

[Gareth Aye [:gaye] (back from PTO)]

https://github.com/mozilla-b2g/gaia/commit/a07994714f0552f89801d6097982308d8b0a1ee1 relanded on master and thanks Wes for catching.

Comment 32

[Josh Cheng [:josh]]

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!

Comment 33

[GitHub Autolander Bot]

Attachment: [PullReq] mpizza:bug_821604_v2.0m to mozilla-b2g:v2.0m

Comment 34

[GaryChen [:GaryChen][:PYChen][:gchen][:陳柏宇]]

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

Comment 35

[Kai-Zhen Li [:seinlin][:kli]]

v2.0m: https://github.com/mozilla-b2g/gaia/commit/603c5a030960d69223b53a249686dd1793e68260

Comment 36

[Josh Cheng [:josh]]

Hi Gary,
Thanks!