Convert Punycode domains to Unicode in URLs shown in the followings:...

Shows Unicode IDN instead of Punycode in the followings:
 - Bookmark Manager
 - Edit Bookmark dialog opened by Bookmark Manager
 - Edit Bookmark dialog opened by the star on the left of the address bar

Introduces new function, net::FormatUrl(), which has the following
parameters in addition to gfx::GetCleanStringFromUrl().
 - bool omit_username_password
 - bool unescape
and moves gfx::GetClienStringFromUrl() to net:: namespace, and removed
the last two parameters.


BUG=3991

[jungshik at Google, 2009-05-14 21:31:13]

lgtm with a minor issue below taken care of.

http://codereview.chromium.org/115346/diff/1012/17
File net/base/net_util_unittest.cc (right):

http://codereview.chromium.org/115346/diff/1012/17#newcode701
Line 701: EXPECT_EQ(L"pre http://\x671d\x65e5\x3042\x3055\x3072.jp/", output);
The same test with 'en' and empty lang  would be nice  although many cases like
that are tested in another test.

[tkent_google.com, 2009-05-15 01:24:09]

Thanks for reviewing.

Would you submit this if it is ok, please?

http://codereview.chromium.org/115346/diff/1012/17
File net/base/net_util_unittest.cc (right):

http://codereview.chromium.org/115346/diff/1012/17#newcode701
Line 701: EXPECT_EQ(L"pre http://\x671d\x65e5\x3042\x3055\x3072.jp/", output);
On 2009/05/14 21:31:13, Jungshik Shin wrote:
> The same test with 'en' and empty lang  would be nice  although many cases
like
> that are tested in another test. 

Done.

[tkent_google.com, 2009-05-15 01:32:42]

The patch is conflicting.  I'm resolving it now.

[tkent_google.com, 2009-05-15 04:31:49]

On 2009/05/15 01:32:42, Kent wrote:
> The patch is conflicting.  I'm resolving it now.

The conflict was resolved and "gcl try" was success.

[jungshik at Google, 2009-05-17 21:59:22]

Brett, are you ok with this CL? Then, I'll land it on Kent's behalf. 



On 2009/05/15 04:31:49, Kent wrote:
> On 2009/05/15 01:32:42, Kent wrote:
> > The patch is conflicting.  I'm resolving it now.
> 
> The conflict was resolved and "gcl try" was success.

[brettw, 2009-05-17 23:12:15]

I think the relationship between this an GetCleanStringFromUrl is very
confusing. If they do the same thing except for the username/password, it seems
like they shouldn't be different functions in totally different places in the
code.

I think there should be only one function with a bool that indicates whether the
username/password should be displayed. Then the text elider and all other
callers would use that, and the text elider would be used only for eliding
(which makes more sense anyway).

What do you think?

[tkent_google.com, 2009-05-18 10:39:42]

The patch merging GetClearStringFromUrl() and AppendDiaplayURL() is ready. 
Would you review it please?

[tkent_google.com, 2009-05-19 06:11:58]

I moved the main implementation to net:: namespace, and modified all of callers.
Would you take a look at the code please?

[brettw, 2009-05-19 17:26:32]

Thanks, this is the reorganization I was looking for. I have some more comments,
but hopefully this is the last pass.

http://codereview.chromium.org/115346/diff/69/1054
File net/base/net_util.cc (right):

http://codereview.chromium.org/115346/diff/69/1054#newcode1071
Line 1071: void AppendFormattedComponent(const std::string& spec,
I'd probably just move this function to the anonymous namespace at the top of
the file, where there are already some file-static functions. You don't need the
forward-declare then, either.

http://codereview.chromium.org/115346/diff/69/1054#newcode1094
Line 1094: const GURL& url, const std::wstring& languages, bool
omit_username_password,
I would probably also put these arguments on different lines.

http://codereview.chromium.org/115346/diff/69/1055
File net/base/net_util.h (right):

http://codereview.chromium.org/115346/diff/69/1055#newcode180
Line 180: // omitted. If |unescape| is ture and the path part and the query part
seem to
ture -> true

http://codereview.chromium.org/115346/diff/69/1055#newcode186
Line 186: const GURL& url, const std::wstring& languages, bool
omit_username_password,
We normally put the arguments on different lines in this case. You can put them
all aligned after the ( since there's room.

http://codereview.chromium.org/115346/diff/69/1055#newcode195
Line 195: inline std::wstring GetCleanStringFromUrl(const GURL& url,
I've always disliked the name for this function. Now that you're expressing it
in terms of another function, it would be nice if the name reflected that
similarity. I asked Scott what he thought, and he suggested just calling it
FormatURL as well. I think that this is OK, so you can specify all the options
or not.

Can you also do a little work on the comments? Some parts of the comment for
this function are out-of-date (like the discussion of the parsed out param). Can
you make sure all of these details are in the first version, and then maybe just
give a small comment for this one.

http://codereview.chromium.org/115346/diff/69/1053
File net/base/net_util_unittest.cc (right):

http://codereview.chromium.org/115346/diff/69/1053#newcode985
Line 985: TEST(NetUtilTest, FormatUrl) {
Thanks for the nice tests!

[tkent_google.com, 2009-05-20 02:32:27]

Thanks for the review.

http://codereview.chromium.org/115346/diff/69/1055
File net/base/net_util.h (right):

http://codereview.chromium.org/115346/diff/69/1055#newcode180
Line 180: // omitted. If |unescape| is ture and the path part and the query part
seem to
On 2009/05/19 17:26:32, brettw wrote:
> ture -> true

Done.

http://codereview.chromium.org/115346/diff/69/1055#newcode186
Line 186: const GURL& url, const std::wstring& languages, bool
omit_username_password,
On 2009/05/19 17:26:32, brettw wrote:
> We normally put the arguments on different lines in this case. You can put
them
> all aligned after the ( since there's room.

Done.

http://codereview.chromium.org/115346/diff/69/1055#newcode195
Line 195: inline std::wstring GetCleanStringFromUrl(const GURL& url,
On 2009/05/19 17:26:32, brettw wrote:
> I've always disliked the name for this function. Now that you're expressing it
> in terms of another function, it would be nice if the name reflected that
> similarity. I asked Scott what he thought, and he suggested just calling it
> FormatURL as well. I think that this is OK, so you can specify all the options
> or not.
> 
> Can you also do a little work on the comments? Some parts of the comment for
> this function are out-of-date (like the discussion of the parsed out param).
Can
> you make sure all of these details are in the first version, and then maybe
just
> give a small comment for this one.

ok, I have renamed it to FormatUrl(), and cleaned the comment.

[brettw, 2009-05-20 22:34:34]

LGTM! Thanks for putting up with this long review.