Bug 59226 - Should have an easy way to construct starting BidiStatus for a paragraph root
Summary: Should have an easy way to construct starting BidiStatus for a paragraph root
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Eric Seidel (no email)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-22 13:18 PDT by Eric Seidel (no email)
Modified: 2011-04-28 01:47 PDT (History)
11 users (show)

See Also:


Attachments
Patch (29.17 KB, patch)
2011-04-22 13:21 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Patch (31.65 KB, patch)
2011-04-22 14:52 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Patch (32.25 KB, patch)
2011-04-22 16:07 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Patch (33.46 KB, patch)
2011-04-22 16:16 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2011-04-22 13:18:17 PDT
Should have an easy way to construct starting BidiStatus for a paragraph root
Comment 1 Eric Seidel (no email) 2011-04-22 13:21:33 PDT
Created attachment 90748 [details]
Patch
Comment 2 WebKit Review Bot 2011-04-22 13:43:33 PDT
Attachment 90748 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8498424
Comment 3 Build Bot 2011-04-22 14:26:39 PDT
Attachment 90748 [details] did not build on win:
Build output: http://queues.webkit.org/results/8496456
Comment 4 Eric Seidel (no email) 2011-04-22 14:52:52 PDT
Created attachment 90768 [details]
Patch
Comment 5 WebKit Review Bot 2011-04-22 14:55:23 PDT
Attachment 90748 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8494569
Comment 6 WebKit Review Bot 2011-04-22 15:31:48 PDT
Attachment 90768 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8497472
Comment 7 Eric Seidel (no email) 2011-04-22 16:07:41 PDT
Created attachment 90791 [details]
Patch
Comment 8 Build Bot 2011-04-22 16:12:53 PDT
Attachment 90768 [details] did not build on win:
Build output: http://queues.webkit.org/results/8486944
Comment 9 Eric Seidel (no email) 2011-04-22 16:16:37 PDT
Created attachment 90795 [details]
Patch
Comment 10 Eric Seidel (no email) 2011-04-22 18:07:19 PDT
Assuming GTK passes, this patch should be good to go!
Comment 11 Eric Seidel (no email) 2011-04-26 22:30:19 PDT
This should basically just be a rubber stamp.  This patch is mostly just moving code.
Comment 12 Ryosuke Niwa 2011-04-27 16:40:56 PDT
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.
Comment 13 Eric Seidel (no email) 2011-04-27 16:48:45 PDT
Comment on attachment 90795 [details]
Patch

I'll clean that up in a later pass.  Thanks!
Comment 14 WebKit Commit Bot 2011-04-27 20:21:04 PDT
Comment on attachment 90795 [details]
Patch

Clearing flags on attachment: 90795

Committed r85143: <http://trac.webkit.org/changeset/85143>
Comment 15 WebKit Commit Bot 2011-04-27 20:21:10 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 WebKit Review Bot 2011-04-27 21:16:51 PDT
http://trac.webkit.org/changeset/85143 might have broken WinCE Release (Build)
Comment 17 Eric Seidel (no email) 2011-04-27 22:58:11 PDT
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.
Comment 18 Eric Seidel (no email) 2011-04-27 22:58:59 PDT
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).
Comment 19 Eric Seidel (no email) 2011-04-27 22:59:28 PDT
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?
Comment 20 Eric Seidel (no email) 2011-04-27 23:06:01 PDT
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.
Comment 21 Eric Seidel (no email) 2011-04-27 23:16:08 PDT
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?
Comment 22 Eric Seidel (no email) 2011-04-27 23:17:50 PDT
I wonder if these failures on windows could be from bug 54573?
Comment 23 Eric Seidel (no email) 2011-04-27 23:23:57 PDT
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.
Comment 24 Eric Seidel (no email) 2011-04-27 23:32:33 PDT
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!
Comment 25 Eric Seidel (no email) 2011-04-28 01:47:11 PDT
We're back down to 1 failure after a clean build.  This was a just a Windows-build-can't-understand-dependencies problem.