[webkit-reviews] review granted: [Bug 185272] [WebGL] WEBGL_compressed_texture_astc support : [Attachment 339698] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Justin Fan <justin_fan at apple.com>'s
request for review:
Bug 185272: [WebGL] WEBGL_compressed_texture_astc support
https://bugs.webkit.org/show_bug.cgi?id=185272

Attachment 339698: Patch

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




--- Comment #13 from Dean Jackson <dino at apple.com> ---
Comment on attachment 339698
  --> https://bugs.webkit.org/attachment.cgi?id=339698
Patch

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

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:4109
> +		D0A20D572092A0A700E0C259 /* WebGLCompressedTextureASTC.h in
Headers */ = {isa = PBXBuildFile; fileRef = D0A20D542092A0A600E0C259 /*
WebGLCompressedTextureASTC.h */; };
> +		D0A20D582092A0A700E0C259 /* WebGLCompressedTextureASTC.cpp in
Sources */ = {isa = PBXBuildFile; fileRef = D0A20D562092A0A600E0C259 /*
WebGLCompressedTextureASTC.cpp */; };

Technically, you could just add this to Sources.txt rather than to the Xcode
project. I plan to move all the WebGL stuff into Sources.txt at some point.

> Source/WebCore/html/canvas/WebGLCompressedTextureASTC.cpp:28
> +#include "config.h"
> +#if ENABLE(WEBGL)
> +#include "WebGLCompressedTextureASTC.h"

Nit: Should be:

#include "config.h"

#include "WebGLCompressedTextureASTC.h"

#if ENABLE(WEBGL)

> Source/WebCore/html/canvas/WebGLCompressedTextureASTC.cpp:89
> +    {
> +	   Vector<String> result;
> +	   
> +	   if (m_isHDRSupported)
> +	       result.append(ASCIILiteral { "hdr" });
> +	   if (m_isLDRSupported)
> +	       result.append(ASCIILiteral { "ldr" });
> +	   
> +	   return result;
> +    }

Nit: de-indent

> Source/WebCore/html/canvas/WebGLCompressedTextureASTC.cpp:95
> +    return
RuntimeEnabledFeatures::sharedFeatures().webGLCompressedTextureASTCSupportEnabl
ed()
> +	   &&
(context.graphicsContext3D()->getExtensions().supports(ASCIILiteral {
"GL_KHR_texture_compression_astc_hdr" })
> +	       ||
context.graphicsContext3D()->getExtensions().supports(ASCIILiteral {
"GL_KHR_texture_compression_astc_ldr" }));

Nit: We don't indent that third line.

> Source/WebCore/html/canvas/WebGLCompressedTextureASTC.h:41
> +    static bool supported(const WebGLRenderingContextBase&);
> +private:

Nit: Empty line here.

> LayoutTests/ChangeLog:9
> +	   * fast/canvas/webgl/resources/js-test-post.js: Added.

It's weird that we didn't already have these files. Are you sure they are not
there?

> LayoutTests/ChangeLog:28
> +	   * fast/canvas/webgl/resources/js-test-pre.js: Added.
> +	   (window.console.log):
> +	   (window.console.error):
> +	   (initNonKhronosFramework):
> +	   (this.initTestingHarness):
> +	   (getUrlOptions):
> +	   (typeof.quietMode.string_appeared_here.quietMode):
> +	   (nonKhronosFrameworkNotifyDone):
> +	   (reportTestResultsToHarness):
> +	   (reportSkippedTestResultsToHarness):
> +	   (notifyFinishedToHarness):
> +	   (bufferedLogToConsole):
> +	   (_flushBufferedLogsToConsole):
> +	   (enableJSTestPreVerboseLogging):
> +	   (description):
> +	   (_addSpan):
> +	   (debug):
> +	   (escapeHTML):
> +	   (TestFailedException):

I usually remove all the function names in the ChangeLog, and just leave the
line saying you added a file (for JS files).