Only enable the Low Box token on Windows 8 and 8.1

Only enable the Low Box token on Windows 8 and 8.1

r319208 added the Low Box token to PreSpawnTarget. For some reason, this doesn't play well with Windows 10 preview builds, so this CL selectively enables the feature on Windows 8 & 8.1

BUG=464779, 455496
Committed: https://crrev.com/b5c098bbecc0cf42863cec0aa24651b00642b3af
Cr-Commit-Position: refs/heads/master@{#319790}

[hichris123, 2015-03-08 15:50:46]

hichris123@gmail.com changed reviewers:
+ cpu@chromium.org, shrikant@chromium.org

[hichris123, 2015-03-08 15:52:19]

cpu, Shrikant: PTAL. This has been causing Canary on Windows 10 not to load, see
bug 464779.

[Shrikant Kelkar, 2015-03-09 00:07:03]

On 2015/03/08 15:52:19, hichris123 wrote:
> cpu, Shrikant: PTAL. This has been causing Canary on Windows 10 not to load,
see
> bug 464779.

Thanks for the CL. Please remove windows_version.cc file, modified for Windows
10 builds earlier than 9926.

[hichris123, 2015-03-09 00:12:21]

On 2015/03/09 00:07:03, Shrikant Kelkar wrote:
> On 2015/03/08 15:52:19, hichris123 wrote:
> > cpu, Shrikant: PTAL. This has been causing Canary on Windows 10 not to load,
> see
> > bug 464779.
> 
> Thanks for the CL. Please remove windows_version.cc file, modified for Windows
> 10 builds earlier than 9926.

Okay, but that means Chrome will not work on older Technical Preview builds
still.

[Will Harris, 2015-03-09 00:16:21]

On 2015/03/09 00:12:21, hichris123 wrote:
> On 2015/03/09 00:07:03, Shrikant Kelkar wrote:
> > On 2015/03/08 15:52:19, hichris123 wrote:
> > > cpu, Shrikant: PTAL. This has been causing Canary on Windows 10 not to
load,
> > see
> > > bug 464779.
> > 
> > Thanks for the CL. Please remove windows_version.cc file, modified for
Windows
> > 10 builds earlier than 9926.
> 
> Okay, but that means Chrome will not work on older Technical Preview builds
> still.

How about changing the test in
content/browser/renderer_host/render_process_host_impl.cc to match only win8,
especially given we only have not coverage on win8 at the moment.

We can add win10 support once we have better coverage and it gets closer to rtm.

[hichris123, 2015-03-09 00:18:41]

On 2015/03/09 00:16:21, Will Harris wrote:
> On 2015/03/09 00:12:21, hichris123 wrote:
> > On 2015/03/09 00:07:03, Shrikant Kelkar wrote:
> > > On 2015/03/08 15:52:19, hichris123 wrote:
> > > > cpu, Shrikant: PTAL. This has been causing Canary on Windows 10 not to
> load,
> > > see
> > > > bug 464779.
> > > 
> > > Thanks for the CL. Please remove windows_version.cc file, modified for
> Windows
> > > 10 builds earlier than 9926.
> > 
> > Okay, but that means Chrome will not work on older Technical Preview builds
> > still.
> 
> How about changing the test in
> content/browser/renderer_host/render_process_host_impl.cc to match only win8,
> especially given we only have not coverage on win8 at the moment.
> 
> We can add win10 support once we have better coverage and it gets closer to
rtm.

*Just* Win8 or 8.1 too?

The reason why I ask is older Win10 builds are identified by Chrome as Win8.1,
so we could either a) change windows_version, b) match only Win8 and not 8.1, or
c) match Win 8 and 8.1, and ignore users on older versions of the preview.

[Will Harris, 2015-03-09 00:34:20]

On 2015/03/09 00:18:41, hichris123 wrote:
> On 2015/03/09 00:16:21, Will Harris wrote:
> > On 2015/03/09 00:12:21, hichris123 wrote:
> > > On 2015/03/09 00:07:03, Shrikant Kelkar wrote:
> > > > On 2015/03/08 15:52:19, hichris123 wrote:
> > > > > cpu, Shrikant: PTAL. This has been causing Canary on Windows 10 not to
> > load,
> > > > see
> > > > > bug 464779.
> > > > 
> > > > Thanks for the CL. Please remove windows_version.cc file, modified for
> > Windows
> > > > 10 builds earlier than 9926.
> > > 
> > > Okay, but that means Chrome will not work on older Technical Preview
builds
> > > still.
> > 
> > How about changing the test in
> > content/browser/renderer_host/render_process_host_impl.cc to match only
win8,
> > especially given we only have not coverage on win8 at the moment.
> > 
> > We can add win10 support once we have better coverage and it gets closer to
> rtm.
> 
> *Just* Win8 or 8.1 too?
> 
> The reason why I ask is older Win10 builds are identified by Chrome as Win8.1,
> so we could either a) change windows_version, b) match only Win8 and not 8.1,
or
> c) match Win 8 and 8.1, and ignore users on older versions of the preview.

