Pass calls to GetDocuments through the scheduler

Pass calls to GetDocuments through the scheduler

R=kinaba@chromium.org
BUG=160904
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170712

[kinaba, 2012-11-22 08:04:07]

+satorux for MockDriveService use (http://crbug.com/162350).
How should we proceed?

https://codereview.chromium.org/11418127/diff/2001/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_scheduler.cc (right):

https://codereview.chromium.org/11418127/diff/2001/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_scheduler.cc:364: void
DriveScheduler::OnFileOperationJobDone(int job_id, DriveFileError error) {
This duplication worries me a bit. How about factoring the queue operation part
out like:

scoped_ptr<QueueEntry> OnJobDone(job_id, error) {
  retry and return null;
  or, erase from job_info_map and return the entry;
}

void OnFileOperationJobDone(...) {
  job = OnJobDone(...);
  if (job) { callback; }
}

void OnGetDataJobDone(...) { same }

https://codereview.chromium.org/11418127/diff/2001/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_scheduler_unittest.cc (right):

https://codereview.chromium.org/11418127/diff/2001/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_scheduler_unittest.cc:106: //
DriveFileSystem object, which will own the mock object.
IIUC, DriveFileSystem does not own DriveService (that's the reason it has to be
deleted in TearDown).

https://codereview.chromium.org/11418127/diff/2001/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_scheduler_unittest.cc:107:
mock_drive_service_ = new StrictMock<google_apis::MockDriveService>;
Satorux is deprecating MockDriveService http://crbug.com/162350. We may want to
coordinate on how to write the tests depending on it...

[satorux1, 2012-11-22 08:12:44]

https://codereview.chromium.org/11418127/diff/2001/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_scheduler_unittest.cc (right):

https://codereview.chromium.org/11418127/diff/2001/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_scheduler_unittest.cc:266:
google_apis::test_util::RunBlockingPoolTask();
I think we should check the results we get by GetDocuments.

To do that, we need a fake DriveService implementation (not a mock), that
returns something meaningful.

For implementing a fake, see this patch, which does something similar:
https://codereview.chromium.org/11414126/diff/5002/chrome/browser/chromeos/dr...

To get the results from GetDataCallback, we should use
CopyResultsFromGetDataCallback(), which is currently defined in
base_operations_unittest.cc. We should move this to c/b/google_apis/test_util.h,
so we can use it from here.

[Zachary Kuznia, 2012-11-28 06:54:56]

https://codereview.chromium.org/11418127/diff/2001/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_scheduler.cc (right):

https://codereview.chromium.org/11418127/diff/2001/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_scheduler.cc:364: void
DriveScheduler::OnFileOperationJobDone(int job_id, DriveFileError error) {
On 2012/11/22 08:04:07, kinaba wrote:
> This duplication worries me a bit. How about factoring the queue operation
part
> out like:
> 
> scoped_ptr<QueueEntry> OnJobDone(job_id, error) {
>   retry and return null;
>   or, erase from job_info_map and return the entry;
> }
> 
> void OnFileOperationJobDone(...) {
>   job = OnJobDone(...);
>   if (job) { callback; }
> }
> 
> void OnGetDataJobDone(...) { same }

Done.

https://codereview.chromium.org/11418127/diff/2001/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_scheduler_unittest.cc (right):

https://codereview.chromium.org/11418127/diff/2001/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_scheduler_unittest.cc:106: //
DriveFileSystem object, which will own the mock object.
On 2012/11/22 08:04:07, kinaba wrote:
> IIUC, DriveFileSystem does not own DriveService (that's the reason it has to
be
> deleted in TearDown).

Removed the comment.

https://codereview.chromium.org/11418127/diff/2001/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_scheduler_unittest.cc:266:
google_apis::test_util::RunBlockingPoolTask();
On 2012/11/22 08:12:45, satorux1 wrote:
> I think we should check the results we get by GetDocuments.
> 
> To do that, we need a fake DriveService implementation (not a mock), that
> returns something meaningful.
> 
> For implementing a fake, see this patch, which does something similar:
>
https://codereview.chromium.org/11414126/diff/5002/chrome/browser/chromeos/dr...
> 
> To get the results from GetDataCallback, we should use
> CopyResultsFromGetDataCallback(), which is currently defined in
> base_operations_unittest.cc. We should move this to
c/b/google_apis/test_util.h,
> so we can use it from here.

Done.

[satorux1, 2012-11-28 23:59:47]

https://codereview.chromium.org/11418127/diff/2002/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_scheduler.cc (right):

https://codereview.chromium.org/11418127/diff/2002/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_scheduler.cc:231: JobInfo& job_info =
job_iter->second->job_info;
Can you make it const? We rarely use a non-const reference.

https://codereview.chromium.org/11418127/diff/2002/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_scheduler.cc:233: 
What about adding:

const QueueEntry& query_entry = *job_iter->second;

so we don't repeat  job_iter->second in many places below

https://codereview.chromium.org/11418127/diff/2002/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_scheduler.cc:361:
base::MessageLoopProxy::current()->PostNonNestableTask(
Could you add some comment about why you don't call DoJobLoop() directly here?

Besides, why not just PostTask()?

https://codereview.chromium.org/11418127/diff/2002/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_scheduler.cc:394:
scoped_ptr<DriveScheduler::QueueEntry> job_info(OnJobDone(job_id, error));
nit: When taking an object from a function returning scoped_ptr<>, please use
'='. This makes it clear that the function is returning a scoped_ptr (otherwise,
it won't compile)

scoped_ptr<DriveScheduler::QueueEntry> job_info = OnJobDone(job_id, error);

https://codereview.chromium.org/11418127/diff/2002/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_scheduler.cc:396: if (job_info.get()) {
nit:

if (!job_info)
  return;

to save one indent level.

Please omit .get() from boolean context. That's a new preferred manner.

https://codereview.chromium.org/11418127/diff/2002/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_scheduler.cc:410: scoped_ptr<QueueEntry>
job_info(OnJobDone(job_id, drive_error));
ditto.

https://codereview.chromium.org/11418127/diff/2002/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_scheduler.cc:412: if (job_info.get()) {
ditto.

https://codereview.chromium.org/11418127/diff/2002/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_scheduler_unittest.cc (right):

https://codereview.chromium.org/11418127/diff/2002/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_scheduler_unittest.cc:81:
MessageLoop::current()->PostTask(FROM_HERE,
Please use MessageLoopProxy instead, to be consistent with other pieces of code
in 'drive'

https://codereview.chromium.org/11418127/diff/2002/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_scheduler_unittest.cc:401:
ASSERT_EQ(google_apis::HTTP_SUCCESS, error);
ASSERT_TRUE(value);

https://codereview.chromium.org/11418127/diff/2002/chrome/browser/google_apis...
File chrome/browser/google_apis/test_util.h (left):

https://codereview.chromium.org/11418127/diff/2002/chrome/browser/google_apis...
chrome/browser/google_apis/test_util.h:8: #include
"chrome/browser/google_apis/test_util.h"
wow, that was recursive!

[Zachary Kuznia, 2012-11-30 05:42:30]

https://codereview.chromium.org/11418127/diff/2002/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_scheduler.cc (right):

https://codereview.chromium.org/11418127/diff/2002/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_scheduler.cc:231: JobInfo& job_info =
job_iter->second->job_info;
On 2012/11/28 23:59:47, satorux1 wrote:
> Can you make it const? We rarely use a non-const reference.

Unfortunately, making it const makes the next line fail.  I think it's ok to use
a non-const reference, because it's not a function argument, so it shouldn't
violate the style guide.

https://codereview.chromium.org/11418127/diff/2002/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_scheduler.cc:233: 
On 2012/11/28 23:59:47, satorux1 wrote:
> What about adding:
> 
> const QueueEntry& query_entry = *job_iter->second;
> 
> so we don't repeat  job_iter->second in many places below

Done.

https://codereview.chromium.org/11418127/diff/2002/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_scheduler.cc:361:
base::MessageLoopProxy::current()->PostNonNestableTask(
On 2012/11/28 23:59:47, satorux1 wrote:
> Could you add some comment about why you don't call DoJobLoop() directly here?
> 
> Besides, why not just PostTask()?

Changed.  Done.

https://codereview.chromium.org/11418127/diff/2002/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_scheduler.cc:394:
scoped_ptr<DriveScheduler::QueueEntry> job_info(OnJobDone(job_id, error));
On 2012/11/28 23:59:47, satorux1 wrote:
> nit: When taking an object from a function returning scoped_ptr<>, please use
> '='. This makes it clear that the function is returning a scoped_ptr
(otherwise,
> it won't compile)
> 
> scoped_ptr<DriveScheduler::QueueEntry> job_info = OnJobDone(job_id, error);

Done.

https://codereview.chromium.org/11418127/diff/2002/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_scheduler.cc:396: if (job_info.get()) {
On 2012/11/28 23:59:47, satorux1 wrote:
> nit:
> 
> if (!job_info)
>   return;
> 
> to save one indent level.
> 
> Please omit .get() from boolean context. That's a new preferred manner.

Done.

https://codereview.chromium.org/11418127/diff/2002/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_scheduler.cc:410: scoped_ptr<QueueEntry>
job_info(OnJobDone(job_id, drive_error));
On 2012/11/28 23:59:47, satorux1 wrote:
> ditto.

Done.

https://codereview.chromium.org/11418127/diff/2002/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_scheduler.cc:412: if (job_info.get()) {
On 2012/11/28 23:59:47, satorux1 wrote:
> ditto.

Done.

https://codereview.chromium.org/11418127/diff/2002/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_scheduler_unittest.cc (right):

https://codereview.chromium.org/11418127/diff/2002/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_scheduler_unittest.cc:81:
MessageLoop::current()->PostTask(FROM_HERE,
On 2012/11/28 23:59:47, satorux1 wrote:
> Please use MessageLoopProxy instead, to be consistent with other pieces of
code
> in 'drive'

Done.

https://codereview.chromium.org/11418127/diff/2002/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_scheduler_unittest.cc:401:
ASSERT_EQ(google_apis::HTTP_SUCCESS, error);
On 2012/11/28 23:59:47, satorux1 wrote:
> ASSERT_TRUE(value);

Done.

[satorux1, 2012-12-02 23:29:02]

LGTM

[kinaba, 2012-12-03 00:32:29]

LGTM +1

[commit-bot: I haz the power, 2012-12-03 03:56:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zork@chromium.org/11418127/15002

[commit-bot: I haz the power, 2012-12-03 04:27:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zork@chromium.org/11418127/9005

[commit-bot: I haz the power, 2012-12-03 08:33:39]

Change committed as 170712