Closed Bug 858017 Opened 7 years ago Closed 6 years ago

[COST CONTROL] Use Gecko native support for data usage alarms provided by NetworkStats API 2.0

Categories

(Firefox OS Graveyard :: Gaia::Cost Control, defect)

x86_64
Linux
defect
Not set

Tracking

(blocking-b2g:1.3T+, b2g-v1.3T fixed, b2g-v1.4 fixed)

RESOLVED FIXED
1.4 S3 (14mar)
blocking-b2g 1.3T+
Tracking Status
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: salva, Assigned: mai)

References

Details

Attachments

(2 files, 6 obsolete files)

Usage application should use native Gecko support for data usage alarms instead of work arounds in Gaia.
This can be addressed now. Prioritization?
Blocks: 850125
blocking-b2g: --- → 1.3?
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.
blocking-b2g: 1.3? → 1.4?
Assignee: nobody → mri
Depends on: 964270
Depends on: 965319
Depends on: 966175
Depends on: 966244
Depends on: 937251
Attached file patch v1.0 (obsolete) —
Please, could you give me your feedback?
Attachment #8369899 - Flags: feedback?(salva)
Comment on attachment 8369899 [details] [review]
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!
Attachment #8369899 - Flags: feedback?(salva)
Depends on: 968689
Comment on attachment 8369899 [details] [review]
patch v1.0

Please, could you give me your feedback?
Attachment #8369899 - Flags: feedback?(salva)
should be needed for tarako version
blocking-b2g: 1.4? → 1.3T?
Comment on attachment 8369899 [details] [review]
patch v1.0

Salva, could you review the patch?
Attachment #8369899 - Flags: feedback?(salva) → review?(salva)
Comment on attachment 8369899 [details] [review]
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}]
Attachment #8369899 - Flags: review?(salva) → review-
Attached file patch1-Add-NetworkUsageAlarms.html (obsolete) —
Salva, please, could you review the first patch with the new module and tests?
Regards
Attachment #8380487 - Flags: review?(salva)
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.
Attached file patch 1 -- Add network usage alarms (obsolete) —
Please, Salva could you review the patch?
Attachment #8380487 - Attachment is obsolete: true
Attachment #8380487 - Flags: review?(salva)
Attachment #8380516 - Flags: review?(salva)
Comment on attachment 8380516 [details] [review]
patch 1 -- Add network usage alarms

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

PR, updated with your comments
Attachment #8380516 - Flags: review?(salva)
Salva, could you review the second patch?
Attachment #8380613 - Flags: review?(salva)
Comment on attachment 8380516 [details] [review]
patch 1 -- Add network usage alarms

Working perfectly. Let's go!
Attachment #8380516 - Flags: review?(salva) → review+
Comment on attachment 8380613 [details] [review]
patch 2 -- Add listener to network usage alarms on messageHandler

I merged this before changing the flag to r+ but it was ok ,)
Attachment #8380613 - Flags: review?(salva) → review+
Please, could you review the latest patch?
Attachment #8381315 - Flags: review?(salva)
Comment on attachment 8381315 [details] [review]
patch 3 - Replacing datausageAlarm methods

Working fine. All is reviewed now. Thank you :mai.
Attachment #8381315 - Flags: review?(salva) → review+
Ok, now all is merged in the cc-alarms branch let's squash it in only one commit and prepare a PR for master.
Attached file patch v1.1 (obsolete) —
Final patch.
Attachment #8369899 - Attachment is obsolete: true
Attachment #8381337 - Flags: review?(salva)
Comment on attachment 8381337 [details] [review]
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!
Attachment #8381337 - Flags: review?(salva) → review+
Hi Fabrice,
May I land this new feature on the master branch?

Regards
Flags: needinfo?(fabrice)
Yes, you can land - just have good test coverage, and don't break anything.
Flags: needinfo?(fabrice)
Master: 877ddee8e913319780f7e9ccf1770c8af584b976
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.4 S2 (28feb)
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: 1.4 S2 (28feb) → ---
Attached file patch v1.2
Fix errors on TBPL test
Attachment #8383595 - Flags: review?(salva)
Comment on attachment 8383595 [details] [review]
patch v1.2

All Ok, it was a surprise the TBPL mode. Now working. Thank you.
Attachment #8383595 - Flags: review?(salva) → review+
Master: 9f7bdc9080b10c1480346d97d08cad592671f9aa
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
traige: 1.3T+ to get this into tarako for usage app memory saving
blocking-b2g: 1.3T? → 1.3T+
Depends on: 979341
Depends on: 980672
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
Flags: needinfo?(ying.xu)
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
Flags: needinfo?(mri)
Flags: needinfo?(jcheng)
Attached file patch for tarako
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
Attachment #8380516 - Attachment is obsolete: true
Attachment #8380613 - Attachment is obsolete: true
Attachment #8381315 - Attachment is obsolete: true
Attachment #8381337 - Attachment is obsolete: true
Flags: needinfo?(mri)
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.
Flags: needinfo?(jcheng)
(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!
Depends on: 963108, 809031, 968110
Since  963108, 809031, 968110 have been merged
Are there other things to merge?
Flags: needinfo?(ying.xu)
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
Flags: needinfo?(mri)
Comment on attachment 8391142 [details] [review]
patch for tarako

I'm update the PR over v1.3t.
Flags: needinfo?(mri)
(In reply to marina rodríguez [:mai] from comment #42)
> Comment on attachment 8391142 [details] [review]
> 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
Flags: needinfo?(mri)
Target Milestone: --- → 1.4 S3 (14mar)
Flags: needinfo?(mri)
You need to log in before you can comment on or make changes to this bug.