[webkit-reviews] review denied: [Bug 135634] URLs sometimes remain in IconDatabase files after history is cleared. : [Attachment 236076] Patch

[bugzilla-daemon at webkit.org]

Brady Eidson <beidson at apple.com> has denied Gordon Sheridan
<gordon_sheridan at apple.com>'s request for review:
Bug 135634: URLs sometimes remain in IconDatabase files after history is
cleared.
https://bugs.webkit.org/show_bug.cgi?id=135634

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

------- Additional Comments from Brady Eidson <beidson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=236076&action=review


In general, looks good.

> I chose to add a general purpose call to WebCore::Page, to execute a function
or closure for each Page.

I think we should be more narrow and add a function specific to this.

> Source/WebCore/ChangeLog:8
> +	   No new tests (OOPS!).

I think we need an API test for this.

> Source/WebCore/page/Page.cpp:278
> +    for (auto it = allPages->begin(), end = allPages->end(); it != end;
++it) {
> +	   if (callBack(*it))
> +	       return;
> +    }

This should be a C++11 for loop.

for (auto& page : *allPages) { }