URL's not properly unescaping when displayed

Local text file with spaces in filename is urlencoded in tab title

When viewing a local text file with spaces in filename, it is still urlencoded. Filename should be displayed with spaces, not with urlencoding. It would be more user-friendly.

Since net::FormatURL is already implemented, using it would be great. But it doesn't escape SPACES, just NORMAL, it doesn't even escape unicode. I plumbed out a unescapeurl that could be used whether we allow conversion of spaces or not.

BUG=8775
(http://crbug.com/8775)
TEST=Tested whether the input is escaped in the navigational context and ran the net tests

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

[Mohamed Mansour (USE mhm), 2009-03-29 06:16:06]

Added maruel for the text_edler.cc change, where changed Unescape rule from
NORMAL to SPACE.

Added brettw for the navigation_controller.cc change, where I called the method
to properly unescape the title..

[brettw, 2009-03-30 15:08:26]

NavigationController change LGTM

[M-A Ruel, 2009-03-30 15:15:27]

I'm a bit surprised it stills passes the unit tests. You can check in as is but
you need to update text_elider_unittest.cc to verify what you just modified. I'd
prefer to have the unit test updated in the same change if possible for you.

[Mohamed Mansour (USE mhm), 2009-03-30 17:45:04]

On 2009/03/30 15:15:27, M-A wrote:
> I'm a bit surprised it stills passes the unit tests. You can check in as is
but
> you need to update text_elider_unittest.cc to verify what you just modified.
I'd
> prefer to have the unit test updated in the same change if possible for you.

I am at work right now, will do it when I get home. Will update the unittest in
this same change. There is no %20 testcase for spaces in that unittest, I will
add that tonight. It makes sense that it will work cause it all the high-bit
characters are unescaped correctly.

[Mohamed Mansour (USE mhm), 2009-03-31 00:47:51]

done, unittest updated with one more test. And passed.

[M-A Ruel, 2009-03-31 01:19:38]

lgtm

[Mohamed Mansour (USE mhm), 2009-04-05 20:56:52]

On 2009/03/31 01:19:38, M-A wrote:
> lgtm

Could anyone commit this?

[M-A Ruel, 2009-04-06 16:23:27]

On 2009/04/05 20:56:52, Mohamed Mansour wrote:
> On 2009/03/31 01:19:38, M-A wrote:
> > lgtm
> 
> Could anyone commit this?

I can't since it causes one unit test breakage on 3 platforms:

URLFixerUpperTest.FixupFile:
..\..\browser\net\url_fixer_upper_unittest.cc(260): error: Value of: fixedup
  Actual: "file:///C:/b/slave/win/build/src/chrome/Debug/url fixer upper
existing file.txt"
Expected: golden
Which is:
"file:///C:/b/slave/win/build/src/chrome/Debug/url%20fixer%20upper%20existing%20file.txt"
..\..\browser\net\url_fixer_upper_unittest.cc(269): error: Value of: fixedup
  Actual: "file:///C:/b/slave/win/build/src/chrome/Debug/url fixer upper
existing file.txt"
Expected: golden
Which is:
"file:///C:/b/slave/win/build/src/chrome/Debug/url%20fixer%20upper%20existing%20file.txt"
..\..\browser\net\url_fixer_upper_unittest.cc(300): error: Value of: fixedup
  Actual: "file:///C:/This%2520is a non-existent file.txt"
Expected: file_cases[i].output
Which is: "file:///C:/This%2520is%20a%20non-existent%20file.txt"

Have fun. :)

[Mohamed Mansour (USE mhm), 2009-04-06 16:47:17]

On 2009/04/06 16:23:27, M-A wrote:
> Have fun. :)

Ah thanks, I was worried about that (I understood why), I will do up another
solution which will make that pass since on IRC, you would like that situation
to be passed.

[Mohamed Mansour (USE mhm), 2009-04-07 05:37:29]

Please review, I changed it now. Since Maruel didn't accept filenames to be
sorted, this patch will just unescape the titlename directly with two escape
values, SPACES and URL_SPECIAL_CHARS.

