Add Move operation to the the scheduler

Add Move operation to the the scheduler

BUG=156003
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=166408

[Zachary Kuznia, 2012-10-16 04:23:06]

Hi Satoru,

Could you review this?

Thanks,
-Zach

[satorux1, 2012-10-16 04:34:01]

http://codereview.chromium.org/11142036/diff/1/chrome/browser/chromeos/drive/...
File chrome/browser/chromeos/drive/drive_scheduler.cc (right):

http://codereview.chromium.org/11142036/diff/1/chrome/browser/chromeos/drive/...
chrome/browser/chromeos/drive/drive_scheduler.cc:76: QueueEntry* new_job = new
QueueEntry(TYPE_MOVE, src_file_path, callback);
This made me think how the object is deleted. This will be added to job info map
and be deleted as the value is linked_ptr, right? Please add some comment about
lifetime of the object.

Alternatively, you might want to use scoped_ptr<> here, and pass it to
QueueJob() using Pass(). This will make ownership transfer clearer.

http://codereview.chromium.org/11142036/diff/1/chrome/browser/chromeos/drive/...
chrome/browser/chromeos/drive/drive_scheduler.cc:89: QueueEntry* new_job = new
QueueEntry(TYPE_REMOVE, file_path, callback);
ditto.

http://codereview.chromium.org/11142036/diff/1/chrome/browser/chromeos/drive/...
chrome/browser/chromeos/drive/drive_scheduler.cc:158: break;
Please add:

default:
  NOTREACHED() << "Unknown job type: " << job_info.job_type

http://codereview.chromium.org/11142036/diff/1/chrome/browser/chromeos/drive/...
File chrome/browser/chromeos/drive/drive_scheduler.h (right):

http://codereview.chromium.org/11142036/diff/1/chrome/browser/chromeos/drive/...
chrome/browser/chromeos/drive/drive_scheduler.h:165: JobMap job_info_;
job_info_map_ would be more descriptive.

[Zachary Kuznia, 2012-10-16 04:59:10]

https://codereview.chromium.org/11142036/diff/1/chrome/browser/chromeos/drive...
File chrome/browser/chromeos/drive/drive_scheduler.cc (right):

https://codereview.chromium.org/11142036/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/drive_scheduler.cc:76: QueueEntry* new_job = new
QueueEntry(TYPE_MOVE, src_file_path, callback);
On 2012/10/16 04:34:01, satorux1 wrote:
> This made me think how the object is deleted. This will be added to job info
map
> and be deleted as the value is linked_ptr, right? Please add some comment
about
> lifetime of the object.
> 
> Alternatively, you might want to use scoped_ptr<> here, and pass it to
> QueueJob() using Pass(). This will make ownership transfer clearer.

Done.

https://codereview.chromium.org/11142036/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/drive_scheduler.cc:89: QueueEntry* new_job = new
QueueEntry(TYPE_REMOVE, file_path, callback);
On 2012/10/16 04:34:01, satorux1 wrote:
> ditto.

Done.

https://codereview.chromium.org/11142036/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/drive_scheduler.cc:158: break;
On 2012/10/16 04:34:01, satorux1 wrote:
> Please add:
> 
> default:
>   NOTREACHED() << "Unknown job type: " << job_info.job_type

By leaving out a default case, the compiler will complain if a type is
unhandled.

https://codereview.chromium.org/11142036/diff/1/chrome/browser/chromeos/drive...
File chrome/browser/chromeos/drive/drive_scheduler.h (right):

https://codereview.chromium.org/11142036/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/drive_scheduler.h:165: JobMap job_info_;
On 2012/10/16 04:34:01, satorux1 wrote:
> job_info_map_ would be more descriptive.

Done.

[satorux1, 2012-10-16 05:10:36]

LGTM with nits

http://codereview.chromium.org/11142036/diff/1/chrome/browser/chromeos/drive/...
File chrome/browser/chromeos/drive/drive_scheduler.cc (right):

