Rendering Insecure WebVR Warnings into a texture with Skia.

Rendering Insecure WebVR Warnings into a texture with Skia.

Icons and rounded backgrounds reuse gfx code.
Text rendering is a temporary solution that cannot handle fallback fonts.
RTL rendering is not handled either.

BUG=710150
Review-Url: https://codereview.chromium.org/2818553002
Cr-Commit-Position: refs/heads/master@{#464523}
Committed: https://chromium.googlesource.com/chromium/src/+/d9cff176581d901a8b2b1fed51035c13418f87b9

[acondor_, 2017-04-12 19:08:14]

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

[acondor_, 2017-04-12 19:08:14]

PTAL

[acondor_, 2017-04-12 19:51:08]

Description was changed from

==========
Rendering Insecure WebVR Warnings into a texture with Skia.

Icons and rounded backgrounds reuse gfx code.
Text rendering is a temporary solution that cannot handle fallback fonts.
RTL rendering is also not handled.

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

to

==========
Rendering Insecure WebVR Warnings into a texture with Skia.

Icons and rounded backgrounds reuse gfx code.
Text rendering is a temporary solution that cannot handle fallback fonts.
RTL rendering is not handled either.

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

[cjgrant, 2017-04-12 20:38:54]

Quick first pass.  Overall, this looks nice and compact.  It'd be nice to see
how you tie it into vr_shell_gl.cc, but that can follow on.

https://codereview.chromium.org/2818553002/diff/1/chrome/browser/android/vr_s...
File
chrome/browser/android/vr_shell/textures/insecure_content_permanent_texture.cc
(right):

https://codereview.chromium.org/2818553002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/insecure_content_permanent_texture.cc:21:
const SkColor kPermanentWarningBgColor = SK_ColorWHITE;
Since this file is permanent warning specific, you can drop prefixes like this -
it could just be BackgroundColor.

https://codereview.chromium.org/2818553002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/textures/ui_texture.h (right):

https://codereview.chromium.org/2818553002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/ui_texture.h:30: gfx::Size GetSize()
const { return size_; }
There's a naming convention for simple getters - I think it's just size() in
this case.

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

https://codereview.chromium.org/2818553002/diff/1/ui/gfx/BUILD.gn#newcode334
ui/gfx/BUILD.gn:334: if (use_aura || !is_ios) {
Should we use the enable_vr flag here instead?

[acondor_, 2017-04-12 21:21:46]

https://codereview.chromium.org/2818553002/diff/1/chrome/browser/android/vr_s...
File
chrome/browser/android/vr_shell/textures/insecure_content_permanent_texture.cc
(right):

https://codereview.chromium.org/2818553002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/insecure_content_permanent_texture.cc:21:
const SkColor kPermanentWarningBgColor = SK_ColorWHITE;
On 2017/04/12 20:38:53, cjgrant wrote:
> Since this file is permanent warning specific, you can drop prefixes like this
-
> it could just be BackgroundColor.

Done.

https://codereview.chromium.org/2818553002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/textures/ui_texture.h (right):

https://codereview.chromium.org/2818553002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/ui_texture.h:30: gfx::Size GetSize()
const { return size_; }
On 2017/04/12 20:38:54, cjgrant wrote:
> There's a naming convention for simple getters - I think it's just size() in
> this case.

Done.

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

https://codereview.chromium.org/2818553002/diff/1/ui/gfx/BUILD.gn#newcode334
ui/gfx/BUILD.gn:334: if (use_aura || !is_ios) {
On 2017/04/12 20:38:54, cjgrant wrote:
> Should we use the enable_vr flag here instead?

Done.

[mthiesse, 2017-04-13 15:31:38]

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

https://codereview.chromium.org/2818553002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/textures/insecure_content_permanent_texture.cc:30:
DCHECK(texture_size_ % 4 == 0);
Add a comment on why this DCHECK is necessary.

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

https://codereview.chromium.org/2818553002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/textures/insecure_content_transient_texture.cc:30:
DCHECK(texture_size_ % 2 == 0);
Comment on why the DCHECK

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

https://codereview.chromium.org/2818553002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/textures/ui_texture.cc:23:
std::vector<std::string> SplitText(const std::string& text,
I think we should land this change without the text rendering for now, as your
follow-on CL will do text rendering properly.

[acondor_, 2017-04-13 15:58:32]

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

[acondor_, 2017-04-13 15:58:33]

+estade please review BUILD.gn in relation to vector_icons
+mthiesse, +cjgrant for vr_shell/

[mthiesse, 2017-04-13 16:01:33]

lgtm

[cjgrant, 2017-04-13 16:04:18]

lgtm

[mthiesse, 2017-04-13 19:19:05]

No need to wait for estade, the BUILD.gn change is trivial and we're already
owners technically.

[acondor_, 2017-04-13 19:19:37]

The CQ bit was checked by acondor@google.com

[commit-bot: I haz the power, 2017-04-13 19:20:22]

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-13 20:20:49]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1492111177728040,
"parent_rev": "af9a9230d0afc8c8ba34f30a2a12fafc3ae144e4", "commit_rev":
"d9cff176581d901a8b2b1fed51035c13418f87b9"}

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

Description was changed from

==========
Rendering Insecure WebVR Warnings into a texture with Skia.

Icons and rounded backgrounds reuse gfx code.
Text rendering is a temporary solution that cannot handle fallback fonts.
RTL rendering is not handled either.

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

to

==========
Rendering Insecure WebVR Warnings into a texture with Skia.

Icons and rounded backgrounds reuse gfx code.
Text rendering is a temporary solution that cannot handle fallback fonts.
RTL rendering is not handled either.

BUG=710150

Review-Url: https://codereview.chromium.org/2818553002
Cr-Commit-Position: refs/heads/master@{#464523}
Committed:
https://chromium.googlesource.com/chromium/src/+/d9cff176581d901a8b2b1fed5103...
==========

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

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/d9cff176581d901a8b2b1fed5103...