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

Issue 26315003: Bidi-Isolate inlines break layout with collapsed whitespace (Closed)

Created:
7 years, 2 months ago by igoroliveira
Modified:
7 years, 2 months ago
CC:
blink-reviews, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

Collapsed space never worked properly if we have spaces inside and/or after a Bidi-isolate. The problem happens in two parts of the code: 1. The midpointState is not updated properly in addFakeRunIfNecessary. So when an isolated run is added the next midpoint state is wrong. 2. Since isolated runs have a two pass algorithm. In the second pass the midpoint state is wrong. This patches fixes the first problem implementing a method called adjustMidpointsAndAppendRunsForObjectIfNeeded, when a fake run is added it basically updates the midpointState properly. And the second problem is fixed saving and restoring the midpointState for Bidi-isolate inlines. Additionally, with the recent modifications, the fake run needs to have width zero because constructBidiRunsForSegment not necessary will replace the fake run with a valid run. BUG=304701 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=159203 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=160400

Patch Set 1 #

Total comments: 2

Patch Set 2 : Proposed patch v2 #

Total comments: 1

Patch Set 3 : Proposed patch v3 #

Patch Set 4 : Proposed patch fixing crash #

Total comments: 3

Patch Set 5 : Proposed patch #

Patch Set 6 : Rebased patch #

Unified diffs Side-by-side diffs Delta from patch set Stats (+214 lines, -58 lines) Patch
A LayoutTests/fast/text/international/bdi-break-crash-1.html View 1 2 3 4 1 chunk +13 lines, -0 lines 0 comments Download
A LayoutTests/fast/text/international/bdi-break-crash-1-expected.txt View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
A LayoutTests/fast/text/international/bdi-break-crash-2.html View 1 2 3 4 1 chunk +17 lines, -0 lines 0 comments Download
A LayoutTests/fast/text/international/bdi-break-crash-2-expected.txt View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
A LayoutTests/fast/text/international/unicode-bidi-isolate-collapsed-whitespace.html View 1 chunk +42 lines, -0 lines 0 comments Download
A LayoutTests/fast/text/international/unicode-bidi-isolate-collapsed-whitespace-expected.html View 1 chunk +40 lines, -0 lines 0 comments Download
M Source/core/rendering/InlineIterator.h View 1 2 3 4 5 4 chunks +68 lines, -15 lines 0 comments Download
M Source/core/rendering/RenderBlockLineLayout.cpp View 1 2 3 4 5 4 chunks +13 lines, -43 lines 0 comments Download
M Source/platform/text/BidiResolver.h View 4 chunks +19 lines, -0 lines 0 comments Download

Messages

Total messages: 16 (0 generated)
igoroliveira
7 years, 2 months ago (2013-10-07 20:32:34 UTC) #1
igoroliveira
The failure in linux_layout is a false positive.
7 years, 2 months ago (2013-10-07 20:53:47 UTC) #2
leviw_travelin_and_unemployed
https://codereview.chromium.org/26315003/diff/1/Source/core/rendering/InlineIterator.h File Source/core/rendering/InlineIterator.h (right): https://codereview.chromium.org/26315003/diff/1/Source/core/rendering/InlineIterator.h#newcode503 Source/core/rendering/InlineIterator.h:503: void adjustMidpointForIsolatedRun(RenderObject* obj, unsigned start, unsigned end, InlineBidiResolver& resolver) ...
7 years, 2 months ago (2013-10-07 21:58:42 UTC) #3
igoroliveira
On 2013/10/07 21:58:42, Levi wrote: > https://codereview.chromium.org/26315003/diff/1/Source/core/rendering/InlineIterator.h > File Source/core/rendering/InlineIterator.h (right): > > https://codereview.chromium.org/26315003/diff/1/Source/core/rendering/InlineIterator.h#newcode503 > ...
7 years, 2 months ago (2013-10-07 23:37:36 UTC) #4
leviw_travelin_and_unemployed
https://codereview.chromium.org/26315003/diff/10001/Source/core/rendering/InlineIterator.h File Source/core/rendering/InlineIterator.h (right): https://codereview.chromium.org/26315003/diff/10001/Source/core/rendering/InlineIterator.h#newcode486 Source/core/rendering/InlineIterator.h:486: static void adjustMidpointsAndAppendRunsForObjectIfNeeded(RenderObject* obj, unsigned start, unsigned end, InlineBidiResolver& ...
7 years, 2 months ago (2013-10-07 23:52:55 UTC) #5
igoroliveira
On 2013/10/07 23:52:55, Levi wrote: > https://codereview.chromium.org/26315003/diff/10001/Source/core/rendering/InlineIterator.h > File Source/core/rendering/InlineIterator.h (right): > > https://codereview.chromium.org/26315003/diff/10001/Source/core/rendering/InlineIterator.h#newcode486 > ...
7 years, 2 months ago (2013-10-08 20:52:16 UTC) #6
leviw_travelin_and_unemployed
That looks a lot better to me, thank you. LGTM :)
7 years, 2 months ago (2013-10-08 20:57:46 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/igor.o@sisa.samsung.com/26315003/20001
7 years, 2 months ago (2013-10-08 20:57:56 UTC) #8
commit-bot: I haz the power
Retried try job too often on blink_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_presubmit&number=8367
7 years, 2 months ago (2013-10-08 21:16:29 UTC) #9
igoroliveira
Adding Source/platform OWNER.
7 years, 2 months ago (2013-10-08 21:32:36 UTC) #10
eseidel
lgtm
7 years, 2 months ago (2013-10-08 22:03:09 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/igor.o@sisa.samsung.com/26315003/20001
7 years, 2 months ago (2013-10-08 22:03:23 UTC) #12
commit-bot: I haz the power
Change committed as 159203
7 years, 2 months ago (2013-10-09 05:39:32 UTC) #13
leviw_travelin_and_unemployed
LGTM with nits. https://codereview.chromium.org/26315003/diff/101001/LayoutTests/fast/text/international/bdi-break-crash-1-expected.txt File LayoutTests/fast/text/international/bdi-break-crash-1-expected.txt (right): https://codereview.chromium.org/26315003/diff/101001/LayoutTests/fast/text/international/bdi-break-crash-1-expected.txt#newcode1 LayoutTests/fast/text/international/bdi-break-crash-1-expected.txt:1: > ࣟ7 > Can we clean ...
7 years, 2 months ago (2013-10-22 22:25:12 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/igor.o@sisa.samsung.com/26315003/341001
7 years, 2 months ago (2013-10-24 00:07:20 UTC) #15
commit-bot: I haz the power
7 years, 2 months ago (2013-10-24 01:09:18 UTC) #16
Message was sent while issue was closed.
Change committed as 160400

Powered by Google App Engine
This is Rietveld 408576698