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

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has denied 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 212387: Patch
https://bugs.webkit.org/attachment.cgi?id=212387&action=review

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


I think the general idea of this change is good, but I don't agree with
removing some of these methods, as they may be used by other WebKit clients.

> Source/WebKit/win/WebHistory.cpp:322
> +    DateToEntriesMap::iterator found =
m_entriesByDate.find(dateKey(calendarDate));

How about 'auto found = ...'?

> Source/WebKit/win/WebHistory.cpp:343
> +	   entriesForDate.at(i).copyRefTo(&items[i]);

Why use the "at(i)" syntax here? Is there some benefit over the square-bracket
operator?

> Source/WebKit/win/WebHistory.h:-141
> -    HRESULT insertItem(IWebHistoryItem* entry, DateKey);

We don't want to get rid of the insertItem method.  I think
"addItemToDateCaches" should be implemented in terms of insertItem.

> Source/WebKit/win/WebHistory.h:-143
> -    bool findKey(DateKey*, CFAbsoluteTime forDay);

Why don't we have 'bool findKey(DateKey*, DATE)'?  Why don't we want to keep
this?