All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=714886

Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDgoueQmgoM


Bot(s) for this bug's original alert(s):

Android Builder

Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8981386598239794560

=== Auto-CCing suspected CL author acondor@google.com ===

Hi acondor@google.com, the bisect results pointed to your CL, please take a look at the
results.


=== BISECT JOB RESULTS ===
Perf regression found with culprit

Suspected Commit
  Author : acondor
  Commit : 423d7c1b4001927a1a6f6180aa0f3eebe72348e4
  Date   : Fri Apr 21 22:37:48 2017
  Subject: 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.

Bisect Details
  Configuration: android_nexus7_perf_bisect
  Benchmark    : resource_sizes
  Metric       : MonochromePublic.apk_Specifics/normalized apk size
  Change       : 0.09% | 73591590.0 -> 73657817.0

Revision             Result               N
chromium@466471      73591590 +- 0.0      6      good
chromium@466472      73657817 +- 0.0      6      bad       <--

To Run This Test
  src/build/android/resource_sizes.py --chromium-output-directory {CHROMIUM_OUTPUT_DIR} --chartjson {CHROMIUM_OUTPUT_DIR}/apks/MonochromePublic.apk

Debug Info
  https://chromeperf.appspot.com/buildbucket_job_status/8981386598239794560

Is this bisect wrong?
  https://chromeperf.appspot.com/bad_bisect?try_job_id=5782702399160320


| O O | Visit http://www.chromium.org/developers/speed-infra/perf-bug-faq
|  X  | for more information addressing perf regression bugs. For feedback,
| / \ | file a bug with component Speed>Bisection.  Thank you!

I'm checking now. Thanks for the info.

Partial SymbolDiff via //tools/binary_size:

Are all of these symbols actually needed?

Showing 314 symbols with total size: 56430 bytes
.text=54.5kb     .rodata=256 bytes  other=352 bytes  total=55.1kb
Number of object files: 42

First columns are: running total, type, size
+     2276 t@0x787118   2276    ui/gfx/gfx/render_text_harfbuzz.o
               gfx::RenderTextHarfBuzz::EnsureLayout
+     4136 t@0x786730   1860    ui/gfx/gfx/render_text_harfbuzz.o
               gfx::RenderTextHarfBuzz::ShapeRun
+     5700 t@0x78b738   1564    ui/gfx/gfx/render_text.o
               gfx::RenderText::Elide
+     7168 t@0x7837b0   1468    ui/gfx/gfx/render_text_harfbuzz.o
               std::__ndk1::__tree<gfx::Font, gfx::CaseInsensitiveCompare, std::__ndk1::allocator<gfx::Font> >::destroy
+     8616 t@0x78060c   1448    ui/gfx/gfx/text_elider.o
               gfx::ElideRectangleText
