gdata: Fix "save as pdf" to work on Google Drive.

gdata: Fix "save as pdf" to work on Google Drive in ChromeOS.

BUG=137411
TEST=unit_tests --gtest_filter='*GDataFileWritingTest*'
TEST=press Ctrl+P and then the save button, choose Google Drive, and verify it is correctly saved.

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

[kinaba, 2012-07-27 10:22:07]

Satorux, I'd first like to ask if this approach is fine from GData side, can you
take a look?

[satorux1, 2012-07-27 22:56:29]

http://codereview.chromium.org/10827068/diff/9001/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/10827068/diff/9001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:4200: GDataFileError result)
{
result -> error

http://codereview.chromium.org/10827068/diff/9001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:4207: base::Bind(callback,
result, FilePath()));
we should just run the callback here.

PostTask should rarely be used. Maybe we should check and fix the existing
usage. I'll file a bug.

http://codereview.chromium.org/10827068/diff/9001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:4222: GDataFileError result,
error

http://codereview.chromium.org/10827068/diff/9001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:4228:
content::BrowserThread::GetBlockingPool()->PostTask(
ditto

http://codereview.chromium.org/10827068/diff/9001/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_file_system_interface.h (right):

http://codereview.chromium.org/10827068/diff/9001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system_interface.h:186: const
OpenFileCallback& callback) = 0;
We want to minimize functions in GDataFileSystem. Can you implement this outside
of GDataFileSystem?

Besides, please write a unit test for the new function.

[kinaba, 2012-07-28 00:06:10]

http://codereview.chromium.org/10827068/diff/9001/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_file_system_interface.h (right):

http://codereview.chromium.org/10827068/diff/9001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system_interface.h:186: const
OpenFileCallback& callback) = 0;
On 2012/07/27 22:56:29, satorux1 wrote:
> We want to minimize functions in GDataFileSystem. Can you implement this
outside
> of GDataFileSystem?

I'll think about it. Do you have any advice where it would be the better place
to put it?
I currently chose to place it inside GDataFileSystem for safely getting (or not
getting, when already destructed) pointer to it for calling CloseFile().

> 
> Besides, please write a unit test for the new function.

[satorux1, 2012-07-28 00:44:07]

http://codereview.chromium.org/10827068/diff/9001/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_file_system_interface.h (right):

http://codereview.chromium.org/10827068/diff/9001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system_interface.h:186: const
OpenFileCallback& callback) = 0;
On 2012/07/28 00:06:10, kinaba wrote:
> On 2012/07/27 22:56:29, satorux1 wrote:
> > We want to minimize functions in GDataFileSystem. Can you implement this
> outside
> > of GDataFileSystem?
> 
> I'll think about it. Do you have any advice where it would be the better place
> to put it?
> I currently chose to place it inside GDataFileSystem for safely getting (or
not
> getting, when already destructed) pointer to it for calling CloseFile().

I guess we can move this to gdata_util.cc. For CloseFile(), we should probably
check if GDataSystemService::file_system() is non-null before calling it. I'm
not sure if it's feasible, though.

You might also want to make it a class and put into new files. gdata_util.cc is
also getting bigger. :P

> 
> > 
> > Besides, please write a unit test for the new function.

[kinaba, 2012-08-01 13:48:45]

http://codereview.chromium.org/10827068/diff/9001/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/10827068/diff/9001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:4200: GDataFileError result)
{
On 2012/07/27 22:56:29, satorux1 wrote:
> result -> error

Done.

http://codereview.chromium.org/10827068/diff/9001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:4207: base::Bind(callback,
result, FilePath()));
On 2012/07/27 22:56:29, satorux1 wrote:
> we should just run the callback here.
> 
> PostTask should rarely be used. Maybe we should check and fix the existing
> usage. I'll file a bug.

