Closed Bug 1291360 Opened 9 years ago Closed 4 years ago

Add a JobCreator class for Downloadable-Content

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED INCOMPLETE

People

(Reporter: k.krish, Assigned: k.krish, Mentored)

References

Details

Attachments

(1 file)

In order to have proper scheduling, we need to implement JobCreator class from Android-job library and add every single jobs that are to be scheduled.
Assignee: nobody → k.krish
Mentor: s.kaspari
Blocks: 1248901
Comment on attachment 8777285 [details] Bug 1291360 - Add a JobCreator class for Downloadable-Content https://reviewboard.mozilla.org/r/68884/#review67138 ::: mobile/android/base/java/org/mozilla/gecko/dlc/DLCJobCreator.java:1 (Diff revision 1) > +package org.mozilla.gecko.dlc; nit: license header is missing ::: mobile/android/base/java/org/mozilla/gecko/dlc/DLCJobCreator.java:14 (Diff revision 1) > + > +/** > + * Using android-job scheduler to schedule the downloadable contents > + * DLCJobCreator is used to create separate jobs for each action. > + */ > +public class DLCJobCreator implements JobCreator { Other classes in this package use "DownloadContent" as prefix (instead of "DC"). ::: mobile/android/base/java/org/mozilla/gecko/dlc/DLCJobCreator.java:15 (Diff revision 1) > +/** > + * Using android-job scheduler to schedule the downloadable contents > + * DLCJobCreator is used to create separate jobs for each action. > + */ > +public class DLCJobCreator implements JobCreator { > + nit: Empty line (Style) ::: mobile/android/base/java/org/mozilla/gecko/dlc/DLCJobCreator.java:26 (Diff revision 1) > + @Override > + public Job create(String tag) { > + catalog = new DownloadContentCatalog(context); > + return null; > + } I don't understand what this method is meant to do (It doesn't do anything so far?). And why are we creating a catalog here?
Attachment #8777285 - Flags: review?(s.kaspari)
Comment on attachment 8777285 [details] Bug 1291360 - Add a JobCreator class for Downloadable-Content https://reviewboard.mozilla.org/r/68884/#review67138 > I don't understand what this method is meant to do (It doesn't do anything so far?). And why are we creating a catalog here? With the scheduler being included in the build, we will be moving from DownloadContentService to DownloadContentJobCreator. The library uses "Tags" to identify unique job. The tags obtained as parameter. For SyncAction I will be adding the following code in Bug 1257492. switch (tag) { case SyncAction.TAG: return new SyncAction(context, catalog); default: return null; } For this purpose we are creating the catalog. So as and when we are converting the BaseAction classes to Job, we will be adding a case for each Action.
Comment on attachment 8777285 [details] Bug 1291360 - Add a JobCreator class for Downloadable-Content https://reviewboard.mozilla.org/r/68884/#review68280 ::: mobile/android/base/java/org/mozilla/gecko/dlc/DownloadContentJobCreator.java:30 (Diff revision 2) > + this.context = context; > + } > + > + @Override > + public Job create(String tag) { > + catalog = new DownloadContentCatalog(context); DownloadContentCatalog is build in a way that there should always only be one instance (something that was always true for the service). I assume this can't be guaranteed here?
Attachment #8777285 - Flags: review?(s.kaspari)
Comment on attachment 8777285 [details] Bug 1291360 - Add a JobCreator class for Downloadable-Content https://reviewboard.mozilla.org/r/68884/#review68280 > DownloadContentCatalog is build in a way that there should always only be one instance (something that was always true for the service). I assume this can't be guaranteed here? I have converted the DownloadContentCatalog into a Singleton. It will remain in the memory as long as the memory is available (Android may reclaim the memory when needed).
(In reply to k.krish from comment #7) > Comment on attachment 8777285 [details] > Bug 1291360 - Add a JobCreator class for Downloadable-Content > > https://reviewboard.mozilla.org/r/68884/#review68280 > > > DownloadContentCatalog is build in a way that there should always only be one instance (something that was always true for the service). I assume this can't be guaranteed here? > > I have converted the DownloadContentCatalog into a Singleton. It will remain > in the memory as long as the memory is available (Android may reclaim the > memory when needed). Thanks! That's helpful. I wonder if we could unload the catalog at some point (to avoid keeping the whole catalog in memory for the whole time).
Comment on attachment 8777285 [details] Bug 1291360 - Add a JobCreator class for Downloadable-Content https://reviewboard.mozilla.org/r/68884/#review71376 This is looking good. See comments below: Can we initialize the catalog later? Can we unload the catalog later (File a follow-up bug if this is too big for now)? ::: mobile/android/base/java/org/mozilla/gecko/dlc/DownloadContentJobCreator.java:30 (Diff revision 3) > + this.context = context; > + } > + > + @Override > + public Job create(String tag) { > + catalog = DownloadContentCatalog.getInstance(context); The singleton approach is good! Can we move getting the catalog inside the action now? Getting the instance here will start loading the catalog from disk; even though we might not need that now.
Attachment #8777285 - Flags: review?(s.kaspari)
We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → INCOMPLETE
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: