Skip to content
Permalink
Browse files

Revert "[AW] Always schedule variations seed download jobs when a see…

…d is requested"

This reverts commit 5e8d35c.

Reason for revert: Caused a possible seed freshness regression.

Original change's description:
> [AW] Always schedule variations seed download jobs when a seed is requested
> 
> We don't currently schedule a seed download in WebView if the previous
> download occurred less than 24 hours ago, which can add an additional 24
> hours to the time between seed downloads. This CL reimplements the 24
> hour throttle to be a delay in the scheduled job, meaning a job will
> always be scheduled when a new seed is requested, but Android will only
> run the job once 24 hours have past since the last download.
> 
> This CL also adds a missing hamcrest dependency to
> //aw/test:webview_instrumentation_test_apk
> 
> Test: ninja -C out/aw webview_instrumentation_test_apk
> Test: out/aw/bin/run_webview_instrumentation_test_apk -f 'AwVariationsSeedFetcherTest#*'
> Bug: 979075
> Change-Id: I0ea1cc8197f66b77634e8de2b145fe4228c74067
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1906121
> Reviewed-by: Changwan Ryu <changwan@chromium.org>
> Reviewed-by: Nate Fischer <ntfschr@chromium.org>
> Commit-Queue: Robbie McElrath <rmcelrath@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#716305}

TBR=changwan@chromium.org,ntfschr@chromium.org,rmcelrath@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

(cherry picked from commit 414a833)

