[Webkit-unassigned] [Bug 148388] We should also store the time information for recent searches

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=148388

--- Comment #35 from Zach Li <zacharyli323 at gmail.com> ---
(In reply to comment #34)
> Comment on attachment 262745 [details]
> 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.

I will change it to forward declaration, and this change will result me in including
the header in RenderSearchField.cpp file now.

> 
> > Source/WebCore/platform/SearchPopupMenu.h:39
> >      virtual ~SearchPopupMenu() {}
> 
> We normally put a space in braces in cases like this.

Will fix since it is a trivial change.

> 
> > 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.

I think I will leave this improvement to later because changing this function declaration means changes in a lot of different places and it is not what the patch primarily addresses.

> 
> > 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.

I will use the 2015 copyright statement.

> 
> > 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.

Originally I was afraid of function name collision, but I guess it is unique enough to
be at the WebCore namespace level.

> 
> The loadRecentSearches function should use a return value rather than an out
> argument.

I will change that.

> 
> > 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.

You are right, looking at the documentation, these are not necessary. The reason why I had them before was that we used them in WebProcessPoolCocoa.mm

> 
> > 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.

I will use toSystemClockTime.

> 
> > 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.

I will use toNSDateFromSystemClock.

> 
> > Source/WebCore/platform/cocoa/SearchPopupMenuCocoa.mm:95
> > +    [recentSearchesPlist writeToFile:searchFieldRecentSearchesPlistPath() atomically:NO];
> 
> Same comment about secure coding.

Somehow, I couldn't find the comments 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.

You are absolutely right. Plus I need to check the type of the object stored under name is also 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.

I will revise this.

> 
> 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 } ...

I had { String{ searchString } ... before, but when I ran check-webkit-style, I got this error: Missing space before {. I searched in the WebKit repository and there are no examples of using either String{ or String { to do the cast, so I will leave it as it is for now unless you feel strongly about using the style above, I will change it to your suggested style and file a bug against check-webkit-style.

> 
> > 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.

Totally missed this...

> 
> 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 will use the comments above. It is more clear and concise than what I can come up with.

> 
> 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.

Okay, I will stick with min(). My design is, when we support storing search times on Windows, these search entries in the old format will be delete when users select to delete all history.

> 
> > 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 } ...

Same comment as above.

> 
> > 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.

I don't think this is done for Vector::insert, I used m_recentSearches.insert(0, { value, std::chrono::system_clock::now() }) before, but it gave me "No matching member
function for call to '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.

Will fix.

> 
> > 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.

Will restore it.

> 
> > 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.

Ditto.

-- 
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/49d90507/attachment-0001.html>