<rdar://problem/53056118>
Created attachment 377151 [details] Patch
Comment on attachment 377151 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=377151&action=review > Source/WebCore/editing/TextIterator.h:57 > +WEBCORE_EXPORT String foldQuoteMarks(String); Seems a little peculiar to have the argument the be "String" rather than "const String&". Also, I am a little concerned using this outside the context of ICU’s usearch. The folding here was specifically to work around what usearch does and does not do. This folding alone without all the other folding that usearch does is a sort of peculiar subset. Finally, it’s not great to have this in this header since it’s not really an "iteration" thing. Just because the file-local version was here doesn’t mean this is a good logical home for the function.
Comment on attachment 377151 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=377151&action=review > Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2342 > textForRange = plainTextReplacingNoBreakSpace(range.get()); Might be cleaner to integrate the quote mark folding into a replacement for the "plainTextReplacingNoBreakSpace" function instead of doing it as a separate step.
(In reply to Darin Adler from comment #3) > Comment on attachment 377151 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=377151&action=review > > > Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2342 > > textForRange = plainTextReplacingNoBreakSpace(range.get()); > > Might be cleaner to integrate the quote mark folding into a replacement for > the "plainTextReplacingNoBreakSpace" function instead of doing it as a > separate step. This was my first thought, but then I realized that we’d still need a way to fold quote marks for the original text passed into applyAutocorrectionInternal.
Comment on attachment 377151 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=377151&action=review >>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2342 >>> textForRange = plainTextReplacingNoBreakSpace(range.get()); >> >> Might be cleaner to integrate the quote mark folding into a replacement for the "plainTextReplacingNoBreakSpace" function instead of doing it as a separate step. > > This was my first thought, but then I realized that we’d still need a way to fold quote marks for the original text passed into applyAutocorrectionInternal. Does that text perhaps also ultimately come from a call to plainTextReplacingNoBreakSpace?
Comment on attachment 377151 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=377151&action=review >>>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2342 >>>> textForRange = plainTextReplacingNoBreakSpace(range.get()); >>> >>> Might be cleaner to integrate the quote mark folding into a replacement for the "plainTextReplacingNoBreakSpace" function instead of doing it as a separate step. >> >> This was my first thought, but then I realized that we’d still need a way to fold quote marks for the original text passed into applyAutocorrectionInternal. > > Does that text perhaps also ultimately come from a call to plainTextReplacingNoBreakSpace? It comes from UIKit (which, in turn, gets it from kbd).
(In reply to Darin Adler from comment #2) > Comment on attachment 377151 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=377151&action=review > > > Source/WebCore/editing/TextIterator.h:57 > > +WEBCORE_EXPORT String foldQuoteMarks(String); > > Seems a little peculiar to have the argument the be "String" rather than > "const String&”. Agreed. On closer inspection, we should be able to make this const String& (the only call site that depends on this is one of the SearchBuffer constructors that could be tweaked). I’ll post a v2 that does this. > Also, I am a little concerned using this outside the > context of ICU’s usearch. The folding here was specifically to work around > what usearch does and does not do. This folding alone without all the other > folding that usearch does is a sort of peculiar subset. I see. Perhaps the function could be renamed in such a way that makes this clear? > Finally, it’s not > great to have this in this header since it’s not really an "iteration" > thing. Just because the file-local version was here doesn’t mean this is a > good logical home for the function. Yeah, I had considered moving this (either to WTFString.h as an instance method of WTF::String, or a standalone function in that file, or just a function in StringCommon.h, or even a new header in WebCore) but decided to leave it out of this patch to make this change simpler. One thing to note is that wherever we decide to move it, we should probably keep foldQuoteMarks and foldQuoteMark in the same place. Would it be okay to do this in a followup patch?
Created attachment 377154 [details] Make foldQuoteMarks take a const String&
> One thing to note is that wherever we decide to move it, we should probably > keep foldQuoteMarks and foldQuoteMark in the same place. > > Would it be okay to do this in a followup patch? I filed <https://bugs.webkit.org/show_bug.cgi?id=201092> for this.
Comment on attachment 377154 [details] Make foldQuoteMarks take a const String& Clearing flags on attachment: 377154 Committed r249074: <https://trac.webkit.org/changeset/249074>
All reviewed patches have been landed. Closing bug.
Comment on attachment 377154 [details] Make foldQuoteMarks take a const String& View in context: https://bugs.webkit.org/attachment.cgi?id=377154&action=review > Source/WebCore/editing/TextIterator.cpp:-2422 > - foldQuoteMarks(m_target); What you did here seems to be fixing a serious bug! This was calling a function and ignoring its result so it wasn’t folding the quote marks. Glad you fixed it, but not at all happy that we didn’t have test coverage that showed us what was happening.
(In reply to Darin Adler from comment #12) > Comment on attachment 377154 [details] > Make foldQuoteMarks take a const String& > > View in context: > https://bugs.webkit.org/attachment.cgi?id=377154&action=review > > > Source/WebCore/editing/TextIterator.cpp:-2422 > > - foldQuoteMarks(m_target); > > What you did here seems to be fixing a serious bug! This was calling a > function and ignoring its result so it wasn’t folding the quote marks. Glad > you fixed it, but not at all happy that we didn’t have test coverage that > showed us what was happening. Indeed! One thing to note here is that this codepath is only compiled if UCONFIG_NO_COLLATION, which might explain the lack of test coverage (at least, on macOS and iOS bots).
Comment on attachment 377154 [details] Make foldQuoteMarks take a const String& View in context: https://bugs.webkit.org/attachment.cgi?id=377154&action=review >>> Source/WebCore/editing/TextIterator.cpp:-2422 >>> - foldQuoteMarks(m_target); >> >> What you did here seems to be fixing a serious bug! This was calling a function and ignoring its result so it wasn’t folding the quote marks. Glad you fixed it, but not at all happy that we didn’t have test coverage that showed us what was happening. > > Indeed! > > One thing to note here is that this codepath is only compiled if UCONFIG_NO_COLLATION, which might explain the lack of test coverage (at least, on macOS and iOS bots). Aha, yes, that makes sense. And I think that means this does not affect any Cocoa platforms.