Convert file url to lowercase to make it match a file handler pattern.

Convert file url to lowercase to make it match a file handler pattern.


BUG=
TEST=


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

[Vladislav Kaznacheev, 2012-03-14 08:18:42]

Hi,

Please review this small patch.

Thanks

  Vlad

[SeRya, 2012-03-14 09:08:55]

https://chromiumcodereview.appspot.com/9700011/diff/1/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

https://chromiumcodereview.appspot.com/9700011/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/file_handler_util.cc:231:
StringToLowerASCII(&url_spec);
How about non-ASCII patterns? A third party app may register that.

[Vladislav Kaznacheev, 2012-03-14 11:08:20]

PTAL

https://chromiumcodereview.appspot.com/9700011/diff/1/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

https://chromiumcodereview.appspot.com/9700011/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/file_handler_util.cc:231:
StringToLowerASCII(&url_spec);
On 2012/03/14 09:08:55, SeRya wrote:
> How about non-ASCII patterns? A third party app may register that.

Done.

[SeRya, 2012-03-14 11:35:04]

https://chromiumcodereview.appspot.com/9700011/diff/3002/chrome/browser/chrom...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

https://chromiumcodereview.appspot.com/9700011/diff/3002/chrome/browser/chrom...
chrome/browser/chromeos/extensions/file_handler_util.cc:103:
net::UnescapeURLComponent(str, net::UnescapeRule::NORMAL));
Looks suspicious. You are unescaping whole URL with a method dedicated for
unescaping a component. Please, check that files like 'a#b.png' and 'a?b.png'
work file. We need to make sure this transformation doesn't move characters from
path to query or hash.

[Vladislav Kaznacheev, 2012-03-14 12:24:42]

https://chromiumcodereview.appspot.com/9700011/diff/3002/chrome/browser/chrom...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

https://chromiumcodereview.appspot.com/9700011/diff/3002/chrome/browser/chrom...
chrome/browser/chromeos/extensions/file_handler_util.cc:103:
net::UnescapeURLComponent(str, net::UnescapeRule::NORMAL));
Actually there is no net::UnescapeURL. The second parameter
(net::UnescapeRule::NORMAL) defines that none of the special symbols (%+&/#)
will be unescaped. This is why a#b works fine (I checked).

On 2012/03/14 11:35:04, SeRya wrote:
> Looks suspicious. You are unescaping whole URL with a method dedicated for
> unescaping a component. Please, check that files like 'a#b.png' and 'a?b.png'
> work file. We need to make sure this transformation doesn't move characters
from
> path to query or hash.

[Vladislav Kaznacheev, 2012-03-14 20:06:43]

May I have my LGTM?

On 2012/03/14 12:24:42, Vladislav Kaznacheev wrote:
>
https://chromiumcodereview.appspot.com/9700011/diff/3002/chrome/browser/chrom...
> File chrome/browser/chromeos/extensions/file_handler_util.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9700011/diff/3002/chrome/browser/chrom...
> chrome/browser/chromeos/extensions/file_handler_util.cc:103:
> net::UnescapeURLComponent(str, net::UnescapeRule::NORMAL));
> Actually there is no net::UnescapeURL. The second parameter
> (net::UnescapeRule::NORMAL) defines that none of the special symbols (%+&/#)
> will be unescaped. This is why a#b works fine (I checked).
> 
> On 2012/03/14 11:35:04, SeRya wrote:
> > Looks suspicious. You are unescaping whole URL with a method dedicated for
> > unescaping a component. Please, check that files like 'a#b.png' and
'a?b.png'
> > work file. We need to make sure this transformation doesn't move characters
> from
> > path to query or hash.

[dgozman, 2012-03-15 10:27:07]

Please check that gfx_unittests.FileSystemExtensionApiTest works. LGTM

[Vladislav Kaznacheev, 2012-03-15 11:01:05]

On 2012/03/15 10:27:07, dgozman wrote:
> Please check that gfx_unittests.FileSystemExtensionApiTest works. LGTM

Checked browser_tests.FileSystemExtensionApiTest.
Thanks for the review!

[commit-bot: I haz the power, 2012-03-15 11:01:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaznacheev@chromium.org/9700011/1002

[commit-bot: I haz the power, 2012-03-15 12:40:22]

Change committed as 126891