Add type checking to isEqual methods

https://bugs.webkit.org/show_bug.cgi?id=126862 Reviewed by Anders Carlsson. Source/WebCore: * page/ios/WebEventRegion.mm: (-[WebEventRegion isEqual:]): Add type checking on the argument. Add a FIXME about the lack of a hash method override. Formatted to match the usual WebKit coding style. Source/WebKit/ios: * WebCoreSupport/WebVisiblePosition.mm: (-[WebVisiblePosition isEqual:]): Add type checking on the argument. Add a FIXME about the lack of a hash method override. Simplified by removing the unneeded local variables. Source/WebKit/mac: * WebCoreSupport/WebSecurityOrigin.mm: (-[WebSecurityOrigin isEqual:]): Added a FIXME about the lack of a hash method override. Tweaked formatting. * WebView/WebDashboardRegion.mm: (-[WebDashboardRegion isEqual:]): Added type checking on the argument. Added a FIXME about the lack of a hash method override. Source/WebKit2: * UIProcess/API/ios/WKInteractionView.mm: (-[WKTextRange isEqual:]): Added type checking for the argument. The old code asserted instead, and it's not clear what guarantees that assertion is true. Added a FIXME about the lack of a hash method. Added another FIXME about the fact that this method ignores much of the object state. Removed an unneeded extra fetch of the isRange property. Deleted some dead code. (-[WKTextPosition isEqual:]): Ditto. Canonical link: https://commits.webkit.org/144849@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@161850 268f45cc-cd09-0410-ab3c-d52691b4dbfc
WebKit · Jan 13, 2014 · 46619ed · 46619ed
1 parent 46e4d0c
commit 46619ed
Show file tree

Hide file tree

Showing 9 changed files with 93 additions and 17 deletions.
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,15 @@
+2014-01-12  Darin Adler  <darin@apple.com>
+
+        Add type checking to isEqual methods
+        https://bugs.webkit.org/show_bug.cgi?id=126862
+
+        Reviewed by Anders Carlsson.
+
+        * page/ios/WebEventRegion.mm:
+        (-[WebEventRegion isEqual:]): Add type checking on the argument.
+        Add a FIXME about the lack of a hash method override. Formatted to match
+        the usual WebKit coding style.
+
 2014-01-12  Anders Carlsson  <andersca@apple.com>
 
         Try to fix the 32-bit build.

diff --git a/Source/WebCore/page/ios/WebEventRegion.mm b/Source/WebCore/page/ios/WebEventRegion.mm
@@ -66,12 +66,16 @@ - (BOOL)hitTest:(CGPoint)point
     return [self quad].containsPoint(point);
 }
 
+// FIXME: Overriding isEqual: without overriding hash will cause trouble if this ever goes into an NSSet or is the key in an NSDictionary,
+// since two equal objects could have different hashes.
 - (BOOL)isEqual:(id)other
 {
-    return  CGPointEqualToPoint(p1, ((WebEventRegion *)other)->p1) &&
-            CGPointEqualToPoint(p2, ((WebEventRegion *)other)->p2) &&
-            CGPointEqualToPoint(p3, ((WebEventRegion *)other)->p3) &&
-            CGPointEqualToPoint(p4, ((WebEventRegion *)other)->p4);
+    if (![other isKindOfClass:[WebEventRegion class]])
+        return NO;
+    return CGPointEqualToPoint(p1, ((WebEventRegion *)other)->p1)
+        && CGPointEqualToPoint(p2, ((WebEventRegion *)other)->p2)
+        && CGPointEqualToPoint(p3, ((WebEventRegion *)other)->p3)
+        && CGPointEqualToPoint(p4, ((WebEventRegion *)other)->p4);
 }
 
 - (FloatQuad)quad

diff --git a/Source/WebKit/ios/ChangeLog b/Source/WebKit/ios/ChangeLog
@@ -1,3 +1,15 @@
+2014-01-12  Darin Adler  <darin@apple.com>
+
+        Add type checking to isEqual methods
+        https://bugs.webkit.org/show_bug.cgi?id=126862
+
+        Reviewed by Anders Carlsson.
+
+        * WebCoreSupport/WebVisiblePosition.mm:
+        (-[WebVisiblePosition isEqual:]): Add type checking on the argument.
+        Add a FIXME about the lack of a hash method override. Simplified by
+        removing the unneeded local variables.
+
 2014-01-02  Andy Estes  <aestes@apple.com>
 
         [iOS] Upstream remainder of minimal-ui viewport changes

diff --git a/Source/WebKit/ios/WebCoreSupport/WebVisiblePosition.mm b/Source/WebKit/ios/WebCoreSupport/WebVisiblePosition.mm
@@ -87,11 +87,13 @@ - (void)dealloc
     [super dealloc];
 }
 
-- (BOOL)isEqual:(WebVisiblePosition *)other
+// FIXME: Overriding isEqual: without overriding hash will cause trouble if this ever goes into an NSSet or is the key in an NSDictionary,
+// since two equal objects could have different hashes.
+- (BOOL)isEqual:(id)other
 {
-    VisiblePosition myVP = [self _visiblePosition];
-    VisiblePosition otherVP = [other _visiblePosition];
-    return (myVP == otherVP);
+    if (![other isKindOfClass:[WebVisiblePosition class]])
+        return NO;
+    return [self _visiblePosition] == [(WebVisiblePosition *)other _visiblePosition];
 }
 
 - (NSComparisonResult)compare:(WebVisiblePosition *)other

