Address post-review feedback after r238438
authorwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 23 Nov 2018 03:10:34 +0000 (03:10 +0000)
committerwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 23 Nov 2018 03:10:34 +0000 (03:10 +0000)
https://bugs.webkit.org/show_bug.cgi?id=191913

Reviewed by Ryosuke Niwa.

Source/WebCore:

Replace `bool` arguments to `FrameSelection::setSelectedRange`, `Editor::replaceSelectionWithText`, and
`Editor::replaceSelectionWithFragment` with `enum class`es instead. In particular, introduce the following:

FrameSelection::ShouldCloseTyping { No, Yes }
Editor::SelectReplacement { No, Yes }
Editor::SmartReplace { No, Yes }
Editor::MatchStyle { No, Yes }

* accessibility/AccessibilityObject.cpp:
(WebCore::AccessibilityObject::selectText):
* editing/Editor.cpp:
(WebCore::Editor::handleTextEvent):
(WebCore::Editor::replaceSelectionWithFragment):
(WebCore::Editor::replaceSelectionWithText):
(WebCore::Editor::setComposition):
(WebCore::Editor::markMisspellingsAfterTypingToWord):
(WebCore::Editor::changeBackToReplacedString):
(WebCore::Editor::transpose):
(WebCore::Editor::insertAttachment):

At various call sites, replace boolean arguments with named enums.

* editing/Editor.h:
* editing/EditorCommand.cpp:
(WebCore::expandSelectionToGranularity):
(WebCore::executeDeleteToMark):
(WebCore::executeSelectToMark):
* editing/FrameSelection.cpp:
(WebCore::FrameSelection::setSelectedRange):
* editing/FrameSelection.h:
* page/Page.cpp:
(WebCore::replaceRanges):

Avoid a bit of ref-count churn, and adjust a few functions to take `const Vector&`s instead of `Vector&&`s.

(WebCore::Page::replaceRangesWithText):
(WebCore::Page::replaceSelectionWithText):
* page/Page.h:

Source/WebKit:

Replace boolean arguments to setSelectedRange, replaceSelectionWithText and replaceSelectionWithFragment with
enum flags, and tweak a couple of functions to take `const Vector&` instead of `Vector&&`.

* WebProcess/InjectedBundle/API/c/WKBundlePage.cpp:
(WKBundlePageReplaceStringMatches):
* WebProcess/WebPage/FindController.cpp:
(WebKit::FindController::replaceMatches):
* WebProcess/WebPage/FindController.h:
* WebProcess/WebPage/WebPage.cpp:
(WebKit::WebPage::replaceStringMatchesFromInjectedBundle):
(WebKit::WebPage::replaceMatches):
(WebKit::WebPage::replaceSelectionWithText):
* WebProcess/WebPage/WebPage.h:
* WebProcess/WebPage/ios/WebPageIOS.mm:
(WebKit::WebPage::selectWithGesture):
(WebKit::WebPage::updateSelectionWithTouches):
(WebKit::WebPage::selectWithTwoTouches):
(WebKit::WebPage::extendSelection):
(WebKit::WebPage::selectWordBackward):
(WebKit::WebPage::moveSelectionByOffset):
(WebKit::WebPage::selectPositionAtPoint):
(WebKit::WebPage::selectPositionAtBoundaryWithDirection):
(WebKit::WebPage::moveSelectionAtBoundaryWithDirection):
(WebKit::WebPage::selectTextWithGranularityAtPoint):
(WebKit::WebPage::updateSelectionWithExtentPointAndBoundary):
(WebKit::WebPage::updateSelectionWithExtentPoint):
(WebKit::WebPage::replaceSelectedText):
(WebKit::WebPage::replaceDictatedText):
(WebKit::WebPage::syncApplyAutocorrection):

Source/WebKitLegacy/mac:

* WebView/WebFrame.mm:
(-[WebFrame setSelectedDOMRange:affinity:closeTyping:]):
(-[WebFrame _replaceSelectionWithFragment:selectReplacement:smartReplace:matchStyle:]):
* WebView/WebView.mm:
(-[WebView setSelectedDOMRange:affinity:]):

Source/WebKitLegacy/win:

* AccessibleTextImpl.cpp:
(AccessibleText::replaceText):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@238454 268f45cc-cd09-0410-ab3c-d52691b4dbfc

21 files changed:
Source/WebCore/ChangeLog
Source/WebCore/accessibility/AccessibilityObject.cpp
Source/WebCore/editing/Editor.cpp
Source/WebCore/editing/Editor.h
Source/WebCore/editing/EditorCommand.cpp
Source/WebCore/editing/FrameSelection.cpp
Source/WebCore/editing/FrameSelection.h
Source/WebCore/page/Page.cpp
Source/WebCore/page/Page.h
Source/WebKit/ChangeLog
Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp
Source/WebKit/WebProcess/WebPage/FindController.cpp
Source/WebKit/WebProcess/WebPage/FindController.h
Source/WebKit/WebProcess/WebPage/WebPage.cpp
Source/WebKit/WebProcess/WebPage/WebPage.h
Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm
Source/WebKitLegacy/mac/ChangeLog
Source/WebKitLegacy/mac/WebView/WebFrame.mm
Source/WebKitLegacy/mac/WebView/WebView.mm
Source/WebKitLegacy/win/AccessibleTextImpl.cpp
Source/WebKitLegacy/win/ChangeLog

