Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(359)

Issue 58383002: Update BidiResolver's max level depth to 125 per tr9r29 (Closed)

Created:
7 years, 1 month ago by eseidel
Modified:
7 years, 1 month ago
CC:
blink-reviews, leviw_travelin_and_unemployed, igor.o, pals, aharon
Visibility:
Public.

Description

Update BidiResolver's max level depth to 125 per tr9r29 This is mostly just to demonstrate the process of making a change to BidiResolver and using the new BidiTest.txt based testing in BidiResolverTest.cpp. Our test failures are now: WARNING: Skipped 418143 tests. Ran 352098 tests: 44882 level failures 19151 order failures. Which is down 7 level failures from before and 2 order failures. The process to test this was: curl -O http://www.unicode.org/Public/UNIDATA/BidiTest.txt (The version in our tree is older, BidiTest.txt needs to be in your working directory for the BidiTest.txt based tests to run.) ninja -C out/Debug blink_platform_unittests ./out/Debug/blink_platform_unittests And then I adjusted the EXPECT_EQ to have the new (lower) numbers. I also flipped the order of arguments in the EXPECT_EQ calls so that the expected/actual numbers are printed correctly on failure. BUG=242238 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=161290

Patch Set 1 #

Total comments: 2

Patch Set 2 : Updated per leviw's review #

Unified diffs Side-by-side diffs Delta from patch set Stats (+16 lines, -9 lines) Patch
M Source/platform/text/BidiContext.h View 1 2 chunks +8 lines, -1 line 0 comments Download
M Source/platform/text/BidiResolver.h View 1 2 chunks +2 lines, -2 lines 0 comments Download
M Source/platform/text/BidiResolverTest.cpp View 2 chunks +6 lines, -6 lines 0 comments Download

Messages

Total messages: 5 (0 generated)
eseidel
7 years, 1 month ago (2013-11-04 20:21:16 UTC) #1
leviw_travelin_and_unemployed
LGTM. https://codereview.chromium.org/58383002/diff/1/Source/platform/text/BidiResolver.h File Source/platform/text/BidiResolver.h (right): https://codereview.chromium.org/58383002/diff/1/Source/platform/text/BidiResolver.h#newcode500 Source/platform/text/BidiResolver.h:500: unsigned char levelLow = 128; // Should this ...
7 years, 1 month ago (2013-11-04 20:24:56 UTC) #2
eseidel
https://codereview.chromium.org/58383002/diff/1/Source/platform/text/BidiResolver.h File Source/platform/text/BidiResolver.h (right): https://codereview.chromium.org/58383002/diff/1/Source/platform/text/BidiResolver.h#newcode500 Source/platform/text/BidiResolver.h:500: unsigned char levelLow = 128; // Should this be ...
7 years, 1 month ago (2013-11-04 20:47:43 UTC) #3
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eseidel@chromium.org/58383002/60001
7 years, 1 month ago (2013-11-04 20:51:23 UTC) #4
commit-bot: I haz the power
7 years, 1 month ago (2013-11-04 23:01:00 UTC) #5
Message was sent while issue was closed.
Change committed as 161290

Powered by Google App Engine
This is Rietveld 408576698