Add an observer interface for DriveScheduler.

Add an observer interface for DriveScheduler.

This patch is just for setting up the interface. Actual code
to fire each event will come soon.

BUG=154243
TBR=sky@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=194551

[kinaba, 2013-04-16 06:37:02]

Extracted the job list observation part into an interface.
I'd like to expose the interface from DriveSystemService in
a separate patch.

[tfarina, 2013-04-16 14:07:28]

https://codereview.chromium.org/14148006/diff/12001/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/drive_job_list_interface.h (right):

https://codereview.chromium.org/14148006/diff/12001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_job_list_interface.h:80: class
DriveJobListObserver {
does this needs a protected virtual destructor?

[satorux1, 2013-04-17 00:11:29]

https://codereview.chromium.org/14148006/diff/12001/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/drive_job_list_interface.h (right):

https://codereview.chromium.org/14148006/diff/12001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_job_list_interface.h:67: int
completed_bytes;
maybe num_completed_bytes

https://codereview.chromium.org/14148006/diff/12001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_job_list_interface.h:70: int total_bytes;
add num_ ?

https://codereview.chromium.org/14148006/diff/12001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_job_list_interface.h:80: class
DriveJobListObserver {
Let's drop 'Drive' prefix

https://codereview.chromium.org/14148006/diff/12001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_job_list_interface.h:89: virtual void
OnJobUpdated(const JobInfo& job_info) = 0;
Not sure if all of these should be pure virtual. Are observers always interested
in all of the events?

https://codereview.chromium.org/14148006/diff/12001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_job_list_interface.h:93: class
DriveJobListInterface {
Ditto. Drop 'Drive'. Please change the file names too.

https://codereview.chromium.org/14148006/diff/12001/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/drive_scheduler.cc (right):

https://codereview.chromium.org/14148006/diff/12001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_scheduler.cc:917: OnJobUpdated(job_info));
These are not used in this patch. Intentional?

[kinaba, 2013-04-17 00:51:42]

https://codereview.chromium.org/14148006/diff/12001/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/drive_job_list_interface.h (right):

https://codereview.chromium.org/14148006/diff/12001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_job_list_interface.h:67: int
completed_bytes;
On 2013/04/17 00:11:29, satorux1 wrote:
> maybe num_completed_bytes

Done. Along the way changed the type to int64.

https://codereview.chromium.org/14148006/diff/12001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_job_list_interface.h:70: int total_bytes;
On 2013/04/17 00:11:29, satorux1 wrote:
> add num_ ?

Done.

https://codereview.chromium.org/14148006/diff/12001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_job_list_interface.h:80: class
DriveJobListObserver {
On 2013/04/17 00:11:29, satorux1 wrote:
> Let's drop 'Drive' prefix

Done.

https://codereview.chromium.org/14148006/diff/12001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_job_list_interface.h:80: class
DriveJobListObserver {
On 2013/04/16 14:07:28, tfarina wrote:
> does this needs a protected virtual destructor?

Done. Thanks.

https://codereview.chromium.org/14148006/diff/12001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_job_list_interface.h:89: virtual void
OnJobUpdated(const JobInfo& job_info) = 0;
On 2013/04/17 00:11:29, satorux1 wrote:
> Not sure if all of these should be pure virtual. Are observers always
interested
> in all of the events? 

Not sure but there may be a case that some of them are not needed.
Changed to empty body.

https://codereview.chromium.org/14148006/diff/12001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_job_list_interface.h:93: class
DriveJobListInterface {
On 2013/04/17 00:11:29, satorux1 wrote:
> Ditto. Drop 'Drive'. Please change the file names too.

Done.

https://codereview.chromium.org/14148006/diff/12001/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/drive_scheduler.cc (right):

https://codereview.chromium.org/14148006/diff/12001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_scheduler.cc:917: OnJobUpdated(job_info));
On 2013/04/17 00:11:29, satorux1 wrote:
> These are not used in this patch. Intentional?

Intentional (especially for NotifyJobUpdated, not all the code
is ready for updating jobinfo). I'll add them in a separate patch.

[satorux1, 2013-04-17 00:53:57]

LGTM with a nit

https://codereview.chromium.org/14148006/diff/20001/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/drive_scheduler.h (right):

https://codereview.chromium.org/14148006/diff/20001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_scheduler.h:29: // of each.  See:
crbug.com/154243
remove the TODO, now you are adding such an interface?

[kinaba, 2013-04-17 01:03:54]

https://codereview.chromium.org/14148006/diff/20001/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/drive_scheduler.h (right):

https://codereview.chromium.org/14148006/diff/20001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_scheduler.h:29: // of each.  See:
crbug.com/154243
On 2013/04/17 00:53:57, satorux1 wrote:
> remove the TODO, now you are adding such an interface?

Done. Good catch.

[kinaba, 2013-04-17 01:16:48]

TBR'ing +sky for the gyp change.

[satorux1, 2013-04-17 03:34:59]

https://codereview.chromium.org/14148006/diff/37001/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/job_list_interface.h (right):

https://codereview.chromium.org/14148006/diff/37001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/job_list_interface.h:102: virtual
std::vector<JobInfo> GetJobInfoList() = 0;
const method?

[kinaba, 2013-04-17 05:39:08]

https://codereview.chromium.org/14148006/diff/37001/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/job_list_interface.h (right):

https://codereview.chromium.org/14148006/diff/37001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/job_list_interface.h:102: virtual
std::vector<JobInfo> GetJobInfoList() = 0;
On 2013/04/17 03:34:59, satorux1 wrote:
> const method?

I wish if I could make it const, but the problem is
const base::IDMap doesn't support enumeration of elements.

[satorux1, 2013-04-17 06:20:36]

On 2013/04/17 05:39:08, kinaba wrote:
>
https://codereview.chromium.org/14148006/diff/37001/chrome/browser/chromeos/d...
> File chrome/browser/chromeos/drive/job_list_interface.h (right):
> 
>
https://codereview.chromium.org/14148006/diff/37001/chrome/browser/chromeos/d...
> chrome/browser/chromeos/drive/job_list_interface.h:102: virtual
> std::vector<JobInfo> GetJobInfoList() = 0;
> On 2013/04/17 03:34:59, satorux1 wrote:
> > const method?
> 
> I wish if I could make it const, but the problem is
> const base::IDMap doesn't support enumeration of elements.

NP. LGTM

[kinaba, 2013-04-17 09:18:10]

Committed patchset #7 manually as r194551 (presubmit successful).