diff --git a/Source/WebKit/mac/ChangeLog b/Source/WebKit/mac/ChangeLog
@@ -1,3 +1,18 @@
+2014-01-12  Darin Adler  <darin@apple.com>
+
+        Add type checking to isEqual methods
+        https://bugs.webkit.org/show_bug.cgi?id=126862
+
+        Reviewed by Anders Carlsson.
+
+        * WebCoreSupport/WebSecurityOrigin.mm:
+        (-[WebSecurityOrigin isEqual:]): Added a FIXME about the lack of a hash method
+        override. Tweaked formatting.
+
+        * WebView/WebDashboardRegion.mm:
+        (-[WebDashboardRegion isEqual:]): Added type checking on the argument. Added a
+        FIXME about the lack of a hash method override.
+
 2014-01-11  Alexey Proskuryakov  <ap@apple.com>
 
         [Mac] [Windows] Stop scheduling network requests in WebCore

diff --git a/Source/WebKit/mac/WebCoreSupport/WebSecurityOrigin.mm b/Source/WebKit/mac/WebCoreSupport/WebSecurityOrigin.mm
@@ -89,11 +89,12 @@ - (unsigned short)port
     return reinterpret_cast<SecurityOrigin*>(_private)->port();
 }
 
+// FIXME: Overriding isEqual: without overriding hash will cause trouble if this ever goes into an NSSet or is the key in an NSDictionary,
+// since two equal objects could have different hashes.
 - (BOOL)isEqual:(id)anObject
 {
-    if (![anObject isMemberOfClass:[WebSecurityOrigin class]]) {
+    if (![anObject isMemberOfClass:[WebSecurityOrigin class]])
         return NO;
-    }
 
     return [self _core]->equal([anObject _core]);
 }

diff --git a/Source/WebKit/mac/WebView/WebDashboardRegion.mm b/Source/WebKit/mac/WebView/WebDashboardRegion.mm
@@ -80,9 +80,13 @@ - (NSString *)description
     return [NSString stringWithFormat:@"rect:%@ clip:%@ type:%s", NSStringFromRect(rect), NSStringFromRect(clip), typeName(type)];
 }
 
-// FIXME: Overriding isEqual: without overriding hash will cause trouble if this ever goes into a NSSet or is the key in an NSDictionary.
+// FIXME: Overriding isEqual: without overriding hash will cause trouble if this ever goes into an NSSet or is the key in an NSDictionary,
+// since two equal objects could have different hashes.
 - (BOOL)isEqual:(id)other
 {
+    if (![other isKindOfClass:[WebDashboardRegion class]])
+        return NO;
+
     return NSEqualRects(rect, [other dashboardRegionRect]) && NSEqualRects(clip, [other dashboardRegionClip]) && type == [other dashboardRegionType];
 }
 

diff --git a/Source/WebKit2/ChangeLog b/Source/WebKit2/ChangeLog
@@ -1,3 +1,18 @@
+2014-01-12  Darin Adler  <darin@apple.com>
+
+        Add type checking to isEqual methods
+        https://bugs.webkit.org/show_bug.cgi?id=126862
+
+        Reviewed by Anders Carlsson.
+
+        * UIProcess/API/ios/WKInteractionView.mm:
+        (-[WKTextRange isEqual:]): Added type checking for the argument. The old
+        code asserted instead, and it's not clear what guarantees that assertion is
+        true. Added a FIXME about the lack of a hash method. Added another FIXME
+        about the fact that this method ignores much of the object state. Removed
+        an unneeded extra fetch of the isRange property. Deleted some dead code.
+        (-[WKTextPosition isEqual:]): Ditto.
+
 2014-01-12  Anders Carlsson  <andersca@apple.com>
 
         Remove the last remaining uses of AtomicallyInitializedStatic

diff --git a/Source/WebKit2/UIProcess/API/ios/WKInteractionView.mm b/Source/WebKit2/UIProcess/API/ios/WKInteractionView.mm
@@ -1515,24 +1515,31 @@ - (BOOL)isEmpty
     return !self.isRange;
 }
 
+// FIXME: Overriding isEqual: without overriding hash will cause trouble if this ever goes into an NSSet or is the key in an NSDictionary,
+// since two equal items could have different hashes.
 - (BOOL)isEqual:(id)other
 {
-    assert([other isKindOfClass:[WKTextRange class]]);
+    if (![other isKindOfClass:[WKTextRange class]])
+        return NO;
+
     WKTextRange *otherRange = (WKTextRange *)other;
-    
+
     if (self == other)
         return YES;
-
+
+    // FIXME: Probably incorrect for equality to ignore so much of the object state.
+    // It ignores isNone, isEditable, selectedTextLength, and selectionRects.
+
     if (self.isRange) {
         if (!otherRange.isRange)
             return NO;
         return CGRectEqualToRect(self.startRect, otherRange.startRect) && CGRectEqualToRect(self.endRect, otherRange.endRect);
-    } else if (!self.isRange) {
+    } else {
         if (otherRange.isRange)
             return NO;
+        // FIXME: Do we need to check isNone here?
         return CGRectEqualToRect(self.startRect, otherRange.startRect);
     }
-    return otherRange.isNone;
 }
 
 @end
@@ -1548,9 +1555,13 @@ + (WKTextPosition *)textPositionWithRect:(CGRect)positionRect
     return [pos autorelease];
 }
 
+// FIXME: Overriding isEqual: without overriding hash will cause trouble if this ever goes into a NSSet or is the key in an NSDictionary,
+// since two equal items could have different hashes.
 - (BOOL)isEqual:(id)other
 {
-    assert([other isKindOfClass:[self class]]);
+    if (![object isKindOfClass:[WKTextPosition class]])
+        return NO;
+
     return CGRectEqualToRect(self.positionRect, ((WKTextPosition *)other).positionRect);
 }