DLC Sync: Better scheduling of sync action

NEW
Assigned to

Status

()

3 years ago
2 years ago

People

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

Tracking

(Depends on: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Currently the sync action is launched after app start after receiving the DelayedStartup event.

We can do better. Especially if we can use more advanced APIs like JobScheduler or GcmNetworkManager - see bug 1248901.
No longer blocks: 1248901
Depends on: 1248901
Depends on: 1260406
(Assignee)

Updated

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

Updated

3 years ago
URL: 1291180
(Assignee)

Updated

3 years ago
URL: 1291180
Comment hidden (mozreview-request)
(Reporter)

Comment 2

3 years ago
mozreview-review
Comment on attachment 8779485 [details]
Bug 1257492 - DLC Sync: Better scheduling of sync action

https://reviewboard.mozilla.org/r/70464/#review68282

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:1945
(Diff revision 1)
>                      }, oneSecondInMillis);
>                  }
>  
>                  if (AppConstants.MOZ_ANDROID_DOWNLOAD_CONTENT_SERVICE) {
> -                    // TODO: Better scheduling of sync action (Bug 1257492)
> -                    DownloadContentService.startSync(this);
> +                    //To check whether the SyncAction (Periodic Job) is already scheduled to run.
> +                    if (JobManager.instance().getAllJobRequestsForTag(SyncAction.JOBTAG).size() < 1) {

.isEmpty() ?

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:4124
(Diff revision 1)
> +
> +    /**
> +     *  Scheduling the SyncAction as a periodic job. Repeats once in 5 days.
> +     */
> +    private void scheduleSyncAction() {
> +        long time = 60000 * 60 * 24 * 5;

Any specific reason why you picked 5 days? Currently we run sync on every (cold) app start. Maybe start with 1 day? Can we add additional requirements here (like network conncetion)?

::: mobile/android/base/java/org/mozilla/gecko/dlc/SyncAction.java:292
(Diff revision 1)
> +        perform(context, catalog);
> +
> +        return null;
> +    }
> +
> +    protected HttpURLConnection buildHttpURLConnection(String url)

I assume this is copied from the base class? Can we move this to a helper class to avoid duplicating this code?
Attachment #8779485 - Flags: review?(s.kaspari)
Comment hidden (mozreview-request)
(Reporter)

Comment 4

3 years ago
mozreview-review
Comment on attachment 8779485 [details]
Bug 1257492 - DLC Sync: Better scheduling of sync action

https://reviewboard.mozilla.org/r/70464/#review70592

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:4123
(Diff revision 2)
> +
> +    /**
> +     *  Scheduling the SyncAction as a periodic job. Repeats once in a day.
> +     */
> +    private void scheduleSyncAction() {
> +        int jobId = new JobRequest.Builder(SyncAction.JOBTAG)

nit: The job id is not used. No need to assign it here, I guess?

::: mobile/android/base/java/org/mozilla/gecko/dlc/DownloadContentHelper.java:21
(Diff revision 2)
> +import java.net.URISyntaxException;
> +
> +/**
> + * DownloadContentHelper Class contains all the utility methods for the the Downloadable Contents
> + */
> +public class DownloadContentHelper {

This new class needs to be added to mobile/android/base/moz.build

::: mobile/android/base/java/org/mozilla/gecko/dlc/DownloadContentService.java:14
(Diff revision 2)
> +import org.mozilla.gecko.AppConstants;
> +import org.mozilla.gecko.GeckoAppShell;
> +import org.mozilla.gecko.dlc.catalog.DownloadContent;
> +import org.mozilla.gecko.dlc.catalog.DownloadContentCatalog;
> +import org.mozilla.gecko.util.HardwareUtils;

Did Android Studio move all those lines?

::: mobile/android/base/java/org/mozilla/gecko/dlc/SyncAction.java:294
(Diff revision 2)
> +
> +    @NonNull
> +    @Override
> +    protected Result onRunJob(Params params) {
> +
> +        perform(context, catalog);

The DownloadContentService was responsible for persiting the catalog after an action was executed. It seems like this is not happening anymore and the synchronized state might get lost as soon as the app is killed.

::: mobile/android/base/java/org/mozilla/gecko/dlc/SyncAction.java:296
(Diff revision 2)
> +    @Override
> +    protected Result onRunJob(Params params) {
> +
> +        perform(context, catalog);
> +
> +        return null;

Doesn't this need to return Result.SUCCESS (or other results depending on whether the sync was successful).
Attachment #8779485 - Flags: review?(s.kaspari)
You need to log in before you can comment on or make changes to this bug.