Set the user agent for 0.6

Set the user agent for 0.6

This CL includes
- Move blimp/client/app/user_agent.* to
blimp/client/core/settings/user_agent.*
- Have SettingsFeature::PushSettings() send a message including the generated user agent.

BUG=652032
Committed: https://crrev.com/fb557601c87da27fcb8d2ed40d8f9fb9a5602b81
Cr-Commit-Position: refs/heads/master@{#429335}

[Menglin, 2016-11-01 21:10:15]

mlliu@chromium.org changed reviewers:
+ khushalsagar@chromium.org

[Menglin, 2016-11-01 21:10:16]

Hi Khushal, ptal. Thanks!

Menglin

[Khushal, 2016-11-01 21:28:55]

https://codereview.chromium.org/2473543002/diff/1/blimp/client/core/settings/...
File blimp/client/core/settings/settings_feature.cc (right):

https://codereview.chromium.org/2473543002/diff/1/blimp/client/core/settings/...
blimp/client/core/settings/settings_feature.cc:44: if (settings_) {
Why the if check? Isn't settings_ supposed to outlive this class?

[Menglin, 2016-11-01 21:37:52]

https://codereview.chromium.org/2473543002/diff/1/blimp/client/core/settings/...
File blimp/client/core/settings/settings_feature.cc (right):

https://codereview.chromium.org/2473543002/diff/1/blimp/client/core/settings/...
blimp/client/core/settings/settings_feature.cc:44: if (settings_) {
On 2016/11/01 21:28:55, Khushal wrote:
> Why the if check? Isn't settings_ supposed to outlive this class?

it's for BlimpClientSession. because in BlimpClientSession
https://cs.chromium.org/chromium/src/blimp/client/app/session/blimp_client_se...,
a nullptr is passed as the settings when constructing the settings_feature_ (we
don't care about the settings object there).

so when BlimpClientSessionAndroid calls GetSettingsFeature()->PushSettings(), it
should essentially only sends the client os info.

[Khushal, 2016-11-01 23:33:06]

https://codereview.chromium.org/2473543002/diff/1/blimp/client/core/settings/...
File blimp/client/core/settings/settings_feature.cc (right):

https://codereview.chromium.org/2473543002/diff/1/blimp/client/core/settings/...
blimp/client/core/settings/settings_feature.cc:44: if (settings_) {
On 2016/11/01 21:37:52, Menglin wrote:
> On 2016/11/01 21:28:55, Khushal wrote:
> > Why the if check? Isn't settings_ supposed to outlive this class?
> 
> it's for BlimpClientSession. because in BlimpClientSession
>
https://cs.chromium.org/chromium/src/blimp/client/app/session/blimp_client_se...,
> a nullptr is passed as the settings when constructing the settings_feature_
(we
> don't care about the settings object there).
> 
> so when BlimpClientSessionAndroid calls GetSettingsFeature()->PushSettings(),
it
> should essentially only sends the client os info.

Hmmm, I just looked at that class and it seems like the rest of the settings
don't even work anymore. :/ Like record whole document is no longer read and
set.

Would it be possible to build a Settings object from the app? For the
PrefService, you can do it the same way we do in
BlimpBrowserContext::InitPrefService and use a PrefRegisterySimple.

[Menglin, 2016-11-02 01:14:52]

new patch uploaded. ptal. Thanks!

On 2016/11/01 23:33:06, Khushal wrote:
>
https://codereview.chromium.org/2473543002/diff/1/blimp/client/core/settings/...
> File blimp/client/core/settings/settings_feature.cc (right):
> 
>
https://codereview.chromium.org/2473543002/diff/1/blimp/client/core/settings/...
> blimp/client/core/settings/settings_feature.cc:44: if (settings_) {
> On 2016/11/01 21:37:52, Menglin wrote:
> > On 2016/11/01 21:28:55, Khushal wrote:
> > > Why the if check? Isn't settings_ supposed to outlive this class?
> > 
> > it's for BlimpClientSession. because in BlimpClientSession
> >
>
https://cs.chromium.org/chromium/src/blimp/client/app/session/blimp_client_se...,
> > a nullptr is passed as the settings when constructing the settings_feature_
> (we
> > don't care about the settings object there).
> > 
> > so when BlimpClientSessionAndroid calls
GetSettingsFeature()->PushSettings(),
> it
> > should essentially only sends the client os info.
> 
> Hmmm, I just looked at that class and it seems like the rest of the settings
> don't even work anymore. :/ Like record whole document is no longer read and
> set.
> 
> Would it be possible to build a Settings object from the app? For the
> PrefService, you can do it the same way we do in
> BlimpBrowserContext::InitPrefService and use a PrefRegisterySimple.

as we discussed offline, BlimpClientSession will be gone. Blimp.apk will use
BlimpClientContext instead of BlimpClientSession. So we don't need to care about
the settings that it sends to the engine. And we don't want to add the if check
in SettingsFeature.cc. So we're removing setting feature from BlimpClientSession

[Khushal, 2016-11-02 01:32:49]

lgtm. Thanks.

https://codereview.chromium.org/2473543002/diff/20001/blimp/client/core/setti...
File blimp/client/core/settings/settings_feature.cc (right):

https://codereview.chromium.org/2473543002/diff/20001/blimp/client/core/setti...
blimp/client/core/settings/settings_feature.cc:17:
SettingsFeature::SettingsFeature(Settings* settings) : settings_(settings) {}
DCHECK(settings_) here now? :)

https://codereview.chromium.org/2473543002/diff/20001/blimp/client/core/setti...
File blimp/client/core/settings/user_agent.h (right):

https://codereview.chromium.org/2473543002/diff/20001/blimp/client/core/setti...
blimp/client/core/settings/user_agent.h:10: namespace blimp {
This should be blimp::client namespace.

[Menglin, 2016-11-02 16:34:31]

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

[commit-bot: I haz the power, 2016-11-02 16:34:50]

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

[Menglin, 2016-11-02 16:34:57]

https://codereview.chromium.org/2473543002/diff/20001/blimp/client/core/setti...
File blimp/client/core/settings/settings_feature.cc (right):

https://codereview.chromium.org/2473543002/diff/20001/blimp/client/core/setti...
blimp/client/core/settings/settings_feature.cc:17:
SettingsFeature::SettingsFeature(Settings* settings) : settings_(settings) {}
On 2016/11/02 01:32:49, Khushal wrote:
> DCHECK(settings_) here now? :)

Done.

https://codereview.chromium.org/2473543002/diff/20001/blimp/client/core/setti...
File blimp/client/core/settings/user_agent.h (right):

https://codereview.chromium.org/2473543002/diff/20001/blimp/client/core/setti...
blimp/client/core/settings/user_agent.h:10: namespace blimp {
On 2016/11/02 01:32:49, Khushal wrote:
> This should be blimp::client namespace.

Done.

[commit-bot: I haz the power, 2016-11-02 17:38:39]

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

[commit-bot: I haz the power, 2016-11-02 17:38:40]

Dry run: This issue passed the CQ dry run.

[Menglin, 2016-11-02 17:50:20]

The CQ bit was checked by mlliu@chromium.org

[Menglin, 2016-11-02 17:50:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
khushalsagar@chromium.org
Link to the patchset: https://codereview.chromium.org/2473543002/#ps40001
(title: "nits")

[commit-bot: I haz the power, 2016-11-02 17:51:10]

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, 2016-11-02 17:58:04]

Committed patchset #3 (id:40001)

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

Description was changed from

==========
Set the user agent for 0.6

This CL includes
- Move blimp/client/app/user_agent.* to
blimp/client/core/settings/user_agent.*
- Have SettingsFeature::PushSettings() send a message including the generated
user agent.

BUG=652032
==========

to

==========
Set the user agent for 0.6

This CL includes
- Move blimp/client/app/user_agent.* to
blimp/client/core/settings/user_agent.*
- Have SettingsFeature::PushSettings() send a message including the generated
user agent.

BUG=652032

Committed: https://crrev.com/fb557601c87da27fcb8d2ed40d8f9fb9a5602b81
Cr-Commit-Position: refs/heads/master@{#429335}
==========

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

Patchset 3 (id:??) landed as
https://crrev.com/fb557601c87da27fcb8d2ed40d8f9fb9a5602b81
Cr-Commit-Position: refs/heads/master@{#429335}