[webkit-reviews] review denied: [Bug 119389] Make WebHistory more type safe : [Attachment 207912] 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 119389: Make WebHistory more type safe
https://bugs.webkit.org/show_bug.cgi?id=119389

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

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


Looks good!  Can you please use 'auto'?

> Source/WebKit/win/WebHistory.cpp:246
> +    for (URLToEntriesMap::const_iterator it = m_entriesByURL.begin(); it !=
m_entriesByURL.end(); ++it)

Can this be for (auto it = ...)?

> Source/WebKit/win/WebHistory.cpp:345
> +    for (URLToEntriesMap::const_iterator it = m_entriesByURL.begin(); it !=
m_entriesByURL.end(); ++i, ++it) {

auto!

>> Source/WebKit/win/WebHistory.cpp:532
>> +	/* [in] */ BSTR url,
> 
> Weird number of spaces at line-start.  Are you using a 4-space indent? 
[whitespace/indent] [3]

I've been changing these to be one-line in "WebKit Style" when I'm changing the
signature.  Do we really need the STDMETHODCALLTYPE here? I thought that if it
was in the header file that was sufficient.

> Source/WebKit/win/WebHistory.cpp:539
> +    URLToEntriesMap::iterator it = m_entriesByURL.find(url);

auto it = ...

> Source/WebKit/win/WebHistory.cpp:549
> +    URLToEntriesMap::iterator it = m_entriesByURL.find(urlString);

auto ...

> Source/WebKit/win/WebHistory.cpp:757
> +    for (URLToEntriesMap::const_iterator it = m_entriesByURL.begin(); it !=
m_entriesByURL.end(); ++it) {

auto...