Unpunycode search keywords and short names.

components/search_engines/template_url_parser.cc

 // Copyright 2014 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 "components/search_engines/template_url_parser.h"
 
 #include <algorithm>
 #include <map>
 #include <vector>
 
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "components/search_engines/template_url.h"
 #include "libxml/parser.h"
 #include "libxml/xmlwriter.h"
+#include "net/base/net_util.h"
 #include "ui/gfx/favicon_size.h"
 #include "url/gurl.h"
 #include "url/url_constants.h"
 
 namespace {
 
 // NOTE: libxml uses the UTF-8 encoding. As 0-127 of UTF-8 corresponds
 // to that of char, the following names are all in terms of char. This avoids
 // having to convert to wide, then do comparisons.
 
@@ skipping 215 matching lines @@
 
 // static
 void TemplateURLParsingContext::EndElementImpl(void* ctx, const xmlChar* name) {
   TemplateURLParsingContext* context =
       reinterpret_cast<TemplateURLParsingContext*>(ctx);
   switch (context->GetKnownType()) {
     case TemplateURLParsingContext::URL:
       context->ProcessURLParams();
       break;
     case TemplateURLParsingContext::SHORT_NAME:
-      context->data_.SetShortName(context->string_);
+      // If someone gives us ShortName in punycode, decode it.
+      context->data_.SetShortName(
+          base::IsStringASCII(context->string_)

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

Why do an ASCII check here?  Why not just unconditionally go the IDNToUnicode()
route as you have elsewhere?

If there's an efficiency issue, perhaps IDNToUnicode() should do an ASCII check
and early-out itself?  Or maybe we need to insert these sorts of conditionals in
other places too?

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

IDNToUnicode requires ASCII because it calls ASCIIToUTF16 on its argument. Here
it's the data from OpenSearch xml which could contain unicode. In other places
it is guaranteed to be in ASCII: in JS API it previously called ASCIIToUTF16, in
TemplateURL::GenerateKeyword it uses hostname.

But it might indeed make sense to move the ASCII check to IDNToUnicode.

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

At the least you need a comment in all these places noting why you do or do not
need an ASCII check.

+              ? net::IDNToUnicode(base::UTF16ToASCII(context->string_), "")
+              : context->string_);
       break;
     case TemplateURLParsingContext::IMAGE: {
       GURL image_url(base::UTF16ToUTF8(context->string_));
       if (image_url.SchemeIs(url::kDataScheme)) {
         // TODO (jcampan): bug 1169256: when dealing with data URL, we need to
         // decode the data URL in the renderer. For now, we'll just point to the
         // favicon from the URL.
         context->derive_image_from_url_ = true;
       } else if (context->image_is_valid_for_favicon_ && image_url.is_valid() &&
                  (image_url.SchemeIs(url::kHttpScheme) ||
@@ skipping 238 matching lines @@
   sax_handler.startElement = &TemplateURLParsingContext::StartElementImpl;
   sax_handler.endElement = &TemplateURLParsingContext::EndElementImpl;
   sax_handler.characters = &TemplateURLParsingContext::CharactersImpl;
   int error = xmlSAXUserParseMemory(&sax_handler, &context, data,
                                     static_cast<int>(length));
   xmlSubstituteEntitiesDefault(last_sub_entities_value);
 
   return error ?
       NULL : context.GetTemplateURL(search_terms_data, show_in_default_list);
 }