[webkit-reviews] review denied: [Bug 17526] REGRESSION: iframes are added to Safari's History menu : [Attachment 19542] patch

[bugzilla-daemon at webkit.org]

mitz at webkit.org has denied Darin Adler <darin at apple.com>'s request for review:
Bug 17526: REGRESSION: iframes are added to Safari's History menu
http://bugs.webkit.org/show_bug.cgi?id=17526

Attachment 19542: patch
http://bugs.webkit.org/attachment.cgi?id=19542&action=edit

------- Additional Comments from mitz at webkit.org
[See comment #7 for a review of the WebCore part]

+	 (-[WebHistory _addVisitedLinksToPageGroup:WebCore::]): Added. For use
only inside WebKit.

prepare-ChangeLog messed up the method signature.

-#import "WebHistory.h"
-#import "WebHistoryPrivate.h"
+#import "WebHistoryInternal.h"
+#import "WebHistoryInternal.h"

You removed WebHistory.h and added WebHistoryInternal.h twice.

+    int itemLimit;
+    int ageInDaysLimit;

Should these be unsigned?

+- (BOOL)containsItemForURLString:(NSString *)URLString;
+- (BOOL)containsURL:(NSURL *)URL;
+- (WebHistoryItem *)itemForURL:(NSURL *)URL;
+- (WebHistoryItem *)itemForURLString:(NSString *)URLString;

It would be nice to change the order of the first two methods here.

+- (NSCalendarDate*)ageLimitDate;

Missing a space before the *.

+    if (count == 0)

Should be written as "if (count)".

+    NSMutableArray *discardedItems = [[NSMutableArray alloc] init];	

Doesn't this array leak in the "successful" branch?

r- because of the problems mentioned in comment #7 and the apparent leak.