Rendering text on insecure-site warnings for WebVR

When presenting WebVR on insecure sites, a warning needs to be presented to the user. This change renders these messages on textures to be added to a GL scene.

- Text is rendered with gfx::Canvas and gfx::RenderText, previously disabled for android.
- Fallback fonts were handled with Skia's font manager.
- The text is measured before rendering in order to layout properly, reducing empty space.
- The icons are positioned on the right on RTL languages.

BUG=713779
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2817663003
Cr-Commit-Position: refs/heads/master@{#466472}
Committed: https://chromium.googlesource.com/chromium/src/+/423d7c1b4001927a1a6f6180aa0f3eebe72348e4

[acondor_, 2017-04-12 22:34:58]

Description was changed from

==========
Building RenderText in Android for VR

BUG=
==========

to

==========
We need to use RenderText capabilities to render text into the textures that
will be used in VR Shell.

BUG=
==========

[acondor_, 2017-04-12 22:41:36]

Description was changed from

==========
We need to use RenderText capabilities to render text into the textures that
will be used in VR Shell.

BUG=
==========

to

==========
We need to use RenderText capabilities to render text into the textures that
will be used in VR Shell.

BUG=704937
==========

[acondor_, 2017-04-13 14:00:36]

Description was changed from

==========
We need to use RenderText capabilities to render text into the textures that
will be used in VR Shell.

BUG=704937
==========

to

==========
We need to use RenderText capabilities to render text into the textures that
will be used in VR Shell.

BUG=710150
==========

[acondor_, 2017-04-13 14:00:51]

https://codereview.chromium.org/2817663003/diff/1/ui/gfx/font_render_params_a...
File ui/gfx/font_render_params_android.cc (left):

https://codereview.chromium.org/2817663003/diff/1/ui/gfx/font_render_params_a...
ui/gfx/font_render_params_android.cc:30: 
Implementation taken from ui/gfx/font_render_params_linux.cc

[mthiesse, 2017-04-13 14:16:51]

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

[mthiesse, 2017-04-13 14:16:51]

We'll see what owners think, but I think we can safely remove the use_aura flags
when they're already enclosed in an is_android flag, as aura on Android is
pretty dead.

Also, it might make sense to just always enable it on Android, not behind an
enable_vr flag? Again, we should see what owners think.

https://codereview.chromium.org/2817663003/diff/20001/ui/gfx/font_render_para...
File ui/gfx/font_render_params.h (right):

https://codereview.chromium.org/2817663003/diff/20001/ui/gfx/font_render_para...
ui/gfx/font_render_params.h:115: #if defined(OS_CHROMEOS) || defined(OS_LINUX)
|| defined(OS_ANDROID)
You would need to also include ENABLE_VR here or you'll get compile errors when
ENABLE_VR is off.

[sadrul, 2017-04-13 14:57:55]

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

[sadrul, 2017-04-13 14:57:55]

https://codereview.chromium.org/2817663003/diff/20001/ui/gfx/BUILD.gn
File ui/gfx/BUILD.gn (right):

https://codereview.chromium.org/2817663003/diff/20001/ui/gfx/BUILD.gn#newcode263
ui/gfx/BUILD.gn:263: if (use_aura || is_mac || (is_android && enable_vr)) {
I think you can just do 'enable_vr' instead of 'is_android && enable_vr', right?

https://codereview.chromium.org/2817663003/diff/20001/ui/gfx/BUILD.gn#newcode285
ui/gfx/BUILD.gn:285: if (is_android && (use_aura || enable_vr)) {
As mthiesse@ suggested, you can get rid of the use_aura check here and below
(~309)

https://codereview.chromium.org/2817663003/diff/20001/ui/gfx/font_render_para...
File ui/gfx/font_render_params.h (right):

https://codereview.chromium.org/2817663003/diff/20001/ui/gfx/font_render_para...
ui/gfx/font_render_params.h:115: #if defined(OS_CHROMEOS) || defined(OS_LINUX)
|| defined(OS_ANDROID)
On 2017/04/13 14:16:51, mthiesse wrote:
> You would need to also include ENABLE_VR here or you'll get compile errors
when
> ENABLE_VR is off.

I think if you add ENABLE_VR, then you won't need OS_ANDROID?

https://codereview.chromium.org/2817663003/diff/20001/ui/gfx/font_render_para...
ui/gfx/font_render_params.h:121: GFX_EXPORT void
SetFontRenderParamsDeviceScaleFactor(
It doesn't look like there's any android code that makes this call. So fonts
probably won't be using the right scale-factor I think, unless you add this call
from somewhere? (I am just pointing this out, you can add the code to set the
correct DSF in a separate CL)

[acondor_, 2017-04-18 20:43:13]

Description was changed from

==========
We need to use RenderText capabilities to render text into the textures that
will be used in VR Shell.

BUG=710150
==========

to

==========
- Text is rendered with gfx::Canvas and gfx::RenderText.
- Fallback fonts were handled with Skia's font manager.

BUG=710150
==========

[acondor_, 2017-04-18 20:59:04]

acondor@google.com changed reviewers:
+ cjgrant@chromium.org

[acondor_, 2017-04-18 20:59:04]

PTAL
+mthiesse +cjgrant for vr_shell/
+sadrul for gfx/

https://codereview.chromium.org/2817663003/diff/20001/ui/gfx/BUILD.gn
File ui/gfx/BUILD.gn (right):

https://codereview.chromium.org/2817663003/diff/20001/ui/gfx/BUILD.gn#newcode263
ui/gfx/BUILD.gn:263: if (use_aura || is_mac || (is_android && enable_vr)) {
On 2017/04/13 14:57:55, sadrul wrote:
> I think you can just do 'enable_vr' instead of 'is_android && enable_vr',
right?

Acknowledged.

https://codereview.chromium.org/2817663003/diff/20001/ui/gfx/BUILD.gn#newcode285
ui/gfx/BUILD.gn:285: if (is_android && (use_aura || enable_vr)) {
On 2017/04/13 14:57:55, sadrul wrote:
> As mthiesse@ suggested, you can get rid of the use_aura check here and below
> (~309)

Done.

https://codereview.chromium.org/2817663003/diff/20001/ui/gfx/font_render_para...
File ui/gfx/font_render_params.h (right):

https://codereview.chromium.org/2817663003/diff/20001/ui/gfx/font_render_para...
ui/gfx/font_render_params.h:115: #if defined(OS_CHROMEOS) || defined(OS_LINUX)
|| defined(OS_ANDROID)
On 2017/04/13 14:57:55, sadrul wrote:
> On 2017/04/13 14:16:51, mthiesse wrote:
> > You would need to also include ENABLE_VR here or you'll get compile errors
> when
> > ENABLE_VR is off.
> 
> I think if you add ENABLE_VR, then you won't need OS_ANDROID?

Added ENABLE_VR check. mthiesse, what are your thoughts on removing OS_ANDROID?

https://codereview.chromium.org/2817663003/diff/20001/ui/gfx/font_render_para...
ui/gfx/font_render_params.h:121: GFX_EXPORT void
SetFontRenderParamsDeviceScaleFactor(
On 2017/04/13 14:57:55, sadrul wrote:
> It doesn't look like there's any android code that makes this call. So fonts
> probably won't be using the right scale-factor I think, unless you add this
call
> from somewhere? (I am just pointing this out, you can add the code to set the
> correct DSF in a separate CL)

Thanks. Added a TODO for that on vr_shell.cc

[mthiesse, 2017-04-18 21:30:36]

https://codereview.chromium.org/2817663003/diff/60001/chrome/browser/android/...
File
chrome/browser/android/vr_shell/textures/insecure_content_permanent_texture.cc
(right):

https://codereview.chromium.org/2817663003/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/textures/insecure_content_permanent_texture.cc:58:
text, GetFontList(height_ * 0.3, text), kForegroundColor,
Can you make constants for the magic numbers (like 0.3, 2.9, etc.) and briefly
explain where they come from?

https://codereview.chromium.org/2817663003/diff/60001/chrome/browser/android/...
File
chrome/browser/android/vr_shell/textures/insecure_content_transient_texture.cc
(right):

https://codereview.chromium.org/2817663003/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/textures/insecure_content_transient_texture.cc:57:
text, GetFontList(height_ * 0.1, text), kForegroundColor,
Constants for these magic numbers too please.

https://codereview.chromium.org/2817663003/diff/60001/chrome/browser/android/...
File chrome/browser/android/vr_shell/textures/ui_texture.cc (right):

https://codereview.chromium.org/2817663003/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/textures/ui_texture.cc:58: gfx::FontList
UITexture::GetFontList(int size, base::string16 text) {
This feels like code that shouldn't be specific to vr_shell. Add a TODO to find
a better location for it?

[acondor_, 2017-04-18 23:48:46]

https://codereview.chromium.org/2817663003/diff/60001/chrome/browser/android/...
File
chrome/browser/android/vr_shell/textures/insecure_content_permanent_texture.cc
(right):

https://codereview.chromium.org/2817663003/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/textures/insecure_content_permanent_texture.cc:58:
text, GetFontList(height_ * 0.3, text), kForegroundColor,
On 2017/04/18 21:30:35, mthiesse wrote:
> Can you make constants for the magic numbers (like 0.3, 2.9, etc.) and briefly
> explain where they come from?

Done.

https://codereview.chromium.org/2817663003/diff/60001/chrome/browser/android/...
File
chrome/browser/android/vr_shell/textures/insecure_content_transient_texture.cc
(right):

https://codereview.chromium.org/2817663003/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/textures/insecure_content_transient_texture.cc:57:
text, GetFontList(height_ * 0.1, text), kForegroundColor,
On 2017/04/18 21:30:35, mthiesse wrote:
> Constants for these magic numbers too please.

Done.

https://codereview.chromium.org/2817663003/diff/60001/chrome/browser/android/...
File chrome/browser/android/vr_shell/textures/ui_texture.cc (right):

https://codereview.chromium.org/2817663003/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/textures/ui_texture.cc:58: gfx::FontList
UITexture::GetFontList(int size, base::string16 text) {
On 2017/04/18 21:30:35, mthiesse wrote:
> This feels like code that shouldn't be specific to vr_shell. Add a TODO to
find
> a better location for it?

Added a TODO. It should be gfx::GetFallbackFonts, but it doesn't accept
characters as input. Blink's code for fallback fonts also uses Skia Font
Manager, but it also uses a character to get it. On the other hand,
gfx::GetFallbackFonts uses fontconfig on linux. So, we have to decide if we want
to port this approach to android or stay with Blink's instead.

[mthiesse, 2017-04-19 14:18:56]

lgtm

[mthiesse, 2017-04-20 16:30:45]

Bit of a strange request, but since Chrome release folks care more about number
of CLs than granularity of CLs, and we want to merge this back to 59, could you
merge your CL https://codereview.chromium.org/2826173002/ into this one? I'll
re-review after.

[mthiesse, 2017-04-20 16:32:55]

Also, could you please create a new bug for this CL to make tracking the merge
easier?

[acondor_, 2017-04-20 17:37:17]

Description was changed from

==========
- Text is rendered with gfx::Canvas and gfx::RenderText.
- Fallback fonts were handled with Skia's font manager.

BUG=710150
==========

to

==========
- Text is rendered with gfx::Canvas and gfx::RenderText.
- Fallback fonts were handled with Skia's font manager.
- The text is measured before rendering to avoid empty space.
- The icons are positioned on the right on RTL languages.

BUG=713779
==========

[acondor_, 2017-04-20 17:38:08]

On 2017/04/20 16:32:55, mthiesse wrote:
> Also, could you please create a new bug for this CL to make tracking the merge
> easier?

Merged, created the bug and edited the description.

[mthiesse, 2017-04-21 00:03:12]

lgtm

https://codereview.chromium.org/2817663003/diff/100001/chrome/browser/android...
File
chrome/browser/android/vr_shell/textures/insecure_content_permanent_texture.cc
(right):

https://codereview.chromium.org/2817663003/diff/100001/chrome/browser/android...
chrome/browser/android/vr_shell/textures/insecure_content_permanent_texture.cc:76:
text, GetFontList(height_ * kFontSizeFactor, text), kForegroundColor,
reuse |fonts|

[mthiesse, 2017-04-21 00:12:24]

https://codereview.chromium.org/2817663003/diff/100001/chrome/browser/android...
File
chrome/browser/android/vr_shell/textures/insecure_content_permanent_texture.cc
(right):

https://codereview.chromium.org/2817663003/diff/100001/chrome/browser/android...
chrome/browser/android/vr_shell/textures/insecure_content_permanent_texture.cc:53:
canvas->SizeStringInt(text, fonts, &text_width, &text_height, 0, text_flags);
Canvas::SizeStringInt is a static function

https://codereview.chromium.org/2817663003/diff/100001/chrome/browser/android...
File
chrome/browser/android/vr_shell/textures/insecure_content_transient_texture.cc
(right):

https://codereview.chromium.org/2817663003/diff/100001/chrome/browser/android...
chrome/browser/android/vr_shell/textures/insecure_content_transient_texture.cc:50:
canvas->SizeStringInt(text, fonts, &text_width, &text_height, 0, text_flags);
Canvas::SizeStringInt is a static function

[sadrul, 2017-04-21 03:36:42]

Can you sanitize the CL title/description a little bit? It's not making a lot of
sense right now.

Code change lgtm

[acondor_, 2017-04-21 14:20:18]

Description was changed from

==========
- Text is rendered with gfx::Canvas and gfx::RenderText.
- Fallback fonts were handled with Skia's font manager.
- The text is measured before rendering to avoid empty space.
- The icons are positioned on the right on RTL languages.

BUG=713779
==========

to

==========
When presenting WebVR on insecure sites, a warning needs to be presented to the
user. This change renders these messages on textures to be added to a GL scene.

- Text is rendered with gfx::Canvas and gfx::RenderText, previously disabled for
android.
- Fallback fonts were handled with Skia's font manager.
- The text is measured before rendering in order to layout properly, reducing
empty space.
- The icons are positioned on the right on RTL languages.

BUG=713779
==========

[acondor_, 2017-04-21 14:22:27]

The CQ bit was checked by acondor@google.com

[commit-bot: I haz the power, 2017-04-21 14:22:43]

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

[acondor_, 2017-04-21 14:23:05]

The CQ bit was unchecked by acondor@google.com

[acondor_, 2017-04-21 14:23:31]

The CQ bit was checked by acondor@google.com

[acondor_, 2017-04-21 14:23:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
mthiesse@chromium.org, sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/2817663003/#ps120001
(title: "static functions usage")

[commit-bot: I haz the power, 2017-04-21 14:23:54]

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, 2017-04-21 14:31:04]

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

[commit-bot: I haz the power, 2017-04-21 14:31:05]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[acondor_, 2017-04-21 17:54:08]

Description was changed from

==========
When presenting WebVR on insecure sites, a warning needs to be presented to the
user. This change renders these messages on textures to be added to a GL scene.

- Text is rendered with gfx::Canvas and gfx::RenderText, previously disabled for
android.
- Fallback fonts were handled with Skia's font manager.
- The text is measured before rendering in order to layout properly, reducing
empty space.
- The icons are positioned on the right on RTL languages.

BUG=713779
==========

to

==========
When presenting WebVR on insecure sites, a warning needs to be presented to the
user. This change renders these messages on textures to be added to a GL scene.

- Text is rendered with gfx::Canvas and gfx::RenderText, previously disabled for
android.
- Fallback fonts were handled with Skia's font manager.
- The text is measured before rendering in order to layout properly, reducing
empty space.
- The icons are positioned on the right on RTL languages.

BUG=713779
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[acondor_, 2017-04-21 18:00:49]

Patchset #8 (id:140001) has been deleted

[acondor_, 2017-04-21 18:25:12]

Patchset #8 (id:160001) has been deleted

[acondor_, 2017-04-21 19:06:49]

Patchset #9 (id:200001) has been deleted

[acondor_, 2017-04-21 19:06:59]

Patchset #8 (id:180001) has been deleted

[acondor_, 2017-04-21 19:24:49]

Patchset #8 (id:210001) has been deleted

[acondor_, 2017-04-21 19:35:28]

Patchset #8 (id:230001) has been deleted

[acondor_, 2017-04-21 19:40:55]

Patchset #8 (id:250001) has been deleted

[acondor_, 2017-04-21 19:59:13]

Patchset #8 (id:270001) has been deleted

[acondor_, 2017-04-21 20:03:52]

The CQ bit was checked by acondor@google.com

[acondor_, 2017-04-21 20:03:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
mthiesse@chromium.org, sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/2817663003/#ps290001
(title: "build workaround")

[commit-bot: I haz the power, 2017-04-21 20:04:12]

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, 2017-04-21 20:40:41]

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

[commit-bot: I haz the power, 2017-04-21 20:40:42]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[acondor_, 2017-04-21 21:35:30]

The CQ bit was checked by acondor@google.com

[commit-bot: I haz the power, 2017-04-21 21:36:00]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sadrul, 2017-04-21 22:31:47]

https://codereview.chromium.org/2817663003/diff/290001/ui/gfx/BUILD.gn
File ui/gfx/BUILD.gn (right):

https://codereview.chromium.org/2817663003/diff/290001/ui/gfx/BUILD.gn#newcod...
ui/gfx/BUILD.gn:258: "//device/vr:features",
Actually, can you move this down, in (is_android && enable_vr) block?

[commit-bot: I haz the power, 2017-04-21 22:37:51]

CQ is committing da patch.

Bot data: {"patchset_id": 290001, "attempt_start_ts": 1492810530087690,
"parent_rev": "5c5337d8b474a6d07320536d0bab8bfbf548c219", "commit_rev":
"423d7c1b4001927a1a6f6180aa0f3eebe72348e4"}

[commit-bot: I haz the power, 2017-04-21 22:38:04]

Description was changed from

==========
When presenting WebVR on insecure sites, a warning needs to be presented to the
user. This change renders these messages on textures to be added to a GL scene.

- Text is rendered with gfx::Canvas and gfx::RenderText, previously disabled for
android.
- Fallback fonts were handled with Skia's font manager.
- The text is measured before rendering in order to layout properly, reducing
empty space.
- The icons are positioned on the right on RTL languages.

BUG=713779
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
When presenting WebVR on insecure sites, a warning needs to be presented to the
user. This change renders these messages on textures to be added to a GL scene.

- Text is rendered with gfx::Canvas and gfx::RenderText, previously disabled for
android.
- Fallback fonts were handled with Skia's font manager.
- The text is measured before rendering in order to layout properly, reducing
empty space.
- The icons are positioned on the right on RTL languages.

BUG=713779
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2817663003
Cr-Commit-Position: refs/heads/master@{#466472}
Committed:
https://chromium.googlesource.com/chromium/src/+/423d7c1b4001927a1a6f6180aa0f...
==========

[commit-bot: I haz the power, 2017-04-21 22:38:06]

Committed patchset #8 (id:290001) as
https://chromium.googlesource.com/chromium/src/+/423d7c1b4001927a1a6f6180aa0f...

[acondor_, 2017-04-24 14:28:21]

https://codereview.chromium.org/2817663003/diff/290001/ui/gfx/BUILD.gn
File ui/gfx/BUILD.gn (right):

https://codereview.chromium.org/2817663003/diff/290001/ui/gfx/BUILD.gn#newcod...
ui/gfx/BUILD.gn:258: "//device/vr:features",
On 2017/04/21 22:31:47, sadrul wrote:
> Actually, can you move this down, in (is_android && enable_vr) block?

"features" is a header that defines ENABLE_VR, so it is in fact needed in all
platforms.

[sadrul, 2017-04-24 14:31:50]

https://codereview.chromium.org/2817663003/diff/290001/ui/gfx/BUILD.gn
File ui/gfx/BUILD.gn (right):

https://codereview.chromium.org/2817663003/diff/290001/ui/gfx/BUILD.gn#newcod...
ui/gfx/BUILD.gn:258: "//device/vr:features",
On 2017/04/24 14:28:21, acondor wrote:
> On 2017/04/21 22:31:47, sadrul wrote:
> > Actually, can you move this down, in (is_android && enable_vr) block?
> 
> "features" is a header that defines ENABLE_VR, so it is in fact needed in all
> platforms.

I see. Maybe that header could move into an OS_ANDROID check?

Do you know whether/when we plan on supporting |enable_vr| on non-android
platforms? If it is on the plans, then this is ok. If not, some of the following
could also be simplified (e.g. 'is_android && enable_vr' could just be
'enable_vr')

[mthiesse, 2017-04-24 15:31:27]

On 2017/04/24 14:31:50, sadrul wrote:
> https://codereview.chromium.org/2817663003/diff/290001/ui/gfx/BUILD.gn
> File ui/gfx/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2817663003/diff/290001/ui/gfx/BUILD.gn#newcod...
> ui/gfx/BUILD.gn:258: "//device/vr:features",
> On 2017/04/24 14:28:21, acondor wrote:
> > On 2017/04/21 22:31:47, sadrul wrote:
> > > Actually, can you move this down, in (is_android && enable_vr) block?
> > 
> > "features" is a header that defines ENABLE_VR, so it is in fact needed in
all
> > platforms.
> 
> I see. Maybe that header could move into an OS_ANDROID check?
> 
> Do you know whether/when we plan on supporting |enable_vr| on non-android
> platforms? If it is on the plans, then this is ok. If not, some of the
following
> could also be simplified (e.g. 'is_android && enable_vr' could just be
> 'enable_vr')

We do plan on supporting non-android platforms in the near future.

[mthiesse, 2017-04-24 15:31:29]

On 2017/04/24 14:31:50, sadrul wrote:
> https://codereview.chromium.org/2817663003/diff/290001/ui/gfx/BUILD.gn
> File ui/gfx/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2817663003/diff/290001/ui/gfx/BUILD.gn#newcod...
> ui/gfx/BUILD.gn:258: "//device/vr:features",
> On 2017/04/24 14:28:21, acondor wrote:
> > On 2017/04/21 22:31:47, sadrul wrote:
> > > Actually, can you move this down, in (is_android && enable_vr) block?
> > 
> > "features" is a header that defines ENABLE_VR, so it is in fact needed in
all
> > platforms.
> 
> I see. Maybe that header could move into an OS_ANDROID check?
> 
> Do you know whether/when we plan on supporting |enable_vr| on non-android
> platforms? If it is on the plans, then this is ok. If not, some of the
following
> could also be simplified (e.g. 'is_android && enable_vr' could just be
> 'enable_vr')

We do plan on supporting non-android platforms in the near future.