Bug: 979075
Change-Id: Iac6d048bdad48ae4bad004c5e6d27d37335cdb68
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1994645
Reviewed-by: Robbie McElrath <rmcelrath@chromium.org>
Reviewed-by: Nate Fischer <ntfschr@chromium.org>
Reviewed-by: Changwan Ryu <changwan@chromium.org>
Commit-Queue: Robbie McElrath <rmcelrath@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#730308}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1997647
Cr-Commit-Position: refs/branch-heads/3987@{#495}
Cr-Branched-From: c4e8da9-refs/heads/master@{#722274}
  • Loading branch information
robbiemc authored and Commit Bot committed Jan 13, 2020
1 parent ea43c8d commit 45df6ff1db00723a4e23bf1a4e78f04a5cc19c4f
@@ -26,13 +26,13 @@
import org.chromium.android_webview.test.util.VariationsTestUtils;
import org.chromium.base.ContextUtils;
import org.chromium.base.test.util.CallbackHelper;
import org.chromium.components.background_task_scheduler.TaskIds;
import org.chromium.components.variations.firstrun.VariationsSeedFetcher;

import java.io.File;
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;

/**
@@ -41,8 +41,7 @@
@RunWith(AwJUnit4ClassRunner.class)
@OnlyRunIn(SINGLE_PROCESS)
public class AwVariationsSeedFetcherTest {
// Jan 1, 2019 12:00AM GMT
private static final long FAKE_NOW_MS = 1546300800000L;
private static final int JOB_ID = TaskIds.WEBVIEW_VARIATIONS_SEED_FETCH_JOB_ID;

// A mock JobScheduler which only holds one job, and never does anything with it.
private class MockJobScheduler extends JobScheduler {
@@ -52,10 +51,12 @@ public void clear() {
mJob = null;
}

public void assertScheduledWithDelayEqualTo(long delay) {
public void assertScheduled() {
Assert.assertNotNull("No job scheduled", mJob);
Assert.assertEquals(
"Job scheduled with wrong delay", delay, mJob.getMinLatencyMillis());
}

public void assertNotScheduled() {
Assert.assertNull("Job should not have been scheduled", mJob);
}

@Override
@@ -88,8 +89,7 @@ public JobInfo getPendingJob(int jobId) {

@Override
public int schedule(JobInfo job) {
Assert.assertEquals(
"Job scheduled with wrong ID", AwVariationsSeedFetcher.JOB_ID, job.getId());
Assert.assertEquals("Job scheduled with wrong ID", JOB_ID, job.getId());
Assert.assertEquals("Job scheduled with wrong network type",
JobInfo.NETWORK_TYPE_ANY, job.getNetworkType());
Assert.assertTrue("Job scheduled without charging requirement",
@@ -120,7 +120,6 @@ public SeedInfo downloadContent(@VariationsSeedFetcher.VariationsPlatform int pl
@Before
public void setUp() throws IOException {
AwVariationsSeedFetcher.setMocks(mScheduler, mDownloader);
AwVariationsSeedFetcher.setMinJobPeriodMillisForTesting(TimeUnit.DAYS.toMillis(1));
VariationsTestUtils.deleteSeeds();
}

@@ -135,8 +134,8 @@ public void tearDown() throws IOException {
@SmallTest
public void testScheduleWithNoStamp() {
try {
AwVariationsSeedFetcher.scheduleIfNeeded(FAKE_NOW_MS);
mScheduler.assertScheduledWithDelayEqualTo(0);
AwVariationsSeedFetcher.scheduleIfNeeded();
mScheduler.assertScheduled();
} finally {
mScheduler.clear();
}
@@ -152,30 +151,25 @@ public void testScheduleWithExpiredStamp() throws IOException {
Assert.assertFalse("Stamp file already exists", stamp.exists());
Assert.assertTrue("Failed to create stamp file", stamp.createNewFile());
Assert.assertTrue("Failed to set stamp time", stamp.setLastModified(0));
AwVariationsSeedFetcher.scheduleIfNeeded(FAKE_NOW_MS);
mScheduler.assertScheduledWithDelayEqualTo(0);
AwVariationsSeedFetcher.scheduleIfNeeded();
mScheduler.assertScheduled();
} finally {
mScheduler.clear();
VariationsTestUtils.deleteSeeds(); // Remove the stamp file.
}
}

// Create a stamp file with time = 7 hours ago, indicating the download job ran recently. Then
// test scheduleIfNeeded(), which should schedule the job 17 hours in the future.
// Create a stamp file with time = now, indicating the download job ran recently. Then test
// scheduleIfNeeded(), which should not schedule a job.
@Test
@MediumTest
public void testScheduleWithFreshStamp() throws IOException {
long seedAge = TimeUnit.HOURS.toMillis(7);
File stamp = VariationsUtils.getStampFile();
try {
long stampLastModified = FAKE_NOW_MS - seedAge;
Assert.assertFalse("Stamp file already exists", stamp.exists());
Assert.assertTrue("Failed to create stamp file", stamp.createNewFile());
Assert.assertTrue("Failed to set stamp time", stamp.setLastModified(stampLastModified));

AwVariationsSeedFetcher.scheduleIfNeeded(FAKE_NOW_MS);

mScheduler.assertScheduledWithDelayEqualTo(TimeUnit.HOURS.toMillis(17));
AwVariationsSeedFetcher.scheduleIfNeeded();
mScheduler.assertNotScheduled();
} finally {
mScheduler.clear();
VariationsTestUtils.deleteSeeds(); // Remove the stamp file.
@@ -192,15 +186,15 @@ public void testScheduleAlreadyScheduled() {
@SuppressLint("JobSchedulerService")
ComponentName component = new ComponentName(
ContextUtils.getApplicationContext(), AwVariationsSeedFetcher.class);
JobInfo job = new JobInfo.Builder(AwVariationsSeedFetcher.JOB_ID, component)
.setRequiredNetworkType(JobInfo.NETWORK_TYPE_ANY)
.setRequiresCharging(true)
.build();
JobInfo job = new JobInfo.Builder(JOB_ID, component)
.setRequiredNetworkType(JobInfo.NETWORK_TYPE_ANY)
.setRequiresCharging(true)
.build();
mScheduler.schedule(job);
AwVariationsSeedFetcher.scheduleIfNeeded(FAKE_NOW_MS);
AwVariationsSeedFetcher.scheduleIfNeeded();
// Check that our job object hasn't been replaced (meaning that scheduleIfNeeded didn't
// schedule a job).
Assert.assertSame(job, mScheduler.getPendingJob(AwVariationsSeedFetcher.JOB_ID));
Assert.assertSame(job, mScheduler.getPendingJob(JOB_ID));
} finally {
mScheduler.clear();
}

This file was deleted.

@@ -26,6 +26,7 @@
import org.chromium.components.version_info.VersionConstants;

import java.io.IOException;
import java.util.Date;
import java.util.concurrent.TimeUnit;

/**
@@ -41,14 +42,12 @@
*/
@TargetApi(Build.VERSION_CODES.LOLLIPOP) // for JobService
public class AwVariationsSeedFetcher extends JobService {
public static final int JOB_ID = TaskIds.WEBVIEW_VARIATIONS_SEED_FETCH_JOB_ID;
private static final long MIN_JOB_PERIOD_MILLIS = TimeUnit.DAYS.toMillis(1);

private static final String TAG = "AwVariationsSeedFet-";
private static final int JOB_ID = TaskIds.WEBVIEW_VARIATIONS_SEED_FETCH_JOB_ID;
private static final long MIN_JOB_PERIOD_MILLIS = TimeUnit.DAYS.toMillis(1);

private static JobScheduler sMockJobScheduler;
private static VariationsSeedFetcher sMockDownloader;
private static long sMinJobPeriodMillis = MIN_JOB_PERIOD_MILLIS;

private FetchTask mFetchTask;

@@ -82,7 +81,7 @@ private static JobScheduler getScheduler() {
Context.JOB_SCHEDULER_SERVICE);
}

public static void scheduleIfNeeded(long nowMs) {
public static void scheduleIfNeeded() {
JobScheduler scheduler = getScheduler();
if (scheduler == null) return;

@@ -91,30 +90,26 @@ public static void scheduleIfNeeded(long nowMs) {
return;
}

long lastRequestTimeMs = VariationsUtils.getStampTime();
// Check how long it's been since FetchTask last ran.
long lastRequestTime = VariationsUtils.getStampTime();
if (lastRequestTime != 0) {
long now = (new Date()).getTime();
if (now < lastRequestTime + MIN_JOB_PERIOD_MILLIS) {
return;
}
}

ComponentName thisComponent = new ComponentName(
ContextUtils.getApplicationContext(), AwVariationsSeedFetcher.class);
JobInfo job = new JobInfo.Builder(JOB_ID, thisComponent)
.setRequiredNetworkType(JobInfo.NETWORK_TYPE_ANY)
.setRequiresCharging(true)
.setMinimumLatency(computeJobDelay(nowMs, lastRequestTimeMs))
.build();
.setRequiredNetworkType(JobInfo.NETWORK_TYPE_ANY)
.setRequiresCharging(true)
.build();
if (scheduler.schedule(job) != JobScheduler.RESULT_SUCCESS) {
Log.e(TAG, "Failed to schedule job");
}
}

// Calculates the minimum amount of time to delay the job so we wait at least
// sMinJobPeriodMillis between requests. There should be no delay if the previous request
// occurred more than sMinJobPeriodMillis ago.
public static long computeJobDelay(long nowMs, long lastRequestTimeMs) {
if (lastRequestTimeMs == 0) {
return 0;
}
long timeSinceLastRequestMs = nowMs - lastRequestTimeMs;
return Math.max(0, sMinJobPeriodMillis - timeSinceLastRequestMs);
}

private class FetchTask extends BackgroundOnlyAsyncTask<Void> {
private JobParameters mParams;

@@ -185,8 +180,4 @@ public static void setMocks(JobScheduler scheduler, VariationsSeedFetcher fetche
sMockJobScheduler = scheduler;
sMockDownloader = fetcher;
}

public static void setMinJobPeriodMillisForTesting(long minJobPeriodMillis) {
sMinJobPeriodMillis = minJobPeriodMillis;
}
}
@@ -17,7 +17,6 @@
import java.io.File;
import java.io.FileNotFoundException;
import java.io.FileOutputStream;
import java.util.Date;

/**
* VariationsSeedHolder is a singleton which manages the local copy of the variations seed - both
@@ -148,7 +147,7 @@ public void updateSeed(SeedInfo newSeed, Runnable onFinished) {

@VisibleForTesting
public void scheduleFetchIfNeeded() {
AwVariationsSeedFetcher.scheduleIfNeeded(new Date().getTime());
AwVariationsSeedFetcher.scheduleIfNeeded();
}

// overridden by tests
@@ -190,7 +190,6 @@ instrumentation_test_apk("webview_instrumentation_test_apk") {
"//services/device/public/java:geolocation_java_test_support",
"//third_party/android_support_test_runner:rules_java",
"//third_party/android_support_test_runner:runner_java",
"//third_party/hamcrest:hamcrest_java",
"//third_party/junit",
"//third_party/metrics_proto:metrics_proto_java",
"//ui/android:ui_java",
@@ -453,7 +452,6 @@ junit_binary("android_webview_junit_tests") {
"../junit/src/org/chromium/android_webview/robolectric/FindAddressTest.java",
"../junit/src/org/chromium/android_webview/robolectric/AwScrollOffsetManagerTest.java",
"../junit/src/org/chromium/android_webview/robolectric/common/services/ServiceNamesTest.java",
"../junit/src/org/chromium/android_webview/robolectric/services/AwVariationsSeedFetcherTest.java",
]

deps = [

0 comments on commit 45df6ff

Please sign in to comment.
You can’t perform that action at this time.