Save As for content retrieved via POST works in most circumstances.

"Save As" for content retrieved via POST works in most circumstances.

If the content is in the HTTP cache, this will work in the following cases:
  - "Save Page As" from the wrench menu for non-HTML pages.
  - Context-menu save image as, or save audio/video plugin as.
  - ViewHostMsg_SaveURLAs IPC's.
  
This will work for back/forward navigations to an item which was POST'ed, as well as for tabs restored via the TabRestoreService.

It does not work for tabs restored via the SessionService, such as after Chrome crashes or for users who have the "Reopen the Pages that were Open Last" option set.

This CL currently depends on http://codereview.chromium.org/9317009/
 
BUG=55551
TEST=DownloadTest.SavePageNonHTMLViaPost


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

[cbentzel, 2012-02-01 18:54:40]

This is an FYI at this point. I'm not expecting a response from you right now. 

I don't like adding a new virtual to WebContents, and this needs automated tests
as well as cleanup.

[cbentzel, 2012-02-02 16:35:45]

I'd like a review at this point.

I don't love how some of the logic about extracting the post id is duplicated in
the context menu code from tab_contents. However, my initial patch added a
pure-virtual-method to WebContents so this logic could be consolidated, and it
felt like a strange thing to add to the WebContents API.

I'm also working on browser_tests which test the "Save As" via context-menu
approaches. May put those in a separate CL.

[Randy Smith (Not in Mondays), 2012-02-02 19:46:10]

Looks pretty good.

* With regard to the duplication of code, I could imagine writing a utility
routine (creating a download_util.h at the chrome level), but I suspect it's not
worth it.  I agree that a routine on WebContents isn't worth it.  So I'm good
with the duplication.

* Does ViewHost_SaveURL IPCs encompass the "Save" button in the PDF viewer?  How
does the print button on the PDF viewer behave?

http://codereview.chromium.org/9314037/diff/5039/chrome/browser/download/down...
File chrome/browser/download/download_browsertest.cc (right):

http://codereview.chromium.org/9314037/diff/5039/chrome/browser/download/down...
chrome/browser/download/download_browsertest.cc:1820: false,
DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_FAIL));
In general, I don't consider finish_on_select_file=false a good idea; it means
if the download system gets confused and decides that it needs to prompt, the
test will time out.  Set it to true, and then assert that's not why the observer
finished.  (Yes, the infrastructure should be written so that this is the
natural thing to do :-} :-J.)

http://codereview.chromium.org/9314037/diff/5039/content/browser/download/dow...
File content/browser/download/download_manager_impl.cc (right):

