Deprecate electron spellchecker by chromium's #244
Conversation
| if (spellcheck) { | ||
| if (spellcheck->GetCustomDictionary() | ||
| ->HasWord(base::UTF16ToUTF8(new_params.selection_text))) { | ||
| new_params.misspelled_word = |
There was a problem hiding this comment.
I'm not sure I understand what is happening here
There was a problem hiding this comment.
It is used for https://github.com/brave/browser-laptop/pull/9951/files#diff-b9f459fd47114771333b78b87fac7daaR557 which provides user an option to remove added word to custom dictionary.
because if a word is in custom dictionary, we can't tell if it is correctly spelled word or in custom dictionary from ContextMenuParams. misspelled_word and dictionary_suggestions will be empty
There was a problem hiding this comment.
per discussion on slack, we should use ContextMenuParams.properties
| @@ -971,6 +989,50 @@ void WebContents::AuthorizePlugin(mate::Arguments* args) { | |||
| web_contents(), true, resource_id); | |||
| } | |||
|
|
|||
| void WebContents::AddWord(mate::Arguments* args) { | |||
There was a problem hiding this comment.
the spellcheck service is associated with the browser context, not the webcontents so this should probably be an api attached to session like autofill
There was a problem hiding this comment.
will do. and this will helps for migrating old words in legacy custom dictionary.
| if (SpellCheckProvider* provider = | ||
| SpellCheckProvider::Get(render_view->GetMainRenderFrame())) | ||
| provider->EnableSpellcheck(spellcheck_->IsSpellcheckEnabled()); | ||
| new SpellCheckPanel(render_view); |
There was a problem hiding this comment.
shouldn't this be wrapped in #if BUILDFLAG(HAS_SPELLCHECK_PANEL) ?
There was a problem hiding this comment.
yes, we should. because I was working based on cr59 and this is added in cr60.
When I pushed the commit, I just rebase the muon for our cr60 commits
| Emit("context-menu", std::make_pair(params, web_contents())); | ||
| // For forgetting custom dictionary option | ||
| content::ContextMenuParams new_params(params); | ||
| if (new_params.misspelled_word.empty()) { |
There was a problem hiding this comment.
we should also check to make sure selection_text is not empty
|
@bridiver feedback addressed! |
| if (spellcheck->GetCustomDictionary()->HasWord(selection_text)) { | ||
| new_params.properties.insert( | ||
| std::pair<std::string, std::string> | ||
| (std::string("learnedSpelling"), selection_text)); |
There was a problem hiding this comment.
maybe "customDictionaryWord"?
| @@ -71,36 +67,9 @@ WebFrameBindings::~WebFrameBindings() { | |||
| } | |||
|
|
|||
| void WebFrameBindings::Invalidate() { | |||
There was a problem hiding this comment.
we should remove this method override
| Emit("context-menu", std::make_pair(params, web_contents())); | ||
| // For forgetting custom dictionary option | ||
| content::ContextMenuParams new_params(params); | ||
| if (new_params.misspelled_word.empty() && |
There was a problem hiding this comment.
actually we should also check is_editable and spellcheck_enabled because spellcheck only applies to editable fields. Just want to narrow this check to run as infrequently as possible
|
lgtm. Just want to verify that this is targeted to browser-laptop 0.18? If not please update the milestone to 4.4.0 |
|
my bad.it should be in 4.4.0 |
Deprecate electron spellchecker by chromium's
Auditors: @bridiver, @bbondy, @bsclifton