Add a JobCreator class for Downloadable-Content

NEW
Assigned to

Status

()

defect
3 years ago
3 years ago

People

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

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Assignee

Description

3 years ago
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

Updated

3 years ago
Assignee: nobody → k.krish
Mentor: s.kaspari
Assignee

Updated

3 years ago
Blocks: 1248901

Comment 2

3 years ago
mozreview-review
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 hidden (mozreview-request)
Assignee

Comment 4

3 years ago
mozreview-review-reply
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 5

3 years ago
mozreview-review
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 hidden (mozreview-request)
Assignee

Comment 7

3 years ago
mozreview-review-reply
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 9

3 years ago
mozreview-review
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)
You need to log in before you can comment on or make changes to this bug.