Bug 142608 – Disallow ruby base from having leading or trailing expansions

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

Disallow ruby base from having leading nor trailing expansions

[Myles C. Maxfield]

Created attachment 248483 [details]
No tests

[Myles C. Maxfield]

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]

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 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]

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 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]

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]

Doesn't work with all-latin text, where expansion opportunities are not inserted at the end of every run.

[Myles C. Maxfield]

Created attachment 248589 [details]
WIP

[Myles C. Maxfield]

Created attachment 248769 [details]
WIP

[Myles C. Maxfield]

Created attachment 248770 [details]
WIP

[Myles C. Maxfield]

Created attachment 248789 [details]
WIP

[Myles C. Maxfield]

Created attachment 248792 [details]
Some tests

[Myles C. Maxfield]

Created attachment 248862 [details]
Some more tests

[Myles C. Maxfield]

Created attachment 248900 [details]
Simpler RTL test

[Myles C. Maxfield]

Created attachment 248901 [details]
WIP

[Myles C. Maxfield]

Created attachment 248946 [details]
WIP

[Myles C. Maxfield]

Created attachment 249017 [details]
RTL Complex test

[Myles C. Maxfield]

Created attachment 249018 [details]
WIP

[Myles C. Maxfield]

Created attachment 249137 [details]
Very close to being finished

[Myles C. Maxfield]

Created attachment 249158 [details]
Patch

[Build Bot]

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]

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 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]

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]

Created attachment 249168 [details]
Patch

[Myles C. Maxfield]

Created attachment 249176 [details]
Patch

[Myles C. Maxfield]

Win appears to be a false negative

[Dave Hyatt]

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]

Created attachment 249633 [details]
WIP

[Myles C. Maxfield]

Created attachment 249786 [details]
Patch

[WebKit Commit Bot]

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]

Created attachment 249987 [details]
Patch

[Dave Hyatt]

Comment on attachment 249987 [details]
Patch

r=me

[Build Bot]

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]

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 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]

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]

Created attachment 249994 [details]
Patch for landing

[Build Bot]

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]

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 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]

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]

Created attachment 250001 [details]
Patch for landing

[WebKit Commit Bot]

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]

Committed r182286: <http://trac.webkit.org/changeset/182286>

[Brent Fulgham]

These are tests are failing on Windows. Please add a skip (if relevant), or roll the patch out.

[Brent Fulgham]

Actually, these tests are failing on EFL, GTK, and Windows. Do these changes rely on something Mac-specific?