Added KioskModeAppPack proto.

Added KioskModeAppPack proto.

Also introduced files for the AppPackUpdater class, which currently only holds a couple of constants.

BUG=chromium-os:25463
TEST=Nothing breaks


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=123261

[Joao da Silva, 2012-02-20 16:59:02]

Here we go!

@pastarmovj: please review
@mnissler: FYI

[Mattias Nissler (ping if slow), 2012-02-20 17:39:01]

Looks good from my side, but I defer the main review to Julian.

[pastarmovj, 2012-02-21 14:06:39]

As discussed offline I put my two concerns in the code review.

http://codereview.chromium.org/9355044/diff/17/chrome/browser/policy/app_pack...
File chrome/browser/policy/app_pack_updater.h (right):

http://codereview.chromium.org/9355044/diff/17/chrome/browser/policy/app_pack...
chrome/browser/policy/app_pack_updater.h:12: class AppPackUpdater {
I guess this class is going to grow beyond a container for three constants
right? If so I am fine with how you have it now. If not I guess a namespace
would be better.

Btw this is a rather vague memory but I think there was some issues with static
members and inheritance over lib boundaries. Could that impact mock-ability of
this class? What speaks against having those as a global constants in the policy
namespace possible renamed s/^k/kAppPack/.

http://codereview.chromium.org/9355044/diff/17/chrome/browser/policy/proto/ch...
File chrome/browser/policy/proto/chrome_device_policy.proto (right):

http://codereview.chromium.org/9355044/diff/17/chrome/browser/policy/proto/ch...
chrome/browser/policy/proto/chrome_device_policy.proto:101: message
KioskModeAppPackEntryProto {
I don't think the name of this proto needs to contain the word "Kiosk". Those
AppPacks might as well ever only be used in Kiosk mode but for now we tend to
overload kiosk for a few different scenarios and we might end up splitting those
in separete modes then it might be awkward to have to explain why
KioskModeAppPackProto and not RetailModeAppPackProto etc.

http://codereview.chromium.org/9355044/diff/17/chrome/browser/policy/proto/ch...
chrome/browser/policy/proto/chrome_device_policy.proto:107: message
KioskModeAppPackProto {
Ditto.

http://codereview.chromium.org/9355044/diff/17/chrome/browser/policy/proto/ch...
chrome/browser/policy/proto/chrome_device_policy.proto:109: repeated
KioskModeAppPackEntryProto app_pack = 1;
ditto

http://codereview.chromium.org/9355044/diff/17/chrome/browser/policy/proto/ch...
chrome/browser/policy/proto/chrome_device_policy.proto:126: optional
KioskModeAppPackProto kiosk_mode_app_pack = 14;
ditto

[Joao da Silva, 2012-02-21 15:24:49]

Hey Julian, thanks for the review! PTAL.

http://codereview.chromium.org/9355044/diff/17/chrome/browser/policy/app_pack...
File chrome/browser/policy/app_pack_updater.h (right):

http://codereview.chromium.org/9355044/diff/17/chrome/browser/policy/app_pack...
chrome/browser/policy/app_pack_updater.h:12: class AppPackUpdater {
On 2012/02/21 14:06:40, pastarmovj wrote:
> I guess this class is going to grow beyond a container for three constants
> right? If so I am fine with how you have it now. If not I guess a namespace
> would be better.

It a big class :-) This is the bare minimum for this CL, since the field names
are required to decode the message.

> 
> Btw this is a rather vague memory but I think there was some issues with
static
> members and inheritance over lib boundaries. Could that impact mock-ability of
> this class? What speaks against having those as a global constants in the
policy
> namespace possible renamed s/^k/kAppPack/.

I don't expect these constants to be used out of libbrowser, and I also don't
think they matter to mocking. I've scoped them within the class to avoid
polluting the policy namespace, and to signal that these are AppPack-related
constants. Having an kAppPack prefix in the policy namespace would also work.

http://codereview.chromium.org/9355044/diff/17/chrome/browser/policy/proto/ch...
File chrome/browser/policy/proto/chrome_device_policy.proto (right):

http://codereview.chromium.org/9355044/diff/17/chrome/browser/policy/proto/ch...
chrome/browser/policy/proto/chrome_device_policy.proto:101: message
KioskModeAppPackEntryProto {
On 2012/02/21 14:06:40, pastarmovj wrote:
> I don't think the name of this proto needs to contain the word "Kiosk". Those
> AppPacks might as well ever only be used in Kiosk mode but for now we tend to
> overload kiosk for a few different scenarios and we might end up splitting
those
> in separete modes then it might be awkward to have to explain why
> KioskModeAppPackProto and not RetailModeAppPackProto etc.

Done.

http://codereview.chromium.org/9355044/diff/17/chrome/browser/policy/proto/ch...
chrome/browser/policy/proto/chrome_device_policy.proto:107: message
KioskModeAppPackProto {
On 2012/02/21 14:06:40, pastarmovj wrote:
> Ditto.

Done.

http://codereview.chromium.org/9355044/diff/17/chrome/browser/policy/proto/ch...
chrome/browser/policy/proto/chrome_device_policy.proto:109: repeated
KioskModeAppPackEntryProto app_pack = 1;
On 2012/02/21 14:06:40, pastarmovj wrote:
> ditto

Done.

http://codereview.chromium.org/9355044/diff/17/chrome/browser/policy/proto/ch...
chrome/browser/policy/proto/chrome_device_policy.proto:126: optional
KioskModeAppPackProto kiosk_mode_app_pack = 14;
On 2012/02/21 14:06:40, pastarmovj wrote:
> ditto

Done.

[Joao da Silva, 2012-02-23 13:16:58]

@pastarmovj: friendly ping

[pastarmovj, 2012-02-23 13:29:56]

LGTM. Sorry for the delay. :(

[commit-bot: I haz the power, 2012-02-23 15:18:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/9355044/22001

[commit-bot: I haz the power, 2012-02-23 17:03:50]

Change committed as 123261