VR: Avoiding regeneration of RenderText objects for texture rendering

VR: Avoiding regeneration of RenderText objects for texture rendering

- Code is based in gfx::Canvas::DrawStringRectWithFlags, but tailored for VR needs and instead of drawing, it returns a list of RenderText.
- For textures that require text measurement, both the measurement and drawing is done in a single pass.
- Being the text layout the most expensive operation for texture rendering, times are reduced to almost half.

BUG=715591
Review-Url: https://codereview.chromium.org/2877673005
Cr-Commit-Position: refs/heads/master@{#471503}
Committed: https://chromium.googlesource.com/chromium/src/+/b40dc0c1f2fea886a3184e9c3265c9d9f8d7275f

[acondor_, 2017-05-11 20:53:39]

Description was changed from

==========
VR: Avoiding regeneration of RenderText objects for textures' rendering

- Code is based in gfx::Canvas::DrawStringRectWithFlags, but tailored for VR
needs and instead of drawing, it returns a list of RenderText.
- For textures that require text measurement, both the measurement and drawing
is done in a single pass.
- Being the text layout the most expensive operation for texture rendering,
times are reduced to almost half.

BUG=715591
==========

to

==========
VR: Avoiding regeneration of RenderText objects for texture rendering

- Code is based in gfx::Canvas::DrawStringRectWithFlags, but tailored for VR
needs and instead of drawing, it returns a list of RenderText.
- For textures that require text measurement, both the measurement and drawing
is done in a single pass.
- Being the text layout the most expensive operation for texture rendering,
times are reduced to almost half.

BUG=715591
==========

[acondor_, 2017-05-11 20:55:59]

PTAL

[cjgrant, 2017-05-11 21:15:40]

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

[cjgrant, 2017-05-11 21:15:41]

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

https://codereview.chromium.org/2877673005/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/ui_texture.cc:43: if (flags &
UiTexture::TEXT_ALIGN_LEFT)
It looks like RenderText defaults to either RIGHT or LEFT based on its own RTL. 
You could take advantage of that here to only override if RIGHT or CENTER, but
maybe you're choosing to be very explicit here?

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

https://codereview.chromium.org/2877673005/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/ui_texture.h:32: 
Probably no need for either blank line in here.

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

https://codereview.chromium.org/2877673005/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/url_bar_texture.cc:148:
PrepareDrawStringRect(base::UTF8ToUTF16(gurl_.spec()),
Is this the 'git cl format' formatting result?

[acondor_, 2017-05-11 22:26:49]

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

https://codereview.chromium.org/2877673005/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/ui_texture.cc:43: if (flags &
UiTexture::TEXT_ALIGN_LEFT)
On 2017/05/11 21:15:40, cjgrant wrote:
> It looks like RenderText defaults to either RIGHT or LEFT based on its own
RTL. 
> You could take advantage of that here to only override if RIGHT or CENTER, but
> maybe you're choosing to be very explicit here?

Good catch. I'm removing the flag setup in PrepareDrawStringRect, but if the
flag is explicitly set, we should enforce RenderText alignment too.

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

https://codereview.chromium.org/2877673005/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/ui_texture.h:32: 
On 2017/05/11 21:15:40, cjgrant wrote:
> Probably no need for either blank line in here.

Done.

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