index 87b934c..49cc123 100644 (file)
@@ -1,3 +1,49 @@
+2018-11-22  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        Address post-review feedback after r238438
+        https://bugs.webkit.org/show_bug.cgi?id=191913
+
+        Reviewed by Ryosuke Niwa.
+
+        Replace `bool` arguments to `FrameSelection::setSelectedRange`, `Editor::replaceSelectionWithText`, and
+        `Editor::replaceSelectionWithFragment` with `enum class`es instead. In particular, introduce the following:
+
+        FrameSelection::ShouldCloseTyping { No, Yes }
+        Editor::SelectReplacement { No, Yes }
+        Editor::SmartReplace { No, Yes }
+        Editor::MatchStyle { No, Yes }
+
+        * accessibility/AccessibilityObject.cpp:
+        (WebCore::AccessibilityObject::selectText):
+        * editing/Editor.cpp:
+        (WebCore::Editor::handleTextEvent):
+        (WebCore::Editor::replaceSelectionWithFragment):
+        (WebCore::Editor::replaceSelectionWithText):
+        (WebCore::Editor::setComposition):
+        (WebCore::Editor::markMisspellingsAfterTypingToWord):
+        (WebCore::Editor::changeBackToReplacedString):
+        (WebCore::Editor::transpose):
+        (WebCore::Editor::insertAttachment):
+
+        At various call sites, replace boolean arguments with named enums.
+
+        * editing/Editor.h:
+        * editing/EditorCommand.cpp:
+        (WebCore::expandSelectionToGranularity):
+        (WebCore::executeDeleteToMark):
+        (WebCore::executeSelectToMark):
+        * editing/FrameSelection.cpp:
+        (WebCore::FrameSelection::setSelectedRange):
+        * editing/FrameSelection.h:
+        * page/Page.cpp:
+        (WebCore::replaceRanges):
+
+        Avoid a bit of ref-count churn, and adjust a few functions to take `const Vector&`s instead of `Vector&&`s.
+
+        (WebCore::Page::replaceRangesWithText):
+        (WebCore::Page::replaceSelectionWithText):
+        * page/Page.h:
+
 2018-11-21  Ryosuke Niwa  <rniwa@webkit.org>
 
         Modernize SVGURIReference::targetElementFromIRIString
