[webkit-reviews] review denied: [Bug 27777] ImageSourceCG makes bad data refs (race condition causes blank images) : [Attachment 43479] Revised patch.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Nov 19 09:44:20 PST 2009


Darin Adler <darin at apple.com> has denied Avi Drissman <avi at drissman.com>'s
request for review:
Bug 27777: ImageSourceCG makes bad data refs (race condition causes blank
images)
https://bugs.webkit.org/show_bug.cgi?id=27777

Attachment 43479: Revised patch.
https://bugs.webkit.org/attachment.cgi?id=43479&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
Looks great. Two style things that should be fixed before landing this.

> +    size_t source_size = sharedBuffer->size();

WebKit style uses sourceSize, not source_size, for a local variable like this.

> +    size_t amount = min<size_t>(count, source_size - position);

I presume you tried this without <size_t>. If it worked that way we would want
to leave it out.

> +    const CGDataProviderDirectCallbacks providerCallbacks = { 0, 0, 0,
sharedBufferGetBytesAtPosition, sharedBufferRelease };

The const on this line isn't helpful. In general, there are many local
variables that could be marked const, but as a general rule we do not do it
since the extra verbiage doesn't help clarify things.

We could decide to go the other way and start putting in const for local
variables whenever possible, but I'd prefer not to do it on a case by case
basis.

> +#if !PLATFORM(MAC)
> +// From ImageSourceCG
> +extern size_t sharedBufferGetBytesAtPosition(void* info, void* buffer, off_t
position, size_t count);
> +#endif

This needs to go into a header file, ImageSourceCG.h. We don't put externs into
other files like this. I believe that you can even add a header file without
modifying any of the project files, although it's customary to reference it in
the Mac and Windows projects.


More information about the webkit-reviews mailing list