Remove "Ask Brave for suggestions" from context menu spellcheck section #682

mkarolin · 2018-10-18T04:36:18Z

Hides RenderViewContextMenu::AddSpellCheckServiceItem method with one in BraveRenderViewContextMenu that does not add Ask Brave menu item.

Fixes brave/brave-browser#1623

Submitter Checklist:

Automated Test Plan:

On all platforms:
npm run test -- brave_browser_tests --filter=BraveSpellingMenuObserverTest.*

On Windows and Linux:
npm run test -- brave_browser_tests --filter=BraveSpellingOptionsSubMenuObserverTest.*

Test Plan:

[Windows & Linux] Verify that Spellcheck sub-menu doesn't contain "Ask Brave for suggestions":

Start the browser and open brave://settings/languages,
Expand "Spell check" section and make sure that at least one language is turned on for spell checking,
In a new tab navigate to google.com,
Right-click in the search box on the site and expand Spellcheck sub-menu,
Verify that the sub-menu doesn't contain a menu item "Ask Brave for suggestions".

[Windows & Linux] Verify that Spellcheck sub-menu doesn't end with a separator when spell checking is turned off for all languages:

Start the browser and open brave://settings/languages,
Expand "Spell check" section and turn off spell checking for all languages,
In a new tab navigate to google.com,
Right-click in the search box on the site and expand Spellcheck sub-menu,
Verify that the sub-menu doesn't end with a separator.

[All Platforms] Verify that spelling suggestions don't show "Ask Brave for suggestions":

Start the browser and open brave://settings/languages,
Expand "Spell check" section and make sure that at least one language is turned on for spell checking,
In a new tab navigate to dictionary.com,
Right-click in the search box on the site and expand Spellcheck sub-menu, make sure that "Check for spelling of text fields" menu item is checked,
Type a misspelled word into the search box on the site (e.g. correctz),
Right click on the misspelled word,
Verify that "Ask Brave for suggestions" doesn't show in the menu (after "Add to dictionary").

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions
Verify test plan is specified in PR before merging to source

bsclifton · 2018-10-18T06:44:33Z

@mkarolin would it be possible to make a browser test for this? I'm not 100% sure context menu items can be tested, but it would be cool to check 😄. If it was possible, a great test would be to ensure this item never shows up when right clicking a text field

mkarolin · 2018-10-18T13:25:11Z

Yes, I believe it’s possible. I think I saw a menu observer browser test in chrome/browser/renderer_context_menu that tests for the presence of this menu item. Will add tomorrow.

…

On Oct 18, 2018, at 02:44, Brian Clifton ***@***.***> wrote: @mkarolin would it be possible to make a browser test for this? I'm not 100% sure context menu items can be tested, but it would be cool to check 😄. If it was possible, a great test would be to ensure this item never shows up when right clicking a text field — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

bsclifton · 2018-10-18T22:20:19Z

Change worked as expected- however, it leaves an awkward separator above where the item used to be:

It might be better to look at the caller for this, which seems to be:
src/chrome/browser/renderer_context_menu/spelling_menu_observer.cc
(see InitMenu())

While we're there, you might check other places that use integrate_spelling_service_ member variable (which I think is tied to the Profile preference spellcheck.use_spelling_service, which is set to false by default)

mkarolin · 2018-10-19T01:41:42Z

Oh that's interesting, I always have another item there:

I'll take a closer look to see what appears when. Thanks for trying it out!

mkarolin · 2018-10-22T17:43:41Z

Updated. There were 2 places where "Ask Brave for suggestions" could show up:

In the Spellcheck sub-menu,
In the menu if spellcheck is enabled for text fields and the menu is invoked on a misspelled word.

Updated test plan to check for both cases.
Added browser tests to verify these cases and to verify the Spellcheck sub-menu doesn't end with a separator.

bsclifton · 2018-10-24T05:33:14Z

browser/renderer_context_menu/brave_spelling_options_submenu_observer.cc

