RESOLVED FIXED 142608
Disallow ruby base from having leading or trailing expansions
https://bugs.webkit.org/show_bug.cgi?id=142608
Summary Disallow ruby base from having leading or trailing expansions
Myles C. Maxfield
Reported 2015-03-11 21:01:41 PDT
Disallow ruby base from having leading nor trailing expansions
Attachments
No tests (12.29 KB, patch)
2015-03-11 21:02 PDT, Myles C. Maxfield
no flags
Archive of layout-test-results from ews105 for mac-mavericks-wk2 (674.35 KB, application/zip)
2015-03-11 21:43 PDT, Build Bot
no flags
Archive of layout-test-results from ews102 for mac-mavericks (650.39 KB, application/zip)
2015-03-11 21:59 PDT, Build Bot
no flags
WIP (21.62 KB, patch)
2015-03-13 10:22 PDT, Myles C. Maxfield
no flags
WIP (32.10 KB, patch)
2015-03-16 16:52 PDT, Myles C. Maxfield
no flags
WIP (34.50 KB, patch)
2015-03-16 17:21 PDT, Myles C. Maxfield
no flags
WIP (34.67 KB, patch)
2015-03-16 20:09 PDT, Myles C. Maxfield
no flags
Some tests (2.31 KB, text/html)
2015-03-16 20:09 PDT, Myles C. Maxfield
no flags
Some more tests (1014 bytes, text/html)
2015-03-17 13:13 PDT, Myles C. Maxfield
no flags
Simpler RTL test (426 bytes, text/html)
2015-03-17 19:25 PDT, Myles C. Maxfield
no flags
WIP (41.45 KB, patch)
2015-03-17 19:27 PDT, Myles C. Maxfield
no flags
WIP (56.15 KB, patch)
2015-03-18 11:33 PDT, Myles C. Maxfield
no flags
RTL Complex test (640 bytes, text/html)
2015-03-18 22:43 PDT, Myles C. Maxfield
no flags
WIP (60.87 KB, patch)
2015-03-18 22:50 PDT, Myles C. Maxfield
no flags
Very close to being finished (66.52 KB, patch)
2015-03-20 14:49 PDT, Myles C. Maxfield
no flags
Patch (75.71 KB, patch)
2015-03-20 20:34 PDT, Myles C. Maxfield
no flags
Archive of layout-test-results from ews101 for mac-mavericks (539.77 KB, application/zip)
2015-03-20 21:39 PDT, Build Bot
no flags
Archive of layout-test-results from ews107 for mac-mavericks-wk2 (572.77 KB, application/zip)
2015-03-20 21:45 PDT, Build Bot
no flags
Patch (75.73 KB, patch)
2015-03-21 00:57 PDT, Myles C. Maxfield
no flags
Patch (75.70 KB, patch)
2015-03-21 12:03 PDT, Myles C. Maxfield
no flags
WIP (123.21 KB, patch)
2015-03-27 18:45 PDT, Myles C. Maxfield
no flags
Patch (125.52 KB, patch)
2015-03-30 17:39 PDT, Myles C. Maxfield
no flags
Patch (50.63 KB, patch)
2015-04-02 09:45 PDT, Myles C. Maxfield
hyatt: review+
buildbot: commit-queue-
Archive of layout-test-results from ews100 for mac-mavericks (542.64 KB, application/zip)
2015-04-02 10:37 PDT, Build Bot
no flags
Archive of layout-test-results from ews107 for mac-mavericks-wk2 (600.15 KB, application/zip)
2015-04-02 10:45 PDT, Build Bot
no flags
Patch for landing (50.63 KB, patch)
2015-04-02 10:50 PDT, Myles C. Maxfield
buildbot: commit-queue-
Archive of layout-test-results from ews101 for mac-mavericks (536.68 KB, application/zip)
2015-04-02 11:42 PDT, Build Bot
no flags
Archive of layout-test-results from ews104 for mac-mavericks-wk2 (600.98 KB, application/zip)
2015-04-02 11:49 PDT, Build Bot
no flags
Patch for landing (50.65 KB, patch)
2015-04-02 11:55 PDT, Myles C. Maxfield
commit-queue: commit-queue-
Myles C. Maxfield
Comment 1 2015-03-11 21:02:29 PDT
Created attachment 248483 [details] No tests
Myles C. Maxfield
Comment 2 2015-03-11 21:07:40 PDT
Comment on attachment 248483 [details] No tests View in context: https://bugs.webkit.org/attachment.cgi?id=248483&action=review > Source/WebCore/rendering/RenderBlockLineLayout.cpp:801 > + textAlign = CENTER; This is where we could implement part of ruby-align
WebKit Commit Bot
Comment 3 2015-03-11 21:08:33 PDT
Attachment 248483 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Build Bot
Comment 4 2015-03-11 21:43:04 PDT
Comment on attachment 248483 [details] No tests Attachment 248483 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5723198331027456 New failing tests: fast/ruby/positioned-ruby-text.html fast/ruby/ruby-justification.html
Build Bot
Comment 5 2015-03-11 21:43:07 PDT
Created attachment 248488 [details] Archive of layout-test-results from ews105 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Build Bot
Comment 6 2015-03-11 21:59:16 PDT
Comment on attachment 248483 [details] No tests Attachment 248483 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5605930691461120 New failing tests: fast/ruby/positioned-ruby-text.html fast/ruby/ruby-justification.html
Build Bot
Comment 7 2015-03-11 21:59:19 PDT
Created attachment 248490 [details] Archive of layout-test-results from ews102 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-mavericks Platform: Mac OS X 10.9.5
Myles C. Maxfield
Comment 8 2015-03-12 08:17:17 PDT
Doesn't work with all-latin text, where expansion opportunities are not inserted at the end of every run.
Myles C. Maxfield
Comment 9 2015-03-13 10:22:39 PDT
Myles C. Maxfield
Comment 10 2015-03-16 16:52:47 PDT
Myles C. Maxfield
Comment 11 2015-03-16 17:21:11 PDT
Myles C. Maxfield
Comment 12 2015-03-16 20:09:14 PDT
Myles C. Maxfield
Comment 13 2015-03-16 20:09:29 PDT
Created attachment 248792 [details] Some tests
Myles C. Maxfield
Comment 14 2015-03-17 13:13:38 PDT
Created attachment 248862 [details] Some more tests
Myles C. Maxfield
Comment 15 2015-03-17 19:25:53 PDT
Created attachment 248900 [details] Simpler RTL test
Myles C. Maxfield
Comment 16 2015-03-17 19:27:25 PDT
Myles C. Maxfield
Comment 17 2015-03-18 11:33:19 PDT
Myles C. Maxfield
Comment 18 2015-03-18 22:43:49 PDT
Created attachment 249017 [details] RTL Complex test
Myles C. Maxfield
Comment 19 2015-03-18 22:50:34 PDT
Myles C. Maxfield
Comment 20 2015-03-20 14:49:15 PDT
Created attachment 249137 [details] Very close to being finished
Myles C. Maxfield
Comment 21 2015-03-20 20:34:04 PDT
Build Bot
Comment 22 2015-03-20 21:39:40 PDT
Comment on attachment 249158 [details] Patch Attachment 249158 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6424905121792000 New failing tests: fast/ruby/ruby-expansion-cjk-4.html
Build Bot
Comment 23 2015-03-20 21:39:48 PDT
Created attachment 249160 [details] Archive of layout-test-results from ews101 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-mavericks Platform: Mac OS X 10.9.5
Build Bot
Comment 24 2015-03-20 21:45:24 PDT
Comment on attachment 249158 [details] Patch Attachment 249158 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6313939809861632 New failing tests: fast/ruby/ruby-expansion-cjk-4.html
Build Bot
Comment 25 2015-03-20 21:45:29 PDT
Created attachment 249161 [details] Archive of layout-test-results from ews107 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Myles C. Maxfield
Comment 26 2015-03-21 00:57:47 PDT
Myles C. Maxfield
Comment 27 2015-03-21 12:03:34 PDT
Myles C. Maxfield
Comment 28 2015-03-21 16:29:44 PDT
Win appears to be a false negative
Dave Hyatt
Comment 29 2015-03-24 12:38:10 PDT
Comment on attachment 249176 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=249176&action=review r- > Source/WebCore/platform/graphics/WidthIterator.cpp:66 > + if (run.forcesLeadingExpansion() && !m_isAfterExpansion && !FontCascade::initialExpansionOpportunity(m_run.text(), m_run.ltr() ? LTR : RTL)) > + ++expansionOpportunityCount; > + if (run.forcesTrailingExpansion() && !isAfterExpansion) > + ++expansionOpportunityCount; > + else if (m_run.forbidsTrailingExpansion() && isAfterExpansion) > + --expansionOpportunityCount; Seems like with some cleanup, this could could move inside FontCascade::expansionOpportunityCount? Maybe I am not understanding the code completely though. > Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:154 > + if (run.forcesLeadingExpansion() && !m_afterExpansion && !FontCascade::initialExpansionOpportunity(m_run.text(), m_run.ltr() ? LTR : RTL)) > + ++expansionOpportunityCount; > + if (run.forcesTrailingExpansion() && !isAfterExpansion) > + ++expansionOpportunityCount; > + else if (m_run.forbidsTrailingExpansion() && isAfterExpansion) > + --expansionOpportunityCount; Same here. You had to repeat the same code. If you could shift this into FontCascade::expansionOpportunityCount, you could avoid duplicating code. > Source/WebCore/rendering/RenderBlockLineLayout.cpp:740 > + if (textBox.forceLeadingExpansion() && !FontCascade::initialExpansionOpportunity(renderText.stringView(), run->box()->direction())) > + ++opportunitiesInRun; > + if (textBox.forceTrailingExpansion() && !isAfterExpansion) > + ++opportunitiesInRun; Why don't you need the forbidsTrailingExpansion() case that removes an opportunity that you had in the other code that was just after FontCascade::expansionOpporunityCount? > Source/WebCore/rendering/RenderBlockLineLayout.cpp:761 > if (is<RenderRubyRun>(run->renderer()) && textAlign == JUSTIFY && run != trailingSpaceRun && downcast<RenderRubyRun>(run->renderer()).rubyBase()) { Seems like this code has gotten substantial enough to warrant its own helper function.
Myles C. Maxfield
Comment 30 2015-03-27 18:45:35 PDT
Myles C. Maxfield
Comment 31 2015-03-30 17:39:56 PDT
WebKit Commit Bot
Comment 32 2015-03-30 17:44:00 PDT
Attachment 249786 [details] did not pass style-queue: ERROR: Source/WebCore/platform/text/TextHelpers.h:31: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Total errors found: 1 in 59 files If any of these errors are false positives, please file a bug against check-webkit-style.
Myles C. Maxfield
Comment 33 2015-04-02 09:45:50 PDT
Dave Hyatt
Comment 34 2015-04-02 09:55:05 PDT
Comment on attachment 249987 [details] Patch r=me
Build Bot
Comment 35 2015-04-02 10:37:43 PDT
Comment on attachment 249987 [details] Patch Attachment 249987 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6112374041870336 New failing tests: fast/ruby/ruby-expansion-cjk-4.html
Build Bot
Comment 36 2015-04-02 10:37:47 PDT
Created attachment 249991 [details] Archive of layout-test-results from ews100 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-mavericks Platform: Mac OS X 10.9.5
Build Bot
Comment 37 2015-04-02 10:45:16 PDT
Comment on attachment 249987 [details] Patch Attachment 249987 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6677922450505728 New failing tests: fast/ruby/ruby-expansion-cjk-4.html
Build Bot
Comment 38 2015-04-02 10:45:21 PDT
Created attachment 249992 [details] Archive of layout-test-results from ews107 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Myles C. Maxfield
Comment 39 2015-04-02 10:50:34 PDT
Created attachment 249994 [details] Patch for landing
Build Bot
Comment 40 2015-04-02 11:42:12 PDT
Comment on attachment 249994 [details] Patch for landing Attachment 249994 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6119117140525056 New failing tests: fast/ruby/ruby-expansion-cjk-4.html
Build Bot
Comment 41 2015-04-02 11:42:17 PDT
Created attachment 249998 [details] Archive of layout-test-results from ews101 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-mavericks Platform: Mac OS X 10.9.5
Build Bot
Comment 42 2015-04-02 11:49:47 PDT
Comment on attachment 249994 [details] Patch for landing Attachment 249994 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4944574782898176 New failing tests: fast/ruby/ruby-expansion-cjk-4.html
Build Bot
Comment 43 2015-04-02 11:49:52 PDT
Created attachment 250000 [details] Archive of layout-test-results from ews104 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Myles C. Maxfield
Comment 44 2015-04-02 11:55:11 PDT
Created attachment 250001 [details] Patch for landing
WebKit Commit Bot
Comment 45 2015-04-02 13:15:40 PDT
Comment on attachment 250001 [details] Patch for landing Rejecting attachment 250001 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 250001, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in LayoutTests/ChangeLog contains OOPS!. Full output: http://webkit-queues.appspot.com/results/5119113663873024
Myles C. Maxfield
Comment 46 2015-04-02 13:30:15 PDT
Brent Fulgham
Comment 47 2015-04-02 16:51:56 PDT
These are tests are failing on Windows. Please add a skip (if relevant), or roll the patch out.
Brent Fulgham
Comment 48 2015-04-02 17:31:24 PDT
Actually, these tests are failing on EFL, GTK, and Windows. Do these changes rely on something Mac-specific?
Note You need to log in before you can comment on or make changes to this bug.