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(worditerator): bump up worditerator into latest #11819

Merged
merged 1 commit into from Feb 12, 2018

Conversation

Projects
None yet
4 participants
@kwonoj
Member

kwonoj commented Feb 3, 2018

This PR intends to bump up SpellcheckWordIterator implementation from latest chromium source. There were few fix / enhancements around word iterator, brief changes are like below:

75b1c3a Convert 0 and NULL to nullptr in components. 
fabb284 Convert chrome/renderer to std::unique_ptr by thakis 
3a99491 Correct OutputArabic()s filtering of chars irrelevant to spell-checking by krb 
e73d852 Convert Pass()→std::move() in //chrome by dcheng 
2729e44 Switch to standard integer types in chrome/. by avi 
4f336a6 Updates SpellcheckWordIterator::GetNextWord to return an enum.

Not sure how to migrate test cases - there are spellcheck_worditerator_unittest.cc in chromium source, but doesn't seem like corresponding test cases in Electron's repo side. Open up as PR to get suggestion / or opinions around.

@kwonoj kwonoj requested a review from electron/reviewers as a code owner Feb 3, 2018

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Feb 3, 2018

@kwonoj How does this impact tools like electron-spellchecker?

@kwonoj

This comment has been minimized.

Member

kwonoj commented Feb 3, 2018

@MarshallOfSound SpellcheckWordIterator is not directly exposed to any webFrame::spellchecker interfaces in Electron, so functionally it shouldn't impact (as SpellcheckWordIterator interface does not changed mostly). As log indicated there are few improvements over for some word boundary detection, it may help some of false negatives around word boundary detection (but it is not verified at this moment).

@kwonoj

This comment has been minimized.

Member

kwonoj commented Feb 3, 2018

Our application heavily rely on Spellchecker interface, and also getting some false negatives around word boundary - reason tried to bump up implementation just in case it enhances situation. We are currently trying to use https://github.com/kwonoj/electron-hunspell instead of electron-spellchecker, fyi.

@deepak1556

LGTM with minor changes.

if (iterator_->IsWord()) {
if (Normalize(start, length, word_string)) {
switch (iterator_->GetWordBreakStatus()) {
case base::i18n::BreakIterator::IS_WORD_BREAK: {

This comment has been minimized.

@deepak1556

deepak1556 Feb 5, 2018

Member

It would be enough to handle only base::i18n::BreakIterator::IS_WORD_BREAK if we were to maintain consistency with existing logic. Currently we don't seem to handle IS_SKIPPABLE_WORD

This comment has been minimized.

@kwonoj

kwonoj Feb 5, 2018

Member

I am planning to come up with another PR to utilize skippable word enum status. (And would like to leave worditerator as is to chromium's)

This comment has been minimized.

@deepak1556

deepak1556 Feb 5, 2018

Member

In that case, we should let the loop https://github.com/electron/electron/pull/11819/files#diff-bfd35a1e98d8d7cb08cc06502b38b7d8R125 continue for IS_SKIPPABLE_WORD until its utilized.

This comment has been minimized.

@kwonoj

kwonoj Feb 5, 2018

Member

make sense, will try to update.

This comment has been minimized.

@kwonoj

kwonoj Feb 5, 2018

Member

second thought, doesn't current loop keep iterate when IS_SKIPPABLE_WORD reaches? while only exits when IS_END_TEXT reaches, and IS_SKIPPABLE_WORD runs to call getNextWord again. (same as previous logic, only stops loop when getNextWord returns false)

This comment has been minimized.

@deepak1556

deepak1556 Feb 5, 2018

Member

Yes it does iterate, but also does spell checking with IS_SKIPPABLE_WORD. We should instead continue the loop.

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)) {
   if (status == SpellcheckWordIterator::IS_SKIPPABLE)
     continue;

This comment has been minimized.

@kwonoj

kwonoj Feb 5, 2018

Member

so my original intention was leave it as-is (to not to change spellcheck side behavior yet, cause current spellchecker did similar anway to check skippable word but iterator just didn't distinguish it), but sounds fair and updated per suggestion.

@@ -171,7 +178,13 @@ bool SpellCheckClient::IsValidContraction(const SpellCheckScope& scope,
int word_start;
int word_length;
while (contraction_iterator_.GetNextWord(&word, &word_start, &word_length)) {
for (auto status =
text_iterator_.GetNextWord(&word, &word_start, &word_length);

This comment has been minimized.

@deepak1556

deepak1556 Feb 5, 2018

Member

text_iterator_ => contraction_iterator_

This comment has been minimized.

@kwonoj

kwonoj Feb 5, 2018

Member

Ooopy, sorry for my copy-pasta.

@deepak1556

Can you run clang-format to fix some style issues. Thanks!

@kwonoj

This comment has been minimized.

Member

kwonoj commented Feb 6, 2018

@deepak1556 updated PR with clang-format.

@ckerr

This comment has been minimized.

Member

ckerr commented Feb 7, 2018

@kwonoj the code looks OK but can you check on those electron-linux-ia32 and electron-linux-x64 CI failures? I'd prefer for those to be green before merging.

@kwonoj

This comment has been minimized.

Member

kwonoj commented Feb 7, 2018

@ckerr those build fails with test below. I am quite certain this PR doesn't change behavior around crashrepporter and not sure what to do fix those test. Any suggestions?

Your build ran 884 tests with 1 failure
crashReporter module with sandbox should send minidump when renderer crashes - should send minidump when renderer crashes

@kwonoj

This comment has been minimized.

Member

kwonoj commented Feb 7, 2018

@ckerr ok, retriggered build and seems now fixed.

@MarshallOfSound MarshallOfSound merged commit 01dcdde into electron:master Feb 12, 2018

8 checks passed

ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
@welcome

This comment has been minimized.

welcome bot commented Feb 12, 2018

Congrats on merging your first pull request! 🎉🎉🎉

@kwonoj kwonoj deleted the kwonoj:feat-worditerator branch Feb 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment