Ignorable characters were shown/rendered in Blink

Ignorable characters were shown/rendered in Blink

Updating Unicode List to include ignorable characters while calculating
code path to take for rendering and layouting

BUG=256333
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=174133

[h.joshi, 2014-05-09 10:05:12]

PTAL

[behdad_google, 2014-05-09 17:44:00]

lgtm

[eseidel, 2014-05-09 17:46:39]

Can't this be tested?

[h.joshi, 2014-05-10 01:45:57]

Thanks Behdad,
@eseidel: We can add test case mentioned in http://crbug.com/256333, pls suggest

On 2014/05/09 17:46:39, eseidel wrote:
> Can't this be tested?

[eseidel, 2014-05-10 06:08:36]

Do the tests added for https://codereview.chromium.org/58383002/ catch this?

[h.joshi, 2014-05-11 03:46:46]

All test passed (test was run on Mac 10.9.2), go following on running test:
SUCCESS: all tests passed.
Tests took 5 seconds.

 Followed following to run test
cd blink/src/third_party/icu/source/test/testdata
curl -O http://www.unicode.org/Public/UNIDATA/BidiTest.txt
ninja -C out/Debug blink_platform_unittests

 Pls suggest if above steps were correct.

On 2014/05/10 06:08:36, eseidel wrote:
> Do the tests added for https://codereview.chromium.org/58383002/ catch this?

[eseidel, 2014-05-11 04:23:52]

There are instructions on how to run the tests at:
https://codereview.chromium.org/39523002

The instructions you list do not look right.

The test you're trying to run is found in:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Notice how it knows how to early-exit if it can't find the BidiTest.txt file
which is why you have to download that separately.

[eseidel, 2014-05-11 04:29:25]

I just did the following and it worked:

./out/Debug/blink_platform_unittests --gtest_filter=BidiResolver.BidiTest_txt

Notice how that if you don't have BidiTest.txt in your current working directory
it doesn't do anything.

If you do have BidiTest.txt in your current working directory it prints pages
and pages of test results.

If your change is covered by that test, then that's sufficient IMO.

[h.joshi, 2014-05-11 05:18:38]

Followed the suggested command, downloaded "BiDitest.txt" to source folder and
now it shows many things related to RTL/LTR levels and ordering for different
cases.
 With and without my changes I am seeing following output:
