Bug 77067 – Parenthesis in RTL fonts are not mirrored with SVG fonts

• Component: SVG
• Status: RESOLVED FIXED

[Philip Rogers]

When rendering RTL text some glyphs (e.g., parenthesis) are flipped according to the Unicode algorithm: http://www.unicode.org/reports/tr9/#Mirroring This mirroring is not being honored when rendering with an SVG font.

Original bug: http://code.google.com/p/chromium/issues/detail?id=108170&

[Philip Rogers]

Created attachment 124053 [details]
First-pass at fixing mirroring with SVG fonts

[Nikolas Zimmermann]

Comment on attachment 124053 [details]
First-pass at fixing mirroring with SVG fonts

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

Nice first stab, needs more tests and some speedups:

> Source/WebCore/svg/SVGFontData.cpp:142
> +    if (mirror)
> +        remainingTextInRun = replaceMirroredCharacters(remainingTextInRun.characters(), remainingTextInRun.length());
>      if (!currentCharacter && arabicForms.isEmpty())
>          arabicForms = charactersWithArabicForm(remainingTextInRun, mirror);

Danger Will Robinson :-) This can potentially break the arabic-form support - which already handles mirroring. Please add another testcase (you can grep for arabic-form and duplicate an existing) to see how it plays together with your testcase text data.

