Move the save file code from chrome to content. This is just a file move so the DEPS in content\b...

Move the save file code from chrome to content. This is just a file move so the DEPS in content\browser\download is very permissive for now. The one exception is save_package.cc, where I had to split off the usage of SelectFileDialog because the code depended on grit which can't be used in content (circular dependencies in gyp).

TBR=rdsmith

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

[jam, 2011-07-14 22:18:37]

btw the one behavioral change is that SavePackageFilePicker (new class that I
added in chrome) now has a refcount on SavePackage. Previously, SavePackage
would own SelectFileDialog and would tell it in its destructor that it's going
away. I don't think this makes a difference in practice since the TabContnts
won't go away before the dialog, since the latter is modal.

[Randy Smith (Not in Mondays), 2011-07-15 17:16:18]

Two comments beyond the below:
* Is there no way to move save_page_uitest.cc?  I think it's a real win to have
test code right next to the code it's testing.  
* I'm worried about leaks.  I'm not at all sure we're guaranteed to get
callbacks from the file selection dialog during shutdown, and if we don't, we've
leaked both both SavePackageFilePicker and SavePackage.  If you re-routed things
to use the ListenerDestroyed() pattern, how much of a hassle would it be?

http://codereview.chromium.org/7373004/diff/4081/chrome/browser/download/save...
File chrome/browser/download/save_package_file_picker.cc (right):

http://codereview.chromium.org/7373004/diff/4081/chrome/browser/download/save...
chrome/browser/download/save_package_file_picker.cc:56: //
GetSaveInfo->CreateDirectoryOnFileThread->ContinueGetSaveInfo.
Could you specify in this comment both that you're talking about SavePackage
methods, and why it's safe to use the save_package argument after the
TabContents has been destroyed even though that generally results in SavePackage
destruction?  (The refcount only occurs in the constructor).

http://codereview.chromium.org/7373004/diff/4081/chrome/browser/download/save...
File chrome/browser/download/save_package_file_picker.h (right):

http://codereview.chromium.org/7373004/diff/4081/chrome/browser/download/save...
chrome/browser/download/save_package_file_picker.h:35:
scoped_refptr<SelectFileDialog> select_file_dialog_;
You remarked in a Rietveld comment that this wasn't necessary.  Could you also
document that here?  At some point we're going to be doing refactoring in this
code, and it would be good to hold onto the possibility of dropping the pointer
(with appropriate DCHECKs) if it isn't needed.

[jam, 2011-07-15 18:02:10]

On 2011/07/15 17:16:18, rdsmith wrote:
> Two comments beyond the below:
> * Is there no way to move save_page_uitest.cc?  I think it's a real win to
have
> test code right next to the code it's testing.  

Yeah I would like to move it as well, the problem is that it and the
browser_test depend on chrome UI. if we were to move them to content, we would
have to open up the deps file to these chrome UI, and other non-test code might
use it which would be a problem.

> * I'm worried about leaks.  I'm not at all sure we're guaranteed to get
> callbacks from the file selection dialog during shutdown, and if we don't,
we've
> leaked both both SavePackageFilePicker and SavePackage.  If you re-routed
things
> to use the ListenerDestroyed() pattern, how much of a hassle would it be?

Generally, memory leaks at shutdown are ok. Does SavePackage do anything to the
disk in its destructor?

The reason I didn't use ListenerDestroyed is that then we'd have to create an
interface around the file picker so that content can know about it, and I was
too lazy to add it :) We can alternatively make SavePackage support weak ptr,
and have SavePackageFilePicker use that. Before I do the work, I'd just like to
know if we need it?

> 
>
http://codereview.chromium.org/7373004/diff/4081/chrome/browser/download/save...
> File chrome/browser/download/save_package_file_picker.cc (right):
> 
>
http://codereview.chromium.org/7373004/diff/4081/chrome/browser/download/save...
> chrome/browser/download/save_package_file_picker.cc:56: //
> GetSaveInfo->CreateDirectoryOnFileThread->ContinueGetSaveInfo.
> Could you specify in this comment both that you're talking about SavePackage
> methods, and why it's safe to use the save_package argument after the
> TabContents has been destroyed even though that generally results in
SavePackage
> destruction?  (The refcount only occurs in the constructor).

