Decode url in save package download

Decode url in save package download.

Save package does not decode the url when the web content has no title.

Also add some unit tests for special characters.


BUG=654591
Review-Url: https://codereview.chromium.org/2595753002
Cr-Commit-Position: refs/heads/master@{#442710}
Committed: https://chromium.googlesource.com/chromium/src/+/0d4d0fd0973238c92bbd629960eeed5b3481b48c

[xingliu, 2016-12-21 00:31:09]

Description was changed from

==========
Decode url in save package code.

Save package does not decode the url when the web content has no title.
This involves URL decode and decode with OS's charset from a path name
into a base::string16.

Now use net::GenerateFileName to do this, it also santizes the user
input.



BUG=654591
==========

to

==========
Decode url in save package download.

Save package does not decode the url when the web content has no title.
This involves URL decode and decode with OS's charset from a path name
into a base::string16.

Now use net::GenerateFileName to do this, it also santizes the user
input.



BUG=654591
==========

[xingliu, 2016-12-21 00:31:26]

Description was changed from

==========
Decode url in save package download.

Save package does not decode the url when the web content has no title.
This involves URL decode and decode with OS's charset from a path name
into a base::string16.

Now use net::GenerateFileName to do this, it also santizes the user
input.



BUG=654591
==========

to

==========
Decode url in save package download.

Save package does not decode the url when the web content has no title.
This involves URL decode and decode with OS's charset from a path name
into a base::string16.

Now use net::GenerateFileName to do this, it also santizes the user
input.



BUG=654591
==========

[xingliu, 2016-12-21 00:31:32]

The CQ bit was checked by xingliu@chromium.org to run a CQ dry run

[xingliu, 2016-12-21 00:32:58]

xingliu@chromium.org changed reviewers:
+ dtrainor@chromium.org

[xingliu, 2016-12-21 00:32:59]

Fix the ✰✰✰.txt bug..

[commit-bot: I haz the power, 2016-12-21 00:32:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xingliu, 2016-12-21 00:38:13]

xingliu@chromium.org changed reviewers:
+ asanka@chromium.org

[xingliu, 2016-12-21 00:38:13]

asanka@chromium.org: Please help take a look.

[commit-bot: I haz the power, 2016-12-21 01:35:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-21 01:35:25]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[xingliu, 2016-12-21 19:02:15]

Description was changed from

==========
Decode url in save package download.

Save package does not decode the url when the web content has no title.
This involves URL decode and decode with OS's charset from a path name
into a base::string16.

Now use net::GenerateFileName to do this, it also santizes the user
input.



BUG=654591
==========

to

==========
Decode url in save package download.

Save package does not decode the url when the web content has no title.

Also add some unit tests for special characters.


BUG=654591
==========

[xingliu, 2016-12-21 19:03:36]

The CQ bit was checked by xingliu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-21 19:03:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-21 20:32:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-21 20:32:46]

Dry run: This issue passed the CQ dry run.

[xingliu, 2016-12-21 23:31:47]

The CQ bit was checked by xingliu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-21 23:32:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-21 23:36:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-21 23:36:53]

Dry run: This issue passed the CQ dry run.

[asanka, 2016-12-22 19:38:43]

https://codereview.chromium.org/2595753002/diff/20001/content/browser/downloa...
File content/browser/download/save_package.cc (right):

https://codereview.chromium.org/2595753002/diff/20001/content/browser/downloa...
content/browser/download/save_package.cc:1267: page_url.path(), "/",
base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
Can you check if GURL::ExtractFilename() gives us what we want instead of
rewriting all this logic?

Oh actually, can you check if net::GenerateFileName() gives us a usable filename
from the URL? If so, we can eliminate this entire block.

https://codereview.chromium.org/2595753002/diff/20001/content/browser/downloa...
content/browser/download/save_package.cc:1276: url_path = page_url.host();
Should call url_formatter::IDNToUnicode() on this, otherwise punicode hostnames
will show up wrong.

Can you add an IDN hostname to the tests to make sure we are handling those
correctly?