+  DCHECK(submenu_model_.GetItemCount());
+
+  // Special accommodations for gtest.
+  if (gtest_mode_ != GTEST_MODE_DISABLED) {


Just a thought for the future (I'm curious too) - you might check if unit/browser tests expose a #define that you can check for with preprocessors? ex: you could wrap this section (and potentially the definition/setting of gtest_mode_) with #ifdef/#endif guards. I'm not sure if there's a best practice? ex: a common way to check. But you could check like this:

#ifdef IN_PROC_BROWSER_TEST_ #endif

I was looking for something like this as well but didn't see any additional definitions in the tests build in our build setup. gtest itself doesn't seem to add any (https://stackoverflow.com/questions/6077833/how-to-check-if-google-test-is-running-in-my-code). Ideally, I wouldn't want to have a different code path for tests in the first place, but in this case it would mean creating a more complex mock object (the current one was taken from Chromium and slightly modified).

bsclifton · 2018-10-24T05:49:20Z

browser/renderer_context_menu/brave_spelling_options_submenu_observer.cc

+  int index = submenu_model_.GetItemCount() - 1;
+  if (index >= 0 &&
+      submenu_model_.GetTypeAt(index) == ui::MenuModel::TYPE_SEPARATOR)
+    submenu_model_.RemoveItemAt(index);


nitpick: when a comparison is multiple lines (or has weird spacing, enclosing the block within { and } can make it more readable (clearly capturing the start/end)

bsclifton

Really well done! 😄👍 I ran through the manual test plans, automated tests, and then reviewed the code. Left a few comments (nitpicks 😛- no action needed; just food for thought). Great job 😄

bsclifton · 2018-10-24T05:56:45Z

Basically ready for merge - just going to verify tests work great on macOS, since this option isn't available there (build in progress...)

bsclifton · 2018-10-24T06:12:51Z

Getting some compile errors (likely need platform preprocessor guards around this):

../../brave/browser/renderer_context_menu/brave_spelling_options_submenu_observer.cc:52:35: error: use of undeclared identifier 'IDS_CONTENT_CONTEXT_SPELLCHECK_MENU'
        l10n_util::GetStringUTF16(IDS_CONTENT_CONTEXT_SPELLCHECK_MENU),
                                  ^
../../brave/browser/renderer_context_menu/brave_spelling_options_submenu_observer.cc:62:35: error: use of undeclared identifier 'IDS_CONTENT_CONTEXT_SPELLCHECK_MENU'
        l10n_util::GetStringUTF16(IDS_CONTENT_CONTEXT_SPELLCHECK_MENU));
                                  ^
2 errors generated.

bsclifton

Looks like we'll need to put some preprocessors around this functionality. I think we can subclass SpellingOptionsSubMenuObserver wrapped in a #if !defined(OS_MACOSX)

The tests may need that same guard checked too

mkarolin · 2018-10-24T20:06:01Z

Updated with ifdefs for Mac. One curious thing: in the original render_view_context_menu.cc code the header inclusion is predicated on #if !BUILDFLAG(USE_BROWSER_SPELLCHECKER) whereas the use of SpellingOptionsSubMenuObserver is controlled by if !defined (OS_MACOSX). I kept it the same in the override.

one in BraveRenderViewContextMenu that does not add Ask Brave menu item. Fixes brave/brave-browser#1623 Also, overrides SpellingMenuObserver to remove a possible dangling menu separator. Adds browser tests for spelling menu and spelling options submenu that check that the "Ask Brave for suggestions" menu item is not present.

bsclifton · 2018-10-30T00:16:30Z

Built great on macOS and all unit/browser tests passed; waiting for Windows to finish...

bsclifton

Works great on Windows (ran through each test plan) and compiles/tests pass/etc on macOS 😄 👍

Remove "Ask Brave for suggestions" from context menu spellcheck section

bsclifton · 2018-10-30T03:58:03Z

master d743327
0.57.x 5d8323a
0.56.x d674e0b

mkarolin self-assigned this Oct 18, 2018

mkarolin requested review from bsclifton and riastradh-brave October 18, 2018 04:36

mkarolin force-pushed the maxk-remove-ask-brave-for-suggestions branch from 1d53a44 to caf6b4f Compare October 22, 2018 16:10

mkarolin changed the title ~~Remove "Ask Brave for suggestions" from context menu spellcheck section~~ WIP: Remove "Ask Brave for suggestions" from context menu spellcheck section Oct 22, 2018

mkarolin added the pr/work-in-progress label Oct 22, 2018

mkarolin force-pushed the maxk-remove-ask-brave-for-suggestions branch from caf6b4f to e4eef6b Compare October 22, 2018 17:34

mkarolin changed the title ~~WIP: Remove "Ask Brave for suggestions" from context menu spellcheck section~~ Remove "Ask Brave for suggestions" from context menu spellcheck section Oct 22, 2018

mkarolin removed the pr/work-in-progress label Oct 22, 2018

bsclifton reviewed Oct 24, 2018

View reviewed changes

bsclifton approved these changes Oct 24, 2018

View reviewed changes

bsclifton requested changes Oct 24, 2018

View reviewed changes

mkarolin force-pushed the maxk-remove-ask-brave-for-suggestions branch 2 times, most recently from d77639f to 69d1689 Compare October 24, 2018 19:28

bsclifton force-pushed the maxk-remove-ask-brave-for-suggestions branch from 69d1689 to c82efbc Compare October 29, 2018 23:43

srirambv mentioned this pull request Oct 30, 2018

Ask Brave for suggestion needs to be removed brave/brave-browser#1623

Closed

bsclifton approved these changes Oct 30, 2018

View reviewed changes

bsclifton merged commit d743327 into master Oct 30, 2018

bsclifton deleted the maxk-remove-ask-brave-for-suggestions branch October 30, 2018 03:56

bsclifton added a commit that referenced this pull request Oct 30, 2018

Merge pull request #682 from brave/maxk-remove-ask-brave-for-suggestions

5d8323a

Remove "Ask Brave for suggestions" from context menu spellcheck section

bsclifton added a commit that referenced this pull request Oct 30, 2018

Merge pull request #682 from brave/maxk-remove-ask-brave-for-suggestions

d674e0b

Remove "Ask Brave for suggestions" from context menu spellcheck section

bsclifton added the merged/0.56.x-beta label Oct 30, 2018

bbondy added this to the 0.56.x - Release milestone Jan 14, 2019

snyk-bot mentioned this pull request Dec 19, 2020

[Snyk] Upgrade jszip from 3.2.2 to 3.5.0 hixio-mh/brave-core#4

Open

snyk-bot mentioned this pull request Jul 28, 2021

[Snyk] Security upgrade jszip from 3.2.2 to 3.7.0 saminfante/brave-core#2

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove "Ask Brave for suggestions" from context menu spellcheck section #682

Remove "Ask Brave for suggestions" from context menu spellcheck section #682

mkarolin commented Oct 18, 2018 •

edited

Loading

bsclifton commented Oct 18, 2018

mkarolin commented Oct 18, 2018 via email

bsclifton commented Oct 18, 2018

mkarolin commented Oct 19, 2018

mkarolin commented Oct 22, 2018 •

edited

Loading

bsclifton Oct 24, 2018 •

edited

Loading

mkarolin Oct 24, 2018

bsclifton Oct 24, 2018

bsclifton left a comment

bsclifton commented Oct 24, 2018

bsclifton commented Oct 24, 2018

bsclifton left a comment

mkarolin commented Oct 24, 2018

bsclifton commented Oct 30, 2018

bsclifton left a comment

bsclifton commented Oct 30, 2018

Remove "Ask Brave for suggestions" from context menu spellcheck section #682

Remove "Ask Brave for suggestions" from context menu spellcheck section #682

Conversation

mkarolin commented Oct 18, 2018 • edited Loading

Submitter Checklist:

Automated Test Plan:

Test Plan:

Reviewer Checklist:

bsclifton commented Oct 18, 2018

mkarolin commented Oct 18, 2018 via email

bsclifton commented Oct 18, 2018

mkarolin commented Oct 19, 2018

mkarolin commented Oct 22, 2018 • edited Loading

bsclifton Oct 24, 2018 • edited Loading

Choose a reason for hiding this comment

mkarolin Oct 24, 2018

Choose a reason for hiding this comment

bsclifton Oct 24, 2018

Choose a reason for hiding this comment

bsclifton left a comment

Choose a reason for hiding this comment

bsclifton commented Oct 24, 2018

bsclifton commented Oct 24, 2018

bsclifton left a comment

Choose a reason for hiding this comment

mkarolin commented Oct 24, 2018

bsclifton commented Oct 30, 2018

bsclifton left a comment

Choose a reason for hiding this comment

bsclifton commented Oct 30, 2018

mkarolin commented Oct 18, 2018 •

edited

Loading

mkarolin commented Oct 22, 2018 •

edited

Loading

bsclifton Oct 24, 2018 •

edited

Loading