Make FileTaskExecutor testable.

Make FileTaskExecutor testable.

The existing implementation directly tries to open browser windows and
relies on profiles to obtain Drive related services.

The CL inserts an indirection to those places and make it easy to
test, i.e., to verify the correctness of opened URLs and to
insert test fake Drive instances.

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

[kinaba, 2014-07-15 08:56:49]

ptal. The actual test cases are coming as a separate CL.

[hirono, 2014-07-16 03:33:52]

Sorry for late.

https://codereview.chromium.org/391003006/diff/1/chrome/browser/chromeos/driv...
File chrome/browser/chromeos/drive/file_task_executor.cc (left):

https://codereview.chromium.org/391003006/diff/1/chrome/browser/chromeos/driv...
chrome/browser/chromeos/drive/file_task_executor.cc:103: if (!service ||
!service->IsMounted() ||
Can we remove IsMounted check?

https://codereview.chromium.org/391003006/diff/1/chrome/browser/chromeos/driv...
File chrome/browser/chromeos/drive/file_task_executor.cc (right):

https://codereview.chromium.org/391003006/diff/1/chrome/browser/chromeos/driv...
chrome/browser/chromeos/drive/file_task_executor.cc:55: Profile* profile_;
nit: Profile* const

https://codereview.chromium.org/391003006/diff/1/chrome/browser/chromeos/driv...
chrome/browser/chromeos/drive/file_task_executor.cc:96: FileSystemInterface*
file_system = delegate_->GetFileSystem();
nit: FileSystemInterface* const

https://codereview.chromium.org/391003006/diff/1/chrome/browser/chromeos/driv...
chrome/browser/chromeos/drive/file_task_executor.cc:120: DriveServiceInterface*
drive_service = delegate_->GetDriveService();
nit: DriveServiceInterface* const

[kinaba, 2014-07-16 04:11:03]

https://codereview.chromium.org/391003006/diff/1/chrome/browser/chromeos/driv...
File chrome/browser/chromeos/drive/file_task_executor.cc (left):

https://codereview.chromium.org/391003006/diff/1/chrome/browser/chromeos/driv...
chrome/browser/chromeos/drive/file_task_executor.cc:103: if (!service ||
!service->IsMounted() ||
On 2014/07/16 03:33:51, hirono wrote:
> Can we remove IsMounted check?
Yes, I believe so.

If IsMounted is false, GetDriveService() in OnFileEntryFetched returns NULL.
So basically, we don't reach this point in the case.

The only subtle case is that when the user disabled Drive integration during
this async call (which is the only possibility that turns IsMounted off).
The link can be opened safely even in this case. And I think it's ok to open
the link, since that's the user indicated before (s)he disabled. Of course,
re-checking IsMounted flag here is also a possible choice, but I'd like to
prefer
simplicity.

https://codereview.chromium.org/391003006/diff/1/chrome/browser/chromeos/driv...
File chrome/browser/chromeos/drive/file_task_executor.cc (right):

https://codereview.chromium.org/391003006/diff/1/chrome/browser/chromeos/driv...
chrome/browser/chromeos/drive/file_task_executor.cc:55: Profile* profile_;
On 2014/07/16 03:33:51, hirono wrote:
> nit: Profile* const

Done.

https://codereview.chromium.org/391003006/diff/1/chrome/browser/chromeos/driv...
chrome/browser/chromeos/drive/file_task_executor.cc:96: FileSystemInterface*
file_system = delegate_->GetFileSystem();
On 2014/07/16 03:33:51, hirono wrote:
> nit: FileSystemInterface* const

Done.

https://codereview.chromium.org/391003006/diff/1/chrome/browser/chromeos/driv...
chrome/browser/chromeos/drive/file_task_executor.cc:120: DriveServiceInterface*
drive_service = delegate_->GetDriveService();
On 2014/07/16 03:33:51, hirono wrote:
> nit: DriveServiceInterface* const

Done.

[hirono, 2014-07-16 04:12:46]

lgtm!

https://codereview.chromium.org/391003006/diff/1/chrome/browser/chromeos/driv...
File chrome/browser/chromeos/drive/file_task_executor.cc (left):

https://codereview.chromium.org/391003006/diff/1/chrome/browser/chromeos/driv...
chrome/browser/chromeos/drive/file_task_executor.cc:103: if (!service ||
!service->IsMounted() ||
On 2014/07/16 04:11:03, kinaba wrote:
> On 2014/07/16 03:33:51, hirono wrote:
> > Can we remove IsMounted check?
> Yes, I believe so.
> 
> If IsMounted is false, GetDriveService() in OnFileEntryFetched returns NULL.
> So basically, we don't reach this point in the case.
> 
> The only subtle case is that when the user disabled Drive integration during
> this async call (which is the only possibility that turns IsMounted off).
> The link can be opened safely even in this case. And I think it's ok to open
> the link, since that's the user indicated before (s)he disabled. Of course,
> re-checking IsMounted flag here is also a possible choice, but I'd like to
> prefer
> simplicity.

Got it. Thank you!

[kinaba, 2014-07-16 04:14:25]

The CQ bit was checked by kinaba@chromium.org

[commit-bot: I haz the power, 2014-07-16 04:22:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kinaba@chromium.org/391003006/20001

[commit-bot: I haz the power, 2014-07-16 21:14:37]

Change committed as 283506