Some functions on DictionaryLookup.h should just be generic functions elsewhere
authortimothy_horton@apple.com <timothy_horton@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 18 Aug 2015 02:02:36 +0000 (02:02 +0000)
committertimothy_horton@apple.com <timothy_horton@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 18 Aug 2015 02:02:36 +0000 (02:02 +0000)
https://bugs.webkit.org/show_bug.cgi?id=138567

Reviewed by Dean Jackson.

No new tests, just refactoring.

* dom/Range.cpp:
(WebCore::Range::contains):
* dom/Range.h:
Add a Range::contains(VisiblePosition).
It's simpler than the old thing from DictionaryLookup.mm, but does the same thing.
It's so simple that it doesn't necessarily need to exist, but it seems useful.

* editing/VisiblePosition.cpp:
(WebCore::makeRange):
nullptrs

* editing/mac/DictionaryLookup.h:
* editing/mac/DictionaryLookup.mm:
(WebCore::selectionContainsPosition):
(WebCore::rangeForDictionaryLookupAtHitTestResult):
(WebCore::isPositionInRange): Deleted.
(WebCore::shouldUseSelection): Deleted.
Move isPositionInRange to Range.
Rename shouldUseSelection to what it really means.
I didn't move selectionContainsPosition to VisibleSelection because it
only handles Range selections, not any of the ohers, and thus isn't
generic enough to put there.

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

Source/WebCore/ChangeLog
Source/WebCore/dom/Range.cpp
Source/WebCore/dom/Range.h
Source/WebCore/editing/VisiblePosition.cpp
Source/WebCore/editing/mac/DictionaryLookup.h
Source/WebCore/editing/mac/DictionaryLookup.mm

index 7d61515..d273881 100644 (file)
@@ -1,3 +1,35 @@
+2015-08-17  Tim Horton  <timothy_horton@apple.com>
+
+        Some functions on DictionaryLookup.h should just be generic functions elsewhere
+        https://bugs.webkit.org/show_bug.cgi?id=138567
+
+        Reviewed by Dean Jackson.
+
+        No new tests, just refactoring.
+
+        * dom/Range.cpp:
+        (WebCore::Range::contains):
+        * dom/Range.h:
+        Add a Range::contains(VisiblePosition).
+        It's simpler than the old thing from DictionaryLookup.mm, but does the same thing.
+        It's so simple that it doesn't necessarily need to exist, but it seems useful.
+
+        * editing/VisiblePosition.cpp:
+        (WebCore::makeRange):
+        nullptrs
+
+        * editing/mac/DictionaryLookup.h:
+        * editing/mac/DictionaryLookup.mm:
+        (WebCore::selectionContainsPosition):
+        (WebCore::rangeForDictionaryLookupAtHitTestResult):
+        (WebCore::isPositionInRange): Deleted.
+        (WebCore::shouldUseSelection): Deleted.
+        Move isPositionInRange to Range.
+        Rename shouldUseSelection to what it really means.
+        I didn't move selectionContainsPosition to VisibleSelection because it
+        only handles Range selections, not any of the ohers, and thus isn't
+        generic enough to put there.
+
 2015-08-17  Alex Christensen  <achristensen@webkit.org>
 
         [Win CMake] Allow WebKitLibraries directory to be set from the command line
index a8d5b8d..9ba4814 100644 (file)
@@ -1960,6 +1960,14 @@ bool Range::contains(const Range& other) const
     return endToEnd >= 0;
 }
 
+bool Range::contains(const VisiblePosition& position) const
+{
+    RefPtr<Range> positionRange = makeRange(position, position);
+    if (!positionRange)
+        return false;
+    return contains(*positionRange);
+}
+
 bool areRangesEqual(const Range* a, const Range* b)
 {
     if (a == b)
index c6e25ae..08bccff 100644 (file)
@@ -155,6 +155,7 @@ public:
 #endif
 
     WEBCORE_EXPORT bool contains(const Range&) const;
+    bool contains(const VisiblePosition&) const;
 
 private:
     explicit Range(Document&);
index 78241b9..63c0fbe 100644 (file)
@@ -670,12 +670,12 @@ void VisiblePosition::showTreeForThis() const
 PassRefPtr<Range> makeRange(const VisiblePosition &start, const VisiblePosition &end)
 {
     if (start.isNull() || end.isNull())
-        return 0;
+        return nullptr;
     
     Position s = start.deepEquivalent().parentAnchoredEquivalent();
     Position e = end.deepEquivalent().parentAnchoredEquivalent();
     if (s.isNull() || e.isNull())
-        return 0;
+        return nullptr;
 
     return Range::create(s.containerNode()->document(), s.containerNode(), s.offsetInContainerNode(), e.containerNode(), e.offsetInContainerNode());
 }
index 782ba6f..f6228f5 100644 (file)
@@ -37,14 +37,8 @@ namespace WebCore {
 
 class HitTestResult;
 class Range;
-class VisiblePosition;
 class VisibleSelection;
 
-// FIXME: Some of these functions should probably be in a more generic class.
-// https://bugs.webkit.org/show_bug.cgi?id=138567
-bool isPositionInRange(const VisiblePosition&, Range*);
-bool shouldUseSelection(const VisiblePosition&, const VisibleSelection&);
-
 WEBCORE_EXPORT PassRefPtr<Range> rangeForDictionaryLookupForSelection(const VisibleSelection&, NSDictionary **options);
 WEBCORE_EXPORT PassRefPtr<Range> rangeForDictionaryLookupAtHitTestResult(const HitTestResult&, NSDictionary **options);
 WEBCORE_EXPORT NSString *dictionaryLookupForPDFSelection(PDFSelection *, NSDictionary **options);
index a1e4f64..ed0479e 100644 (file)
 
 namespace WebCore {
 
-bool isPositionInRange(const VisiblePosition& position, Range* range)
-{
-    RefPtr<Range> positionRange = makeRange(position, position);
-
-    ExceptionCode ec = 0;
-    range->compareBoundaryPoints(Range::START_TO_START, positionRange.get(), ec);
-    if (ec)
-        return false;
-
-    if (!range->isPointInRange(positionRange->startContainer(), positionRange->startOffset(), ec))
-        return false;
-    if (ec)
-        return false;
-
-    return true;
-}
-
-bool shouldUseSelection(const VisiblePosition& position, const VisibleSelection& selection)
+static bool selectionContainsPosition(const VisiblePosition& position, const VisibleSelection& selection)
 {
     if (!selection.isRange())
         return false;
@@ -75,7 +58,7 @@ bool shouldUseSelection(const VisiblePosition& position, const VisibleSelection&
     if (!selectedRange)
         return false;
 
-    return isPositionInRange(position, selectedRange.get());
+    return selectedRange->contains(position);
 }
 
 PassRefPtr<Range> rangeForDictionaryLookupForSelection(const VisibleSelection& selection, NSDictionary **options)
@@ -127,8 +110,9 @@ PassRefPtr<Range> rangeForDictionaryLookupAtHitTestResult(const HitTestResult& h
     if (position.isNull())
         position = firstPositionInOrBeforeNode(node);
 
+    // If we hit the selection, use that instead of letting Lookup decide the range.
     VisibleSelection selection = frame->page()->focusController().focusedOrMainFrame().selection().selection();
-    if (shouldUseSelection(position, selection))
+    if (selectionContainsPosition(position, selection))
         return rangeForDictionaryLookupForSelection(selection, options);
 
     VisibleSelection selectionAccountingForLineRules = VisibleSelection(position);