Personally, I rather escape all with the gfx::GetCleanStringFromUrl, cause it
makes sense because:
1) The omnibox will be escaped (like in FF, Safari, IE)
2) History URL Provider
3) Etc, many places use or depend on that, and its ESCAPING is set to just
Normal, no unicode nor spaces escape.

At the end, you guys decide.

[M-A Ruel, 2009-04-08 20:16:49]

No, you misunderstood what I wanted. I prefer to have the whitespace behavior
configurable, with the original behavior being the recommended one. So
ElideUrl() would have a bool convert_spaces parameter, which would pass it to
GetCleanStringFromUrl and then to AppendFormattedComponent.

It's not about sorting in fact.

[Mohamed Mansour (USE mhm), 2009-04-09 14:02:22]

Okay, done, I ran the tests and they all passed:
[  PASSED  ] 792 tests.

ElideURL did not need a parameter since everything was depending on
AppendFormattedComponent, so I just forced spaces to be elided, and plumbed down
for AppendFormattedComponent.

Do you want me to plumb down ElideURL to everyone who is calling it as well, and
introduce a bool for it? All those callers will have true.

Your call, the tests pass, and the feature works (tested), as well as some code
styles were corrected.

Let me know if anything is needed, and/or you will like me to plumb all ElideURL
callers.

[Mohamed Mansour (USE mhm), 2009-04-09 17:43:10]

Hey guys, Since you guys touched this part of the code, I would like you guys to
check this one line change in your areas:
estade -> Autocomplete, HistoryUrlProvider, UrlFixerUpper and ToolBarModel
brettw -> NavigationController and NavigationEntry
maruel -> TextEdler's

Basically, I added a boolean to gfx::GetCleanStringFromUrl called
|convert_spaces|. Since GetCleanStringFromUrl does not escape spaces, it just
escapes normally, maruel recommended me to add that boolean. 

Please check the files, and let me know if the value I added (true, or false)
makes sense in that context.

Thanks!

[Evan Stade, 2009-04-09 17:58:12]

the files you assigned to me LG, and thanks for all the cleanup

http://codereview.chromium.org/56053/diff/6001/4004
File chrome/browser/net/url_fixer_upper.cc (right):

http://codereview.chromium.org/56053/diff/6001/4004#newcode228
Line 228: DCHECK(part);
uh, mistake?

http://codereview.chromium.org/56053/diff/6001/4004#newcode294
Line 294: (scheme.find(".") != std::wstring::npos) ||
hmm I just noticed that this should be std::string::npos (I know you didn't do
it but can you fix it?)

http://codereview.chromium.org/56053/diff/6001/4004#newcode470
Line 470: return UTF8ToWide(SegmentURL(WideToUTF8(text), parts));
you're gonna need to merge this with the latest checkin

http://codereview.chromium.org/56053/diff/6001/4005
File chrome/browser/toolbar_model.cc (right):

http://codereview.chromium.org/56053/diff/6001/4005#newcode1
Line 1: // Copyright (c) 2006-2009 The Chromium Authors. All rights reserved.
I hear you can delete "2006-"

http://codereview.chromium.org/56053/diff/6001/4012
File chrome/common/gfx/text_elider.h (right):

http://codereview.chromium.org/56053/diff/6001/4012#newcode34
Line 34: bool convert_spaces);
add comment for this new variable

[Mohamed Mansour (USE mhm), 2009-04-09 18:30:45]

@estade: fixed your remarks!

http://codereview.chromium.org/56053/diff/6001/4004
File chrome/browser/net/url_fixer_upper.cc (right):

http://codereview.chromium.org/56053/diff/6001/4004#newcode228
Line 228: DCHECK(part);
On 2009/04/09 17:58:12, estade wrote:
> uh, mistake?

Done. Mistake indeed :)

http://codereview.chromium.org/56053/diff/6001/4004#newcode294
Line 294: (scheme.find(".") != std::wstring::npos) ||
On 2009/04/09 17:58:12, estade wrote:
> hmm I just noticed that this should be std::string::npos (I know you didn't do
> it but can you fix it?)

Done.

http://codereview.chromium.org/56053/diff/6001/4004#newcode470
Line 470: return UTF8ToWide(SegmentURL(WideToUTF8(text), parts));
On 2009/04/09 17:58:12, estade wrote:
> you're gonna need to merge this with the latest checkin

