[Webkit-unassigned] [Bug 148388] We should also store the time information for recent searches

[bugzilla-daemon at webkit.org]

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

Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #262212|review?                     |review-
              Flags|                            |

--- Comment #28 from Darin Adler <darin at apple.com> ---
Comment on attachment 262212
  --> https://bugs.webkit.org/attachment.cgi?id=262212
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.

-- 
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/20151001/b0acb728/attachment-0001.html>