index 7bc452c..df1331d 100644 (file)
@@ -838,7 +838,7 @@ String AccessibilityObject::selectText(AccessibilitySelectTextCriteria* criteria
         
         String closestString = closestStringRange->text();
         bool replaceSelection = false;
-        if (frame->selection().setSelectedRange(closestStringRange.get(), DOWNSTREAM, true)) {
+        if (frame->selection().setSelectedRange(closestStringRange.get(), DOWNSTREAM, FrameSelection::ShouldCloseTyping::Yes)) {
             switch (activity) {
             case AccessibilitySelectTextActivity::FindAndCapitalize:
                 replacementString = capitalize(closestString, ' '); // FIXME: Needs to take locale into account to work correctly.
@@ -871,7 +871,7 @@ String AccessibilityObject::selectText(AccessibilitySelectTextCriteria* criteria
             // A bit obvious, but worth noting the API contract for this method is that we should
             // return the replacement string when replacing, but the selected string if not.
             if (replaceSelection) {
-                frame->editor().replaceSelectionWithText(replacementString, true, true);
+                frame->editor().replaceSelectionWithText(replacementString, Editor::SelectReplacement::Yes, Editor::SmartReplace::Yes);
                 return replacementString;
             }
             
index 2b65bdd..0b0cdaf 100644 (file)
@@ -310,9 +310,9 @@ bool Editor::handleTextEvent(TextEvent& event)
             if (client()->performsTwoStepPaste(event.pastingFragment()))
                 return true;
 #endif
-            replaceSelectionWithFragment(*event.pastingFragment(), false, event.shouldSmartReplace(), event.shouldMatchStyle(), EditAction::Paste, event.mailBlockquoteHandling());
+            replaceSelectionWithFragment(*event.pastingFragment(), SelectReplacement::No, event.shouldSmartReplace() ? SmartReplace::Yes : SmartReplace::No, event.shouldMatchStyle() ? MatchStyle::Yes : MatchStyle::No, EditAction::Paste, event.mailBlockquoteHandling());
         } else
-            replaceSelectionWithText(event.data(), false, event.shouldSmartReplace(), EditAction::Paste);
+            replaceSelectionWithText(event.data(), SelectReplacement::No, event.shouldSmartReplace() ? SmartReplace::Yes : SmartReplace::No, EditAction::Paste);
         return true;
     }
 
@@ -639,7 +639,7 @@ bool Editor::shouldInsertFragment(DocumentFragment& fragment, Range* replacingDO
     return client()->shouldInsertNode(&fragment, replacingDOMRange, givenAction);
 }
 
-void Editor::replaceSelectionWithFragment(DocumentFragment& fragment, bool selectReplacement, bool smartReplace, bool matchStyle, EditAction editingAction, MailBlockquoteHandling mailBlockquoteHandling)
+void Editor::replaceSelectionWithFragment(DocumentFragment& fragment, SelectReplacement selectReplacement, SmartReplace smartReplace, MatchStyle matchStyle, EditAction editingAction, MailBlockquoteHandling mailBlockquoteHandling)
 {
     VisibleSelection selection = m_frame.selection().selection();
     if (selection.isNone() || !selection.isContentEditable())
@@ -650,11 +650,11 @@ void Editor::replaceSelectionWithFragment(DocumentFragment& fragment, bool selec
         replacedText = AccessibilityReplacedText(selection);
 
     OptionSet<ReplaceSelectionCommand::CommandOption> options { ReplaceSelectionCommand::PreventNesting, ReplaceSelectionCommand::SanitizeFragment };
-    if (selectReplacement)
+    if (selectReplacement == SelectReplacement::Yes)
         options.add(ReplaceSelectionCommand::SelectReplacement);
-    if (smartReplace)
+    if (smartReplace == SmartReplace::Yes)
         options.add(ReplaceSelectionCommand::SmartReplace);
-    if (matchStyle)
+    if (matchStyle == MatchStyle::Yes)
         options.add(ReplaceSelectionCommand::MatchStyle);
     if (mailBlockquoteHandling == MailBlockquoteHandling::IgnoreBlockquote)
         options.add(ReplaceSelectionCommand::IgnoreMailBlockquote);
@@ -685,13 +685,13 @@ void Editor::replaceSelectionWithFragment(DocumentFragment& fragment, bool selec
         m_spellChecker->requestCheckingFor(request.releaseNonNull());
 }
 
-void Editor::replaceSelectionWithText(const String& text, bool selectReplacement, bool smartReplace, EditAction editingAction)
+void Editor::replaceSelectionWithText(const String& text, SelectReplacement selectReplacement, SmartReplace smartReplace, EditAction editingAction)
 {
     RefPtr<Range> range = selectedRange();
     if (!range)
         return;
 
-    replaceSelectionWithFragment(createFragmentFromText(*range, text), selectReplacement, smartReplace, true, editingAction);
+    replaceSelectionWithFragment(createFragmentFromText(*range, text), selectReplacement, smartReplace, MatchStyle::Yes, editingAction);
 }
 
 RefPtr<Range> Editor::selectedRange()
@@ -2023,7 +2023,7 @@ void Editor::setComposition(const String& text, const Vector<CompositionUnderlin
             unsigned start = std::min(baseOffset + selectionStart, extentOffset);
             unsigned end = std::min(std::max(start, baseOffset + selectionEnd), extentOffset);
             RefPtr<Range> selectedRange = Range::create(baseNode->document(), baseNode, start, baseNode, end);
-            m_frame.selection().setSelectedRange(selectedRange.get(), DOWNSTREAM, false);
+            m_frame.selection().setSelectedRange(selectedRange.get(), DOWNSTREAM, FrameSelection::ShouldCloseTyping::No);
         }
     }
 
@@ -2517,7 +2517,7 @@ void Editor::markMisspellingsAfterTypingToWord(const VisiblePosition &wordStart,
 
         if (!m_frame.editor().shouldInsertText(autocorrectedString, misspellingRange.get(), EditorInsertAction::Typed))
             return;
-        m_frame.editor().replaceSelectionWithText(autocorrectedString, false, false, EditAction::Insert);
+        m_frame.editor().replaceSelectionWithText(autocorrectedString, SelectReplacement::No, SmartReplace::No, EditAction::Insert);
 
         // Reset the charet one character further.
         m_frame.selection().moveTo(m_frame.selection().selection().end());
@@ -2875,7 +2875,7 @@ void Editor::changeBackToReplacedString(const String& replacedString)
     
     m_alternativeTextController->recordAutocorrectionResponse(AutocorrectionResponse::Reverted, replacedString, selection.get());
     TextCheckingParagraph paragraph(*selection);
-    replaceSelectionWithText(replacedString, false, false, EditAction::Insert);
+    replaceSelectionWithText(replacedString, SelectReplacement::No, SmartReplace::No, EditAction::Insert);
     auto changedRange = paragraph.subrange(paragraph.checkingStart(), replacedString.length());
     changedRange->startContainer().document().markers().addMarker(changedRange.ptr(), DocumentMarker::Replacement, String());
     m_alternativeTextController->markReversed(changedRange);
@@ -3121,7 +3121,7 @@ void Editor::transpose()
     // Insert the transposed characters.
     if (!shouldInsertText(transposed, range.get(), EditorInsertAction::Typed))
         return;
-    replaceSelectionWithText(transposed, false, false, EditAction::Insert);
+    replaceSelectionWithText(transposed, SelectReplacement::No, SmartReplace::No, EditAction::Insert);
 }
 
 void Editor::addRangeToKillRing(const Range& range, KillRingInsertionMode mode)
@@ -4098,7 +4098,7 @@ void Editor::insertAttachment(const String& identifier, std::optional<uint64_t>&
     auto fragmentToInsert = document().createDocumentFragment();
     fragmentToInsert->appendChild(attachment.get());
 
-    replaceSelectionWithFragment(fragmentToInsert.get(), false, false, true);
+    replaceSelectionWithFragment(fragmentToInsert.get(), SelectReplacement::No, SmartReplace::No, MatchStyle::Yes);
 }
 
 #endif // ENABLE(ATTACHMENT_ELEMENT)
index e8ffcc8..7a14edc 100644 (file)
@@ -438,8 +438,11 @@ public:
     void textDidChangeInTextArea(Element*);
     WEBCORE_EXPORT WritingDirection baseWritingDirectionForSelectionStart() const;
 
-    WEBCORE_EXPORT void replaceSelectionWithFragment(DocumentFragment&, bool selectReplacement, bool smartReplace, bool matchStyle, EditAction = EditAction::Insert, MailBlockquoteHandling = MailBlockquoteHandling::RespectBlockquote);
-    WEBCORE_EXPORT void replaceSelectionWithText(const String&, bool selectReplacement, bool smartReplace, EditAction = EditAction::Insert);
+    enum class SelectReplacement : bool { No, Yes };
+    enum class SmartReplace : bool { No, Yes };
+    enum class MatchStyle : bool { No, Yes };
+    WEBCORE_EXPORT void replaceSelectionWithFragment(DocumentFragment&, SelectReplacement, SmartReplace, MatchStyle, EditAction = EditAction::Insert, MailBlockquoteHandling = MailBlockquoteHandling::RespectBlockquote);
+    WEBCORE_EXPORT void replaceSelectionWithText(const String&, SelectReplacement, SmartReplace, EditAction = EditAction::Insert);
     WEBCORE_EXPORT bool selectionStartHasMarkerFor(DocumentMarker::MarkerType, int from, int length) const;
     void updateMarkersForWordsAffectedByEditing(bool onlyHandleWordsContainingSelection);
     void deletedAutocorrectionAtPosition(const Position&, const String& originalString);
index 86f9ccc..ca6df10 100644 (file)
@@ -185,7 +185,7 @@ static bool expandSelectionToGranularity(Frame& frame, TextGranularity granulari
     EAffinity affinity = selection.affinity();
     if (!frame.editor().client()->shouldChangeSelectedRange(oldRange.get(), newRange.get(), affinity, false))
         return false;
-    frame.selection().setSelectedRange(newRange.get(), affinity, true);
+    frame.selection().setSelectedRange(newRange.get(), affinity, FrameSelection::ShouldCloseTyping::Yes);
     return true;
 }
 
@@ -355,7 +355,7 @@ static bool executeDeleteToMark(Frame& frame, Event*, EditorCommandSource, const
     RefPtr<Range> mark = frame.editor().mark().toNormalizedRange();
     FrameSelection& selection = frame.selection();
     if (mark && frame.editor().selectedRange()) {
-        bool selected = selection.setSelectedRange(unionDOMRanges(*mark, *frame.editor().selectedRange()).get(), DOWNSTREAM, true);
+        bool selected = selection.setSelectedRange(unionDOMRanges(*mark, *frame.editor().selectedRange()).get(), DOWNSTREAM, FrameSelection::ShouldCloseTyping::Yes);
         ASSERT(selected);
         if (!selected)
             return false;
@@ -1030,7 +1030,7 @@ static bool executeSelectToMark(Frame& frame, Event*, EditorCommandSource, const
         PAL::systemBeep();
         return false;
     }
-    frame.selection().setSelectedRange(unionDOMRanges(*mark, *selection).get(), DOWNSTREAM, true);
+    frame.selection().setSelectedRange(unionDOMRanges(*mark, *selection).get(), DOWNSTREAM, FrameSelection::ShouldCloseTyping::Yes);
     return true;
 }
 
index b2c7c5e..424e32e 100644 (file)
@@ -1979,7 +1979,7 @@ void FrameSelection::selectAll()
     }
 }
 
-bool FrameSelection::setSelectedRange(Range* range, EAffinity affinity, bool closeTyping, EUserTriggered userTriggered)
+bool FrameSelection::setSelectedRange(Range* range, EAffinity affinity, ShouldCloseTyping closeTyping, EUserTriggered userTriggered)
 {
     if (!range)
         return false;
@@ -1994,7 +1994,7 @@ bool FrameSelection::setSelectedRange(Range* range, EAffinity affinity, bool clo
 #endif
 
     OptionSet<SetSelectionOption> selectionOptions {  ClearTypingStyle };
-    if (closeTyping)
+    if (closeTyping == ShouldCloseTyping::Yes)
         selectionOptions.add(CloseTyping);
 
     if (userTriggered == UserTriggered) {
index d19f365..6d752df 100644 (file)
@@ -150,7 +150,9 @@ public:
 
     const VisibleSelection& selection() const { return m_selection; }
     WEBCORE_EXPORT void setSelection(const VisibleSelection&, OptionSet<SetSelectionOption> = defaultSetSelectionOptions(), AXTextStateChangeIntent = AXTextStateChangeIntent(), CursorAlignOnScroll = AlignCursorOnScrollIfNeeded, TextGranularity = CharacterGranularity);
-    WEBCORE_EXPORT bool setSelectedRange(Range*, EAffinity, bool closeTyping, EUserTriggered = NotUserTriggered);
+
+    enum class ShouldCloseTyping : bool { No, Yes };
+    WEBCORE_EXPORT bool setSelectedRange(Range*, EAffinity, ShouldCloseTyping, EUserTriggered = NotUserTriggered);
     WEBCORE_EXPORT void selectAll();
     WEBCORE_EXPORT void clear();
     void prepareForDestruction();
index c9a60e5..c493697 100644 (file)
@@ -772,7 +772,7 @@ struct FindReplacementRange {
     size_t length { 0 };
 };
 
-static void replaceRanges(Page& page, Vector<FindReplacementRange>&& ranges, const String& replacementText)
+static void replaceRanges(Page& page, const Vector<FindReplacementRange>& ranges, const String& replacementText)
 {
     HashMap<RefPtr<ContainerNode>, Vector<FindReplacementRange>> rangesByContainerNode;
     for (auto& range : ranges) {
@@ -819,7 +819,7 @@ static void replaceRanges(Page& page, Vector<FindReplacementRange>&& ranges, con
         return frameToTraversalIndexMap.get(firstFrame) > frameToTraversalIndexMap.get(secondFrame);
     });
 
-    for (auto container : containerNodesInOrderOfReplacement) {
+    for (auto& container : containerNodesInOrderOfReplacement) {
         auto frame = makeRefPtr(container->document().frame());
         if (!frame)
             continue;
@@ -831,13 +831,13 @@ static void replaceRanges(Page& page, Vector<FindReplacementRange>&& ranges, con
             if (!range || range->collapsed())
                 continue;
 
-            frame->selection().setSelectedRange(range.get(), DOWNSTREAM, true);
-            frame->editor().replaceSelectionWithText(replacementText, true, false, EditAction::InsertReplacement);
+            frame->selection().setSelectedRange(range.get(), DOWNSTREAM, FrameSelection::ShouldCloseTyping::Yes);
+            frame->editor().replaceSelectionWithText(replacementText, Editor::SelectReplacement::Yes, Editor::SmartReplace::No, EditAction::InsertReplacement);
         }
     }
 }
 
-uint32_t Page::replaceRangesWithText(Vector<Ref<Range>>&& rangesToReplace, const String& replacementText, bool selectionOnly)
+uint32_t Page::replaceRangesWithText(const Vector<Ref<Range>>& rangesToReplace, const String& replacementText, bool selectionOnly)
 {
     // FIXME: In the future, we should respect the `selectionOnly` flag by checking whether each range being replaced is
     // contained within its frame's selection.
@@ -866,7 +866,7 @@ uint32_t Page::replaceRangesWithText(Vector<Ref<Range>>&& rangesToReplace, const
         replacementRanges.append({ WTFMove(highestRoot), replacementLocation, replacementLength });
     }
 
-    replaceRanges(*this, WTFMove(replacementRanges), replacementText);
+    replaceRanges(*this, replacementRanges, replacementText);
     return rangesToReplace.size();
 }
 
@@ -878,7 +878,7 @@ uint32_t Page::replaceSelectionWithText(const String& replacementText)
         return 0;
 
     auto editAction = selection.isRange() ? EditAction::InsertReplacement : EditAction::Insert;
-    frame->editor().replaceSelectionWithText(replacementText, true, false, editAction);
+    frame->editor().replaceSelectionWithText(replacementText, Editor::SelectReplacement::Yes, Editor::SmartReplace::No, editAction);
     return 1;
 }
 
index f840d11..f8465aa 100644 (file)
@@ -278,7 +278,7 @@ public:
     bool tabKeyCyclesThroughElements() const { return m_tabKeyCyclesThroughElements; }
 
     WEBCORE_EXPORT bool findString(const String&, FindOptions, DidWrap* = nullptr);
-    WEBCORE_EXPORT uint32_t replaceRangesWithText(Vector<Ref<Range>>&& rangesToReplace, const String& replacementText, bool selectionOnly);
+    WEBCORE_EXPORT uint32_t replaceRangesWithText(const Vector<Ref<Range>>& rangesToReplace, const String& replacementText, bool selectionOnly);
     WEBCORE_EXPORT uint32_t replaceSelectionWithText(const String& replacementText);
 
     WEBCORE_EXPORT RefPtr<Range> rangeOfString(const String&, Range*, FindOptions);
index 1877934..e6fe8c1 100644 (file)
@@ -1,3 +1,40 @@
+2018-11-22  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        Address post-review feedback after r238438
+        https://bugs.webkit.org/show_bug.cgi?id=191913
+
+        Reviewed by Ryosuke Niwa.
+
+        Replace boolean arguments to setSelectedRange, replaceSelectionWithText and replaceSelectionWithFragment with
+        enum flags, and tweak a couple of functions to take `const Vector&` instead of `Vector&&`.
+
+        * WebProcess/InjectedBundle/API/c/WKBundlePage.cpp:
+        (WKBundlePageReplaceStringMatches):
+        * WebProcess/WebPage/FindController.cpp:
+        (WebKit::FindController::replaceMatches):
+        * WebProcess/WebPage/FindController.h:
+        * WebProcess/WebPage/WebPage.cpp:
+        (WebKit::WebPage::replaceStringMatchesFromInjectedBundle):
+        (WebKit::WebPage::replaceMatches):
+        (WebKit::WebPage::replaceSelectionWithText):
+        * WebProcess/WebPage/WebPage.h:
+        * WebProcess/WebPage/ios/WebPageIOS.mm:
+        (WebKit::WebPage::selectWithGesture):
+        (WebKit::WebPage::updateSelectionWithTouches):
+        (WebKit::WebPage::selectWithTwoTouches):
+        (WebKit::WebPage::extendSelection):
+        (WebKit::WebPage::selectWordBackward):
+        (WebKit::WebPage::moveSelectionByOffset):
+        (WebKit::WebPage::selectPositionAtPoint):
+        (WebKit::WebPage::selectPositionAtBoundaryWithDirection):
+        (WebKit::WebPage::moveSelectionAtBoundaryWithDirection):
+        (WebKit::WebPage::selectTextWithGranularityAtPoint):
+        (WebKit::WebPage::updateSelectionWithExtentPointAndBoundary):
+        (WebKit::WebPage::updateSelectionWithExtentPoint):
+        (WebKit::WebPage::replaceSelectedText):
+        (WebKit::WebPage::replaceDictatedText):
+        (WebKit::WebPage::syncApplyAutocorrection):
+
 2018-11-22  Mark Lam  <mark.lam@apple.com>
 
         Rollout r238432: Breaks internal Mac builds.
index 8bf86ae..7993057 100644 (file)
@@ -461,11 +461,12 @@ void WKBundlePageReplaceStringMatches(WKBundlePageRef pageRef, WKArrayRef matchI
     Vector<uint32_t> indices;
     indices.reserveInitialCapacity(matchIndices->size());
 
-    for (size_t arrayIndex = 0; arrayIndex < matchIndices->size(); ++arrayIndex) {
-        if (auto* indexAsObject = matchIndices->at<API::UInt64>(arrayIndex))
+    auto numberOfMatchIndices = matchIndices->size();
+    for (size_t i = 0; i < numberOfMatchIndices; ++i) {
+        if (auto* indexAsObject = matchIndices->at<API::UInt64>(i))
             indices.uncheckedAppend(indexAsObject->value());
     }
-    toImpl(pageRef)->replaceStringMatchesFromInjectedBundle(WTFMove(indices), toWTFString(replacementText), selectionOnly);
+    toImpl(pageRef)->replaceStringMatchesFromInjectedBundle(indices, toWTFString(replacementText), selectionOnly);
 }
 
 WKImageRef WKBundlePageCreateSnapshotWithOptions(WKBundlePageRef pageRef, WKRect rect, WKSnapshotOptions options)
index d797c2e..9fefdbc 100644 (file)
@@ -99,7 +99,7 @@ void FindController::countStringMatches(const String& string, FindOptions option
     m_webPage->send(Messages::WebPageProxy::DidCountStringMatches(string, matchCount));
 }
 
-uint32_t FindController::replaceMatches(Vector<uint32_t>&& matchIndices, const String& replacementText, bool selectionOnly)
+uint32_t FindController::replaceMatches(const Vector<uint32_t>& matchIndices, const String& replacementText, bool selectionOnly)
 {
     if (matchIndices.isEmpty())
         return m_webPage->corePage()->replaceSelectionWithText(replacementText);
@@ -117,7 +117,7 @@ uint32_t FindController::replaceMatches(Vector<uint32_t>&& matchIndices, const S
         if (rangesToReplace.size() >= maximumNumberOfMatchesToReplace)
             break;
     }
-    return m_webPage->corePage()->replaceRangesWithText(WTFMove(rangesToReplace), replacementText, selectionOnly);
+    return m_webPage->corePage()->replaceRangesWithText(rangesToReplace, replacementText, selectionOnly);
 }
 
 static Frame* frameWithSelection(Page* page)
index 8aef205..e2f4a11 100644 (file)
@@ -61,7 +61,7 @@ public:
     void selectFindMatch(uint32_t matchIndex);
     void hideFindUI();
     void countStringMatches(const String&, FindOptions, unsigned maxMatchCount);
-    uint32_t replaceMatches(Vector<uint32_t>&& matchIndices, const String& replacementText, bool selectionOnly);
+    uint32_t replaceMatches(const Vector<uint32_t>& matchIndices, const String& replacementText, bool selectionOnly);
     
     void hideFindIndicator();
     void showFindIndicatorInSelection();
index 74b06dc..4aae14f 100644 (file)
@@ -3796,9 +3796,9 @@ void WebPage::findStringMatchesFromInjectedBundle(const String& target, FindOpti
     findController().findStringMatches(target, options, 0);
 }
 
-void WebPage::replaceStringMatchesFromInjectedBundle(Vector<uint32_t>&& matchIndices, const String& replacementText, bool selectionOnly)
+void WebPage::replaceStringMatchesFromInjectedBundle(const Vector<uint32_t>& matchIndices, const String& replacementText, bool selectionOnly)
 {
-    findController().replaceMatches(WTFMove(matchIndices), replacementText, selectionOnly);
+    findController().replaceMatches(matchIndices, replacementText, selectionOnly);
 }
 
 void WebPage::findString(const String& string, uint32_t options, uint32_t maxMatchCount)
@@ -3831,9 +3831,9 @@ void WebPage::countStringMatches(const String& string, uint32_t options, uint32_
     findController().countStringMatches(string, static_cast<FindOptions>(options), maxMatchCount);
 }
 
-void WebPage::replaceMatches(Vector<uint32_t>&& matchIndices, const String& replacementText, bool selectionOnly, CallbackID callbackID)
+void WebPage::replaceMatches(const Vector<uint32_t>& matchIndices, const String& replacementText, bool selectionOnly, CallbackID callbackID)
 {
-    auto numberOfReplacements = findController().replaceMatches(WTFMove(matchIndices), replacementText, selectionOnly);
+    auto numberOfReplacements = findController().replaceMatches(matchIndices, replacementText, selectionOnly);
     send(Messages::WebPageProxy::UnsignedCallback(numberOfReplacements, callbackID));
 }
 
@@ -4036,9 +4036,7 @@ void WebPage::didSelectItemFromActiveContextMenu(const WebContextMenuItemData& i
 
 void WebPage::replaceSelectionWithText(Frame* frame, const String& text)
 {
-    bool selectReplacement = true;
-    bool smartReplace = false;
-    return frame->editor().replaceSelectionWithText(text, selectReplacement, smartReplace);
+    return frame->editor().replaceSelectionWithText(text, WebCore::Editor::SelectReplacement::Yes, WebCore::Editor::SmartReplace::No);
 }
 
 #if !PLATFORM(IOS_FAMILY)
index 911a2f8..5753298 100644 (file)
@@ -409,7 +409,7 @@ public:
 
     bool findStringFromInjectedBundle(const String&, FindOptions);
     void findStringMatchesFromInjectedBundle(const String&, FindOptions);
-    void replaceStringMatchesFromInjectedBundle(Vector<uint32_t>&& matchIndices, const String& replacementText, bool selectionOnly);
+    void replaceStringMatchesFromInjectedBundle(const Vector<uint32_t>& matchIndices, const String& replacementText, bool selectionOnly);
 
     WebFrame* mainWebFrame() const { return m_mainFrame.get(); }
 
@@ -1306,7 +1306,7 @@ private:
     void selectFindMatch(uint32_t matchIndex);
     void hideFindUI();
     void countStringMatches(const String&, uint32_t findOptions, uint32_t maxMatchCount);
-    void replaceMatches(Vector<uint32_t>&& matchIndices, const String& replacementText, bool selectionOnly, CallbackID);
+    void replaceMatches(const Vector<uint32_t>& matchIndices, const String& replacementText, bool selectionOnly, CallbackID);
 
 #if USE(COORDINATED_GRAPHICS)
     void sendViewportAttributesChanged(const WebCore::ViewportArguments&);
index ee082c4..57807c6 100644 (file)
@@ -1214,7 +1214,7 @@ void WebPage::selectWithGesture(const IntPoint& point, uint32_t granularity, uin
         break;
     }
     if (range)
-        frame.selection().setSelectedRange(range.get(), position.affinity(), true, UserTriggered);
+        frame.selection().setSelectedRange(range.get(), position.affinity(), WebCore::FrameSelection::ShouldCloseTyping::Yes, UserTriggered);
 
     send(Messages::WebPageProxy::GestureCallback(point, gestureType, gestureState, static_cast<uint32_t>(flags), callbackID));
 }
@@ -1364,7 +1364,7 @@ void WebPage::updateSelectionWithTouches(const IntPoint& point, uint32_t touches
         break;
     }
     if (range)
-        frame.selection().setSelectedRange(range.get(), position.affinity(), true, UserTriggered);
+        frame.selection().setSelectedRange(range.get(), position.affinity(), WebCore::FrameSelection::ShouldCloseTyping::Yes, UserTriggered);
 
     send(Messages::WebPageProxy::TouchesCallback(point, touches, flags, callbackID));
 }
@@ -1380,7 +1380,7 @@ void WebPage::selectWithTwoTouches(const WebCore::IntPoint& from, const WebCore:
             range = Range::create(*frame.document(), fromPosition, toPosition);
         else
             range = Range::create(*frame.document(), toPosition, fromPosition);
-        frame.selection().setSelectedRange(range.get(), fromPosition.affinity(), true, UserTriggered);
+        frame.selection().setSelectedRange(range.get(), fromPosition.affinity(), WebCore::FrameSelection::ShouldCloseTyping::Yes, UserTriggered);
     }
 
     // We can use the same callback for the gestures with one point.
@@ -1395,7 +1395,7 @@ void WebPage::extendSelection(uint32_t granularity)
         return;
 
     VisiblePosition position = frame.selection().selection().start();
-    frame.selection().setSelectedRange(wordRangeFromPosition(position).get(), position.affinity(), true, UserTriggered);
+    frame.selection().setSelectedRange(wordRangeFromPosition(position).get(), position.affinity(), WebCore::FrameSelection::ShouldCloseTyping::Yes, UserTriggered);
 }
 
 void WebPage::selectWordBackward()
@@ -1407,7 +1407,7 @@ void WebPage::selectWordBackward()
     VisiblePosition position = frame.selection().selection().start();
     VisiblePosition startPosition = positionOfNextBoundaryOfGranularity(position, WordGranularity, DirectionBackward);
     if (startPosition.isNotNull() && startPosition != position)
-        frame.selection().setSelectedRange(Range::create(*frame.document(), startPosition, position).ptr(), position.affinity(), true, UserTriggered);
+        frame.selection().setSelectedRange(Range::create(*frame.document(), startPosition, position).ptr(), position.affinity(), WebCore::FrameSelection::ShouldCloseTyping::Yes, UserTriggered);
 }
 
 void WebPage::moveSelectionByOffset(int32_t offset, CallbackID callbackID)
@@ -1425,7 +1425,7 @@ void WebPage::moveSelectionByOffset(int32_t offset, CallbackID callbackID)
             break;
     }
     if (position.isNotNull() && startPosition != position)
-        frame.selection().setSelectedRange(Range::create(*frame.document(), position, position).ptr(), position.affinity(), true, UserTriggered);
+        frame.selection().setSelectedRange(Range::create(*frame.document(), position, position).ptr(), position.affinity(), WebCore::FrameSelection::ShouldCloseTyping::Yes, UserTriggered);
     send(Messages::WebPageProxy::VoidCallback(callbackID));
 }
     
@@ -1545,7 +1545,7 @@ void WebPage::selectPositionAtPoint(const WebCore::IntPoint& point, bool isInter
     VisiblePosition position = visiblePositionInFocusedNodeForPoint(frame, point, isInteractingWithAssistedNode);
     
     if (position.isNotNull())
-        frame.selection().setSelectedRange(Range::create(*frame.document(), position, position).ptr(), position.affinity(), true, UserTriggered);
+        frame.selection().setSelectedRange(Range::create(*frame.document(), position, position).ptr(), position.affinity(), WebCore::FrameSelection::ShouldCloseTyping::Yes, UserTriggered);
     send(Messages::WebPageProxy::VoidCallback(callbackID));
 }
 
@@ -1557,7 +1557,7 @@ void WebPage::selectPositionAtBoundaryWithDirection(const WebCore::IntPoint& poi
     if (position.isNotNull()) {
         position = positionOfNextBoundaryOfGranularity(position, static_cast<WebCore::TextGranularity>(granularity), static_cast<SelectionDirection>(direction));
         if (position.isNotNull())
-            frame.selection().setSelectedRange(Range::create(*frame.document(), position, position).ptr(), UPSTREAM, true, UserTriggered);
+            frame.selection().setSelectedRange(Range::create(*frame.document(), position, position).ptr(), UPSTREAM, WebCore::FrameSelection::ShouldCloseTyping::Yes, UserTriggered);
     }
     send(Messages::WebPageProxy::VoidCallback(callbackID));
 }
@@ -1571,7 +1571,7 @@ void WebPage::moveSelectionAtBoundaryWithDirection(uint32_t granularity, uint32_
         VisiblePosition position = (isForward) ? frame.selection().selection().visibleEnd() : frame.selection().selection().visibleStart();
         position = positionOfNextBoundaryOfGranularity(position, static_cast<WebCore::TextGranularity>(granularity), static_cast<SelectionDirection>(direction));
         if (position.isNotNull())
-            frame.selection().setSelectedRange(Range::create(*frame.document(), position, position).ptr(), isForward? UPSTREAM : DOWNSTREAM, true, UserTriggered);
+            frame.selection().setSelectedRange(Range::create(*frame.document(), position, position).ptr(), isForward? UPSTREAM : DOWNSTREAM, WebCore::FrameSelection::ShouldCloseTyping::Yes, UserTriggered);
     }
     send(Messages::WebPageProxy::VoidCallback(callbackID));
 }
@@ -1628,7 +1628,7 @@ void WebPage::selectTextWithGranularityAtPoint(const WebCore::IntPoint& point, u
     }
 
     if (range)
-        frame.selection().setSelectedRange(range.get(), UPSTREAM, true, UserTriggered);
+        frame.selection().setSelectedRange(range.get(), UPSTREAM, WebCore::FrameSelection::ShouldCloseTyping::Yes, UserTriggered);
     m_initialSelection = range;
     send(Messages::WebPageProxy::VoidCallback(callbackID));
 }
