[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

--- Comment #14 from Zach Li <zacharyli323 at gmail.com> ---
(In reply to comment #13)
> Comment on attachment 263526 [details]
> 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.

Then I will use -removeSearchFieldRecentSearches as the function name to be distinct from Recent Web Searches in Safari.

> 
> 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.

Your suggested name is so much clearer. I will use that.

> 
> > 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)
> 

I think I will go with:

      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)

To be consistent, I will go with:

      static NSDate *typeCheckedDateInRecentSearch(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.

How about

static RetainPtr<NSDictionary> updatedRecentSearchesDictionary(NSDate *dateModified)?

> 
> > 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?

Safari will be passing [NSDate distantPast] to clear all recent searches. I also expect if other applications want to clear all recent searches, they would pass this date.

> 
> > 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) {
> 

Will change.

> > 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?

If we encounter an unexpected data type, I want to take the chance to purify the plist instead of leaving the corrupted piece of data there. We can imagine an extreme case where all keys are not NSString and we would skip everything and the corrupted entries would still be in the plist, which I think is not something clients expect when they initiate clearing recent searches.

> 
> > 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?

Ditto. But keys -> recentSearches.

> 
> > 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?

Ditto. But keys -> dates.

> 
> > 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?

Since in the case where we return nil, we will construct an empty dictionary and save it to disk, I do not want to construct an empty dictionary again when we already have an empty dictionary. Plus, this means less code.

> 
> > 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()

Will change.

> 
> > 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?

I think Anders has a pretty strong opinion on using atomically:YES and his arguments are:
1. The extra cost is a call to rename() and most likely the data is going to be in the buffer cache anyway.
2. It's more important that we don't corrupt the saved searches than be performant in this case
(He also mentioned if it was about writing a cache file he would be less worried about writing atomically, but this is user data)

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

Ditto.

-- 
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/20151021/80524697/attachment.html>