Synchronize the check status of the "Check Spelling While Typing" item.

Synchronize the check status of the "Check Spelling While Typing" item.
This change uses the profile value for "browser.enable_spellcheck" to check/uncheck the "Check Spelling While Typing" item not only for saving its status but also for synchronizing its status between a context mnu and a menu bar.

BUG=131712
TEST=SpellCheckerSubMenuObserverTest.ToggleSpelling

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=144114

[Hironori Bono, 2012-06-13 11:09:17]

Greetings Jeremy and Avi,

I would like to use the "browser.enable_spellcheck" to synchronize the check
status on all platforms since I originally added this profile for this purpose.
(Somehow, Chrome does not change this value at all and it is always true.) Would
it be possible to give me your thoughts?

Regards,

Hironori Bono

[jeremy, 2012-06-13 17:43:30]

I only reviewed the Mac side of this change.

http://codereview.chromium.org/10548022/diff/1/chrome/browser/tab_contents/sp...
File chrome/browser/tab_contents/spellchecker_submenu_observer_mac.cc (left):

http://codereview.chromium.org/10548022/diff/1/chrome/browser/tab_contents/sp...
chrome/browser/tab_contents/spellchecker_submenu_observer_mac.cc:89: return
check_spelling_while_typing_;
If we're just reading this straight from the prefs, I'm just guessing we can
remove this member variable from the class now?

http://codereview.chromium.org/10548022/diff/1/chrome/browser/tab_contents/sp...
File chrome/browser/tab_contents/spellchecker_submenu_observer_mac.cc (right):

http://codereview.chromium.org/10548022/diff/1/chrome/browser/tab_contents/sp...
chrome/browser/tab_contents/spellchecker_submenu_observer_mac.cc:87: return
profile->GetPrefs()->GetBoolean(prefs::kEnableSpellCheck);
Are you sure that removing the Null check here is OK?

[Hironori Bono, 2012-06-14 08:16:03]

Greetings Jeremy,

Thanks for your review and comments. I have updated my change to apply your
comments. (I do not feel well today and I'm not totally confident of it,
though.)

Regards,

Hironori Bono

http://codereview.chromium.org/10548022/diff/1/chrome/browser/tab_contents/sp...
File chrome/browser/tab_contents/spellchecker_submenu_observer_mac.cc (left):

http://codereview.chromium.org/10548022/diff/1/chrome/browser/tab_contents/sp...
chrome/browser/tab_contents/spellchecker_submenu_observer_mac.cc:89: return
check_spelling_while_typing_;
Thanks for noticing it. Yes, we can remove it. I have updated my change to
remove unnecessary variables on all platforms.

http://codereview.chromium.org/10548022/diff/1/chrome/browser/tab_contents/sp...
File chrome/browser/tab_contents/spellchecker_submenu_observer_mac.cc (right):

http://codereview.chromium.org/10548022/diff/1/chrome/browser/tab_contents/sp...
chrome/browser/tab_contents/spellchecker_submenu_observer_mac.cc:87: return
profile->GetPrefs()->GetBoolean(prefs::kEnableSpellCheck);
Thanks for noticing it. Even though I somehow removed a NULL check from my
previous change, I tested this code and noticed I needed a NULL check here (at
least on Linux).

[jeremy, 2012-06-14 14:49:50]

Do you know under what case the profile can be null?

http://codereview.chromium.org/10548022/diff/8003/chrome/browser/tab_contents...
File chrome/browser/tab_contents/spellchecker_submenu_observer_hunspell.cc
(right):

http://codereview.chromium.org/10548022/diff/8003/chrome/browser/tab_contents...
chrome/browser/tab_contents/spellchecker_submenu_observer_hunspell.cc:115:
return profile && profile->GetPrefs()->GetBoolean(prefs::kEnableSpellCheck);
nit: can you be more explicite here:

if (!profile)
  return false;
return profile->...