@@ -1663,7 +1663,7 @@ void WebPage::updateSelectionWithExtentPointAndBoundary(const WebCore::IntPoint&
         range = Range::create(*frame.document(), selectionStart, selectionEnd);
     
     if (range)
-        frame.selection().setSelectedRange(range.get(), UPSTREAM, true, UserTriggered);
+        frame.selection().setSelectedRange(range.get(), UPSTREAM, WebCore::FrameSelection::ShouldCloseTyping::Yes, UserTriggered);
     
     send(Messages::WebPageProxy::UnsignedCallback(selectionStart == m_initialSelection->startPosition(), callbackID));
 }
@@ -1706,7 +1706,7 @@ void WebPage::updateSelectionWithExtentPoint(const WebCore::IntPoint& point, boo
         range = Range::create(*frame.document(), selectionStart, selectionEnd);
 
     if (range)
-        frame.selection().setSelectedRange(range.get(), UPSTREAM, true, UserTriggered);
+        frame.selection().setSelectedRange(range.get(), UPSTREAM, WebCore::FrameSelection::ShouldCloseTyping::Yes, UserTriggered);
 
     send(Messages::WebPageProxy::UnsignedCallback(m_selectionAnchor == Start, callbackID));
 }
@@ -1769,7 +1769,7 @@ void WebPage::replaceSelectedText(const String& oldText, const String& newText)
         return;
     
     frame.editor().setIgnoreSelectionChanges(true);
