<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#c31">Comment # 31</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:zacharyli323@gmail.com" title="Zach Li <zacharyli323@gmail.com>"> <span class="fn">Zach Li</span></a>
</span></b>
<pre>(In reply to <a href="show_bug.cgi?id=148388#c29">comment #29</a>)
<span class="quote">> (In reply to <a href="show_bug.cgi?id=148388#c28">comment #28</a>)
> > 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)
>
> 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.</span >
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.
<span class="quote">>
> 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.</span >
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.
<span class="quote">>
> 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.</span >
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.
<span class="quote">>
> 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!</span >
(In reply to <a href="show_bug.cgi?id=148388#c28">comment #28</a>)
<span class="quote">> 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)
>
> > 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.</span ></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>