[webkit-reviews] review granted: [Bug 216327] Text replacements at the beginning of a second line are replaced too early : [Attachment 408476] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Megan Gardner
<megan_gardner at apple.com>'s request for review:
Bug 216327: Text replacements at the beginning of a second line are replaced
too early
https://bugs.webkit.org/show_bug.cgi?id=216327

Attachment 408476: Patch

https://bugs.webkit.org/attachment.cgi?id=408476&action=review




--- Comment #9 from Darin Adler <darin at apple.com> ---
Comment on attachment 408476
  --> https://bugs.webkit.org/attachment.cgi?id=408476
Patch

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

review+ assuming tests pass on all Mac platform EWS testers (I wasn’t patient
enough)

> Source/WebCore/editing/Editor.cpp:2828
> -	   } else if (resultEndLocation <= automaticReplacementEndLocation &&
resultEndLocation >= paragraph.automaticReplacementStart()
> +	   } else if (automaticReplacementStartLocation <= resultEndLocation &&
resultEndLocation <= automaticReplacementEndLocation

Not sure why it’s important to reorder the two checks, here. Maybe we could
have just added "true" on this line and then not done the other refactoring
above. Just added true here, and added true above, without introducing the new
local variable.

> Source/WebCore/editing/TextCheckingHelper.cpp:211
> +uint64_t TextCheckingParagraph::automaticReplacementStart(bool oldBehaviour)
const

Hope this really is temporary: Boolean not explained at all in the header,
spelling here is British English. Not clear what "old" means.

Likely none of that matters if we quickly find a more straightforward fix.

> Source/WebCore/editing/TextCheckingHelper.cpp:214
> +	   return characterCount({ m_automaticReplacementRange.start,
m_automaticReplacementRange.start });

This is just a way to write:

   return 0;

> LayoutTests/platform/ios/TestExpectations:130
>  # Requires support for testing text replacement
>  editing/spelling/text-replacement-after-typing-to-word.html [ Skip ]
> +editing/spelling/text-replacement-first-word-second-line.html [ Skip ]

Seems like these should be (eventually) moved into a directory so we can
disable/enable them all at once if we add text replacement to a platform. I
imagine we might want more than 2 tests, even.