Bug 62387 – Simplify ComplexTextController::collectComplexTextRuns()

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

Simplify ComplexTextController::collectComplexTextRuns()

Created attachment 96610 [details]
Always iterate the characters in logical order

[Darin Adler]

Comment on attachment 96610 [details]
Always iterate the characters in logical order

View in context: https://bugs.webkit.org/attachment.cgi?id=96610&action=review

Is the prepend operation fast enough? Would it be faster to reverse the vector after building it?

> Source/WebCore/platform/graphics/mac/ComplexTextControllerATSUI.cpp:346
> +        m_complexTextRuns.preprend(complexTextRun.release());

Typo: preprend

(In reply to comment #2)
> (From update of attachment 96610 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96610&action=review
> 
> Is the prepend operation fast enough? Would it be faster to reverse the vector after building it?

I think it could be faster. I will post another patch.

> 
> > Source/WebCore/platform/graphics/mac/ComplexTextControllerATSUI.cpp:346
> > +        m_complexTextRuns.preprend(complexTextRun.release());
> 
> Typo: preprend

Created attachment 96614 [details]
Always iterate the characters in logical order

[Darin Adler]

Comment on attachment 96614 [details]
Always iterate the characters in logical order

View in context: https://bugs.webkit.org/attachment.cgi?id=96614&action=review

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:253
> +        size_t runCount = m_complexTextRuns.size();
> +        for (size_t i = 0; i < runCount / 2; ++i)
> +            swap(m_complexTextRuns[i], m_complexTextRuns[runCount - i - 1]);

I’d like to see this “reverse” algorithm separated out into a function or function template. I could even imagine putting it into <wtf/Vector.h>.

(In reply to comment #5)
> (From update of attachment 96614 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96614&action=review
> 
> > Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:253
> > +        size_t runCount = m_complexTextRuns.size();
> > +        for (size_t i = 0; i < runCount / 2; ++i)
> > +            swap(m_complexTextRuns[i], m_complexTextRuns[runCount - i - 1]);
> 
> I’d like to see this “reverse” algorithm separated out into a function or function template. I could even imagine putting it into <wtf/Vector.h>.

Bug 62393.

In r88478. <http://trac.webkit.org/changeset/88478>

[Dimitri Glazkov (Google)]

This changed a bunch of expectations in Chromium Mac Snow Leopard:

http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Mac10.6/builds/4874/steps/webkit_tests/logs/stdio

http://test-esults.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&group=%40ToT%20-%20chromium.org&tests=fast%2Ftext%2Fatsui-negative-spacing-features.html%2Cfast%2Ftext%2Fatsui-spacing-features.html%2Csvg%2FW3C-I18N%2Fg-dirRTL-ubNone.svg%2Csvg%2FW3C-I18N%2Fg-dirRTL-ubOverride.svg%2Csvg%2FW3C-I18N%2Ftext-dirRTL-ubNone.svg%2Csvg%2FW3C-I18N%2Ftext-dirRTL-ubOverride.svg%2Csvg%2FW3C-I18N%2Ftspan-dirNone-ubOverride-in-rtl-context.svg%2Csvg%2FW3C-I18N%2Ftspan-dirRTL-ubEmbed-in-default-context.svg%2Csvg%2FW3C-I18N%2Ftspan-dirRTL-ubEmbed-in-ltr-context.svg%2Csvg%2FW3C-I18N%2Ftspan-dirRTL-ubOverride-in-default-context.svg%2Csvg%2FW3C-I18N%2Ftspan-dirRTL-ubOverride-in-ltr-context.svg%2Csvg%2FW3C-I18N%2Ftspan-dirRTL-ubOverride-in-rtl-context.svg

  fast/text/atsui-negative-spacing-features.html = IMAGE
  fast/text/atsui-spacing-features.html = IMAGE
  svg/W3C-I18N/g-dirRTL-ubNone.svg = IMAGE
  svg/W3C-I18N/g-dirRTL-ubOverride.svg = IMAGE
  svg/W3C-I18N/text-dirRTL-ubNone.svg = IMAGE
  svg/W3C-I18N/text-dirRTL-ubOverride.svg = IMAGE
  svg/W3C-I18N/tspan-dirNone-ubOverride-in-rtl-context.svg = IMAGE
  svg/W3C-I18N/tspan-dirRTL-ubEmbed-in-default-context.svg = IMAGE
  svg/W3C-I18N/tspan-dirRTL-ubEmbed-in-ltr-context.svg = IMAGE
  svg/W3C-I18N/tspan-dirRTL-ubOverride-in-default-context.svg = IMAGE
  svg/W3C-I18N/tspan-dirRTL-ubOverride-in-ltr-context.svg = IMAGE
  svg/W3C-I18N/tspan-dirRTL-ubOverride-in-rtl-context.svg = IMAGE

Is this expected?

(In reply to comment #8)
> This changed a bunch of expectations in Chromium Mac Snow Leopard:
> 
> http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Mac10.6/builds/4874/steps/webkit_tests/logs/stdio


> Is this expected?

No.

[Dimitri Glazkov (Google)]

(In reply to comment #9)
> (In reply to comment #8)
> > This changed a bunch of expectations in Chromium Mac Snow Leopard:
> > 
> > http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Mac10.6/builds/4874/steps/webkit_tests/logs/stdio
> 
> 
> > Is this expected?
> 
> No.

Should I roll the patch out?

I am looking into this, and in particular into the difference between the two versions of the patch.

Going to attach the fix here.

Created attachment 96647 [details]
Fix broken tests

[WebKit Review Bot]

Attachment 96647 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Fixed broken tests in r88487. <http://trac.webkit.org/r88487>

[Dimitri Glazkov (Google)]

(In reply to comment #15)
> Fixed broken tests in r88487. <http://trac.webkit.org/r88487>

Bitchin'!