I actually just moved this comment over when moving the code.

> 
>
http://codereview.chromium.org/7373004/diff/4081/chrome/browser/download/save...
> File chrome/browser/download/save_package_file_picker.h (right):
> 
>
http://codereview.chromium.org/7373004/diff/4081/chrome/browser/download/save...
> chrome/browser/download/save_package_file_picker.h:35:
> scoped_refptr<SelectFileDialog> select_file_dialog_;
> You remarked in a Rietveld comment that this wasn't necessary.

Which comment are you referring to?

>  Could you also
> document that here?  At some point we're going to be doing refactoring in this
> code, and it would be good to hold onto the possibility of dropping the
pointer
> (with appropriate DCHECKs) if it isn't needed.

[jam, 2011-07-15 18:24:07]

On 2011/07/15 18:02:10, John Abd-El-Malek wrote:
> On 2011/07/15 17:16:18, rdsmith wrote:
> > Two comments beyond the below:
> > * Is there no way to move save_page_uitest.cc?  I think it's a real win to
> have
> > test code right next to the code it's testing.  
> 
> Yeah I would like to move it as well, the problem is that it and the
> browser_test depend on chrome UI. if we were to move them to content, we would
> have to open up the deps file to these chrome UI, and other non-test code
might
> use it which would be a problem.
> 
> > * I'm worried about leaks.  I'm not at all sure we're guaranteed to get
> > callbacks from the file selection dialog during shutdown, and if we don't,
> we've
> > leaked both both SavePackageFilePicker and SavePackage.  If you re-routed
> things
> > to use the ListenerDestroyed() pattern, how much of a hassle would it be?
> 
> Generally, memory leaks at shutdown are ok. Does SavePackage do anything to
the
> disk in its destructor?
> 
> The reason I didn't use ListenerDestroyed is that then we'd have to create an
> interface around the file picker so that content can know about it, and I was
> too lazy to add it :) We can alternatively make SavePackage support weak ptr,
> and have SavePackageFilePicker use that. Before I do the work, I'd just like
to
> know if we need it?

ok I just looked at SavePackage's destructor, and we do want it to run. Since
I'm not sure if the SavePackageFilePicker's destructor will run on all
platforms, I'll figure out a way to do this and send you a cl, thanks.
> 
> > 
> >
>
http://codereview.chromium.org/7373004/diff/4081/chrome/browser/download/save...
> > File chrome/browser/download/save_package_file_picker.cc (right):
> > 
> >
>
http://codereview.chromium.org/7373004/diff/4081/chrome/browser/download/save...
> > chrome/browser/download/save_package_file_picker.cc:56: //
> > GetSaveInfo->CreateDirectoryOnFileThread->ContinueGetSaveInfo.
> > Could you specify in this comment both that you're talking about SavePackage
> > methods, and why it's safe to use the save_package argument after the
> > TabContents has been destroyed even though that generally results in
> SavePackage
> > destruction?  (The refcount only occurs in the constructor).
> 
> I actually just moved this comment over when moving the code.
> 
> > 
> >
>
http://codereview.chromium.org/7373004/diff/4081/chrome/browser/download/save...
> > File chrome/browser/download/save_package_file_picker.h (right):
> > 
> >
>
http://codereview.chromium.org/7373004/diff/4081/chrome/browser/download/save...
> > chrome/browser/download/save_package_file_picker.h:35:
> > scoped_refptr<SelectFileDialog> select_file_dialog_;
> > You remarked in a Rietveld comment that this wasn't necessary.
> 
> Which comment are you referring to?
> 
> >  Could you also
> > document that here?  At some point we're going to be doing refactoring in
this
> > code, and it would be good to hold onto the possibility of dropping the
> pointer
> > (with appropriate DCHECKs) if it isn't needed.