Add a FakeImeFeatureDelegate to the Linux client.

Add a FakeImeFeatureDelegate to the Linux client.

The BlimpClientSession creates an ImeFeature which requires that a
Delegate be provided before the first IME message is received. Since the
Linux client is for developer testing only, we add a fake delegate which
just logs the IME show/hide requests.

This CL also corrects some BlimpMessageProcessor::ProcessMessage()
overrides to be public.

BUG=615584
Committed: https://crrev.com/4b1943d8eace94680d2fced00f872695801e192a
Cr-Commit-Position: refs/heads/master@{#397478}

[Wez, 2016-05-28 02:39:12]

wez@chromium.org changed reviewers:
+ xyzzyz@chromium.org

[Wez, 2016-05-28 02:39:12]

PTAL

[Wez, 2016-05-28 02:40:44]

PTAL

[xyzzyz, 2016-05-31 17:11:08]

https://codereview.chromium.org/2023613002/diff/1/blimp/client/app/linux/blim...
File blimp/client/app/linux/blimp_client_session_linux.cc (right):

https://codereview.chromium.org/2023613002/diff/1/blimp/client/app/linux/blim...
blimp/client/app/linux/blimp_client_session_linux.cc:21: const char
kDefaultAssignerUrl[] =
There's a script that catches these:
https://code.google.com/p/chromium/codesearch#chromium/src/tools/linux/dump-s...

Maybe we should incorporate it in our buildbot.

https://codereview.chromium.org/2023613002/diff/1/blimp/client/app/linux/blim...
blimp/client/app/linux/blimp_client_session_linux.cc:109:
GetImeFeature()->set_delegate(ime_feature_delegate_.get());
The above SetDelegate is CamelCase. We should be consistent.

[xyzzyz, 2016-05-31 18:41:50]

Also, please either update ImeFeature so that the comment at [1] is correct, or
make it impossible to create ImeFeature without a non-null delegate. 

[1] -
https://code.google.com/p/chromium/codesearch#chromium/src/blimp/client/featu...

[Wez, 2016-06-02 17:26:35]

On 2016/05/31 18:41:50, xyzzyz wrote:
> Also, please either update ImeFeature so that the comment at [1] is correct,
or
> make it impossible to create ImeFeature without a non-null delegate. 
> 
> [1] -
>
https://code.google.com/p/chromium/codesearch#chromium/src/blimp/client/featu...

Oh, good catch! Have replaced that comment; the case of delegate_ being null is
a coding error so I don't think the commented behaviour was appropriate.

The class is constructed by the generic BlimpClientSession Core, while the
Delegate is supplied by the platform-specific derived class, so supplying it at
construction time would introduce false dependencies in BlimpClientSession.

[Wez, 2016-06-02 17:37:32]

Description was changed from

==========
Add a FakeImeFeatureDelegate to the Linux client.

The BlimpClientSession creates an ImeFeature which requires that a
Delegate be provided before the first IME message is received. Since the
Linux client is for developer testing only, we add a fake delegate which
just logs the IME show/hide requests.

BUG=615584
==========

to

==========
Add a FakeImeFeatureDelegate to the Linux client.

The BlimpClientSession creates an ImeFeature which requires that a
Delegate be provided before the first IME message is received. Since the
Linux client is for developer testing only, we add a fake delegate which
just logs the IME show/hide requests.

This CL also corrects some BlimpMessageProcessor::ProcessMessage()
overrides to be public.

BUG=615584
==========

[Wez, 2016-06-02 17:38:33]

PTAL

(Incidentally noticed that each Feature is-a BlimpMessageProcessor (it's not
just an implementation-internal inheritance) so made the ProcessMessage
overrides public.)

https://codereview.chromium.org/2023613002/diff/1/blimp/client/app/linux/blim...
File blimp/client/app/linux/blimp_client_session_linux.cc (right):

https://codereview.chromium.org/2023613002/diff/1/blimp/client/app/linux/blim...
blimp/client/app/linux/blimp_client_session_linux.cc:21: const char
kDefaultAssignerUrl[] =
On 2016/05/31 17:11:08, xyzzyz wrote:
> There's a script that catches these:
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/linux/dump-s...
> 
> Maybe we should incorporate it in our buildbot.

Acknowledged.

https://codereview.chromium.org/2023613002/diff/1/blimp/client/app/linux/blim...
blimp/client/app/linux/blimp_client_session_linux.cc:109:
GetImeFeature()->set_delegate(ime_feature_delegate_.get());
On 2016/05/31 17:11:08, xyzzyz wrote:
> The above SetDelegate is CamelCase. We should be consistent.

Yeah, that bothered me as well.

The set_delegate() is a simple-accessor, declared inline, so Chromium
style-guide says it should be named hacker_style.

The SetDelegate() call is not a simple accessor, so can't be inline, and it's
questionable whether it fits the "cheapness" criterion required for
hacker_style.

[xyzzyz, 2016-06-02 17:53:34]

LGTM

[xyzzyz, 2016-06-02 17:53:34]

LGTM

[Wez, 2016-06-02 18:17:21]

The CQ bit was checked by wez@chromium.org

[commit-bot: I haz the power, 2016-06-02 18:18:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2023613002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2023613002/20001

[commit-bot: I haz the power, 2016-06-02 18:34:10]

Description was changed from

==========
Add a FakeImeFeatureDelegate to the Linux client.

The BlimpClientSession creates an ImeFeature which requires that a
Delegate be provided before the first IME message is received. Since the
Linux client is for developer testing only, we add a fake delegate which
just logs the IME show/hide requests.

This CL also corrects some BlimpMessageProcessor::ProcessMessage()
overrides to be public.

BUG=615584
==========

to

==========
Add a FakeImeFeatureDelegate to the Linux client.

The BlimpClientSession creates an ImeFeature which requires that a
Delegate be provided before the first IME message is received. Since the
Linux client is for developer testing only, we add a fake delegate which
just logs the IME show/hide requests.

This CL also corrects some BlimpMessageProcessor::ProcessMessage()
overrides to be public.

BUG=615584
==========

[commit-bot: I haz the power, 2016-06-02 18:34:11]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-06-02 18:35:32]

Description was changed from

==========
Add a FakeImeFeatureDelegate to the Linux client.

The BlimpClientSession creates an ImeFeature which requires that a
Delegate be provided before the first IME message is received. Since the
Linux client is for developer testing only, we add a fake delegate which
just logs the IME show/hide requests.

This CL also corrects some BlimpMessageProcessor::ProcessMessage()
overrides to be public.

BUG=615584
==========

to

==========
Add a FakeImeFeatureDelegate to the Linux client.

The BlimpClientSession creates an ImeFeature which requires that a
Delegate be provided before the first IME message is received. Since the
Linux client is for developer testing only, we add a fake delegate which
just logs the IME show/hide requests.

This CL also corrects some BlimpMessageProcessor::ProcessMessage()
overrides to be public.

BUG=615584

Committed: https://crrev.com/4b1943d8eace94680d2fced00f872695801e192a
Cr-Commit-Position: refs/heads/master@{#397478}
==========

[commit-bot: I haz the power, 2016-06-02 18:35:33]

Patchset 2 (id:??) landed as
https://crrev.com/4b1943d8eace94680d2fced00f872695801e192a
Cr-Commit-Position: refs/heads/master@{#397478}