Done.

http://codereview.chromium.org/56053/diff/6001/4005
File chrome/browser/toolbar_model.cc (right):

http://codereview.chromium.org/56053/diff/6001/4005#newcode1
Line 1: // Copyright (c) 2006-2009 The Chromium Authors. All rights reserved.
On 2009/04/09 17:58:12, estade wrote:
> I hear you can delete "2006-"

Done.

http://codereview.chromium.org/56053/diff/6001/4012
File chrome/common/gfx/text_elider.h (right):

http://codereview.chromium.org/56053/diff/6001/4012#newcode34
Line 34: bool convert_spaces);
On 2009/04/09 17:58:12, estade wrote:
> add comment for this new variable

Done.

[Evan Stade, 2009-04-09 18:42:05]

http://codereview.chromium.org/56053/diff/4014/6014
File chrome/browser/net/url_fixer_upper.cc (right):

http://codereview.chromium.org/56053/diff/4014/6014#newcode30
Line 30: std::string text_utf8,
tabbing off by 2

http://codereview.chromium.org/56053/diff/4014/6021
File chrome/common/gfx/text_elider.cc (right):

http://codereview.chromium.org/56053/diff/4014/6021#newcode373
Line 373: bool convert_spaces) {
You know on second thought, is there some reason you can't just pass the
UnescapeRule instead of |convert_spaces|? (i.e., is there some reason you'd want
to limit the caller to choosing between SPACES and NORMAL?) Because that would
be more flexible and self-documenting.

[Mohamed Mansour (USE mhm), 2009-04-09 19:48:39]

http://codereview.chromium.org/56053/diff/4014/6014
File chrome/browser/net/url_fixer_upper.cc (right):

http://codereview.chromium.org/56053/diff/4014/6014#newcode30
Line 30: std::string text_utf8,
On 2009/04/09 18:42:05, estade wrote:
> tabbing off by 2

Done.

http://codereview.chromium.org/56053/diff/4014/6021
File chrome/common/gfx/text_elider.cc (right):

http://codereview.chromium.org/56053/diff/4014/6021#newcode373
Line 373: bool convert_spaces) {
On 2009/04/09 18:42:05, estade wrote:
> You know on second thought, is there some reason you can't just pass the
> UnescapeRule instead of |convert_spaces|? (i.e., is there some reason you'd
want
> to limit the caller to choosing between SPACES and NORMAL?) Because that would
> be more flexible and self-documenting.

Done. Maruel agreed as well, so refactored the code to accompany the new param.

[Evan Stade, 2009-04-09 20:04:14]

http://codereview.chromium.org/56053/diff/9029/9030
File chrome/browser/net/url_fixer_upper.cc (right):

http://codereview.chromium.org/56053/diff/9029/9030#newcode508
Line 508: UnescapeRule::NORMAL));
is this backwards? Don't you want to unescape spaces, since it is a a file url?

[Mohamed Mansour (USE mhm), 2009-04-09 20:36:01]

http://codereview.chromium.org/56053/diff/9029/9030
File chrome/browser/net/url_fixer_upper.cc (right):

http://codereview.chromium.org/56053/diff/9029/9030#newcode508
Line 508: UnescapeRule::NORMAL));
On 2009/04/09 20:04:14, estade wrote:
> is this backwards? Don't you want to unescape spaces, since it is a a file
url?

Your right! Done.

[Evan Stade, 2009-04-09 20:43:00]

I think my following comments are all right, but you should independently verify
them.

http://codereview.chromium.org/56053/diff/9039/9043
File chrome/browser/autocomplete/autocomplete.cc (right):

http://codereview.chromium.org/56053/diff/9039/9043#newcode430
Line 430: std::wstring(), NULL, NULL, UnescapeRule::SPACES);
NORMAL (also, isn't this ported to mac now?)

http://codereview.chromium.org/56053/diff/9039/9042
File chrome/browser/autocomplete/history_url_provider.cc (right):

http://codereview.chromium.org/56053/diff/9039/9042#newcode810
Line 810: NULL, NULL, UnescapeRule::SPACES);
NORMAL