WARNING: Skipped 418143 tests.
Ran 352098 tests: 44882 level failures 19151 order failures.
[       OK ] BidiResolver.BidiTest_txt (9937 ms)
[----------] 1 test from BidiResolver (9937 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (9946 ms total)
[  PASSED  ] 1 test.

  YOU HAVE 2 DISABLED TESTS

LEAK: 755607 BidiCharacterRun
[1/1] BidiResolver.BidiTest_txt (9937 ms)
SUCCESS: all tests passed.
Tests took 10 seconds.


On 2014/05/11 04:29:25, eseidel wrote:
> I just did the following and it worked:
> 
> ./out/Debug/blink_platform_unittests --gtest_filter=BidiResolver.BidiTest_txt
> 
> Notice how that if you don't have BidiTest.txt in your current working
directory
> it doesn't do anything.
> 
> If you do have BidiTest.txt in your current working directory it prints pages
> and pages of test results.
> 
> If your change is covered by that test, then that's sufficient IMO.

[eseidel, 2014-05-11 05:25:55]

That means that your change is not covered by that test.  If it were, then the
test would "fail" as the number of cases we get right would be differnet.

[eseidel, 2014-05-11 05:32:09]

Sorry, it makes sense to me now that your change would not be covere by
BidiTest.txt.  It probably is covered by BidiCharacters.txt, but we don't run
that test yet.

[eseidel, 2014-05-11 05:35:11]

Why do these characters need to send us down the complex text path?
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Can you show an example HTML page where we're currently rendering these
characters incorrectly?

[h.joshi, 2014-05-11 07:37:35]

in crbug.com:256333 we have following test case:
data:text/html,a⁦b⁩c

 Now as per Unicode 6.3, 0x2066 and 0x2069 should be considered while
calculating and processing of RTL langauge and code flow but should not be
displayed on web page.
Firefox was working correctly in this case, in case of Blink before this patch
we were not considering these glyphs as Complex and following Simple Code path
and hence rendering Default glyphs.

On 2014/05/11 05:35:11, eseidel wrote:
> Why do these characters need to send us down the complex text path?
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> Can you show an example HTML page where we're currently rendering these
> characters incorrectly?

[eseidel, 2014-05-11 08:37:41]

So can we just write a LayoutTest which covers your fix then?  We just need some
way to test your fix so that we don't break it again in the future.

[h.joshi, 2014-05-11 12:19:07]

Sure, will upload LayoutTest with net patch and then submit.

On 2014/05/11 08:37:41, eseidel wrote:
> So can we just write a LayoutTest which covers your fix then?  We just need
some
> way to test your fix so that we don't break it again in the future.

[h.joshi, 2014-05-12 06:06:13]

The CQ bit was checked by h.joshi@samsung.com

[commit-bot: I haz the power, 2014-05-12 06:06:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/h.joshi@samsung.com/276883002/20001

[commit-bot: I haz the power, 2014-05-12 06:06:25]

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

[commit-bot: I haz the power, 2014-05-12 06:06:26]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[eseidel, 2014-05-12 06:08:10]

Does this need to be a pixel test? or is a text-based test sufficient?  Pixel
tests are unfortunate as they require per-platform results.

[h.joshi, 2014-05-12 06:15:53]

I think text-based should also work, as when rendering is correct default glyphs
will bot be drawn so we will have no data related to default glyphs in render
tree
Pls suggets.

On 2014/05/12 06:08:10, eseidel wrote:
> Does this need to be a pixel test? or is a text-based test sufficient?  Pixel
> tests are unfortunate as they require per-platform results.

[eseidel, 2014-05-12 06:16:57]

I don't know.  If you have a text-based test which fails w/o your patch and
passes with your patch that's fine.  We can also have an image-based test, but
text-based tests are nicer to work with when possible to use.

[h.joshi, 2014-05-12 06:34:28]

Sorry for my ignorance, for text-based I have to generate only
bidi-ignorable-unicode-expected.txt or some other process needs to be followed

[eseidel, 2014-05-12 07:32:27]

window.testRunner.dumpAsText()
will make your test text-only:

http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree
http://www.chromium.org/developers/testing/webkit-layout-tests

[h.joshi, 2014-05-12 09:08:21]

Pls review, added new patch with "text-only" setting.

On 2014/05/12 07:32:27, eseidel wrote:
> window.testRunner.dumpAsText()
> will make your test text-only:
> 
> http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree
> http://www.chromium.org/developers/testing/webkit-layout-tests

[leviw_travelin_and_unemployed, 2014-05-12 10:03:22]

lgtm with nits

https://codereview.chromium.org/276883002/diff/40001/LayoutTests/fast/text/bi...
File LayoutTests/fast/text/bidi-ignorable-unicode.html (right):

https://codereview.chromium.org/276883002/diff/40001/LayoutTests/fast/text/bi...
LayoutTests/fast/text/bidi-ignorable-unicode.html:1: <html>
Nit: please add a doctype.

https://codereview.chromium.org/276883002/diff/40001/LayoutTests/fast/text/bi...
LayoutTests/fast/text/bidi-ignorable-unicode.html:3: <script
type="text/javascript">
Nit: don't need the type="text/javascript"

https://codereview.chromium.org/276883002/diff/40001/LayoutTests/fast/text/bi...
LayoutTests/fast/text/bidi-ignorable-unicode.html:9: <h3>Test for chromium bug :
<a href="https://crbug.com/256333">256333</a>.Test case for Ignorable
characters</h3>
Nit: space after period.

[eseidel, 2014-05-12 10:16:40]

https://codereview.chromium.org/276883002/diff/40001/LayoutTests/fast/text/bi...
File LayoutTests/fast/text/bidi-ignorable-unicode-expected.txt (right):

https://codereview.chromium.org/276883002/diff/40001/LayoutTests/fast/text/bi...
LayoutTests/fast/text/bidi-ignorable-unicode-expected.txt:5: a⁦b⁩c
This looks like the test is failing?

[h.joshi, 2014-05-12 10:46:42]

Output should be "a⁦b⁩c" only as 0x2066 and 0x2069 will not be rendered.

On 2014/05/12 10:16:40, eseidel wrote:
>
https://codereview.chromium.org/276883002/diff/40001/LayoutTests/fast/text/bi...
> File LayoutTests/fast/text/bidi-ignorable-unicode-expected.txt (right):
> 
>
https://codereview.chromium.org/276883002/diff/40001/LayoutTests/fast/text/bi...
> LayoutTests/fast/text/bidi-ignorable-unicode-expected.txt:5: a⁦b⁩c
> This looks like the test is failing?

[behdad_google, 2014-05-12 18:48:46]

I would be really surprised if you can test this with a text-only test...  This
is testing something in the rendering.  That cannot be detected by text
operations.

[h.joshi, 2014-05-13 05:27:13]

Sorry for the confusion created, initially I did thought dump render tree would
not be ideal for this but for the text-only output it showed only "abc" which
created confusion.
 New patch added with expected PNG image and text file.

[h.joshi, 2014-05-14 15:03:22]

The CQ bit was checked by h.joshi@samsung.com

[commit-bot: I haz the power, 2014-05-14 15:03:31]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/h.joshi@samsung.com/276883002/80001

[commit-bot: I haz the power, 2014-05-14 16:12:26]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/7436)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/7203)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/6692)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/7159)

[commit-bot: I haz the power, 2014-05-14 16:47:46]

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

[commit-bot: I haz the power, 2014-05-14 16:47:46]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/7207)

[h.joshi, 2014-05-15 14:50:09]

The CQ bit was checked by h.joshi@samsung.com

[commit-bot: I haz the power, 2014-05-15 14:50:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/h.joshi@samsung.com/276883002/120001

[commit-bot: I haz the power, 2014-05-15 21:45:37]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/6808)

[commit-bot: I haz the power, 2014-05-15 22:41:39]

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

[commit-bot: I haz the power, 2014-05-15 22:41:40]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/6808)

[h.joshi, 2014-05-16 06:11:49]

The CQ bit was checked by h.joshi@samsung.com

[commit-bot: I haz the power, 2014-05-16 06:11:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/h.joshi@samsung.com/276883002/140001

[commit-bot: I haz the power, 2014-05-16 07:29:49]

Change committed as 174133

[eseidel, 2014-05-16 18:08:34]

I really don't like that rietveld allows you to land a patch which is completely
different from a patch approved 4 days ago.

[h.joshi, 2014-05-19 04:16:28]

@eseidel: I did not changed any thing in the code, only test case expected
output file "png/text" was changed. Initially generated file "expected.txt" was
not correct, I should have re-checked expected text file before committing text
file, due to some confusion of output format I missed that. Will make sure to
cross check in future patches.