Omnibox - Open Search - Handle Lack of Short Name Smartly

Omnibox - Open Search - Handle Lack of Short Name Smartly

BUG=714081
Review-Url: https://codereview.chromium.org/2902043004
Cr-Commit-Position: refs/heads/master@{#476125}
Committed: https://chromium.googlesource.com/chromium/src/+/ee5a39ceca0897f43f63059d44635da6f02d0a50

[Mark P, 2017-05-24 20:21:31]

Description was changed from

==========
Omnibox - Open Search - Handle Lack of Short Name Smartly

BUG=714081
==========

to

==========
Omnibox - Open Search - Handle Lack of Short Name Smartly

BUG=714081
==========

[Mark P, 2017-05-24 20:21:32]

mpearson@chromium.org changed reviewers:
+ pkasting@chromium.org

[Mark P, 2017-05-24 20:23:06]

Peter, take a look at your leisure, no hurry.

I didn't audit all the SetShortName() calls when removing the DCHECK.  I did
glance at them to see if there were any other obvious places where I should add
a fallback (use a different short name if one isn't provided) and didn't see any
others.  Otherwise I'll assume that things that previously would've hit this
DCHECK will crash elsewhere now.

--mark

[Peter Kasting, 2017-05-25 07:25:04]

Note: I'm behind on codereviews but will try and get to this today (Thursday May
25), after I get some sleep.

[Mark P, 2017-05-25 18:04:24]

On 2017/05/25 07:25:04, Peter Kasting wrote:
> Note: I'm behind on codereviews but will try and get to this today (Thursday
May
> 25), after I get some sleep.

No problem.  I will always be clear about when something is urgent.  This isn't.
 It can wait weeks.

--mark

[Peter Kasting, 2017-05-26 01:38:40]

LGTM

https://codereview.chromium.org/2902043004/diff/1/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_fetcher_unittest.cc (right):

https://codereview.chromium.org/2902043004/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_fetcher_unittest.cc:192:
ASSERT_FALSE(test_util()->model()->GetTemplateURLForKeyword(keyword));
Nit: I'd remove all the checks here that just duplicate the test above and make
this test simply about the short name fallback.

So, doesn't seem like we really care about this

https://codereview.chromium.org/2902043004/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_fetcher_unittest.cc:196: EXPECT_EQ(0,
requests_completed());
Nit: Or this

https://codereview.chromium.org/2902043004/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_fetcher_unittest.cc:199: EXPECT_EQ(1,
requests_completed());
Nit: Or this

https://codereview.chromium.org/2902043004/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_fetcher_unittest.cc:206:
t_url->url_ref().DisplayURL(test_util()->model()->search_terms_data()));
Nit: Or this

https://codereview.chromium.org/2902043004/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_fetcher_unittest.cc:208:
EXPECT_TRUE(t_url->safe_for_autoreplace());
Nit: Or this

https://codereview.chromium.org/2902043004/diff/1/chrome/test/data/simple_ope...
File chrome/test/data/simple_open_search_no_name.xml (right):

https://codereview.chromium.org/2902043004/diff/1/chrome/test/data/simple_ope...
chrome/test/data/simple_open_search_no_name.xml:1: <?xml version="1.0"?>
How come we need both this file and the one in components/?  Is one of them not
actually used?

[Mark P, 2017-05-26 21:56:26]

https://codereview.chromium.org/2902043004/diff/1/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_fetcher_unittest.cc (right):

https://codereview.chromium.org/2902043004/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_fetcher_unittest.cc:192:
ASSERT_FALSE(test_util()->model()->GetTemplateURLForKeyword(keyword));
On 2017/05/26 01:38:39, Peter Kasting wrote:
> Nit: I'd remove all the checks here that just duplicate the test above and
make
> this test simply about the short name fallback.
> 
> So, doesn't seem like we really care about this

Removed unnecessary checks.  Kept this one, because otherwise it's possible this
test could pass if there's a template with this keyword remaining around.  This
is a useful precondition to check.

https://codereview.chromium.org/2902043004/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_fetcher_unittest.cc:196: EXPECT_EQ(0,
requests_completed());
On 2017/05/26 01:38:39, Peter Kasting wrote:
> Nit: Or this

Done.

https://codereview.chromium.org/2902043004/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_fetcher_unittest.cc:199: EXPECT_EQ(1,
requests_completed());
On 2017/05/26 01:38:39, Peter Kasting wrote:
> Nit: Or this

Done.

https://codereview.chromium.org/2902043004/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_fetcher_unittest.cc:206:
t_url->url_ref().DisplayURL(test_util()->model()->search_terms_data()));
On 2017/05/26 01:38:39, Peter Kasting wrote:
> Nit: Or this

Done.

https://codereview.chromium.org/2902043004/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_fetcher_unittest.cc:208:
EXPECT_TRUE(t_url->safe_for_autoreplace());
On 2017/05/26 01:38:39, Peter Kasting wrote:
> Nit: Or this

Done.

https://codereview.chromium.org/2902043004/diff/1/chrome/test/data/simple_ope...
File chrome/test/data/simple_open_search_no_name.xml (right):

https://codereview.chromium.org/2902043004/diff/1/chrome/test/data/simple_ope...
chrome/test/data/simple_open_search_no_name.xml:1: <?xml version="1.0"?>
On 2017/05/26 01:38:39, Peter Kasting wrote:
> How come we need both this file and the one in components/?  Is one of them
not
> actually used?

I believe we only need the components one, but the simple_open_search.xml exists
on both places and I think it makes most sense to keep this parallel.  This
looks like it's being fixed as part of bug 527882, which is out of scope of this
changelist.

[Mark P, 2017-05-31 23:35:27]

The CQ bit was checked by mpearson@chromium.org

[Mark P, 2017-05-31 23:35:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2902043004/#ps20001
(title: "pkasting comments")

[commit-bot: I haz the power, 2017-05-31 23:36:18]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-01 01:14:26]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1496273727323270,
"parent_rev": "6995b86d4c47b0de61cfc3d7417b91488e8a6b02", "commit_rev":
"ee5a39ceca0897f43f63059d44635da6f02d0a50"}

[commit-bot: I haz the power, 2017-06-01 01:14:37]

Description was changed from

==========
Omnibox - Open Search - Handle Lack of Short Name Smartly

BUG=714081
==========

to

==========
Omnibox - Open Search - Handle Lack of Short Name Smartly

BUG=714081

Review-Url: https://codereview.chromium.org/2902043004
Cr-Commit-Position: refs/heads/master@{#476125}
Committed:
https://chromium.googlesource.com/chromium/src/+/ee5a39ceca0897f43f63059d4463...
==========

[commit-bot: I haz the power, 2017-06-01 01:14:38]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/ee5a39ceca0897f43f63059d4463...