Adds support for the os x spelling panel to chromium. Users can...

Adds support for the os x spelling panel to chromium. Users can
now access it from the context menu and use it to perform
spelling tasks. For more detail, see http://code.google.com/p/chromium/wiki/SpellingPanelPlanningDoc
BUG=None
TEST=The spelling panel should work in os x.

[Avi (use Gerrit), 2009-08-04 14:37:42]

Mostly nits, an initialization question, but LG.

http://codereview.chromium.org/160565/diff/1035/1053
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/160565/diff/1035/1053#newcode676
Line 676: // This message is sent whenever the user specifies that a word should
be changed
80col

http://codereview.chromium.org/160565/diff/1035/1053#newcode683
Line 683:
thisHostView->GetRenderWidgetHost()->ReplaceWord(base::SysNSStringToWide(newWord));
80col

http://codereview.chromium.org/160565/diff/1035/1053#newcode689
Line 689: // other spelling panel methods. This is probably because Apple
assumes that an
The fragment "this is probably because..." is incomplete; please fill it in.

http://codereview.chromium.org/160565/diff/1035/1059
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/160565/diff/1035/1059#newcode205
Line 205: has_spell_checker_document_tag_(false) {
where's the initialization of the document tag?

http://codereview.chromium.org/160565/diff/1035/1058
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/160565/diff/1035/1058#newcode319
Line 319: virtual int  SpellCheckerDocumentTag();
one space after int

http://codereview.chromium.org/160565/diff/1035/1041
File webkit/glue/editor_client_impl.cc (right):

http://codereview.chromium.org/160565/diff/1035/1041#newcode877
Line 877: d->SpellCheck(word,spellCheckerDocumentTag(),
space after comma, next line is indented weird

http://codereview.chromium.org/160565/diff/1035/1041#newcode929
Line 929: const WebCore::String& misspelledWord) {
weird indent; params get 4 spaces

http://codereview.chromium.org/160565/diff/1035/1041#newcode947
Line 947: // browser and ask for the visiblitly, then we run into problems
accessing
spelling for visibility

[jeremy, 2009-08-04 17:04:41]

This looks great! A few comments.

http://codereview.chromium.org/160565/diff/1035/1053
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/160565/diff/1035/1053#newcode681
Line 681: NSString* newWord = [[sender selectedCell] stringValue];
SysNSStringToWide will crash if the argument we give to it is nil, might want to
do a nil check defensively.

http://codereview.chromium.org/160565/diff/1035/1053#newcode704
Line 704: base::SysNSStringToUTF8([sender stringValue]));
Again, nil check.

http://codereview.chromium.org/160565/diff/1035/1046
File chrome/browser/spellcheck_unittest.cc (right):

http://codereview.chromium.org/160565/diff/1035/1046#newcode842
Line 842: const wchar_t* input;
No need for newline here.

http://codereview.chromium.org/160565/diff/1035/1046#newcode844
Line 844: bool input_result;
No need for newline here.

http://codereview.chromium.org/160565/diff/1035/1046#newcode910
Line 910: #endif
#endif  // OS_MACOSX

http://codereview.chromium.org/160565/diff/1035/1059
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/160565/diff/1035/1059#newcode2376
Line 2376: // make the call to get the tag
nit: Capitalze "make" and . at end of comment.

http://codereview.chromium.org/160565/diff/1035/1059#newcode2390
Line 2390: 
extra newline.

http://codereview.chromium.org/160565/diff/1035/1041
File webkit/glue/editor_client_impl.cc (right):

http://codereview.chromium.org/160565/diff/1035/1041#newcode179
Line 179: #endif
#else
NOTIMPLEMENTED();

?

#endif  // OS_MACOSX

http://codereview.chromium.org/160565/diff/1035/1039
File webkit/glue/webframe.h (right):

http://codereview.chromium.org/160565/diff/1035/1039#newcode333
Line 333: // Tell webkit to advannce the selection to the next misspelling.
*advance

[pwicks86, 2009-08-12 17:48:02]

Adding pink to the reviewers.

[Avi (use Gerrit), 2009-08-12 18:13:41]

http://codereview.chromium.org/160565/diff/1070/71
File chrome/browser/renderer_host/render_view_host.h (right):

http://codereview.chromium.org/160565/diff/1070/71#newcode275
Line 275: void ToggleSpellPanel(bool visible);
This is a bit contradictory. "Toggle" usually means to change a value from false
to true or vice versa, so to "toggle" the spell panel to match a boolean is a
bit weird. Later on you have a ShowSpellingPanel(bool) which makes more sense.
(Why the two different names?)

http://codereview.chromium.org/160565/diff/1070/76
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/160565/diff/1070/76#newcode857
Line 857: void RenderView::OnToggleSpellCheck() {
Yes, here toggle means alternate between true and false, and takes no params.

http://codereview.chromium.org/160565/diff/1070/56
File webkit/glue/webframe.h (right):

http://codereview.chromium.org/160565/diff/1070/56#newcode337
Line 337: virtual void ToggleSpellPanel() = 0;
Here toggle is good.

[pwicks86, 2009-08-12 19:15:40]

> http://codereview.chromium.org/160565/diff/1070/71#newcode275
> Line 275: void ToggleSpellPanel(bool visible);
> This is a bit contradictory. "Toggle" usually means to change a value from
false
> to true or vice versa, so to "toggle" the spell panel to match a boolean is a
> bit weird. Later on you have a ShowSpellingPanel(bool) which makes more sense.
> (Why the two different names?)

Yes, this is a bit strange. The boolean argument to ToggleSpellingPanel is the
current spelling panel visibility. It is included here so that when the IPC
message to RenderView is handled, the visibility can be cached in webview().
This way, when webkit (Editor.cpp:2145, i think) asks for the visibility, we can
avoid another IPC call. Not that an additional IPC call is that bad, but any
code that checks the visibility of the spelling panel must run on the UI thread,
which makes getting it from resource_message_filter.cc difficult. Thus, this
seemed to be a decent solution, although maybe there is a better way? If not,
ToggleSpellPanel(bool visible) should probably be changed to
ToggleSpellPanel(bool currentVisibilityStatus) + a comment explaining why it
takes an argument.

[Avi (use Gerrit), 2009-08-12 19:21:32]

On 2009/08/12 19:15:40, pwicks86 wrote:
> If not,
> ToggleSpellPanel(bool visible) should probably be changed to
> ToggleSpellPanel(bool currentVisibilityStatus) + a comment explaining why it
> takes an argument.

OK, that makes sense. Change the argument to something like isCurrentlyVisible
and I'm happy.

Avi

[pink (ping after 24hrs), 2009-08-13 14:29:30]

LGTM with these fixes. Make these changes and I'll try to get it landed.

http://codereview.chromium.org/160565/diff/3003/2023
File chrome/browser/renderer_host/render_widget_host.h (right):

http://codereview.chromium.org/160565/diff/3003/2023#newcode334
Line 334: // Makes an IPC call to tell webkit to advance to the next misspelling
period at end of sentence.

http://codereview.chromium.org/160565/diff/3003/2020
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/160565/diff/3003/2020#newcode700
Line 700: // This message is sent by the spelling panel whenever a word is
ignored
period at end of sentence.

http://codereview.chromium.org/160565/diff/3003/2018
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/160565/diff/3003/2018#newcode904
Line 904: void ResourceMessageFilter::OnUpdateSpellingPanelWithMisspelledWord(
blank line between methods.

http://codereview.chromium.org/160565/diff/3003/2013
File chrome/browser/spellcheck_unittest.cc (right):

http://codereview.chromium.org/160565/diff/3003/2013#newcode868
Line 868: int doc_tag = SpellCheckerPlatform::GetDocumentTag();
you should make sure you comment that the ignores are only valid for |doc_tag|
and not persisted anywhere. It took me a while to figure that out and others may
wonder if this is safe across crashes.

http://codereview.chromium.org/160565/diff/3003/2008
File webkit/glue/editor_client_impl.cc (right):

http://codereview.chromium.org/160565/diff/3003/2008#newcode932
Line 932: std::wstring word = webkit_glue::StringToStdWString(misspelledWord);
C++ code should use C++ style. |misspelled_word|.

http://codereview.chromium.org/160565/diff/3003/2009
File webkit/glue/webview_impl.cc (right):

http://codereview.chromium.org/160565/diff/3003/2009#newcode1977
Line 1977: void  WebViewImpl::SetSpellingPanelVisibility(bool is_visible) {
are there two spaces after |void|? Below too.

[pink (ping after 24hrs), 2009-08-17 15:03:55]

this patch has bitrotted (eg, webkit/glue/webframe.h no longer exists). Can you
please re-upload an updated one? Thanks!

[pink (ping after 24hrs), 2009-09-09 22:21:09]

landed as r25787 and r25794.