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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Oct 22 11:30:58 PDT 2015


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

--- Comment #16 from Jessie Berlin <jberlin at webkit.org> ---
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.
> 
> Then I will use -removeSearchFieldRecentSearches as the function name to be distinct from Recent Web Searches in Safari.

Let's go with Darin's suggestion here: removeRecentlyModifiedRecentSearches.

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

Zach and I came up with something slightly better:

static RetainPtr<NSDictionary> typeCheckedRecentSearchesAddedBeforeDate(NSDate *date)

OR

static RetainPtr<NSDictionary> typeCheckedRecentSearchesRemovingRecentSearchesAddedAfterDate(NSDate *date)

>>> Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:107
>>> +            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.

I think what Zach is trying to say here is that the user indicated they wanted data cleared and we don't want to risk leaving behind incriminating data that was written in either a slightly different format  or corrupted in some other way. This is erring on the side of privacy, for which I think it is acceptable to incur some data loss.

>>> Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:129
>>> +    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.

For consistency, maybe we should just return nil here if the dictionary is empty.

-- 
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/20151022/430bbba9/attachment.html>


More information about the webkit-unassigned mailing list