Bug 28420

Created attachment 35020 [details] patch 1: remove ENABLE_RUBY flag as discussed with Dave and Maciej Remove ENABLE_RUBY guards as discussed with Dave Hyatt and Maciej Stachowiak on IRC.

Created attachment 35033 [details] patch 2: ruby functionality Fundamentally the same patch as the last one submitted to https://bugs.webkit.org/show_bug.cgi?id=3749 . See the descriptions there for discussions and caveats (brief summary: implements HTML5 ruby only, no CSS3/XHTML support, some minor known issues with selection and copy/paste).

Created attachment 35034 [details] patch 3: layout tests for ruby New layout tests for <ruby>. These test 2 main areas: .) basic rendering .) correctness of render tree after DOM manipulation

Created attachment 38526 [details] patch 2: ruby functionality Updated patch: first version attempted to be independent of other patches included here. However, this was not feasible and would have resulted in problems when committing. Also improved the change log entry. Provided these patches are accepted, they should be committed in numbered order, i.e., "patch 1" first, then "patch 2", and "patch 3" last.

Rebaselined versions of the layout tests for Chromium-Win: http://codereview.chromium.org/173347

Comment on attachment 35020 [details] patch 1: remove ENABLE_RUBY flag as discussed with Dave and Maciej I'll trust you that they agreed to this. This looks technically correct.

Comment on attachment 35020 [details] patch 1: remove ENABLE_RUBY flag as discussed with Dave and Maciej Actually, removing cq+, since the commit-queue will close the bug when it lands this patch (despite the other r=? patches) due to a bug.

bug 28230 is the bug I was referring to.

Comment on attachment 38526 [details] patch 2: ruby functionality Typo in RenderRubyAsInline: "Note: ':after' content is handled implicitely below" "implicitely" hould be "implicitly" Everything else looks good. r=me

Comment on attachment 35020 [details] patch 1: remove ENABLE_RUBY flag as discussed with Dave and Maciej Let commit bot land it

Comment on attachment 35020 [details] patch 1: remove ENABLE_RUBY flag as discussed with Dave and Maciej Rejecting patch 35020 from commit-queue. Patch https://bugs.webkit.org/attachment.cgi?id=35020 from bug 28420 failed to download and apply.

Comment on attachment 35034 [details] patch 3: layout tests for ruby Rejecting patch 35034 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1 Running build-dumprendertree Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Testing 11484 test cases. fast/ruby/ruby-empty-rt.html -> failed Exiting early after 1 failures. 7830 tests run. 141.56s total testing time 7829 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 3 test cases (<1%) had stderr output

Comment on attachment 38526 [details] patch 2: ruby functionality Rejecting patch 38526 from commit-queue. Patch https://bugs.webkit.org/attachment.cgi?id=38526 from bug 28420 failed to download and apply.

The patches depend on each other and probably need someone to land them manually. The commit-queue isn't really designed for this type of multi-patch change.

(In reply to comment #14) > The patches depend on each other and probably need someone to land them > manually. The commit-queue isn't really designed for this type of multi-patch > change. Thanks for handling this so far! I'll get my committer account for WebKit soon, so I should be able to land it myself then.

Landed patch 1.

Landed patch 2.

http://trac.webkit.org/changeset/50397 broke the windows build. We need to fix it or roll it out.

Fixed the windows build.

Created attachment 42475 [details] patch 4 - re-baselined layout test results for Windows Added new patch that contains the re-baselined results for the ruby layout tests for the Windows platform.

Landed patch 3 As it caused another layout test to fail, the ruby layout tests are currently added to the Skipped list for Mac. After more investigation it turns out that the fault seems to lay with the single failing layout test, however.

*** Bug 3749 has been marked as a duplicate of this bug. ***

Can we remove the tests from the Windows Skipped List now?

I tried re-running these tests with the new Windows specific results, and they still failed on my machine. Any idea why that would be happening?

(In reply to comment #24) > I tried re-running these tests with the new Windows specific results, and they > still failed on my machine. Any idea why that would be happening? Not really :p - I generated the re-baselined results on an XP laptop, but perhaps something in the setup wasn't quite correct. Is there a way to have these auto-generated instead?

Created attachment 43029 [details] patch 5a - remove fast/ruby tests from Skipped list for Mac After https://bugs.webkit.org/show_bug.cgi?id=31200 has been fixed, there is no reason to skip the ruby layout tests for Mac any longer.

Roland: Ping?

(In reply to comment #27) > Roland: Ping? Yes, sorry, I should have posted an update here too: I am currently re-writing the layout tests to use latin characters - see https://bugs.webkit.org/show_bug.cgi?id=31865. I want to submit the baselines for both Mac and Windows there before re-enabling them, to avoid having some intermediate state between old and new versions.

Should we r- these patches then? I'm unclear on what should be done with this bug.

(In reply to comment #29) Sorry, I should have been more clear in my updates - patch 4 is indeed obsolete once I (finally) get to do the Windows baselines of the new Latin-only versions. As for patch 5a, that's basically still valid, I just wanted to get the baselines in before enabling the layout tests, followed by a "patch 5b" to also enable them for Windows. However, in order to avoid confusion, it may be easier to r- patch 5a here. I'll just re-add it to https://bugs.webkit.org/show_bug.cgi?id=31865 once I get around with the Windows baselines there (or open a new bug entry to enable both Mac and Windows layout tests).

Comment on attachment 42475 [details] patch 4 - re-baselined layout test results for Windows Mark patch 4 as obsolete.

Landed patch 5a as rev. 53531

Created attachment 47101 [details] patch 5b - remove fast/ruby tests from Skipped list for Win Last patch for full first implementation of ruby: remove ruby layout tests from Skipped list for Windows. After the landing of the new Latin-character only layout tests (https://bugs.webkit.org/show_bug.cgi?id=31865), and the resolving of the issue that caused other tests to fail (not actually caused by the ruby layout tests(cf. https://bugs.webkit.org/show_bug.cgi?id=31200, problem not actually caused by ruby), there's no reason to leave the ruby layout tests in the Skipped list for Windows.

Comment on attachment 47101 [details] patch 5b - remove fast/ruby tests from Skipped list for Win r=me In the future, please try to make new bugs for new patches instead of reusing the same bug - it's confusing when multiple independent changes are associated with the same bug. And webkit-patch makes it super easy to file a new bug for each patch.

Patch 5b landed as 53791, with follow-up fix in 53793