This is not a PostTask to the current thread, it is posting to blocking thread
pool (since |callback| needs to be called on that thread in successful cases,
I'd like to align the failure case to it.)

http://codereview.chromium.org/10827068/diff/9001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:4222: GDataFileError result,
On 2012/07/27 22:56:29, satorux1 wrote:
> error

Done.

http://codereview.chromium.org/10827068/diff/9001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:4228:
content::BrowserThread::GetBlockingPool()->PostTask(
On 2012/07/27 22:56:29, satorux1 wrote:
> ditto

See the reply above.

http://codereview.chromium.org/10827068/diff/9001/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_file_system_interface.h (right):

http://codereview.chromium.org/10827068/diff/9001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system_interface.h:186: const
OpenFileCallback& callback) = 0;
On 2012/07/28 00:44:07, satorux1 wrote:
> On 2012/07/28 00:06:10, kinaba wrote:
> > On 2012/07/27 22:56:29, satorux1 wrote:
> > > We want to minimize functions in GDataFileSystem. Can you implement this
> > outside
> > > of GDataFileSystem?
> > 
> > I'll think about it. Do you have any advice where it would be the better
place
> > to put it?
> > I currently chose to place it inside GDataFileSystem for safely getting (or
> not
> > getting, when already destructed) pointer to it for calling CloseFile().
> 
> I guess we can move this to gdata_util.cc. For CloseFile(), we should probably
> check if GDataSystemService::file_system() is non-null before calling it. I'm
> not sure if it's feasible, though.
> 
> You might also want to make it a class and put into new files. gdata_util.cc
is
> also getting bigger. :P
> 
> > 
> > > 
> > > Besides, please write a unit test for the new function.
> 

Created a separate class (suggestions are highly welcome for
the name of the class!) and made it to be owned by GDataSystemService.

The reason is, we need to call GDataFileSystem::CloseFile() after
|callback| executed its task on FILE thread, during which, not only
GDataFileSystemInterface but also GDataSystemService and Profile might
have gone (if I understand the lifetime of Profile correctly).
I thought the most handy way to detect this destruction is having a
weak_ptr to an instance (in)directly owned by Profile.

Let me know if there's a better approach.

[satorux1, 2012-08-02 01:08:09]

http://codereview.chromium.org/10827068/diff/15009/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_file_writing.h (right):

http://codereview.chromium.org/10827068/diff/15009/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_writing.h:18: class GDataFileWriting {
The class name looks rather unusual. Maybe something like FileWriteHelper?

http://codereview.chromium.org/10827068/diff/15009/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_writing.h:55:
base::WeakPtrFactory<GDataFileWriting> ui_weak_ptr_factory_;
weak_ptr_factory_

http://codereview.chromium.org/10827068/diff/15009/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_writing.h:56:
base::WeakPtr<GDataFileWriting> ui_weak_ptr_;
Please remove this. Please use GetWeakPtr()

[kinaba, 2012-08-02 04:57:31]

http://codereview.chromium.org/10827068/diff/15009/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_file_writing.h (right):

http://codereview.chromium.org/10827068/diff/15009/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_writing.h:18: class GDataFileWriting {
On 2012/08/02 01:08:09, satorux1 wrote:
> The class name looks rather unusual. Maybe something like FileWriteHelper?

Done.

http://codereview.chromium.org/10827068/diff/15009/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_writing.h:55:
base::WeakPtrFactory<GDataFileWriting> ui_weak_ptr_factory_;
On 2012/08/02 01:08:09, satorux1 wrote:
> weak_ptr_factory_

Done.

http://codereview.chromium.org/10827068/diff/15009/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_writing.h:56:
base::WeakPtr<GDataFileWriting> ui_weak_ptr_;
On 2012/08/02 01:08:09, satorux1 wrote:
> Please remove this. Please use GetWeakPtr()

Done.

[satorux1, 2012-08-02 06:56:53]

http://codereview.chromium.org/10827068/diff/6021/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_file_write_helper.cc (right):

http://codereview.chromium.org/10827068/diff/6021/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_write_helper.cc:41:
weak_ptr_factory_.GetWeakPtr(),
ah, I was wrong. If you are to post a task to UI thread, you shouldn't use
GetWeakPtr(). Please create ui_weak_ptr in the constructor...

http://codereview.chromium.org/10827068/diff/6021/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_file_write_helper.h (right):

http://codereview.chromium.org/10827068/diff/6021/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_write_helper.h:18: class
GDataFileWriteHelper {
We can remove "GData" prefix from the class, and the file name. Historically,
we've been abusing the term "gdata" to mean both GDATA API and Drive, but we
should stop doing that.

[satorux1, 2012-08-02 06:59:15]

http://codereview.chromium.org/10827068/diff/6021/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_file_write_helper.cc (right):

http://codereview.chromium.org/10827068/diff/6021/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_write_helper.cc:41:
weak_ptr_factory_.GetWeakPtr(),
On 2012/08/02 06:56:53, satorux1 wrote:
> ah, I was wrong. If you are to post a task to UI thread, you shouldn't use
> GetWeakPtr(). Please create ui_weak_ptr in the constructor...

Another option. As of of now, there is only one caller of this function. If the
caller is on UI thread, we should get rid of the redirection logic. We can add
it back when it's needed. YAGNI.

http://codereview.chromium.org/10827068/diff/6021/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right):

http://codereview.chromium.org/10827068/diff/6021/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview/print_preview_handler.cc:934: #ifdef
OS_CHROMEOS
on what thread this function is called? can you add a DCHECK?

[kinaba, 2012-08-02 07:22:39]

http://codereview.chromium.org/10827068/diff/6021/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_file_write_helper.cc (right):

http://codereview.chromium.org/10827068/diff/6021/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_write_helper.cc:41:
weak_ptr_factory_.GetWeakPtr(),
On 2012/08/02 06:59:16, satorux1 wrote:
> On 2012/08/02 06:56:53, satorux1 wrote:
> > ah, I was wrong. If you are to post a task to UI thread, you shouldn't use
> > GetWeakPtr(). Please create ui_weak_ptr in the constructor...
> 
> Another option. As of of now, there is only one caller of this function. If
the
> caller is on UI thread, we should get rid of the redirection logic. We can add
> it back when it's needed. YAGNI.

Took the latter option.
I'll soon add another client (crbug.com/138593) but it also lives in the UI
thread.

http://codereview.chromium.org/10827068/diff/6021/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_file_write_helper.h (right):

http://codereview.chromium.org/10827068/diff/6021/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_write_helper.h:18: class
GDataFileWriteHelper {
On 2012/08/02 06:56:53, satorux1 wrote:
> We can remove "GData" prefix from the class, and the file name. Historically,
> we've been abusing the term "gdata" to mean both GDATA API and Drive, but we
> should stop doing that.

Done.

http://codereview.chromium.org/10827068/diff/6021/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right):

http://codereview.chromium.org/10827068/diff/6021/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview/print_preview_handler.cc:934: #ifdef
OS_CHROMEOS
On 2012/08/02 06:59:16, satorux1 wrote:
> on what thread this function is called? can you add a DCHECK?

Done.

[satorux1, 2012-08-02 07:37:51]

LGTM with nits

http://codereview.chromium.org/10827068/diff/9/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/file_write_helper.h (right):

http://codereview.chromium.org/10827068/diff/9/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/file_write_helper.h:50: // WeakPtrFactory bound to
the UI thread.
please add

  // Note: This should remain the last member so it'll be destroyed and         
  // invalidate its weak pointers before any other members are destroyed.

http://codereview.chromium.org/10827068/diff/9/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_util.cc (right):

http://codereview.chromium.org/10827068/diff/9/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_util.cc:591: const OpenFileCallback&
callback) {
Add DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));

[kinaba, 2012-08-02 08:30:02]

http://codereview.chromium.org/10827068/diff/9/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/file_write_helper.h (right):

http://codereview.chromium.org/10827068/diff/9/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/file_write_helper.h:50: // WeakPtrFactory bound to
the UI thread.
On 2012/08/02 07:37:51, satorux1 wrote:
> please add
> 
>   // Note: This should remain the last member so it'll be destroyed and       
 
>   // invalidate its weak pointers before any other members are destroyed.     
 

Done.

http://codereview.chromium.org/10827068/diff/9/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_util.cc (right):

http://codereview.chromium.org/10827068/diff/9/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_util.cc:591: const OpenFileCallback&
callback) {
On 2012/08/02 07:37:51, satorux1 wrote:
> Add DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));

