Add "WebVR experimental rendering optimizations" flag (issue 2698573002 by klausw@chromium.org)

[a...@chromium.org]

https://codereview.chromium.org/2698573002/diff/1/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2698573002/diff/1/chrome/browser/about_flags.cc#newcode1614
chrome/browser/about_flags.cc:1614:
{"enable-webvr-experimental-rendering",
Let's only show this when webvr is enabled (which should soon be renamed
to enable_vr_platfrom instead of being webvr specific).

https://codereview.chromium.org/2698573002/

[kla...@chromium.org]

Reviewers: amp, jochen, rkaplow
CL: https://codereview.chromium.org/2698573002/

Message:
jochen@: Please review the content/feature list parts and the histograms change.

rkaplow@: Finch ambassador.

Launch bug is http://crbug.com/670502 "OWP Launch: WebVR"

https://codereview.chromium.org/2698573002/diff/1/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2698573002/diff/1/chrome/browser/about_flags.cc#newcode1614
chrome/browser/about_flags.cc:1614:
{"enable-webvr-experimental-rendering",

On 2017/02/14 23:23:26, amp wrote:
> Let's only show this when webvr is enabled (which should soon be
renamed to
> enable_vr_platfrom instead of being webvr specific).

Done.

Description:
Add "WebVR experimental rendering optimizations" flag

This is intended to support features such as direct-draw-to-surface
which are considered experimental or risky, so that they can be
manually enabled for initial testing. It is also in preparation
for a finch experiment for future rollout.

BUG=655733

Affected files (+21, -0 lines):
M chrome/app/generated_resources.grd
M chrome/browser/about_flags.cc
M content/public/common/content_features.h
M content/public/common/content_features.cc
M content/public/common/content_switches.h
M content/public/common/content_switches.cc
M tools/metrics/histograms/histograms.xml


