<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<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#c7">Comment # 7</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:mitz@webkit.org" title="mitz@webkit.org <mitz@webkit.org>"> <span class="fn">mitz@webkit.org</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=259787&action=diff" name="attach_259787" title="Patch">attachment 259787</a> <a href="attachment.cgi?id=259787&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=259787&action=review">https://bugs.webkit.org/attachment.cgi?id=259787&action=review</a>
This seems to be breaking the Windows build.
<span class="quote">> Source/WebCore/ChangeLog:4
> + Replace Vector<String> with Vector<std::pair<String, double>> for recent searches
> + in order to store additional time information.</span >
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”.
<span class="quote">> Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:64
> + [items addObject:@{ searchItem.first: [[NSNumber alloc] initWithDouble:searchItem.second] }];</span >
Just like the single-object HashMap before, what is the point of a single-object dictionary? Why not just an array?
<span class="quote">> Source/WebKit/mac/WebCoreSupport/SearchPopupMenuMac.mm:76
> + if ([item isKindOfClass:[NSDictionary class]] && item.allKeys > 0) {</span >
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.
<span class="quote">> 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());</span >
Seems weird to use CF API instead of Foundation API in this Objective-C file.
<span class="quote">> 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));
> + }</span >
Ditto.</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>