Pass the filename obtained from content-disposition header for download

Pass the filename obtained from content-disposition header for download

This CL adds another parameter (filename) for Android to get correct
filename to download. Rather than parsing the header again in Java side,
already-implemented native header parser is used.

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

[Yaron, 2014-02-07 16:43:59]

Can you help me understand how this differs from the internal patch? It looks
like you added parsing there so I'm not sure why you're adding it here too.

[Yaron, 2014-02-07 17:01:39]

On 2014/02/07 16:43:59, Yaron wrote:
> Can you help me understand how this differs from the internal patch? It looks
> like you added parsing there so I'm not sure why you're adding it here too.

lgtm

[boliu, 2014-02-07 18:11:46]

https://codereview.chromium.org/152683003/diff/1/content/browser/android/down...
File content/browser/android/download_controller_android_impl.cc (right):

https://codereview.chromium.org/152683003/diff/1/content/browser/android/down...
content/browser/android/download_controller_android_impl.cc:227:
net::HttpContentDisposition header(info.content_disposition, "");
Hmm...

If we use the full blown GetSuggestedFilename in net_util.cc here (ie take into
account whatever mimetype we have here as well), then there should be no need
for any filename guessing in java code.

Win?

https://codereview.chromium.org/152683003/diff/1/content/browser/android/down...
File content/browser/android/download_controller_android_impl.h (right):

https://codereview.chromium.org/152683003/diff/1/content/browser/android/down...
content/browser/android/download_controller_android_impl.h:68: std::string
filename;
nothing uses this afaict?

[shashi, 2014-02-07 18:59:11]

https://codereview.chromium.org/152683003/diff/1/content/browser/android/down...
File content/browser/android/download_controller_android_impl.cc (right):

https://codereview.chromium.org/152683003/diff/1/content/browser/android/down...
content/browser/android/download_controller_android_impl.cc:227:
net::HttpContentDisposition header(info.content_disposition, "");
I agree with Bo, that should be the best way and even easier to keep desktop and
Android implementations in sync. and we will be able to even reduce our
temporary/incomplete parsing implementation in Java.

*Slight tricky to make sure we do not break any existing scenarios (I hope we
have regression tests for different download bugs on Android over years).


On 2014/02/07 18:11:47, boliu wrote:
> Hmm...
> 
> If we use the full blown GetSuggestedFilename in net_util.cc here (ie take
into
> account whatever mimetype we have here as well), then there should be no need
> for any filename guessing in java code.
> 
> Win?

[Jinsuk Kim, 2014-02-08 02:49:27]

Thanks a lot for the review. Much appreciated.

https://codereview.chromium.org/152683003/diff/1/content/browser/android/down...
File content/browser/android/download_controller_android_impl.cc (right):

https://codereview.chromium.org/152683003/diff/1/content/browser/android/down...
content/browser/android/download_controller_android_impl.cc:227:
net::HttpContentDisposition header(info.content_disposition, "");
On 2014/02/07 18:59:12, shashi wrote:
> I agree with Bo, that should be the best way and even easier to keep desktop
and
> Android implementations in sync. and we will be able to even reduce our
> temporary/incomplete parsing implementation in Java.
> 
> *Slight tricky to make sure we do not break any existing scenarios (I hope we
> have regression tests for different download bugs on Android over years).
> 
> 
> On 2014/02/07 18:11:47, boliu wrote:
> > Hmm...
> > 
> > If we use the full blown GetSuggestedFilename in net_util.cc here (ie take
> into
> > account whatever mimetype we have here as well), then there should be no
need
> > for any filename guessing in java code.
> > 
> > Win?
> 

I'd like to upstream the filename decision logic eventually. I guess it takes
more understanding on other parts like Yaron mentioned in the bug entry. Mind if
I started it in the next CL while letting this one handle the bug fix only?

https://codereview.chromium.org/152683003/diff/1/content/browser/android/down...
content/browser/android/download_controller_android_impl.cc:227:
net::HttpContentDisposition header(info.content_disposition, "");
On 2014/02/07 18:11:47, boliu wrote:
> Hmm...
> 
> If we use the full blown GetSuggestedFilename in net_util.cc here (ie take
into
> account whatever mimetype we have here as well), then there should be no need
> for any filename guessing in java code.
> 
> Win?

I was not aware of GetSuggestedFilename and it is a great suggestion. Thanks!
I'd like to do it in the follow up CL if you're okay.

https://codereview.chromium.org/152683003/diff/1/content/browser/android/down...
File content/browser/android/download_controller_android_impl.h (right):

https://codereview.chromium.org/152683003/diff/1/content/browser/android/down...
content/browser/android/download_controller_android_impl.h:68: std::string
filename;
On 2014/02/07 18:11:47, boliu wrote:
> nothing uses this afaict?

Ah right. Saw it wrong. Reverted.

[boliu, 2014-02-08 02:51:22]

https://codereview.chromium.org/152683003/diff/1/content/browser/android/down...
File content/browser/android/download_controller_android_impl.cc (right):

https://codereview.chromium.org/152683003/diff/1/content/browser/android/down...
content/browser/android/download_controller_android_impl.cc:227:
net::HttpContentDisposition header(info.content_disposition, "");
On 2014/02/08 02:49:28, jindor wrote:
> I was not aware of GetSuggestedFilename and it is a great suggestion. Thanks!
> I'd like to do it in the follow up CL if you're okay.

sgtm, yaron already gave lgtm, which is all you need

[Jinsuk Kim, 2014-02-08 02:55:19]

The CQ bit was checked by jinsukkim@chromium.org

[commit-bot: I haz the power, 2014-02-08 02:55:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jinsukkim@chromium.org/152683003/100001

[commit-bot: I haz the power, 2014-02-08 04:48:38]

Change committed as 249936