http://codereview.chromium.org/10548022/diff/8003/chrome/browser/tab_contents...
chrome/browser/tab_contents/spellchecker_submenu_observer_hunspell.cc:165:
profile->GetPrefs()->SetBoolean(
Do you need to null-check profile here too?

http://codereview.chromium.org/10548022/diff/8003/chrome/browser/tab_contents...
File chrome/browser/tab_contents/spellchecker_submenu_observer_mac.cc (right):

http://codereview.chromium.org/10548022/diff/8003/chrome/browser/tab_contents...
chrome/browser/tab_contents/spellchecker_submenu_observer_mac.cc:84: return
profile && profile->GetPrefs()->GetBoolean(prefs::kEnableSpellCheck);
same comment about splitting this into an if and early return.

http://codereview.chromium.org/10548022/diff/8003/chrome/browser/tab_contents...
chrome/browser/tab_contents/spellchecker_submenu_observer_mac.cc:115:
profile->GetPrefs()->SetBoolean(
null-check profile?

[Hironori Bono, 2012-06-18 07:23:55]

Greetings Jeremy,

Thanks for your comments.
In brief, |profile| became NULL on the old version of my new browsertest
(SpellCheckerSubMenuObserverTest) and I added NULL checks for this test.
Nevertheless, after reading your comments, I have fixed the test so it returns a
TestingProfile object so SpellCheckerSubMenuObserver does not need NULL checks.
Would it be possible to review the updated one?

Regards,

Hironori Bono

http://codereview.chromium.org/10548022/diff/8003/chrome/browser/tab_contents...
File chrome/browser/tab_contents/spellchecker_submenu_observer_hunspell.cc
(right):

http://codereview.chromium.org/10548022/diff/8003/chrome/browser/tab_contents...
chrome/browser/tab_contents/spellchecker_submenu_observer_hunspell.cc:115:
return profile && profile->GetPrefs()->GetBoolean(prefs::kEnableSpellCheck);
Thanks for your comment. I have added a DCHECK() since I have fixed my test and
this profile does not become NULL any more.

http://codereview.chromium.org/10548022/diff/8003/chrome/browser/tab_contents...
chrome/browser/tab_contents/spellchecker_submenu_observer_hunspell.cc:165:
profile->GetPrefs()->SetBoolean(
Thanks for your comment. Same as above, I have added a DCHECK() at Line 149.

http://codereview.chromium.org/10548022/diff/8003/chrome/browser/tab_contents...
File chrome/browser/tab_contents/spellchecker_submenu_observer_mac.cc (right):

http://codereview.chromium.org/10548022/diff/8003/chrome/browser/tab_contents...
chrome/browser/tab_contents/spellchecker_submenu_observer_mac.cc:84: return
profile && profile->GetPrefs()->GetBoolean(prefs::kEnableSpellCheck);
Thanks for your comment. I have removed this profile and added a DCHECK().

http://codereview.chromium.org/10548022/diff/8003/chrome/browser/tab_contents...
chrome/browser/tab_contents/spellchecker_submenu_observer_mac.cc:115:
profile->GetPrefs()->SetBoolean(
Thanks for your comment. I was too focused on synchronizing the check status and
forgot checking it. I have added a DCHECK(). Also, I have added a NULL check for
|rvh| because my browser tests returns NULL.

[jeremy, 2012-06-19 12:14:43]

http://codereview.chromium.org/10548022/diff/15001/chrome/browser/renderer_ho...
File chrome/browser/renderer_host/chrome_render_widget_host_view_mac_delegate.mm
(right):

http://codereview.chromium.org/10548022/diff/15001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/chrome_render_widget_host_view_mac_delegate.mm:299:
if (profile) {
Is this null check still needed?

http://codereview.chromium.org/10548022/diff/15001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/chrome_render_widget_host_view_mac_delegate.mm:361:
if (profile) {
Same question here

http://codereview.chromium.org/10548022/diff/15001/chrome/browser/tab_content...
File chrome/browser/tab_contents/spellchecker_submenu_observer_browsertest.cc
(right):

http://codereview.chromium.org/10548022/diff/15001/chrome/browser/tab_content...
chrome/browser/tab_contents/spellchecker_submenu_observer_browsertest.cc:29: //
hold menu items added by this test.
I think you can drop the second sentence of this comment.

http://codereview.chromium.org/10548022/diff/15001/chrome/browser/tab_content...
chrome/browser/tab_contents/spellchecker_submenu_observer_browsertest.cc:86: //
test. A test should delete this RenderViewContextMenuObserver object.
How about this for the second sentence:
This is a weak pointer, the test is responsible for deleting the object.

http://codereview.chromium.org/10548022/diff/15001/chrome/browser/tab_content...
chrome/browser/tab_contents/spellchecker_submenu_observer_browsertest.cc:241: }
Can you just declare these inline in the class? Same for the empty methods in
the other classes in this file.

[Hironori Bono, 2012-06-21 09:38:18]

Greetings Jeremy,

Thanks for your comments. I have updated my change to apply them as much as
possible. Would it be possible to take another look?

Regards,

Hironori Bono

http://codereview.chromium.org/10548022/diff/15001/chrome/browser/renderer_ho...
File chrome/browser/renderer_host/chrome_render_widget_host_view_mac_delegate.mm
(right):

http://codereview.chromium.org/10548022/diff/15001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/chrome_render_widget_host_view_mac_delegate.mm:299:
if (profile) {
Thanks for your comment. In my honest opinion, I have not tested this menu-bar
code thoroughly. (I tested only on my Snow-Leopard Mac.) As far as I tested on
my Mac, it is OK to remove this NULL check and I have replaced this check with a
DCHECK().

http://codereview.chromium.org/10548022/diff/15001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/chrome_render_widget_host_view_mac_delegate.mm:361:
if (profile) {
On 2012/06/19 12:14:43, jeremy wrote:
> Same question here

Done.

http://codereview.chromium.org/10548022/diff/15001/chrome/browser/tab_content...
File chrome/browser/tab_contents/spellchecker_submenu_observer_browsertest.cc
(right):

http://codereview.chromium.org/10548022/diff/15001/chrome/browser/tab_content...
chrome/browser/tab_contents/spellchecker_submenu_observer_browsertest.cc:29: //
hold menu items added by this test.
On 2012/06/19 12:14:43, jeremy wrote:
> I think you can drop the second sentence of this comment.

Done. I have also removed code not needed by this test. (This test had some
redundant code due to my copy-and-paste work.)

http://codereview.chromium.org/10548022/diff/15001/chrome/browser/tab_content...
chrome/browser/tab_contents/spellchecker_submenu_observer_browsertest.cc:86: //
test. A test should delete this RenderViewContextMenuObserver object.
On 2012/06/19 12:14:43, jeremy wrote:
> How about this for the second sentence:
> This is a weak pointer, the test is responsible for deleting the object.

Done. Thanks for correcting my comment.

http://codereview.chromium.org/10548022/diff/15001/chrome/browser/tab_content...
chrome/browser/tab_contents/spellchecker_submenu_observer_browsertest.cc:241: }
On 2012/06/19 12:14:43, jeremy wrote:
> Can you just declare these inline in the class? Same for the empty methods in
> the other classes in this file.

Done. Thanks for noticing it.

[jeremy, 2012-06-21 09:48:35]

LGTM

http://codereview.chromium.org/10548022/diff/28001/chrome/browser/tab_content...
File chrome/browser/tab_contents/spellchecker_submenu_observer_browsertest.cc
(right):

http://codereview.chromium.org/10548022/diff/28001/chrome/browser/tab_content...
chrome/browser/tab_contents/spellchecker_submenu_observer_browsertest.cc:44: }
I think that for these empty functions you can just put the {} on the same line.

http://codereview.chromium.org/10548022/diff/28001/chrome/browser/tab_content...
chrome/browser/tab_contents/spellchecker_submenu_observer_browsertest.cc:46: }
here too...

http://codereview.chromium.org/10548022/diff/28001/chrome/browser/tab_content...
chrome/browser/tab_contents/spellchecker_submenu_observer_browsertest.cc:87:
return NULL;
I just thought the code would be more readable if the empty methods were
declared in the class definition, leaving the other methods defined outside
seems fine to me.

No need to change this back though...

http://codereview.chromium.org/10548022/diff/28001/chrome/browser/tab_content...
chrome/browser/tab_contents/spellchecker_submenu_observer_browsertest.cc:134: }
also here.

http://codereview.chromium.org/10548022/diff/28001/chrome/browser/tab_content...
chrome/browser/tab_contents/spellchecker_submenu_observer_browsertest.cc:165: //
false.
nit:
* verify -> verify that
* become -> is now

[Hironori Bono, 2012-06-25 06:54:08]

Greetings Jeremy,

Thanks for your review and comments. I have applied most of them.

Greetings Avi,

Would it be possible to review the changes under tab_contents?

Regards,

Hironori Bono

[Avi (use Gerrit), 2012-06-25 13:07:30]

lgtm

[commit-bot: I haz the power, 2012-06-26 03:51:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hbono@chromium.org/10548022/51001

[commit-bot: I haz the power, 2012-06-26 05:05:18]

Change committed as 144114