Bug 59226 – Should have an easy way to construct starting BidiStatus for a paragraph root

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Eric Seidel (no email)]

Should have an easy way to construct starting BidiStatus for a paragraph root

[Eric Seidel (no email)]

Created attachment 90748 [details]
Patch

[WebKit Review Bot]

Attachment 90748 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8498424

[Build Bot]

Attachment 90748 [details] did not build on win:
Build output: http://queues.webkit.org/results/8496456

[Eric Seidel (no email)]

Created attachment 90768 [details]
Patch

[WebKit Review Bot]

Attachment 90748 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8494569

[WebKit Review Bot]

Attachment 90768 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8497472

[Eric Seidel (no email)]

Created attachment 90791 [details]
Patch

[Build Bot]

Attachment 90768 [details] did not build on win:
Build output: http://queues.webkit.org/results/8486944

[Eric Seidel (no email)]

Created attachment 90795 [details]
Patch

[Eric Seidel (no email)]

Assuming GTK passes, this patch should be good to go!

[Eric Seidel (no email)]

This should basically just be a rubber stamp.  This patch is mostly just moving code.

[Ryosuke Niwa]

Comment on attachment 90795 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=90795&action=review

> Source/WebCore/platform/graphics/GraphicsContext.cpp:418
> +        bool isRTL = bidiRun->level() % 2;
> +        subrun.setDirection(isRTL ? RTL : LTR);

I'm not sure if this local variable is anyway helpful.
subrun.setDirection(bidiRun->level() % 2? RTL : LTR)
is clear enough to me.

[Eric Seidel (no email)]

Comment on attachment 90795 [details]
Patch

I'll clean that up in a later pass.  Thanks!

[WebKit Commit Bot]

Comment on attachment 90795 [details]
Patch

Clearing flags on attachment: 90795

Committed r85143: <http://trac.webkit.org/changeset/85143>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/85143 might have broken WinCE Release (Build)

[Eric Seidel (no email)]

This made 17 tests fail on windows.  I do not understand why.  Nothing in the windows specific code looks related.  We're not using an enum as a bitfield (which can cause oddities with MSVC).  I'm not sure how to move forward here.

[Eric Seidel (no email)]

http://build.webkit.org/results/Windows%20XP%20Debug%20(Tests)/r85143%20(28134)/fast/text/basic/003-pretty-diff.html

Is the failure (which again seems related to this revision).

[Eric Seidel (no email)]

I suspect it may also be that bad dependency tracking on windows caused my change to be blamed even though this was an earlier regression?

[Eric Seidel (no email)]

I'm happy to roll this out, but I'm not sure how to move forward once that is done.

It would be best if someone with access to a Windows build could try clean-building the revision before this and see if these tests fail.

[Eric Seidel (no email)]

I've looked at the chromium builders and been unable to find a failure matching any of these tests.  I'm going to see if I can kick the Win builder into a clean build?

[Eric Seidel (no email)]

I wonder if these failures on windows could be from bug 54573?

[Eric Seidel (no email)]

I've kicked the Win Debug builder to make it clean build.  I may also just try rolling out the patch and seeing if the failure goes away for windows.  I can always roll it back in.

[Eric Seidel (no email)]

This isn't failing on Win 7 Release tests, or on any Chromium bot. :(  So I'm going to blame the XP bot here and ignore this failure warning and sleep.  If someone else believes otherwise, feel free to roll out the patch!

[Eric Seidel (no email)]

We're back down to 1 failure after a clean build.  This was a just a Windows-build-can't-understand-dependencies problem.