http://codereview.chromium.org/56053/diff/9039/9040
File chrome/browser/net/url_fixer_upper.cc (right):

http://codereview.chromium.org/56053/diff/9039/9040#newcode124
Line 124: UnescapeRule::NORMAL));
SPACES

http://codereview.chromium.org/56053/diff/9039/9044
File chrome/browser/tab_contents/navigation_controller.cc (right):

http://codereview.chromium.org/56053/diff/9039/9044#newcode491
Line 491: UnescapeRule::SPACES))));
NORMAL

http://codereview.chromium.org/56053/diff/9039/9045
File chrome/browser/tab_contents/navigation_entry.cc (right):

http://codereview.chromium.org/56053/diff/9039/9045#newcode85
Line 85: display_url_, languages, NULL, NULL, UnescapeRule::SPACES));
NORMAL

http://codereview.chromium.org/56053/diff/9039/9045#newcode88
Line 88: url_, languages, NULL, NULL, UnescapeRule::SPACES));
NORMAL

http://codereview.chromium.org/56053/diff/9039/9041
File chrome/browser/toolbar_model.cc (right):

http://codereview.chromium.org/56053/diff/9039/9041#newcode47
Line 47: UnescapeRule::SPACES);
NORMAL

http://codereview.chromium.org/56053/diff/9039/9047
File chrome/common/gfx/text_elider.cc (right):

http://codereview.chromium.org/56053/diff/9039/9047#newcode461
Line 461: &prefix_end_, UnescapeRule::SPACES));
should be NORMAL

[Mohamed Mansour (USE mhm), 2009-04-13 18:51:12]

@estade: the main reason for this CL is to properly display URLs that have %20
with a space.

For example, bug 8775 stated when navigating page that points to a file that has
a space, it will show up as "My%20File.txt", they wanted "My File.txt", since it
is a space. 

I don't know really, maybe I should make everything NORMAL, and just change
chrome/browser/tab_contents/navigation_controller.cc to SPACES.

[brettw, 2009-04-13 20:35:43]

http://codereview.chromium.org/56053/diff/9039/9042
File chrome/browser/autocomplete/history_url_provider.cc (right):

http://codereview.chromium.org/56053/diff/9039/9042#newcode1
Line 1: // Copyright (c) 2009 The Chromium Authors. All rights reserved.
Use 2006-2009

http://codereview.chromium.org/56053/diff/9039/9040
File chrome/browser/net/url_fixer_upper.cc (right):

http://codereview.chromium.org/56053/diff/9039/9040#newcode1
Line 1: // Copyright (c) 2009 The Chromium Authors. All rights reserved.
Use 2006-2009

http://codereview.chromium.org/56053/diff/9039/9044
File chrome/browser/tab_contents/navigation_controller.cc (right):

http://codereview.chromium.org/56053/diff/9039/9044#newcode491
Line 491: UnescapeRule::SPACES))));
I think we would want spaces for the tab title, actually.

http://codereview.chromium.org/56053/diff/9039/9045
File chrome/browser/tab_contents/navigation_entry.cc (right):

http://codereview.chromium.org/56053/diff/9039/9045#newcode1
Line 1: // Copyright (c) 2009 The Chromium Authors. All rights reserved.
2006-2009

http://codereview.chromium.org/56053/diff/9039/9045#newcode88
Line 88: url_, languages, NULL, NULL, UnescapeRule::SPACES));
I think these two places should probably be spaces.

[Evan Stade, 2009-04-13 20:40:03]

http://codereview.chromium.org/56053/diff/9039/9045
File chrome/browser/tab_contents/navigation_entry.cc (right):

http://codereview.chromium.org/56053/diff/9039/9045#newcode1
Line 1: // Copyright (c) 2009 The Chromium Authors. All rights reserved.
On 2009/04/13 20:35:43, brettw wrote:
> 2006-2009

I told him to just put 2009. Brett I sent you an email about it.

[Mohamed Mansour (USE mhm), 2009-04-13 21:45:45]

Please read 
http://codereview.chromium.org/56053/diff/9039/9040#newcode124

What do you recommend me to do? I changed the tests to reflect  the SPACES
portion. I believe that is the right way since a space is valid in file paths.

