New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 729156 link

Starred by 1 user
Status: Fixed
Owner:
█ nyquist@chromium.org
Traveling - Back 2/6
Closed: Aug 2017
Cc:
fgor...@chromium.org
█ awdf@chromium.org
dtrainor@chromium.org
█ nyquist@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug



Sign in to add a comment

BackgroundTaskScheduler should be a pure virtual interface

Project Member Reported by nyquist@chromium.org, Jun 2 2017 
Chrome Version: daec770ed8e9a12165629a75e2a30d192ea6865c
OS: Android

What steps will reproduce the problem?
(1) Try to create test using BackgroundTaskScheduler

What is the expected result?
It should be easy to write a test without mocking.

What happens instead?
It's hard, since you can't construct it easily.

Suggested fix:
- Rename BackgroundTaskScheduler to BackgroundTaskSchedulerImpl, and make BackgroundTaskSchedulerFactory create that instead.
- Create an interface BackgroundTaskScheduler that BackgroundTaskSchedulerImpl implements.
- Change the return type of BackgroundTaskSchedulerFactory#getScheduler() to be BackgroundTaskScheduler.
- Ensure that BackgroundTaskSchedulerDelegate and BackgroundTaskSchedulerImpl are package protected.
Project Member

Comment 1 by bugdroid1@chromium.org, Aug 16 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c2501b459b6f4ee75b89846b0d0dc0f1b918135e

commit c2501b459b6f4ee75b89846b0d0dc0f1b918135e
Author: Tommy Nyquist <nyquist@chromium.org>
Date: Wed Aug 16 18:58:29 2017

Make BackgroundTaskScheduler become a pure virtual interface.

The BackgroundTaskScheduler is currently a concrete class, which hampers
the ability to use it in tests outside of the component itself.

This CL moves the BackgroundTaskScheduler implementation to a new class
BackgroundTaskScedulerImpl, and makes BackgroundTaskScheduler become
just pure virtual interface.

This made it possible to make both the *Impl and *Delegate classes
become package protected as well, and simplified some tests in the
//chrome layer.

Lastly, it creates a new class BackgroundTaskReflection which contains
functionality related to inspect and create BackgroundTask instances,
and it was used outside of the BackgroundTaskSchedulerImpl itself.

BUG= 729156 

Change-Id: Ide01a6e12176fb4cf910634f06f5681c04ec38e8
Reviewed-on: https://chromium-review.googlesource.com/612127
Reviewed-by: Filip Gorski <fgorski@chromium.org>
Commit-Queue: Tommy Nyquist <nyquist@chromium.org>
Cr-Commit-Position: refs/heads/master@{#494887}
[modify] https://crrev.com/c2501b459b6f4ee75b89846b0d0dc0f1b918135e/chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/prefetch/PrefetchBackgroundTaskTest.java
[modify] https://crrev.com/c2501b459b6f4ee75b89846b0d0dc0f1b918135e/chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/prefetch/PrefetchBackgroundTaskUnitTest.java
[modify] https://crrev.com/c2501b459b6f4ee75b89846b0d0dc0f1b918135e/components/background_task_scheduler/BUILD.gn
[add] https://crrev.com/c2501b459b6f4ee75b89846b0d0dc0f1b918135e/components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskReflection.java
[modify] https://crrev.com/c2501b459b6f4ee75b89846b0d0dc0f1b918135e/components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskScheduler.java
[modify] https://crrev.com/c2501b459b6f4ee75b89846b0d0dc0f1b918135e/components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerDelegate.java
[modify] https://crrev.com/c2501b459b6f4ee75b89846b0d0dc0f1b918135e/components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerFactory.java
[modify] https://crrev.com/c2501b459b6f4ee75b89846b0d0dc0f1b918135e/components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerGcmNetworkManager.java
[add] https://crrev.com/c2501b459b6f4ee75b89846b0d0dc0f1b918135e/components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerImpl.java
[modify] https://crrev.com/c2501b459b6f4ee75b89846b0d0dc0f1b918135e/components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerJobService.java
[modify] https://crrev.com/c2501b459b6f4ee75b89846b0d0dc0f1b918135e/components/background_task_scheduler/android/junit/src/org/chromium/components/background_task_scheduler/BackgroundTaskGcmTaskServiceTest.java
[rename] https://crrev.com/c2501b459b6f4ee75b89846b0d0dc0f1b918135e/components/background_task_scheduler/android/junit/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerImplTest.java

Comment 2 by nyquist@chromium.org, Aug 16 2017

Status: Fixed (was: Assigned)

Comment 3 by nyquist@chromium.org, Sep 11 2017

Components: Internals>BackgroundTaskScheduler
►

Sign in to add a comment