New thin layer of API extension chrome.i18n.detectLanguage

chrome/browser/extensions/api/i18n/i18n_api.cc

 // Copyright (c) 2012 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/i18n/i18n_api.h"
 
 #include <algorithm>
-#include <string>
 #include <vector>
 
-#include "base/lazy_instance.h"
 #include "base/prefs/pref_service.h"
 #include "base/strings/string_piece.h"
 #include "base/strings/string_split.h"
 #include "chrome/browser/profiles/profile.h"
-#include "chrome/common/extensions/api/i18n.h"
 #include "chrome/common/pref_names.h"
 
-namespace GetAcceptLanguages = extensions::api::i18n::GetAcceptLanguages;
-
 namespace extensions {
 
+namespace GetAcceptLanguages = extensions::api::i18n::GetAcceptLanguages;
+// Typedef only introduced to avoid unreadable code.

[not at google - send to devlin, 2015/07/14 18:42:57]

comment not necessary.

[amalika, 2015/07/14 22:41:02]

Done.

+typedef struct extensions::api::i18n::DetectLanguage::Results::Result::
+    LanguagesType DetectedLanguage;
+typedef struct extensions::api::i18n::DetectLanguage::Results::Result
+    LanguageDetectionResult;

[not at google - send to devlin, 2015/07/14 18:42:58]

c++11 has "using LanguageDetectionResult = ..." not typedef.

[amalika, 2015/07/14 22:41:02]

Done.

+
 namespace {
-
+const int kCldNumLangs = 3;

[not at google - send to devlin, 2015/07/14 18:42:58]

comment; maintain existing blank line spacing

[amalika, 2015/07/14 22:41:02]

Done.

 // Errors.
 static const char kEmptyAcceptLanguagesError[] = "accept-languages is empty.";
 
 }
 
 bool I18nGetAcceptLanguagesFunction::RunSync() {
   std::string accept_languages =
       GetProfile()->GetPrefs()->GetString(prefs::kAcceptLanguages);
   // Currently, there are 2 ways to set browser's accept-languages: through UI
   // or directly modify the preference file. The accept-languages set through
@@ skipping 17 matching lines @@
 
   if (languages.empty()) {
     error_ = kEmptyAcceptLanguagesError;
     return false;
   }
 
   results_ = GetAcceptLanguages::Results::Create(languages);
   return true;
 }
 
+ExtensionFunction::ResponseAction I18nDetectLanguageFunction::Run() {
+  scoped_ptr<api::i18n::DetectLanguage::Params> params(
+      api::i18n::DetectLanguage::Params::Create(*args_));
+
+  EXTENSION_FUNCTION_VALIDATE(params.get());
+
+  GetLanguage(params->text);
+
+  return RespondLater();

[not at google - send to devlin, 2015/07/14 18:42:57]

This should all be:

scoped_ptr<..> params(...);
EXTENSION_FUNCTION_VALIDATE(params);
return RespondNow(ArgumentList(GetLanguage(params->text)));

and GetLanguage should return a scoped_ptr<Value> of the result.

no need for SendLanguagesResult, calling SetResult/SendResponse separtately.

[amalika, 2015/07/14 22:41:02]

Done.

+}
+
+void I18nDetectLanguageFunction::GetLanguage(const std::string& text) {
+  // Sets up the input String
+  const std::string utf8_text(text);

[not at google - send to devlin, 2015/07/14 18:42:57]

const on local variable values (i.e. those that aren't pointers) are pointless,
and IMO they detract from readability.

[mcindy, 2015/07/14 22:41:21]

Done.

+  const int num_utf8_bytes = static_cast<int>(utf8_text.size());
+  const char* raw_utf8_bytes = utf8_text.c_str();

[not at google - send to devlin, 2015/07/14 18:42:57]

writing these to local variables before using them detracts from readability.
just inline the casting as part of the CLD2:: call.

[mcindy, 2015/07/14 21:38:27]

We're currently modifying the CLD2:: call to accomodate with the bug toyoshi
mentioned and we will be using these local variables twice. Is it still okay to
put the casting inline as part of the call? Because I'm worried it will make it
look a bit redundant and unclear. 

[not at google - send to devlin, 2015/07/14 21:48:33]

I'd make it a cast for now and change it if/when it needs to later.

[mcindy, 2015/07/14 22:41:21]

Done.

+
+  // TODO(mcindy): improve this by providing better CLD hints
+  const char tld_hint[] = "";  // asummed no cld hints provided
+  int encoding_hint = CLD2::UNKNOWN_ENCODING;
+  CLD2::Language language_hint = CLD2::UNKNOWN_LANGUAGE;
+  CLD2::CLDHints cldhints = {NULL, tld_hint, encoding_hint, language_hint};

[not at google - send to devlin, 2015/07/14 18:42:58]

NULL -> nullptr - and I think there is overuse of local variables here as well.
makes things look more complicated than they really are.

[mcindy, 2015/07/14 22:41:21]

Done.

+
+  const bool is_plain_text = true;
+  CLD2::Language languages[kCldNumLangs];
+  int percents[kCldNumLangs];
+  int text_bytes;  // amount of non-tag/letters-only text (assumed 0)
+  bool is_reliable = false;
+  int flags = 0;
+  int valid_prefix_bytes = false;
+  double normalized_score[kCldNumLangs];
+
+  // populating languages and percents
+  CLD2::ExtDetectLanguageSummaryCheckUTF8(

[Takashi Toyoshima, 2015/07/14 07:28:16]

Should we refer crbug.com/444258 here so that we can update this code when we
solve the problem?

Or if we can share more code with
components/translate/core/language_detection/language_detection_util.cc, that's
great.

[mcindy, 2015/07/14 22:41:21]

Done.

+      raw_utf8_bytes, num_utf8_bytes, is_plain_text, &cldhints, flags,
+      languages, percents, normalized_score,
+      NULL,  // assumed no ResultChunkVector is used
+      &text_bytes, &is_reliable, &valid_prefix_bytes);
+
+  LanguageDetectionResult result;
+  result.is_reliable = is_reliable;
+  InitDetectedLanguages(languages, percents, &result.languages);
+
+  SendLanguagesResult(result);
+}
+
+void I18nDetectLanguageFunction::SendLanguagesResult(
+    const LanguageDetectionResult& result) {
+  SetResult(result.ToValue());
+  SendResponse(true);
+}
+
+void I18nDetectLanguageFunction::InitDetectedLanguages(
+    CLD2::Language* languages,
+    int* percents,
+    std::vector<linked_ptr<DetectedLanguage>>* detected_languages) {
+  for (int i = 0; i < kCldNumLangs; i++) {
+    std::string language_code = "";
+
+    // Convert LanguageCode 'zh' to 'zh-CN' and 'zh-Hant' to 'zh-TW' for
+    // Translate server usage. see DetermineTextLanguage in
+    // components/translate/core/language_detection/language_detection_util.cc
+    if (languages[i] == CLD2::UNKNOWN_LANGUAGE) {
+      // no need to save in detected_languages
+      break;
+    } else if (languages[i] == CLD2::CHINESE) {
+      language_code = "zh-CN";
+    } else if (languages[i] == CLD2::CHINESE_T) {
+      language_code = "zh-TW";
+    } else {
+      language_code =
+          CLD2::LanguageCode(static_cast<CLD2::Language>(languages[i]));
+    }
+    linked_ptr<DetectedLanguage> detected_lang(new DetectedLanguage);
+    detected_lang->language = language_code;
+    detected_lang->percentage = percents[i];
+    detected_languages->push_back(detected_lang);
+  }
+}
+
 }  // namespace extensions