-    frame.selection().setSelectedRange(wordRange.get(), UPSTREAM, true);
+    frame.selection().setSelectedRange(wordRange.get(), UPSTREAM, WebCore::FrameSelection::ShouldCloseTyping::Yes);
     frame.editor().insertText(newText, 0);
     frame.editor().setIgnoreSelectionChanges(false);
 }
@@ -1796,7 +1796,7 @@ void WebPage::replaceDictatedText(const String& oldText, const String& newText)
 
     // We don't want to notify the client that the selection has changed until we are done inserting the new text.
     frame.editor().setIgnoreSelectionChanges(true);
-    frame.selection().setSelectedRange(range.get(), UPSTREAM, true);
+    frame.selection().setSelectedRange(range.get(), UPSTREAM, WebCore::FrameSelection::ShouldCloseTyping::Yes);
     frame.editor().insertText(newText, 0);
     frame.editor().setIgnoreSelectionChanges(false);
 }
@@ -1930,7 +1930,7 @@ void WebPage::syncApplyAutocorrection(const String& correction, const String& or
     if (range && range->collapsed())
         affinity = VisiblePosition(range->startPosition(), UPSTREAM).affinity();
     
-    frame.selection().setSelectedRange(range.get(), affinity, true);
+    frame.selection().setSelectedRange(range.get(), affinity, WebCore::FrameSelection::ShouldCloseTyping::Yes);
     if (correction.length())
         frame.editor().insertText(correction, 0, originalText.isEmpty() ? TextEventInputKeyboard : TextEventInputAutocompletion);
     else
index 61f00d3..ff1c7a9 100644 (file)
@@ -1,3 +1,16 @@
+2018-11-22  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        Address post-review feedback after r238438
+        https://bugs.webkit.org/show_bug.cgi?id=191913
+
+        Reviewed by Ryosuke Niwa.
+
+        * WebView/WebFrame.mm:
+        (-[WebFrame setSelectedDOMRange:affinity:closeTyping:]):
+        (-[WebFrame _replaceSelectionWithFragment:selectReplacement:smartReplace:matchStyle:]):
+        * WebView/WebView.mm:
+        (-[WebView setSelectedDOMRange:affinity:]):
+
 2018-11-19  Joseph Pecoraro  <pecoraro@apple.com>
 
         Web Inspector: "Reload Web Inspector" button no longer partially works
index 541e740..c144029 100644 (file)
@@ -1482,7 +1482,7 @@ static WebFrameLoadType toWebFrameLoadType(FrameLoadType frameLoadType)
         }
     }
 
