[Webkit-unassigned] [Bug 148388] We should also store the time information for recent searches
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Oct 9 09:13:04 PDT 2015
https://bugs.webkit.org/show_bug.cgi?id=148388
Darin Adler <darin at apple.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #262745|review? |review-
Flags| |
--- Comment #34 from Darin Adler <darin at apple.com> ---
Comment on attachment 262745
--> https://bugs.webkit.org/attachment.cgi?id=262745
Patch v5
View in context: https://bugs.webkit.org/attachment.cgi?id=262745&action=review
Many comments about possible improvements. The only one that caused me to say review- was the lack of check of the type of itemsKey; most of the other ideas for improvement are optional.
> Source/WebCore/platform/SearchPopupMenu.h:24
> #include "PopupMenu.h"
Seems like a forward declaration of PopupMenu would suffice; no need to include the entire header.
> Source/WebCore/platform/SearchPopupMenu.h:39
> virtual ~SearchPopupMenu() {}
We normally put a space in braces in cases like this.
> Source/WebCore/platform/SearchPopupMenu.h:42
> + virtual void loadRecentSearches(const AtomicString& name, Vector<RecentSearch>& searchItems) = 0;
It would be nice cleanup/modernization to change this to return a Vector instead of using an out argument.
> Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.h:3
> + * Copyright (C) 2006 Apple Inc.
> + * Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies).
These seem like the wrong copyrights for this code. While we did move some code from a file with these dates on it, I don’t think that’s accurate about who wrote the code that is now here. In particular, this header seems all new, so should be copyright 2015 Apple. Also missing All rights reserved here.
> Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.h:32
> +class SearchPopupMenuCocoa {
> +public:
> +WEBCORE_EXPORT static void saveRecentSearches(const AtomicString& name, const Vector<RecentSearch>& searchItems);
> +WEBCORE_EXPORT static void loadRecentSearches(const AtomicString& name, Vector<RecentSearch>& searchItems);
> +};
There’s no reason we need to put these functions in a namespace with the name Cocoa in it. The code is compiled only for Cocoa and doesn’t need to reiterate its platform name in the function or namespace name.
These two functions should just be free functions. No value in wrapping them in a class and very little value in namespacing them at all. If you really do want them in a namespace, then I suggest a namespace, not a class. But the names seem sufficiently unique to be at the WebCore namespace level.
The loadRecentSearches function should use a return value rather than an out argument.
> Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:3
> + * Copyright (C) 2006 Apple Inc.
> + * Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies).
Same comment about copyright date. The code here is almost all new. I don’t think it needs the old copyright on it just because it is obliquely related to the code that was in WebKit2.
> Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:40
> + return storagePath.stringByStandardizingPath.stringByResolvingSymlinksInPath;
Is it important to call stringByStandardizingPath and stringByResolvingSymlinksInPath? We’re not storing this path, just using it to write or read a file. I don’t think either of those operations are needed.
> Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:57
> +static system_clock::time_point fromNSDatetoSystemClockTime(NSDate *date)
When we write “to†functions we should list the “to†part first because that’s the return type and what’s most useful to see at the call site. Should be named toSystemClockTime. The argument type already makes it clear this takes an NSDate so I don’t think it needs to be named toSystemClockTimeFromNSDate.
> Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:64
> +static NSDate *fromSystemClockTimetoNSDate(system_clock::time_point time)
For the same reason as above, this should be named toNSDate or perhaps toNSDateFromSystemClock.
> Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:95
> + [recentSearchesPlist writeToFile:searchFieldRecentSearchesPlistPath() atomically:NO];
Same comment about secure coding.
> Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:110
> + NSArray *recentSearches = [[[recentSearchesPlist objectForKey:itemsKey] objectForKey:name] objectForKey:searchesKey];
> + if (![recentSearches isKindOfClass:[NSArray class]])
> + return;
This is missing a check of the type of the object stored under itemsKey. We need to check that is an NSDictionary.
> Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:114
> + if (![item isKindOfClass:[NSDictionary class]] || item.count != 2)
> + continue;
Should omit this item.count check. Harmless to ignore additional key/value pairs in the dictionary; perhaps we might want to add a third key later without breaking ability of older versions of WebKit reading it.
Generally speaking we don’t check for extra things in dictionaries like this. We don’t do it for the top level dictionary above, for example.
> Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:124
> + searchItems.append({ String { searchString }, fromNSDatetoSystemClockTime(date) });
I think our usual formatting puts the class name right next to the { }, so it can be:
{ String{ searchString } ...
> Source/WebCore/platform/win/SearchPopupMenuWin.cpp:96
> + // The timestamp is not used on Windows platform, but we need to conform to the new RecentSearch struct type.
> + // The time is initialized to min() because we do not know the time when the search string was created so we
> + // assume it was created long time ago.
It’s not appropriate to refer to something as “new†in a comment like this. The type won’t be new years from now when someone is reading this comment.
Comment should be more like:
// We are choosing not to use or store search times on Windows at this time, so for now it's OK to use a "distant past" time as a placeholder.
That may not be exactly right, but it’s more like the kind of comment we should write.
I also think we could just use { } instead of std::chrono::system_clock::time_point::min(); it would store the epoch instead of the minimum time, which I think might also be OK. But I guess min() is slightly better.
> Source/WebCore/platform/win/SearchPopupMenuWin.cpp:97
> + searchItems.append({ String { item }, std::chrono::system_clock::time_point::min() });
I think our usual formatting puts the class name right next to the { }, so it can be:
{ String{ item } ...
> Source/WebCore/rendering/RenderSearchField.cpp:95
> + RecentSearch recentSearch = { value, std::chrono::system_clock::now() };
> + m_recentSearches.insert(0, recentSearch);
Do we need a local variable? What happens if we just write this?
m_recentSearches.insert(0, { value, std::chrono::system_clock::now() });
A while ago someone made sure that Vector::append was properly overloaded to make things like this work. Maybe it hasn’t been done yet for Vector::insert.
> Source/WebCore/rendering/RenderSearchField.h:28
> +#include "SearchPopupMenu.h"
We can get rid of the forward declaration of the SearchPopupMenu class below now that we are including the header. Unfortunate we have to do that, but not so bad I guess.
> Source/WebKit2/UIProcess/Cocoa/WebPageProxyCocoa.mm:-43
> - if (!name) {
> - // FIXME: This should be a message check.
> - return;
> - }
Doesn’t seem good to be removing this code and comment, unless the FIXME is inaccurate.
> Source/WebKit2/UIProcess/Cocoa/WebPageProxyCocoa.mm:-63
> - if (!name) {
> - // FIXME: This should be a message check.
> - return;
> - }
Doesn’t seem good to be removing this code and comment, unless the FIXME is inaccurate.
--
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/20151009/7f67a981/attachment.html>
More information about the webkit-unassigned
mailing list