Done.

[kinaba, 2012-08-02 09:13:58]

brettw:
I'd just like to ask your owner's stamp for file addition to
chrome_{browser,tests}.gypi.

abodenha:
Could you review the changes in print_preview_handler.cc?
I admit that it would be ideal if we could implement file
writing to Drive fully transparently, but it requires more efforts.
So I propose the current patch as a practical solution for
the coming M22.

Thanks!

[Albert Bodenhamer, 2012-08-02 16:20:59]

print_preview_handler lgtm

http://codereview.chromium.org/10827068/diff/10029/chrome/browser/ui/webui/pr...
File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right):

http://codereview.chromium.org/10827068/diff/10029/chrome/browser/ui/webui/pr...
chrome/browser/ui/webui/print_preview/print_preview_handler.cc:62: #ifdef
OS_CHROMEOS
Could you create a bug to unify/simplify the flow and add a TODO here that
references it?

[brettw, 2012-08-02 18:17:14]

owners lgtm

[kinaba, 2012-08-03 05:19:49]

Done. Thank you for reviewing.

http://codereview.chromium.org/10827068/diff/10029/chrome/browser/ui/webui/pr...
File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right):

http://codereview.chromium.org/10827068/diff/10029/chrome/browser/ui/webui/pr...
chrome/browser/ui/webui/print_preview/print_preview_handler.cc:62: #ifdef
OS_CHROMEOS
On 2012/08/02 16:20:59, Albert Bodenhamer wrote:
> Could you create a bug to unify/simplify the flow and add a TODO here that
> references it?

Done.

[Albert Bodenhamer, 2012-08-05 02:54:53]

still lgtm


On Thu, Aug 2, 2012 at 10:19 PM, <kinaba@chromium.org> wrote:

> Done. Thank you for reviewing.
>
>
>
> http://codereview.chromium.**org/10827068/diff/10029/**
>
chrome/browser/ui/webui/print_**preview/print_preview_handler.**cc<http://codereview.chromium.org/10827068/diff/10029/chrome/browser/ui/webui/print_preview/print_preview_handler.cc>
> File chrome/browser/ui/webui/print_**preview/print_preview_handler.**cc
> (right):
>
> http://codereview.chromium.**org/10827068/diff/10029/**
> chrome/browser/ui/webui/print_**preview/print_preview_handler.**
>
cc#newcode62<http://codereview.chromium.org/10827068/diff/10029/chrome/browser/ui/webui/print_preview/print_preview_handler.cc#newcode62>
> chrome/browser/ui/webui/print_**preview/print_preview_handler.**cc:62:
> #ifdef OS_CHROMEOS
> On 2012/08/02 16:20:59, Albert Bodenhamer wrote:
>
>> Could you create a bug to unify/simplify the flow and add a TODO here
>>
> that
>
>> references it?
>>
>
> Done.
>
>
http://codereview.chromium.**org/10827068/<http://codereview.chromium.org/108...
>



-- 
Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com>
org