I fixed up some lints as well ;x

http://codereview.chromium.org/56053/diff/9039/9043
File chrome/browser/autocomplete/autocomplete.cc (right):

http://codereview.chromium.org/56053/diff/9039/9043#newcode430
Line 430: std::wstring(), NULL, NULL, UnescapeRule::SPACES);
On 2009/04/09 20:43:00, Evan Stade wrote:
> NORMAL (also, isn't this ported to mac now?)

Done. Correct, its ported to mac

http://codereview.chromium.org/56053/diff/9039/9042
File chrome/browser/autocomplete/history_url_provider.cc (right):

http://codereview.chromium.org/56053/diff/9039/9042#newcode1
Line 1: // Copyright (c) 2009 The Chromium Authors. All rights reserved.
On 2009/04/13 20:35:43, brettw wrote:
> Use 2006-2009
I have been told to use 2009, since I used 2006-2009

http://codereview.chromium.org/56053/diff/9039/9042#newcode810
Line 810: NULL, NULL, UnescapeRule::SPACES);
On 2009/04/09 20:43:00, Evan Stade wrote:
> NORMAL

Done.

http://codereview.chromium.org/56053/diff/9039/9040
File chrome/browser/net/url_fixer_upper.cc (right):

http://codereview.chromium.org/56053/diff/9039/9040#newcode124
Line 124: UnescapeRule::NORMAL));
On 2009/04/09 20:43:00, Evan Stade wrote:
> SPACES

This should stay as NORMAL, because the unittest URLFixerUpperTest.FixupFile
will fail on this string:
// File URLs go through GURL, which tries to escape 
// intelligently.
{"c:\\This%20is a non-existent file.txt", "",
 "file:///C:/This%2520is%20a%20non-existent%20file.txt"}

Unless you want me to change the unittest to reflect the spaces ?

http://codereview.chromium.org/56053/diff/9039/9044
File chrome/browser/tab_contents/navigation_controller.cc (right):

http://codereview.chromium.org/56053/diff/9039/9044#newcode491
Line 491: UnescapeRule::SPACES))));
On 2009/04/13 20:35:43, brettw wrote:
> I think we would want spaces for the tab title, actually.
> 

Done.

http://codereview.chromium.org/56053/diff/9039/9045
File chrome/browser/tab_contents/navigation_entry.cc (right):

http://codereview.chromium.org/56053/diff/9039/9045#newcode1
Line 1: // Copyright (c) 2009 The Chromium Authors. All rights reserved.
On 2009/04/13 20:40:04, Evan Stade wrote:
> On 2009/04/13 20:35:43, brettw wrote:
> > 2006-2009
> 
> I told him to just put 2009. Brett I sent you an email about it.

Done.

http://codereview.chromium.org/56053/diff/9039/9045#newcode85
Line 85: display_url_, languages, NULL, NULL, UnescapeRule::SPACES));
On 2009/04/09 20:43:00, Evan Stade wrote:
> NORMAL

Done.

http://codereview.chromium.org/56053/diff/9039/9045#newcode88
Line 88: url_, languages, NULL, NULL, UnescapeRule::SPACES));
On 2009/04/13 20:35:43, brettw wrote:
> I think these two places should probably be spaces.
> 

Done.

http://codereview.chromium.org/56053/diff/9039/9041
File chrome/browser/toolbar_model.cc (right):

http://codereview.chromium.org/56053/diff/9039/9041#newcode47
Line 47: UnescapeRule::SPACES);
On 2009/04/09 20:43:00, Evan Stade wrote:
> NORMAL

Done.

http://codereview.chromium.org/56053/diff/9039/9047
File chrome/common/gfx/text_elider.cc (right):

http://codereview.chromium.org/56053/diff/9039/9047#newcode461
Line 461: &prefix_end_, UnescapeRule::SPACES));
On 2009/04/09 20:43:00, Evan Stade wrote:
> should be NORMAL

Done.

[Mohamed Mansour (USE mhm), 2009-04-13 22:05:27]

Beh, in net_util.cc FilePathToFileURL is failing from this test case:
TEST(URLFixerUpperTest, FixupFile) 

because its doing this:
std::string golden = net::FilePathToFileURL(original).spec();

