Bug 150019 – We should be able to clear search field recent searches based on time given

• Component: WebKit API
• Status: RESOLVED FIXED

[Zach Li]

To fix rdar://problem/16876762, add WebKit API to support removing search field recent searches based on time given.

[Zach Li]

Created attachment 262876 [details]
Patch

This patch is dependent on the change for https://bugs.webkit.org/show_bug.cgi?id=148388, so unless the patch for https://bugs.webkit.org/show_bug.cgi?id=148388 is checked in, this patch will not build.

Comment on attachment 262876 [details]
Patch

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

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

This can probably just be a static function in SearchPopupMenu, and then it can have a shorter name.

> Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:148
> +        NSMutableDictionary *emptyRecentSearchesDictionary = [NSMutableDictionary dictionaryWithObjectsAndKeys:[NSMutableDictionary dictionary], itemsKey, nil];
> +        [emptyRecentSearchesDictionary writeToFile:searchFieldRecentSearchesPlistPath() atomically:NO];

Can we just delete this file instead of writing an empty file? Does the presence of a file signify something? Does it need to include this key-value pair? Why is everything here mutable?

> Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:149
> +    }

Did you mean to return early after this?

> Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:153
> +    if (![[itemsDictionary classForCoder] isSubclassOfClass:[NSMutableDictionary class]])

What is the significance of checking this way vs. the -isKindOfClass: checks below?

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

I mean this…

> Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:162
> +        if (![[nameDictionary classForCoder] isSubclassOfClass:[NSMutableDictionary class]])

…versus this.

> Source/WebKit2/Shared/WebsiteData/WebsiteDataTypes.h:48
> +    WebsiteDataTypeSearchFieldRecentSearches = 1 << 10,
>  #if ENABLE(NETSCAPE_PLUGIN_API)
> -    WebsiteDataTypePlugInData = 1 << 10,
> +    WebsiteDataTypePlugInData = 1 << 11,
>  #endif
>  #if ENABLE(MEDIA_STREAM)
> -    WebsiteDataTypeMediaDeviceIdentifier = 1 << 11,
> +    WebsiteDataTypeMediaDeviceIdentifier = 1 << 12,
>  #endif

Why insert this in the middle?

> Source/WebKit2/UIProcess/API/Cocoa/WKWebsiteDataRecordPrivate.h:32
> +WK_EXTERN NSString * const _WKWebsiteDataTypeSearchFieldRecentSearches WK_AVAILABLE(10_11, 9_0);

This is not present in 10.11 and 9.0. You should use WK_MAC_TBA and WK_IOS_TBA instead.

> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:40
> +#if PLATFORM(COCOA)
> +#include <WebCore/SearchPopupMenuCocoa.h>
> +#endif

Conditional imports go in a separate paragraph after the main block of imports.

[Zach Li]

(In reply to comment #2)
> Comment on attachment 262876 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=262876&action=review
> 
> > Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.h:35
> > +WEBCORE_EXPORT void removeSearchFieldRecentSearchesModifiedSince(std::chrono::system_clock::time_point modifiedSince);
> 
> This can probably just be a static function in SearchPopupMenu, and then it
> can have a shorter name.

SearchPopupMenu is only a header file. And this function will only work on cocoa platform and I want to be consistent with the functions I already declared which are saveRecentSearches and loadRecentSearches.

> 
> > Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:148
> > +        NSMutableDictionary *emptyRecentSearchesDictionary = [NSMutableDictionary dictionaryWithObjectsAndKeys:[NSMutableDictionary dictionary], itemsKey, nil];
> > +        [emptyRecentSearchesDictionary writeToFile:searchFieldRecentSearchesPlistPath() atomically:NO];
> 
> Can we just delete this file instead of writing an empty file? Does the
> presence of a file signify something? Does it need to include this key-value
> pair? Why is everything here mutable?

The presence of the file does not signify anything if it is empty. But this is following the behavior in saveRecentSearches that we will overwrite the plist to be an empty file when we detect the plist is corrupted instead of deleting the file.

The key-value pair is there to make saveRecentSearches easier when we construct the plist.

Not sure if it adds any value by making them mutable because the mutability will not be reserved when saving as a plist? But we do expect them to be mutable when reading the plist.
 
> 
> > Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:149
> > +    }
> 
> Did you mean to return early after this?

Yup!

> 
> > Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:153
> > +    if (![[itemsDictionary classForCoder] isSubclassOfClass:[NSMutableDictionary class]])
> 
> What is the significance of checking this way vs. the -isKindOfClass: checks
> below?
> 

Remy explained to me that [itemsDictionary isKindOfClass:[NSMutableDictionary class]] can return true even if itemsDictionary is not mutable because of the presence of __NSCFDictionary inside NSMutableDictionary. We have this check in Safari codebase.

> > Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:158
> > +        if (![key isKindOfClass:[NSString class]])
> 
> I mean this…
> 
> > Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:162
> > +        if (![[nameDictionary classForCoder] isSubclassOfClass:[NSMutableDictionary class]])
> 
> …versus this.
> 
> > Source/WebKit2/Shared/WebsiteData/WebsiteDataTypes.h:48
> > +    WebsiteDataTypeSearchFieldRecentSearches = 1 << 10,
> >  #if ENABLE(NETSCAPE_PLUGIN_API)
> > -    WebsiteDataTypePlugInData = 1 << 10,
> > +    WebsiteDataTypePlugInData = 1 << 11,
> >  #endif
> >  #if ENABLE(MEDIA_STREAM)
> > -    WebsiteDataTypeMediaDeviceIdentifier = 1 << 11,
> > +    WebsiteDataTypeMediaDeviceIdentifier = 1 << 12,
> >  #endif
> 
> Why insert this in the middle?

No particular reason. I guess I want to put it before those conditionals.

> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKWebsiteDataRecordPrivate.h:32
> > +WK_EXTERN NSString * const _WKWebsiteDataTypeSearchFieldRecentSearches WK_AVAILABLE(10_11, 9_0);
> 
> This is not present in 10.11 and 9.0. You should use WK_MAC_TBA and
> WK_IOS_TBA instead.

Will fix.

> 
> > Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:40
> > +#if PLATFORM(COCOA)
> > +#include <WebCore/SearchPopupMenuCocoa.h>
> > +#endif
> 
> Conditional imports go in a separate paragraph after the main block of
> imports.

Will fix.

[Zach Li]

Created attachment 263034 [details]
Revised Patch

Added -removeRecentSearches and did refactoring.

[Zach Li]

Created attachment 263036 [details]
Revised Patch

Added -removeRecentSearches and did refactoring.

This patch is dependent on the change for https://bugs.webkit.org/show_bug.cgi?id=148388, so unless the patch for https://bugs.webkit.org/show_bug.cgi?id=148388 is checked in, this patch will not build.

[Zach Li]

Created attachment 263173 [details]
Patch v2

Check if it builds on all platforms.

[Zach Li]

Created attachment 263184 [details]
Patch v2

The patch did not build on GTK and Efl. The speculative fix is to include the newly added SearchPopupMenu.cpp file to PlatformGTK.cmake and PlatformEfl.cmake

[Anders Carlsson]

Comment on attachment 263184 [details]
Patch v2

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

> Source/WebCore/platform/SearchPopupMenu.h:45
> +WEBCORE_EXPORT static void removeRecentSearches(std::chrono::system_clock::time_point modifiedSince);

Indentation.

> Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:135
> +    RetainPtr<NSDictionary> emptyItemsDictionary = adoptNS([[NSDictionary alloc] init]);
> +    RetainPtr<NSDictionary> emptyRecentSearchesDictionary = adoptNS([[NSDictionary alloc] initWithObjectsAndKeys:emptyItemsDictionary.get(), itemsKey, nil]);

Can use auto here.

> Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:203
> +    RetainPtr<NSDictionary> recentSearchesPlist = returnUncorruptedRecentSearchesPlist(dateModified);

Can use auto here.

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

Should be atomically:YES here.

> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:1085
> +    WebCore::SearchPopupMenu::removeRecentSearches(modifiedSince);

I don't think you should call into WebCore here. Why can't you just put the contents of removeRecentSearches here?

[Zach Li]

