[Webkit-unassigned] [Bug 148388] We should also store the time information for recent searches
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Oct 1 12:36:42 PDT 2015
https://bugs.webkit.org/show_bug.cgi?id=148388
--- Comment #29 from Zach Li <zacharyli323 at gmail.com> ---
(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!
--
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/03ad7a49/attachment-0001.html>
More information about the webkit-unassigned
mailing list