So what should we do? Let it be NORMAL again? Or let FilePathToFileURL take
account of the space?

[Mohamed Mansour (USE mhm), 2009-04-13 22:25:41]

Reverted the change to NORMAL files, passes tests now, unless that should be
changed/

[Mohamed Mansour (USE mhm), 2009-04-16 19:23:35]

Hi, if anyone could look into this before the source gets out of sync. Thanks :)

[Mohamed Mansour (USE mhm), 2009-05-27 01:23:16]

On 2009/04/16 19:23:35, Mohamed Mansour wrote:
> Hi, if anyone could look into this before the source gets out of sync. Thanks
:)

I guess this is no longer needed ? No comments for since 5 weeks :)

[M-A Ruel, 2009-05-27 01:27:47]

On 2009/05/27 01:23:16, Mohamed Mansour wrote:
> I guess this is no longer needed ? No comments for since 5 weeks :)


I'd like Brett to accept this patch.

[M-A Ruel, 2009-05-27 01:28:02]

On 2009/05/27 01:27:47, M-A wrote:
> On 2009/05/27 01:23:16, Mohamed Mansour wrote:
> > I guess this is no longer needed ? No comments for since 5 weeks :)
> 
> 
> I'd like Brett to accept this patch.

I mean to review.

[brettw, 2009-05-27 16:46:21]

Ah, sorry. For future reference, you don't need to wait so long before
complaining. If you don't hear anything in a few days, it's expected you bother
the reviewers...

The bad news is that this changed around since you made the patch.
GetCleanStringForURL, which was horribly named, noved to net_util and is called
FormatURL. There are now two forms of FormatURL, so you can put your additional
options in there without changing all the callers, who mostly use the simple
form.

Sorry for the extra work, it's my fault for missing this :(

[Mohamed Mansour (USE mhm), 2009-05-27 16:50:20]

Its okay, I will close this CL and revisit this bug with a clean sync.

[brettw, 2009-05-27 16:55:19]

Generally it's easier to see the history if you use this same CL, so reviewers
can tell what was discussed before. This can be difficult in some case, though,
so if you need to make a new one, just be sure to reference this previous review
in it.

[Mohamed Mansour (USE mhm), 2009-05-27 18:05:10]

Correct, I will include the linkage. The reason why I will start a new CL is
because I lost the gcl_info for this and I don't know how to hookup my gcl
upload to this CL. I will reference this with the new CL once I start working on
it, after I finish my daily job :)

[Mohamed Mansour (USE mhm), 2009-05-28 04:34:32]

Alright here is the new CL (synced tree).

I changed the net utils test and they all pass successfully. And the bug  seems
to be fixed (I needed the toolbar_model changed since it depends on that).

[Mohamed Mansour (USE mhm), 2009-05-29 16:41:06]

Do you guys think this is a good approach?

[Evan Stade, 2009-05-29 19:32:14]

Drive by review.

General nit: many places you use the term "escape" when you mean "unescape".
Please fix that for clarity.

http://codereview.chromium.org/56053/diff/13028/12043
File net/base/net_util.cc (right):

http://codereview.chromium.org/56053/diff/13028/12043#newcode659
Line 659: // and the resulting range will be filled into |out_component|.
|escape_rules|
can you make this read

  |unescape_rules| defines how to clean the URL for human readability.

http://codereview.chromium.org/56053/diff/13028/12043#newcode663
Line 663: UnescapeRule::Type escape_rules,
nit: |unescape_rules|

http://codereview.chromium.org/56053/diff/13028/12041
File net/base/net_util_unittest.cc (right):