(In reply to comment #8)
> Comment on attachment 263184 [details]
> Patch v2
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=263184&action=review
> 
> > Source/WebCore/platform/SearchPopupMenu.h:45
> > +WEBCORE_EXPORT static void removeRecentSearches(std::chrono::system_clock::time_point modifiedSince);
> 
> Indentation.

Will fix.

> 
> > Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:135
> > +    RetainPtr<NSDictionary> emptyItemsDictionary = adoptNS([[NSDictionary alloc] init]);
> > +    RetainPtr<NSDictionary> emptyRecentSearchesDictionary = adoptNS([[NSDictionary alloc] initWithObjectsAndKeys:emptyItemsDictionary.get(), itemsKey, nil]);
> 
> Can use auto here.

What is the benefit of using auto? Is it more modern?

> 
> > Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:203
> > +    RetainPtr<NSDictionary> recentSearchesPlist = returnUncorruptedRecentSearchesPlist(dateModified);
> 
> Can use auto here.

Ditto.

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

Darin suggested I should use atomically:NO because of the performance cost.

> 
> > Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:1085
> > +    WebCore::SearchPopupMenu::removeRecentSearches(modifiedSince);
> 
> I don't think you should call into WebCore here. Why can't you just put the
> contents of removeRecentSearches here?

Originally I had the contents of removeRecentSearches in WebsiteDataStore, but Darin thinks we should not do

#if PLATFORM(COCOA)
#endif

in WebsiteDataStore and it would be better to move this logic to the platform independent SearchPopupMenu file.

[Anders Carlsson]

(In reply to comment #9)
> (In reply to comment #8)
> > Comment on attachment 263184 [details]
> > Patch v2
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=263184&action=review
> > 
> > > Source/WebCore/platform/SearchPopupMenu.h:45
> > > +WEBCORE_EXPORT static void removeRecentSearches(std::chrono::system_clock::time_point modifiedSince);
> > 
> > Indentation.
> 
> Will fix.
> 
> > 
> > > Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:135
> > > +    RetainPtr<NSDictionary> emptyItemsDictionary = adoptNS([[NSDictionary alloc] init]);
> > > +    RetainPtr<NSDictionary> emptyRecentSearchesDictionary = adoptNS([[NSDictionary alloc] initWithObjectsAndKeys:emptyItemsDictionary.get(), itemsKey, nil]);
> > 
> > Can use auto here.
> 
> What is the benefit of using auto? Is it more modern?

It avoids having "NSDictionary" twice on one line.

> > 
> > > Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:205
> > > +        [recentSearchesPlist writeToFile:searchFieldRecentSearchesPlistPath() atomically:NO];
> > 
> > Should be atomically:YES here.
> 
> Darin suggested I should use atomically:NO because of the performance cost.

I think the performance cost is negligible and not corrupting data is better.


> > > Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:1085
> > > +    WebCore::SearchPopupMenu::removeRecentSearches(modifiedSince);
> > 
> > I don't think you should call into WebCore here. Why can't you just put the
> > contents of removeRecentSearches here?
> 
> Originally I had the contents of removeRecentSearches in WebsiteDataStore,
> but Darin thinks we should not do
> 
> #if PLATFORM(COCOA)
> #endif
> 
> in WebsiteDataStore and it would be better to move this logic to the
> platform independent SearchPopupMenu file.

Just call platformRemoveRecentSearches and implement it in WebsiteDataStoreCocoa.mm.

[Zach Li]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > Comment on attachment 263184 [details]
> > > Patch v2
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=263184&action=review
> > > 
> > > > Source/WebCore/platform/SearchPopupMenu.h:45
> > > > +WEBCORE_EXPORT static void removeRecentSearches(std::chrono::system_clock::time_point modifiedSince);
> > > 
> > > Indentation.
> > 
> > Will fix.
> > 
> > > 
> > > > Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:135
> > > > +    RetainPtr<NSDictionary> emptyItemsDictionary = adoptNS([[NSDictionary alloc] init]);
> > > > +    RetainPtr<NSDictionary> emptyRecentSearchesDictionary = adoptNS([[NSDictionary alloc] initWithObjectsAndKeys:emptyItemsDictionary.get(), itemsKey, nil]);
> > > 
> > > Can use auto here.
> > 
> > What is the benefit of using auto? Is it more modern?
> 
> It avoids having "NSDictionary" twice on one line.
> 
> > > 
> > > > Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:205
> > > > +        [recentSearchesPlist writeToFile:searchFieldRecentSearchesPlistPath() atomically:NO];
> > > 
> > > Should be atomically:YES here.
> > 
> > Darin suggested I should use atomically:NO because of the performance cost.
> 
> I think the performance cost is negligible and not corrupting data is better.

Jessie, Anders, and I talked, we will go with atomically:NO.

> 
> 
> > > > Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:1085
> > > > +    WebCore::SearchPopupMenu::removeRecentSearches(modifiedSince);
> > > 
> > > I don't think you should call into WebCore here. Why can't you just put the
> > > contents of removeRecentSearches here?
> > 
> > Originally I had the contents of removeRecentSearches in WebsiteDataStore,
> > but Darin thinks we should not do
> > 
> > #if PLATFORM(COCOA)
> > #endif
> > 
> > in WebsiteDataStore and it would be better to move this logic to the
> > platform independent SearchPopupMenu file.
> 
> Just call platformRemoveRecentSearches and implement it in
> WebsiteDataStoreCocoa.mm.

Let me try that.

[Zach Li]

Created attachment 263526 [details]
Patch

This patch addresses Anders' comments.

[Darin Adler]

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.

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.

[Zach Li]

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

[Jessie Berlin]

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

[Jessie Berlin]

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

[Zach Li]

Created attachment 263894 [details]
Patch

Addresses Darin's comments and check if it builds on all platforms

[WebKit Commit Bot]

Attachment 263894 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/efl/WebPageProxyEfl.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Zach Li]

Will fix the style issue after review.

[Zach Li]

Created attachment 264142 [details]
Patch Already Reviewed

Fix the coding style issue and the patch was r+ by Anders.

[WebKit Commit Bot]

Comment on attachment 264142 [details]
Patch Already Reviewed

Clearing flags on attachment: 264142

Committed r191628: <http://trac.webkit.org/changeset/191628>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Zach Li]

Created attachment 264196 [details]
Fix variable name issue in patch committed

The variable name and the parameter name are both `date`. Rename the variable name to `dateAdded` so that the comparison between dates works.

[Zach Li]

The "fix variable name issue patch" also needs to be landed, so I am reopening the bug for commit bots to operate on.

[WebKit Commit Bot]

Comment on attachment 264196 [details]
Fix variable name issue in patch committed

Clearing flags on attachment: 264196

Committed r192066: <http://trac.webkit.org/changeset/192066>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.