[Webkit-unassigned] [Bug 150019] We should be able to clear search field recent searches based on time given

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=150019

Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #263526|review?                     |review-
              Flags|                            |

--- Comment #13 from Darin Adler <darin at apple.com> ---
Comment on attachment 263526
  --> https://bugs.webkit.org/attachment.cgi?id=263526
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=263526&action=review

> Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.h:35
> +WEBCORE_EXPORT void removeSearchFieldRecentSearches(std::chrono::system_clock::time_point modifiedSince);

This function shouldn’t include the words “search field” in its name since the other two functions above don’t; we’d want that in all three names, or in none of the names.

I don’t think the time point should be named “modified since”. I think this makes better sense:

    removeRecentlyModifiedRecentSearches(std::chrono::system_clock::time_point);

The argument arguably doesn’t even need a name. Or it could be named oldestTimeToRemove.

> Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:69
> +static NSMutableArray *returnUncorruptedRecentSearchesArray(NSDictionary *itemsDictionary, NSString *name)

1) I think “type checked” would be a better term than “uncorrupted” or we could just leave it out of the function name entirely; it’s good that the function checks the types but I don’t think the name needs to state that explicitly.
2) I don’t think a function should have the verb “return” in it. We don’t use verbs in getter functions that return a result in WebKit code.
3) This function assumes the dictionary is part of a graph of objects that are all mutable, which is why it returns an NSMutableArray, so it should take an NSMutableDictionary *.

    static NSMutableArray *recentSearchesArray(NSMutableDictionary *itemsDictionary, NSString *name)

    static NSMutableArray *typeCheckedRecentSearchesArray(NSMutableDictionary *itemsDictionary, NSString *name)

> Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:82
> +static NSDate *returnUncorruptedDateInRecentSearch(NSDictionary *recentSearch)

Same issues as the (1) and (2) mentioned above.

    static NSDate *dateInRecentSearch(NSDictionary *recentSearch)

> Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:94
> +static RetainPtr<NSDictionary> returnUncorruptedRecentSearchesPlist(NSDate *dateModified)

Same issues as the (1) and (2) mentioned above, and also I don’t think it’s right to call what this returns a “recent searches plist” and also call the result of “readSearchFieldRecentSearchesPlist” a recent searches plist.

It’s also strange that this function name does not even mention that it removes the most recently added searches. That’s sort of leaving out the main point of the function.

> Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:97
> +    if ([dateModified isEqualToDate:[NSDate distantPast]])
> +        return nil;

Why do we need this? Is anyone going to call this function with that date? Do we really need to optimize that case?

> Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:105
> +    for (NSString *key in itemsDictionary.allKeys) {

Saying ".allKeys" here just hurts performance. Iteration of a dictionary already iterates its keys efficiently without allocating an array the way allKeys does:

    for (NSString *key in itemsDictionary) {

> Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:107
> +        if (![key isKindOfClass:[NSString class]])
> +            return nil;

Why not just skip an item by doing a continue? Do we really want to drop all the data if there’s even one thing wrong?

> Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:111
> +        if (!recentSearches)
> +            return nil;

Why not just skip an item by doing a continue? Do we really want to drop all the data if there’s even one thing wrong?

> Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:117
> +            if (!date)
> +                return nil;

Why not just skip an item by doing a continue? Do we really want to drop all the data if there’s even one thing wrong?

> Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:129
> +    [itemsDictionary removeObjectsForKeys:keysToRemove.get()];
> +    return recentSearchesPlist;

No special case here to return nil instead of an empty dictionary. But above you did that for distantPast. Is this an important optimization or not?

> Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:132
> +static void cleanRecentSearchesPlist()

I don’t think “clean” is a good name for writing an empty list to disk.

    static void writeEmptyRecentSearchesPlist()

> Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:163
> -    [recentSearchesPlist writeToFile:searchFieldRecentSearchesPlistPath() atomically:NO];
> +    [recentSearchesPlist writeToFile:searchFieldRecentSearchesPlistPath() atomically:YES];

I thought you all agreed to use atomically:NO? Who decided on YES? What is the rationale?

> Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:205
> +        [recentSearchesPlist writeToFile:searchFieldRecentSearchesPlistPath() atomically:YES];

Same question about atomically:YES.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20151020/86efd459/attachment-0001.html>