http://codereview.chromium.org/56053/diff/13028/12041#newcode1097
Line 1097: {"unescape=true",
can you update the descriptions (here and L1088)

http://codereview.chromium.org/56053/diff/13028/12041#newcode1105
Line 1105: {"Escape Spaces",
nit: s/Escape/Unescape

[Mohamed Mansour (USE mhm), 2009-05-29 22:01:56]

Alright, I have looked through the CL and fixed the clarity of "unescape". You
guys could review it one last time.

http://codereview.chromium.org/56053/diff/13028/12043
File net/base/net_util.cc (right):

http://codereview.chromium.org/56053/diff/13028/12043#newcode659
Line 659: // and the resulting range will be filled into |out_component|.
|escape_rules|
On 2009/05/29 19:32:14, Evan Stade wrote:
> can you make this read
> 
>   |unescape_rules| defines how to clean the URL for human readability.

Done.

http://codereview.chromium.org/56053/diff/13028/12043#newcode663
Line 663: UnescapeRule::Type escape_rules,
On 2009/05/29 19:32:14, Evan Stade wrote:
> nit: |unescape_rules|

Done.

[Evan Stade, 2009-05-29 22:11:31]

I think there are some uses of FormatUrl that you missed, for example in
render_view_context_menu.cc. Aside from that LGTM.

[Mohamed Mansour (USE mhm), 2009-05-29 22:55:24]

On 2009/05/29 22:11:31, Evan Stade wrote:
> I think there are some uses of FormatUrl that you missed, for example in
> render_view_context_menu.cc. Aside from that LGTM.

Good catch, I forgot two places. I should be more careful next time.

[brettw, 2009-06-01 17:47:08]

This patch is looking pretty good! I have some hopefully minor comments. Sorry I
took so long to get back to you (pretty backed up right now).

http://codereview.chromium.org/56053/diff/12077/12085
File chrome/browser/bookmarks/bookmark_table_model.cc (right):

http://codereview.chromium.org/56053/diff/12077/12085#newcode320
Line 320: net::FormatUrl(node->GetURL(), languages, false, UnescapeRule::NORMAL,
I think we can probably handle ::SPACES here.

http://codereview.chromium.org/56053/diff/12077/12086
File chrome/browser/tab_contents/navigation_controller.cc (right):

http://codereview.chromium.org/56053/diff/12077/12086#newcode364
Line 364: file_util::GetFilenameFromPath(net::FormatUrl(
This should be indented 4 spaces. If we can change the default mode to SPACES,
can you use the simpler version of FormatURL here?

http://codereview.chromium.org/56053/diff/12077/12087
File chrome/browser/tab_contents/navigation_entry.cc (right):

http://codereview.chromium.org/56053/diff/12077/12087#newcode84
Line 84: display_url_, languages, true, UnescapeRule::SPACES, NULL, NULL));
Same comment as previously about the simpler version.

http://codereview.chromium.org/56053/diff/12077/12079
File net/base/escape.h (right):

http://codereview.chromium.org/56053/diff/12077/12079#newcode50
Line 50: // symbols will get unescaped with this mode.
Can you add "All other unescape rules imply "normal" in addition to their
special meaning." after "absense of them)."

http://codereview.chromium.org/56053/diff/12077/12082
File net/base/net_util.h (right):

http://codereview.chromium.org/56053/diff/12077/12082#newcode188
Line 188: // omitted. |unescape_rules| defines how to clean the URL for human
readability.
After this "...readability." sentence, I'd like to add another little comment to
help people.

"You will generally want |UnescapeRule::SPACES| for display to the user if you
can handle spaces, or |UnescapeRule::NORMAL| if not."

http://codereview.chromium.org/56053/diff/12077/12082#newcode204
Line 204: return FormatUrl(url, languages, true, UnescapeRule::NORMAL, NULL,
NULL);
Do you think it makes sense to have the default be ::SPACES? I can't think of a
case where SPACES is inappropriate yet you want unescaping.

[Evan Stade, 2009-06-01 17:53:21]

noticed some spelling errors (that you didn't make)

http://codereview.chromium.org/56053/diff/12077/12079
File net/base/escape.h (right):

http://codereview.chromium.org/56053/diff/12077/12079#newcode49
Line 49: // just the absense of them). Things like escaped letters, digits, and
most
nit: s/absense/absence

http://codereview.chromium.org/56053/diff/12077/12079#newcode60
Line 60: // including '%', '+', '&', '/', '#'. If we unescaped these charaters,
the
nit: s/charaters/characters

http://codereview.chromium.org/56053/diff/12077/12079#newcode66
Line 66: // Unescapes control characters such as %01. This INCLUDES NULLs!. This
is
nit: s/!./.

[Mohamed Mansour (USE mhm), 2009-06-02 01:06:38]

Please review, I ran the test cases (unit_test and net_tests) and verified they
are successful.

Please note that url_fixer_upper requires a NORMAL unescape rule, because files
shouldn't unescape %20 if they are initially there. Example, if we have the
following:
"c:\\This%20is a non-existent file.txt"

The URLFixerUpper::FixupFile would be:
"file:///C:/This%2520is%20a%20non-existent%20file.txt"

http://codereview.chromium.org/56053/diff/12077/12085
File chrome/browser/bookmarks/bookmark_table_model.cc (right):

http://codereview.chromium.org/56053/diff/12077/12085#newcode320
Line 320: net::FormatUrl(node->GetURL(), languages, false, UnescapeRule::NORMAL,
On 2009/06/01 17:47:08, brettw wrote:
> I think we can probably handle ::SPACES here.

Done.

http://codereview.chromium.org/56053/diff/12077/12086
File chrome/browser/tab_contents/navigation_controller.cc (right):

http://codereview.chromium.org/56053/diff/12077/12086#newcode364
Line 364: file_util::GetFilenameFromPath(net::FormatUrl(
On 2009/06/01 17:47:08, brettw wrote:
> This should be indented 4 spaces. If we can change the default mode to SPACES,
> can you use the simpler version of FormatURL here?

Done.

http://codereview.chromium.org/56053/diff/12077/12087
File chrome/browser/tab_contents/navigation_entry.cc (right):

http://codereview.chromium.org/56053/diff/12077/12087#newcode84
Line 84: display_url_, languages, true, UnescapeRule::SPACES, NULL, NULL));
On 2009/06/01 17:47:08, brettw wrote:
> Same comment as previously about the simpler version.

Done.

http://codereview.chromium.org/56053/diff/12077/12084
File chrome/browser/toolbar_model.cc (right):

http://codereview.chromium.org/56053/diff/12077/12084#newcode44
Line 44: NULL);
converted that to shorter version.

http://codereview.chromium.org/56053/diff/12077/12079
File net/base/escape.h (right):

http://codereview.chromium.org/56053/diff/12077/12079#newcode49
Line 49: // just the absense of them). Things like escaped letters, digits, and
most
On 2009/06/01 17:53:22, Evan Stade wrote:
> nit: s/absense/absence

Done.

http://codereview.chromium.org/56053/diff/12077/12079#newcode50
Line 50: // symbols will get unescaped with this mode.
On 2009/06/01 17:47:08, brettw wrote:
> Can you add "All other unescape rules imply "normal" in addition to their
> special meaning." after "absense of them)."

Done.

http://codereview.chromium.org/56053/diff/12077/12079#newcode60
Line 60: // including '%', '+', '&', '/', '#'. If we unescaped these charaters,
the
On 2009/06/01 17:53:22, Evan Stade wrote:
> nit: s/charaters/characters

Done.

http://codereview.chromium.org/56053/diff/12077/12079#newcode66
Line 66: // Unescapes control characters such as %01. This INCLUDES NULLs!. This
is
On 2009/06/01 17:53:22, Evan Stade wrote:
> nit: s/!./.

Done.

http://codereview.chromium.org/56053/diff/12077/12082
File net/base/net_util.h (right):

http://codereview.chromium.org/56053/diff/12077/12082#newcode188
Line 188: // omitted. |unescape_rules| defines how to clean the URL for human
readability.
On 2009/06/01 17:47:08, brettw wrote:
> After this "...readability." sentence, I'd like to add another little comment
to
> help people.
> 
> "You will generally want |UnescapeRule::SPACES| for display to the user if you
> can handle spaces, or |UnescapeRule::NORMAL| if not."

Done.

http://codereview.chromium.org/56053/diff/12077/12082#newcode204
Line 204: return FormatUrl(url, languages, true, UnescapeRule::NORMAL, NULL,
NULL);
On 2009/06/01 17:47:08, brettw wrote:
> Do you think it makes sense to have the default be ::SPACES? I can't think of
a
> case where SPACES is inappropriate yet you want unescaping.

Done. Thats what I wanted to do in my earlier changes.

[brettw, 2009-06-02 20:37:27]

LGTM!