"domain" item added to HistoryEntry to allow showing IDN in chrome://history/

"domain" item added to HistoryEntry to allow showing IDN in chrome://history/

BUG=272732
TEST=Try visiting any IDN site and confirm no punycode is shown in history.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221822

[Patrick Dubroy, 2013-08-20 14:26:37]

Sorry for the slow review -- I was away on vacation for a few days.

The implementation mostly looks good to me, but I'd like to loop in someone with
more i18n expertise to confirm that this is the right way to do things.

https://codereview.chromium.org/22862006/diff/10001/chrome/browser/ui/webui/h...
File chrome/browser/ui/webui/history_ui.cc (right):

https://codereview.chromium.org/22862006/diff/10001/chrome/browser/ui/webui/h...
chrome/browser/ui/webui/history_ui.cc:325: void
BrowsingHistoryHandler::HistoryEntry::SetDisplayableDomain(
I don't see much value in having this as a separate function -- I would just
inline it at the call site.

https://codereview.chromium.org/22862006/diff/10001/chrome/browser/ui/webui/h...
chrome/browser/ui/webui/history_ui.cc:973: return profile ?
profile->GetPrefs()->GetString(prefs::kAcceptLanguages) :
Nit: looks like this actually fits on one line.

https://codereview.chromium.org/22862006/diff/10001/chrome/browser/ui/webui/h...
File chrome/browser/ui/webui/history_ui.h (right):

https://codereview.chromium.org/22862006/diff/10001/chrome/browser/ui/webui/h...
chrome/browser/ui/webui/history_ui.h:91: std::string languages;
Nit: Might as well call this "accept_languages".

https://codereview.chromium.org/22862006/diff/10001/chrome/browser/ui/webui/h...
chrome/browser/ui/webui/history_ui.h:175: std::string GetLanguages() const;
...and this, "GetAcceptLanguages()"

https://codereview.chromium.org/22862006/diff/10001/chrome/browser/ui/webui/h...
File chrome/browser/ui/webui/history_ui_unittest.cc (right):

https://codereview.chromium.org/22862006/diff/10001/chrome/browser/ui/webui/h...
chrome/browser/ui/webui/history_ui_unittest.cc:134: TEST(HistoryUITest,
SetDisplayableDomain) {
I think it would be much more valuable to add a test to history_browsertest.js.

[brettw, 2013-08-21 21:54:48]

I think the C++ extraction looks correct. We should be doing that. The only
suggestion I have is that we should be doing it more (see the TODO in
getDomainFromURL).

https://codereview.chromium.org/22862006/diff/10001/chrome/browser/resources/...
File chrome/browser/resources/history/history.js (left):

https://codereview.chromium.org/22862006/diff/10001/chrome/browser/resources/...
chrome/browser/resources/history/history.js:179: domain.textContent =
this.getDomainFromURL_(this.url_);
It seems like we should be updating every place where getDomainFromURL is
called. There are clearly some cases like the visit attempt stuff that have the
same bug.

[yuusuke, 2013-08-22 12:23:38]

* Removed getDomainFromURL_ - now all |domain| variables in history.js are in
unicode
 * URLDatabase::GetTextMatches updated - now we can search IDN in unicode.
history.js requires such behaviour for "More from this site" option
 * Tests moved to history_browsertest.js and history_querying_unittest.cc
 * CodeStyle fixed

Can anyone "try" this patch for me?

[yuusuke, 2013-08-26 13:51:08]

On 2013/08/21 21:54:48, brettw wrote:
> I think the C++ extraction looks correct. We should be doing that. The only
> suggestion I have is that we should be doing it more (see the TODO in
> getDomainFromURL).
> 
>
https://codereview.chromium.org/22862006/diff/10001/chrome/browser/resources/...
> File chrome/browser/resources/history/history.js (left):
> 
>
https://codereview.chromium.org/22862006/diff/10001/chrome/browser/resources/...
> chrome/browser/resources/history/history.js:179: domain.textContent =
> this.getDomainFromURL_(this.url_);
> It seems like we should be updating every place where getDomainFromURL is
> called. There are clearly some cases like the visit attempt stuff that have
the
> same bug.

Fixed

[yuusuke, 2013-08-26 13:51:22]

On 2013/08/20 14:26:37, dubroy wrote:
> Sorry for the slow review -- I was away on vacation for a few days.
> 
> The implementation mostly looks good to me, but I'd like to loop in someone
with
> more i18n expertise to confirm that this is the right way to do things.
> 
>
https://codereview.chromium.org/22862006/diff/10001/chrome/browser/ui/webui/h...
> File chrome/browser/ui/webui/history_ui.cc (right):
> 
>
https://codereview.chromium.org/22862006/diff/10001/chrome/browser/ui/webui/h...
> chrome/browser/ui/webui/history_ui.cc:325: void
> BrowsingHistoryHandler::HistoryEntry::SetDisplayableDomain(
> I don't see much value in having this as a separate function -- I would just
> inline it at the call site.
> 
>
https://codereview.chromium.org/22862006/diff/10001/chrome/browser/ui/webui/h...
> chrome/browser/ui/webui/history_ui.cc:973: return profile ?
> profile->GetPrefs()->GetString(prefs::kAcceptLanguages) :
> Nit: looks like this actually fits on one line.
> 
>
https://codereview.chromium.org/22862006/diff/10001/chrome/browser/ui/webui/h...
> File chrome/browser/ui/webui/history_ui.h (right):
> 
>
https://codereview.chromium.org/22862006/diff/10001/chrome/browser/ui/webui/h...
> chrome/browser/ui/webui/history_ui.h:91: std::string languages;
> Nit: Might as well call this "accept_languages".
> 
>
https://codereview.chromium.org/22862006/diff/10001/chrome/browser/ui/webui/h...
> chrome/browser/ui/webui/history_ui.h:175: std::string GetLanguages() const;
> ...and this, "GetAcceptLanguages()"
> 
>
https://codereview.chromium.org/22862006/diff/10001/chrome/browser/ui/webui/h...
> File chrome/browser/ui/webui/history_ui_unittest.cc (right):
> 
>
https://codereview.chromium.org/22862006/diff/10001/chrome/browser/ui/webui/h...
> chrome/browser/ui/webui/history_ui_unittest.cc:134: TEST(HistoryUITest,
> SetDisplayableDomain) {
> I think it would be much more valuable to add a test to
history_browsertest.js.

Fixed

[yuusuke, 2013-08-29 13:47:21]

Ping... Anyone? :) I'm new to this whole "contribute" thing - can anyone guide
me?

[Patrick Dubroy, 2013-08-29 15:17:14]

On 2013/08/29 13:47:21, yuusuke wrote:
> Ping... Anyone? :) I'm new to this whole "contribute" thing - can anyone guide
> me?

Sorry for the delay...will take a look later today. In general, I think it's ok
to ping reviewers if you haven't gotten a response in 24 hours. Sometimes we get
busy with other things :-)

[Patrick Dubroy, 2013-08-29 17:20:05]

On 2013/08/29 15:17:14, dubroy wrote:
> On 2013/08/29 13:47:21, yuusuke wrote:
> > Ping... Anyone? :) I'm new to this whole "contribute" thing - can anyone
guide
> > me?
> 
> Sorry for the delay...will take a look later today. In general, I think it's
ok
> to ping reviewers if you haven't gotten a response in 24 hours. Sometimes we
get
> busy with other things :-)

