Make SpellCheckPanel compiled and used only on Mac

Make SpellCheckPanel compiled and used only on Mac

Only Mac has a spellcheck panel. Hence, this patch makes class
SpellCheckPanel and relevant messages compiled and used only on Mac.

BUG=711509
Review-Url: https://codereview.chromium.org/2818043002
Cr-Commit-Position: refs/heads/master@{#464774}
Committed: https://chromium.googlesource.com/chromium/src/+/c6cdea59ad514b6cc875dc65ad3083e633f3df91

[Xiaocheng, 2017-04-14 00:11:59]

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

[commit-bot: I haz the power, 2017-04-14 00:12:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Xiaocheng, 2017-04-14 00:49:42]

xiaochengh@chromium.org changed reviewers:
+ rouslan@chromium.org, thestig@chromium.org

[Xiaocheng, 2017-04-14 00:49:43]

PTAL.

[Lei Zhang, 2017-04-14 00:55:38]

https://codereview.chromium.org/2818043002/diff/1/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/2818043002/diff/1/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:171: #endif
Which #endif is which? Once you have nested #ifs, I would recommend adding
comments to them.

https://codereview.chromium.org/2818043002/diff/1/components/spellcheck/rende...
File components/spellcheck/renderer/BUILD.gn (right):

https://codereview.chromium.org/2818043002/diff/1/components/spellcheck/rende...
components/spellcheck/renderer/BUILD.gn:28: if (!use_browser_spellchecker) {
You may want to negate all of these and change to the model where you add
sources. That way you won't have to list files twice.

[Xiaocheng, 2017-04-14 01:19:43]

Patchset #3 (id:40001) has been deleted

[Xiaocheng, 2017-04-14 01:20:19]

Thanks for reviewing.

https://codereview.chromium.org/2818043002/diff/1/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/2818043002/diff/1/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:171: #endif
On 2017/04/14 at 00:55:38, Lei Zhang wrote:
> Which #endif is which? Once you have nested #ifs, I would recommend adding
comments to them.

Done.

https://codereview.chromium.org/2818043002/diff/1/components/spellcheck/rende...
File components/spellcheck/renderer/BUILD.gn (right):

https://codereview.chromium.org/2818043002/diff/1/components/spellcheck/rende...
components/spellcheck/renderer/BUILD.gn:28: if (!use_browser_spellchecker) {
On 2017/04/14 at 00:55:38, Lei Zhang wrote:
> You may want to negate all of these and change to the model where you add
sources. That way you won't have to list files twice.

Sure. I'll do that in another patch.

This patch focuses on limiting the compiling of spellcheck-panel-related stuff.

[Lei Zhang, 2017-04-14 01:24:39]

chrome/ lgtm

https://codereview.chromium.org/2818043002/diff/20001/components/spellcheck/r...
File components/spellcheck/renderer/BUILD.gn (right):

https://codereview.chromium.org/2818043002/diff/20001/components/spellcheck/r...
components/spellcheck/renderer/BUILD.gn:28: #TODO(xiaochengh): Negate these
checks so that each file is listed only once.
nit: space after '#'

[Xiaocheng, 2017-04-14 01:29:59]

https://codereview.chromium.org/2818043002/diff/20001/components/spellcheck/r...
File components/spellcheck/renderer/BUILD.gn (right):

https://codereview.chromium.org/2818043002/diff/20001/components/spellcheck/r...
components/spellcheck/renderer/BUILD.gn:28: #TODO(xiaochengh): Negate these
checks so that each file is listed only once.
On 2017/04/14 at 01:24:39, Lei Zhang wrote:
> nit: space after '#'

Whoops. Done.

[please use gerrit instead, 2017-04-14 14:19:58]

LGTM!

https://codereview.chromium.org/2818043002/diff/60001/components/spellcheck/c...
File components/spellcheck/common/spellcheck_messages.h (right):

https://codereview.chromium.org/2818043002/diff/60001/components/spellcheck/c...
components/spellcheck/common/spellcheck_messages.h:133: #endif
Since you're moving these around, would you mind grouping all
HAS_SPELLCHECK_PANEL messages together?

[Xiaocheng, 2017-04-14 17:27:35]

Thanks for reviewing!

https://codereview.chromium.org/2818043002/diff/60001/components/spellcheck/c...
File components/spellcheck/common/spellcheck_messages.h (right):

https://codereview.chromium.org/2818043002/diff/60001/components/spellcheck/c...
components/spellcheck/common/spellcheck_messages.h:133: #endif
On 2017/04/14 at 14:19:58, ಠ_ಠ wrote:
> Since you're moving these around, would you mind grouping all
HAS_SPELLCHECK_PANEL messages together?

The messages are grouped as "from renderer to browser" and "from browser to
renderer".

So I guess the spellcheck panel messages should also be grouped like that.

[Xiaocheng, 2017-04-14 17:27:38]

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

[commit-bot: I haz the power, 2017-04-14 17:27:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[please use gerrit instead, 2017-04-14 17:30:14]

On 2017/04/14 17:27:35, Xiaocheng wrote:
> Thanks for reviewing!
> 
>
https://codereview.chromium.org/2818043002/diff/60001/components/spellcheck/c...
> File components/spellcheck/common/spellcheck_messages.h (right):
> 
>
https://codereview.chromium.org/2818043002/diff/60001/components/spellcheck/c...
> components/spellcheck/common/spellcheck_messages.h:133: #endif
> On 2017/04/14 at 14:19:58, ಠ_ಠ wrote:
> > Since you're moving these around, would you mind grouping all
> HAS_SPELLCHECK_PANEL messages together?
> 
> The messages are grouped as "from renderer to browser" and "from browser to
> renderer".
> 
> So I guess the spellcheck panel messages should also be grouped like that.

As long as there's good logic behind it, I'm OK with what you choose.

[Xiaocheng, 2017-04-14 17:44:29]

On 2017/04/14 at 17:30:14, rouslan wrote:
> On 2017/04/14 17:27:35, Xiaocheng wrote:
> > Thanks for reviewing!
> > 
> >
https://codereview.chromium.org/2818043002/diff/60001/components/spellcheck/c...
> > File components/spellcheck/common/spellcheck_messages.h (right):
> > 
> >
https://codereview.chromium.org/2818043002/diff/60001/components/spellcheck/c...
> > components/spellcheck/common/spellcheck_messages.h:133: #endif
> > On 2017/04/14 at 14:19:58, ಠ_ಠ wrote:
> > > Since you're moving these around, would you mind grouping all
> > HAS_SPELLCHECK_PANEL messages together?
> > 
> > The messages are grouped as "from renderer to browser" and "from browser to
> > renderer".
> > 
> > So I guess the spellcheck panel messages should also be grouped like that.
> 
> As long as there's good logic behind it, I'm OK with what you choose.

Thanks!

[commit-bot: I haz the power, 2017-04-14 18:31:03]

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

[commit-bot: I haz the power, 2017-04-14 18:31:04]

Dry run: This issue passed the CQ dry run.

[Xiaocheng, 2017-04-14 18:41:54]

The CQ bit was checked by xiaochengh@chromium.org

[Xiaocheng, 2017-04-14 18:41:55]

The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/2818043002/#ps60001
(title: "Thu Apr 13 18:29:22 PDT 2017")

[commit-bot: I haz the power, 2017-04-14 18:42:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-14 18:49:13]

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

[commit-bot: I haz the power, 2017-04-14 18:49:15]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Xiaocheng, 2017-04-14 18:52:46]

xiaochengh@chromium.org changed reviewers:
+ dcheng@chromium.org

[Xiaocheng, 2017-04-14 18:52:46]

+dcheng for spellcheck_messages.h

[dcheng, 2017-04-14 19:03:15]

rs lgtm

[Xiaocheng, 2017-04-14 19:14:47]

The CQ bit was checked by xiaochengh@chromium.org

[Xiaocheng, 2017-04-14 19:14:47]

Thanks!

[commit-bot: I haz the power, 2017-04-14 19:15:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-14 19:26:45]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1492197287439190,
"parent_rev": "77227cc6631117209eb6cfbbc5c78bf0d3c8a4b4", "commit_rev":
"c6cdea59ad514b6cc875dc65ad3083e633f3df91"}

[commit-bot: I haz the power, 2017-04-14 19:27:39]

Description was changed from

==========
Make SpellCheckPanel compiled and used only on Mac

Only Mac has a spellcheck panel. Hence, this patch makes class
SpellCheckPanel and relevant messages compiled and used only on Mac.

BUG=711509
==========

to

==========
Make SpellCheckPanel compiled and used only on Mac

Only Mac has a spellcheck panel. Hence, this patch makes class
SpellCheckPanel and relevant messages compiled and used only on Mac.

BUG=711509

Review-Url: https://codereview.chromium.org/2818043002
Cr-Commit-Position: refs/heads/master@{#464774}
Committed:
https://chromium.googlesource.com/chromium/src/+/c6cdea59ad514b6cc875dc65ad30...
==========

[commit-bot: I haz the power, 2017-04-14 19:27:40]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/c6cdea59ad514b6cc875dc65ad30...