Thanks for filing. I'm the right person, I'm off tomorrow but can take a look on Monday.

Out of curiosity, is it flaky? Do you know why this hasn't turned the bots red?

Not flaky. It works on the bots, just not on my corporate machine.

Hmm...it works on my corp machine. I wonder if it's due to font differences...

Just ran into this again. Anything I can try to help diagnose this?

Hmm, as a start, could you add:

  LOG(WARNING) << "Point: " << point.ToString();
  LOG(WARNING) << "RoundedPoint: " << result.RoundedPointInInnerNodeFrame().ToString();

to WebLocalFrameImpl::CharacterIndexForPoint after the declaration of `result` and see what that outputs. I get:

[31689:31689:0814/145458.701629:682879478907:WARNING:WebLocalFrameImpl.cpp(1008)] Point: "210,254"
[31689:31689:0814/145458.702191:682879479129:WARNING:WebLocalFrameImpl.cpp(1009)] RoundedPoint: "210,254"

I suspect that'll be the same for you but lets start there. The test uses Ahem font which I've seen recommended for writing portable tests, you could confirm that you have that font installed and available but I'm not exactly sure how to do that.

Same here:
[72735:72735:0814/123847.779673:1868530146054:WARNING:WebLocalFrameImpl.cpp(1015)] Point: "210,254"
[72735:72735:0814/123847.779805:1868530146175:WARNING:WebLocalFrameImpl.cpp(1016)] RoundedPoint: "210,254"

I have Ahem installed. Just tested manually, and without it, many more tests would fail.

Ok, thanks. In the test itself, please add this line just before `main_frame` is declared and paste the results:

  showLayerTree(web_view_helper.LocalMainFrame()->GetFrameView()->GetLayoutView());

Here's what it shows for me:

