[webkit-reviews] review denied: [Bug 48032] [chromium] Added PluginLayerChromium for hardware accelerated compositing of plugins : [Attachment 71809] Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has denied Al <apatrick at chromium.org>'s
request for review:
Bug 48032: [chromium] Added PluginLayerChromium for hardware accelerated
compositing of plugins
https://bugs.webkit.org/show_bug.cgi?id=48032

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

------- Additional Comments from Kenneth Russell <kbr at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=71809&action=review

This looks good to me. I'm only marking it r- because of the copyright headers.
Please update them and re-upload the patch.

> WebCore/platform/graphics/chromium/PluginLayerChromium.cpp:28
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

It turns out that this is the wrong form of license header for contributed code
to WebKit. Even though other files in this directory are also incorrect, let's
not propagate the error further. You can find the correct header under
WebKit/LICENSE or a C++ formatted version with the Google copyright in e.g.
WebKit/chromium/tests/PODIntervalTreeTest.cpp.

> WebCore/platform/graphics/chromium/PluginLayerChromium.h:28
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

Fix copyright header here too.

> WebKit/chromium/public/WebPlugin.h:68
> +    // If the plugin instance is backed by an OpenGL, return its ID in the

by an OpenGL -> by an OpenGL context (?)

> WebKit/chromium/public/WebPluginContainer.h:57
> +    // If the plugin instance is backed by an OpenGL, return its ID in the

Same as above.

> WebKit/chromium/src/WebPluginContainerImpl.cpp:291
> +#if USE(ACCELERATED_COMPOSITING)

If the intent is to make WebPluginContainer::getBackingTextureId() pure virtual
then you need an alternate path when USE(ACCELERATED_COMPOSITING) is not
enabled. Perhaps conditionalize the method bodies.

> WebKit/chromium/src/WebPluginContainerImpl.h:92
> +#if USE(ACCELERATED_COMPOSITING)

Same comment as above about USE(ACCELERATED_COMPOSITING).