Stuff under chrome/test/data, chrome/browser/ui, and chrome/browser/resources
LGTM.

Brett, do you know if this is the right thing to do from an i18n perspective? Or
is there someone else who should review?

[Patrick Dubroy, 2013-08-29 17:23:21]

On 2013/08/29 13:47:21, yuusuke wrote:
> Ping... Anyone? :) I'm new to this whole "contribute" thing - can anyone guide
> me?

BTW, normally when a reviewer leaves you comments, it's best to respond to each
one in the code review tool. Normally, you can just click "Done" to indicate
that you have addressed the comment.

[yuusuke, 2013-08-29 17:40:08]

On 2013/08/29 17:23:21, dubroy wrote:
> BTW, normally when a reviewer leaves you comments, it's best to respond to
each
> one in the code review tool. Normally, you can just click "Done" to indicate
> that you have addressed the comment.

Thanks, didn't notice it before. Nice feature/button. Will use it :)

[yuusuke, 2013-09-02 06:40:11]

Brettw, Sky, could you please review remaining chrome/browser/history/* files?

Also, I haven't "tried" this patch via try servers (don't have necessary
access/right) - can anyone do it for me?

[Pam (message me for reviews), 2013-09-02 09:24:46]

FYI - sky and brettw are in the US, where it's a holiday today even once the sun
rises. I've started the tryservers for you.

- Pam

[yuusuke, 2013-09-02 18:07:08]

Broken RangeHistoryWebUITest tests fixed
Compilation on non-Win platform fixed

Still waiting for review of chrome/browser/history/*

It would be nice if someone could "try" this patch for me.

[brettw, 2013-09-03 20:44:31]

Sorry for the delay. If you're waiting a few days for me please feel free to
email me separately for a ping.

https://codereview.chromium.org/22862006/diff/58001/chrome/browser/history/hi...
File chrome/browser/history/history_querying_unittest.cc (right):

https://codereview.chromium.org/22862006/diff/58001/chrome/browser/history/hi...
chrome/browser/history/history_querying_unittest.cc:445: // Tests IDN text
search by both punycode and UTF.
"UTF": I'd write "ASCII" instead.

https://codereview.chromium.org/22862006/diff/58001/chrome/browser/history/ur...
File chrome/browser/history/url_database.cc (right):

https://codereview.chromium.org/22862006/diff/58001/chrome/browser/history/ur...
chrome/browser/history/url_database.cc:380: string16 ascii =
base::i18n::ToLower(base::ASCIIToUTF16(gurl.host()));
You don't need this stuff. A GURL's host is guaranteed 8-bit lowercase ASCII.

https://codereview.chromium.org/22862006/diff/58001/chrome/browser/history/ur...
chrome/browser/history/url_database.cc:381: string16 utf =
base::i18n::ToLower(net::IDNToUnicode(gurl.host(), ""));
I don't think you need to ToLower call here. I believe the IDNToUnicode result
should also be "pretty much" canonicalized according to various rules.

Additionally, I'd pass std::string() instead of "" since it ends up being
measurably less generated code and is more precise (you're passing an empty
string object rather than an empty C string).

https://codereview.chromium.org/22862006/diff/58001/chrome/browser/ui/webui/h...
File chrome/browser/ui/webui/history_ui.cc (right):

https://codereview.chromium.org/22862006/diff/58001/chrome/browser/ui/webui/h...
chrome/browser/ui/webui/history_ui.cc:968: if (!profile)
I don't think having a null profile is possible here. Was there some reason you
added this code? We generally prefer to have very clear rules for what things
can be null, and never null check otherwise.

[yuusuke, 2013-09-04 08:31:12]

All comments addressed. Please "try" this patch for me (still not sure if linux
compilation was fixed).

https://codereview.chromium.org/22862006/diff/58001/chrome/browser/history/hi...
File chrome/browser/history/history_querying_unittest.cc (right):

https://codereview.chromium.org/22862006/diff/58001/chrome/browser/history/hi...
chrome/browser/history/history_querying_unittest.cc:445: // Tests IDN text
search by both punycode and UTF.
On 2013/09/03 20:44:31, brettw wrote:
> "UTF": I'd write "ASCII" instead.

Done.

https://codereview.chromium.org/22862006/diff/58001/chrome/browser/history/ur...
File chrome/browser/history/url_database.cc (right):

https://codereview.chromium.org/22862006/diff/58001/chrome/browser/history/ur...
chrome/browser/history/url_database.cc:380: string16 ascii =
base::i18n::ToLower(base::ASCIIToUTF16(gurl.host()));
On 2013/09/03 20:44:31, brettw wrote:
> You don't need this stuff. A GURL's host is guaranteed 8-bit lowercase ASCII.

Done.

https://codereview.chromium.org/22862006/diff/58001/chrome/browser/history/ur...
chrome/browser/history/url_database.cc:381: string16 utf =
base::i18n::ToLower(net::IDNToUnicode(gurl.host(), ""));
On 2013/09/03 20:44:31, brettw wrote:
> I don't think you need to ToLower call here. I believe the IDNToUnicode result
> should also be "pretty much" canonicalized according to various rules.
> 
> Additionally, I'd pass std::string() instead of "" since it ends up being
> measurably less generated code and is more precise (you're passing an empty
> string object rather than an empty C string).

Done.

https://codereview.chromium.org/22862006/diff/58001/chrome/browser/ui/webui/h...
File chrome/browser/ui/webui/history_ui.cc (right):

https://codereview.chromium.org/22862006/diff/58001/chrome/browser/ui/webui/h...
chrome/browser/ui/webui/history_ui.cc:968: if (!profile)
On 2013/09/03 20:44:31, brettw wrote:
> I don't think having a null profile is possible here. Was there some reason
you
> added this code? We generally prefer to have very clear rules for what things
> can be null, and never null check otherwise.

Done.

[brettw, 2013-09-06 17:35:26]

lgtm

[commit-bot: I haz the power, 2013-09-06 20:05:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yuusuke@yandex-team.ru/22862006/58002

[commit-bot: I haz the power, 2013-09-07 00:06:56]

Change committed as 221822