Bidi-Isolate inlines break layout with collapsed whitespace

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

[igoroliveira, 2013-10-07 20:53:47]

The failure in linux_layout is a false positive.

[leviw_travelin_and_unemployed, 2013-10-07 21:58:42]

https://codereview.chromium.org/26315003/diff/1/Source/core/rendering/InlineI...
File Source/core/rendering/InlineIterator.h (right):

https://codereview.chromium.org/26315003/diff/1/Source/core/rendering/InlineI...
Source/core/rendering/InlineIterator.h:503: void
adjustMidpointForIsolatedRun(RenderObject* obj, unsigned start, unsigned end,
InlineBidiResolver& resolver)
This seems really similar to RenderBlock::appendRunsForObject, which makes me
sad the logic is duplicated. It's also hard for me to wrap my head fully around
how this solves the problem.

https://codereview.chromium.org/26315003/diff/1/Source/core/rendering/RenderB...
File Source/core/rendering/RenderBlockLineLayout.cpp (right):

https://codereview.chromium.org/26315003/diff/1/Source/core/rendering/RenderB...
Source/core/rendering/RenderBlockLineLayout.cpp:1345: while
(!isolatedResolver.isolatedRuns().isEmpty()) {
It's weird to have nested while loops with the same conditional. If this is
really necessary, can you put it in a function with a descriptive name?

[igoroliveira, 2013-10-07 23:37:36]

On 2013/10/07 21:58:42, Levi wrote:
>
https://codereview.chromium.org/26315003/diff/1/Source/core/rendering/InlineI...
> File Source/core/rendering/InlineIterator.h (right):
> 
>
https://codereview.chromium.org/26315003/diff/1/Source/core/rendering/InlineI...
> Source/core/rendering/InlineIterator.h:503: void
> adjustMidpointForIsolatedRun(RenderObject* obj, unsigned start, unsigned end,
> InlineBidiResolver& resolver)
> This seems really similar to RenderBlock::appendRunsForObject, which makes me
> sad the logic is duplicated. 
Shame on me =/. I created a helper function who shares the logic.

> It's also hard for me to wrap my head fully around
> how this solves the problem.
> 

midpointState logic is witchcraft :). With the new patchset probably would be
easier to understand what i am doing.

>
https://codereview.chromium.org/26315003/diff/1/Source/core/rendering/RenderB...
> File Source/core/rendering/RenderBlockLineLayout.cpp (right):
> 
>
https://codereview.chromium.org/26315003/diff/1/Source/core/rendering/RenderB...
> Source/core/rendering/RenderBlockLineLayout.cpp:1345: while
> (!isolatedResolver.isolatedRuns().isEmpty()) {
> It's weird to have nested while loops with the same conditional. If this is
> really necessary, can you put it in a function with a descriptive name?

In fact the conditional are different the outer loop uses topResolver and the
inner isolatedResolver. I created this code in a function anyway.

[leviw_travelin_and_unemployed, 2013-10-07 23:52:55]

https://codereview.chromium.org/26315003/diff/10001/Source/core/rendering/Inl...
File Source/core/rendering/InlineIterator.h (right):

https://codereview.chromium.org/26315003/diff/10001/Source/core/rendering/Inl...
Source/core/rendering/InlineIterator.h:486: static void
adjustMidpointsAndAppendRunsForObjectIfNeeded(RenderObject* obj, unsigned start,
unsigned end, InlineBidiResolver& resolver, bool isAppendingFakeRunIn = true,
BidiRunList<BidiRun>* runs = 0)
I'd like to avoid boolean parameters here if possible. Can you create an enum
for isAppendingFakeRunIn instead?

[igoroliveira, 2013-10-08 20:52:16]

On 2013/10/07 23:52:55, Levi wrote:
>
https://codereview.chromium.org/26315003/diff/10001/Source/core/rendering/Inl...
> File Source/core/rendering/InlineIterator.h (right):
> 
>
https://codereview.chromium.org/26315003/diff/10001/Source/core/rendering/Inl...
> Source/core/rendering/InlineIterator.h:486: static void
> adjustMidpointsAndAppendRunsForObjectIfNeeded(RenderObject* obj, unsigned
start,
> unsigned end, InlineBidiResolver& resolver, bool isAppendingFakeRunIn = true,
> BidiRunList<BidiRun>* runs = 0)
> I'd like to avoid boolean parameters here if possible. Can you create an enum
> for isAppendingFakeRunIn instead?

Okey Dokey!. Patch updated.

[leviw_travelin_and_unemployed, 2013-10-08 20:57:46]

That looks a lot better to me, thank you. LGTM :)

[commit-bot: I haz the power, 2013-10-08 20:57:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/igor.o@sisa.samsung.com/26315003/20001

[commit-bot: I haz the power, 2013-10-08 21:16:29]

Retried try job too often on blink_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...

[igoroliveira, 2013-10-08 21:32:36]

Adding Source/platform OWNER.

[eseidel, 2013-10-08 22:03:09]

lgtm

[commit-bot: I haz the power, 2013-10-08 22:03:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/igor.o@sisa.samsung.com/26315003/20001

[commit-bot: I haz the power, 2013-10-09 05:39:32]

Change committed as 159203

[leviw_travelin_and_unemployed, 2013-10-22 22:25:12]

LGTM with nits.

https://codereview.chromium.org/26315003/diff/101001/LayoutTests/fast/text/in...
File LayoutTests/fast/text/international/bdi-break-crash-1-expected.txt (right):

https://codereview.chromium.org/26315003/diff/101001/LayoutTests/fast/text/in...
LayoutTests/fast/text/international/bdi-break-crash-1-expected.txt:1: > ࣟ7 >
Can we clean this up after ensuring we don't crash so it's not in the output?

https://codereview.chromium.org/26315003/diff/101001/LayoutTests/fast/text/in...
File LayoutTests/fast/text/international/bdi-break-crash-2-expected.txt (right):

https://codereview.chromium.org/26315003/diff/101001/LayoutTests/fast/text/in...
LayoutTests/fast/text/international/bdi-break-crash-2-expected.txt:1: ࣟ7 >
Ditto.

https://codereview.chromium.org/26315003/diff/101001/Source/core/rendering/In...
File Source/core/rendering/InlineIterator.h (right):

https://codereview.chromium.org/26315003/diff/101001/Source/core/rendering/In...
Source/core/rendering/InlineIterator.h:563: if
(RenderBlock::shouldSkipCreatingRunsForObject(obj))
This makes a lot more sense.

[commit-bot: I haz the power, 2013-10-24 00:07:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/igor.o@sisa.samsung.com/26315003/341001

[commit-bot: I haz the power, 2013-10-24 01:09:18]

Change committed as 160400