Patch 2: Added a preprocessor flag for platform spellcheck.

Added a preprocessor flag for platform spellcheck.

Also replaced some Mac-specific #ifdef's with this flag.

BUG=415302
Committed: https://crrev.com/0a669b8909530cf5e3489dda0bd50e28196fb7bb
Cr-Commit-Position: refs/heads/master@{#337969}

[please use gerrit instead, 2015-07-02 04:27:05]

rouslan@chromium.org changed reviewers:
+ rouslan@chromium.org

[please use gerrit instead, 2015-07-02 04:27:06]

Sorry if this review is premature.

https://codereview.chromium.org/1210943008/diff/20001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/1210943008/diff/20001/build/common.gypi#newco...
build/common.gypi:570: # Signify that the platform has a native spellchecker.
This comment can be confusing, because "native" means "hunspell" here:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/sp...

I suggest this comment instead:

# Use the operating system spellchecker, e.g. on NSSpellChecker on Mac or
SpellCheckerService on Android.

(Is that the correct API name for Android?)

https://codereview.chromium.org/1210943008/diff/20001/build/common.gypi#newco...
build/common.gypi:824: # Android and OSX have native spellcheckers that we can
utilize.
s/native/operating system/

Or anything other term you pick instead of the word "native".

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

https://codereview.chromium.org/1210943008/diff/20001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter.cc:40: #if
!(USE_PLATFORM_SPELLCHECKER)
You accidentally deleted "defined".

https://codereview.chromium.org/1210943008/diff/20001/chrome/renderer/spellch...
File chrome/renderer/spellchecker/spellcheck_provider.h (right):

https://codereview.chromium.org/1210943008/diff/20001/chrome/renderer/spellch...
chrome/renderer/spellchecker/spellcheck_provider.h:112: #endif //
USE_PLATFORM_SPELLCHECKER
Here and everywhere else: Let's try to preserve the existing styling of
#if-#else-#endif, because altering the style is not the purpose of this patch.
For example, please do not add new // after #endif.

[dylanking, 2015-07-06 22:36:31]

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

[dylanking, 2015-07-07 02:31:23]

https://codereview.chromium.org/1210943008/diff/20001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/1210943008/diff/20001/build/common.gypi#newco...
build/common.gypi:570: # Signify that the platform has a native spellchecker.
Fixed in https://codereview.chromium.org/1227553002 so that I could apply the
changes to all the new or already modified code that's not in this CL.

https://codereview.chromium.org/1210943008/diff/20001/build/common.gypi#newco...
build/common.gypi:824: # Android and OSX have native spellcheckers that we can
utilize.
Fixed in https://codereview.chromium.org/1227553002 so that I could apply the
changes to all the new or already modified code that's not in this CL.

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

https://codereview.chromium.org/1210943008/diff/20001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter.cc:40: #if
!(USE_PLATFORM_SPELLCHECKER)
Fixed in https://codereview.chromium.org/1227553002. Hope that's ok.

https://codereview.chromium.org/1210943008/diff/20001/chrome/renderer/spellch...
File chrome/renderer/spellchecker/spellcheck_provider.h (right):

https://codereview.chromium.org/1210943008/diff/20001/chrome/renderer/spellch...
chrome/renderer/spellchecker/spellcheck_provider.h:112: #endif //
USE_PLATFORM_SPELLCHECKER
Fixed in https://codereview.chromium.org/1227553002 so that I could apply the
changes to all the new or already modified code that's not in this CL.

[please use gerrit instead, 2015-07-07 15:54:14]

On 2015/07/07 02:31:23, dylanking wrote:
> Fixed in https://codereview.chromium.org/1227553002 so that I could apply the
> changes to all the new or already modified code that's not in this CL.

This patch cannot be approved until you fix these issue here, though. FYI.

[dylanking, 2015-07-07 19:02:15]

Fixes suggested by rouslan@ implemented.

https://codereview.chromium.org/1210943008/diff/20001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/1210943008/diff/20001/build/common.gypi#newco...
build/common.gypi:570: # Signify that the platform has a native spellchecker.
Now fixed here, too.

https://codereview.chromium.org/1210943008/diff/20001/build/common.gypi#newco...
build/common.gypi:824: # Android and OSX have native spellcheckers that we can
utilize.
Now fixed here, too.

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

https://codereview.chromium.org/1210943008/diff/20001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_message_filter.cc:40: #if
!(USE_PLATFORM_SPELLCHECKER)
Now fixed here, too.

https://codereview.chromium.org/1210943008/diff/20001/chrome/renderer/spellch...
File chrome/renderer/spellchecker/spellcheck_provider.h (right):

https://codereview.chromium.org/1210943008/diff/20001/chrome/renderer/spellch...
chrome/renderer/spellchecker/spellcheck_provider.h:112: #endif //
USE_PLATFORM_SPELLCHECKER
Now fixed here, too.

[please use gerrit instead, 2015-07-07 19:30:28]

spellcheck lgtm

[aurimas (slooooooooow), 2015-07-07 22:24:19]

LGTM.

Do we want to set use_platfrom_spellchecker to true for Android already in
common.gypi? Should we do it at the same time when we set enable_spellcheck to
true?

[dylanking, 2015-07-07 22:33:28]

On 2015/07/07 at 22:24:19, aurimas wrote:
> LGTM.
> 
> Do we want to set use_platfrom_spellchecker to true for Android already in
common.gypi? Should we do it at the same time when we set enable_spellcheck to
true?

After some thought, I think that's a good idea.  The android/platform code that
gets introduced in https://codereview.chromium.org/1204293003 doesn't exist here
yet, so it's probably best to only enable the flag for Android in that patch in
order to keep things copacetic.  The flag will still be turned on for Mac in
this CL.

[dylanking, 2015-07-07 23:06:44]

dylanking@google.com changed reviewers:
+ cjhopman@chromium.org, dcheng@chromium.org, newt@chromium.org

[dylanking, 2015-07-07 23:06:44]

+cjhopman@ for owners review of build/*
+dcheng@ for owners review of chrome/common/spellcheck_messages.h
+newt@ for owners review of chrome/android/*

[newt (away), 2015-07-07 23:12:04]

On 2015/07/07 23:06:44, dylanking wrote:
> +cjhopman@ for owners review of build/*
> +dcheng@ for owners review of chrome/common/spellcheck_messages.h
> +newt@ for owners review of chrome/android/*

chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java lgtm

[dcheng, 2015-07-07 23:13:50]

I guess there will be a followup patch to implement the platform spellchecker
bits on Android? I don't see any config bits that would toggle it on otherwise.

[dylanking, 2015-07-07 23:24:43]

On 2015/07/07 at 23:13:50, dcheng wrote:
> I guess there will be a followup patch to implement the platform spellchecker
bits on Android? I don't see any config bits that would toggle it on otherwise.

Platform (which is meant to include Android) code has been set up here:
https://codereview.chromium.org/1204293003/#ps70001
Which is logically followed by: https://codereview.chromium.org/1227553002

[dcheng, 2015-07-07 23:38:19]

IPC changes lgtm

"Interesting" (non-interesting changes are rename and/or deletion-only changes;
everything else is interesting) changes to the message filter should probably go
through IPC security review too (I didn't see any in particular in the patches
you linked though).

[cjhopman, 2015-07-08 22:29:30]

https://codereview.chromium.org/1210943008/diff/80001/build/config/BUILD.gn
File build/config/BUILD.gn (right):

https://codereview.chromium.org/1210943008/diff/80001/build/config/BUILD.gn#n...
build/config/BUILD.gn:83: if (use_platform_spellchecker) {
is use_platform_spellchecker already declared somewhere?

[dylanking, 2015-07-08 22:37:56]

https://codereview.chromium.org/1210943008/diff/80001/build/config/BUILD.gn
File build/config/BUILD.gn (right):

https://codereview.chromium.org/1210943008/diff/80001/build/config/BUILD.gn#n...
build/config/BUILD.gn:83: if (use_platform_spellchecker) {
I added it to build/common.gypi in this CL, is there somewhere else I need to
register it?

[cjhopman, 2015-07-08 22:43:36]

https://codereview.chromium.org/1210943008/diff/80001/build/config/BUILD.gn
File build/config/BUILD.gn (right):

https://codereview.chromium.org/1210943008/diff/80001/build/config/BUILD.gn#n...
build/config/BUILD.gn:83: if (use_platform_spellchecker) {
On 2015/07/08 22:37:56, dylanking wrote:
> I added it to build/common.gypi in this CL, is there somewhere else I need to
> register it?

Yeah, you need to declare it in a gn file somewhere. Probably should go in
build/config/features.gni.

You have two options:
1. put it in the declare_args block (like safe_browsing_mode). If you do this,
then a user can configure the value directly with build arguments. Also, if a
comment precedes the declaration, that comment will be included in the output of
`gn args --list`.
2. put it outside the declare_args block (like enable_spellcheck). If you do
this, the value can only be changed by editing the file (well, or changing
things that the value depends on).

Do 2 if the value should just depend on other variables (like if it just depends
on the OS). Do 1 if certain platforms/configurations will support the feature
both on and off (this is particularly sometimes the case while changing a
platform from on to/from off).

[dylanking, 2015-07-08 23:30:21]

https://codereview.chromium.org/1210943008/diff/80001/build/config/BUILD.gn
File build/config/BUILD.gn (right):

https://codereview.chromium.org/1210943008/diff/80001/build/config/BUILD.gn#n...
build/config/BUILD.gn:83: if (use_platform_spellchecker) {
Done, hopefully correctly!

[cjhopman, 2015-07-08 23:36:17]

lgtm

[dylanking, 2015-07-08 23:37:04]

The CQ bit was checked by dylanking@google.com

[dylanking, 2015-07-08 23:37:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
aurimas@chromium.org, rouslan@chromium.org, dcheng@chromium.org,
newt@chromium.org
Link to the patchset: https://codereview.chromium.org/1210943008/#ps100001
(title: "Declared the preprocessor flag in build/config/features.gni")

[commit-bot: I haz the power, 2015-07-08 23:37:58]

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

[commit-bot: I haz the power, 2015-07-09 02:20:49]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-07-09 02:21:38]

Patchset 6 (id:??) landed as
https://crrev.com/0a669b8909530cf5e3489dda0bd50e28196fb7bb
Cr-Commit-Position: refs/heads/master@{#337969}