http://codereview.chromium.org/9314037/diff/5039/content/browser/download/dow...
content/browser/download/download_manager_impl.cc:83: if (url_params.post_id_ >=
0) {
If we decide that we eventually want to support POST with bodies that won't hit
in cache (see related comment), there should be a TODO comment here.

http://codereview.chromium.org/9314037/diff/5039/content/browser/renderer_hos...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/9314037/diff/5039/content/browser/renderer_hos...
content/browser/renderer_host/resource_dispatcher_host.cc:896: if
(request->get_upload() != NULL)
I think I'd like a comment here.  The form of the comment depends on whether we
think we want to long-term support POST in the non-cache case; if we do it
should be a TODO/bug reference, if not just an explanation as to why
conditionalizing ONLY_FROM_CACHE on get_upload().

http://codereview.chromium.org/9314037/diff/5039/content/browser/tab_contents...
File content/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/9314037/diff/5039/content/browser/tab_contents...
content/browser/tab_contents/tab_contents.cc:1416: void
TabContents::OnSaveURL(const GURL& url) {
When does this get called?  I ask because we should have a UMA for it.  I'm
doing a revamp of the initiation UMAs at the moment, so I'll just fold this in,
but I'd like to know what to call the UMA :-}.

http://codereview.chromium.org/9314037/diff/5039/content/browser/tab_contents...
content/browser/tab_contents/tab_contents.cc:2282: if (url == GetURL()) {
Could you comment on the purpose of this test?  (Either in the code, or to me if
it should be obvious from the code.)  It seems a bit dodgy given how the helper
function is used in this file; it's as if you're saying "If we're doing a save
from OnSavePage(), probe for the post_id, otherwise don't." in which case it
should be a level up.  But that may be because I don't understand OnSaveUrl().

http://codereview.chromium.org/9314037/diff/5039/content/public/browser/downl...
File content/public/browser/download_manager.h (right):

http://codereview.chromium.org/9314037/diff/5039/content/public/browser/downl...
content/public/browser/download_manager.h:184: int64 post_id,
Please document this argument, including the "don't do a POST" value (-1).

(I'm not ecstatic about overloading the POST/Don't-POST signal on top of the
post id, but given our various constraints I think it makes sense.  If we had
DownloadRetrieveInfo, I'd want them separate.)

[cbentzel, 2012-02-02 19:53:06]

I'll let you know when I put things in to match your comments.

http://codereview.chromium.org/9314037/diff/5039/chrome/browser/download/down...
File chrome/browser/download/download_browsertest.cc (right):

http://codereview.chromium.org/9314037/diff/5039/chrome/browser/download/down...
chrome/browser/download/download_browsertest.cc:1820: false,
DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_FAIL));
On 2012/02/02 19:46:10, rdsmith wrote:
> In general, I don't consider finish_on_select_file=false a good idea; it means
> if the download system gets confused and decides that it needs to prompt, the
> test will time out.  Set it to true, and then assert that's not why the
observer
> finished.  (Yes, the infrastructure should be written so that this is the
> natural thing to do :-} :-J.)

Actually, it does prompt in this case. I have the NullSelectFile at the top of
the test, and an EXPECT_TRUE(waiter->select_file_dialog_seen()) below.

http://codereview.chromium.org/9314037/diff/5039/content/public/browser/downl...
File content/public/browser/download_manager.h (right):

http://codereview.chromium.org/9314037/diff/5039/content/public/browser/downl...
content/public/browser/download_manager.h:184: int64 post_id,
On 2012/02/02 19:46:10, rdsmith wrote:
> Please document this argument, including the "don't do a POST" value (-1).
> 
> (I'm not ecstatic about overloading the POST/Don't-POST signal on top of the
> post id, but given our various constraints I think it makes sense.  If we had
> DownloadRetrieveInfo, I'd want them separate.)

Agreed.

[cbentzel, 2012-02-03 15:54:47]

Added comments.

http://codereview.chromium.org/9314037/diff/5039/content/browser/download/dow...
File content/browser/download/download_manager_impl.cc (right):

http://codereview.chromium.org/9314037/diff/5039/content/browser/download/dow...
content/browser/download/download_manager_impl.cc:83: if (url_params.post_id_ >=
0) {
On 2012/02/02 19:46:10, rdsmith wrote:
> If we decide that we eventually want to support POST with bodies that won't
hit
> in cache (see related comment), there should be a TODO comment here.

Done.

http://codereview.chromium.org/9314037/diff/5039/content/browser/renderer_hos...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/9314037/diff/5039/content/browser/renderer_hos...
content/browser/renderer_host/resource_dispatcher_host.cc:896: if
(request->get_upload() != NULL)
On 2012/02/02 19:46:10, rdsmith wrote:
> I think I'd like a comment here.  The form of the comment depends on whether
we
> think we want to long-term support POST in the non-cache case; if we do it
> should be a TODO/bug reference, if not just an explanation as to why
> conditionalizing ONLY_FROM_CACHE on get_upload().

I cover this with a comment in BeginDownload in download_manager_impl.cc. Do you
also want it here?

http://codereview.chromium.org/9314037/diff/5039/content/browser/tab_contents...
File content/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/9314037/diff/5039/content/browser/tab_contents...
content/browser/tab_contents/tab_contents.cc:1416: void
TabContents::OnSaveURL(const GURL& url) {
On 2012/02/02 19:46:10, rdsmith wrote:
> When does this get called?  I ask because we should have a UMA for it.  I'm
> doing a revamp of the initiation UMAs at the moment, so I'll just fold this
in,
> but I'd like to know what to call the UMA :-}.
> 

This is currently called when the save button icon is clicked within the bundled
PDF viewer plugin.

http://codereview.chromium.org/9314037/diff/5039/content/browser/tab_contents...
content/browser/tab_contents/tab_contents.cc:2282: if (url == GetURL()) {
On 2012/02/02 19:46:10, rdsmith wrote:
> Could you comment on the purpose of this test?  (Either in the code, or to me
if
> it should be obvious from the code.)  It seems a bit dodgy given how the
helper
> function is used in this file; it's as if you're saying "If we're doing a save
> from OnSavePage(), probe for the post_id, otherwise don't." in which case it
> should be a level up.  But that may be because I don't understand OnSaveUrl().

Added a comment.

http://codereview.chromium.org/9314037/diff/5039/content/public/browser/downl...
File content/public/browser/download_manager.h (right):

http://codereview.chromium.org/9314037/diff/5039/content/public/browser/downl...
content/public/browser/download_manager.h:184: int64 post_id,
On 2012/02/02 19:53:06, cbentzel wrote:
> On 2012/02/02 19:46:10, rdsmith wrote:
> > Please document this argument, including the "don't do a POST" value (-1).
> > 
> > (I'm not ecstatic about overloading the POST/Don't-POST signal on top of the
> > post id, but given our various constraints I think it makes sense.  If we
had
> > DownloadRetrieveInfo, I'd want them separate.)
> 
> Agreed.

I documented this. I considered adding a constant to indicate "No post" but
couldn't find a great place to do it - plus it doesn't seem like there are many
constants used in content/public.

[Randy Smith (Not in Mondays), 2012-02-03 19:43:06]

LGTM with extra comment request below.

http://codereview.chromium.org/9314037/diff/5039/chrome/browser/download/down...
File chrome/browser/download/download_browsertest.cc (right):

http://codereview.chromium.org/9314037/diff/5039/chrome/browser/download/down...
chrome/browser/download/download_browsertest.cc:1820: false,
DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_FAIL));
On 2012/02/02 19:53:06, cbentzel wrote:
> On 2012/02/02 19:46:10, rdsmith wrote:
> > In general, I don't consider finish_on_select_file=false a good idea; it
means
> > if the download system gets confused and decides that it needs to prompt,
the
> > test will time out.  Set it to true, and then assert that's not why the
> observer
> > finished.  (Yes, the infrastructure should be written so that this is the
> > natural thing to do :-} :-J.)
> 
> Actually, it does prompt in this case. I have the NullSelectFile at the top of
> the test, and an EXPECT_TRUE(waiter->select_file_dialog_seen()) below. 
> 
Right, forgot about the NullSelectFile().  Thanks.

http://codereview.chromium.org/9314037/diff/5039/content/browser/renderer_hos...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/9314037/diff/5039/content/browser/renderer_hos...
content/browser/renderer_host/resource_dispatcher_host.cc:896: if
(request->get_upload() != NULL)
On 2012/02/03 15:54:47, cbentzel wrote:
> On 2012/02/02 19:46:10, rdsmith wrote:
> > I think I'd like a comment here.  The form of the comment depends on whether
> we
> > think we want to long-term support POST in the non-cache case; if we do it
> > should be a TODO/bug reference, if not just an explanation as to why
> > conditionalizing ONLY_FROM_CACHE on get_upload().
> 
> I cover this with a comment in BeginDownload in download_manager_impl.cc. Do
you
> also want it here?

I'm afraid so.  Code's better when it can be understood with minimal saved state
from other places, so I think we should have something (either independent
comment or pointer is ok).

http://codereview.chromium.org/9314037/diff/5039/content/public/browser/downl...
File content/public/browser/download_manager.h (right):

http://codereview.chromium.org/9314037/diff/5039/content/public/browser/downl...
content/public/browser/download_manager.h:184: int64 post_id,
On 2012/02/03 15:54:47, cbentzel wrote:
> On 2012/02/02 19:53:06, cbentzel wrote:
> > On 2012/02/02 19:46:10, rdsmith wrote:
> > > Please document this argument, including the "don't do a POST" value (-1).
> > > 
> > > (I'm not ecstatic about overloading the POST/Don't-POST signal on top of
the
> > > post id, but given our various constraints I think it makes sense.  If we
> had
> > > DownloadRetrieveInfo, I'd want them separate.)
> > 
> > Agreed.
> 
> I documented this. I considered adding a constant to indicate "No post" but
> couldn't find a great place to do it - plus it doesn't seem like there are
many
> constants used in content/public. 

Yeah, I considered asking for a constant but had the same reaction :-}.

[cbentzel, 2012-02-03 20:29:31]

Currently rebasing/trying and will get appropriate OWNERS permissions.

http://codereview.chromium.org/9314037/diff/5039/content/browser/renderer_hos...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/9314037/diff/5039/content/browser/renderer_hos...
content/browser/renderer_host/resource_dispatcher_host.cc:896: if
(request->get_upload() != NULL)
On 2012/02/03 19:43:06, rdsmith wrote:
> On 2012/02/03 15:54:47, cbentzel wrote:
> > On 2012/02/02 19:46:10, rdsmith wrote:
> > > I think I'd like a comment here.  The form of the comment depends on
whether
> > we
> > > think we want to long-term support POST in the non-cache case; if we do it
> > > should be a TODO/bug reference, if not just an explanation as to why
> > > conditionalizing ONLY_FROM_CACHE on get_upload().
> > 
> > I cover this with a comment in BeginDownload in download_manager_impl.cc. Do
> you
> > also want it here?
> 
> I'm afraid so.  Code's better when it can be understood with minimal saved
state
> from other places, so I think we should have something (either independent
> comment or pointer is ok).

Patch Set #9 adds a comment here.

[cbentzel, 2012-02-03 20:36:11]

+jam: content stuff.
+mihaip: trivial chrome/browser/extensions change

[jam, 2012-02-03 20:52:42]

lgtm with one nit

http://codereview.chromium.org/9314037/diff/1017/content/browser/tab_contents...
File content/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/9314037/diff/1017/content/browser/tab_contents...
content/browser/tab_contents/tab_contents.cc:2284: // PDF viewer plugin has the
save button clicked.
we avoid talking about chrome features in content comments. perhaps you can make
this generic and say if a pepper plugin initiates the save?

[cbentzel, 2012-02-04 14:59:08]

http://codereview.chromium.org/9314037/diff/1017/content/browser/tab_contents...
File content/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/9314037/diff/1017/content/browser/tab_contents...
content/browser/tab_contents/tab_contents.cc:2284: // PDF viewer plugin has the
save button clicked.
On 2012/02/03 20:52:42, John Abd-El-Malek wrote:
> we avoid talking about chrome features in content comments. perhaps you can
make
> this generic and say if a pepper plugin initiates the save?

Done.

[commit-bot: I haz the power, 2012-02-04 16:52:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cbentzel@chromium.org/9314037/10024

[commit-bot: I haz the power, 2012-02-04 18:08:59]

Can't apply patch for file content/browser/download/mock_download_manager.h.
While running patch -p1 --forward --force;
patching file content/browser/download/mock_download_manager.h
Hunk #1 FAILED at 49.
1 out of 1 hunk FAILED -- saving rejects to file
content/browser/download/mock_download_manager.h.rej

[commit-bot: I haz the power, 2012-02-04 19:15:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cbentzel@chromium.org/9314037/3066

[commit-bot: I haz the power, 2012-02-04 22:09:16]

Change committed as 120494