Escaping file names correctly. Also fixed a crush in chromeos debug build while saving a web page.

Escaping file names correctly. Also fixed a crush in chromeos debug build while saving a web page.

BUG=chromium-os:13130
TEST=Save a file with special characters like '#', '&', '"'.


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

[SeRya, 2011-06-01 12:45:20]

This CL makes possible to save a file with special characters (including '#')
and open such a file.

But file API suffer the same issue. So it's impossible for such a file or dir to
get metadata, enter a directory, rename and so on.

[rginda, 2011-06-01 16:53:51]

http://codereview.chromium.org/7057053/diff/1001/chrome/browser/extensions/fi...
File chrome/browser/extensions/file_manager_util.cc (right):

http://codereview.chromium.org/7057053/diff/1001/chrome/browser/extensions/fi...
chrome/browser/extensions/file_manager_util.cc:20: #include
"third_party/libjingle/source/talk/base/urlencode.h"
libjingle has a UrlEncodeStringWithoutEncodingSpaceAsPlus() function which is
used elsewhere in this file for encoding urls without turning spaces into
plusses.  I like your approach better (including string escapes from libjingle
seems wrong).  Could you remove the libjingle calls in this CL too?

http://codereview.chromium.org/7057053/diff/1001/chrome/browser/resources/fil...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

http://codereview.chromium.org/7057053/diff/1001/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/js/file_manager.js:555: fileName =
encodeURI(fileName);
If you switch to encodeURIComponent you won't need to clean up the # (or other
special url characters like ? and &).

http://codereview.chromium.org/7057053/diff/1001/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/js/file_manager.js:563: * "Save a file"
dislog is suttposed to have a combo box with available
dislog is suttposed => dialog is supposed

http://codereview.chromium.org/7057053/diff/1001/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/js/file_manager.js:564: * file types.
Selecting an item filter files by extension and specifies how
filters files

[SeRya, 2011-06-02 08:42:40]

http://codereview.chromium.org/7057053/diff/1001/chrome/browser/extensions/fi...
File chrome/browser/extensions/file_manager_util.cc (right):

http://codereview.chromium.org/7057053/diff/1001/chrome/browser/extensions/fi...
chrome/browser/extensions/file_manager_util.cc:20: #include
"third_party/libjingle/source/talk/base/urlencode.h"
On 2011/06/01 16:53:51, rginda wrote:
> libjingle has a UrlEncodeStringWithoutEncodingSpaceAsPlus() function which is
> used elsewhere in this file for encoding urls without turning spaces into
> plusses.  I like your approach better (including string escapes from libjingle
> seems wrong).  Could you remove the libjingle calls in this CL too?

Done.

http://codereview.chromium.org/7057053/diff/1001/chrome/browser/resources/fil...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

http://codereview.chromium.org/7057053/diff/1001/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/js/file_manager.js:555: fileName =
encodeURI(fileName);
On 2011/06/01 16:53:51, rginda wrote:
> If you switch to encodeURIComponent you won't need to clean up the # (or other
> special url characters like ? and &).

Done.

http://codereview.chromium.org/7057053/diff/1001/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/js/file_manager.js:563: * "Save a file"
dislog is suttposed to have a combo box with available
On 2011/06/01 16:53:51, rginda wrote:
> dislog is suttposed => dialog is supposed

Done.

http://codereview.chromium.org/7057053/diff/1001/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/js/file_manager.js:564: * file types.
Selecting an item filter files by extension and specifies how
On 2011/06/01 16:53:51, rginda wrote:
> filters files

Done.

[rginda, 2011-06-03 17:39:01]

LGTM

[commit-bot: I haz the power, 2011-06-06 13:05:05]

Presubmit check for 7057053-12001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
net/base/escape.h,net/base/escape.cc,net/base/escape_unittest.cc

[SeRya, 2011-06-06 13:15:57]

Needs owner's review.

[ahendrickson, 2011-06-06 15:29:51]

Please fix the nit.

Also, recommend adding either mrossetti or pkasting to the reviewers list as
they have touched this file a lot.

http://codereview.chromium.org/7057053/diff/12001/net/base/escape.h
File net/base/escape.h (right):

http://codereview.chromium.org/7057053/diff/12001/net/base/escape.h#newcode27
net/base/escape.h:27: bool use_plus = true);
Nit: The style guide does not allow default arguments in this case:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Default_Arguments:

