[webkit-reviews] review denied: [Bug 78139] [chromium] Resolve GraphicsContext3D use and re-enable filters in the threaded compositor : [Attachment 133935] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Stephen White
<senorblanco at chromium.org>'s request for review:
Bug 78139: [chromium] Resolve GraphicsContext3D use and re-enable filters in
the threaded compositor
https://bugs.webkit.org/show_bug.cgi?id=78139

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

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=133935&action=review


I think the reference filter clone is wrong. Rest looks fine.

> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:466
> +	   for (unsigned i = 0; i < m_filters.size(); ++i)

size_t would be slightly better (more to be warning-clean than to handle >4B
filters)

> Source/WebCore/platform/graphics/filters/CustomFilterOperation.h:67
> +	   ASSERT(false);

ASSERT_NOT_REACHED(). a comment explaining this condition wouldn't hurt.

would it be better to filer these node types out in our compositor code? we
don't have this one #ifdef'd in for the chromium build at least, but people may
add other filter types that we can't clone naively in the future that aren't
#ifdef guarded

> Source/WebCore/platform/graphics/filters/FilterOperation.h:157
> +	   return adoptRef(new ReferenceFilterOperation(m_reference, m_type));

are you sure this is safe? it's copying an AtomicString, which is typically a
fairly thread-hostile thing. For instance the common atoms (like nullAtom,
emptyAtom, etc) are stored in TLS and only valid to access from the main
thread. things like "is this string the empty string?" will fail from non-main
threads.

do we have any thread-side support for reference filters? should this be
filtered out on the compositor side, perhaps?

> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:80
> +    GET, CREATE

WebKit style for enums is to use normal TypeNameStyle, not MACRO_STYLE