<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:darin@apple.com" title="Darin Adler <darin@apple.com>"> <span class="fn">Darin Adler</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - We should also store the time information for recent searches"
href="https://bugs.webkit.org/show_bug.cgi?id=148388">bug 148388</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #262212 Flags</td>
<td>review?
</td>
<td>review-
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - We should also store the time information for recent searches"
href="https://bugs.webkit.org/show_bug.cgi?id=148388#c28">Comment # 28</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - We should also store the time information for recent searches"
href="https://bugs.webkit.org/show_bug.cgi?id=148388">bug 148388</a>
from <span class="vcard"><a class="email" href="mailto:darin@apple.com" title="Darin Adler <darin@apple.com>"> <span class="fn">Darin Adler</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=262212&action=diff" name="attach_262212" title="Patch v4">attachment 262212</a> <a href="attachment.cgi?id=262212&action=edit" title="Patch v4">[details]</a></span>
Patch v4
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=262212&action=review">https://bugs.webkit.org/attachment.cgi?id=262212&action=review</a>
Looks generally good, but review- because of the incorrect code to read from disk (see below for details)
<span class="quote">> 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;</span >
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;
};
<span class="quote">> Source/WebCore/platform/win/SearchPopupMenuWin.cpp:94
> + searchItems.append(RecentSearchItem((String)(CFStringRef)item, std::chrono::system_clock::time_point::min()));</span >
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() });
<span class="quote">> Source/WebCore/rendering/RenderSearchField.cpp:94
> + m_recentSearches.insert(0, RecentSearchItem(value, std::chrono::system_clock::now()));</span >
Please use aggregate syntax rather than constructor syntax:
m_recentSearches.insert(0, { value, std::chrono::system_clock::now() });
<span class="quote">> Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:52
> +static NSMutableDictionary *readSearchFieldRecentSearchesPlist()
> +{
> + return [[NSMutableDictionary alloc] initWithContentsOfFile:searchFieldRecentSearchesPlistPath()];
> +}</span >
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.
<span class="quote">> Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:78
> + NSMutableDictionary *recentSearchesPlist = readSearchFieldRecentSearchesPlist();</span >
Here we leak the contents of this file. That’s why we need to use a RetainPtr or autorelease.
<span class="quote">> Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:80
> + [recentSearchesPlist[itemsKey] removeObjectForKey:name];</span >
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.
<span class="quote">> Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:84
> + NSTimeInterval timeInterval = duration_cast<duration<double>>(searchItem.time.time_since_epoch()).count();</span >
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.
<span class="quote">> Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:88
> + NSDictionary *recentSearch = @{
> + searchStringKey: searchItem.searchString,
> + dateKey: [NSDate dateWithTimeIntervalSince1970:timeInterval]
> + };</span >
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.
<span class="quote">> Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:94
> + [recentSearchesPlist[itemsKey] setObject:@{ searchesKey: items.get() } forKey:name];</span >
Same problem here as in the removeObjectForKey: call above, and same issue with literals.
<span class="quote">> Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:96
> + recentSearchesPlist = [@{ itemsKey: @{ name: @{ searchesKey: items.get() } } } mutableCopy];</span >
Same issue with literals. Doesn’t seem to good to make a mutable copy here. Wasteful.
<span class="quote">> Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:99
> + [recentSearchesPlist writeToFile:searchFieldRecentSearchesPlistPath() atomically:YES];</span >
Using atomically:YES is costly in performance and in NAND life. We should be sure this is necessary.
<span class="quote">> Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:112
> + NSArray *recentSearches = [[[recentSearchesPlist objectForKey:itemsKey] objectForKey:name] objectForKey:searchesKey];</span >
Same comment about calling objectForKey: on an object of unknown type as above.
<span class="quote">> 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)));</span >
Same comment about using helper functions to convert between NSDate and time points as above.
<span class="quote">> Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:116
> + searchItems.append(WebCore::RecentSearchItem((String)item[searchStringKey], searchItemTime));</span >
Same comment about aggregate syntax.
<span class="quote">> 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";</span >
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.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>