It's also a PITA that we're allocating a remainingTextInRun String object several times :( This needs to be optimized, applySVGGlyphSelection is hot.
You shoulnd't need to build a whole new string, but instead iterate over the remainingTextInRun by looping over its const UChar* buffer, and call mirroredChar on the individual characters.

[Philip Rogers]

Created attachment 124235 [details]
Update for fixing mirroring with SVG fonts

Added a test to show Arabic forms are preserved and cleaned up the glyph-selection-bidi-mirror.svg test.

[WebKit Review Bot]

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

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

Source/WebCore/svg/SVGFontData.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/svg/SVGFontData.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 8 files


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

[Philip Rogers]

(In reply to comment #2)
> (From update of attachment 124053 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124053&action=review
> 
> Nice first stab, needs more tests and some speedups:
> 
> > Source/WebCore/svg/SVGFontData.cpp:142
> > +    if (mirror)
> > +        remainingTextInRun = replaceMirroredCharacters(remainingTextInRun.characters(), remainingTextInRun.length());
> >      if (!currentCharacter && arabicForms.isEmpty())
> >          arabicForms = charactersWithArabicForm(remainingTextInRun, mirror);
> 
> Danger Will Robinson :-) This can potentially break the arabic-form support - which already handles mirroring. Please add another testcase (you can grep for arabic-form and duplicate an existing) to see how it plays together with your testcase text data.

Good eye :) I added the glyph-selection-arabic-forms.svg test and I think everything still works as it did (except now with correct mirroring!) I'm new to Arabic forms but if I understand them correctly, the original code did not handle mirroring for Arabic forms. A mirror parameter is passed to charactersWithArabicForm but only for determining whether the text should be processed ltr or rtl. I think it would be best to change that bool parameter from "mirror" to "rtl" in charactersWithArabicForm to prevent confusion--do you agree?

> 
> It's also a PITA that we're allocating a remainingTextInRun String object several times :( This needs to be optimized, applySVGGlyphSelection is hot.
> You shoulnd't need to build a whole new string, but instead iterate over the remainingTextInRun by looping over its const UChar* buffer, and call mirroredChar on the individual characters.

We definitely agree here but (per our IRC chat) I think it's best to leave this slightly slow function because the characters are immutable. If this ends up being a problem, a future optimization would be to do both mirroring and space normalization in a single pass.


Note: This patch will fail tests on most platforms at the moment. I need to add/update platform-specific test expectations when I get in the office tomorrow morning.

[Philip Rogers]

Created attachment 124241 [details]
Update for fixing mirroring with SVG fonts

(Fix import ordering and run check-webkit-style before uploading this time)

[WebKit Review Bot]

Comment on attachment 124241 [details]
Update for fixing mirroring with SVG fonts

Attachment 124241 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11265580

New failing tests:
svg/custom/glyph-selection-bidi-mirror.svg
svg/custom/glyph-selection-arabic-forms.svg

[Nikolas Zimmermann]

Comment on attachment 124241 [details]
Update for fixing mirroring with SVG fonts

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

r- for the lacking ChangeLog, and as there's more to do I think:

> Source/WebCore/ChangeLog:8
> +
> +        Tests: svg/custom/glyph-selection-arabic-forms.svg

Needs a real ChangeLog :-)

> Source/WebCore/svg/SVGFontData.cpp:277
> +        mirroredCharacters.append((UChar) mirroredChar(characters[i]));

You're casting from UChar32 to UChar here - doesn't this break any non-bmp glyphs?
What you're supposed to do is using:

        UChar32 character = 0;
        unsigned clusterLength = 0;
        SurrogatePairAwareTextIterator textIterator(characters, 0, length, length);
        while (textIterator.consume(character, clusterLength)) {

the SurrogatePairAwareTextIterator.. - see eg. SVGGlyphMap.h collectGlyphsForString(), where the 'remainingTextInRun' is being passed to.
The mirroring needs to happen on UChar32 basis - you wouldn't want to mirror parts of a surrogate pair, but instead the full UTF-16 code point, if I get this right.

Then to create a String again, you need to decompose those potentially mirrored UTF-16 UChar32, back into two utf-8 UChar pairs.
Now if mirroring the two parts of a surrogate pair, is the same as mirroring the full UChar32, then my comment is wrong, but I think this isn't the case.
Please double check.

[Nikolas Zimmermann]

(In reply to comment #7)
> (From update of attachment 124241 [details])
> Attachment 124241 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/11265580
> 
> New failing tests:
> svg/custom/glyph-selection-bidi-mirror.svg
> svg/custom/glyph-selection-arabic-forms.svg

You didn't update the cr expectations.

(In reply to comment #5)
> Good eye :) I added the glyph-selection-arabic-forms.svg test and I think everything still works as it did (except now with correct mirroring!) I'm new to Arabic forms but if I understand them correctly, the original code did not handle mirroring for Arabic forms. A mirror parameter is passed to charactersWithArabicForm but only for determining whether the text should be processed ltr or rtl. I think it would be best to change that bool parameter from "mirror" to "rtl" in charactersWithArabicForm to prevent confusion--do you agree?
Hah, good spot as well, yeah stupid that I named it mirror :-) Please rename to avoid the confusion.

> > It's also a PITA that we're allocating a remainingTextInRun String object several times :( This needs to be optimized, applySVGGlyphSelection is hot.
> > You shoulnd't need to build a whole new string, but instead iterate over the remainingTextInRun by looping over its const UChar* buffer, and call mirroredChar on the individual characters.
Meep, forgot that a String is immutable, ignore that comment..

> We definitely agree here but (per our IRC chat) I think it's best to leave this slightly slow function because the characters are immutable. If this ends up being a problem, a future optimization would be to do both mirroring and space normalization in a single pass.
... fuly agreed. It needs someone to create a perf testcase for this, and Instruments-it. If we ever see it in profiling, we can revisit this.

> Note: This patch will fail tests on most platforms at the moment. I need to add/update platform-specific test expectations when I get in the office tomorrow morning.
Ah okay, great.

[Philip Rogers]

Created attachment 124330 [details]
Update per reviewer comments

[Philip Rogers]

(In reply to comment #8)
> (From update of attachment 124241 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124241&action=review
> 
> r- for the lacking ChangeLog, and as there's more to do I think:
> 
> > Source/WebCore/ChangeLog:8
> > +
> > +        Tests: svg/custom/glyph-selection-arabic-forms.svg
> 
> Needs a real ChangeLog :-)

Added!

> 
> > Source/WebCore/svg/SVGFontData.cpp:277
> > +        mirroredCharacters.append((UChar) mirroredChar(characters[i]));
> 
> You're casting from UChar32 to UChar here - doesn't this break any non-bmp glyphs?
> What you're supposed to do is using:
> 
>         UChar32 character = 0;
>         unsigned clusterLength = 0;
>         SurrogatePairAwareTextIterator textIterator(characters, 0, length, length);
>         while (textIterator.consume(character, clusterLength)) {
> 
> the SurrogatePairAwareTextIterator.. - see eg. SVGGlyphMap.h collectGlyphsForString(), where the 'remainingTextInRun' is being passed to.
> The mirroring needs to happen on UChar32 basis - you wouldn't want to mirror parts of a surrogate pair, but instead the full UTF-16 code point, if I get this right.
> 
> Then to create a String again, you need to decompose those potentially mirrored UTF-16 UChar32, back into two utf-8 UChar pairs.
> Now if mirroring the two parts of a surrogate pair, is the same as mirroring the full UChar32, then my comment is wrong, but I think this isn't the case.
> Please double check.

This was a completely valid thing to check (and the new test glyph-selection-non-bmp.svg now proves it) but it turns out that the cast isn't problematic because WTF:UChar is 16 bits (unlike the 8bit version we're both used to). This cast was actually already occurring in Font::normalizeSpaces, which works similarly to replaceMirroredCharacters.

[WebKit Review Bot]

Comment on attachment 124330 [details]
Update per reviewer comments

Attachment 124330 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11349786

New failing tests:
svg/custom/glyph-selection-arabic-forms.svg

[Darin Adler]

Comment on attachment 124330 [details]
Update per reviewer comments

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

> Source/WebCore/svg/SVGFontData.cpp:279
> +    StringBuilder mirroredCharacters;
> +    mirroredCharacters.reserveCapacity(length);
> +
> +    for (unsigned i = 0; i < length; ++i)
> +        mirroredCharacters.append((UChar) mirroredChar(characters[i]));
> +
> +    return mirroredCharacters.toString();

This loop is incorrect. It walks through UTF-16 character codes rather than walking through Unicode characters so won’t correctly mirror non-BMP characters. The need to cast mirroredChar’s result to (UChar) emphasizes the mistake.

It’s straightforward to iterate the characters instead and handle surrogates correctly. There are examples of the use of U16_NEXT to read a character at a time in functions such as SegmentedFontData::containsCharacters and we can correctly write the characters in UTF-16 by using U16_LENGTH, U16_LEAD, and U16_TRAIL.

> Source/WebCore/svg/SVGFontData.h:59
> +    String replaceMirroredCharacters(const UChar* characters, unsigned length) const;

Naming this function that returns a newly created string “replace” is not so great. I would name it something like createStringWithMirroredCharacters.

[Philip Rogers]

Created attachment 124346 [details]
Update per reviewer comments

[Nikolas Zimmermann]

(In reply to comment #13)

> It’s straightforward to iterate the characters instead and handle surrogates correctly. There are examples of the use of U16_NEXT to read a character at a time in functions such as SegmentedFontData::containsCharacters and we can correctly write the characters in UTF-16 by using U16_LENGTH, U16_LEAD, and U16_TRAIL.
I suggested before your review, that he should use SurrogatePairAwareTextiterator - which encapsulates this while handling hiragana/katagana correctly. What do you think?

[Philip Rogers]

(In reply to comment #15)
> (In reply to comment #13)
> 
> > It’s straightforward to iterate the characters instead and handle surrogates correctly. There are examples of the use of U16_NEXT to read a character at a time in functions such as SegmentedFontData::containsCharacters and we can correctly write the characters in UTF-16 by using U16_LENGTH, U16_LEAD, and U16_TRAIL.
> I suggested before your review, that he should use SurrogatePairAwareTextiterator - which encapsulates this while handling hiragana/katagana correctly. What do you think?

Do you think it will have an effect? The mirrored characters are listed at http://www.unicode.org/Public/UNIDATA/BidiMirroring.txt and I don't think we even mirror the ones over 0xFFFF (they are listed as optional to mirror).

[WebKit Review Bot]

Comment on attachment 124346 [details]
Update per reviewer comments

Attachment 124346 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11191845

New failing tests:
svg/custom/glyph-selection-arabic-forms.svg

[Philip Rogers]

Comment on attachment 124346 [details]
Update per reviewer comments

I'm not sure why EWS is complaining about this; there's no information in the log and it passes locally on Linux.

[Adam Barth]

> I'm not sure why EWS is complaining about this; there's no information in the log and it passes locally on Linux.

The EWS might not have all its fonts set up exactly right.

[Philip Rogers]

(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #13)
> > 
> > > It’s straightforward to iterate the characters instead and handle surrogates correctly. There are examples of the use of U16_NEXT to read a character at a time in functions such as SegmentedFontData::containsCharacters and we can correctly write the characters in UTF-16 by using U16_LENGTH, U16_LEAD, and U16_TRAIL.
> > I suggested before your review, that he should use SurrogatePairAwareTextiterator - which encapsulates this while handling hiragana/katagana correctly. What do you think?
> 
> Do you think it will have an effect? The mirrored characters are listed at http://www.unicode.org/Public/UNIDATA/BidiMirroring.txt and I don't think we even mirror the ones over 0xFFFF (they are listed as optional to mirror).

I wanted to expand on this a bit to make sure I understand correctly. The SurrogatePairTextIterator handles combining some Japanese semi-voiced characters. For example:
べ - U+3079 - "HIRAGANA LETTER BE"
へ - U+3078 - "HIRAGANA LETTER HE"
゙ - U+3099 - "COMBINING KATAKANA-HIRAGANA VOICED SOUND MARK"
When iterating over the string "abcべdef" the iterator will combine the HE letter and the voice mark to form the Be character, so the iterator values will be 'a', 'b', 'c', 'Be', 'd', 'e', 'f'. The reason this doesn't matter for mirroring is that a relatively small set of characters are mirrored (such as parenthesis, brackets, etc) and they don't fall in the range of these Japanese voiced characters.


Re: the EWS failure:
I've installed some font packages in the process of tracking down this bug so it may be that my test expectations are the ones with the wrong font setup. I'll rebaseline on a vanilla linux box and put this patch back up tomorrow.

[Philip Rogers]

Created attachment 124563 [details]
Update glyph-selection-arabic-forms.svg to only use Khan character in the hopes that EWS Linux will pass

[Nikolas Zimmermann]

Comment on attachment 124563 [details]
Update glyph-selection-arabic-forms.svg to only use Khan character in the hopes that EWS Linux will pass

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

Patch looks great, r=me with a few comments:

> Source/WebCore/svg/SVGFontData.cpp:277
> +    for (unsigned i = 0; i < length; ) {

I would have used a while loop for this. No blocker.

> LayoutTests/ChangeLog:11
> +        which handles mirroring the characters when rendering selecting glyphs
> +        for SVG fonts. I also made a small cosmetic change in the function

s/rendering// ? I'm not sure what you mean otherwise.
The ChangeLog should be fixed at least before landing.

[Philip Rogers]

Created attachment 124972 [details]
Update per reviewer comments

[Philip Rogers]

Comment on attachment 124972 [details]
Update per reviewer comments

Neglected to switch to the while loop. Updated patch forthcoming

[Philip Rogers]

Created attachment 124974 [details]
Update per reviewer comments

Updated patch--now with more while!

[WebKit Review Bot]

Comment on attachment 124974 [details]
Update per reviewer comments

Clearing flags on attachment: 124974

Committed r106551: <http://trac.webkit.org/changeset/106551>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Philip Rogers]

Created attachment 125187 [details]
Update for test failures

This change slightly modifies the tests to not require specific fonts on the system. Before I had an example of the test output using non-SVG fonts as a comparison, but that ended up being problematic due to cross-platform high-unicode fonts. This change also requires rebaselining on Windows and GTK due to font aliasing issues.

[WebKit Review Bot]

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

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

Traceback (most recent call last):
  File "Tools/Scripts/check-webkit-style", line 48, in <module>
    sys.exit(CheckWebKitStyle().main())
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/main.py", line 154, in main
    patch_checker.check(patch)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/patchreader.py", line 66, in check
    self._text_file_reader.process_file(file_path=path, line_numbers=line_numbers)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/filereader.py", line 130, in process_file
    self._processor.process(lines, file_path, **kwargs)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checker.py", line 826, in process
    checker.check(lines)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/test_expectations.py", line 117, in check
    overrides=overrides)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/test_expectations.py", line 96, in check_test_expectations
    is_lint_mode=True, overrides=overrides)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py", line 721, in __init__
    self._add_skipped_tests(port.skipped_tests(tests))
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/layout_tests/port/webkit.py", line 376, in skipped_tests
    return self.skipped_layout_tests(test_list)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/layout_tests/port/webkit.py", line 372, in skipped_layout_tests
    tests_to_skip.update(self._skipped_tests_for_unsupported_features(test_list))
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/layout_tests/port/webkit.py", line 319, in _skipped_tests_for_unsupported_features
    if self._has_test_in_directories(self._missing_feature_to_skipped_tests().values(), test_list):
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/layout_tests/port/webkit.py", line 311, in _has_test_in_directories
    for directory, test in itertools.product(directories, test_list):
TypeError: 'NoneType' object is not iterable


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

[Nikolas Zimmermann]

Comment on attachment 125187 [details]
Update for test failures

Philip says this is safe, and applies locally, just the bots have problems. I'll trust him :-)

[Philip Rogers]

(In reply to comment #30)
> (From update of attachment 125187 [details])
> Philip says this is safe, and applies locally, just the bots have problems. I'll trust him :-)

Filed a bug for the patch issue and Ojan jumped on it and landed a fix! https://bugs.webkit.org/show_bug.cgi?id=77744

This patch appears to be stuck somewhere in cqland. I'll re-upload bright and early Monday morning so we can land this.

[Philip Rogers]

Created attachment 125637 [details]
Reupload now that check-webkit-style is fixed.

[Philip Rogers]

(In reply to comment #31)
> This patch appears to be stuck somewhere in cqland. I'll re-upload bright and early Monday morning so we can land this.

10AM = bright and early in engineer terms :)

[WebKit Review Bot]

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

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

Last 3072 characters of output:
xt:111:  Path does not exist. tables/mozilla_expected_failures/marvin/tables_caption_align_right.html  [test/expectations] [5]
LayoutTests/platform/win/test_expectations.txt:112:  Path does not exist. tables/mozilla_expected_failures/marvin/x_caption_align_left.xml  [test/expectations] [5]
LayoutTests/platform/win/test_expectations.txt:113:  Path does not exist. tables/mozilla_expected_failures/marvin/x_caption_align_right.xml  [test/expectations] [5]
LayoutTests/platform/win/test_expectations.txt:114:  Path does not exist. tables/mozilla_expected_failures/other/test4.html  [test/expectations] [5]
LayoutTests/platform/win/test_expectations.txt:115:  Path does not exist. tables/mozilla/bugs/bug29157.html  [test/expectations] [5]
LayoutTests/platform/win/test_expectations.txt:116:  Path does not exist. tables/mozilla/other/wa_table_thtd_rowspan.html  [test/expectations] [5]
LayoutTests/platform/win/test_expectations.txt:117:  Path does not exist. tables/mozilla/other/wa_table_tr_align.html  [test/expectations] [5]
LayoutTests/platform/win/test_expectations.txt:118:  Path does not exist. tables/mozilla_expected_failures/marvin/backgr_layers-show.html  [test/expectations] [5]
LayoutTests/platform/win/test_expectations.txt:119:  Path does not exist. tables/mozilla_expected_failures/marvin/table_frame_lhs.html  [test/expectations] [5]
LayoutTests/platform/win/test_expectations.txt:120:  Path does not exist. tables/mozilla_expected_failures/marvin/table_frame_rhs.html  [test/expectations] [5]
LayoutTests/platform/win/test_expectations.txt:121:  Path does not exist. fast/css/bidi-override-in-anonymous-block.html  [test/expectations] [5]
LayoutTests/platform/win/test_expectations.txt:122:  Path does not exist. fast/dom/HTMLTableElement/colSpan.html  [test/expectations] [5]
LayoutTests/platform/win/test_expectations.txt:123:  Path does not exist. fast/dom/HTMLTableElement/createCaption.html  [test/expectations] [5]
LayoutTests/platform/win/test_expectations.txt:124:  Path does not exist. fast/repaint/table-section-repaint.html  [test/expectations] [5]
LayoutTests/platform/win/test_expectations.txt:127:  Path does not exist. http/tests/media/video-buffering-repaints-controls.html  [test/expectations] [5]
LayoutTests/platform/win/test_expectations.txt:130:  Path does not exist. fast/css/text-overflow-input.html  [test/expectations] [5]
LayoutTests/platform/win/test_expectations.txt:133:  Path does not exist. fast/table/027.html  [test/expectations] [5]
LayoutTests/platform/win/test_expectations.txt:134:  Path does not exist. fast/table/027-vertical.html  [test/expectations] [5]
LayoutTests/platform/win/test_expectations.txt:137:  Path does not exist. svg/custom/glyph-selection-arabic-forms.svg  [test/expectations] [5]
LayoutTests/platform/win/test_expectations.txt:138:  Path does not exist. svg/custom/glyph-selection-bidi-mirror.svg  [test/expectations] [5]
LayoutTests/platform/win/test_expectations.txt:139:  Path does not exist. svg/custom/glyph-selection-non-bmp.svg  [test/expectations] [5]
Total errors found: 116 in 13 files


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

[Philip Rogers]

(In reply to comment #34)
> Attachment 125637 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
> If any of these errors are false positives, please file a bug against check-webkit-style.

I think this is a flake as well, as making a space change in platform/win/test_expectations.txt with a fresh, clean checkout will result in a similar error.

I filed another bug against check-webkit-style: https://bugs.webkit.org/show_bug.cgi?id=77873

[Nikolas Zimmermann]

Comment on attachment 125637 [details]
Reupload now that check-webkit-style is fixed.

Philip, can you find anyone manually landing this for you? Otherwise I'll try to do it today. Patch still looks good, I'll try cq+, let's see if it still applies.

[Nikolas Zimmermann]

Comment on attachment 125637 [details]
Reupload now that check-webkit-style is fixed.

Landed patch manually for Philip in r107211.