Added validation to adding new search engines

Added validation to adding new search engines - 3rd iteration :)
BUG=21882
TEST=none

[tfarina (gmail-do not use), 2009-11-06 12:40:26]

http://codereview.chromium.org/361032/diff/2001/3002
File chrome/browser/search_engines/edit_search_engine_controller.cc (right):

http://codereview.chromium.org/361032/diff/2001/3002#newcode59
Line 59: return true;  // Always allow no keyword.
The comment of this function says: "The keyword is valid if it is non-empty".

So returning true here is a contradiction, no?

[GeorgeY, 2009-11-06 17:35:47]

http://codereview.chromium.org/361032/diff/2001/3002
File chrome/browser/search_engines/edit_search_engine_controller.cc (right):

http://codereview.chromium.org/361032/diff/2001/3002#newcode59
Line 59: return true;  // Always allow no keyword.
On 2009/11/06 12:40:27, tfarina wrote:
> The comment of this function says: "The keyword is valid if it is non-empty".
> 
> So returning true here is a contradiction, no?
If I understood correctly Keyword is optional. But if it present it *must* be
unique.

So, for example, "\t \t" is OK, as it is empty. and " Google" <=> "Google"

[georgey1, 2009-11-06 17:46:38]

BTW, this is how it is implemented in original code. I could either remove
validity of the empty entry or modify the comment in header file - in any
case change is less than two lines :)

George.

On Fri, Nov 6, 2009 at 9:35 AM, <georgey@chromium.org> wrote:

>
> http://codereview.chromium.org/361032/diff/2001/3002
> File chrome/browser/search_engines/edit_search_engine_controller.cc
> (right):
>
> http://codereview.chromium.org/361032/diff/2001/3002#newcode59
> Line 59: return true;  // Always allow no keyword.
> On 2009/11/06 12:40:27, tfarina wrote:
>
>> The comment of this function says: "The keyword is valid if it is
>>
> non-empty".
>
>  So returning true here is a contradiction, no?
>>
> If I understood correctly Keyword is optional. But if it present it
> *must* be unique.
>
> So, for example, "\t \t" is OK, as it is empty. and " Google" <=>
> "Google"
>
>
> http://codereview.chromium.org/361032
>

[jcampan, 2009-11-06 18:10:47]

http://codereview.chromium.org/361032/diff/2001/3002
File chrome/browser/search_engines/edit_search_engine_controller.cc (right):

http://codereview.chromium.org/361032/diff/2001/3002#newcode42
Line 42: if (url_input.find(L"%s") == std::wstring::npos)
Just talked with Scott and realize we should not enforce that. Sorry for giving
you the wrong info :-(
It seems a search engine can be created simply as a shortcut to a search engine,
so %s should not be mandatory.

[georgey1, 2009-11-06 18:13:16]

OK, reverting this, np

On Fri, Nov 6, 2009 at 10:10 AM, <jcampan@chromium.org> wrote:

>
> http://codereview.chromium.org/361032/diff/2001/3002
> File chrome/browser/search_engines/edit_search_engine_controller.cc
> (right):
>
> http://codereview.chromium.org/361032/diff/2001/3002#newcode42
> Line 42: if (url_input.find(L"%s") == std::wstring::npos)
> Just talked with Scott and realize we should not enforce that. Sorry for
> giving you the wrong info :-(
> It seems a search engine can be created simply as a shortcut to a search
> engine, so %s should not be mandatory.
>
>
> http://codereview.chromium.org/361032
>

[tfarina (gmail-do not use), 2009-11-06 19:26:15]

http://codereview.chromium.org/361032/diff/6002/7001
File chrome/browser/search_engines/edit_search_engine_controller.cc (right):

http://codereview.chromium.org/361032/diff/6002/7001#newcode55
Line 55: return false;  // Do not allow empty keyword.
Nit: I prefer to put this comment in the function prototype, but it is fine
here.

[jcampan, 2009-11-06 20:07:48]

LGTM

[tfarina (gmail-do not use), 2009-11-06 20:14:29]

The mac failures seems real.

Maybe you need to update some tests in
edit_search_engine_cocoa_controller_unittest.mm to fit in this new behavior.

[GeorgeY, 2009-11-06 20:44:23]

On 2009/11/06 20:14:29, tfarina wrote:
> The mac failures seems real.
> 
> Maybe you need to update some tests in
> edit_search_engine_cocoa_controller_unittest.mm to fit in this new behavior.

Reverted that file too much :)
Re-running tests again - will update when they pass

[GeorgeY, 2009-11-06 22:07:29]

All tests on mac are working, cl re-uploaded

[tfarina (gmail-do not use), 2009-11-06 22:14:35]

LG