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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Sep 23 15:31:19 PDT 2013

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

Attachment 212387: Patch

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

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

> 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

More information about the webkit-reviews mailing list