Remove dead ContextMenu code from Blink core

Remove dead ContextMenu code from Blink core

Most of ContextMenuController was devoted to building up context menus
and handling actions for those menus it had built up, but in Blink/Chromium,
those menus are almost entirely dropped on the floor by the embedder.
Only those menu items tagged as "custom" are preserved. And these are added
elsewhere, by ContextMenuProvider instances; the only currently-existing
one belongs to the web inspector.

As part of this cleanup, renamed ContextMenuClient::customizeMenu() to
showContextMenu(), since it's a call to that method from the controller
which actually causes a context menu to appear. Also tightened up that
method, which has no reason to transfer ownership (or even allow mutation)
of its ContextMenu to the embedder.

R=darin

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=150375

[adamk, 2013-05-14 00:30:52]

https://codereview.chromium.org/14795011/diff/1/Source/core/page/ContextMenuC...
File Source/core/page/ContextMenuController.cpp (left):

https://codereview.chromium.org/14795011/diff/1/Source/core/page/ContextMenuC...
Source/core/page/ContextMenuController.cpp:575: Vector<String> guesses =
frame->editor()->guessesForMisspelledOrUngrammatical(misspelling, badGrammar);
Just happened to be looking at the code for
guessesForMisspelledOrUngrammatical() and found that it actually has
side-effects. So I'm a little worried that this chunk of code, which we used to
run to almost no effect, might not be entirely dead.

Any suggestions for what tests I might want to run to trigger this stuff?

[adamk, 2013-05-14 00:33:01]

+groby for Mac spellchecking insight. See my comment about
Editor::guessesForMisspelledOrUngrammatical(): it happens to call
updateSpellingUIWithMisspelledWord(). Is this actually live code somehow?

[groby-ooo-7-16, 2013-05-14 00:57:54]

I'm reasonably confident this is dead code, see inline comment.

https://codereview.chromium.org/14795011/diff/1/Source/core/page/ContextMenuC...
File Source/core/page/ContextMenuController.cpp (left):

https://codereview.chromium.org/14795011/diff/1/Source/core/page/ContextMenuC...
Source/core/page/ContextMenuController.cpp:575: Vector<String> guesses =
frame->editor()->guessesForMisspelledOrUngrammatical(misspelling, badGrammar);
I'm not sure what side effects you're seeing there?

As far as I can see, this is side-effect free, except for populating the
ContextMenu. If you're concerned about losing the suggestions, we do populate
those ourselves in Source/WebKit/chromium/src/ContextMenuClientImpl.cpp

It should be safe to remove this code. (You can easily test it manually - after
removal, you should still see suggestions for misspellings, even when the
server-side checking is off)

I am not aware that this code is actually used, or of any tests exercising it.

On 2013/05/14 00:30:52, adamk wrote:
> Just happened to be looking at the code for
> guessesForMisspelledOrUngrammatical() and found that it actually has
> side-effects. So I'm a little worried that this chunk of code, which we used
to
> run to almost no effect, might not be entirely dead.
> 
> Any suggestions for what tests I might want to run to trigger this stuff?

[adamk, 2013-05-14 01:05:12]

https://codereview.chromium.org/14795011/diff/1/Source/core/page/ContextMenuC...
File Source/core/page/ContextMenuController.cpp (left):

https://codereview.chromium.org/14795011/diff/1/Source/core/page/ContextMenuC...
Source/core/page/ContextMenuController.cpp:575: Vector<String> guesses =
frame->editor()->guessesForMisspelledOrUngrammatical(misspelling, badGrammar);
On 2013/05/14 00:57:54, groby wrote:
> I'm not sure what side effects you're seeing there?

The call sequence I'm worried about is:

Editor::guessesForMisspelledOrUngrammatical
TextCheckingHelper::guessesForMisspelledOrUngrammaticalRange
TextCheckingClient::updateSpellingUIWithMisspelledWord
SpellCheckProvider::updateSpellingUIWithMisspelledWord

and the provider (on MAC) does:

Send(new SpellCheckHostMsg_UpdateSpellingPanelWithMisspelledWord(routing_id(),
word))

[groby-ooo-7-16, 2013-05-14 01:44:31]

Ah. That does indeed have a side effect, sorry for missing it.

If you have the "Spelling and Grammar" panel on Mac open, it shows you the
currently misspelled word. Right-clicking on a misspelling makes that the
current misspelled word. I don't believe there are tests.
The easiest way to keep this still working is in ContextMenuClientImpl.cc,
line 315 (after call to selectMisspellingAsync), add

if(data.misspelledWord.length() &&  m_webView->spellCheckClient())


m_webView->spellCheckClient()->updateSpellingUIWithMisspelledWord(data.misspelledWord)

That should do the trick. (It's _slightly_ different - it only works if
there are already misspelling markers in place. I.e. if you
type/right-click very fast, it won't do the update. I can live with that)

Please do verify this actually works - I currently have my tree in a state
that doesn't allow me to actually try this code :)



On Mon, May 13, 2013 at 6:05 PM, <adamk@chromium.org> wrote:

>
> https://codereview.chromium.**org/14795011/diff/1/Source/**core/page/**
>
ContextMenuController.cpp<https://codereview.chromium.org/14795011/diff/1/Source/core/page/ContextMenuController.cpp>
> File Source/core/page/**ContextMenuController.cpp (left):
>
> https://codereview.chromium.**org/14795011/diff/1/Source/**core/page/**
>
ContextMenuController.cpp#**oldcode575<https://codereview.chromium.org/14795011/diff/1/Source/core/page/ContextMenuController.cpp#oldcode575>
> Source/core/page/**ContextMenuController.cpp:575: Vector<String> guesses =
> frame->editor()->**guessesForMisspelledOrUngramma**tical(misspelling,
> badGrammar);
> On 2013/05/14 00:57:54, groby wrote:
>
>> I'm not sure what side effects you're seeing there?
>>
>
> The call sequence I'm worried about is:
>
> Editor::**guessesForMisspelledOrUngramma**tical
> TextCheckingHelper::**guessesForMisspelledOrUngramma**ticalRange
> TextCheckingClient::**updateSpellingUIWithMisspelled**Word
> SpellCheckProvider::**updateSpellingUIWithMisspelled**Word
>
> and the provider (on MAC) does:
>
> Send(new
> SpellCheckHostMsg_**UpdateSpellingPanelWithMisspel**ledWord(routing_id(),
> word))
>
>
https://codereview.chromium.**org/14795011/<https://codereview.chromium.org/1...
>

[adamk, 2013-05-14 21:18:08]

On 2013/05/14 01:44:31, groby wrote:
> Ah. That does indeed have a side effect, sorry for missing it.
> 
> If you have the "Spelling and Grammar" panel on Mac open, it shows you the
> currently misspelled word. Right-clicking on a misspelling makes that the
> current misspelled word. I don't believe there are tests.

Testing this manually seems to work for me, even with my patch applied:
right-clicking causes the right-clicked word to show up in the panel. So maybe
this is all a false alarm?

> The easiest way to keep this still working is in ContextMenuClientImpl.cc,
> line 315 (after call to selectMisspellingAsync), add
> 
> if(data.misspelledWord.length() &&  m_webView->spellCheckClient())
> 
> 
>
m_webView->spellCheckClient()->updateSpellingUIWithMisspelledWord(data.misspelledWord)
> 
> That should do the trick. (It's _slightly_ different - it only works if
> there are already misspelling markers in place. I.e. if you
> type/right-click very fast, it won't do the update. I can live with that)
> 
> Please do verify this actually works - I currently have my tree in a state
> that doesn't allow me to actually try this code :)
> 
> 
> 
> On Mon, May 13, 2013 at 6:05 PM, <mailto:adamk@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.**org/14795011/diff/1/Source/**core/page/**
> >
>
ContextMenuController.cpp<https://codereview.chromium.org/14795011/diff/1/Source/core/page/ContextMenuController.cpp>
> > File Source/core/page/**ContextMenuController.cpp (left):
> >
> > https://codereview.chromium.**org/14795011/diff/1/Source/**core/page/**
> >
>
ContextMenuController.cpp#**oldcode575<https://codereview.chromium.org/14795011/diff/1/Source/core/page/ContextMenuController.cpp#oldcode575>
> > Source/core/page/**ContextMenuController.cpp:575: Vector<String> guesses =
> > frame->editor()->**guessesForMisspelledOrUngramma**tical(misspelling,
> > badGrammar);
> > On 2013/05/14 00:57:54, groby wrote:
> >
> >> I'm not sure what side effects you're seeing there?
> >>
> >
> > The call sequence I'm worried about is:
> >
> > Editor::**guessesForMisspelledOrUngramma**tical
> > TextCheckingHelper::**guessesForMisspelledOrUngramma**ticalRange
> > TextCheckingClient::**updateSpellingUIWithMisspelled**Word
> > SpellCheckProvider::**updateSpellingUIWithMisspelled**Word
> >
> > and the provider (on MAC) does:
> >
> > Send(new
> > SpellCheckHostMsg_**UpdateSpellingPanelWithMisspel**ledWord(routing_id(),
> > word))
> >
> >
>
https://codereview.chromium.**org/14795011/%3Chttps://codereview.chromium.org...>
> >

[adamk, 2013-05-14 21:19:01]

+eseidel to review

[darin (slow to review), 2013-05-14 21:25:34]

LGTM

[commit-bot: I haz the power, 2013-05-14 21:27:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/adamk@chromium.org/14795011/1

[commit-bot: I haz the power, 2013-05-14 23:34:05]

Change committed as 150375