[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 #3 from Zach Li <zacharyli323 at gmail.com> ---
(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.

-- 
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/20151013/a157954d/attachment.html>