Disallow ruby base from having leading nor trailing expansions
Created attachment 248483 [details] No tests
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
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.
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
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
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
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
Doesn't work with all-latin text, where expansion opportunities are not inserted at the end of every run.
Created attachment 248589 [details] WIP
Created attachment 248769 [details] WIP
Created attachment 248770 [details] WIP
Created attachment 248789 [details] WIP
Created attachment 248792 [details] Some tests
Created attachment 248862 [details] Some more tests
Created attachment 248900 [details] Simpler RTL test
Created attachment 248901 [details] WIP
Created attachment 248946 [details] WIP
Created attachment 249017 [details] RTL Complex test
Created attachment 249018 [details] WIP
Created attachment 249137 [details] Very close to being finished
Created attachment 249158 [details] Patch
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
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
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
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
Created attachment 249168 [details] Patch
Created attachment 249176 [details] Patch
Win appears to be a false negative
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.
Created attachment 249633 [details] WIP
Created attachment 249786 [details] Patch
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.
Created attachment 249987 [details] Patch
Comment on attachment 249987 [details] Patch r=me
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
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
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
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
Created attachment 249994 [details] Patch for landing
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
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
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
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
Created attachment 250001 [details] Patch for landing
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
Committed r182286: <http://trac.webkit.org/changeset/182286>
These are tests are failing on Windows. Please add a skip (if relevant), or roll the patch out.
Actually, these tests are failing on EFL, GTK, and Windows. Do these changes rely on something Mac-specific?