*layer 0x3ecf78014010 at (0,0) size 640x480 (composited, bounds=at (0,0) size 640x480, drawsContent=1)
  LayoutView 0x3ecf78004010 at (0,0) size 640x480
 positive z-order list(1)
   layer 0x3ecf780140e8 at (0,0) size 640x8
    LayoutBlockFlow 0x3ecf78024010 {HTML} at (0,0) size 640x8
      LayoutBlockFlow 0x3ecf78024130 {BODY} at (8,8) size 624x0
   positive z-order list(1)
     layer 0x3ecf780141c0 at (200,250) size 91x22
      LayoutBlockFlow (positioned) 0x3ecf78024250 {DIV} at (200,250) size 90.69x22 id="target"
        LayoutText 0x3ecf78030010 {#text} at (0,0) size 51x11
          text run at (0,0) width 51: "First Line "
        LayoutBR 0x3ecf780300d8 {BR} at (50,0) size 1x11
        LayoutText 0x3ecf780301a0 {#text} at (0,11) size 91x11
          text run at (0,11) width 91: "Second Text Row"

I get something very different:

*layer 0x2944a0014010 at (0,0) size 640x480 (composited, bounds=at (0,0) size 640x480, drawsContent=1)
  LayoutView 0x2944a0004010 at (0,0) size 640x480
 positive z-order list(1)
   layer 0x2944a00140e8 at (0,0) size 640x8
    LayoutBlockFlow 0x2944a0024010 {HTML} at (0,0) size 640x8
      LayoutBlockFlow 0x2944a0024130 {BODY} at (8,8) size 624x0
   positive z-order list(1)
     layer 0x2944a00141c0 at (200,250) size 150x20
      LayoutBlockFlow (positioned) 0x2944a0024250 {DIV} at (200,250) size 150x20 id="target"
        LayoutText 0x2944a00304c0 {#text} at (0,0) size 110x10
          text run at (0,0) width 110: "First Line "
        LayoutBR 0x2944a0030588 {BR} at (110,0) size 0x10
        LayoutText 0x2944a00301a0 {#text} at (0,10) size 150x10
          text run at (0,10) width 150: "Second Text Row"

Ok, it does look like the text runs are significantly different in size. Unfortunately, I don't much know where to go from here, but I'm adding Blink>Fonts and untriaging - hopefully someone more familiar with fonts and how they layout can help diagnose further.

I used content_shell to load the html file used in the test, src/third_party/WebKit/Source/core/testing/data/sometext.html

Now I am even more confused.

The main difference is size of our LayoutText runs, which is determined by the width of 2nd LayoutText.

2nd LayoutText content is "Second Text Row". It is 15 characters wide, rendered as 10pt Ahem. Since Ahem is square, I'd expect it text to be 10*15=> 150px wide. That is what it is on my machine, inside content_shell and chromium, but not on yours.

I am very curious about what is going on.

bokan@, do you install Ahem locally?

It looks like "sometext.html" uses Ahem, so every character should have 10px width, and that atotic's result is correct.

You can force loading Ahem in unit tests, please see RenderingTest::LoadAhem().

I've never manually messed with fonts so this should be fairly a stock install of gLinux. The fact that it's passing in the waterfall indicates that the bots are in the same state, right?

I added the code from LoadAhem into the test and, sure enough, I get the same result as atotic@. So, how come atotic@'s tests run with ahem loaded but the bots and others' machines don't? Is there a font we can use that's always loaded and universally available?

Yeah, I was surprised bots don't have Ahem, but most of us working on layout are likely to install Ahem locally. I do, and I suppose atotic@ do too.

Well, "Times" or "Arial" are available, or "serif", but all of them do not guarantee the same metrics (width, height, etc.) Since your tests relies on position and size of each character (CharacterIndexAtPoint does), the only way to run reliably is to use Ahem along with LoadAhem. For tests that do not need precisely the same metrics, other fonts can do too.

Note, if you run HTML-based tests (LayoutTests), test runner loads Ahem automatically. It's optional for unit tests. We used to install Ahem on all bots, but sometimes it was not done, which made lots of tests flaky, so someone changed test runner to load Ahem.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/eb3ee745851fe2aa1ee7bd67dd029e9cea70a62d

commit eb3ee745851fe2aa1ee7bd67dd029e9cea70a62d
Author: Koji Ishii <kojii@chromium.org>
Date: Tue Aug 22 02:18:01 2017

Unify LoadAhem() and make it available for other tests

RenderingTest::LoadAhem() is useful for other tests when they rely on
specific metrics of the font. This patch makes it a static function so
that other tests can use it.

Currently, RangeTest copies the logic, and we found WebFrameTest needs
it too in  issue 749836 .

Bug:  749836 
Change-Id: I3f3338cdfe62fa565a572fca60128544ea884b01
Reviewed-on: https://chromium-review.googlesource.com/622822
Reviewed-by: David Bokan <bokan@chromium.org>
Reviewed-by: Aleks Totic <atotic@chromium.org>
Commit-Queue: Koji Ishii <kojii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#496177}
[modify] https://crrev.com/eb3ee745851fe2aa1ee7bd67dd029e9cea70a62d/third_party/WebKit/Source/core/dom/RangeTest.cpp
[modify] https://crrev.com/eb3ee745851fe2aa1ee7bd67dd029e9cea70a62d/third_party/WebKit/Source/core/exported/WebFrameTest.cpp
[modify] https://crrev.com/eb3ee745851fe2aa1ee7bd67dd029e9cea70a62d/third_party/WebKit/Source/core/frame/FrameTestHelpers.cpp
[modify] https://crrev.com/eb3ee745851fe2aa1ee7bd67dd029e9cea70a62d/third_party/WebKit/Source/core/frame/FrameTestHelpers.h
[modify] https://crrev.com/eb3ee745851fe2aa1ee7bd67dd029e9cea70a62d/third_party/WebKit/Source/core/layout/LayoutTestHelper.cpp
[modify] https://crrev.com/eb3ee745851fe2aa1ee7bd67dd029e9cea70a62d/third_party/WebKit/Source/core/layout/LayoutTestHelper.h

I have a patch up that'll fix this specific test. Perhaps we should call LoadAhem from FrameTestHelpers::Initialize() so that we don't run into this case accidentally again? I tried this, all webkit_unit_tests passed except these:

All/ParameterizedWebFrameTest.ContextNotificationsLoadUnload/0 
All/ParameterizedWebFrameTest.ContextNotificationsLoadUnload/1 
All/ParameterizedWebFrameTest.ContextNotificationsReload/0 
All/ParameterizedWebFrameTest.ContextNotificationsReload/1 

atotic@, do you see these failures as well?

kojii@, is there a significant performance impact to calling LoadAhem for each test case? WDYT?

I do not see these failures. Surprising, since at a glance, these tests do not depend upon Ahem.

Yeah, the LoadAhem() injects Ahem as a web font by running a script. It looks like the tests monitors script context life cycle, is possible it may be affected.

I don't think there's a significant performance impact, and it's probably safe for layout tests like yours, but may affect script tests.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/57f5b834928ff596e8c5520d532ac8f22706886f

commit 57f5b834928ff596e8c5520d532ac8f22706886f
Author: David Bokan <bokan@chromium.org>
Date: Wed Aug 23 04:12:43 2017

Make CharacterIndexAtPointWithPinchZoom load Ahem

This test uses the Ahem font but on machines/bots without this font
it will fallback. The expectations were set for the fallback without
realizing it. This causes the test to fail on a system with Ahem
installed.

This patch calls LoadAhem in the test and resets the expectations.

Bug:  749836 
Change-Id: I7c0854f8d45bbadf1b88c27ab4964b2dac312058
Reviewed-on: https://chromium-review.googlesource.com/627378
Reviewed-by: Koji Ishii <kojii@chromium.org>
Commit-Queue: Koji Ishii <kojii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#496585}
[modify] https://crrev.com/57f5b834928ff596e8c5520d532ac8f22706886f/third_party/WebKit/Source/core/exported/WebFrameTest.cpp
[modify] https://crrev.com/57f5b834928ff596e8c5520d532ac8f22706886f/third_party/WebKit/Source/core/testing/data/sometext.html

Given that this doesn't seem to be an issue in other tests, I'm just going to mark this fixed.