Add spellcheck and word suggestion to the prediction service

services/prediction/dictionary_service.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 <algorithm>
+#include <deque>
+#include <fstream>
+#include <string>
+
+#include "base/base_paths.h"
+#include "base/files/file_path.h"
+#include "base/path_service.h"
+#include "mojo/services/prediction/public/interfaces/prediction.mojom.h"
+#include "mojo/tools/embed/data.h"
+#include "services/prediction/dictionary_service.h"
+#include "services/prediction/input_info.h"
+#include "services/prediction/kDictFile.h"
+#include "services/prediction/key_set.h"
+#include "third_party/prediction/suggest/core/dictionary/dictionary.h"
+#include "third_party/prediction/suggest/core/result/suggestion_results.h"
+#include "third_party/prediction/suggest/core/session/dic_traverse_session.h"
+#include "third_party/prediction/suggest/core/session/prev_words_info.h"
+#include "third_party/prediction/suggest/core/suggest_options.h"
+#include "third_party/prediction/suggest/policyimpl/dictionary/structure/dictionary_structure_with_buffer_policy_factory.h"
+
+namespace prediction {
+
+DictionaryService::DictionaryService() {
+  max_suggestion_size = 50;
+  default_dictionary = NULL;

[APW, 2015/07/23 18:02:15]

use nullptr everywhere instead of NULL.

[riajiang, 2015/07/31 02:13:05]

Done.

+  default_session = NULL;
+}
+
+DictionaryService::~DictionaryService() {
+  delete default_dictionary;

[qsr, 2015/07/23 19:40:42]

You want to use a std::unique_ptr instead of having to delete the object
yourself.

Mainly, you should only use raw pointer if they are weak. Every time you have
ownership, or pass ownership, you should use a std::unique_ptr.

[riajiang, 2015/07/31 02:13:04]

I changed it to use base/memory/scoped_ptr, is it the same in this case?

+}
+
+void DictionaryService::LoadDictionary() {

[APW, 2015/07/23 19:51:38]

This function name is misleading... I think you're Creating the dictionary or
maybe Copying it to File  or something like that.

Actually maybe EnsureDictionaryExists would be a good one and then make this
check to see if the file already exists - if not, create it.

[riajiang, 2015/07/31 02:13:05]

How's CopyDictionaryFromRes? And added checking if file exists 

+  base::FilePath dir_temp;

[APW, 2015/07/23 19:51:39]

Since the path for writing the dictionary and reading the dictionary *must* be
the same, I think you should create a function for creating that path and use it
in both places (to avoid future issues)

[riajiang, 2015/07/31 02:13:04]

I changed to only creating the path once in GetDictionarySuggestion and pass it
to both places 

+  PathService::Get(base::DIR_TEMP, &dir_temp);
+  std::string path = dir_temp.value() + "/main_en.dict";
+  std::ofstream dic_file(path.c_str(),
+                         std::ofstream::out | std::ofstream::binary);
+  dic_file.write(prediction::kDictFile.data, prediction::kDictFile.size);
+  dic_file.close();
+}
+
+latinime::Dictionary* const DictionaryService::OpenDictionary(
+    const std::string path,
+    const int start_offset,
+    const int size,
+    const bool is_updatable) {
+  latinime::DictionaryStructureWithBufferPolicy::StructurePolicyPtr
+      dictionary_structure_with_buffer_policy(
+          latinime::DictionaryStructureWithBufferPolicyFactory::
+              newPolicyForExistingDictFile(path.c_str(), start_offset, size,

[APW, 2015/07/23 19:51:38]

Function names must start upper case unless its a simple accessor:

NewPolicyForExistingDictFile

[riajiang, 2015/07/31 02:13:05]

This is a function in Android's native code. Since all functions there are named
like this should I change all of them/ only change this specific one/ use it as
it is?  

+                                           is_updatable));
+  if (!dictionary_structure_with_buffer_policy) {
+    return 0;
+  }
+
+  latinime::Dictionary* const dictionary = new latinime::Dictionary(
+      std::move(dictionary_structure_with_buffer_policy));
+  return dictionary;
+}
+
+mojo::Array<mojo::String> DictionaryService::GetDictionarySuggestion(
+    PredictionInfoPtr prediction_info,
+    ProximityInfoService* proximity_info) {
+  mojo::Array<mojo::String> suggestion_words =
+      mojo::Array<mojo::String>::New(0);
+
+  // dictionary
+  base::FilePath dir_temp;
+  PathService::Get(base::DIR_TEMP, &dir_temp);
+  std::string path = dir_temp.value() + "/main_en.dict";
+  if (!default_dictionary) {
+    LoadDictionary();
+    default_dictionary =

[APW, 2015/07/23 19:51:39]

default_dictionary should probably be a scoped_ptr so you don't need to worry
about deleting it or who owns it.

[riajiang, 2015/07/31 02:13:04]

Done.

+        this->OpenDictionary(path, 0, prediction::kDictFile.size, false);
+    if (!default_dictionary) {
+      suggestion_words.push_back(prediction_info->current_word);
+      return suggestion_words.Clone().Pass();
+    }
+  }
+
+  // dic_traverse_session
+  if (!default_session) {
+    default_session = reinterpret_cast<latinime::DicTraverseSession*>(

[APW, 2015/07/23 19:51:39]

should this be scoped_ptr too?

[riajiang, 2015/07/31 02:13:04]

Done.

+        latinime::DicTraverseSession::getSessionInstance(
+            "en", prediction::kDictFile.size));
+    latinime::PrevWordsInfo empty_prev_words;
+    default_session->init(default_dictionary, &empty_prev_words, 0);
+  }
+
+  // current word
+  int input_size = prediction_info->current_word.size();
+  InputInfo input_info(input_size);
+  input_info.ProcessInput(prediction_info->current_word);
+
+  // previous words
+  latinime::PrevWordsInfo* prev_words_info =

[APW, 2015/07/23 19:51:38]

since you allocate this here and delete it right after using it below - can e
make this not be allocated on heap?

[riajiang, 2015/07/31 02:13:05]

Done.

+      ProcessPrevWord(prediction_info->previous_words,
+                      prediction_info->are_beginning_of_sentence);
+
+  // suggestion options
+  // is_gesture, use_full_edit_distance,
+  // block_offensive_words, space_aware gesture_enabled
+  int options[4] = {0, 0, 0, 0};
+  latinime::SuggestOptions suggest_options(options, 4);

[APW, 2015/07/23 19:51:38]

there's a macro for determining the array size of an array.  Use that instead of
magic numbers here and above line.

[riajiang, 2015/07/31 02:13:05]

Done.

+
+  latinime::SuggestionResults suggestion_results(max_suggestion_size);
+  if (input_size > 0) {
+    DictionaryService::default_dictionary->getSuggestions(
+        proximity_info->GetNativeProximityInfoService(),
+        DictionaryService::default_session, input_info.GetXCoordinates(),
+        input_info.GetYCoordinates(), input_info.GetTimes(),
+        input_info.GetPointerIds(), input_info.GetCodepoints(), input_size,
+        prev_words_info, &suggest_options, -1.0f, &suggestion_results);
+  } else {
+    DictionaryService::default_dictionary->getPredictions(prev_words_info,
+                                                          &suggestion_results);
+  }
+  delete prev_words_info;
+
+  // process suggestion results
+  std::deque<std::string> suggestion_words_reverse;
+  char cur_beginning = prediction_info->current_word[0];

[APW, 2015/07/23 19:51:39]

this assumes current_word is size > 0.  You didn't assume that above.  This will
crash in that case.

[riajiang, 2015/07/31 02:13:05]

Done.

+  std::string cur_rest =
+      std::string(prediction_info->current_word.data())
+          .substr(1, prediction_info->current_word.size() - 1);
+  std::string lo_cur = std::string(1, (char)tolower(cur_beginning)) + cur_rest;
+  std::string up_cur = std::string(1, (char)toupper(cur_beginning)) + cur_rest;
+  while (!suggestion_results.mSuggestedWords.empty()) {

[APW, 2015/07/23 19:51:38]

Please use an iterator instead of popping and checking empty

[riajiang, 2015/07/31 02:13:05]

But mSuggestedWords is a std::priority_queue?

[APW, 2015/07/31 21:49:15]

shoudl still be iterable I think

+    const latinime::SuggestedWord& suggested_word =
+        suggestion_results.mSuggestedWords.top();
+    int word_count =
+        std::min(suggested_word.getCodePointCount(), MAX_WORD_LENGTH);
+    char word[word_count + 1];
+    for (int i = 0; i < word_count; i++) {
+      word[i] = (char)suggested_word.getCodePoint()[i];

[APW, 2015/07/23 19:51:38]

While this conversion from code point (int) to char works for ascii, this won't
work for many other languages that have characters expressed via multiple bytes.

I think we'll need to do the conversion another way for other languages to work.

[APW, 2015/07/23 19:51:39]

use static_cast<char>(var)

[riajiang, 2015/07/31 02:13:04]

Changed to use char16 as we discussed.

[riajiang, 2015/07/31 02:13:04]

Changed to use char16 as we discussed.

+    }
+    word[word_count] = '\0';
+    std::string word_string(word);
+    // remove dups
+    if (word_string != lo_cur && word_string != up_cur) {

[APW, 2015/07/23 19:51:39]

I don't think this does what you think.  You'll need to call:
http://www.cplusplus.com/reference/string/string/compare/

Also, you're only comparing the current suggestion with the original word.  It's
not really removing duplicates - just any suggestions that are the same as the
original word.

[riajiang, 2015/07/31 02:13:05]

Changed to use compare and added removing dups within suggestion words

+      if (isupper(cur_beginning))
+        word_string[0] = toupper(word_string[0]);
+      suggestion_words_reverse.push_front(word_string);
+    }
+    suggestion_results.mSuggestedWords.pop();
+  }
+  for (std::deque<std::string>::iterator it = suggestion_words_reverse.begin();

[APW, 2015/07/23 19:51:39]

Why do the reverse thing?

You can push_back instead of push_front in the loop?

[riajiang, 2015/07/31 02:13:05]

mSuggestedWords is a priority queue but I don't know why it's ordered with the
most possible suggestion at the very end. That's why I was using push_front.

+       it != suggestion_words_reverse.end(); ++it) {
+    suggestion_words.push_back(mojo::String(*it));
+  }
+
+  if (suggestion_words.size() == 0)
+    suggestion_words.push_back(prediction_info->current_word);

[APW, 2015/07/23 19:51:38]

Why return current word as the suggestion if empty?

[riajiang, 2015/07/31 02:13:04]

I was thinking if there's no suggestion based on user's typed word they would
still want to see their word show up as suggestion, but I guess it doesn't
really make sense if the user typed a long ugly word. I deleted this so now the
suggestion would be empty.

+
+  return suggestion_words.Clone().Pass();
+}
+
+// modified from Android JniDataUtils::constructPrevWordsInfo
+latinime::PrevWordsInfo* DictionaryService::ProcessPrevWord(
+    mojo::Array<mojo::String>& prev_words,
+    mojo::Array<bool>& beginnings) {
+  int prev_word_codepoints[MAX_PREV_WORD_COUNT_FOR_N_GRAM][MAX_WORD_LENGTH];
+  int prev_word_codepoint_count[MAX_PREV_WORD_COUNT_FOR_N_GRAM];
+  bool is_beginning_of_sentence[MAX_PREV_WORD_COUNT_FOR_N_GRAM];
+  int prevwords_count =
+      std::min((int)prev_words.size(), MAX_PREV_WORD_COUNT_FOR_N_GRAM);

[APW, 2015/07/23 19:51:38]

instead of using int here can you use size_t?

[riajiang, 2015/07/31 02:13:04]

I used (size_t) for MAX_PREV_WORD_COUNT_FOR_N_GRAM, is this what you mean?

+  for (int i = 0; i < prevwords_count; ++i) {
+    prev_word_codepoint_count[i] = 0;
+    is_beginning_of_sentence[i] = false;
+    int prev_word_size = prev_words[i].size();
+    if (prev_word_size > MAX_WORD_LENGTH) {
+      continue;
+    }
+    for (int j = 0; j < prev_word_size; j++) {
+      prev_word_codepoints[i][j] = (int)prev_words[i][j];
+    }
+    prev_word_codepoint_count[i] = prev_word_size;
+    is_beginning_of_sentence[i] = beginnings[i];
+  }
+  latinime::PrevWordsInfo* prev_words_info = new latinime::PrevWordsInfo(
+      prev_word_codepoints, prev_word_codepoint_count, is_beginning_of_sentence,
+      prevwords_count);
+  return prev_words_info;
+}
+
+}  // namespace prediction