Using Chromium's spellchecker #9951

darkdh · 2017-07-11T02:55:58Z

Test Plan:

a. Suggestions

Go to about:styles
Type braave in textbox
There should be redline under braave
Open context menu on braave
You can see bravo, brave, Brava
Click on brave
The text should be replaced with brave with no redline

b. Learn Spelling

Go to about:styles
Type braave in textbox
There should be redline under braave
Open context menu on braave
Click Learn Spelling
braave will no longer be redlined
Next time you type braave, you will not see redline

c. Forget Learned Spelling

Go to about:styles
Type braave in textbox
There should be redline under braave
Open context menu on braave
Click Learn Spelling
braave will no longer be redlined
Open context menu on braave
Click Forget Learned Spelling
Next time you type braave, you will see redline under it

d. Legacy data migration

Use older Brave to Learn Spelling for braave and Ignore Spelling for braaave
Update to the version with chromium spellchecker
There will be no dictionary structure in session store
You can see those two words will be added to Custom Dictionary.txt under user folder
And try to type braave and braaave in about:styles
They should be valid words

Auditors: @bridiver, @bbondy, @bsclifton

fixes #9880

requires brave/muon#244

Submitter Checklist:

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).
Tagged reviewers and labelled the pull request as needed.

Reviewer Checklist:

Tests

Adequate test coverage exists to prevent regressions
Tests should be independent and work correctly when run individually or as a suite ref
New files have MPL2 license header

darkdh · 2017-07-11T02:56:25Z

working on migrating old custom dictionary data

NejcZdovc · 2017-07-11T05:43:44Z

app/browser/menu.js

