Usage application should use native Gecko support for data usage alarms instead of work arounds in Gaia.

This can be addressed now. Prioritization?

This is required to fix bug 850125, it could not be fixed before as there was no way to monitor real time data usage. Now we have data usage alarms in version 1.3, so it is time to fix this bug, otherwise users will receive data usage alarms too late, increasing their expenses.

Given the current scheduling and after checking with :noe I prefer to move it for 1.4 It is not critical but a good improvement and it must be well tested before landing completely.

Please, could you give me your feedback?

Comment on attachment 8369899 [details] patch v1.0 You have your feedback on GitHub. My impression is it is a quite big bug. Maybe we could split in some pieces and land them progressively. Hope blocking bugs will be solved soon. Thank you for your efforts :mai!

Comment on attachment 8369899 [details] patch v1.0 Please, could you give me your feedback?

should be needed for tarako version

Comment on attachment 8369899 [details] patch v1.0 Salva, could you review the patch?

Comment on attachment 8369899 [details] patch v1.0 I'm giving you the r- because the patch is huge. Let's land this split, ok? We could start by landing the new JavaScript module and its tests. --- Anyway, while trying, the alarm didn't trigger for 3MiB (application was closed) and once limit was surpassed I tried to set in again receiving: E/GeckoConsole( 865): Content JS ERROR at app://costcontrol.gaiamobile.org/js/settings/networkUsageAlarm.js:22 in error: Error, when trying to addAlarm to the interfaceId: 8934075100210426854 and limit: 3000000 When I rose the limit to 6MiB, nothing was logged to the terminal and keeping the Cost Control application in background, the alarm triggered on time. Resetting the alarm for a third time and closing CC app, the alarm was not triggered for 7MiB and these errors are often seen: E/GeckoConsole( 541): [JavaScript Error: "aStats is undefined" {file: "resource://gre/modules/NetworkStatsDB.jsm" line: 346}] E/GeckoConsole( 541): [JavaScript Error: "aStats is undefined" {file: "resource://gre/modules/NetworkStatsDB.jsm" line: 346}]

Salva, please, could you review the first patch with the new module and tests? Regards

FYI The patch is very big so we are landing this step by step in: https://github.com/lodr/gaia/branches/cc-alarms Once all is landed, we will squish the entire patch and ask for landing it on master.

Please, Salva could you review the patch?

Comment on attachment 8380516 [details] patch 1 -- Add network usage alarms Please, attend issues on GitHub and ask for my review again. Thank you :mai!

Comment on attachment 8380516 [details] patch 1 -- Add network usage alarms PR, updated with your comments

Salva, could you review the second patch?

Comment on attachment 8380516 [details] patch 1 -- Add network usage alarms Working perfectly. Let's go!

Comment on attachment 8380613 [details] patch 2 -- Add listener to network usage alarms on messageHandler I merged this before changing the flag to r+ but it was ok ,)

Please, could you review the latest patch?

Comment on attachment 8381315 [details] patch 3 - Replacing datausageAlarm methods Working fine. All is reviewed now. Thank you :mai.

Ok, now all is merged in the cc-alarms branch let's squash it in only one commit and prepare a PR for master.

Final patch.

Comment on attachment 8381337 [details] patch v1.1 Perfect, checked with Git this is nothing more than the former patches squashed. Easy to review. Working fine. Marce, Albert, Noe and Marina. Congrats & good work!

Hi Fabrice, May I land this new feature on the master branch? Regards

Yes, you can land - just have good test coverage, and don't break anything.

Master: 877ddee8e913319780f7e9ccf1770c8af584b976

I had to revert this in https://github.com/mozilla-b2g/gaia/commit/73105a3dfa82ea20ddadca81a27327149049cfd4 because gaia-unit tests were failing on TBPL: https://tbpl.mozilla.org/php/getParsedLog.php?id=35382359&tree=B2g-Inbound

Fix errors on TBPL test

Comment on attachment 8383595 [details] patch v1.2 All Ok, it was a surprise the TBPL mode. Now working. Thank you.

Master: 9f7bdc9080b10c1480346d97d08cad592671f9aa

traige: 1.3T+ to get this into tarako for usage app memory saving

Hi Ying Xu, heard that you will be doing uplifts to 1.3T branch. After you completed the uplift, can you please set status-b2g-v1.3T to fixed? please let us know if you have problems with it. thanks

hi,mri I am doing uplifts to 1.3T branch for this patch,and when I do cherry-pick there are 3 files conflicts,so I'd like you to review this patch.Thank you very much!The pull request is: https://github.com/mozilla-b2g/gaia/pull/17185

I create a new PR to resolve the conficts. These conficts are produced because of the following bug patchs, that are not landed on v1.3t, but they were landed on master before this patch: * Bug 963108 - [Cost control] Remove reference to icc_helper.js library (Polish bug) * Bug 809031 - OOP <iframe> content disappear if placed below 147px inside the system * Bug 968110 - [costcontrol] Javascript error when open a notification when the application is closed These bugs are not related with the Bug 858017. Regards

shouldn't we land the other bugs individually and then resolve the conflicts, instead of creating a huge new patch?

Reviewing my previous comment, I think that I did not express myself well. The new pr do only contain the patch for the bug 858017, not for the other related bugs.

(In reply to Jose M. Cantera from comment #34) > shouldn't we land the other bugs individually and then resolve the > conflicts, instead of creating a huge new patch? I agree. I think if they are blockers of a blocker we should resolve them instead of providing a new patch.

Per comment 34 and comment 36, adding Bug 963108, Bug 809031 and Bug 968110 as blockers of this bug and nominating them to v1.3T in order the patch corresponding to this bug can be directly uplifted to v1.3T branch. Thanks!

Since 963108, 809031, 968110 have been merged Are there other things to merge?

Yes, the pull request from this bug, https://github.com/mozilla-b2g/gaia/pull/17192 - it needs to be fixed though since there are lint errors.

There are many conflicts in these two files apps/costcontrol/test/unit/common_test.js apps/costcontrol/test/unit/cost_control_test.js Can someone rebase the patch?

Marina, is it something you can help with? thanks

Comment on attachment 8391142 [details] patch for tarako I'm update the PR over v1.3t.

(In reply to marina rodríguez [:mai] from comment #42) > Comment on attachment 8391142 [details] > patch for tarako > > I'm update the PR over v1.3t. I see the travis build failed. Is that OK? https://travis-ci.org/mozilla-b2g/gaia/builds/20999852