[webkit-reviews] review denied: [Bug 43101] Using glMapTexSubImage2d in VideoLayerChromium : [Attachment 62849] Patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Victoria <vrk at google.com>'s request
for review:
Bug 43101: Using glMapTexSubImage2d in VideoLayerChromium
https://bugs.webkit.org/show_bug.cgi?id=43101

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

------- Additional Comments from David Levin <levin at chromium.org>
Just a few very trivial details that it would be nice to clean up.

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> +2010-07-27  Victoria Kirst  <vrk at google.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Added logic to use glMapTexSubImage2D to write video layer to GPU
> +	   texture. Also fixes CPU usage problem from previous patch.
> +	   https://bugs.webkit.org/show_bug.cgi?id=43101

Please explain why there are no tests added.

"No change in user visible functionality (since it isn't turned on), so no new
tests."

When this does become enabled either there needs to be tests or an indication
of what tests cover the functionality.


> diff --git a/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp
b/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp
>  
> +#include "LayerRendererChromium.h"
> +#if PLATFORM(SKIA)
> +#include "NativeImageSkia.h"
> +#include "PlatformContextSkia.h"
> +#endif

#if'ed includes should come after all other includes.

> +#include "RenderLayerBacking.h"
> +#include "skia/ext/platform_canvas.h"
> +
> +#include <GLES2/gl2.h>
> +
> +#define GL_GLEXT_PROTOTYPES
> +#include <GLES2/gl2ext.h>
> +

> +    // If the texture needs to be reallocated then we must redraw the entire


Please add a "," before then.


> +	   // FIXME: Does this take us down a very slow text rendering path?
> +	   // FIXME: why is this is a windows-only call ?

*W*hy (and the space before the ? is odd.)


> +    // If the texture id or size changed since last time then we need to
tell GL
Please add a "," before then.