@@ -266,6 +266,7 @@ const createViewSubmenu = () => {
      label: locale.translation('toggleDeveloperTools'),
      accelerator: isDarwin ? 'Cmd+Alt+I' : 'Ctrl+Shift+I',
      click: function () {
+        const appStore = require('../../js/stores/appStore')


This was already fixed, so if you rebase to the master, you can remove this code 😉

ah, I forgot to remove the workaround. thanks

bridiver · 2017-07-11T18:37:24Z

js/constants/messages.js

@@ -77,7 +77,6 @@ const messages = {
  RELOAD: _,
  DETACH: _,
  IS_MISSPELLED: _, /** @arg {string} word, the word to check */


this should also be removed

bridiver · 2017-07-11T18:39:31Z

app/spellCheck.js

@@ -35,13 +35,6 @@ module.exports.init = () => {
    }


doesn't this whole file go away?

😅 yeah, I forgot to remove

bridiver · 2017-07-11T18:46:01Z

js/contextMenus.js

-          webviewActions.replace(selection)
+        click: (item, focusedWindow) => {
+          if (focusedWindow) {
+            focusedWindow.webContents.addWord(selection)


focusedWindow should be deprecated and context menu items should use actions by default. I would use something like appActions.learnSpelling and appActions.forgetLearnedSpelling and pass the tabId (always set in muon HandleContextMenu and retrieved from nodeProps.properties)

bridiver · 2017-07-13T02:16:34Z

app/browser/reducers/spellCheckerReducer.js

+const {getWebContents} = require('../webContentsCache')
+const spellChecker = require('../../spellChecker')
+
+const migrate = (state) => {


the other data migrations are in session store. We don't want it to get too bloated, but if we don't remove dictionary from the saved state this migration will run on every restart

bsclifton · 2017-07-13T05:00:33Z

Will check this out once Muon 4.4.0 prebuilt is ready to go 😄

bridiver

see comment

bridiver

++

bsclifton

I'm having issues with this; I built locally (macOS), ensured I am using Muon 4.4.0... the wrong spelling (ex: braave) does get underlined red. However, picking from the drop down does not work (both correct spelling and learn spelling)

@bridiver

fixes #9880 Auditors: @bridiver, @bbondy, @bsclifton Test plan: a. Suggestions 1. Go to `about:styles` 2. Type `braave` in textbox 3. There should be redline under `braave` 4. Open context menu on `braave` 5. You can see `bravo`, `brave`, `Brava` 6. Click on `brave` 7. The text should be replaced with `brave` with no redline b. Learn Spelling 1. Go to `about:styles` 2. Type `braave` in textbox 3. There should be redline under `braave` 4. Open context menu on `braave` 5. Click `Learn Spelling` 6. `braave` will no longer be redlined 7. Next time you type `braave`, you will not see redline c. Forget Learned Spelling 1. Go to `about:styles` 2. Type `braave` in textbox 3. There should be redline under `braave` 4. Open context menu on `braave` 5. Click `Learn Spelling` 6. `braave` will no longer be redlined 7. Open context menu on `braave` 8. Click `Forget Learned Spelling` 9. Next time you type `braave`, you will see redline under it d. Legacy data migration 1. Use older Brave to `Learn Spelling` for `braave` and `Ignore Spelling` for `braaave` 2. Update to the version with chromium spellchecker 3. There will be no `dictionary` structure in session store 4. You can see those two words will be added to `Custom Dictionary.txt` under user folder 5. And try to type `braave` and `braaave` in `about:styles` 6. They should be valid words

@darkdh

Auditors: @darkdh Test Plan: `npm run unittest -- --grep="spellCheckerReducer unit tests"`

@darkdh

Auditors: @darkdh

bsclifton

Found issues with immutable that we fixed together 😄 I added unit tests for the spellCheckerReducer to help prevent that in the future. Also, I found/fixed a context menu test which failed because the code goes down a previous branch than before

darkdh

++

bsclifton · 2017-07-14T23:22:43Z

app/spellCheck.js

-  // are incorrectly marked as spelling errors. This lets people get away with
-  // incorrectly spelled contracted words, but it's the best we can do for now.
-  const contractions = [
-    "ain't", "aren't", "can't", "could've", "couldn't", "couldn't've", "didn't", "doesn't", "don't", "hadn't",


pfft new spell checker ain't got these words, y'all

Using Chromium's spellchecker

Jacalz · 2017-07-23T20:18:49Z

This fixed #7967 thank you guys so so much 👍

luixxiul · 2017-07-23T20:33:37Z

@darkdh does this also close #2183 and #4697?

darkdh added the PR/work-in-progress ⚒ label Jul 11, 2017

darkdh self-assigned this Jul 11, 2017

darkdh requested review from bridiver, bbondy and bsclifton July 11, 2017 02:55

darkdh removed request for bridiver, bbondy and bsclifton July 11, 2017 02:56

darkdh changed the title ~~Using Chromium's spellchecker~~ [WIP] Using Chromium's spellchecker Jul 11, 2017

NejcZdovc reviewed Jul 11, 2017

View reviewed changes

darkdh force-pushed the cr-spellchecker branch 2 times, most recently from e394659 to 309e703 Compare July 11, 2017 05:53

bridiver reviewed Jul 11, 2017

View reviewed changes

darkdh force-pushed the cr-spellchecker branch from f233f7c to 0e8f45a Compare July 11, 2017 23:09

darkdh added this to the 0.19.x (Developer Channel) milestone Jul 12, 2017

darkdh changed the title ~~[WIP] Using Chromium's spellchecker~~ Using Chromium's spellchecker Jul 13, 2017

darkdh requested review from bridiver, bbondy and bsclifton July 13, 2017 00:25

darkdh removed the PR/work-in-progress ⚒ label Jul 13, 2017

bridiver reviewed Jul 13, 2017

View reviewed changes

bridiver suggested changes Jul 13, 2017

View reviewed changes

darkdh requested a review from bridiver July 13, 2017 20:26

bridiver approved these changes Jul 13, 2017

View reviewed changes

darkdh added the PR/needs-QA-attention ☕ label Jul 13, 2017

darkdh mentioned this pull request Jul 13, 2017

Move closed tabs to appState #9986

Closed

darkdh force-pushed the cr-spellchecker branch from e5b7aed to 04b1288 Compare July 14, 2017 16:48

bsclifton suggested changes Jul 14, 2017

View reviewed changes

darkdh force-pushed the cr-spellchecker branch from 04b1288 to 4bcde36 Compare July 14, 2017 22:30

darkdh and others added 3 commits July 14, 2017 16:04

Added unit tests for spellCheckerReducer :)

3b5fbd5

Auditors: @darkdh Test Plan: `npm run unittest -- --grep="spellCheckerReducer unit tests"`

Fix for context menu unit test

166b321

Auditors: @darkdh

bsclifton force-pushed the cr-spellchecker branch from 4bcde36 to 166b321 Compare July 14, 2017 23:17

bsclifton approved these changes Jul 14, 2017

View reviewed changes

darkdh mentioned this pull request Jul 14, 2017

Use Chromium spell checker #9880

Closed

darkdh commented Jul 14, 2017

View reviewed changes

bsclifton reviewed Jul 14, 2017

View reviewed changes

bsclifton added feature/context-menu QA/test-plan-specified release-notes/include perf and removed PR/needs-QA-attention ☕ labels Jul 14, 2017

bsclifton merged commit 8ccbbdb into master Jul 14, 2017

bsclifton deleted the cr-spellchecker branch July 14, 2017 23:24

bsclifton added a commit that referenced this pull request Jul 14, 2017

Merge pull request #9951 from brave/cr-spellchecker

7ac39b8

Using Chromium's spellchecker

bsclifton modified the milestones: 0.18.x (Beta Channel), 0.19.x (Developer Channel) Jul 23, 2017

bsclifton added a commit that referenced this pull request Jul 23, 2017

Merge pull request #9951 from brave/cr-spellchecker

9aa3d94

Using Chromium's spellchecker

Jacalz mentioned this pull request Jul 23, 2017

Spellchecking not working with Swedish as system language on Windows #7967

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Using Chromium's spellchecker #9951

Using Chromium's spellchecker #9951

darkdh commented Jul 11, 2017 •

edited by bsclifton

Loading

darkdh commented Jul 11, 2017

NejcZdovc Jul 11, 2017

darkdh Jul 11, 2017

bridiver Jul 11, 2017

bridiver Jul 11, 2017

darkdh Jul 11, 2017

bridiver Jul 11, 2017

bridiver Jul 13, 2017

bsclifton commented Jul 13, 2017

bridiver left a comment

bridiver left a comment

bsclifton left a comment

bsclifton left a comment

darkdh left a comment

bsclifton Jul 14, 2017

Jacalz commented Jul 23, 2017

luixxiul commented Jul 23, 2017

Using Chromium's spellchecker #9951

Using Chromium's spellchecker #9951

Conversation

darkdh commented Jul 11, 2017 • edited by bsclifton Loading

Test Plan:

darkdh commented Jul 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsclifton commented Jul 13, 2017

bridiver left a comment

Choose a reason for hiding this comment

bridiver left a comment

Choose a reason for hiding this comment

bsclifton left a comment

Choose a reason for hiding this comment

bsclifton left a comment

Choose a reason for hiding this comment

darkdh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jacalz commented Jul 23, 2017

luixxiul commented Jul 23, 2017

darkdh commented Jul 11, 2017 •

edited by bsclifton

Loading