kiosk: Add kiosk.required_platform_version manifest key

kiosk: Add kiosk.required_platform_version manifest key

BUG=577779
TEST=KioskModeInfoManifestTest.RequiredPlatformVersion*

Committed: https://crrev.com/c13cf9fa303adc690738c7acf6bdb2c934fb111b
Cr-Commit-Position: refs/heads/master@{#370722}

[xiyuan, 2016-01-15 21:00:00]

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

[xiyuan, 2016-01-15 21:02:26]

xiyuan@chromium.org changed reviewers:
+ asargent@chromium.org, jennyz@chromium.org

[xiyuan, 2016-01-15 21:02:26]

jennyz@, please drive the review.
asargent@, please review and help with owner's stamp.

jinzhang@, fyi

Thanks.

[commit-bot: I haz the power, 2016-01-15 21:03:11]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1595483004/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1595483004/1

[commit-bot: I haz the power, 2016-01-15 22:13:05]

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

[commit-bot: I haz the power, 2016-01-15 22:13:06]

Dry run: This issue passed the CQ dry run.

[jennyz, 2016-01-15 22:30:49]

lgtm

[asargent_no_longer_on_chrome, 2016-01-15 23:27:33]

I took a look at the associated bug but I still don't quite understand what
"required_platform_version" is supposed to mean. Is this the same thing as a
chrome/chromeos version number, or something else entirely? The bug states it is
used as a "ChromeOS platform version prefix" in update requests. 

If it's a chrome/chromeos version number, is there a reason you couldn't just
use the already existing "minimum_chrome_version" number we already have support
for? (See https://developer.chrome.com/extensions/manifest). That value can be
set to a a subset of a full version number with semantics working as you'd
expect, and the autoupdate code already respects it (eg if an updated version of
a currently installed extension/app specifies a minimum_chrome_version that is
greater than our current browser version, we won't try updating it). 

If this is something else entirely, can you explain a little more what this
number means and how it will be used? And is it something that will only ever be
relevant to kiosk apps, or might it make sense for extensions as well?

https://codereview.chromium.org/1595483004/diff/1/extensions/common/manifest_...
File extensions/common/manifest_handlers/kiosk_mode_info.cc (right):

https://codereview.chromium.org/1595483004/diff/1/extensions/common/manifest_...
extensions/common/manifest_handlers/kiosk_mode_info.cc:38: }
could you just use base::Version for this instead of having custom parsing logic
here?

[xiyuan, 2016-01-16 00:09:40]

On 2016/01/15 23:27:33, Antony Sargent wrote:
> I took a look at the associated bug but I still don't quite understand what
> "required_platform_version" is supposed to mean. Is this the same thing as a
> chrome/chromeos version number, or something else entirely? The bug states it
is
> used as a "ChromeOS platform version prefix" in update requests. 
> 

"required_platform_version" is the ChromeOS platform version number. It is a
separate version number from Chrome's version number. The number can uniquely
identify a ChromeOS image and is used by ChromeOS AU. It is needed because there
are cases when ChromeOS image is built without Chrome changes. In such cases,
the platform version number is incremented but Chrome's version number stays the
same. That is why we cannot use the Chrome version here.

> If it's a chrome/chromeos version number, is there a reason you couldn't just
> use the already existing "minimum_chrome_version" number we already have
support
> for? (See https://developer.chrome.com/extensions/manifest). That value can be
> set to a a subset of a full version number with semantics working as you'd
> expect, and the autoupdate code already respects it (eg if an updated version
of
> a currently installed extension/app specifies a minimum_chrome_version that is
> greater than our current browser version, we won't try updating it). 
> 

"minimum_chrome_version" could not be used here. This feature is somewhat the
opposite of minimum_chrome_version, like a maximum_chrome_version. That is, when
an auto launched kiosk app has the key, we want to limit Chrome's version to
never go beyond that version.

> If this is something else entirely, can you explain a little more what this
> number means and how it will be used? And is it something that will only ever
be
> relevant to kiosk apps, or might it make sense for extensions as well?
> 

If all conditions met, the required_platform_version number will be passed to
ChromeOS update engine and included in the update check ping. ChromeOS AU would
then only update to a version that matches the given number instead of always
going to latest. This would help kiosk deployment and give admins more control
of their kiosk devices.

This feature is only relevant to ChromeOS kiosk apps.

Ping me if you have more questions. :)

[asargent_no_longer_on_chrome, 2016-01-19 22:11:00]

Ok, I think I understand the motivations for this change.

I'm a bit concerned about the high level product implications of this feature,
and I think some additional discussion with folks in chrome leadership are in
order. I'll send a separate message with some suggestions. 

In the meantime I think we should make this manifest property experimental and
only available on canary and/or dev channel until you get launch approval.

https://codereview.chromium.org/1595483004/diff/1/extensions/common/api/_mani...
File extensions/common/api/_manifest_features.json (right):

https://codereview.chromium.org/1595483004/diff/1/extensions/common/api/_mani...
extensions/common/api/_manifest_features.json:291: "required_platform_version":
{
I don't think this is a feature we'd ever consider launching for non-kiosk mode,
so I think it would make sense to somehow indicate in the name of this manifest
property that this is kiosk-specific. Ideally that might look something like the
following in a manifest.json file:

{
  "name": "My Kiosk App",
  "version": "2",
  "manifest_version": 2,
  "kiosk_enabled": true,
  "kiosk_only": true,
  "kiosk": {
    "required_platform_version": "1.2.3.4"
  }
}


You can declare that in this file by doing:

"kiosk.required_platform_version" : {
  ...
}



(See app.background, background.persistent, etc. for examples)

https://codereview.chromium.org/1595483004/diff/1/extensions/common/api/_mani...
extensions/common/api/_manifest_features.json:292: "channel": "stable",
Please set channel to canary or dev until you have launch approval

[xiyuan, 2016-01-19 23:15:15]

Description was changed from

==========
kiosk: Add required_platform_version manifest key

BUG=577779
TEST=KioskModeInfoManifestTest.RequiredPlatformVersion*
==========

to

==========
kiosk: Add kiosk.required_platform_version manifest key

BUG=577779
TEST=KioskModeInfoManifestTest.RequiredPlatformVersion*
==========

[xiyuan, 2016-01-19 23:40:53]

https://codereview.chromium.org/1595483004/diff/1/extensions/common/api/_mani...
File extensions/common/api/_manifest_features.json (right):

https://codereview.chromium.org/1595483004/diff/1/extensions/common/api/_mani...
extensions/common/api/_manifest_features.json:291: "required_platform_version":
{
On 2016/01/19 22:11:00, Antony Sargent wrote:
> I don't think this is a feature we'd ever consider launching for non-kiosk
mode,
> so I think it would make sense to somehow indicate in the name of this
manifest
> property that this is kiosk-specific. Ideally that might look something like
the
> following in a manifest.json file:
> 
> {
>   "name": "My Kiosk App",
>   "version": "2",
>   "manifest_version": 2,
>   "kiosk_enabled": true,
>   "kiosk_only": true,
>   "kiosk": {
>     "required_platform_version": "1.2.3.4"
>   }
> }
> 
> 
> You can declare that in this file by doing:
> 
> "kiosk.required_platform_version" : {
>   ...
> }
> 
> 
> 
> (See app.background, background.persistent, etc. for examples)

Done.

https://codereview.chromium.org/1595483004/diff/1/extensions/common/api/_mani...
extensions/common/api/_manifest_features.json:292: "channel": "stable",
On 2016/01/19 22:11:00, Antony Sargent wrote:
> Please set channel to canary or dev until you have launch approval

Done.

https://codereview.chromium.org/1595483004/diff/1/extensions/common/manifest_...
File extensions/common/manifest_handlers/kiosk_mode_info.cc (right):

https://codereview.chromium.org/1595483004/diff/1/extensions/common/manifest_...
extensions/common/manifest_handlers/kiosk_mode_info.cc:38: }
On 2016/01/15 23:27:33, Antony Sargent wrote:
> could you just use base::Version for this instead of having custom parsing
logic
> here?

Good call. Done.

[asargent_no_longer_on_chrome, 2016-01-20 00:05:25]

lgtm

[xiyuan, 2016-01-21 16:05:15]

The CQ bit was checked by xiyuan@chromium.org

[xiyuan, 2016-01-21 16:05:15]

The patchset sent to the CQ was uploaded after l-g-t-m from jennyz@chromium.org
Link to the patchset: https://codereview.chromium.org/1595483004/#ps20001
(title: "put required_platform_version under kiosk and target dev channel")

[commit-bot: I haz the power, 2016-01-21 16:05:47]

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

[commit-bot: I haz the power, 2016-01-21 17:21:45]

Description was changed from

==========
kiosk: Add kiosk.required_platform_version manifest key

BUG=577779
TEST=KioskModeInfoManifestTest.RequiredPlatformVersion*
==========

to

==========
kiosk: Add kiosk.required_platform_version manifest key

BUG=577779
TEST=KioskModeInfoManifestTest.RequiredPlatformVersion*
==========

[commit-bot: I haz the power, 2016-01-21 17:21:46]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-01-21 17:23:40]

Description was changed from

==========
kiosk: Add kiosk.required_platform_version manifest key

BUG=577779
TEST=KioskModeInfoManifestTest.RequiredPlatformVersion*
==========

to

==========
kiosk: Add kiosk.required_platform_version manifest key

BUG=577779
TEST=KioskModeInfoManifestTest.RequiredPlatformVersion*

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

[commit-bot: I haz the power, 2016-01-21 17:23:41]

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