[vr] Bail on unhandled code points.

[vr] Bail on unhandled code points.

When we encounter a URL with glyphs we cannot render
we now exit VR mode. This CL also establishes an
"unsupported mode" enumeration and corresponding
metrics.

BUG=722861
Review-Url: https://codereview.chromium.org/2902393002
Cr-Commit-Position: refs/heads/master@{#475222}
Committed: https://chromium.googlesource.com/chromium/src/+/3181fb56c2c8a939c082f4711c985b7123e0b3e8

[Ian Vollick, 2017-05-25 15:20:42]

Description was changed from

==========
[vr] Bail on unhandled code points.

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

to

==========
[vr] Bail on unhandled code points.

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

[acondor_, 2017-05-25 15:51:42]

acondor@google.com changed reviewers:
+ acondor@google.com

[acondor_, 2017-05-25 15:51:42]

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

https://codereview.chromium.org/2902393002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/ui_texture.cc:172: // This happens a
lot?
GetFallbackFontNameForChar assumes that the default_font is not a fallback, so
it wouldn't return it. Now, we might want to change this function to report
whether the empty string was because it was supported by the default font or
because it didn't find any fallback.

[cjgrant, 2017-05-25 17:47:24]

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

[cjgrant, 2017-05-25 17:47:25]

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

https://codereview.chromium.org/2902393002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/ui_texture.cc:165: bool
UiTexture::CheckFontList(int size, base::string16 text) {
I thought this change would use the existing character lookup, but just report a
failure, so the caller can optionally bail.  I ask because I thought these
lookups were slow...

https://codereview.chromium.org/2902393002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/ui_texture.cc:170: std::string name =
GetFallbackFontNameForChar(default_font, c, "");
Up to you, but probably don't need "name";
if (GetFallbackl().empty()) {
would be just as easy to read.

https://codereview.chromium.org/2902393002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/ui_texture.cc:172: // This happens a
lot?
The comment and print look like leftover debug?

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

https://codereview.chromium.org/2902393002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/url_bar_texture_unittest.cc:60:
TEST(UrlBarTexture, ShortIsSweet) {
My CL adds more tests to the same (future) file.
s/ShortIsSweet/ShortUrlIsSupported/, and same with the next test?  Ie. really
describe what is actually being tested.

[Ian Vollick, 2017-05-26 02:08:37]

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

[commit-bot: I haz the power, 2017-05-26 02:09:27]

Dry run: CQ is trying da patch.

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

[Ian Vollick, 2017-05-26 03:36:13]

vollick@chromium.org changed reviewers:
+ isherman@chromium.org

[Ian Vollick, 2017-05-26 03:38:22]

Description was changed from

==========
[vr] Bail on unhandled code points.

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

to

==========
[vr] Bail on unhandled code points.

When we encounter a URL with glyphs we cannot render
we now exit VR mode. This CL also establishes an
"unsupported mode" enumeration and corresponding
metrics.

BUG=722861
==========

[Ian Vollick, 2017-05-26 03:48:10]

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

https://codereview.chromium.org/2902393002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/ui_texture.cc:165: bool
UiTexture::CheckFontList(int size, base::string16 text) {
On 2017/05/25 17:47:24, cjgrant wrote:
> I thought this change would use the existing character lookup, but just report
a
> failure, so the caller can optionally bail.  I ask because I thought these
> lookups were slow...

It's true, they are slow. I may be misunderstanding what you mean by existing
character lookup -- are you suggesting I instrument GetFontList below? (I
started by doing that, then learned that the URL bar texture doesn't use
GetFontList, in uses GetDefaultFontList above, which isn't as easy to piggy back
upon).

https://codereview.chromium.org/2902393002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/ui_texture.cc:170: std::string name =
GetFallbackFontNameForChar(default_font, c, "");
On 2017/05/25 17:47:24, cjgrant wrote:
> Up to you, but probably don't need "name";
> if (GetFallbackl().empty()) {
> would be just as easy to read.

The code ended up being structured differently, and I don't think this comment
applies anymore.

https://codereview.chromium.org/2902393002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/ui_texture.cc:172: // This happens a
lot?
On 2017/05/25 17:47:24, cjgrant wrote:
> The comment and print look like leftover debug?

Yeah, as we chatted about offline, this was in a WIP state (the unit tests,
too).

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

https://codereview.chromium.org/2902393002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/url_bar_texture_unittest.cc:60:
TEST(UrlBarTexture, ShortIsSweet) {
On 2017/05/25 17:47:25, cjgrant wrote:
> My CL adds more tests to the same (future) file.
> s/ShortIsSweet/ShortUrlIsSupported/, and same with the next test?  Ie. really
> describe what is actually being tested.

That name was from another CL, but nevertheless I was being cute and should have
had a more descriptive test name, even there. Hopefully the unit test name in
the latest patch set is better.

[cjgrant, 2017-05-26 18:52:00]

lgtm

See comments just in case they're useful.

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

https://codereview.chromium.org/2902393002/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/textures/url_bar_texture.cc:219:
gurl_render_texts_ = PrepareDrawStringRect(
This needs some non-trivial rebasing, as we don't touch gurl_.spec() anymore. 
The check should be moved to after the new FormatURL call, which returns the
string16 we'll render.

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

https://codereview.chromium.org/2902393002/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/textures/url_bar_texture_unittest.cc:33: void
SetForceFontFallbackFailure(bool force) {
I'm not suggesting a change in this CL, but am curious.  Another common way to
do this testing is to have an alternate test constructor that takes in helper
classes, allowing use of a mock/fake text classes.  We should talk about this
offline, as soon we'll be writing *many* tests, right?  :)

https://codereview.chromium.org/2902393002/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/enums.xml (right):

https://codereview.chromium.org/2902393002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/enums.xml:36528: +  <int value="0" label="Unhandled
code point"/>
Something about font or URL, or is this specific enough?

[Ian Vollick, 2017-05-26 20:20:28]

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

https://codereview.chromium.org/2902393002/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/textures/url_bar_texture.cc:219:
gurl_render_texts_ = PrepareDrawStringRect(
On 2017/05/26 18:52:00, cjgrant wrote:
> This needs some non-trivial rebasing, as we don't touch gurl_.spec() anymore. 
> The check should be moved to after the new FormatURL call, which returns the
> string16 we'll render.

Done. Also, since we're now using GetFontList, I was able to modify that
function instead and get rid of CheckFontList.

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

https://codereview.chromium.org/2902393002/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/textures/url_bar_texture_unittest.cc:33: void
SetForceFontFallbackFailure(bool force) {
On 2017/05/26 18:52:00, cjgrant wrote:
> I'm not suggesting a change in this CL, but am curious.  Another common way to
> do this testing is to have an alternate test constructor that takes in helper
> classes, allowing use of a mock/fake text classes.  We should talk about this
> offline, as soon we'll be writing *many* tests, right?  :)

In this case, I wonder if we can make the static functions on UiTexture member
functions for easier testing? Let's chat about how best to approach this
offline.

https://codereview.chromium.org/2902393002/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/enums.xml (right):

https://codereview.chromium.org/2902393002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/enums.xml:36528: +  <int value="0" label="Unhandled
code point"/>
On 2017/05/26 18:52:00, cjgrant wrote:
> Something about font or URL, or is this specific enough?

Good point. Updated to mention that this is URL-related.

[Ian Vollick, 2017-05-26 20:21:36]

vollick@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Ian Vollick, 2017-05-26 20:24:00]

vollick@chromium.org changed reviewers:
- asvitkine@chromium.org

[Ian Vollick, 2017-05-26 20:24:50]

vollick@chromium.org changed reviewers:
+ mpearson@chromium.org

[Ian Vollick, 2017-05-26 20:25:53]

vollick@chromium.org changed reviewers:
- isherman@chromium.org

[Ian Vollick, 2017-05-26 20:26:17]

On 2017/05/26 20:20:28, Ian Vollick wrote:
>
https://codereview.chromium.org/2902393002/diff/60001/chrome/browser/android/...
> File chrome/browser/android/vr_shell/textures/url_bar_texture.cc (right):
> 
>
https://codereview.chromium.org/2902393002/diff/60001/chrome/browser/android/...
> chrome/browser/android/vr_shell/textures/url_bar_texture.cc:219:
> gurl_render_texts_ = PrepareDrawStringRect(
> On 2017/05/26 18:52:00, cjgrant wrote:
> > This needs some non-trivial rebasing, as we don't touch gurl_.spec()
anymore. 
> > The check should be moved to after the new FormatURL call, which returns the
> > string16 we'll render.
> 
> Done. Also, since we're now using GetFontList, I was able to modify that
> function instead and get rid of CheckFontList.
> 
>
https://codereview.chromium.org/2902393002/diff/60001/chrome/browser/android/...
> File chrome/browser/android/vr_shell/textures/url_bar_texture_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/2902393002/diff/60001/chrome/browser/android/...
> chrome/browser/android/vr_shell/textures/url_bar_texture_unittest.cc:33: void
> SetForceFontFallbackFailure(bool force) {
> On 2017/05/26 18:52:00, cjgrant wrote:
> > I'm not suggesting a change in this CL, but am curious.  Another common way
to
> > do this testing is to have an alternate test constructor that takes in
helper
> > classes, allowing use of a mock/fake text classes.  We should talk about
this
> > offline, as soon we'll be writing *many* tests, right?  :)
> 
> In this case, I wonder if we can make the static functions on UiTexture member
> functions for easier testing? Let's chat about how best to approach this
> offline.
> 
>
https://codereview.chromium.org/2902393002/diff/60001/tools/metrics/histogram...
> File tools/metrics/histograms/enums.xml (right):
> 
>
https://codereview.chromium.org/2902393002/diff/60001/tools/metrics/histogram...
> tools/metrics/histograms/enums.xml:36528: +  <int value="0" label="Unhandled
> code point"/>
> On 2017/05/26 18:52:00, cjgrant wrote:
> > Something about font or URL, or is this specific enough?
> 
> Good point. Updated to mention that this is URL-related.

+mpearson for tools/metrics/

[amp, 2017-05-26 20:37:38]

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

[amp, 2017-05-26 20:37:39]

lgtm

https://codereview.chromium.org/2902393002/diff/80001/chrome/browser/android/...
File chrome/browser/android/vr_shell/font_fallback.h (right):

https://codereview.chromium.org/2902393002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/font_fallback.h:25: // The funtion, if it
succeeds, sets |font_name|. Even if it succeeds, it may
s/funtion/function

[Ilya Sherman, 2017-05-26 22:19:50]

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

[Ilya Sherman, 2017-05-26 22:19:51]

Metrics LGTM % nits:

https://codereview.chromium.org/2902393002/diff/80001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_unsupported_mode.h (right):

https://codereview.chromium.org/2902393002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_unsupported_mode.h:10: // Ensure that this
stays in sync with VRUnsupportedMode in enums.xml
Please document that this enum is used to back an UMA histogram, and should
therefore be treated as append-only.

https://codereview.chromium.org/2902393002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_unsupported_mode.h:11: enum UiUnsupportedMode
: int {
nit: Mebbe enum class?

https://codereview.chromium.org/2902393002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_unsupported_mode.h:15: kMax,
Optional nit: Usually, "max" is used as an alias for the last item in an enum,
whereas for histograms we really want more of a "count".  So, I'd suggest
renaming this to "kCount".

https://codereview.chromium.org/2902393002/diff/80001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2902393002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:90: static const char*
kUnsupportedModeHistogramName =
nit: no need for "static" in the anonymous namespace.

https://codereview.chromium.org/2902393002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:90: static const char*
kUnsupportedModeHistogramName =
nit: const char kUnsupportedModeHistogramName[] =

(and possibly "constexpr" in place of "const" -- I'm not sure whether there's
any reason to prefer one or the other in a case like this)

[Mark P, 2017-05-26 22:29:18]

https://codereview.chromium.org/2902393002/diff/80001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_unsupported_mode.h (right):

https://codereview.chromium.org/2902393002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_unsupported_mode.h:10: // Ensure that this
stays in sync with VRUnsupportedMode in enums.xml
On 2017/05/26 22:19:51, Ilya Sherman wrote:
> Please document that this enum is used to back an UMA histogram, and should
> therefore be treated as append-only.

Here's a good comment fragment:
// These values are written to logs.  New enum values can be added, but existing
// enums must never be renumbered or deleted and reused.

https://codereview.chromium.org/2902393002/diff/80001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2902393002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:90: static const char*
kUnsupportedModeHistogramName =
On 2017/05/26 22:19:51, Ilya Sherman wrote:
> nit: const char kUnsupportedModeHistogramName[] =
> 
> (and possibly "constexpr" in place of "const" -- I'm not sure whether there's
> any reason to prefer one or the other in a case like this)

If it's only used in one place, why not use the "string" in that place?

[Ian Vollick, 2017-05-27 00:02:44]

https://codereview.chromium.org/2902393002/diff/80001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_unsupported_mode.h (right):

https://codereview.chromium.org/2902393002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_unsupported_mode.h:10: // Ensure that this
stays in sync with VRUnsupportedMode in enums.xml
On 2017/05/26 22:29:18, Mark P wrote:
> On 2017/05/26 22:19:51, Ilya Sherman wrote:
> > Please document that this enum is used to back an UMA histogram, and should
> > therefore be treated as append-only.
> 
> Here's a good comment fragment:
> // These values are written to logs.  New enum values can be added, but
existing
> // enums must never be renumbered or deleted and reused.

Sounds great - I've used it directly. Thanks.

https://codereview.chromium.org/2902393002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_unsupported_mode.h:10: // Ensure that this
stays in sync with VRUnsupportedMode in enums.xml
On 2017/05/26 22:19:51, Ilya Sherman wrote:
> Please document that this enum is used to back an UMA histogram, and should
> therefore be treated as append-only.

Done.

https://codereview.chromium.org/2902393002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_unsupported_mode.h:11: enum UiUnsupportedMode
: int {
On 2017/05/26 22:19:51, Ilya Sherman wrote:
> nit: Mebbe enum class?

Done.

https://codereview.chromium.org/2902393002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_unsupported_mode.h:15: kMax,
On 2017/05/26 22:19:51, Ilya Sherman wrote:
> Optional nit: Usually, "max" is used as an alias for the last item in an enum,
> whereas for histograms we really want more of a "count".  So, I'd suggest
> renaming this to "kCount".

Done.

https://codereview.chromium.org/2902393002/diff/80001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2902393002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:90: static const char*
kUnsupportedModeHistogramName =
On 2017/05/26 22:19:51, Ilya Sherman wrote:
> nit: no need for "static" in the anonymous namespace.

Removed as per mpearson's comment.

https://codereview.chromium.org/2902393002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:90: static const char*
kUnsupportedModeHistogramName =
On 2017/05/26 22:19:51, Ilya Sherman wrote:
> nit: const char kUnsupportedModeHistogramName[] =
> 
> (and possibly "constexpr" in place of "const" -- I'm not sure whether there's
> any reason to prefer one or the other in a case like this)

Nuked.

https://codereview.chromium.org/2902393002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:90: static const char*
kUnsupportedModeHistogramName =
On 2017/05/26 22:29:18, Mark P wrote:
> On 2017/05/26 22:19:51, Ilya Sherman wrote:
> > nit: const char kUnsupportedModeHistogramName[] =
> > 
> > (and possibly "constexpr" in place of "const" -- I'm not sure whether
there's
> > any reason to prefer one or the other in a case like this)
> 
> If it's only used in one place, why not use the "string" in that place?

Done.

[Ian Vollick, 2017-05-27 00:03:14]

The CQ bit was checked by vollick@chromium.org

[Ian Vollick, 2017-05-27 00:03:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
cjgrant@chromium.org, isherman@chromium.org, amp@chromium.org
Link to the patchset: https://codereview.chromium.org/2902393002/#ps120001
(title: ".")

[commit-bot: I haz the power, 2017-05-27 00:03:36]

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-05-27 00:13:45]

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

[commit-bot: I haz the power, 2017-05-27 00:13:46]

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

[Ian Vollick, 2017-05-27 01:29:44]

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

[commit-bot: I haz the power, 2017-05-27 01:29:57]

Dry run: CQ is trying da patch.

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

[Ian Vollick, 2017-05-27 02:16:52]

The CQ bit was unchecked by vollick@chromium.org

[Ian Vollick, 2017-05-27 02:16:53]

The CQ bit was checked by vollick@chromium.org

[commit-bot: I haz the power, 2017-05-27 02:17:02]

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-05-27 02:34:41]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1495851413075650,
"parent_rev": "7b3779b51c2acb9f65812723406c28d72fb6e26f", "commit_rev":
"3181fb56c2c8a939c082f4711c985b7123e0b3e8"}

[commit-bot: I haz the power, 2017-05-27 02:34:53]

Description was changed from

==========
[vr] Bail on unhandled code points.

When we encounter a URL with glyphs we cannot render
we now exit VR mode. This CL also establishes an
"unsupported mode" enumeration and corresponding
metrics.

BUG=722861
==========

to

==========
[vr] Bail on unhandled code points.

When we encounter a URL with glyphs we cannot render
we now exit VR mode. This CL also establishes an
"unsupported mode" enumeration and corresponding
metrics.

BUG=722861

Review-Url: https://codereview.chromium.org/2902393002
Cr-Commit-Position: refs/heads/master@{#475222}
Committed:
https://chromium.googlesource.com/chromium/src/+/3181fb56c2c8a939c082f4711c98...
==========

[commit-bot: I haz the power, 2017-05-27 02:34:54]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/3181fb56c2c8a939c082f4711c98...