Index: chrome/app/generated_resources.grd
diff --git a/chrome/app/generated_resources.grd b/chrome/app/generated_resources.grd
index ce15d8ea301fb9ea4c56a5fbe9d532e85d16ec75..29eb3c7bac88e8617d4793bb1f913753ac4e45be 100644
--- a/chrome/app/generated_resources.grd
+++ b/chrome/app/generated_resources.grd
@@ -5504,6 +5504,12 @@ Keep your key file in a safe place. You will need it to create new versions of y
<message name="IDS_FLAGS_WEBVR_DESCRIPTION" desc="Description for the flag to enable WebVR APIs.">
Enabling this option allows web applications to access experimental Virtual Reality APIs.
</message>
+ <message name="IDS_FLAGS_WEBVR_EXPERIMENTAL_RENDERING_NAME" desc="Name of the 'Enable WebVR experimental optimizations' flag.">
+ WebVR experimental rendering optimizations
+ </message>
+ <message name="IDS_FLAGS_WEBVR_EXPERIMENTAL_RENDERING_DESCRIPTION" desc="Description for the flag to enable experimental WebVR rendering optimizations.">
+ Enabling this option activates experimental rendering path optimizations for WebVR.
+ </message>
<message name="IDS_FLAGS_GAMEPAD_EXTENSIONS_NAME" desc="Name of the flag which allows users to enable experimental extensions to the Gamepad API.">
Gamepad Extensions
</message>
Index: chrome/browser/about_flags.cc
diff --git a/chrome/browser/about_flags.cc b/chrome/browser/about_flags.cc
index 855e4aeb2525d3ee4ec4a44977032db4c0971cd0..a01de4d6ba0fe286faf318e4c9c26add0e16d311 100644
--- a/chrome/browser/about_flags.cc
+++ b/chrome/browser/about_flags.cc
@@ -1612,6 +1612,10 @@ const FeatureEntry kFeatureEntries[] = {
{"enable-webvr", IDS_FLAGS_WEBVR_NAME, IDS_FLAGS_WEBVR_DESCRIPTION, kOsAll,
SINGLE_VALUE_TYPE(switches::kEnableWebVR)},
#if defined(ENABLE_WEBVR)
+ {"enable-webvr-experimental-rendering",
+ IDS_FLAGS_WEBVR_EXPERIMENTAL_RENDERING_NAME,
+ IDS_FLAGS_WEBVR_EXPERIMENTAL_RENDERING_DESCRIPTION, kOsAll,
+ SINGLE_VALUE_TYPE(switches::kEnableWebVRExperimentalRendering)},
{"enable-vr-shell", IDS_FLAGS_ENABLE_VR_SHELL_NAME,
IDS_FLAGS_ENABLE_VR_SHELL_DESCRIPTION, kOsAndroid,
FEATURE_VALUE_TYPE(features::kVrShell)},
Index: content/public/common/content_features.cc
diff --git a/content/public/common/content_features.cc b/content/public/common/content_features.cc
index e9a64b515cdcca10ad73e2aaf61da1c77ad5ffef..d9be0ee08fd1ec4e2e58ce2b50d1c1d11a013df7 100644
--- a/content/public/common/content_features.cc
+++ b/content/public/common/content_features.cc
@@ -253,6 +253,10 @@ const base::Feature kWebRtcHWH264Encoding{
// https://wicg.github.io/webusb
const base::Feature kWebUsb{"WebUSB", base::FEATURE_ENABLED_BY_DEFAULT};

+// Enables WebVR experimental rendering optimizations.
+const base::Feature kWebVrExperimentalRendering{
+ "WebVrExperimentalRendering", base::FEATURE_DISABLED_BY_DEFAULT};
+
// Make sendBeacon throw for a Blob with a non simple type.
const base::Feature kSendBeaconThrowForBlobWithNonSimpleType{
"SendBeaconThrowForBlobWithNonSimpleType",
Index: content/public/common/content_features.h
diff --git a/content/public/common/content_features.h b/content/public/common/content_features.h
index 359a95152ef08c00415f7b88dc65c689dd054337..9ecc990641fd72ea7274fa3acde829627f9cf37d 100644
--- a/content/public/common/content_features.h
+++ b/content/public/common/content_features.h
@@ -67,6 +67,7 @@ CONTENT_EXPORT extern const base::Feature kWebRtcEcdsaDefault;
CONTENT_EXPORT extern const base::Feature kWebRtcHWH264Encoding;
CONTENT_EXPORT extern const base::Feature kWebRtcUseGpuMemoryBufferVideoFrames;
CONTENT_EXPORT extern const base::Feature kWebUsb;
+CONTENT_EXPORT extern const base::Feature kWebVrExperimentalRendering;
CONTENT_EXPORT
extern const base::Feature kSendBeaconThrowForBlobWithNonSimpleType;

Index: content/public/common/content_switches.cc
diff --git a/content/public/common/content_switches.cc b/content/public/common/content_switches.cc
index 5a7fb6c3ce1de82f7a5aa354cf16963b46e88cdd..43531c6afa5deb558d4bc2285ab145afaa4093e8 100644
--- a/content/public/common/content_switches.cc
+++ b/content/public/common/content_switches.cc
@@ -535,6 +535,10 @@ const char kEnableWebGLImageChromium[] = "enable-webgl-image-chromium";
// Enables interaction with virtual reality devices.
const char kEnableWebVR[] = "enable-webvr";

+// Enables experimental rendering optimizations for WebVR.
+const char kEnableWebVRExperimentalRendering[] =
+ "enable-webvr-experimental-rendering";
+
// Enable rasterizer that writes directly to GPU memory associated with tiles.
const char kEnableZeroCopy[] = "enable-zero-copy";

Index: content/public/common/content_switches.h
diff --git a/content/public/common/content_switches.h b/content/public/common/content_switches.h
index 5c4f9a5a78344c1e7b3922c0fa76b86a85190dbc..210899be821ed22728df968b69238e780347d4dd 100644
--- a/content/public/common/content_switches.h
+++ b/content/public/common/content_switches.h
@@ -163,6 +163,7 @@ CONTENT_EXPORT extern const char kEnableWebFontsInterventionTrigger[];
CONTENT_EXPORT extern const char kEnableWebGLDraftExtensions[];
CONTENT_EXPORT extern const char kEnableWebGLImageChromium[];
CONTENT_EXPORT extern const char kEnableWebVR[];
+CONTENT_EXPORT extern const char kEnableWebVRExperimentalRendering[];
CONTENT_EXPORT extern const char kEnableZeroCopy[];
CONTENT_EXPORT extern const char kExplicitlyAllowedPorts[];
CONTENT_EXPORT extern const char kFieldTrialHandle[];
Index: tools/metrics/histograms/histograms.xml
diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml
index da838472d9a1087a76b4e814fe3e83dfa9de6dc7..b9548b02d88964f9e913ab3d3066bf68b007f5d7 100644
--- a/tools/metrics/histograms/histograms.xml
+++ b/tools/metrics/histograms/histograms.xml
@@ -96713,6 +96713,7 @@ value.
<int value="-1478876902" label="disable-permission-action-reporting"/>
<int value="-1473668019" label="token-binding:disabled"/>
<int value="-1473136627" label="enable-web-payments"/>
+ <int value="-1473098749" label="enable-webvr-experimental-rendering"/>
<int value="-1467332609" label="tab-management-experiment-type-anise"/>
<int value="-1466990325" label="CrosCompUpdates:enabled"/>
<int value="-1460462432" label="disable-media-source"/>

[joc...@chromium.org]

I can't approve general histograms changes, just use counter additions.

You add four different mechanisms to control a feature in this CL, but nothing
that actually depends on those flags.

Please bundle the additions of the individual mechanisms together with the code
changes (not lgtm)

https://codereview.chromium.org/2698573002/

[a...@chromium.org]

On 2017/02/15 10:00:38, jochen wrote:
> I can't approve general histograms changes, just use counter additions.
>
> You add four different mechanisms to control a feature in this CL, but nothing
> that actually depends on those flags.
>
> Please bundle the additions of the individual mechanisms together with the
code
> changes (not lgtm)

What are the four mechanisms? I see the feature flag and the content switch
(which we shouldn't need), and there are the about flag references, but that's
just a way to enable the flag rather than a different mechanism, isn't it?


Re the lack of inclusion of code that this feature flag actually gates: I had
recommended that we add the feature flag first as the underlying code is rather
complex and the flag would just be noise in that review, but apologies if it is
preferred to include everything together.

https://codereview.chromium.org/2698573002/

[a...@chromium.org]

https://codereview.chromium.org/2698573002/diff/40001/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2698573002/diff/40001/chrome/browser/about_flags.cc#newcode1617
chrome/browser/about_flags.cc:1617:
IDS_FLAGS_WEBVR_EXPERIMENTAL_RENDERING_DESCRIPTION, kOsAll,
Should we limit this to Android only for now?

https://codereview.chromium.org/2698573002/diff/40001/chrome/browser/about_flags.cc#newcode1618
chrome/browser/about_flags.cc:1618:
SINGLE_VALUE_TYPE(switches::kEnableWebVRExperimentalRendering)},
This should be referring to the feature (like vr shell does below), not
to the switch (which aren't needed).

https://codereview.chromium.org/2698573002/diff/40001/content/public/common/content_features.cc
File content/public/common/content_features.cc (right):

https://codereview.chromium.org/2698573002/diff/40001/content/public/common/content_features.cc#newcode258
content/public/common/content_features.cc:258:
"WebVrExperimentalRendering", base::FEATURE_DISABLED_BY_DEFAULT};
WebVR has a capital 'R' in all the other places we use it. Not sure if
it makes a huge difference, but I'm likely to misspell this on the
command line if I try to use it.

https://codereview.chromium.org/2698573002/diff/40001/content/public/common/content_switches.h
File content/public/common/content_switches.h (right):

https://codereview.chromium.org/2698573002/diff/40001/content/public/common/content_switches.h#newcode166
content/public/common/content_switches.h:166: CONTENT_EXPORT extern
const char kEnableWebVRExperimentalRendering[];
You shouldn't need to add a separate switch. Adding a feature
automatically provides a command line way of enabling it, via something
like:
--enable-features=WebVRExperimentalRendering

https://codereview.chromium.org/2698573002/diff/40001/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5
File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5
(right):

https://codereview.chromium.org/2698573002/diff/40001/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5#newcode914
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5:914:
name: "WebVRExperimentalRendering",
You can add a 'depends_on' section here to limit this being turned on
only when WebVR is turned on (see the 'depends_on' docs at the top of
the file).

https://codereview.chromium.org/2698573002/

[mthi...@chromium.org]

I agree with jochen though that we should definitely get the code landed with
the runtime flag. Is there any reason to land this on its own?

https://codereview.chromium.org/2698573002/

[kla...@chromium.org]

Everyone added: This is an updated version of crrev.com/2586803003 which you had
previously reviewed and which was reverted. I've removed the "low priority" GL
context part which was causing render glitches, and made the change conditional
on a new feature flag (off by default) to enable further testing and a
controlled rollout later.

ael...@chromium.org: Please review changes in
content/renderer/renderer_blink_platform_impl.cc

dch...@chromium.org: Please review changes in
gpu/ipc/common/gpu_command_buffer_traits_multi.h

https://codereview.chromium.org/2698573002/diff/40001/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2698573002/diff/40001/chrome/browser/about_flags.cc#newcode1617
chrome/browser/about_flags.cc:1617:
IDS_FLAGS_WEBVR_EXPERIMENTAL_RENDERING_DESCRIPTION, kOsAll,

On 2017/02/15 17:44:03, amp wrote:
> Should we limit this to Android only for now?

Done. I assume that kOsAndroid here means to just keep it auto-disabled
and hidden on other platforms? An ifdef wouldn't work since the feature
check is in non-Android-specific code.

https://codereview.chromium.org/2698573002/diff/40001/chrome/browser/about_flags.cc#newcode1618
chrome/browser/about_flags.cc:1618:
SINGLE_VALUE_TYPE(switches::kEnableWebVRExperimentalRendering)},

On 2017/02/15 17:44:03, amp wrote:
> This should be referring to the feature (like vr shell does below),
not to the
> switch (which aren't needed).

Done.

https://codereview.chromium.org/2698573002/diff/40001/content/public/common/content_features.cc
File content/public/common/content_features.cc (right):

https://codereview.chromium.org/2698573002/diff/40001/content/public/common/content_features.cc#newcode258
content/public/common/content_features.cc:258:
"WebVrExperimentalRendering", base::FEATURE_DISABLED_BY_DEFAULT};

On 2017/02/15 17:44:03, amp wrote:
> WebVR has a capital 'R' in all the other places we use it. Not sure
if it makes
> a huge difference, but I'm likely to misspell this on the command line
if I try
> to use it.

Done.

https://codereview.chromium.org/2698573002/diff/40001/content/public/common/content_switches.h
File content/public/common/content_switches.h (right):

https://codereview.chromium.org/2698573002/diff/40001/content/public/common/content_switches.h#newcode166
content/public/common/content_switches.h:166: CONTENT_EXPORT extern
const char kEnableWebVRExperimentalRendering[];

On 2017/02/15 17:44:03, amp wrote:
> You shouldn't need to add a separate switch. Adding a feature
automatically
> provides a command line way of enabling it, via something like:
> --enable-features=WebVRExperimentalRendering

Done, removed. I was following the WebVR example, didn't realize that I
got the two separate mechanisms mixed up.

https://codereview.chromium.org/2698573002/diff/40001/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5
File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5
(right):

https://codereview.chromium.org/2698573002/diff/40001/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5#newcode914
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5:914:
name: "WebVRExperimentalRendering",

On 2017/02/15 17:44:03, amp wrote:
> You can add a 'depends_on' section here to limit this being turned on
only when
> WebVR is turned on (see the 'depends_on' docs at the top of the file).

Declaring it as a dependency means that there's no way to turn on the
optimizations in "origin trial only" mode. I've added a code comment to
explain.

https://codereview.chromium.org/2698573002/

[a...@chromium.org]

https://codereview.chromium.org/2698573002/diff/40001/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2698573002/diff/40001/chrome/browser/about_flags.cc#newcode1617
chrome/browser/about_flags.cc:1617:
IDS_FLAGS_WEBVR_EXPERIMENTAL_RENDERING_DESCRIPTION, kOsAll,

On 2017/02/15 19:47:22, klausw wrote:
> On 2017/02/15 17:44:03, amp wrote:
> > Should we limit this to Android only for now?
>
> Done. I assume that kOsAndroid here means to just keep it
auto-disabled and
> hidden on other platforms? An ifdef wouldn't work since the feature
check is in
> non-Android-specific code.

I'm not actually sure what the os param does. Maybe it grays it out on
other platforms (but we are hiding it behind a build flag so it won't
show up anyway).

Either looks good to keep the os param in sync with VrShell below it.

https://codereview.chromium.org/2698573002/diff/60001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
File
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2698573002/diff/60001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode632
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:632:
// if called with a scriptState, not for a canvas.
Comment seems out of sync with code which does check the canvas for an
origin trial.

https://codereview.chromium.org/2698573002/

[kla...@chromium.org]

On 2017/02/15 10:00:38, jochen wrote:
> I can't approve general histograms changes, just use counter additions.
>
> You add four different mechanisms to control a feature in this CL, but nothing
> that actually depends on those flags.
>
> Please bundle the additions of the individual mechanisms together with the
code
> changes (not lgtm)

This should be moot now, I've removed the histograms.xml change since the code
no longer adds a flag.

https://codereview.chromium.org/2698573002/

[kla...@chromium.org]

Sorry, hit "send" too early. I've bundled in the mechanism that depends on the
change, and I think the control mechanisms should now be limited to those
necessary for the feature flag to work, without a separate command line flag.
Please let me know if I have remaining redundancies.

https://codereview.chromium.org/2698573002/

[a...@chromium.org]

I think the histogram change is from adding anything to about://flags, even if
it isn't defined in a command line switch.

I'm not sure, though so check if the unit test for about flags histograms
passes. If it does you should be fine.

https://codereview.chromium.org/2698573002/

[kla...@chromium.org]

amp@ wrote:
> I think the histogram change is from adding anything to about://flags, even if
> it isn't defined in a command line switch.
>
> I'm not sure, though so check if the unit test for about flags histograms
> passes. If it does you should be fine

The test does pass without any addition to histograms.xml, seems like it's only
enforced for actual command line switches.

https://codereview.chromium.org/2698573002/diff/60001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
File
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2698573002/diff/60001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode632
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:632:
// if called with a scriptState, not for a canvas.

On 2017/02/15 20:04:22, amp wrote:
> Comment seems out of sync with code which does check the canvas for an
origin
> trial.

Done.

https://codereview.chromium.org/2698573002/

[mthi...@chromium.org]

https://codereview.chromium.org/2698573002/diff/100001/content/renderer/renderer_blink_platform_impl.cc
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/2698573002/diff/100001/content/renderer/renderer_blink_platform_impl.cc#newcode1029
content/renderer/renderer_blink_platform_impl.cc:1029:
attributes.depth_size = 0;
Are you missing attributes.alpha_size = -1? Or was leaving that out
intentional?

https://codereview.chromium.org/2698573002/

[kla...@chromium.org]

https://codereview.chromium.org/2698573002/diff/100001/content/renderer/renderer_blink_platform_impl.cc
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/2698573002/diff/100001/content/renderer/renderer_blink_platform_impl.cc#newcode1029
content/renderer/renderer_blink_platform_impl.cc:1029:
attributes.depth_size = 0;

On 2017/02/15 21:36:43, mthiesse wrote:
> Are you missing attributes.alpha_size = -1? Or was leaving that out
intentional?

Re-added.

I had removed it in http://crrev.com/2586803003#ps220001 at dcheng@'s
suggestion since it's the default, but on second thought I do think it's
clearer to keep it in and match the pre-patch code path.

https://codereview.chromium.org/2698573002/

[ael...@chromium.org]

https://codereview.chromium.org/2698573002/diff/120001/content/renderer/renderer_blink_platform_impl.cc
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/2698573002/diff/120001/content/renderer/renderer_blink_platform_impl.cc#newcode1021
content/renderer/renderer_blink_platform_impl.cc:1021: if
(web_attributes.supportOwnOffscreenSurface) {
It doesn't look like this outer if statement is needed because the
ternary operator on each individual attribute covers it, so please
remove it.

https://codereview.chromium.org/2698573002/diff/120001/content/renderer/renderer_blink_platform_impl.cc#newcode1041
content/renderer/renderer_blink_platform_impl.cc:1041:
nit: unnecessary newline

https://codereview.chromium.org/2698573002/diff/120001/third_party/WebKit/public/platform/Platform.h
File third_party/WebKit/public/platform/Platform.h (right):

https://codereview.chromium.org/2698573002/diff/120001/third_party/WebKit/public/platform/Platform.h#newcode465
third_party/WebKit/public/platform/Platform.h:465: bool supportAlpha =
true;
These "true"s mixed in look arbitrary, can this be all false? And set
them explicitly where you care if you're relying on the defaults.

https://codereview.chromium.org/2698573002/

[kla...@chromium.org]

dcheng@: I think this no longer needs your review, I've removed the IPC changes.

I've refactored it a bit:

- Move the supportOwnOffscreenSurface conditional into
toPlatformContextAttributes so that it only asks for alpha/depth etc if it
intends to use it.

- Add supportOwnOffscreenSurface check to maybeRestoreContext.

- remove the GPU-level "own_offscreen_surface" attribute. Instead, ask for a
customized surface iff the requested format is incompatible with the default
surface.

What do you think?

https://codereview.chromium.org/2698573002/diff/120001/content/renderer/renderer_blink_platform_impl.cc
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/2698573002/diff/120001/content/renderer/renderer_blink_platform_impl.cc#newcode1021
content/renderer/renderer_blink_platform_impl.cc:1021: if
(web_attributes.supportOwnOffscreenSurface) {

On 2017/02/16 03:25:51, aelias wrote:
> It doesn't look like this outer if statement is needed because the
ternary
> operator on each individual attribute covers it, so please remove it.

Done, and also updated the comment above to match the new behavior.

https://codereview.chromium.org/2698573002/diff/120001/content/renderer/renderer_blink_platform_impl.cc#newcode1041
content/renderer/renderer_blink_platform_impl.cc:1041:
On 2017/02/16 03:25:51, aelias wrote:
> nit: unnecessary newline

Done.

https://codereview.chromium.org/2698573002/diff/120001/third_party/WebKit/public/platform/Platform.h
File third_party/WebKit/public/platform/Platform.h (right):

https://codereview.chromium.org/2698573002/diff/120001/third_party/WebKit/public/platform/Platform.h#newcode465
third_party/WebKit/public/platform/Platform.h:465: bool supportAlpha =
true;

On 2017/02/16 03:25:51, aelias wrote:
> These "true"s mixed in look arbitrary, can this be all false? And set
them
> explicitly where you care if you're relying on the defaults.

Done.

https://codereview.chromium.org/2698573002/

[kla...@chromium.org]

On 2017/02/16 20:06:58, klausw wrote:
> - remove the GPU-level "own_offscreen_surface" attribute. Instead, ask for a
> customized surface iff the requested format is incompatible with the default
> surface.

I've now also removed the Platform ContextAttribute-level
supportOwnOffscreenSurface
attribute, it's now implied by setting any of the supportAlpha etc. attributes.
Updated the comments to match.

https://codereview.chromium.org/2698573002/

[ael...@chromium.org]

content/renderer lgtm

https://codereview.chromium.org/2698573002/

[baj...@chromium.org]

modules/webgl LGTM


https://codereview.chromium.org/2698573002/diff/160001/gpu/command_buffer/common/gles2_cmd_utils.h
File gpu/command_buffer/common/gles2_cmd_utils.h (left):

https://codereview.chromium.org/2698573002/diff/160001/gpu/command_buffer/common/gles2_cmd_utils.h#oldcode342
gpu/command_buffer/common/gles2_cmd_utils.h:342:
Unneeded newline removal.

https://codereview.chromium.org/2698573002/

[kla...@chromium.org]

https://codereview.chromium.org/2698573002/diff/160001/gpu/command_buffer/common/gles2_cmd_utils.h
File gpu/command_buffer/common/gles2_cmd_utils.h (left):

https://codereview.chromium.org/2698573002/diff/160001/gpu/command_buffer/common/gles2_cmd_utils.h#oldcode342
gpu/command_buffer/common/gles2_cmd_utils.h:342:

On 2017/02/16 22:05:41, bajones wrote:
> Unneeded newline removal.

Reverted.

https://codereview.chromium.org/2698573002/

[kla...@chromium.org]

PTAL:

jochen@: please review the feature changes in content/*features* and
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5

jbauman@: please review the gpu/ and ui/gl/ changes

https://codereview.chromium.org/2698573002/

[jba...@chromium.org]

lgtm



https://codereview.chromium.org/2698573002/

[kla...@chromium.org]

mpearson@: please review the histograms.xml additions for the feature flag.

(I mistakenly thought earlier that this was not needed due to running the
desktop unit_tests. The Android version does insist on them.)

https://codereview.chromium.org/2698573002/

[mpea...@chromium.org]

histograms.xml lgtm


https://codereview.chromium.org/2698573002/

[joc...@chromium.org]

lgtm with comment


https://codereview.chromium.org/2698573002/diff/200001/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5
File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5
(right):

https://codereview.chromium.org/2698573002/diff/200001/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5#newcode917
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5:917:
status: "test",
I think this should be experimental not just test

https://codereview.chromium.org/2698573002/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2698573002/220001


https://codereview.chromium.org/2698573002/

[kla...@chromium.org]

Thank you!

https://codereview.chromium.org/2698573002/diff/200001/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5
File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5
(right):

https://codereview.chromium.org/2698573002/diff/200001/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5#newcode917
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5:917:
status: "test",

On 2017/02/17 08:34:33, jochen wrote:
> I think this should be experimental not just test

Done.

https://codereview.chromium.org/2698573002/

[rka...@chromium.org]

lgtm



https://codereview.chromium.org/2698573002/

[commit-bot@chromium.org via codereview.chromium.org]

Try jobs failed on following builders:
linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_rel_ng/builds/367236)

https://codereview.chromium.org/2698573002/

[commit-bot@chromium.org via codereview.chromium.org]

[commit-bot@chromium.org via codereview.chromium.org]

[a...@chromium.org]

lgtm



https://codereview.chromium.org/2698573002/

[commit-bot@chromium.org via codereview.chromium.org]

[commit-bot@chromium.org via codereview.chromium.org]

Failed to apply patch for content/public/common/content_features.h:
While running git apply --index -p1;
error: patch failed: content/public/common/content_features.h:67
error: content/public/common/content_features.h: patch does not apply

Patch: content/public/common/content_features.h
Index: content/public/common/content_features.h
diff --git a/content/public/common/content_features.h
b/content/public/common/content_features.h
index
359a95152ef08c00415f7b88dc65c689dd054337..a81a3fd08ddce5b97fcc4f4b67403e6b313392a8
100644
--- a/content/public/common/content_features.h
+++ b/content/public/common/content_features.h
@@ -67,6 +67,7 @@ CONTENT_EXPORT extern const base::Feature kWebRtcEcdsaDefault;
CONTENT_EXPORT extern const base::Feature kWebRtcHWH264Encoding;
CONTENT_EXPORT extern const base::Feature kWebRtcUseGpuMemoryBufferVideoFrames;
CONTENT_EXPORT extern const base::Feature kWebUsb;
+CONTENT_EXPORT extern const base::Feature kWebVRExperimentalRendering;
CONTENT_EXPORT
extern const base::Feature kSendBeaconThrowForBlobWithNonSimpleType;



https://codereview.chromium.org/2698573002/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2698573002/260001


https://codereview.chromium.org/2698573002/

[commit-bot@chromium.org via codereview.chromium.org]

Try jobs failed on following builders:

ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/builds/157830)

https://codereview.chromium.org/2698573002/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2698573002/280001


https://codereview.chromium.org/2698573002/

[commit-bot@chromium.org via codereview.chromium.org]

Try jobs failed on following builders:

cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
linux_optional_gpu_tests_rel on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

https://codereview.chromium.org/2698573002/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2698573002/290001


https://codereview.chromium.org/2698573002/

[commit-bot@chromium.org via codereview.chromium.org]

Try jobs failed on following builders:
cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)

linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux

[commit-bot@chromium.org via codereview.chromium.org]

[commit-bot@chromium.org via codereview.chromium.org]

Try jobs failed on following builders:

linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_rel_ng/builds/367807)

https://codereview.chromium.org/2698573002/

[commit-bot@chromium.org via codereview.chromium.org]

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #16 (id:290001) as
https://chromium.googlesource.com/chromium/src/+/89ec62330efc014949d78367b94de89c4a7b5405

https://codereview.chromium.org/2698573002/