[webkit-reviews] review granted: [Bug 173478] REGRESSION (r218015) IconLoaders for already-cached resources expect to be asynchronous, no longer are. : [Attachment 313102] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Brady Eidson <beidson at apple.com>'s
request for review:
Bug 173478: REGRESSION (r218015) IconLoaders for already-cached resources
expect to be asynchronous, no longer are.
https://bugs.webkit.org/show_bug.cgi?id=173478

Attachment 313102: Patch

https://bugs.webkit.org/attachment.cgi?id=313102&action=review




--- Comment #7 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 313102
  --> https://bugs.webkit.org/attachment.cgi?id=313102
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313102&action=review

> Source/WebCore/ChangeLog:10
> +	   Being synchronous is actually better, because it's actually resolved
another issue or two.

This sentence does not read well. In particular, the phrase "it's actually
resolved".

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/IconLoadingDelegate.mm:160
> +static const char mainBytes2[] =
> +"Oh, hello there!";

Minor: It is unnecessary to use the keyword static. In C++ and Objective-C++ a
const has internal linkage. I also do not see the need to spread the assignment
across two lines (we are not using VT100 terminals these days) :). I suggest
writing the assignment on one line.

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/IconLoadingDelegate.mm:190
> +    // Load another main resource that results in the same icon being loaded
(which should come from the memory cache)

Minor: Missing period at the end of this sentence.