Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Spellchecker Async Implementation #14032

Merged
merged 8 commits into from Oct 18, 2018
@@ -4,7 +4,7 @@

#include "atom/renderer/api/atom_api_spell_check_client.h"

#include <algorithm>
#include <map>
#include <vector>

#include "atom/common/native_mate_converters/string16_converter.h"
@@ -13,6 +13,7 @@
#include "chrome/renderer/spellchecker/spellcheck_worditerator.h"
#include "native_mate/converter.h"
#include "native_mate/dictionary.h"
#include "native_mate/function_template.h"
#include "third_party/blink/public/web/web_text_checking_completion.h"
#include "third_party/blink/public/web/web_text_checking_result.h"
#include "third_party/icu/source/common/unicode/uscript.h"
@@ -40,30 +41,35 @@ bool HasWordCharacters(const base::string16& text, int index) {

class SpellCheckClient::SpellcheckRequest {
public:
// Map of individual words to list of occurrences in text
using WordMap =
std::map<base::string16, std::vector<blink::WebTextCheckingResult>>;

SpellcheckRequest(const base::string16& text,
blink::WebTextCheckingCompletion* completion)
: text_(text), completion_(completion) {
DCHECK(completion);
}
~SpellcheckRequest() {}

base::string16 text() { return text_; }
const base::string16& text() const { return text_; }
blink::WebTextCheckingCompletion* completion() { return completion_; }
WordMap& wordmap() { return word_map_; }

private:
base::string16 text_; // Text to be checked in this task.

WordMap word_map_; // WordMap to hold distinct words in text
// The interface to send the misspelled ranges to WebKit.
blink::WebTextCheckingCompletion* completion_;

DISALLOW_COPY_AND_ASSIGN(SpellcheckRequest);
};

SpellCheckClient::SpellCheckClient(const std::string& language,
bool auto_spell_correct_turned_on,
v8::Isolate* isolate,
v8::Local<v8::Object> provider)
: isolate_(isolate),
: pending_request_param_(nullptr),
isolate_(isolate),
context_(isolate, isolate->GetCurrentContext()),
provider_(isolate, provider) {
DCHECK(!context_.IsEmpty());
@@ -79,19 +85,6 @@ SpellCheckClient::~SpellCheckClient() {
context_.Reset();
}

void SpellCheckClient::CheckSpelling(
const blink::WebString& text,

This comment has been minimized.

Copy link
@nitsakh

nitsakh Aug 12, 2018

Author Contributor

This is only called by blink when creating the context menu. Clients use electron APIs to create context menus, so we don't need to override this function.

int& misspelling_start,
int& misspelling_len,
blink::WebVector<blink::WebString>* optional_suggestions) {
std::vector<blink::WebTextCheckingResult> results;
SpellCheckText(text.Utf16(), true, &results);
if (results.size() == 1) {
misspelling_start = results[0].location;
misspelling_len = results[0].length;
}
}

void SpellCheckClient::RequestCheckingOfText(
const blink::WebString& textToCheck,
blink::WebTextCheckingCompletion* completionCallback) {
@@ -103,16 +96,15 @@ void SpellCheckClient::RequestCheckingOfText(
}

// Clean up the previous request before starting a new request.
if (pending_request_param_.get()) {
if (pending_request_param_) {
pending_request_param_->completion()->DidCancelCheckingText();
}

pending_request_param_.reset(new SpellcheckRequest(text, completionCallback));

base::ThreadTaskRunnerHandle::Get()->PostTask(
FROM_HERE,
base::BindOnce(&SpellCheckClient::PerformSpellCheck, AsWeakPtr(),
base::Owned(pending_request_param_.release())));
base::BindOnce(&SpellCheckClient::SpellCheckText, AsWeakPtr()));
}

bool SpellCheckClient::IsSpellCheckingEnabled() const {
@@ -128,12 +120,13 @@ bool SpellCheckClient::IsShowingSpellingUI() {
void SpellCheckClient::UpdateSpellingUIWithMisspelledWord(
const blink::WebString& word) {}

void SpellCheckClient::SpellCheckText(
const base::string16& text,
bool stop_at_first_result,
std::vector<blink::WebTextCheckingResult>* results) {
if (text.empty() || spell_check_.IsEmpty())
void SpellCheckClient::SpellCheckText() {
const auto& text = pending_request_param_->text();
if (text.empty() || spell_check_.IsEmpty()) {
pending_request_param_->completion()->DidCancelCheckingText();
pending_request_param_ = nullptr;
return;
}

if (!text_iterator_.IsInitialized() &&
!text_iterator_.Initialize(&character_attributes_, true)) {
@@ -153,64 +146,94 @@ void SpellCheckClient::SpellCheckText(

SpellCheckScope scope(*this);
base::string16 word;
int word_start;
int word_length;
for (auto status =
text_iterator_.GetNextWord(&word, &word_start, &word_length);
status != SpellcheckWordIterator::IS_END_OF_TEXT;
status = text_iterator_.GetNextWord(&word, &word_start, &word_length)) {
std::vector<base::string16> words;
auto& word_map = pending_request_param_->wordmap();

This comment has been minimized.

Copy link
@ckerr

ckerr Oct 4, 2018

Member

Will this every be populated from a previous run -- do we need to .clear() this out?

I think the answer is "no" but am not positive

This comment has been minimized.

Copy link
@nitsakh

nitsakh Oct 14, 2018

Author Contributor

Nope, we don't need to clear it. Ideally, we shouldn't receive another request until the first one is complete, i.e. we call DidFinishCheckingText on blink, which happens at the end of this function.

blink::WebTextCheckingResult result;
for (;;) { // Run until end of text
const auto status =
text_iterator_.GetNextWord(&word, &result.location, &result.length);
if (status == SpellcheckWordIterator::IS_END_OF_TEXT)
break;
if (status == SpellcheckWordIterator::IS_SKIPPABLE)
continue;

// Found a word (or a contraction) that the spellchecker can check the
// spelling of.
if (SpellCheckWord(scope, word))
continue;

// If the given word is a concatenated word of two or more valid words
// (e.g. "hello:hello"), we should treat it as a valid word.
if (IsValidContraction(scope, word))
continue;
std::vector<base::string16> contraction_words;
if (!IsContraction(scope, word, &contraction_words)) {
words.push_back(word);
word_map[word].push_back(result);
} else {
// For a contraction, we want check the spellings of each individual
// part, but mark the entire word incorrect if any part is misspelled
// Hence, we use the same word_start and word_length values for every
// part of the contraction.

This comment has been minimized.

Copy link
@ckerr

ckerr Oct 4, 2018

Member

^ This is a good idea!

for (const auto& w : contraction_words) {
words.push_back(w);
word_map[w].push_back(result);
}
}
}

blink::WebTextCheckingResult result;
result.location = word_start;
result.length = word_length;
results->push_back(result);
// Send out all the words data to the spellchecker to check
SpellCheckWords(scope, words);
}

if (stop_at_first_result)
return;
void SpellCheckClient::OnSpellCheckDone(
const std::vector<base::string16>& misspelled_words) {
std::vector<blink::WebTextCheckingResult> results;
auto* const completion_handler = pending_request_param_->completion();

auto& word_map = pending_request_param_->wordmap();

// Take each word from the list of misspelled words received, find their
// corresponding WebTextCheckingResult that's stored in the map and pass
// all the results to blink through the completion callback.
for (const auto& word : misspelled_words) {
auto iter = word_map.find(word);
if (iter != word_map.end()) {
// Word found in map, now gather all the occurrences of the word
// from the map value
auto& words = iter->second;

This comment has been minimized.

Copy link
@ckerr

ckerr Oct 4, 2018

Member

(minor) 'words' is a confusing variable name here because they're ranges / results. I know 'results' is already taken but could something else be used here?

This comment has been minimized.

Copy link
@nitsakh

nitsakh Oct 15, 2018

Author Contributor

Added a comment.

results.insert(results.end(), words.begin(), words.end());
words.clear();
}
}
completion_handler->DidFinishCheckingText(results);
pending_request_param_ = nullptr;
}

bool SpellCheckClient::SpellCheckWord(
void SpellCheckClient::SpellCheckWords(
const SpellCheckScope& scope,
const base::string16& word_to_check) const {
const std::vector<base::string16>& words) {
DCHECK(!scope.spell_check_.IsEmpty());

v8::Local<v8::Value> word = mate::ConvertToV8(isolate_, word_to_check);
v8::Local<v8::Value> result =
scope.spell_check_->Call(scope.provider_, 1, &word);
v8::Local<v8::FunctionTemplate> templ = mate::CreateFunctionTemplate(
isolate_, base::Bind(&SpellCheckClient::OnSpellCheckDone, AsWeakPtr()));

if (!result.IsEmpty() && result->IsBoolean())
return result->BooleanValue();
else
return true;
v8::Local<v8::Value> args[] = {mate::ConvertToV8(isolate_, words),
templ->GetFunction()};
// Call javascript with the words and the callback function
scope.spell_check_->Call(scope.provider_, 2, args);
}

// Returns whether or not the given string is a valid contraction.
// Returns whether or not the given string is a contraction.
// This function is a fall-back when the SpellcheckWordIterator class
// returns a concatenated word which is not in the selected dictionary
// (e.g. "in'n'out") but each word is valid.
bool SpellCheckClient::IsValidContraction(const SpellCheckScope& scope,
const base::string16& contraction) {
// Output variable contraction_words will contain individual
// words in the contraction.
bool SpellCheckClient::IsContraction(
const SpellCheckScope& scope,
const base::string16& contraction,
std::vector<base::string16>* contraction_words) {
DCHECK(contraction_iterator_.IsInitialized());

contraction_iterator_.SetText(contraction.c_str(), contraction.length());

base::string16 word;
int word_start;
int word_length;

for (auto status =
contraction_iterator_.GetNextWord(&word, &word_start, &word_length);
status != SpellcheckWordIterator::IS_END_OF_TEXT;
@@ -219,18 +242,9 @@ bool SpellCheckClient::IsValidContraction(const SpellCheckScope& scope,
if (status == SpellcheckWordIterator::IS_SKIPPABLE)
continue;

if (!SpellCheckWord(scope, word))
return false;
contraction_words->push_back(word);
}
return true;
}

void SpellCheckClient::PerformSpellCheck(SpellcheckRequest* param) {
DCHECK(param);

std::vector<blink::WebTextCheckingResult> results;
SpellCheckText(param->text(), false, &results);
param->completion()->DidFinishCheckingText(results);
return contraction_words->size() > 1;
}

SpellCheckClient::SpellCheckScope::SpellCheckScope(
@@ -31,19 +31,13 @@ class SpellCheckClient : public blink::WebSpellCheckPanelHostClient,
public base::SupportsWeakPtr<SpellCheckClient> {
public:
SpellCheckClient(const std::string& language,
bool auto_spell_correct_turned_on,
v8::Isolate* isolate,
v8::Local<v8::Object> provider);
~SpellCheckClient() override;

private:
class SpellcheckRequest;
// blink::WebTextCheckClient:
void CheckSpelling(
const blink::WebString& text,
int& misspelledOffset,
int& misspelledLength,
blink::WebVector<blink::WebString>* optionalSuggestions) override;
void RequestCheckingOfText(
const blink::WebString& textToCheck,
blink::WebTextCheckingCompletion* completionCallback) override;
@@ -65,22 +59,27 @@ class SpellCheckClient : public blink::WebSpellCheckPanelHostClient,
~SpellCheckScope();
};

// Check the spelling of text.
void SpellCheckText(const base::string16& text,
bool stop_at_first_result,
std::vector<blink::WebTextCheckingResult>* results);
// Run through the word iterator and send out requests
// to the JS API for checking spellings of words in the current
// request.
void SpellCheckText();

// Call JavaScript to check spelling a word.
bool SpellCheckWord(const SpellCheckScope& scope,
const base::string16& word_to_check) const;
// The javascript function will callback OnSpellCheckDone
// with the results of all the misspelled words.
void SpellCheckWords(const SpellCheckScope& scope,
const std::vector<base::string16>& words);

// Returns whether or not the given word is a contraction of valid words
// (e.g. "word:word").
bool IsValidContraction(const SpellCheckScope& scope,
const base::string16& word);

// Performs spell checking from the request queue.
void PerformSpellCheck(SpellcheckRequest* param);
// Output variable contraction_words will contain individual
// words in the contraction.
bool IsContraction(const SpellCheckScope& scope,
const base::string16& word,
std::vector<base::string16>* contraction_words);

This comment has been minimized.

Copy link
@ckerr

ckerr Oct 4, 2018

Member

Any reason contraction_words is a pointer instead of a reference here?

This comment has been minimized.

Copy link
@nitsakh

nitsakh Oct 15, 2018

Author Contributor

Nope, changed.

This comment has been minimized.

Copy link
@nitsakh

nitsakh Oct 15, 2018

Author Contributor

Ohh, just saw. The linter tells to make this a pointer.
Is this a non-const reference? If so, make const or use a pointer: std::vector<base::string16>& contraction_words [runtime/references] [2]


// Callback for the JS API which returns the list of misspelled words.
void OnSpellCheckDone(const std::vector<base::string16>& misspelled_words);

// Represents character attributes used for filtering out characters which
// are not supported by this SpellCheck object.
@@ -215,15 +215,14 @@ int WebFrame::GetWebFrameId(v8::Local<v8::Value> content_window) {

void WebFrame::SetSpellCheckProvider(mate::Arguments* args,
const std::string& language,
bool auto_spell_correct_turned_on,

This comment has been minimized.

Copy link
@nitsakh

nitsakh Aug 12, 2018

Author Contributor

I found that we weren't doing anything with this parameter in the API, so getting rid of it and also removing from the API.

This comment has been minimized.

Copy link
@jwheare

jwheare May 8, 2019

Contributor

Is this a breaking change? Our app currently has this code which seems like it will now fail:

webFrame.setSpellCheckProvider(locale, true, provider);

This comment has been minimized.

Copy link
@jwheare

jwheare May 8, 2019

Contributor

Just confirmed that it is, see electron-userland/electron-spellchecker#144 this should be noted as breaking in the release notes, it's currently a "Feature"

This comment has been minimized.

Copy link
@jwheare

jwheare May 8, 2019

Contributor

Never mind, I see this has been raised elsewhere #17915

v8::Local<v8::Object> provider) {
if (!provider->Has(mate::StringToV8(args->isolate(), "spellCheck"))) {
args->ThrowError("\"spellCheck\" has to be defined");
return;
}

auto client = std::make_unique<SpellCheckClient>(
language, auto_spell_correct_turned_on, args->isolate(), provider);
auto client =
std::make_unique<SpellCheckClient>(language, args->isolate(), provider);
// Set spellchecker for all live frames in the same process or
// in the sandbox mode for all live sub frames to this WebFrame.
FrameSpellChecker spell_checker(
@@ -58,7 +58,6 @@ class WebFrame : public mate::Wrappable<WebFrame> {
// Set the provider that will be used by SpellCheckClient for spell check.
void SetSpellCheckProvider(mate::Arguments* args,
const std::string& language,
bool auto_spell_correct_turned_on,
v8::Local<v8::Object> provider);

void RegisterURLSchemeAsBypassingCSP(const std::string& scheme);
@@ -57,26 +57,34 @@ Sets the maximum and minimum pinch-to-zoom level.

Sets the maximum and minimum layout-based (i.e. non-visual) zoom level.

### `webFrame.setSpellCheckProvider(language, autoCorrectWord, provider)`
### `webFrame.setSpellCheckProvider(language, provider)`

* `language` String
* `autoCorrectWord` Boolean
* `provider` Object
* `spellCheck` Function - Returns `Boolean`.
* `text` String
* `spellCheck` Function.
* `words` String[]
* `callback` Function
* `misspeltWords` String[]

This comment has been minimized.

Copy link
@kwonoj

kwonoj Aug 12, 2018

Member

curious if callback accepts Array<boolean> instead of returning word itself. i.e,

spellcheck: (words, callback) => {
 // do some async
 callback(words.map(isMisspelled));
}

This comment has been minimized.

Copy link
@nitsakh

nitsakh Aug 12, 2018

Author Contributor

Currently it doesn't, but I can make changes to change the API to accept Array<boolean> if we want.
What would be the benefit of doing that?

This comment has been minimized.

Copy link
@kwonoj

kwonoj Aug 12, 2018

Member

(just my 5c) it makes js callback doesn't need to maintain list of words to be returned but just apply fn then return it directly - i.e I could do similar like below in provider:

// this is fn signature runs spellcheck in async and returns result
const isMisspelled: async (word) => boolean; 

spellcheck: (words, callback) => 
  Observable.from(words)
    .mergeMap((w) => isMisspelled(w))
    .toArray().subscribe(callback);

when provider required to return array of misspelled words, provider fn need to re-map from result of spellcheck to construct list of misspelled words to be returned.

This comment has been minimized.

Copy link
@kwonoj

kwonoj Aug 12, 2018

Member

Also sort of alignment (still it's breaking change), previously provider returns boolean for single words, now returns array of boolean for corresponding words.

This comment has been minimized.

Copy link
@nitsakh

nitsakh Aug 13, 2018

Author Contributor

That sounds reasonable. However, to do that, we will have to maintain the list of all words on the c++ side and then map the returned array to those to get the locations in the text to return back to blink. I was just trying to avoid running through all the words by using a map. So, it's definitely doable but I'm not sure how important it is.
Also, APIwise returning misspelt words seems better than an array of booleans. But that's just me. 😃
I'd like to see if others have any opinions about this. I'm okay going either way!

This comment has been minimized.

Copy link
@nitsakh

nitsakh Aug 13, 2018

Author Contributor

/cc @juturu


Sets a provider for spell checking in input fields and text areas.

The `provider` must be an object that has a `spellCheck` method that returns
whether the word passed is correctly spelled.
The `provider` must be an object that has a `spellCheck` method that accepts
an array of individual words for spellchecking.
The `spellCheck` function runs asynchronously and calls the `callback` function
with an array of misspelt words when complete.

An example of using [node-spellchecker][spellchecker] as provider:

```javascript
const { webFrame } = require('electron')
webFrame.setSpellCheckProvider('en-US', true, {
spellCheck (text) {
return !(require('spellchecker').isMisspelled(text))
const spellChecker = require('spellchecker')
webFrame.setSpellCheckProvider('en-US', {
spellCheck (words, callback) {
setTimeout(() => {
const spellchecker = require('spellchecker')
const misspelled = words.filter(x => spellchecker.isMisspelled(x))
callback(misspelled)
}, 0)
}
})
```
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.