+     9840 t@0x782f2c   1224    ui/gfx/gfx/render_text_harfbuzz.o
               std::__ndk1::__sort<gfx::HarfBuzzLineBreaker::AdvanceLine()::{lambda(gfx::internal::LineSegment const&, gfx::internal::LineSegment const&)#1}&, gfx::internal::LineSegment*>
+    11028 t@0x77be4c   1188    ui/gfx/gfx/font_list.o
               gfx::FontList::ParseDescription
+    12124 t@0x785810   1096    ui/gfx/gfx/render_text_harfbuzz.o
               gfx::RenderTextHarfBuzz::ItemizeTextToRuns
+    13192 t@0x78af00   1068    ui/gfx/gfx/render_text.o
               gfx::RenderText::OnTextAttributeChanged
+    14248 t@0x7841ac   1056    ui/gfx/gfx/render_text_harfbuzz.o
               gfx::RenderTextHarfBuzz::DrawVisualText
+    15256 t@0x780148   1008    ui/gfx/gfx/text_elider.o
               gfx::ElideText
+    16120 t@0x781744   864     ui/gfx/gfx/canvas_skia.o
               gfx::Canvas::SizeStringFloat
+    16964 t@0x7894dc   844     ui/gfx/gfx/render_text.o
               gfx::RenderText::ApplyFadeEffects
+    17800 t@0x785344   836     ui/gfx/gfx/render_text_harfbuzz.o
               gfx::HarfBuzzLineBreaker::AddLineSegment
+    18588 t@0x786088   788     ui/gfx/gfx/render_text_harfbuzz.o
               gfx::RenderTextHarfBuzz::ShapeRunWithFont
+    19352 t@0x78c05c   764     ui/gfx/gfx/render_text.o
               gfx::RenderText::UpdateDisplayText
+    20104 t@0x781aa4   752     ui/gfx/gfx/canvas_skia.o
               gfx::Canvas::DrawStringRectWithFlags
+    20788 t@0x7887d0   684     ui/gfx/gfx/render_text.o
               gfx::RenderText::RenderText
+    21468 t@0x78bd54   680     ui/gfx/gfx/render_text.o
               gfx::RenderText::ElideEmail
+    22120 t@0x785dfc   652     ui/gfx/gfx/render_text_harfbuzz.o
               gfx::RenderTextHarfBuzz::GetSubstringBounds
+    22744 t@0x782540   624     ui/gfx/gfx/harfbuzz_font_skia.o
               gfx::CreateHarfBuzzFont
+    23360 t@0x783f44   616     ui/gfx/gfx/render_text_harfbuzz.o
               gfx::RenderTextHarfBuzz::FindCursorPosition
+    23964 t@0x78b4dc   604     ui/gfx/gfx/render_text.o
               gfx::RenderText::CreateInstanceOfSameStyle const
+    24488 t@0x786ea0   524     ui/gfx/gfx/render_text_harfbuzz.o
               gfx::RenderTextHarfBuzz::EnsureLayoutRunList
+    24968 t@0x78ca68   480     ui/gfx/gfx/platform_font_linux.o
               gfx::PlatformFontLinux::PlatformFontLinux
+    25432 t@0x788324   464     ui/gfx/gfx/render_text.o
               gfx::RenderText::~RenderText
+    25864 t@0x78b32c   432     ui/gfx/gfx/render_text.o
               gfx::RenderText::SetText
+    26276 t@0x77fab4   412     ui/gfx/gfx/text_elider.o
               gfx::StringSlicer::CutString const

Short answer is yes, as stated in the commit message "Text is rendered with gfx::Canvas and gfx::RenderText, previously disabled for android"

We could do some work to strip out some symbols that we don't currently need (like gfx::RenderText::ElideEmail and gfx::RenderText::ApplyFadeEffects gfx::ShadowValue). But, that wouldn't save more than 2kb and it might be the case that we need them later.

Adding some more context:
This CL is the foundation to start implementing the new native UI for VrShell, replacing our previous HTML based textures. That UI was removed in commit 847809ed052d0f4c72098045039fae43be6dee1a

I think this is Working As Expected, and there's not much we can do about this.

Please re-open if there's anything more you'd like us to do here.

Would be great if you could look into why these are new symbols. E.g. we already render fonts in blink and in <canvas>. Why were these symbols new here? Can we reuse the font rendering functions used by blink instead?

The answer might be "no", but I think it's worth looking into.

After discussion with sadrul@ and eae@, it's been made clear that re-using blink text rendering would likely be a year-long effort even if undertaken by the domain experts. 

It's also not clear that doing so would be a binary size reduction, as the gfx::RenderText approach is significantly smaller/simpler than blink's code.

Thanks for looking into it. Sounds like the only way to eliminate the overhead would be to render in java. Looks like it's technically not that hard:

http://stackoverflow.com/questions/18453948/android-drawtext-including-text-wrapping
https://coderwall.com/p/6koh_g/rendering-any-android-view-directly-to-an-opengl-texture

So maybe the bigger deciding factor is whether it's possible to define a reasonable level of abstraction so that Android can be special cased.

Using Android views for font rendering is somewhat appealing initially, because the binary size increase is mostly just the added code to add a view to the view hierarchy, and draw text into it. There’s some prior art here in drawing Android Views into our 3D scene, as we do this for the New Tab Page.

However, there are some major drawbacks:
-This wouldn’t be cross platform - we’d need to maintain separate code paths for all UI elements that contain text indefinitely, which would be a large burden.

-Performance for rendering Android Views into textures is very poor if you want to support animations or other effects that cannot be hardware accelerated when drawing into a texture. For reference, see how amazingly slow updating list index highlighting on the New Tab Page is in VR.

-We would need either multiple Android Surfaces (what would the performance overhead for this be?), or use a complicated texture-atlas scheme for rendering multiple elements with text.

-Why not just implement RenderWrappedTextIntoTexture() and share the rest of the skia code?
We wouldn't be able to re-use the skia surface because the way drawing a view to a surface works is it draws over the whole surface.
We would need two surfaces per element, and to composite the text over top. Performance and memory concerns abound, as well as the additional android-only complexity of having multiple surfaces per element.


Apologies that I haven't fleshed the alternatives out more with numbers from prototypes, we're under a lot of time pressure at the moment. I'll spend some more time thinking about this and see if I can come up with any other approaches.

Thanks for all details. Happy to close this as "clearly done their due diligence".