http://codereview.chromium.org/11142036/diff/1/chrome/browser/chromeos/drive/...
chrome/browser/chromeos/drive/drive_scheduler.cc:158: break;
On 2012/10/16 04:59:10, Zachary Kuznia wrote:
> On 2012/10/16 04:34:01, satorux1 wrote:
> > Please add:
> > 
> > default:
> >   NOTREACHED() << "Unknown job type: " << job_info.job_type
> 
> By leaving out a default case, the compiler will complain if a type is
> unhandled.

I see. Then please add some comment about it.

http://codereview.chromium.org/11142036/diff/5001/chrome/browser/chromeos/dri...
File chrome/browser/chromeos/drive/drive_scheduler_unittest.cc (right):

http://codereview.chromium.org/11142036/diff/5001/chrome/browser/chromeos/dri...
chrome/browser/chromeos/drive/drive_scheduler_unittest.cc:147: DriveFileError
error;
nit: initialize this with DRIVE_FILE_FAILED?

http://codereview.chromium.org/11142036/diff/5001/chrome/browser/chromeos/dri...
chrome/browser/chromeos/drive/drive_scheduler_unittest.cc:161:
EXPECT_CALL(*mock_move_operation_, Move(file_in_root, dest_file, _))
Please add some comment explaining what you are testing (retry due to throttle)

http://codereview.chromium.org/11142036/diff/5001/chrome/browser/chromeos/dri...
chrome/browser/chromeos/drive/drive_scheduler_unittest.cc:165: DriveFileError
error;
nit: initialize this with DRIVE_FILE_FAILED?

[Zachary Kuznia, 2012-11-07 05:46:17]

http://codereview.chromium.org/11142036/diff/1/chrome/browser/chromeos/drive/...
File chrome/browser/chromeos/drive/drive_scheduler.cc (right):

http://codereview.chromium.org/11142036/diff/1/chrome/browser/chromeos/drive/...
chrome/browser/chromeos/drive/drive_scheduler.cc:158: break;
On 2012/10/16 05:10:37, satorux1 wrote:
> On 2012/10/16 04:59:10, Zachary Kuznia wrote:
> > On 2012/10/16 04:34:01, satorux1 wrote:
> > > Please add:
> > > 
> > > default:
> > >   NOTREACHED() << "Unknown job type: " << job_info.job_type
> > 
> > By leaving out a default case, the compiler will complain if a type is
> > unhandled.
> 
> I see. Then please add some comment about it.

Done.

http://codereview.chromium.org/11142036/diff/5001/chrome/browser/chromeos/dri...
File chrome/browser/chromeos/drive/drive_scheduler_unittest.cc (right):

http://codereview.chromium.org/11142036/diff/5001/chrome/browser/chromeos/dri...
chrome/browser/chromeos/drive/drive_scheduler_unittest.cc:147: DriveFileError
error;
On 2012/10/16 05:10:37, satorux1 wrote:
> nit: initialize this with DRIVE_FILE_FAILED?

Done.

http://codereview.chromium.org/11142036/diff/5001/chrome/browser/chromeos/dri...
chrome/browser/chromeos/drive/drive_scheduler_unittest.cc:161:
EXPECT_CALL(*mock_move_operation_, Move(file_in_root, dest_file, _))
On 2012/10/16 05:10:37, satorux1 wrote:
> Please add some comment explaining what you are testing (retry due to
throttle)

Done.

http://codereview.chromium.org/11142036/diff/5001/chrome/browser/chromeos/dri...
chrome/browser/chromeos/drive/drive_scheduler_unittest.cc:165: DriveFileError
error;
On 2012/10/16 05:10:37, satorux1 wrote:
> nit: initialize this with DRIVE_FILE_FAILED?

Done.

[commit-bot: I haz the power, 2012-11-07 05:46:28]

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

[commit-bot: I haz the power, 2012-11-07 13:40:38]

Change committed as 166408