-    frame->selection().setSelectedRange(core(range), (EAffinity)affinity, closeTyping);
+    frame->selection().setSelectedRange(core(range), (EAffinity)affinity, closeTyping ? FrameSelection::ShouldCloseTyping::Yes : FrameSelection::ShouldCloseTyping::No);
     if (!closeTyping)
         frame->editor().ensureLastEditCommandHasCurrentSelectionIfOpenForMoreTyping();
 }
@@ -1952,7 +1952,7 @@ static WebFrameLoadType toWebFrameLoadType(FrameLoadType frameLoadType)
 {
     if (_private->coreFrame->selection().isNone() || !fragment)
         return;
-    _private->coreFrame->editor().replaceSelectionWithFragment(*core(fragment), selectReplacement, smartReplace, matchStyle);
+    _private->coreFrame->editor().replaceSelectionWithFragment(*core(fragment), selectReplacement ? Editor::SelectReplacement::Yes : Editor::SelectReplacement::No, smartReplace ? Editor::SmartReplace::Yes : Editor::SmartReplace::No, matchStyle ? Editor::MatchStyle::Yes : Editor::MatchStyle::No);
 }
 
 #if PLATFORM(IOS_FAMILY)
index e78a8bd..893fa08 100644 (file)
@@ -8351,7 +8351,7 @@ static NSAppleEventDescriptor* aeDescFromJSValue(ExecState* exec, JSC::JSValue j
         if (!coreFrame)
             return;
 
-        coreFrame->selection().setSelectedRange(core(range), core(selectionAffinity), true);
+        coreFrame->selection().setSelectedRange(core(range), core(selectionAffinity), WebCore::FrameSelection::ShouldCloseTyping::Yes);
     }
 }
 
index 4aaa537..9f84568 100644 (file)
@@ -656,7 +656,7 @@ HRESULT AccessibleText::replaceText(long startOffset, long endOffset, BSTR* text
 
     addSelection(startOffset, endOffset);
 
-    frame->editor().replaceSelectionWithText(*text, true, false);
+    frame->editor().replaceSelectionWithText(*text, Editor::SelectReplacement::Yes, Editor::SmartReplace::No);
     return S_OK;
 }
 
index b1d8b0b..0022f8b 100644 (file)
@@ -1,3 +1,13 @@
+2018-11-22  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        Address post-review feedback after r238438
+        https://bugs.webkit.org/show_bug.cgi?id=191913
+
+        Reviewed by Ryosuke Niwa.
+
+        * AccessibleTextImpl.cpp:
+        (AccessibleText::replaceText):
+
 2018-11-19  Joseph Pecoraro  <pecoraro@apple.com>
 
         Web Inspector: "Reload Web Inspector" button no longer partially works