Closed Bug 1257492 Opened 6 years ago Closed 9 months ago

DLC Sync: Better scheduling of sync action

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED INCOMPLETE

People

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

References

Details

Attachments

(1 file)

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: nobody → k.krish
Mentor: s.kaspari
URL: 1291180
URL: 1291180
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 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)
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: 9 months 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.