Unpunycode search keywords and short names.

chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/extensions/api/search_engines_private/search_engines_private_api.h"
 
 #include "base/prefs/pref_service.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
 #include "chrome/browser/extensions/chrome_extension_function.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/search/hotword_audio_history_handler.h"
 #include "chrome/browser/search/hotword_service.h"
 #include "chrome/browser/search/hotword_service_factory.h"
 #include "chrome/browser/search_engines/template_url_service_factory.h"
 #include "chrome/browser/signin/signin_manager_factory.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/search_engines/template_url_service.h"
 #include "components/signin/core/browser/signin_manager.h"
 #include "components/signin/core/browser/signin_manager_base.h"
 #include "extensions/browser/extension_function_registry.h"
+#include "net/base/net_util.h"
 #include "ui/base/l10n/l10n_util.h"
 
 namespace extensions {
 
 namespace search_engines_private = api::search_engines_private;
 
 const char kHotwordServiceMissing[] = "Cannot retrieve hotword service";
 const char kRetrainWithoutAlwaysOnEnabledError[] =
     "Cannot retrain without always search and audio logging enabled";
 const char kOptInWithAudioLoggingError[] =
@@ skipping 79 matching lines @@
     ~SearchEnginesPrivateAddOtherSearchEngineFunction() {
 }
 
 ExtensionFunction::ResponseAction
 SearchEnginesPrivateAddOtherSearchEngineFunction::Run() {
   scoped_ptr<search_engines_private::AddOtherSearchEngine::Params> parameters =
       search_engines_private::AddOtherSearchEngine::Params::Create(*args_);
   EXTENSION_FUNCTION_VALIDATE(parameters.get());
 
   TemplateURLData data;
-  data.SetShortName(base::UTF8ToUTF16(parameters->name));
-  data.SetKeyword(base::UTF8ToUTF16(parameters->keyword));
+  // Unpunycode short name and keyword.

[Peter Kasting, 2015/07/14 18:05:33]

Why do we need to do this for these particular cases, but we don't go so far as
to, say, run IDNToUnicode() unconditionally inside SetShortName() and
SetKeyword()?

[alshabalin, 2015/07/15 14:18:01]

Because I want to perform IDN conversion on user input only. For JS API here and
for site-supplied OpenSearch data (in TemplateURLParser) I defensively run this
conversion to catch possible mistakes from the site maintainer: e.g. when site
returns OpenSearch xml with ShortName in punycode (probably auto-generated from
the hostname), I want the end user to see it in the decoded form.

[Peter Kasting, 2015/07/15 21:19:12]

I don't understand your paragraph.  It seems to first say you want to only
un-punycode user input, then shortly after that you want to un-punycode
non-user-input (e.g. stuff in the OSDD).

[alshabalin, 2015/07/16 15:13:07]

I was thinking server-side user at the time and actually meant input from OSDD.
It is confusing, sorry. Anyway, my point is to leave the decision whether to
un-punycode or not to the caller because un-punycoding ShortName is not
a normal behaviour but a workaround to fix accidental mistakes for the end-user
benefit. I can't come up with an example when doing the conversion would hurt
though.

[Peter Kasting, 2015/07/16 19:16:27]

Well, it's not normal in the sense that we don't currently do it, but I'm not
convinced it's ever wrong.  I'm inclined to suggest centralizing the conversion
so it just always happens.  However, that might make getting the correct
languages tricky, so it may be that the current solution is easier to implement.

[Matt Giuca, 2015/07/17 04:13:11]

It sounds like you're saying that because site maintainers might make the
mistake of putting Punycode in the XML ShortName field, we should pro-actively
decode it for them...?

That sounds like a Bad Thing. The OpenSearch spec [1] does not say that this
should be Punycode-encoded, or that it is a domain name. It simply says that
it's "a brief human-readable title".

You could make a similar argument for decoding percent-escaped text in this
field, or performing any other type of unescaping, "just in case" some badly
formatted data was accidentally encoded. But that's not how user agents should
deal with data.

(Conversely: do you have any evidence of high-profile sites that have a
Punycoded domain in their ShortName field?)

[1]
http://www.opensearch.org/Specifications/OpenSearch/1.1#The_.22ShortName.22_e...

+  data.SetShortName(net::IDNToUnicode(parameters->name, ""));

[Peter Kasting, 2015/07/14 18:05:33]

Nit: Use std::string() in place of "" everywhere in this change.

[alshabalin, 2015/07/15 15:19:50]

Done.

+  data.SetKeyword(net::IDNToUnicode(parameters->keyword, ""));
   data.SetURL(parameters->url);
   TemplateURL* turl = new TemplateURL(data);
 
   TemplateURLService* template_url_service =
       TemplateURLServiceFactory::GetForProfile(chrome_details_.GetProfile());
   template_url_service->Add(turl);
   return RespondNow(NoArguments());
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ skipping 194 matching lines @@
   } else {
     if (prefs->GetBoolean(prefs::kHotwordAlwaysOnSearchEnabled))
       return RespondNow(Error(kAlreadyOptedInError));
   }
 
   hotword_service->OptIntoHotwording(launch_mode);
   return RespondNow(NoArguments());
 }
 
 }  // namespace extensions