Patch 3.1: Refactored spellcheck_message_filter to be generic

Refactored spellcheck_message_filter to be generic.

The header is now meant to be an interface for any
system that would like to use its own spellchecker
instead of the hunspell library.

Split the implementation into two source files, one for mac and one for android.
The android implementation is just method stubs for now.

Updated related build files and #include references.

Part 1 of a 3 part breakdown of https://codereview.chromium.org/1204293003.
The breakdown will also include some changes to the original CL.

BUG=415302
Committed: https://crrev.com/684453c5ecb33796629becb77b64591d939e4458
Cr-Commit-Position: refs/heads/master@{#339195}

[dylanking, 2015-07-09 22:04:10]

dylanking@google.com changed reviewers:
+ aurimas@chromium.org, rouslan@chromium.org

[please use gerrit instead, 2015-07-09 22:21:30]

Good start. You may want to run the patch through the try bots or "CQ dry run"
before sending it out for a review. This will ensure that it compiles and the
tests are passing.

https://codereview.chromium.org/1230933002/diff/20001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/1230933002/diff/20001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:153: #include
"chrome/browser/spellchecker/spellcheck_message_filter_platform.h"
This include should be moved to "#if defined(USE_PLATFORM_SPELLCHECKER)" section
instead.

https://codereview.chromium.org/1230933002/diff/20001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:844: #if defined(OS_MACOSX)
s/OS_MACOSX/ENABLE_PLATFORM_SPELLCHECKER

https://codereview.chromium.org/1230933002/diff/20001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:845: host->AddFilter(new
SpellCheckMessageFilterMac(id));
s/SpellCheckMessageFilterMac/SpellCheckMessageFilterPlatform

https://codereview.chromium.org/1230933002/diff/20001/chrome/browser/spellche...
File chrome/browser/spellchecker/spellcheck_message_filter_mac_browsertest.cc
(right):

https://codereview.chromium.org/1230933002/diff/20001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_mac_browsertest.cc:10:
#include "chrome/browser/spellchecker/spellcheck_message_filter_platform.h"
Please move this #include to the top of the #include section and put an extra
newline after it. Tests usually have #include sections like this:

#include "the/file/under/test.h"

#include "some/dependency.h"
#include "some/other/dependency.h"
#include "yet/another/dependency.h"

https://codereview.chromium.org/1230933002/diff/20001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_mac_browsertest.cc:63: }
This file should be renamed
spellcheck_message_filter_platform_mac_browsertest.cc.

https://codereview.chromium.org/1230933002/diff/20001/chrome/browser/spellche...
File chrome/browser/spellchecker/spellcheck_message_filter_mac_unittest.cc
(right):

https://codereview.chromium.org/1230933002/diff/20001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_mac_unittest.cc:6:
#include "chrome/browser/spellchecker/spellcheck_message_filter_platform.h"
Please put that back in its original position.

