| Summary: | Disallow ruby base from having leading or trailing expansions | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Myles C. Maxfield <mmaxfield> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Component: | New Bugs | Assignee: | Myles C. Maxfield <mmaxfield> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | bfulgham, buildbot, commit-queue, dino, esprehn+autocc, glenn, hyatt, jonlee, kondapallykalyan, rniwa, simon.fraser, thorton | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Bug Depends on: | 143273, 143292 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Bug Blocks: | 143354 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Description
Myles C. Maxfield
2015-03-11 21:01:41 PDT
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? |