[webkit-reviews] review granted: [Bug 121801] Make WebHistory more type safe : [Attachment 212401] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has granted Patrick R. Gansterer
<paroga at paroga.com>'s request for review:
Bug 121801: Make WebHistory more type safe
https://bugs.webkit.org/show_bug.cgi?id=121801

Attachment 212401: Patch
https://bugs.webkit.org/attachment.cgi?id=212401&action=review

------- Additional Comments from Brent Fulgham <bfulgham at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=212401&action=review


Looks good.  I did notice a couple of minor things that I missed last time. 
The only one I really care about is the naming of the last visited time (I
think this should stay as "lastVisitedTime", not be written as "entryTime".

You can fix this while landing, or you can upload a final patch and I will r+
that.

> Source/WebKit/win/WebHistory.cpp:590
> +    DATE entryTime;

I just noticed that this should be "lastVisitedTime".

> Source/WebKit/win/WebHistory.cpp:595
>	   return E_FAIL;

This isn't your doing, but I wonder if attempting to remove an entry that is
already not in the entry queue should be considered failure.  Seems like a safe
"success" case to me.

> Source/WebKit/win/WebHistory.cpp:601
> +	   if (entriesForDate.at(i) == entry)

We don't tend to use the "at()" method, but this is okay.

> Source/WebKit/win/WebHistory.cpp:625
> +	   Vector<COMPtr<IWebHistoryItem> > entries(1);

You don't need the space here "> >" can be ">>" under C++11. :-)

> Source/WebKit/win/WebHistory.cpp:664
> +	   if
(FAILED(entriesForDate.at(mid)->lastVisitedTimeInterval(&itemTime)))

... another "at()".

> Source/WebKit/win/WebHistory.h:120
> +    typedef HashMap<DateKey, Vector<COMPtr<IWebHistoryItem> > >
DateToEntriesMap;

You can write this as "...IWebHistoryItem>>>"