Bug 148388 – We should also store the time information for recent searches

• Component: WebKit API
• Status: RESOLVED FIXED

[Zach Li]

Currently we only save the search strings on a domain for <input type="search">. To fix rdar://problem/16876762, we should also save the time information for recent searches.

[Zach Li]

Created attachment 259769 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:75:  Missing space around : in range-based for statement  [whitespace/colon] [4]
Total errors found: 1 in 17 files


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

Comment on attachment 259769 [details]
Patch

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

> Source/WebCore/ChangeLog:4
> +        Replace Vector<String> with Vector<HashMap<String, double>> for recent searches
> +        in order to store additional time information.

From a cursory look, it looks like theses maps only ever contain one key each. Is this going to change? If not, why not use a Vector of String, double pairs instead?

[Zach Li]

Yes it only contains one item each. I should've used a Vector of (String, double) pair.

[Zach Li]

Created attachment 259787 [details]
Patch

It uses Vector<std::pair<String, double>> instead of Vector<HashMap<String, double>>.

[WebKit Commit Bot]

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


ERROR: Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:75:  Missing space around : in range-based for statement  [whitespace/colon] [4]
Total errors found: 1 in 17 files


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

Comment on attachment 259787 [details]
Patch

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

This seems to be breaking the Windows build.

> Source/WebCore/ChangeLog:4
> +        Replace Vector<String> with Vector<std::pair<String, double>> for recent searches
> +        in order to store additional time information.

Typically the summary begins with a high-level description of the change and what motivates it. Something along the lines of “Augment <input type=search>’s recent search history with the time each entry was added, in order to allow time-based clearing of search history”.

> Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:64
> +        [items addObject:@{ searchItem.first: [[NSNumber alloc] initWithDouble:searchItem.second] }];

Just like the single-object HashMap before, what is the point of a single-object dictionary? Why not just an array?

> Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:76
> +        if ([item isKindOfClass:[NSDictionary class]] && item.allKeys > 0) {

Checking if a pointer type (item.allKeys) is greater than an integer (0) doesn’t make much sense.

This also seems like it will fail to load information saved by the current version of the code, causing it to be lost.

> Source/WebKit2/UIProcess/Cocoa/WebPageProxyCocoa.mm:54
>          items = adoptCF(CFArrayCreateMutable(0, searchItems.size(), &kCFTypeArrayCallBacks));
>  
> -        for (const auto& searchItem : searchItems)
> -            CFArrayAppendValue(items.get(), searchItem.createCFString().get());
> +        for (const auto& searchItem : searchItems) {
> +            const void* searchItemString[] = { searchItem.first };
> +            const void* searchItemDate[] = { CFNumberCreate(kCFAllocatorDefault, kCFNumberDoubleType, &searchItem.second) };
> +            RetainPtr<CFDictionaryRef> searchItemMap = adoptCF(CFDictionaryCreate(kCFAllocatorDefault, searchItemString, searchItemDate, 1, &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks));
> +            CFArrayAppendValue(items.get(), searchItemMap.get());

Seems weird to use CF API instead of Foundation API in this Objective-C file.

> Source/WebKit2/UIProcess/Cocoa/WebPageProxyCocoa.mm:81
> +        if (auto item = dynamic_cf_cast<CFDictionaryRef>(CFArrayGetValueAtIndex(items.get(), i))) {
> +            CFIndex size = CFDictionaryGetCount(item);
> +            Vector<CFStringRef, 1> keys(size);
> +            Vector<CFNumberRef, 1> values(size);
> +            CFDictionaryGetKeysAndValues(item, reinterpret_cast<const void**>(keys.data()), reinterpret_cast<const void**>(values.data()));
> +            if (size > 0)
> +                searchItems.append(std::make_pair((String)keys[0], ((__bridge NSNumber*)values[0]).doubleValue));
> +        }

Ditto.

[Zach Li]

Created attachment 260183 [details]
Patch v2

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/Cocoa/WebPageProxyCocoa.mm:49:  Missing space around : in range-based for statement  [whitespace/colon] [4]
ERROR: Source/WebKit2/UIProcess/Cocoa/WebPageProxyCocoa.mm:61:  Missing space around : in range-based for statement  [whitespace/colon] [4]
ERROR: Source/WebKit2/UIProcess/Cocoa/WebPageProxyCocoa.mm:64:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:75:  Missing space around : in range-based for statement  [whitespace/colon] [4]
ERROR: Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:78:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 5 in 21 files


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

[Zach Li]

Created attachment 261153 [details]
Patch v2 for EWS

This patch is re-uploaded to see the build error on Windows platform.

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/Cocoa/WebPageProxyCocoa.mm:49:  Missing space around : in range-based for statement  [whitespace/colon] [4]
ERROR: Source/WebKit2/UIProcess/Cocoa/WebPageProxyCocoa.mm:61:  Missing space around : in range-based for statement  [whitespace/colon] [4]
ERROR: Source/WebKit2/UIProcess/Cocoa/WebPageProxyCocoa.mm:64:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:75:  Missing space around : in range-based for statement  [whitespace/colon] [4]
ERROR: Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:78:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 5 in 21 files


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

[Zach Li]

Created attachment 261233 [details]
Patch v2

Now it should build on Windows.

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/Cocoa/WebPageProxyCocoa.mm:49:  Missing space around : in range-based for statement  [whitespace/colon] [4]
ERROR: Source/WebKit2/UIProcess/Cocoa/WebPageProxyCocoa.mm:61:  Missing space around : in range-based for statement  [whitespace/colon] [4]
ERROR: Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:75:  Missing space around : in range-based for statement  [whitespace/colon] [4]
Total errors found: 3 in 21 files


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

[Zach Li]

The above style errors are false positive, which is tracked by <rdar://problem/22408083> check-webkit-style reports a false positive error.

[Anders Carlsson]

Comment on attachment 261233 [details]
Patch v2

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

I think you should use std::chrono::system_clock::time_point for times instead of double - then you can use std::chrono::system_clock::now() to create a time.

You can convert between time_points and NSDates using

    NSTimeInterval timeInterval = std::chrono::duration_cast<std::chrono::duration<double>>(modifiedSince.time_since_epoch()).count();
    NSDate *date = [NSDate dateWithTimeIntervalSince1970:timeInterval];

Does older versions of WebKit deal with the updated user defaults format?

> Source/WebCore/platform/win/SearchPopupMenuWin.cpp:96
> +        if (CFGetTypeID(item) == CFArrayGetTypeID() && CFArrayGetCount((CFArrayRef)item) == 2) {

Can use dynamic_cf_cast here.

> Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:64
> +        [items addObject:[NSArray arrayWithObjects:searchItem.first, [[NSNumber alloc] initWithDouble:searchItem.second], nil]];

I think you should use NSDate for the times instead.

> Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:77
> +        if ([item isKindOfClass:[NSArray class]] && ((NSArray*)item).count == 2)
> +            searchItems.append(std::make_pair((String)item[0], ((NSNumber*)item[1]).doubleValue));

You can use dynamic_objc_cast here.

[Zach Li]

Created attachment 261270 [details]
Patch v3

Addressed Anders' comments.

[Darin Adler]

Comment on attachment 261270 [details]
Patch v3

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

> Source/WebCore/platform/SearchPopupMenu.h:36
> +    virtual void saveRecentSearches(const AtomicString& name, const Vector<std::pair<String, double>>& searchItems) = 0;
> +    virtual void loadRecentSearches(const AtomicString& name, Vector<std::pair<String, double>>& searchItems) = 0;

This interface is confusing. Nothing about std::pair<String, double> indicates to me that one element is the search string and the other is a time. We’d probably be better off with a struct so we can name the two members.

Also, I suggest for new code we use std::chrono. Instead of double we would use std::chrono::time_point<std::chrono::system_clock>.

> Source/WebCore/rendering/RenderSearchField.cpp:95
> +    int size = static_cast<int>(m_recentSearches.size());
> +    for (int i = size - 1; i >= 0; --i) {
> +        if (m_recentSearches[i].first == value)
> +            m_recentSearches.remove(i);
> +    }

This is really ugly, especially the static_cast<int> part. We should try to do a little better.

> Source/WebCore/rendering/RenderSearchField.cpp:96
> +    m_recentSearches.insert(0, std::make_pair(value, (double)time(nullptr)));

We should definitely not be using the C standard library function time and converting it to a double! If we use std::chrono, then it would be std::chrono::system_clock::now().

[Darin Adler]

Hmm, it seems like Anders made some of the same comments I did, but they weren’t really addressed in patch v3.

[Zach Li]

Sorry, I clicked on "Review Patch" directly to view Anders' comments and it didn't show his suggestions about using std::chrono somehow. I will revise the patch.

[Zach Li]

Created attachment 261397 [details]
Patch v3

Check if it builds on all platforms.

[Jessie Berlin]

Comment on attachment 261397 [details]
Patch v3

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

These new searches that have a time associated with them should probably be put in a new location (aka with a new preference key). Clients of WebKit will probably need to do the migration from the old preference key to the new one - unless I am missing some facility in WebKit to do that directly. Anders?

> Source/WebCore/platform/win/SearchPopupMenuWin.cpp:106
> +            searchItems.append(RecentSearchItem((String)(CFStringRef)CFArrayGetValueAtIndex((CFArrayRef)item, 0), searchTime));

As Anders pointed out, this will probably break older clients.

> Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:86
> +            searchItems.append(RecentSearchItem((String)item, system_clock::time_point::min()));

As Anders pointed out, this will break older clients. You are storing an array or arrays where they are expecting an array of strings.

[Zach Li]

Comment on attachment 261233 [details]
Patch v2

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

>> Source/WebCore/platform/win/SearchPopupMenuWin.cpp:96
>> +        if (CFGetTypeID(item) == CFArrayGetTypeID() && CFArrayGetCount((CFArrayRef)item) == 2) {
> 
> Can use dynamic_cf_cast here.

I will change that.

>> Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:64
>> +        [items addObject:[NSArray arrayWithObjects:searchItem.first, [[NSNumber alloc] initWithDouble:searchItem.second], nil]];
> 
> I think you should use NSDate for the times instead.

Yup, I should, as using NSDate is consistent with other user defaults.

>> Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:77
>> +            searchItems.append(std::make_pair((String)item[0], ((NSNumber*)item[1]).doubleValue));
> 
> You can use dynamic_objc_cast here.

I will change that.

[Zach Li]

(In reply to comment #17)
> Comment on attachment 261270 [details]
> Patch v3
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=261270&action=review
> 
> > Source/WebCore/platform/SearchPopupMenu.h:36
> > +    virtual void saveRecentSearches(const AtomicString& name, const Vector<std::pair<String, double>>& searchItems) = 0;
> > +    virtual void loadRecentSearches(const AtomicString& name, Vector<std::pair<String, double>>& searchItems) = 0;
> 
> This interface is confusing. Nothing about std::pair<String, double>
> indicates to me that one element is the search string and the other is a
> time. We’d probably be better off with a struct so we can name the two
> members.

That sounds a lot better. I will use a struct to represent the recent search item.

> 
> Also, I suggest for new code we use std::chrono. Instead of double we would
> use std::chrono::time_point<std::chrono::system_clock>.
> 
> > Source/WebCore/rendering/RenderSearchField.cpp:95
> > +    int size = static_cast<int>(m_recentSearches.size());
> > +    for (int i = size - 1; i >= 0; --i) {
> > +        if (m_recentSearches[i].first == value)
> > +            m_recentSearches.remove(i);
> > +    }
> 
> This is really ugly, especially the static_cast<int> part. We should try to
> do a little better.

Totally agreed. I think removeAllMatching can make this better.

> 
> > Source/WebCore/rendering/RenderSearchField.cpp:96
> > +    m_recentSearches.insert(0, std::make_pair(value, (double)time(nullptr)));
> 
> We should definitely not be using the C standard library function time and
> converting it to a double! If we use std::chrono, then it would be
> std::chrono::system_clock::now().

I will change the time type to use std::chromo::system_clock::time_point instead of double! Thanks for the suggestions.

[Zach Li]

(In reply to comment #15)
> Comment on attachment 261233 [details]
> Patch v2
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=261233&action=review
> 
> I think you should use std::chrono::system_clock::time_point for times
> instead of double - then you can use std::chrono::system_clock::now() to
> create a time.
> 
> You can convert between time_points and NSDates using
> 
>     NSTimeInterval timeInterval =
> std::chrono::duration_cast<std::chrono::duration<double>>(modifiedSince.
> time_since_epoch()).count();
>     NSDate *date = [NSDate dateWithTimeIntervalSince1970:timeInterval];
> 
> Does older versions of WebKit deal with the updated user defaults format?

After talking with Jessie, older versions of WebKit do not deal with the updated user defaults format. I should think of a way to migrate the preferences.

> 
> > Source/WebCore/platform/win/SearchPopupMenuWin.cpp:96
> > +        if (CFGetTypeID(item) == CFArrayGetTypeID() && CFArrayGetCount((CFArrayRef)item) == 2) {
> 
> Can use dynamic_cf_cast here.
> 
> > Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:64
> > +        [items addObject:[NSArray arrayWithObjects:searchItem.first, [[NSNumber alloc] initWithDouble:searchItem.second], nil]];
> 
> I think you should use NSDate for the times instead.
> 
> > Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:77
> > +        if ([item isKindOfClass:[NSArray class]] && ((NSArray*)item).count == 2)
> > +            searchItems.append(std::make_pair((String)item[0], ((NSNumber*)item[1]).doubleValue));
> 
> You can use dynamic_objc_cast here.

[Zach Li]

Created attachment 262212 [details]
Patch v4

After discussing with Anders, we think it is better to store these recent searches in a plist in a new location.

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/Cocoa/WebPageProxyCocoa.mm:68:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 24 files


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

[Zach Li]

I will address the style issue after the review.

[Darin Adler]

Comment on attachment 262212 [details]
Patch v4

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

Looks generally good, but review- because of the incorrect code to read from disk (see below for details)

> Source/WebCore/platform/SearchPopupMenu.h:40
> +typedef struct RecentSearchItem {
> +    RecentSearchItem() { }
> +
> +    RecentSearchItem(const String& searchString, std::chrono::system_clock::time_point time)
> +        : searchString(searchString), time(time) { }
> +
> +    String searchString;
> +    std::chrono::system_clock::time_point time;
> +} RecentSearchItem;

This typedef struct X {} X; pattern is completely unnecessary in C++. Just struct X { } is fine. There is no need to create constructors for a struct like this in modern C++. I also don’t think there’s a good reason to call this searchString and time rather than searchString and searchTime or just string and time. And further, there’s no reason to call this both recent searches and recent search items. I don’t think the word items adds much. So this should be:

    struct RecentSearch {
        String string;
        std::chrono::system_clock::time_point time;
    };

> Source/WebCore/platform/win/SearchPopupMenuWin.cpp:94
> +            searchItems.append(RecentSearchItem((String)(CFStringRef)item, std::chrono::system_clock::time_point::min()));

The cast to CFStringRef here is not helpful since item is already a CFStringRef. The C-style cast to String here is not something we normally do. I suggest a C++ style cast. Passing min() here requires a comment; it’s not obvious why ti’s OK. And we should use aggregate syntax for this struct. So please add a comment explaining why min is correct and write something more like this:

    searchItems.append({ String{ item }, std::chrono::system_clock::time_point::min() });

> Source/WebCore/rendering/RenderSearchField.cpp:94
> +    m_recentSearches.insert(0, RecentSearchItem(value, std::chrono::system_clock::now()));

Please use aggregate syntax rather than constructor syntax:

    m_recentSearches.insert(0, { value, std::chrono::system_clock::now() });

> Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:52
> +static NSMutableDictionary *readSearchFieldRecentSearchesPlist()
> +{
> +    return [[NSMutableDictionary alloc] initWithContentsOfFile:searchFieldRecentSearchesPlistPath()];
> +}

This function is dangerous. It returns a newly created Objective-C object, but doesn’t use autorelease, nor wrap the pointer in a RetainPtr, nor does it follow the create/copy rule for naming functions that create new objects without autoreleasing them. I suggest using RetainPtr for the result here so we don’t leak memory.

I’m also not sure that [NSMutableDictionary initWithContentsOfFile:] is safe to use on an untrusted file. We need to instead use an appropriate API that doesn’t allow objects of arbitrary classes.

> Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:78
> +    NSMutableDictionary *recentSearchesPlist = readSearchFieldRecentSearchesPlist();

Here we leak the contents of this file. That’s why we need to use a RetainPtr or autorelease.

> Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:80
> +        [recentSearchesPlist[itemsKey] removeObjectForKey:name];

This won’t compile when targeting older versions of OS X, so we need to use objectForKey here instead of array subscripting syntax.

This code assumes that type of the key in the property list that we read in from will be a mutable dictionary and we can call removeObjectForKey on it. We don’t have a guarantee of that. We need to check the type of the object to see that it‘s a dictionary before modifying it, and we also need to make a mutable copy of the dictionary unless we are using an API that guarantees that any dictionaries it creates when reading the file are created mutable, which we currently are not doing.

> Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:84
> +            NSTimeInterval timeInterval = duration_cast<duration<double>>(searchItem.time.time_since_epoch()).count();

I’d like to see a helper function template to convert a duration object into an NSDate rather than writing this out here. It’s also not good that the code that creates the interval since 1970 is on a different line than the code that create the NSDate.

> Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:88
> +            NSDictionary *recentSearch = @{
> +                searchStringKey: searchItem.searchString,
> +                dateKey: [NSDate dateWithTimeIntervalSince1970:timeInterval]
> +            };

I’m pretty sure that the dictionary literal syntax is not supported in all tools that we use for the versions of OS X that we currently target with WebKit. This might need to use method calls instead. Unless I am mistaken.

> Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:94
> +            [recentSearchesPlist[itemsKey] setObject:@{ searchesKey: items.get() } forKey:name];

Same problem here as in the removeObjectForKey: call above, and same issue with literals.

> Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:96
> +            recentSearchesPlist = [@{ itemsKey: @{ name: @{ searchesKey: items.get() } } } mutableCopy];

Same issue with literals. Doesn’t seem to good to make a mutable copy here. Wasteful.

> Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:99
> +    [recentSearchesPlist writeToFile:searchFieldRecentSearchesPlistPath() atomically:YES];

Using atomically:YES is costly in performance and in NAND life. We should be sure this is necessary.

> Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:112
> +    NSArray *recentSearches = [[[recentSearchesPlist objectForKey:itemsKey] objectForKey:name] objectForKey:searchesKey];

Same comment about calling objectForKey: on an object of unknown type as above.

> Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:115
> +            system_clock::time_point searchItemTime = system_clock::time_point(duration_cast<system_clock::duration>(duration<double>(dynamic_objc_cast<NSDate>(item[dateKey]).timeIntervalSince1970)));

Same comment about using helper functions to convert between NSDate and time points as above.

> Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:116
> +            searchItems.append(WebCore::RecentSearchItem((String)item[searchStringKey], searchItemTime));

Same comment about aggregate syntax.

> Source/WebKit2/UIProcess/Cocoa/WebPageProxyCocoa.mm:38
> +static NSString * const dateKey = @"date";
> +static NSString * const itemsKey = @"items";
> +static NSString * const searchesKey = @"searches";
> +static NSString * const searchStringKey = @"searchString";

Seems quite bad to literally have copied and pasted code for WebKit 1 and WebKit 2. Please find a way to share more of the code.

[Zach Li]

(In reply to comment #28)
> Comment on attachment 262212 [details]
> Patch v4
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=262212&action=review
> 
> Looks generally good, but review- because of the incorrect code to read from
> disk (see below for details)

Yeah my code is not robust and I am making too many assumptions.

> 
> > Source/WebCore/platform/SearchPopupMenu.h:40
> > +typedef struct RecentSearchItem {
> > +    RecentSearchItem() { }
> > +
> > +    RecentSearchItem(const String& searchString, std::chrono::system_clock::time_point time)
> > +        : searchString(searchString), time(time) { }
> > +
> > +    String searchString;
> > +    std::chrono::system_clock::time_point time;
> > +} RecentSearchItem;
> 
> This typedef struct X {} X; pattern is completely unnecessary in C++. Just
> struct X { } is fine. There is no need to create constructors for a struct
> like this in modern C++. I also don’t think there’s a good reason to call
> this searchString and time rather than searchString and searchTime or just
> string and time. And further, there’s no reason to call this both recent
> searches and recent search items. I don’t think the word items adds much. So
> this should be:
> 
>     struct RecentSearch {
>         String string;
>         std::chrono::system_clock::time_point time;
>     };

I was looking at examples of how struct is used in other places in WebKit and a lot of them have constructors, so I thought it is a good practice, but I will refine the struct declaration.

> 
> > Source/WebCore/platform/win/SearchPopupMenuWin.cpp:94
> > +            searchItems.append(RecentSearchItem((String)(CFStringRef)item, std::chrono::system_clock::time_point::min()));
> 
> The cast to CFStringRef here is not helpful since item is already a
> CFStringRef. The C-style cast to String here is not something we normally
> do. I suggest a C++ style cast. Passing min() here requires a comment; it’s
> not obvious why ti’s OK. And we should use aggregate syntax for this struct.
> So please add a comment explaining why min is correct and write something
> more like this:
> 
>     searchItems.append({ String{ item },
> std::chrono::system_clock::time_point::min() });

I will add comments to explain why the use of min().

> 
> > Source/WebCore/rendering/RenderSearchField.cpp:94
> > +    m_recentSearches.insert(0, RecentSearchItem(value, std::chrono::system_clock::now()));
> 
> Please use aggregate syntax rather than constructor syntax:
> 
>     m_recentSearches.insert(0, { value, std::chrono::system_clock::now() });

Will do.
> 
> > Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:52
> > +static NSMutableDictionary *readSearchFieldRecentSearchesPlist()
> > +{
> > +    return [[NSMutableDictionary alloc] initWithContentsOfFile:searchFieldRecentSearchesPlistPath()];
> > +}
> 
> This function is dangerous. It returns a newly created Objective-C object,
> but doesn’t use autorelease, nor wrap the pointer in a RetainPtr, nor does
> it follow the create/copy rule for naming functions that create new objects
> without autoreleasing them. I suggest using RetainPtr for the result here so
> we don’t leak memory.

Nice catch. Will take care of the memory management.

> 
> I’m also not sure that [NSMutableDictionary initWithContentsOfFile:] is safe
> to use on an untrusted file. We need to instead use an appropriate API that
> doesn’t allow objects of arbitrary classes.

When you say "arbitrary classes", do you mean the plist should be a dictionary, not an array or a string, etc.?

> 
> > Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:78
> > +    NSMutableDictionary *recentSearchesPlist = readSearchFieldRecentSearchesPlist();
> 
> Here we leak the contents of this file. That’s why we need to use a
> RetainPtr or autorelease.

Will fix.

> 
> > Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:80
> > +        [recentSearchesPlist[itemsKey] removeObjectForKey:name];
> 
> This won’t compile when targeting older versions of OS X, so we need to use
> objectForKey here instead of array subscripting syntax.
> 
> This code assumes that type of the key in the property list that we read in
> from will be a mutable dictionary and we can call removeObjectForKey on it.
> We don’t have a guarantee of that. We need to check the type of the object
> to see that it‘s a dictionary before modifying it, and we also need to make
> a mutable copy of the dictionary unless we are using an API that guarantees
> that any dictionaries it creates when reading the file are created mutable,
> which we currently are not doing.

Will fix.

> 
> > Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:84
> > +            NSTimeInterval timeInterval = duration_cast<duration<double>>(searchItem.time.time_since_epoch()).count();
> 
> I’d like to see a helper function template to convert a duration object into
> an NSDate rather than writing this out here. It’s also not good that the
> code that creates the interval since 1970 is on a different line than the
> code that create the NSDate.

Will fix.

> 
> > Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:88
> > +            NSDictionary *recentSearch = @{
> > +                searchStringKey: searchItem.searchString,
> > +                dateKey: [NSDate dateWithTimeIntervalSince1970:timeInterval]
> > +            };
> 
> I’m pretty sure that the dictionary literal syntax is not supported in all
> tools that we use for the versions of OS X that we currently target with
> WebKit. This might need to use method calls instead. Unless I am mistaken.

Will fix.

> 
> > Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:94
> > +            [recentSearchesPlist[itemsKey] setObject:@{ searchesKey: items.get() } forKey:name];
> 
> Same problem here as in the removeObjectForKey: call above, and same issue
> with literals.

Will fix.

> 
> > Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:96
> > +            recentSearchesPlist = [@{ itemsKey: @{ name: @{ searchesKey: items.get() } } } mutableCopy];
> 
> Same issue with literals. Doesn’t seem to good to make a mutable copy here.
> Wasteful.

Will fix.

> 
> > Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:99
> > +    [recentSearchesPlist writeToFile:searchFieldRecentSearchesPlistPath() atomically:YES];
> 
> Using atomically:YES is costly in performance and in NAND life. We should be
> sure this is necessary.

I am setting it to YES because I am afraid the file might be corrupted while writing. With my previous approach of reading the plist from disk, I assume the file should be in the expected format. Your suggestions above will alleviate the problem. But I still think we should write atomically, what do you think?

> 
> > Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:112
> > +    NSArray *recentSearches = [[[recentSearchesPlist objectForKey:itemsKey] objectForKey:name] objectForKey:searchesKey];
> 
> Same comment about calling objectForKey: on an object of unknown type as
> above.

Will fix.

> 
> > Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:115
> > +            system_clock::time_point searchItemTime = system_clock::time_point(duration_cast<system_clock::duration>(duration<double>(dynamic_objc_cast<NSDate>(item[dateKey]).timeIntervalSince1970)));
> 
> Same comment about using helper functions to convert between NSDate and time
> points as above.

Will fix.

> 
> > Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:116
> > +            searchItems.append(WebCore::RecentSearchItem((String)item[searchStringKey], searchItemTime));
> 
> Same comment about aggregate syntax.

Will fix.

> 
> > Source/WebKit2/UIProcess/Cocoa/WebPageProxyCocoa.mm:38
> > +static NSString * const dateKey = @"date";
> > +static NSString * const itemsKey = @"items";
> > +static NSString * const searchesKey = @"searches";
> > +static NSString * const searchStringKey = @"searchString";
> 
> Seems quite bad to literally have copied and pasted code for WebKit 1 and
> WebKit 2. Please find a way to share more of the code.

Will do.

Thanks for the review!

[Darin Adler]

(In reply to comment #29)
> (In reply to comment #28)
> > > Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:99
> > > +    [recentSearchesPlist writeToFile:searchFieldRecentSearchesPlistPath() atomically:YES];
> > 
> > Using atomically:YES is costly in performance and in NAND life. We should be
> > sure this is necessary.
> 
> I am setting it to YES because I am afraid the file might be corrupted while
> writing. With my previous approach of reading the plist from disk, I assume
> the file should be in the expected format. Your suggestions above will
> alleviate the problem. But I still think we should write atomically, what do
> you think?

No, I don’t think we should do the atomic write. In theory it might reduce the chance that the file will be corrupted, but in practice I think its cost outweighs its benefit. Should probably talk to some other WebKit experts about it.

[Zach Li]

(In reply to comment #29)
> (In reply to comment #28)
> > Comment on attachment 262212 [details]
> > Patch v4
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=262212&action=review
> > 
> > Looks generally good, but review- because of the incorrect code to read from
> > disk (see below for details)
> 
> Yeah my code is not robust and I am making too many assumptions.
> 
> > 
> > > Source/WebCore/platform/SearchPopupMenu.h:40
> > > +typedef struct RecentSearchItem {
> > > +    RecentSearchItem() { }
> > > +
> > > +    RecentSearchItem(const String& searchString, std::chrono::system_clock::time_point time)
> > > +        : searchString(searchString), time(time) { }
> > > +
> > > +    String searchString;
> > > +    std::chrono::system_clock::time_point time;
> > > +} RecentSearchItem;
> > 
> > This typedef struct X {} X; pattern is completely unnecessary in C++. Just
> > struct X { } is fine. There is no need to create constructors for a struct
> > like this in modern C++. I also don’t think there’s a good reason to call
> > this searchString and time rather than searchString and searchTime or just
> > string and time. And further, there’s no reason to call this both recent
> > searches and recent search items. I don’t think the word items adds much. So
> > this should be:
> > 
> >     struct RecentSearch {
> >         String string;
> >         std::chrono::system_clock::time_point time;
> >     };
> 
> I was looking at examples of how struct is used in other places in WebKit
> and a lot of them have constructors, so I thought it is a good practice, but
> I will refine the struct declaration.
> 
> > 
> > > Source/WebCore/platform/win/SearchPopupMenuWin.cpp:94
> > > +            searchItems.append(RecentSearchItem((String)(CFStringRef)item, std::chrono::system_clock::time_point::min()));
> > 
> > The cast to CFStringRef here is not helpful since item is already a
> > CFStringRef. The C-style cast to String here is not something we normally
> > do. I suggest a C++ style cast. Passing min() here requires a comment; it’s
> > not obvious why ti’s OK. And we should use aggregate syntax for this struct.
> > So please add a comment explaining why min is correct and write something
> > more like this:
> > 
> >     searchItems.append({ String{ item },
> > std::chrono::system_clock::time_point::min() });
> 
> I will add comments to explain why the use of min().
> 
> > 
> > > Source/WebCore/rendering/RenderSearchField.cpp:94
> > > +    m_recentSearches.insert(0, RecentSearchItem(value, std::chrono::system_clock::now()));
> > 
> > Please use aggregate syntax rather than constructor syntax:
> > 
> >     m_recentSearches.insert(0, { value, std::chrono::system_clock::now() });
> 
> Will do.
> > 
> > > Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:52
> > > +static NSMutableDictionary *readSearchFieldRecentSearchesPlist()
> > > +{
> > > +    return [[NSMutableDictionary alloc] initWithContentsOfFile:searchFieldRecentSearchesPlistPath()];
> > > +}
> > 
> > This function is dangerous. It returns a newly created Objective-C object,
> > but doesn’t use autorelease, nor wrap the pointer in a RetainPtr, nor does
> > it follow the create/copy rule for naming functions that create new objects
> > without autoreleasing them. I suggest using RetainPtr for the result here so
> > we don’t leak memory.
> 
> Nice catch. Will take care of the memory management.
> 
> > 
> > I’m also not sure that [NSMutableDictionary initWithContentsOfFile:] is safe
> > to use on an untrusted file. We need to instead use an appropriate API that
> > doesn’t allow objects of arbitrary classes.

Dan and I looked at the implementation of [NSMutableDictionary initWithContentsOfFile:], it will return nil when the object is not a dictionary. It also should return all the container classes in mutable form. But I will check if the dictionary is mutable before modifying it.

> 
> When you say "arbitrary classes", do you mean the plist should be a
> dictionary, not an array or a string, etc.?
> 
> > 
> > > Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:78
> > > +    NSMutableDictionary *recentSearchesPlist = readSearchFieldRecentSearchesPlist();
> > 
> > Here we leak the contents of this file. That’s why we need to use a
> > RetainPtr or autorelease.
> 
> Will fix.
> 
> > 
> > > Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:80
> > > +        [recentSearchesPlist[itemsKey] removeObjectForKey:name];
> > 
> > This won’t compile when targeting older versions of OS X, so we need to use
> > objectForKey here instead of array subscripting syntax.
> > 
> > This code assumes that type of the key in the property list that we read in
> > from will be a mutable dictionary and we can call removeObjectForKey on it.
> > We don’t have a guarantee of that. We need to check the type of the object
> > to see that it‘s a dictionary before modifying it, and we also need to make
> > a mutable copy of the dictionary unless we are using an API that guarantees
> > that any dictionaries it creates when reading the file are created mutable,
> > which we currently are not doing.
> 
> Will fix.
> 
> > 
> > > Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:84
> > > +            NSTimeInterval timeInterval = duration_cast<duration<double>>(searchItem.time.time_since_epoch()).count();
> > 
> > I’d like to see a helper function template to convert a duration object into
> > an NSDate rather than writing this out here. It’s also not good that the
> > code that creates the interval since 1970 is on a different line than the
> > code that create the NSDate.
> 
> Will fix.
> 
> > 
> > > Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:88
> > > +            NSDictionary *recentSearch = @{
> > > +                searchStringKey: searchItem.searchString,
> > > +                dateKey: [NSDate dateWithTimeIntervalSince1970:timeInterval]
> > > +            };
> > 
> > I’m pretty sure that the dictionary literal syntax is not supported in all
> > tools that we use for the versions of OS X that we currently target with
> > WebKit. This might need to use method calls instead. Unless I am mistaken.

There are instances in WebKit where we use the dictionary literal syntax, but because
I had to use method calls in some places, I just used method calls instead of the
dictionary literal syntax in all the places.

> 
> Will fix.
> 
> > 
> > > Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:94
> > > +            [recentSearchesPlist[itemsKey] setObject:@{ searchesKey: items.get() } forKey:name];
> > 
> > Same problem here as in the removeObjectForKey: call above, and same issue
> > with literals.
> 
> Will fix.
> 
> > 
> > > Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:96
> > > +            recentSearchesPlist = [@{ itemsKey: @{ name: @{ searchesKey: items.get() } } } mutableCopy];
> > 
> > Same issue with literals. Doesn’t seem to good to make a mutable copy here.
> > Wasteful.
> 
> Will fix.
> 
> > 
> > > Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:99
> > > +    [recentSearchesPlist writeToFile:searchFieldRecentSearchesPlistPath() atomically:YES];
> > 
> > Using atomically:YES is costly in performance and in NAND life. We should be
> > sure this is necessary.

I decided to use atomically:NO in the end since I added all these checks when reading the plist from the disk so even if the file is corrupted, it shouldn't be a big concern and we avoid the costly performance issue by using atomically:YES.

> 
> I am setting it to YES because I am afraid the file might be corrupted while
> writing. With my previous approach of reading the plist from disk, I assume
> the file should be in the expected format. Your suggestions above will
> alleviate the problem. But I still think we should write atomically, what do
> you think?
> 
> > 
> > > Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:112
> > > +    NSArray *recentSearches = [[[recentSearchesPlist objectForKey:itemsKey] objectForKey:name] objectForKey:searchesKey];
> > 
> > Same comment about calling objectForKey: on an object of unknown type as
> > above.
> 
> Will fix.
> 
> > 
> > > Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:115
> > > +            system_clock::time_point searchItemTime = system_clock::time_point(duration_cast<system_clock::duration>(duration<double>(dynamic_objc_cast<NSDate>(item[dateKey]).timeIntervalSince1970)));
> > 
> > Same comment about using helper functions to convert between NSDate and time
> > points as above.
> 
> Will fix.
> 
> > 
> > > Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:116
> > > +            searchItems.append(WebCore::RecentSearchItem((String)item[searchStringKey], searchItemTime));
> > 
> > Same comment about aggregate syntax.
> 
> Will fix.
> 
> > 
> > > Source/WebKit2/UIProcess/Cocoa/WebPageProxyCocoa.mm:38
> > > +static NSString * const dateKey = @"date";
> > > +static NSString * const itemsKey = @"items";
> > > +static NSString * const searchesKey = @"searches";
> > > +static NSString * const searchStringKey = @"searchString";
> > 
> > Seems quite bad to literally have copied and pasted code for WebKit 1 and
> > WebKit 2. Please find a way to share more of the code.
> 
> Will do.
> 
> Thanks for the review!

(In reply to comment #28)
> Comment on attachment 262212 [details]
> Patch v4
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=262212&action=review
> 
> Looks generally good, but review- because of the incorrect code to read from
> disk (see below for details)
> 
> > Source/WebCore/platform/SearchPopupMenu.h:40
> > +typedef struct RecentSearchItem {
> > +    RecentSearchItem() { }
> > +
> > +    RecentSearchItem(const String& searchString, std::chrono::system_clock::time_point time)
> > +        : searchString(searchString), time(time) { }
> > +
> > +    String searchString;
> > +    std::chrono::system_clock::time_point time;
> > +} RecentSearchItem;
> 
> This typedef struct X {} X; pattern is completely unnecessary in C++. Just
> struct X { } is fine. There is no need to create constructors for a struct
> like this in modern C++. I also don’t think there’s a good reason to call
> this searchString and time rather than searchString and searchTime or just
> string and time. And further, there’s no reason to call this both recent
> searches and recent search items. I don’t think the word items adds much. So
> this should be:
> 
>     struct RecentSearch {
>         String string;
>         std::chrono::system_clock::time_point time;
>     };
> 
> > Source/WebCore/platform/win/SearchPopupMenuWin.cpp:94
> > +            searchItems.append(RecentSearchItem((String)(CFStringRef)item, std::chrono::system_clock::time_point::min()));
> 
> The cast to CFStringRef here is not helpful since item is already a
> CFStringRef. The C-style cast to String here is not something we normally
> do. I suggest a C++ style cast. Passing min() here requires a comment; it’s
> not obvious why ti’s OK. And we should use aggregate syntax for this struct.
> So please add a comment explaining why min is correct and write something
> more like this:
> 
>     searchItems.append({ String{ item },
> std::chrono::system_clock::time_point::min() });
> 
> > Source/WebCore/rendering/RenderSearchField.cpp:94
> > +    m_recentSearches.insert(0, RecentSearchItem(value, std::chrono::system_clock::now()));
> 
> Please use aggregate syntax rather than constructor syntax:
> 
>     m_recentSearches.insert(0, { value, std::chrono::system_clock::now() });
> 
> > Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:52
> > +static NSMutableDictionary *readSearchFieldRecentSearchesPlist()
> > +{
> > +    return [[NSMutableDictionary alloc] initWithContentsOfFile:searchFieldRecentSearchesPlistPath()];
> > +}
> 
> This function is dangerous. It returns a newly created Objective-C object,
> but doesn’t use autorelease, nor wrap the pointer in a RetainPtr, nor does
> it follow the create/copy rule for naming functions that create new objects
> without autoreleasing them. I suggest using RetainPtr for the result here so
> we don’t leak memory.
> 
> I’m also not sure that [NSMutableDictionary initWithContentsOfFile:] is safe
> to use on an untrusted file. We need to instead use an appropriate API that
> doesn’t allow objects of arbitrary classes.
> 
> > Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:78
> > +    NSMutableDictionary *recentSearchesPlist = readSearchFieldRecentSearchesPlist();
> 
> Here we leak the contents of this file. That’s why we need to use a
> RetainPtr or autorelease.
> 
> > Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:80
> > +        [recentSearchesPlist[itemsKey] removeObjectForKey:name];
> 
> This won’t compile when targeting older versions of OS X, so we need to use
> objectForKey here instead of array subscripting syntax.
> 
> This code assumes that type of the key in the property list that we read in
> from will be a mutable dictionary and we can call removeObjectForKey on it.
> We don’t have a guarantee of that. We need to check the type of the object
> to see that it‘s a dictionary before modifying it, and we also need to make
> a mutable copy of the dictionary unless we are using an API that guarantees
> that any dictionaries it creates when reading the file are created mutable,
> which we currently are not doing.
> 
> > Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:84
> > +            NSTimeInterval timeInterval = duration_cast<duration<double>>(searchItem.time.time_since_epoch()).count();
> 
> I’d like to see a helper function template to convert a duration object into
> an NSDate rather than writing this out here. It’s also not good that the
> code that creates the interval since 1970 is on a different line than the
> code that create the NSDate.
> 
> > Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:88
> > +            NSDictionary *recentSearch = @{
> > +                searchStringKey: searchItem.searchString,
> > +                dateKey: [NSDate dateWithTimeIntervalSince1970:timeInterval]
> > +            };
> 
> I’m pretty sure that the dictionary literal syntax is not supported in all
> tools that we use for the versions of OS X that we currently target with
> WebKit. This might need to use method calls instead. Unless I am mistaken.
> 
> > Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:94
> > +            [recentSearchesPlist[itemsKey] setObject:@{ searchesKey: items.get() } forKey:name];
> 
> Same problem here as in the removeObjectForKey: call above, and same issue
> with literals.
> 
> > Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:96
> > +            recentSearchesPlist = [@{ itemsKey: @{ name: @{ searchesKey: items.get() } } } mutableCopy];
> 
> Same issue with literals. Doesn’t seem to good to make a mutable copy here.
> Wasteful.
> 
> > Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:99
> > +    [recentSearchesPlist writeToFile:searchFieldRecentSearchesPlistPath() atomically:YES];
> 
> Using atomically:YES is costly in performance and in NAND life. We should be
> sure this is necessary.
> 
> > Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:112
> > +    NSArray *recentSearches = [[[recentSearchesPlist objectForKey:itemsKey] objectForKey:name] objectForKey:searchesKey];
> 
> Same comment about calling objectForKey: on an object of unknown type as
> above.
> 
> > Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:115
> > +            system_clock::time_point searchItemTime = system_clock::time_point(duration_cast<system_clock::duration>(duration<double>(dynamic_objc_cast<NSDate>(item[dateKey]).timeIntervalSince1970)));
> 
> Same comment about using helper functions to convert between NSDate and time
> points as above.
> 
> > Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:116
> > +            searchItems.append(WebCore::RecentSearchItem((String)item[searchStringKey], searchItemTime));
> 
> Same comment about aggregate syntax.
> 
> > Source/WebKit2/UIProcess/Cocoa/WebPageProxyCocoa.mm:38
> > +static NSString * const dateKey = @"date";
> > +static NSString * const itemsKey = @"items";
> > +static NSString * const searchesKey = @"searches";
> > +static NSString * const searchStringKey = @"searchString";
> 
> Seems quite bad to literally have copied and pasted code for WebKit 1 and
> WebKit 2. Please find a way to share more of the code.

[Zach Li]

Created attachment 262734 [details]
Patch v5

Implement the logic to save and load recent searches in WebCore so that WebKit and WebKit2 can share the code.

[Zach Li]

Created attachment 262745 [details]
Patch v5

[Darin Adler]

Comment on attachment 262745 [details]
Patch v5

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

Many comments about possible improvements. The only one that caused me to say review- was the lack of check of the type of itemsKey; most of the other ideas for improvement are optional.

> Source/WebCore/platform/SearchPopupMenu.h:24
>  #include "PopupMenu.h"

Seems like a forward declaration of PopupMenu would suffice; no need to include the entire header.

> Source/WebCore/platform/SearchPopupMenu.h:39
>      virtual ~SearchPopupMenu() {}

We normally put a space in braces in cases like this.

> Source/WebCore/platform/SearchPopupMenu.h:42
> +    virtual void loadRecentSearches(const AtomicString& name, Vector<RecentSearch>& searchItems) = 0;

It would be nice cleanup/modernization to change this to return a Vector instead of using an out argument.

> Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.h:3
> + * Copyright (C) 2006 Apple Inc.
> + * Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies).

These seem like the wrong copyrights for this code. While we did move some code from a file with these dates on it, I don’t think that’s accurate about who wrote the code that is now here. In particular, this header seems all new, so should be copyright 2015 Apple. Also missing All rights reserved here.

> Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.h:32
> +class SearchPopupMenuCocoa {
> +public:
> +WEBCORE_EXPORT static void saveRecentSearches(const AtomicString& name, const Vector<RecentSearch>& searchItems);
> +WEBCORE_EXPORT static void loadRecentSearches(const AtomicString& name, Vector<RecentSearch>& searchItems);
> +};

There’s no reason we need to put these functions in a namespace with the name Cocoa in it. The code is compiled only for Cocoa and doesn’t need to reiterate its platform name in the function or namespace name.

These two functions should just be free functions. No value in wrapping them in a class and very little value in namespacing them at all. If you really do want them in a namespace, then I suggest a namespace, not a class. But the names seem sufficiently unique to be at the WebCore namespace level.

The loadRecentSearches function should use a return value rather than an out argument.

> Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:3
> + * Copyright (C) 2006 Apple Inc.
> + * Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies).

Same comment about copyright date. The code here is almost all new. I don’t think it needs the old copyright on it just because it is obliquely related to the code that was in WebKit2.

> Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:40
> +    return storagePath.stringByStandardizingPath.stringByResolvingSymlinksInPath;

Is it important to call stringByStandardizingPath and stringByResolvingSymlinksInPath? We’re not storing this path, just using it to write or read a file. I don’t think either of those operations are needed.

> Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:57
> +static system_clock::time_point fromNSDatetoSystemClockTime(NSDate *date)

When we write “to” functions we should list the “to” part first because that’s the return type and what’s most useful to see at the call site. Should be named toSystemClockTime. The argument type already makes it clear this takes an NSDate so I don’t think it needs to be named toSystemClockTimeFromNSDate.

> Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:64
> +static NSDate *fromSystemClockTimetoNSDate(system_clock::time_point time)

For the same reason as above, this should  be named toNSDate or perhaps toNSDateFromSystemClock.

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

Same comment about secure coding.

> Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:110
> +    NSArray *recentSearches = [[[recentSearchesPlist objectForKey:itemsKey] objectForKey:name] objectForKey:searchesKey];
> +    if (![recentSearches isKindOfClass:[NSArray class]])
> +        return;

This is missing a check of the type of the object stored under itemsKey. We need to check that is an NSDictionary.

> Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:114
> +        if (![item isKindOfClass:[NSDictionary class]] || item.count != 2)
> +            continue;

Should omit this item.count check. Harmless to ignore additional key/value pairs in the dictionary; perhaps we might want to add a third key later without breaking ability of older versions of WebKit reading it.

Generally speaking we don’t check for extra things in dictionaries like this. We don’t do it for the top level dictionary above, for example.

> Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:124
> +        searchItems.append({ String { searchString }, fromNSDatetoSystemClockTime(date) });

I think our usual formatting puts the class name right next to the { }, so it can be:

    { String{ searchString } ...

> Source/WebCore/platform/win/SearchPopupMenuWin.cpp:96
> +            // The timestamp is not used on Windows platform, but we need to conform to the new RecentSearch struct type.
> +            // The time is initialized to min() because we do not know the time when the search string was created so we
> +            // assume it was created long time ago.

It’s not appropriate to refer to something as “new” in a comment like this. The type won’t be new years from now when someone is reading this comment.

Comment should be more like:

    // We are choosing not to use or store search times on Windows at this time, so for now it's OK to use a "distant past" time as a placeholder.

That may not be exactly right, but it’s more like the kind of comment we should write.

I also think we could just use { } instead of std::chrono::system_clock::time_point::min(); it would store the epoch instead of the minimum time, which I think might also be OK. But I guess min() is slightly better.

> Source/WebCore/platform/win/SearchPopupMenuWin.cpp:97
> +            searchItems.append({ String { item }, std::chrono::system_clock::time_point::min() });

I think our usual formatting puts the class name right next to the { }, so it can be:

    { String{ item } ...

> Source/WebCore/rendering/RenderSearchField.cpp:95
> +    RecentSearch recentSearch = { value, std::chrono::system_clock::now() };
> +    m_recentSearches.insert(0, recentSearch);

Do we need a local variable? What happens if we just write this?

    m_recentSearches.insert(0, { value, std::chrono::system_clock::now() });

A while ago someone made sure that Vector::append was properly overloaded to make things like this work. Maybe it hasn’t been done yet for Vector::insert.

> Source/WebCore/rendering/RenderSearchField.h:28
> +#include "SearchPopupMenu.h"

We can get rid of the forward declaration of the SearchPopupMenu class below now that we are including the header. Unfortunate we have to do that, but not so bad I guess.

> Source/WebKit2/UIProcess/Cocoa/WebPageProxyCocoa.mm:-43
> -    if (!name) {
> -        // FIXME: This should be a message check.
> -        return;
> -    }

Doesn’t seem good to be removing this code and comment, unless the FIXME is inaccurate.

> Source/WebKit2/UIProcess/Cocoa/WebPageProxyCocoa.mm:-63
> -    if (!name) {
> -        // FIXME: This should be a message check.
> -        return;
> -    }

Doesn’t seem good to be removing this code and comment, unless the FIXME is inaccurate.

[Zach Li]

(In reply to comment #34)
> Comment on attachment 262745 [details]
> Patch v5
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=262745&action=review
> 
> Many comments about possible improvements. The only one that caused me to
> say review- was the lack of check of the type of itemsKey; most of the other
> ideas for improvement are optional.
> 
> > Source/WebCore/platform/SearchPopupMenu.h:24
> >  #include "PopupMenu.h"
> 
> Seems like a forward declaration of PopupMenu would suffice; no need to
> include the entire header.

I will change it to forward declaration, and this change will result me in including
the header in RenderSearchField.cpp file now.

> 
> > Source/WebCore/platform/SearchPopupMenu.h:39
> >      virtual ~SearchPopupMenu() {}
> 
> We normally put a space in braces in cases like this.

Will fix since it is a trivial change.

> 
> > Source/WebCore/platform/SearchPopupMenu.h:42
> > +    virtual void loadRecentSearches(const AtomicString& name, Vector<RecentSearch>& searchItems) = 0;
> 
> It would be nice cleanup/modernization to change this to return a Vector
> instead of using an out argument.

I think I will leave this improvement to later because changing this function declaration means changes in a lot of different places and it is not what the patch primarily addresses.

> 
> > Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.h:3
> > + * Copyright (C) 2006 Apple Inc.
> > + * Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies).
> 
> These seem like the wrong copyrights for this code. While we did move some
> code from a file with these dates on it, I don’t think that’s accurate about
> who wrote the code that is now here. In particular, this header seems all
> new, so should be copyright 2015 Apple. Also missing All rights reserved
> here.

I will use the 2015 copyright statement.

> 
> > Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.h:32
> > +class SearchPopupMenuCocoa {
> > +public:
> > +WEBCORE_EXPORT static void saveRecentSearches(const AtomicString& name, const Vector<RecentSearch>& searchItems);
> > +WEBCORE_EXPORT static void loadRecentSearches(const AtomicString& name, Vector<RecentSearch>& searchItems);
> > +};
> 
> There’s no reason we need to put these functions in a namespace with the
> name Cocoa in it. The code is compiled only for Cocoa and doesn’t need to
> reiterate its platform name in the function or namespace name.
> 
> These two functions should just be free functions. No value in wrapping them
> in a class and very little value in namespacing them at all. If you really
> do want them in a namespace, then I suggest a namespace, not a class. But
> the names seem sufficiently unique to be at the WebCore namespace level.

Originally I was afraid of function name collision, but I guess it is unique enough to
be at the WebCore namespace level.

> 
> The loadRecentSearches function should use a return value rather than an out
> argument.

I will change that.

> 
> > Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:3
> > + * Copyright (C) 2006 Apple Inc.
> > + * Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies).
> 
> Same comment about copyright date. The code here is almost all new. I don’t
> think it needs the old copyright on it just because it is obliquely related
> to the code that was in WebKit2.
> 
> > Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:40
> > +    return storagePath.stringByStandardizingPath.stringByResolvingSymlinksInPath;
> 
> Is it important to call stringByStandardizingPath and
> stringByResolvingSymlinksInPath? We’re not storing this path, just using it
> to write or read a file. I don’t think either of those operations are needed.

You are right, looking at the documentation, these are not necessary. The reason why I had them before was that we used them in WebProcessPoolCocoa.mm

> 
> > Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:57
> > +static system_clock::time_point fromNSDatetoSystemClockTime(NSDate *date)
> 
> When we write “to” functions we should list the “to” part first because
> that’s the return type and what’s most useful to see at the call site.
> Should be named toSystemClockTime. The argument type already makes it clear
> this takes an NSDate so I don’t think it needs to be named
> toSystemClockTimeFromNSDate.

I will use toSystemClockTime.

> 
> > Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:64
> > +static NSDate *fromSystemClockTimetoNSDate(system_clock::time_point time)
> 
> For the same reason as above, this should  be named toNSDate or perhaps
> toNSDateFromSystemClock.

I will use toNSDateFromSystemClock.

> 
> > Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:95
> > +    [recentSearchesPlist writeToFile:searchFieldRecentSearchesPlistPath() atomically:NO];
> 
> Same comment about secure coding.

Somehow, I couldn't find the comments about secure coding?

> 
> > Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:110
> > +    NSArray *recentSearches = [[[recentSearchesPlist objectForKey:itemsKey] objectForKey:name] objectForKey:searchesKey];
> > +    if (![recentSearches isKindOfClass:[NSArray class]])
> > +        return;
> 
> This is missing a check of the type of the object stored under itemsKey. We
> need to check that is an NSDictionary.

You are absolutely right. Plus I need to check the type of the object stored under name is also an NSDictionary.

> 
> > Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:114
> > +        if (![item isKindOfClass:[NSDictionary class]] || item.count != 2)
> > +            continue;
> 
> Should omit this item.count check. Harmless to ignore additional key/value
> pairs in the dictionary; perhaps we might want to add a third key later
> without breaking ability of older versions of WebKit reading it.

I will revise this.

> 
> Generally speaking we don’t check for extra things in dictionaries like
> this. We don’t do it for the top level dictionary above, for example.
> 
> > Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:124
> > +        searchItems.append({ String { searchString }, fromNSDatetoSystemClockTime(date) });
> 
> I think our usual formatting puts the class name right next to the { }, so
> it can be:
> 
>     { String{ searchString } ...

I had { String{ searchString } ... before, but when I ran check-webkit-style, I got this error: Missing space before {. I searched in the WebKit repository and there are no examples of using either String{ or String { to do the cast, so I will leave it as it is for now unless you feel strongly about using the style above, I will change it to your suggested style and file a bug against check-webkit-style.

> 
> > Source/WebCore/platform/win/SearchPopupMenuWin.cpp:96
> > +            // The timestamp is not used on Windows platform, but we need to conform to the new RecentSearch struct type.
> > +            // The time is initialized to min() because we do not know the time when the search string was created so we
> > +            // assume it was created long time ago.
> 
> It’s not appropriate to refer to something as “new” in a comment like this.
> The type won’t be new years from now when someone is reading this comment.

Totally missed this...

> 
> Comment should be more like:
> 
>     // We are choosing not to use or store search times on Windows at this
> time, so for now it's OK to use a "distant past" time as a placeholder.
> 
> That may not be exactly right, but it’s more like the kind of comment we
> should write.

I will use the comments above. It is more clear and concise than what I can come up with.

> 
> I also think we could just use { } instead of
> std::chrono::system_clock::time_point::min(); it would store the epoch
> instead of the minimum time, which I think might also be OK. But I guess
> min() is slightly better.

Okay, I will stick with min(). My design is, when we support storing search times on Windows, these search entries in the old format will be delete when users select to delete all history.

> 
> > Source/WebCore/platform/win/SearchPopupMenuWin.cpp:97
> > +            searchItems.append({ String { item }, std::chrono::system_clock::time_point::min() });
> 
> I think our usual formatting puts the class name right next to the { }, so
> it can be:
> 
>     { String{ item } ...

Same comment as above.

> 
> > Source/WebCore/rendering/RenderSearchField.cpp:95
> > +    RecentSearch recentSearch = { value, std::chrono::system_clock::now() };
> > +    m_recentSearches.insert(0, recentSearch);
> 
> Do we need a local variable? What happens if we just write this?
> 
>     m_recentSearches.insert(0, { value, std::chrono::system_clock::now() });
> 
> A while ago someone made sure that Vector::append was properly overloaded to
> make things like this work. Maybe it hasn’t been done yet for Vector::insert.

I don't think this is done for Vector::insert, I used m_recentSearches.insert(0, { value, std::chrono::system_clock::now() }) before, but it gave me "No matching member
function for call to 'insert'".

> 
> > Source/WebCore/rendering/RenderSearchField.h:28
> > +#include "SearchPopupMenu.h" 
> 
> We can get rid of the forward declaration of the SearchPopupMenu class below
> now that we are including the header. Unfortunate we have to do that, but
> not so bad I guess.

Will fix.

> 
> > Source/WebKit2/UIProcess/Cocoa/WebPageProxyCocoa.mm:-43
> > -    if (!name) {
> > -        // FIXME: This should be a message check.
> > -        return;
> > -    }
> 
> Doesn’t seem good to be removing this code and comment, unless the FIXME is
> inaccurate.

Will restore it.

> 
> > Source/WebKit2/UIProcess/Cocoa/WebPageProxyCocoa.mm:-63
> > -    if (!name) {
> > -        // FIXME: This should be a message check.
> > -        return;
> > -    }
> 
> Doesn’t seem good to be removing this code and comment, unless the FIXME is
> inaccurate.

Ditto.

[Zach Li]

Created attachment 262822 [details]
Patch

New patch and see if it builds on all platforms.

[Darin Adler]

Comment on attachment 262822 [details]
Patch

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

General looks really good. I’d love to say review+ but there is still a bit of wrong code here.

> Source/WebCore/platform/SearchPopupMenu.h:36
> +class PopupMenu;

Normally we’d put a forward declaration before something defined in this file, so this would be slightly better above RecentSearch.

> Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:52
> +static RetainPtr<NSMutableDictionary>readSearchFieldRecentSearchesPlist()

Missing space here before the word "read".

> Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:56
> +    if (![[recentSearchesPlist classForCoder] isSubclassOfClass:[NSMutableDictionary class]])
> +        return nil;

I don’t understand what good this check does. Seems like this should just be removed. -[NSMutableDictionary initWithContentsOfFile:] is guaranteed to return a mutable dictionary or nil; no need to double check that the dictionary is mutable.

> Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:96
> +        NSMutableDictionary *itemsDictionary = [recentSearchesPlist objectForKey:itemsKey];
> +        if ([[itemsDictionary classForCoder] isSubclassOfClass:[NSMutableDictionary class]])
> +            [itemsDictionary setObject:[NSMutableDictionary dictionaryWithObjectsAndKeys:items.get(), searchesKey, nil] forKey:name];
> +        else
> +            [recentSearchesPlist setObject:[NSMutableDictionary dictionary] forKey:itemsKey];

I don’t think this code is correct.

The use of classForCoder seems a peculiar way to check if a dictionary is mutable; the best practice is *not* to check but guarantee it is mutable. Typically by using mutableCopy on a dictionary.

More importantly, if -[NSMutableDictionary initWithContentsOfFile:] does not guarantee it creates all mutable collections, then we’d want to make a mutable copy of an immutable dictionary we find, not throw it away and make a new empty dictionary.

And finally, it looks like else part of the if does this completely wrong. It uses the items array as a key, not a value!

Did we test this? How?

> Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:137
> +        searchItems.append({ String { searchString }, toSystemClockTime(date) });

No space after String before the "{" as I mentioned in my last review.

> Source/WebCore/platform/win/SearchPopupMenuWin.cpp:95
> +            searchItems.append({ String { item }, std::chrono::system_clock::time_point::min() });

No space after String before the "{" as I mentioned in my last review.

[Darin Adler]

Comment on attachment 262822 [details]
Patch

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

>> Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:137
>> +        searchItems.append({ String { searchString }, toSystemClockTime(date) });
> 
> No space after String before the "{" as I mentioned in my last review.

Sorry that I missed your comment last time about being driven by a complaint from the style checker.

>> Source/WebCore/platform/win/SearchPopupMenuWin.cpp:95
>> +            searchItems.append({ String { item }, std::chrono::system_clock::time_point::min() });
> 
> No space after String before the "{" as I mentioned in my last review.

Sorry that I missed your comment last time about being driven by a complaint from the style checker.

[Zach Li]

(In reply to comment #37)
> Comment on attachment 262822 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=262822&action=review
> 
> General looks really good. I’d love to say review+ but there is still a bit
> of wrong code here.
> 
> > Source/WebCore/platform/SearchPopupMenu.h:36
> > +class PopupMenu;
> 
> Normally we’d put a forward declaration before something defined in this
> file, so this would be slightly better above RecentSearch.

Will move.

> 
> > Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:52
> > +static RetainPtr<NSMutableDictionary>readSearchFieldRecentSearchesPlist()
> 
> Missing space here before the word "read".

Will fix.

> 
> > Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:56
> > +    if (![[recentSearchesPlist classForCoder] isSubclassOfClass:[NSMutableDictionary class]])
> > +        return nil;
> 
> I don’t understand what good this check does. Seems like this should just be
> removed. -[NSMutableDictionary initWithContentsOfFile:] is guaranteed to
> return a mutable dictionary or nil; no need to double check that the
> dictionary is mutable.

At Foundation level after calling creation method in CoreFoundation, it is only checking if it is a dictionary. But at CoreFoundation level, when the object is created, it is guaranteed to return a mutable dictionary or nil. So I guess the check is not necessary.

> 
> > Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:96
> > +        NSMutableDictionary *itemsDictionary = [recentSearchesPlist objectForKey:itemsKey];
> > +        if ([[itemsDictionary classForCoder] isSubclassOfClass:[NSMutableDictionary class]])
> > +            [itemsDictionary setObject:[NSMutableDictionary dictionaryWithObjectsAndKeys:items.get(), searchesKey, nil] forKey:name];
> > +        else
> > +            [recentSearchesPlist setObject:[NSMutableDictionary dictionary] forKey:itemsKey];
> 
> I don’t think this code is correct.
> 
> The use of classForCoder seems a peculiar way to check if a dictionary is
> mutable; the best practice is *not* to check but guarantee it is mutable.
> Typically by using mutableCopy on a dictionary.
> 
> More importantly, if -[NSMutableDictionary initWithContentsOfFile:] does not
> guarantee it creates all mutable collections, then we’d want to make a
> mutable copy of an immutable dictionary we find, not throw it away and make
> a new empty dictionary.

The reason I have this check is that itemsDictionary is not guaranteed to be a mutable dictionary, it could be a mutable array or a mutable string, calling setObject:forKey: on those types would be bad. So I think the check is necessary.

> 
> And finally, it looks like else part of the if does this completely wrong.
> It uses the items array as a key, not a value!

In the else part of the if, it uses itemsKey as the key and does not have anything to do with the items array. Are you referring to something else?

So if it is not a mutable dictionary, it means it could be a mutable array or a mutable string, then that probably means the plist is corrupted, so I want to clear the plist and make it an empty dictionary.

> 
> Did we test this? How?

We currently do not have existing tests for loading and saving recent searches. I tested manually by doing searches on different websites, checking the recent searches are shown correctly and the RecentSearches.plist is in the structure I expect it to be.

> 
> > Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:137
> > +        searchItems.append({ String { searchString }, toSystemClockTime(date) });
> 
> No space after String before the "{" as I mentioned in my last review.

Okay. And I filed a bug <rdar://problem/23077261> against check-webkit-style.

> 
> > Source/WebCore/platform/win/SearchPopupMenuWin.cpp:95
> > +            searchItems.append({ String { item }, std::chrono::system_clock::time_point::min() });
> 
> No space after String before the "{" as I mentioned in my last review.

Okay. And I filed a bug <rdar://problem/23077261> against check-webkit-style.

[Anders Carlsson]

(In reply to comment #28)
> Comment on attachment 262212 [details]
> Patch v4
> 
> 
> This function is dangerous. It returns a newly created Objective-C object,
> but doesn’t use autorelease, nor wrap the pointer in a RetainPtr, nor does
> it follow the create/copy rule for naming functions that create new objects
> without autoreleasing them. I suggest using RetainPtr for the result here so
> we don’t leak memory.
> 
> I’m also not sure that [NSMutableDictionary initWithContentsOfFile:] is safe
> to use on an untrusted file. We need to instead use an appropriate API that
> doesn’t allow objects of arbitrary classes.

I agree. This should use NSPropertyListSerialization, and pass NSPropertyListMutableContainersAndLeaves for its mutability options.

[Anders Carlsson]

(In reply to comment #39)
> 
> The reason I have this check is that itemsDictionary is not guaranteed to be
> a mutable dictionary, it could be a mutable array or a mutable string,
> calling setObject:forKey: on those types would be bad. So I think the check
> is necessary.
> 

Please don't check isSubclassOfClass, just use isKindOfClass.

[Zach Li]

(In reply to comment #40)
> (In reply to comment #28)
> > Comment on attachment 262212 [details]
> > Patch v4
> > 
> > 
> > This function is dangerous. It returns a newly created Objective-C object,
> > but doesn’t use autorelease, nor wrap the pointer in a RetainPtr, nor does
> > it follow the create/copy rule for naming functions that create new objects
> > without autoreleasing them. I suggest using RetainPtr for the result here so
> > we don’t leak memory.
> > 
> > I’m also not sure that [NSMutableDictionary initWithContentsOfFile:] is safe
> > to use on an untrusted file. We need to instead use an appropriate API that
> > doesn’t allow objects of arbitrary classes.
> 
> I agree. This should use NSPropertyListSerialization, and pass
> NSPropertyListMutableContainersAndLeaves for its mutability options.

I think NSPropertyListMutableContainers will suffice, which is what [NSMutableDictionary initWithContentsOfFile:] will pass to NSPropertyListSerialization.  We do not need things like NSString in the plist to be guaranteed to be mutable; we just need the containers to be guaranteed mutable.

[Zach Li]

(In reply to comment #41)
> (In reply to comment #39)
> > 
> > The reason I have this check is that itemsDictionary is not guaranteed to be
> > a mutable dictionary, it could be a mutable array or a mutable string,
> > calling setObject:forKey: on those types would be bad. So I think the check
> > is necessary.
> > 
> 
> Please don't check isSubclassOfClass, just use isKindOfClass.

Will use isKindOfClass:, with Darin's help.

[Zach Li]

Created attachment 262998 [details]
Revised Patch

Talked with Anders, the work to add tests to WebKit is probably a lot, not sure if it is worth it for this patch, but I did test manually.

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:131:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/win/SearchPopupMenuWin.cpp:95:  Missing space before {  [whitespace/braces] [5]
Total errors found: 2 in 27 files


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

[Zach Li]

(In reply to comment #45)
> Attachment 262998 [details] did not pass style-queue:
> 
> 
> ERROR: Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:131:  Missing
> space before {  [whitespace/braces] [5]
> ERROR: Source/WebCore/platform/win/SearchPopupMenuWin.cpp:95:  Missing space
> before {  [whitespace/braces] [5]
> Total errors found: 2 in 27 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

rdar://problem/23077261 is tracking this.

[Darin Adler]

Comment on attachment 262998 [details]
Revised Patch

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

> Source/WebCore/platform/SearchPopupMenu.h:42
> +    virtual void saveRecentSearches(const AtomicString& name, const Vector<RecentSearch>& searchItems) = 0;

No need for the second argument to have a name; searchItems is redundant with the type Vector<RecentSearch>.

> Source/WebCore/platform/SearchPopupMenu.h:43
> +    virtual void loadRecentSearches(const AtomicString& name, Vector<RecentSearch>& searchItems) = 0;

No need for the second argument to have a name; searchItems is redundant with the type Vector<RecentSearch>.

We should come back and change this function to use a return value instead of an out argument in a later patch.

> Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:87
> +        for (auto searchItem : searchItems)

This should be auto& rather than auto, since we have no need to copy each searchItem as we iterate the vector.

[Zach Li]

(In reply to comment #47)
> Comment on attachment 262998 [details]
> Revised Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=262998&action=review
> 
> > Source/WebCore/platform/SearchPopupMenu.h:42
> > +    virtual void saveRecentSearches(const AtomicString& name, const Vector<RecentSearch>& searchItems) = 0;
> 
> No need for the second argument to have a name; searchItems is redundant
> with the type Vector<RecentSearch>.

Will change.

> 
> > Source/WebCore/platform/SearchPopupMenu.h:43
> > +    virtual void loadRecentSearches(const AtomicString& name, Vector<RecentSearch>& searchItems) = 0;
> 
> No need for the second argument to have a name; searchItems is redundant
> with the type Vector<RecentSearch>.

Will change.

> 
> We should come back and change this function to use a return value instead
> of an out argument in a later patch.

Okay.

> 
> > Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:87
> > +        for (auto searchItem : searchItems)
> 
> This should be auto& rather than auto, since we have no need to copy each
> searchItem as we iterate the vector.

Oops.

[Zach Li]

Created attachment 263136 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:131:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/win/SearchPopupMenuWin.cpp:95:  Missing space before {  [whitespace/braces] [5]
Total errors found: 2 in 27 files


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

[Zach Li]

(In reply to comment #50)
> Attachment 263136 [details] did not pass style-queue:
> 
> 
> ERROR: Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:131:  Missing
> space before {  [whitespace/braces] [5]
> ERROR: Source/WebCore/platform/win/SearchPopupMenuWin.cpp:95:  Missing space
> before {  [whitespace/braces] [5]
> Total errors found: 2 in 27 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

rdar://problem/23077261 is tracking this.

[Stephanie Lewis]

Comment on attachment 263136 [details]
Patch

Looks like he addressed Darin's last comments so giving him a cq+.

[WebKit Commit Bot]

Comment on attachment 263136 [details]
Patch

Clearing flags on attachment: 263136

Committed r191084: <http://trac.webkit.org/changeset/191084>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.