https://codereview.chromium.org/1230933002/diff/20001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_mac_unittest.cc:15:
TEST(SpellcheckMessageFilterMacTest, CombineResults) {
The test should be called SpellcheckMessageFilterPlatformMacTest.

https://codereview.chromium.org/1230933002/diff/20001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_mac_unittest.cc:35:
SpellCheckMessageFilterMac::CombineResults(&remote_results, local_results);
SpellCheckMessageFilterMac no longer exists, so this will not compile.

https://codereview.chromium.org/1230933002/diff/20001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_mac_unittest.cc:45:
TEST(SpellCheckMessageFilterMacTest, TestOverrideThread) {
The test should be called SpellcheckMessageFilterPlatformMacTest.

https://codereview.chromium.org/1230933002/diff/20001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_mac_unittest.cc:62: }  //
namespace
Please name this file spellcheck_message_filter_platform_mac_unittest.cc.

https://codereview.chromium.org/1230933002/diff/20001/chrome/browser/spellche...
File chrome/browser/spellchecker/spellcheck_message_filter_platform.h (right):

https://codereview.chromium.org/1230933002/diff/20001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_platform.h:1: // Copyright
2015 The Chromium Authors. All rights reserved.
Let's not update copyright to make it clear that this is a copy of an old file
with some renaming.

https://codereview.chromium.org/1230933002/diff/20001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_platform.h:34: friend
class SpellcheckMessageFilterMacTest;
s/SpellcheckMessageFilterMacTest/SpellcheckMessageFilterPlatformMacTest

https://codereview.chromium.org/1230933002/diff/20001/chrome/browser/spellche...
File chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc
(right):

https://codereview.chromium.org/1230933002/diff/20001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:12:
#include "chrome/browser/spellchecker/spellcheck_message_filter_platform.h"
Please put this #include first in file with an extra newline after it.

https://codereview.chromium.org/1230933002/diff/20001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:32:
class SpellingRequest {
Please remove this mac implementation specific class.

https://codereview.chromium.org/1230933002/diff/20001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:119:
SpellCheckMessageFilterMac::SpellCheckMessageFilterMac(int render_process_id)
Here and everywhere:

s/SpellCheckMessageFilterMac/SpellCheckMessageFilterPlatform

https://codereview.chromium.org/1230933002/diff/20001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:168:
// TODO(groby): We are currently not notified of retired tags. We need
Please remove this TODO.

https://codereview.chromium.org/1230933002/diff/20001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:172: }
Please put NOTREACHED() in all of the methods that you think will not make sense
on Android. For example, I am pretty sure that document tags do not make sense
on Android. Therefore, ToDocumentTag() and RetireDocumentTag() should contain
NOTREACHED().

https://codereview.chromium.org/1230933002/diff/20001/chrome/browser/spellche...
File chrome/browser/spellchecker/spellcheck_message_filter_platform_mac.cc
(right):

https://codereview.chromium.org/1230933002/diff/20001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_platform_mac.cc:12:
#include "chrome/browser/spellchecker/spellcheck_message_filter_platform.h"
Please put this back to the location where it was before.

https://codereview.chromium.org/1230933002/diff/20001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_platform_mac.cc:191:
SpellCheckMessageFilterMac::SpellCheckMessageFilterMac(int render_process_id)
Here and everywhere else:

s/SpellCheckMessageFilterMac/SpellCheckMessageFilterPlatform

[dylanking, 2015-07-09 23:38:01]

The CQ bit was checked by dylanking@google.com

[dylanking, 2015-07-09 23:38:03]

The CQ bit was unchecked by dylanking@google.com

[dylanking, 2015-07-09 23:46:04]

The CQ bit was checked by dylanking@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-07-09 23:48:07]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1230933002/60001

[commit-bot: I haz the power, 2015-07-09 23:48:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-09 23:48:10]

Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are
accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[dylanking, 2015-07-09 23:49:58]

I tried a dry-run, but it was automatically rejected because I'm not a full
committer.

https://codereview.chromium.org/1230933002/diff/20001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/1230933002/diff/20001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:153: #include
"chrome/browser/spellchecker/spellcheck_message_filter_platform.h"
Done, also changed a reference to SpellCheckMessageFilterMac around line 845 in
this file to SpellCheckMessageFilterPlatform.

https://codereview.chromium.org/1230933002/diff/20001/chrome/browser/spellche...
File chrome/browser/spellchecker/spellcheck_message_filter_mac_browsertest.cc
(right):

https://codereview.chromium.org/1230933002/diff/20001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_mac_browsertest.cc:10:
#include "chrome/browser/spellchecker/spellcheck_message_filter_platform.h"
Reverted, though this is giving me presubmit warnings about my #include order
being broken.  I'll ignore them so I can follow the format you linked.

https://codereview.chromium.org/1230933002/diff/20001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_mac_browsertest.cc:63: }
Done including relevant build file changes.

https://codereview.chromium.org/1230933002/diff/20001/chrome/browser/spellche...
File chrome/browser/spellchecker/spellcheck_message_filter_mac_unittest.cc
(right):

https://codereview.chromium.org/1230933002/diff/20001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_mac_unittest.cc:6:
#include "chrome/browser/spellchecker/spellcheck_message_filter_platform.h"
Reverted, though this is giving me presubmit warnings about my #include order
being broken.  I'll ignore them so I can follow the format you linked.

https://codereview.chromium.org/1230933002/diff/20001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_mac_unittest.cc:15:
TEST(SpellcheckMessageFilterMacTest, CombineResults) {
Done.

https://codereview.chromium.org/1230933002/diff/20001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_mac_unittest.cc:35:
SpellCheckMessageFilterMac::CombineResults(&remote_results, local_results);
Fixed.

https://codereview.chromium.org/1230933002/diff/20001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_mac_unittest.cc:45:
TEST(SpellCheckMessageFilterMacTest, TestOverrideThread) {
Done.

https://codereview.chromium.org/1230933002/diff/20001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_mac_unittest.cc:62: }  //
namespace
Done.

https://codereview.chromium.org/1230933002/diff/20001/chrome/browser/spellche...
File chrome/browser/spellchecker/spellcheck_message_filter_platform.h (right):

https://codereview.chromium.org/1230933002/diff/20001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_platform.h:1: // Copyright
2015 The Chromium Authors. All rights reserved.
Reverted.

https://codereview.chromium.org/1230933002/diff/20001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_platform.h:34: friend
class SpellcheckMessageFilterMacTest;
Done.

https://codereview.chromium.org/1230933002/diff/20001/chrome/browser/spellche...
File chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc
(right):

https://codereview.chromium.org/1230933002/diff/20001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:12:
#include "chrome/browser/spellchecker/spellcheck_message_filter_platform.h"
Done.

https://codereview.chromium.org/1230933002/diff/20001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:32:
class SpellingRequest {
Done.

https://codereview.chromium.org/1230933002/diff/20001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:119:
SpellCheckMessageFilterMac::SpellCheckMessageFilterMac(int render_process_id)
Done, quite an oversight by me. Realized this right before reading these
comments. Thanks!

https://codereview.chromium.org/1230933002/diff/20001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:168:
// TODO(groby): We are currently not notified of retired tags. We need
Done.

https://codereview.chromium.org/1230933002/diff/20001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:172: }
Done for these two functions.  I'm not sure if any others are eligible for this,
but I have my suspicions about some such as the SpellingPanel methods. 

As I proceed with implementation of the spellchecker, I'll be sure to mark
relevant methods as NOTREACHED().

https://codereview.chromium.org/1230933002/diff/20001/chrome/browser/spellche...
File chrome/browser/spellchecker/spellcheck_message_filter_platform_mac.cc
(right):

https://codereview.chromium.org/1230933002/diff/20001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_platform_mac.cc:12:
#include "chrome/browser/spellchecker/spellcheck_message_filter_platform.h"
Reverted.

https://codereview.chromium.org/1230933002/diff/20001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_platform_mac.cc:191:
SpellCheckMessageFilterMac::SpellCheckMessageFilterMac(int render_process_id)
Done.

[aurimas (slooooooooow), 2015-07-10 00:08:05]

The CQ bit was checked by aurimas@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-07-10 00:08:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1230933002/60001

[aurimas (slooooooooow), 2015-07-10 00:09:55]

The CQ bit was unchecked by aurimas@chromium.org

[please use gerrit instead, 2015-07-10 00:20:55]

https://codereview.chromium.org/1230933002/diff/60001/chrome/browser/spellche...
File chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc
(right):

https://codereview.chromium.org/1230933002/diff/60001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:7:
#include <algorithm>
Please remove this unused #include.

https://codereview.chromium.org/1230933002/diff/60001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:8:
#include <functional>
Please remove this unused #include.

https://codereview.chromium.org/1230933002/diff/60001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:10:
#include "base/barrier_closure.h"
Please remove this unused #include.

https://codereview.chromium.org/1230933002/diff/60001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:11:
#include "base/bind.h"
Please remove this unused #include.

https://codereview.chromium.org/1230933002/diff/60001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:12:
#include "chrome/browser/spellchecker/feedback_sender.h"
Please remove this unused #include.

https://codereview.chromium.org/1230933002/diff/60001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:13:
#include "chrome/browser/spellchecker/spellcheck_factory.h"
Please remove this unused #include.

https://codereview.chromium.org/1230933002/diff/60001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:14:
#include "chrome/browser/spellchecker/spellcheck_platform_mac.h"
Please remove this unused #include.

https://codereview.chromium.org/1230933002/diff/60001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:15:
#include "chrome/browser/spellchecker/spellcheck_service.h"
Please remove this unused #include.

https://codereview.chromium.org/1230933002/diff/60001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:20:
#include "content/public/browser/render_process_host.h"
Please remove this unused #include.

https://codereview.chromium.org/1230933002/diff/60001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:23:
using content::BrowserContext;
Please remove this unused "using".

https://codereview.chromium.org/1230933002/diff/60001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:27:
bool CompareLocation(const SpellCheckResult& r1,
Please remove this unused function.

https://codereview.chromium.org/1230933002/diff/60001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:37:
client_(new SpellingServiceClient) {
Please ask Aurimas whether Android will support "Ask Google for suggestions"
option in spellchecker. If not, then there is no need to instantiate
SpellingServiceClient. (It's used to communicate to the Google spellchecker in
the cloud.)

https://codereview.chromium.org/1230933002/diff/60001/chrome/browser/spellche...
File chrome/browser/spellchecker/spellcheck_message_filter_platform_mac.cc
(right):

https://codereview.chromium.org/1230933002/diff/60001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_platform_mac.cc:148:
local_results_);
Please align &remote_results_ and local_results_ to start at the same column.
You can do this in a more automated way if your editor has support for
clang-format utility.

Don't clang-format the whole file, though. This will cause many whitespace
changes that slow down the code review process.

Some people have had success using the command "git cl format". I don't use it
personally, because it tends to reformat more than what I've changed.

See https://code.google.com/p/chromium/wiki/ClangFormat

https://codereview.chromium.org/1230933002/diff/60001/chrome/test/BUILD.gn
File chrome/test/BUILD.gn (right):

https://codereview.chromium.org/1230933002/diff/60001/chrome/test/BUILD.gn#ne...
chrome/test/BUILD.gn:908: 
Please do not add this newline.

[please use gerrit instead, 2015-07-10 00:24:05]

https://codereview.chromium.org/1230933002/diff/60001/chrome/browser/spellche...
File chrome/browser/spellchecker/spellcheck_message_filter_mac_browsertest.cc
(left):

https://codereview.chromium.org/1230933002/diff/60001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_mac_browsertest.cc:63: }
Looks like you need to run "git rm
chrome/browser/spellchecker/spellcheck_message_filter_mac_browsertest.cc" to get
rid of this file entirely. The code review website marked it as "M", which means
modified. It should be marked "D" instead.

https://codereview.chromium.org/1230933002/diff/60001/chrome/browser/spellche...
File chrome/browser/spellchecker/spellcheck_message_filter_mac_unittest.cc
(left):

https://codereview.chromium.org/1230933002/diff/60001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_mac_unittest.cc:63: }  //
namespace
Looks like you need to run "git rm
chrome/browser/spellchecker/spellcheck_message_filter_mac_unittest.cc" to get
rid of this file entirely. The code review website marked it as "M", which means
modified. It should be marked "D" instead.

[dylanking, 2015-07-10 02:21:23]

https://codereview.chromium.org/1230933002/diff/60001/chrome/browser/spellche...
File chrome/browser/spellchecker/spellcheck_message_filter_mac_browsertest.cc
(left):

https://codereview.chromium.org/1230933002/diff/60001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_mac_browsertest.cc:63: }
'git rm' does not recognize it as a file anymore:


dylanking@dylanking0:/usr/local/google/code/clankium/src$ git rm
chrome/browser/spellchecker/spellcheck_message_filter_mac_browsertest.cc
fatal: pathspec
'chrome/browser/spellchecker/spellcheck_message_filter_mac_browsertest.cc' did
not match any files

https://codereview.chromium.org/1230933002/diff/60001/chrome/browser/spellche...
File chrome/browser/spellchecker/spellcheck_message_filter_mac_unittest.cc
(left):

https://codereview.chromium.org/1230933002/diff/60001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_mac_unittest.cc:63: }  //
namespace
'git rm' does not recognize it as a file anymore:


dylanking@dylanking0:/usr/local/google/code/clankium/src$ git rm
chrome/browser/spellchecker/spellcheck_message_filter_mac_unittest.cc
fatal: pathspec
'chrome/browser/spellchecker/spellcheck_message_filter_mac_unittest.cc' did not
match any files

https://codereview.chromium.org/1230933002/diff/60001/chrome/browser/spellche...
File chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc
(right):

https://codereview.chromium.org/1230933002/diff/60001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:7:
#include <algorithm>
Done.

https://codereview.chromium.org/1230933002/diff/60001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:8:
#include <functional>
Done.

https://codereview.chromium.org/1230933002/diff/60001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:10:
#include "base/barrier_closure.h"
Done.

https://codereview.chromium.org/1230933002/diff/60001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:11:
#include "base/bind.h"
Done.

https://codereview.chromium.org/1230933002/diff/60001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:12:
#include "chrome/browser/spellchecker/feedback_sender.h"
Done.

https://codereview.chromium.org/1230933002/diff/60001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:13:
#include "chrome/browser/spellchecker/spellcheck_factory.h"
Done.

https://codereview.chromium.org/1230933002/diff/60001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:14:
#include "chrome/browser/spellchecker/spellcheck_platform_mac.h"
Done.

https://codereview.chromium.org/1230933002/diff/60001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:15:
#include "chrome/browser/spellchecker/spellcheck_service.h"
Done.

https://codereview.chromium.org/1230933002/diff/60001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:20:
#include "content/public/browser/render_process_host.h"
Done.

https://codereview.chromium.org/1230933002/diff/60001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:23:
using content::BrowserContext;
Done.

https://codereview.chromium.org/1230933002/diff/60001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:27:
bool CompareLocation(const SpellCheckResult& r1,
Done.

https://codereview.chromium.org/1230933002/diff/60001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:37:
client_(new SpellingServiceClient) {
This is a stretch goal, so for now we'll leave client_ null.

https://codereview.chromium.org/1230933002/diff/60001/chrome/browser/spellche...
File chrome/browser/spellchecker/spellcheck_message_filter_platform_mac.cc
(right):

https://codereview.chromium.org/1230933002/diff/60001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter_platform_mac.cc:148:
local_results_);
Done, thanks for the info.

https://codereview.chromium.org/1230933002/diff/60001/chrome/test/BUILD.gn
File chrome/test/BUILD.gn (right):

https://codereview.chromium.org/1230933002/diff/60001/chrome/test/BUILD.gn#ne...
chrome/test/BUILD.gn:908: 
This was added by a formatting tool that presubmit told me to run on BUILD.gn.

[please use gerrit instead, 2015-07-10 18:35:43]

Almost finished.

https://codereview.chromium.org/1230933002/diff/100001/chrome/browser/spellch...
File chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc
(right):

https://codereview.chromium.org/1230933002/diff/100001/chrome/browser/spellch...
chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:7:
#include "chrome/browser/spellchecker/spelling_service_client.h"
This include is no longer necessary, because you decided to not create a
SpellingServiceClient.

https://codereview.chromium.org/1230933002/diff/100001/chrome/browser/spellch...
chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:10:
#include "content/public/browser/browser_context.h"
Based on the code below, I think you should be including browser_thread.h
instead of browser_context.h.

https://codereview.chromium.org/1230933002/diff/100001/chrome/browser/spellch...
chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:26:
return true; // to pass compiler checks
No need for comment.

https://codereview.chromium.org/1230933002/diff/100001/chrome/browser/spellch...
chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:64:
return -1; // to pass compiler checks
No need for comment.

https://codereview.chromium.org/1230933002/diff/100001/chrome/common/BUILD.gn
File chrome/common/BUILD.gn (left):

https://codereview.chromium.org/1230933002/diff/100001/chrome/common/BUILD.gn...
chrome/common/BUILD.gn:194: "spellcheck_common.cc",
I did not find where this is used on Android in this patch. Please point it out,
if it's used. 

Otherwise, let's not add this file to Android build until it's needed.

[dylanking, 2015-07-11 01:46:24]

https://codereview.chromium.org/1230933002/diff/100001/chrome/browser/spellch...
File chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc
(right):

https://codereview.chromium.org/1230933002/diff/100001/chrome/browser/spellch...
chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:7:
#include "chrome/browser/spellchecker/spelling_service_client.h"
Done.

https://codereview.chromium.org/1230933002/diff/100001/chrome/browser/spellch...
chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:10:
#include "content/public/browser/browser_context.h"
Done.

https://codereview.chromium.org/1230933002/diff/100001/chrome/browser/spellch...
chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:26:
return true; // to pass compiler checks
Deleted.

https://codereview.chromium.org/1230933002/diff/100001/chrome/browser/spellch...
chrome/browser/spellchecker/spellcheck_message_filter_platform_android.cc:64:
return -1; // to pass compiler checks
Deleted.

https://codereview.chromium.org/1230933002/diff/100001/chrome/common/BUILD.gn
File chrome/common/BUILD.gn (left):

https://codereview.chromium.org/1230933002/diff/100001/chrome/common/BUILD.gn...
chrome/common/BUILD.gn:194: "spellcheck_common.cc",
I was wondering why I took this out in the first place as well, until I ran some
trybots that failed to link:

FAILED: ........
../../chrome/browser/spellchecker/spellcheck_hunspell_dictionary.cc:301: error:
undefined reference to
'chrome::spellcheck_common::GetVersionedFileName(std::__1::basic_string<char,
std::__1::char_traits<char>, std::__1::allocator<char> > const&, base::FilePath
const&)'
../../chrome/browser/spellchecker/spellcheck_service.cc:305: error: undefined
reference to
'chrome::spellcheck_common::GetISOLanguageCountryCodeFromLocale(std::__1::basic_string<char,
std::__1::char_traits<char>, std::__1::allocator<char> > const&,
std::__1::basic_string<char, std::__1::char_traits<char>,
std::__1::allocator<char> >*, std::__1::basic_string<char,
std::__1::char_traits<char>, std::__1::allocator<char> >*)'
../../chrome/browser/spellchecker/spellcheck_service.cc:71: error: undefined
reference to
'chrome::spellcheck_common::GetISOLanguageCountryCodeFromLocale(std::__1::basic_string<char,
std::__1::char_traits<char>, std::__1::allocator<char> > const&,
std::__1::basic_string<char, std::__1::char_traits<char>,
std::__1::allocator<char> >*, std::__1::basic_string<char,
std::__1::char_traits<char>, std::__1::allocator<char> >*)'
../../chrome/browser/spellchecker/spellcheck_service.cc:83: error: undefined
reference to 'chrome::spellcheck_common::IsMultilingualSpellcheckEnabled()'
../../chrome/browser/spellchecker/spelling_service_client.cc:125: error:
undefined reference to
'chrome::spellcheck_common::IsMultilingualSpellcheckEnabled()'
collect2: error: ld returned 1 exit status


FAILED: ........
../../chrome/browser/spellchecker/spellcheck_hunspell_dictionary.cc:301: error:
undefined reference to
'chrome::spellcheck_common::GetVersionedFileName(std::__1::basic_string<char,
std::__1::char_traits<char>, std::__1::allocator<char> > const&, base::FilePath
const&)'
../../chrome/browser/spellchecker/spellcheck_service.cc:305: error: undefined
reference to
'chrome::spellcheck_common::GetISOLanguageCountryCodeFromLocale(std::__1::basic_string<char,
std::__1::char_traits<char>, std::__1::allocator<char> > const&,
std::__1::basic_string<char, std::__1::char_traits<char>,
std::__1::allocator<char> >*, std::__1::basic_string<char,
std::__1::char_traits<char>, std::__1::allocator<char> >*)'
../../chrome/browser/spellchecker/spellcheck_service.cc:71: error: undefined
reference to
'chrome::spellcheck_common::GetISOLanguageCountryCodeFromLocale(std::__1::basic_string<char,
std::__1::char_traits<char>, std::__1::allocator<char> > const&,
std::__1::basic_string<char, std::__1::char_traits<char>,
std::__1::allocator<char> >*, std::__1::basic_string<char,
std::__1::char_traits<char>, std::__1::allocator<char> >*)'
../../chrome/browser/spellchecker/spellcheck_service.cc:83: error: undefined
reference to 'chrome::spellcheck_common::IsMultilingualSpellcheckEnabled()'
../../chrome/browser/spellchecker/spelling_service_client.cc:125: error:
undefined reference to
'chrome::spellcheck_common::IsMultilingualSpellcheckEnabled()'
collect2: error: ld returned 1 exit status
ninja: build stopped: subcommand failed.


I think we should keep spellcheck_common.cc in the build due to this.

[please use gerrit instead, 2015-07-13 20:21:51]

Looking good overall. Please let me know when you fix the failing trybots for
the last review pass.

[dylanking, 2015-07-14 20:01:01]

Took longer than expected, but it looks like all non-broken tests are (finally)
passing.

[please use gerrit instead, 2015-07-14 21:40:30]

LGTM

Note that you need OWNER reviews for the following files:
 - build/common.gypi
 - build/config/features.gni
 - chrome/common/BUILD.gn
 - chrome/test/BUILD.gn
 - tools/metrics/histograms/histograms.xml

[please use gerrit instead, 2015-07-14 21:41:02]

Oh, and fix one nit in spellcheck_message_filter_platform_mac_browsertest.cc
please.

https://codereview.chromium.org/1230933002/diff/290001/chrome/browser/spellch...
File
chrome/browser/spellchecker/spellcheck_message_filter_platform_mac_browsertest.cc
(right):

https://codereview.chromium.org/1230933002/diff/290001/chrome/browser/spellch...
chrome/browser/spellchecker/spellcheck_message_filter_platform_mac_browsertest.cc:38:
typedef InProcessBrowserTest SpellCheckMessageFilterPlatformBrowserTest;
Add "Mac" in the name.

SpellCheckMessageFilterPlatformMacBrowserTest

(Sorry about such long names..)

https://codereview.chromium.org/1230933002/diff/290001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1230933002/diff/290001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:60813: +  <int value="1862207743"
label="enable-android-spellchecker"/>
I am surprised that you were able to land "enable-android-spellchecker" switch
in chrome/common/chrome_switches.cc without the histogram. Oh well.

[dylanking, 2015-07-15 18:18:38]

dylanking@google.com changed reviewers:
+ isherman@chromium.org, thakis@chromium.org

[dylanking, 2015-07-15 18:18:38]

The CQ bit was checked by dylanking@google.com

[dylanking, 2015-07-15 18:18:39]

+thakis@ for owners review of:
build/common.gypi
build/config/features.gni
chrome/common/BUILD.gn
chrome/test/BUILD.gn

+isherman@ for owners review of:
tools/metrics/histograms/histograms.xml

[dylanking, 2015-07-15 18:18:39]

The patchset sent to the CQ was uploaded after l-g-t-m from rouslan@chromium.org
Link to the patchset: https://codereview.chromium.org/1230933002/#ps310001
(title: "Small name fix")

[commit-bot: I haz the power, 2015-07-15 18:19:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1230933002/310001

[dylanking, 2015-07-15 18:19:21]

The CQ bit was unchecked by dylanking@google.com

[dylanking, 2015-07-15 18:19:24]

The CQ bit was checked by dylanking@google.com

[dylanking, 2015-07-15 18:19:26]

The CQ bit was unchecked by dylanking@google.com

[dylanking, 2015-07-15 18:19:34]

The CQ bit was checked by dylanking@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-07-15 18:21:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1230933002/310001

[commit-bot: I haz the power, 2015-07-15 18:38:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-15 18:38:07]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[dylanking, 2015-07-15 18:47:17]

The CQ bit was checked by dylanking@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-07-15 18:47:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1230933002/140017

[commit-bot: I haz the power, 2015-07-15 22:16:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-15 22:16:42]

Dry run: This issue passed the CQ dry run.

[Ilya Sherman, 2015-07-16 01:41:25]

LGTM

[Nico, 2015-07-16 20:36:51]

My files lgtm

https://codereview.chromium.org/1230933002/diff/140017/build/config/features.gni
File build/config/features.gni (right):

https://codereview.chromium.org/1230933002/diff/140017/build/config/features....
build/config/features.gni:126: enable_spellcheck = true
If this is true unconditionally, can we remove this variable?

[aurimas (slooooooooow), 2015-07-16 20:37:40]

https://codereview.chromium.org/1230933002/diff/140017/build/config/features.gni
File build/config/features.gni (right):

https://codereview.chromium.org/1230933002/diff/140017/build/config/features....
build/config/features.gni:126: enable_spellcheck = true
On 2015/07/16 at 20:36:51, Nico wrote:
> If this is true unconditionally, can we remove this variable?

iOS does not use it.

[dylanking, 2015-07-16 20:46:50]

The CQ bit was checked by dylanking@google.com

[dylanking, 2015-07-16 20:46:51]

The patchset sent to the CQ was uploaded after l-g-t-m from rouslan@chromium.org
Link to the patchset: https://codereview.chromium.org/1230933002/#ps140017
(title: "Small fix to changed symbol")

[commit-bot: I haz the power, 2015-07-16 20:50:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1230933002/140017

[Nico, 2015-07-16 20:56:42]

https://codereview.chromium.org/1230933002/diff/140017/build/config/features.gni
File build/config/features.gni (right):

https://codereview.chromium.org/1230933002/diff/140017/build/config/features....
build/config/features.gni:126: enable_spellcheck = true
On 2015/07/16 20:37:40, aurimas wrote:
> On 2015/07/16 at 20:36:51, Nico wrote:
> > If this is true unconditionally, can we remove this variable?
> 
> iOS does not use it.

Can you change this to !is_ios then? The iOS gn build isn't functional yet, but
that variable exists, and then nobody will simplify this as I suggested. Right
now, I'd do this simplification if I saw the line that's here checked in if I
didn't know about your comment on this review

[Nico, 2015-07-16 20:56:58]

The CQ bit was unchecked by thakis@chromium.org

[dylanking, 2015-07-16 22:13:51]

> Can you change this to !is_ios then? The iOS gn build isn't functional yet,
but that variable exists, and then nobody will simplify this as I suggested.
Right now, I'd do this simplification if I saw the line that's here checked in
if I didn't know about your comment on this review

Done, good catch.  Thanks!

[dylanking, 2015-07-16 22:25:43]

The CQ bit was checked by dylanking@google.com

[dylanking, 2015-07-16 22:25:44]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org,
isherman@chromium.org, rouslan@chromium.org
Link to the patchset: https://codereview.chromium.org/1230933002/#ps340001
(title: "Updated gn build variable to match gyp side")

[commit-bot: I haz the power, 2015-07-16 22:27:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1230933002/340001

[commit-bot: I haz the power, 2015-07-17 01:22:30]

Committed patchset #19 (id:340001)

[commit-bot: I haz the power, 2015-07-17 01:24:41]

Patchset 19 (id:??) landed as
https://crrev.com/684453c5ecb33796629becb77b64591d939e4458
Cr-Commit-Position: refs/heads/master@{#339195}