https://codereview.chromium.org/2595753002/diff/20001/content/browser/downloa...
content/browser/download/save_package.cc:1278: url_path =
net::UnescapeURLComponent(
This shouldn't be called on the hostname (if we end up using it). If
ExtractFilename() gives us a non-empty string, we should be able to safely
unescape it.

[xingliu, 2016-12-22 22:17:08]

The CQ bit was checked by xingliu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-22 22:17:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xingliu, 2016-12-22 22:18:32]

Thanks for the feedback.

Change to use net::GenerateFileName, and try to decode punycode if it uses host
name.

https://codereview.chromium.org/2595753002/diff/20001/content/browser/downloa...
File content/browser/download/save_package.cc (right):

https://codereview.chromium.org/2595753002/diff/20001/content/browser/downloa...
content/browser/download/save_package.cc:1267: page_url.path(), "/",
base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
On 2016/12/22 19:38:43, asanka wrote:
> Can you check if GURL::ExtractFilename() gives us what we want instead of
> rewriting all this logic?
> 
> Oh actually, can you check if net::GenerateFileName() gives us a usable
filename
> from the URL? If so, we can eliminate this entire block.

Done,
net::GenerateFileName() can and it also decodes url-encoding, also add logic to
decode punycode in the host name.

https://codereview.chromium.org/2595753002/diff/20001/content/browser/downloa...
content/browser/download/save_package.cc:1276: url_path = page_url.host();
On 2016/12/22 19:38:43, asanka wrote:
> Should call url_formatter::IDNToUnicode() on this, otherwise punicode
hostnames
> will show up wrong.
> 
> Can you add an IDN hostname to the tests to make sure we are handling those
> correctly?

Done. Current code doesn't decode punycode.

https://codereview.chromium.org/2595753002/diff/20001/content/browser/downloa...
content/browser/download/save_package.cc:1278: url_path =
net::UnescapeURLComponent(
On 2016/12/22 19:38:43, asanka wrote:
> This shouldn't be called on the hostname (if we end up using it). If
> ExtractFilename() gives us a non-empty string, we should be able to safely
> unescape it.

Done. convert to net::GenerateFileName().

[commit-bot: I haz the power, 2016-12-22 23:02:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-22 23:02:57]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[xingliu, 2016-12-27 18:06:40]

The CQ bit was checked by xingliu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-27 18:06:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-27 19:04:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-27 19:04:19]

Dry run: This issue passed the CQ dry run.

[asanka, 2017-01-10 19:42:46]

lgtm

For future reference, it's okay to ping on IM or somesuch after a day of not
hearing back about a code review.

https://codereview.chromium.org/2595753002/diff/80001/content/browser/downloa...
File content/browser/download/save_package.cc (right):

https://codereview.chromium.org/2595753002/diff/80001/content/browser/downloa...
content/browser/download/save_package.cc:1270: if
(name_with_proper_ext.AsUTF8Unsafe() == page_url.host()) {
Darn. This should really go in //net/base, but //net currently doesn't depend on
//components/url_formatter. Adding that dependency would create a cycle in our
DEPS :-(.

I guess we'll leave it here for now.

[xingliu, 2017-01-10 20:08:14]

The CQ bit was checked by xingliu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-10 20:08:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-10 21:41:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-10 21:41:18]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[xingliu, 2017-01-10 21:45:03]

The CQ bit was checked by xingliu@chromium.org

[commit-bot: I haz the power, 2017-01-10 21:45:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-10 22:24:59]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1484084703848190,
"parent_rev": "0637efffc148042848a1b1977668e42c2a73e5b2", "commit_rev":
"0d4d0fd0973238c92bbd629960eeed5b3481b48c"}

[commit-bot: I haz the power, 2017-01-10 22:25:35]

Description was changed from

==========
Decode url in save package download.

Save package does not decode the url when the web content has no title.

Also add some unit tests for special characters.


BUG=654591
==========

to

==========
Decode url in save package download.

Save package does not decode the url when the web content has no title.

Also add some unit tests for special characters.


BUG=654591

Review-Url: https://codereview.chromium.org/2595753002
Cr-Commit-Position: refs/heads/master@{#442710}
Committed:
https://chromium.googlesource.com/chromium/src/+/0d4d0fd0973238c92bbd629960ee...
==========

[commit-bot: I haz the power, 2017-01-10 22:25:36]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/0d4d0fd0973238c92bbd629960ee...