https://codereview.chromium.org/2877673005/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/url_bar_texture.cc:148:
PrepareDrawStringRect(base::UTF8ToUTF16(gurl_.spec()),
On 2017/05/11 21:15:41, cjgrant wrote:
> Is this the 'git cl format' formatting result?

Yes.

[mthiesse, 2017-05-12 01:14:37]

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

[mthiesse, 2017-05-12 01:14:38]

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

https://codereview.chromium.org/2877673005/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/insecure_content_permanent_texture.cc:60:
sk_canvas->drawRoundRect(SkRect::MakeWH(size_.width(), size_.height()),
Why the change from canvas->DrawRoundRect?

https://codereview.chromium.org/2877673005/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/insecure_content_permanent_texture.cc:66:
: size_.height() * kBorderFactor,
Shouldn't this be "2 * size_.height() * kBorderFactor" now?

https://codereview.chromium.org/2877673005/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/insecure_content_permanent_texture.cc:75:
(IsRTL() ? kBorderFactor : 2 * kBorderFactor + kIconSizeFactor),
Shouldn't this be "4 * kBorderFactor + kIconSizeFactor" now?

https://codereview.chromium.org/2877673005/diff/1/chrome/browser/android/vr_s...
File
chrome/browser/android/vr_shell/textures/insecure_content_transient_texture.cc
(left):

https://codereview.chromium.org/2877673005/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/insecure_content_transient_texture.cc:58:
DCHECK_LE(size_.height(), max_height);
why remove this DCHECK?

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

https://codereview.chromium.org/2877673005/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/ui_texture.cc:98:
gfx::ELIDE_LONG_WORDS, &strings);
Probably want WRAP_LONG_WORDS here, to align with the non-eliding UX.

https://codereview.chromium.org/2877673005/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/ui_texture.cc:130:
render_text->SetElideBehavior(gfx::FADE_TAIL);
Pretty sure we want gfx::TRUNCATE per UX specs.

[acondor_, 2017-05-12 14:54:23]

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

https://codereview.chromium.org/2877673005/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/insecure_content_permanent_texture.cc:60:
sk_canvas->drawRoundRect(SkRect::MakeWH(size_.width(), size_.height()),
On 2017/05/12 01:14:38, mthiesse wrote:
> Why the change from canvas->DrawRoundRect?

To be compliant with how we are doing the URL bar. The idea is to only use
gfx::Canvas when there's no other option, to avoid it's overhead.

https://codereview.chromium.org/2877673005/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/insecure_content_permanent_texture.cc:66:
: size_.height() * kBorderFactor,
On 2017/05/12 01:14:38, mthiesse wrote:
> Shouldn't this be "2 * size_.height() * kBorderFactor" now?

Actually the RTL should be 4, and the LRT only 1.

https://codereview.chromium.org/2877673005/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/insecure_content_permanent_texture.cc:75:
(IsRTL() ? kBorderFactor : 2 * kBorderFactor + kIconSizeFactor),
On 2017/05/12 01:14:38, mthiesse wrote:
> Shouldn't this be "4 * kBorderFactor + kIconSizeFactor" now?

2 for RTL and 3 for LTR.

https://codereview.chromium.org/2877673005/diff/1/chrome/browser/android/vr_s...
File
chrome/browser/android/vr_shell/textures/insecure_content_transient_texture.cc
(left):

https://codereview.chromium.org/2877673005/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/insecure_content_transient_texture.cc:58:
DCHECK_LE(size_.height(), max_height);
On 2017/05/12 01:14:38, mthiesse wrote:
> why remove this DCHECK?

Because it's kind of innecessary given that we are already dchecking text_size.

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

https://codereview.chromium.org/2877673005/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/ui_texture.cc:98:
gfx::ELIDE_LONG_WORDS, &strings);
On 2017/05/12 01:14:38, mthiesse wrote:
> Probably want WRAP_LONG_WORDS here, to align with the non-eliding UX.

Done.

https://codereview.chromium.org/2877673005/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/ui_texture.cc:130:
render_text->SetElideBehavior(gfx::FADE_TAIL);
On 2017/05/12 01:14:38, mthiesse wrote:
> Pretty sure we want gfx::TRUNCATE per UX specs.

Done.

[mthiesse, 2017-05-12 19:33:00]

lgtm

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

https://codereview.chromium.org/2877673005/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/textures/ui_texture.cc:136: // bringing text
back to the left.
The comment doesn't really make sense. I'd just remove it as it seems obvious
enough from the code.

[acondor_, 2017-05-12 19:36:31]

The CQ bit was checked by acondor@google.com

[acondor_, 2017-05-12 19:36:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
mthiesse@chromium.org
Link to the patchset: https://codereview.chromium.org/2877673005/#ps40001
(title: "removing unnecessary comment")

[commit-bot: I haz the power, 2017-05-12 19:37:13]

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-12 22:34:35]

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

[commit-bot: I haz the power, 2017-05-12 22:34:36]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[acondor_, 2017-05-12 22:49:27]

The CQ bit was checked by acondor@google.com

[commit-bot: I haz the power, 2017-05-12 22:50:12]

CQ is trying da patch.

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

[mthiesse, 2017-05-12 22:57:17]

The CQ bit was checked by mthiesse@chromium.org

[commit-bot: I haz the power, 2017-05-12 22:57:39]

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-13 00:14:31]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494629837093580,
"parent_rev": "4cbfbbad576296603f11bd1f5046a38a5cd7bb20", "commit_rev":
"b40dc0c1f2fea886a3184e9c3265c9d9f8d7275f"}

[commit-bot: I haz the power, 2017-05-13 00:14:43]

Description was changed from

==========
VR: Avoiding regeneration of RenderText objects for texture rendering

- Code is based in gfx::Canvas::DrawStringRectWithFlags, but tailored for VR
needs and instead of drawing, it returns a list of RenderText.
- For textures that require text measurement, both the measurement and drawing
is done in a single pass.
- Being the text layout the most expensive operation for texture rendering,
times are reduced to almost half.

BUG=715591
==========

to

==========
VR: Avoiding regeneration of RenderText objects for texture rendering

- Code is based in gfx::Canvas::DrawStringRectWithFlags, but tailored for VR
needs and instead of drawing, it returns a list of RenderText.
- For textures that require text measurement, both the measurement and drawing
is done in a single pass.
- Being the text layout the most expensive operation for texture rendering,
times are reduced to almost half.

BUG=715591

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

[commit-bot: I haz the power, 2017-05-13 00:14:45]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/b40dc0c1f2fea886a3184e9c3265...