"we require all arguments to be explicitly specified, to force programmers to
consider the API and the values they are passing for each argument rather than
silently accepting defaults they may not be aware of"

[SeRya, 2011-06-07 12:16:04]

http://codereview.chromium.org/7057053/diff/12001/net/base/escape.h
File net/base/escape.h (right):

http://codereview.chromium.org/7057053/diff/12001/net/base/escape.h#newcode27
net/base/escape.h:27: bool use_plus = true);
On 2011/06/06 15:29:51, ahendrickson wrote:
> Nit: The style guide does not allow default arguments in this case:
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Default_Arguments:
> 
> "we require all arguments to be explicitly specified, to force programmers to
> consider the API and the values they are passing for each argument rather than
> silently accepting defaults they may not be aware of"

Done.

[ahendrickson, 2011-06-07 15:06:38]

LGTM, pending review by either mrossetti or pkasting.

[Peter Kasting, 2011-06-07 18:08:42]

LGTM

http://codereview.chromium.org/7057053/diff/13002/net/base/escape_unittest.cc
File net/base/escape_unittest.cc (right):

http://codereview.chromium.org/7057053/diff/13002/net/base/escape_unittest.cc...
net/base/escape_unittest.cc:159: ASSERT_EQ(
Nit: This indenting is bizarre.  Just do:

  ASSERT_EQ(EscapeUrlEncodedData("a b", true), "a+b");
  ASSERT_EQ(EscapeUrlEncodedData("a b", false), "a%20b");

[SeRya, 2011-06-08 12:24:27]

http://codereview.chromium.org/7057053/diff/13002/net/base/escape_unittest.cc
File net/base/escape_unittest.cc (right):

http://codereview.chromium.org/7057053/diff/13002/net/base/escape_unittest.cc...
net/base/escape_unittest.cc:159: ASSERT_EQ(
On 2011/06/07 18:08:43, Peter Kasting wrote:
> Nit: This indenting is bizarre.  Just do:
> 
>   ASSERT_EQ(EscapeUrlEncodedData("a b", true), "a+b");
>   ASSERT_EQ(EscapeUrlEncodedData("a b", false), "a%20b");

Done.

[commit-bot: I haz the power, 2011-06-08 12:25:16]

Presubmit check for 7057053-27001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for: chrome/browser/sync/engine/net/url_translator.cc

[SeRya, 2011-06-08 12:36:10]

Needed owner's review for chrome/browser/sync/...

[Rick Campbell, 2011-06-08 16:24:29]

On 2011/06/08 12:36:10, SeRya wrote:
> Needed owner's review for chrome/browser/sync/...

Could I get some more background?  My understanding is that + is archaic and %20
is preferred.  This should break things that have strict rules about
percent-encoding, e. g. the OAuth protocol.

[Rick Campbell, 2011-06-08 16:29:20]

On 2011/06/08 16:24:29, Rick Campbell wrote:
> On 2011/06/08 12:36:10, SeRya wrote:
> > Needed owner's review for chrome/browser/sync/...
> 
> Could I get some more background?  My understanding is that + is archaic and
%20
> is preferred.  This should break things that have strict rules about
> percent-encoding, e. g. the OAuth protocol.

Nevermind.  I've read more thoroughly now.

To clarify -- I now see that your change makes it possible to avoid using plus,
allowing proper escaping, and your change to the Sync file is to preserve the
previous behavior since it seems to be working just fine with the plus sign.

LGTM

[rginda, 2011-06-08 16:37:34]

On 2011/06/08 16:24:29, Rick Campbell wrote:
> On 2011/06/08 12:36:10, SeRya wrote:
> > Needed owner's review for chrome/browser/sync/...
> 
> Could I get some more background?  My understanding is that + is archaic and
%20
> is preferred.  This should break things that have strict rules about
> percent-encoding, e. g. the OAuth protocol.

(Jumping in for Sergey, who is in GMT+400, and probably sleeping.)

Some systems expect plus either because they are also archaic, or for historic
reasons.

Before this change, EscapeUrlEncodedData *always* turned spaces into pluses. 
Now callers have the option of encoding space as %20 when it is appropriate.

In the sync case, the escape rules haven't changed.  Space-as-plus is what it
got before, and what will happen with this CL.

[commit-bot: I haz the power, 2011-06-09 08:35:10]

Change committed as 88510