Win 8 and Win 8.1

[hichris123, 2015-03-09 01:09:45]

On 2015/03/09 00:34:20, Will Harris wrote:
> On 2015/03/09 00:18:41, hichris123 wrote:
> > On 2015/03/09 00:16:21, Will Harris wrote:
> > > On 2015/03/09 00:12:21, hichris123 wrote:
> > > > On 2015/03/09 00:07:03, Shrikant Kelkar wrote:
> > > > > On 2015/03/08 15:52:19, hichris123 wrote:
> > > > > > cpu, Shrikant: PTAL. This has been causing Canary on Windows 10 not
to
> > > load,
> > > > > see
> > > > > > bug 464779.
> > > > > 
> > > > > Thanks for the CL. Please remove windows_version.cc file, modified for
> > > Windows
> > > > > 10 builds earlier than 9926.
> > > > 
> > > > Okay, but that means Chrome will not work on older Technical Preview
> builds
> > > > still.
> > > 
> > > How about changing the test in
> > > content/browser/renderer_host/render_process_host_impl.cc to match only
> win8,
> > > especially given we only have not coverage on win8 at the moment.
> > > 
> > > We can add win10 support once we have better coverage and it gets closer
to
> > rtm.
> > 
> > *Just* Win8 or 8.1 too?
> > 
> > The reason why I ask is older Win10 builds are identified by Chrome as
Win8.1,
> > so we could either a) change windows_version, b) match only Win8 and not
8.1,
> or
> > c) match Win 8 and 8.1, and ignore users on older versions of the preview.
> 
> Win 8 and Win 8.1

Okay, uploaded a new changeset.

[Will Harris, 2015-03-09 01:18:25]

wfh@chromium.org changed reviewers:
+ wfh@chromium.org

[Will Harris, 2015-03-09 01:18:25]

please also update the CL description & title to reflect more accurately what
this CL does - something like 'Only enable Low Box token on Windows 8 and 8.1'

https://codereview.chromium.org/986933002/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/986933002/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:334:
base::win::GetVersion() != base::win::VERSION_WIN8_1) {
I think you mean == here?

[hichris123, 2015-03-09 01:21:17]

On 2015/03/09 01:18:25, Will Harris wrote:
> please also update the CL description & title to reflect more accurately what
> this CL does - something like 'Only enable Low Box token on Windows 8 and 8.1'
> 
>
https://codereview.chromium.org/986933002/diff/40001/content/browser/renderer...
> File content/browser/renderer_host/render_process_host_impl.cc (right):
> 
>
https://codereview.chromium.org/986933002/diff/40001/content/browser/renderer...
> content/browser/renderer_host/render_process_host_impl.cc:334:
> base::win::GetVersion() != base::win::VERSION_WIN8_1) {
> I think you mean == here?

Er, yeah, that teaches me to double-check what I'm doing. ;) Fixed.

[hichris123, 2015-03-09 01:50:39]

On 2015/03/09 01:21:17, hichris123 wrote:
> On 2015/03/09 01:18:25, Will Harris wrote:
> > please also update the CL description & title to reflect more accurately
what
> > this CL does - something like 'Only enable Low Box token on Windows 8 and
8.1'
> > 
> >
>
https://codereview.chromium.org/986933002/diff/40001/content/browser/renderer...
> > File content/browser/renderer_host/render_process_host_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/986933002/diff/40001/content/browser/renderer...
> > content/browser/renderer_host/render_process_host_impl.cc:334:
> > base::win::GetVersion() != base::win::VERSION_WIN8_1) {
> > I think you mean == here?
> 
> Er, yeah, that teaches me to double-check what I'm doing. ;) Fixed.

Also updated the CL description. I think I need to read *everything* closer now.
:P

[cpu_(ooo_6.6-7.5), 2015-03-09 20:47:12]

lgtm

[hichris123, 2015-03-09 21:00:35]

On 2015/03/09 20:47:12, cpu wrote:
> lgtm

I'll send this to the CQ, then, unless anyone has any objections.

[Will Harris, 2015-03-09 21:10:32]

The CQ bit was checked by wfh@chromium.org

[commit-bot: I haz the power, 2015-03-09 21:11:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/986933002/60001

[Will Harris, 2015-03-09 21:12:11]

I CQed it - I think you need to be a committer to tick the box.

[commit-bot: I haz the power, 2015-03-10 00:23:27]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-03-10 00:24:49]

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