Bug 109380 – [Chromium] FilterTypeSaturatingBrightness enum

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Florin Malita]

Add WebFilterOperation FilterTypeSaturatingBrightness enum to support internal clients relying on the current saturating behavior.

[Florin Malita]

Created attachment 187485 [details]
Patch

This requires https://codereview.chromium.org/12209071/ in order to not break the build.

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

Attachment 187485 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platform/chromium/public/WebFilterOperation.h']" exit_code: 1
Source/Platform/chromium/public/WebFilterOperation.h:69:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/Platform/chromium/public/WebFilterOperation.h:70:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/Platform/chromium/public/WebFilterOperation.h:126:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/Platform/chromium/public/WebFilterOperation.h:127:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 4 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Florin Malita]

This is a pre-requisite for https://codereview.chromium.org/12033075/.

[James Robinson]

Comment on attachment 187485 [details]
Patch

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

> Source/Platform/chromium/public/WebFilterOperation.h:52
> +        FilterTypeSaturatingBrightness, // Legacy brightness implementation used by internal clients

I'm not sure this comment adds any value in isolation. Perhaps specify that it's not one in CSS/SVG if that's the distinguishing factor?

[Florin Malita]

Comment on attachment 187485 [details]
Patch

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

>> Source/Platform/chromium/public/WebFilterOperation.h:52
>> +        FilterTypeSaturatingBrightness, // Legacy brightness implementation used by internal clients
> 
> I'm not sure this comment adds any value in isolation. Perhaps specify that it's not one in CSS/SVG if that's the distinguishing factor?

Sounds good, thanks!

[Florin Malita]

Committed r142496: <http://trac.webkit.org/changeset/142496>

[James Robinson]

(In reply to comment #7)
> Committed r142496: <http://trac.webkit.org/changeset/142496>

This breaks the build:

FAILED: ../../Source/WebKit/chromium/third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF obj/source/webkit/chromium/cc/cc.render_surface_filters.o.d -DCHROMIUM_BUILD -DUSE_LIBJPEG_TURBO=1 -DENABLE_ONE_CLICK_SIGNIN -DENABLE_REMOTING=1 -DENABLE_WEBRTC=1 -DENABLE_PEPPER_THREADING -DENABLE_CONFIGURATION_POLICY -DENABLE_INPUT_SPEECH -DENABLE_NOTIFICATIONS -DENABLE_HIDPI=1 -DENABLE_GPU=1 -DENABLE_EGLIMAGE=1 -DUSE_SKIA=1 -DENABLE_TASK_MANAGER=1 -DENABLE_EXTENSIONS=1 -DENABLE_PLUGIN_INSTALLATION=1 -DENABLE_PLUGINS=1 -DENABLE_SESSION_SERVICE=1 -DENABLE_THEMES=1 -DENABLE_BACKGROUND=1 -DENABLE_AUTOMATION=1 -DENABLE_GOOGLE_NOW=1 -DENABLE_LANGUAGE_DETECTION=1 -DENABLE_PRINTING=1 -DENABLE_CAPTIVE_PORTAL_DETECTION=1 -DENABLE_APP_LIST=1 -DENABLE_MANAGED_USERS=1 -DCC_IMPLEMENTATION=1 -DSK_BUILD_NO_IMAGE_ENCODE -DSK_DEFERRED_CANVAS_USES_GPIPE=1 '-DGR_GL_CUSTOM_SETUP_HEADER="GrGLConfig_chrome.h"' -DGR_AGGRESSIVE_SHADER_OPTS=1 -DSK_ENABLE_INST_COUNT=0 -DSK_USE_POSIX_THREADS -DU_USING_ICU_NAMESPACE=0 -DU_STATIC_IMPLEMENTATION -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -I../../Source/WebKit/chromium/third_party/icu/public/common -I../../Source/WebKit/chromium/third_party/icu/public/i18n -I../../Source/WebKit/chromium/third_party/khronos -I../../Source/WebKit/chromium/gpu -I../../Source/WebKit/chromium -I../../Source/WebKit/chromium/skia/config -I../../Source/WebKit/chromium/third_party/skia/src/core -I../../Source/WebKit/chromium/third_party/skia/include/config -I../../Source/WebKit/chromium/third_party/skia/include/core -I../../Source/WebKit/chromium/third_party/skia/include/effects -I../../Source/WebKit/chromium/third_party/skia/include/pdf -I../../Source/WebKit/chromium/third_party/skia/include/gpu -I../../Source/WebKit/chromium/third_party/skia/include/gpu/gl -I../../Source/WebKit/chromium/third_party/skia/include/pipe -I../../Source/WebKit/chromium/third_party/skia/include/ports -I../../Source/WebKit/chromium/third_party/skia/include/utils -I../../Source/WebKit/chromium/skia/ext -I../../Source/WebKit/chromium/third_party/skia/include/utils/mac -Igen/ui/gl -I../../Source/WebKit/chromium/third_party/mesa/MesaLib/include -I../../Source/Platform/chromium -Igen/webkit -I../../Source/Platform/chromium -Igen/webcore_headers -I../../Source/WebKit/chromium/third_party/npapi -I../../Source/WebKit/chromium/third_party/npapi/bindings -I../../Source/WebKit/chromium/v8/include -isysroot /Developer/SDKs/MacOSX10.6.sdk -O3 -gdwarf-2 -fvisibility=hidden -Werror -Wnewline-eof -mmacosx-version-min=10.6 -arch i386 -Wendif-labels -Wno-unused-parameter -Wno-missing-field-initializers -Wheader-hygiene -Wno-c++11-narrowing -Wno-reserved-user-defined-literal -Wno-char-subscripts -Wno-unused-function -Wno-covered-switch-default -fno-rtti -fno-exceptions -fvisibility-inlines-hidden -fno-threadsafe-statics -fcolor-diagnostics -fno-strict-aliasing -std=gnu++11  -c ../../Source/WebKit/chromium/cc/render_surface_filters.cc -o obj/source/webkit/chromium/cc/cc.render_surface_filters.o
../../Source/WebKit/chromium/cc/render_surface_filters.cc:348:17: error: enumeration value 'FilterTypeSaturatingBrightness' not handled in switch [-Werror,-Wswitch]
        switch (op.type()) {
                ^
../../Source/WebKit/chromium/cc/render_surface_filters.cc:384:17: error: enumeration value 'FilterTypeSaturatingBrightness' not handled in switch [-Werror,-Wswitch]
        switch (op.type()) {
                ^
2 errors generated.
ninja: build stopped: subcommand failed.

http://build.webkit.org/builders/Chromium%20Mac%20Release/builds/55540/steps/compile-webkit/logs/stdio

I think we'll have to add a default: to the chromium side switch statement.

[Stephen White]

(In reply to comment #8)
> (In reply to comment #7)
> > Committed r142496: <http://trac.webkit.org/changeset/142496>
> 
> This breaks the build:
> 
> FAILED: ../../Source/WebKit/chromium/third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF obj/source/webkit/chromium/cc/cc.render_surface_filters.o.d -DCHROMIUM_BUILD -DUSE_LIBJPEG_TURBO=1 -DENABLE_ONE_CLICK_SIGNIN -DENABLE_REMOTING=1 -DENABLE_WEBRTC=1 -DENABLE_PEPPER_THREADING -DENABLE_CONFIGURATION_POLICY -DENABLE_INPUT_SPEECH -DENABLE_NOTIFICATIONS -DENABLE_HIDPI=1 -DENABLE_GPU=1 -DENABLE_EGLIMAGE=1 -DUSE_SKIA=1 -DENABLE_TASK_MANAGER=1 -DENABLE_EXTENSIONS=1 -DENABLE_PLUGIN_INSTALLATION=1 -DENABLE_PLUGINS=1 -DENABLE_SESSION_SERVICE=1 -DENABLE_THEMES=1 -DENABLE_BACKGROUND=1 -DENABLE_AUTOMATION=1 -DENABLE_GOOGLE_NOW=1 -DENABLE_LANGUAGE_DETECTION=1 -DENABLE_PRINTING=1 -DENABLE_CAPTIVE_PORTAL_DETECTION=1 -DENABLE_APP_LIST=1 -DENABLE_MANAGED_USERS=1 -DCC_IMPLEMENTATION=1 -DSK_BUILD_NO_IMAGE_ENCODE -DSK_DEFERRED_CANVAS_USES_GPIPE=1 '-DGR_GL_CUSTOM_SETUP_HEADER="GrGLConfig_chrome.h"' -DGR_AGGRESSIVE_SHADER_OPTS=1 -DSK_ENABLE_INST_COUNT=0 -DSK_USE_POSIX_THREADS -DU_USING_ICU_NAMESPACE=0 -DU_STATIC_IMPLEMENTATION -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -I../../Source/WebKit/chromium/third_party/icu/public/common -I../../Source/WebKit/chromium/third_party/icu/public/i18n -I../../Source/WebKit/chromium/third_party/khronos -I../../Source/WebKit/chromium/gpu -I../../Source/WebKit/chromium -I../../Source/WebKit/chromium/skia/config -I../../Source/WebKit/chromium/third_party/skia/src/core -I../../Source/WebKit/chromium/third_party/skia/include/config -I../../Source/WebKit/chromium/third_party/skia/include/core -I../../Source/WebKit/chromium/third_party/skia/include/effects -I../../Source/WebKit/chromium/third_party/skia/include/pdf -I../../Source/WebKit/chromium/third_party/skia/include/gpu -I../../Source/WebKit/chromium/third_party/skia/include/gpu/gl -I../../Source/WebKit/chromium/third_party/skia/include/pipe -I../../Source/WebKit/chromium/third_party/skia/include/ports -I../../Source/WebKit/chromium/third_party/skia/include/utils -I../../Source/WebKit/chromium/skia/ext -I../../Source/WebKit/chromium/third_party/skia/include/utils/mac -Igen/ui/gl -I../../Source/WebKit/chromium/third_party/mesa/MesaLib/include -I../../Source/Platform/chromium -Igen/webkit -I../../Source/Platform/chromium -Igen/webcore_headers -I../../Source/WebKit/chromium/third_party/npapi -I../../Source/WebKit/chromium/third_party/npapi/bindings -I../../Source/WebKit/chromium/v8/include -isysroot /Developer/SDKs/MacOSX10.6.sdk -O3 -gdwarf-2 -fvisibility=hidden -Werror -Wnewline-eof -mmacosx-version-min=10.6 -arch i386 -Wendif-labels -Wno-unused-parameter -Wno-missing-field-initializers -Wheader-hygiene -Wno-c++11-narrowing -Wno-reserved-user-defined-literal -Wno-char-subscripts -Wno-unused-function -Wno-covered-switch-default -fno-rtti -fno-exceptions -fvisibility-inlines-hidden -fno-threadsafe-statics -fcolor-diagnostics -fno-strict-aliasing -std=gnu++11  -c ../../Source/WebKit/chromium/cc/render_surface_filters.cc -o obj/source/webkit/chromium/cc/cc.render_surface_filters.o
> ../../Source/WebKit/chromium/cc/render_surface_filters.cc:348:17: error: enumeration value 'FilterTypeSaturatingBrightness' not handled in switch [-Werror,-Wswitch]
>         switch (op.type()) {
>                 ^
> ../../Source/WebKit/chromium/cc/render_surface_filters.cc:384:17: error: enumeration value 'FilterTypeSaturatingBrightness' not handled in switch [-Werror,-Wswitch]
>         switch (op.type()) {
>                 ^
> 2 errors generated.
> ninja: build stopped: subcommand failed.
> 
> http://build.webkit.org/builders/Chromium%20Mac%20Release/builds/55540/steps/compile-webkit/logs/stdio
> 
> I think we'll have to add a default: to the chromium side switch statement.

I think Florin already did that (will check).  Was this on the build.webkit.org builders?  Maybe they just need a Chromium DEPS roll.

[Stephen White]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > Committed r142496: <http://trac.webkit.org/changeset/142496>
> > 
> > This breaks the build:
> > 
> > FAILED: ../../Source/WebKit/chromium/third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF obj/source/webkit/chromium/cc/cc.render_surface_filters.o.d -DCHROMIUM_BUILD -DUSE_LIBJPEG_TURBO=1 -DENABLE_ONE_CLICK_SIGNIN -DENABLE_REMOTING=1 -DENABLE_WEBRTC=1 -DENABLE_PEPPER_THREADING -DENABLE_CONFIGURATION_POLICY -DENABLE_INPUT_SPEECH -DENABLE_NOTIFICATIONS -DENABLE_HIDPI=1 -DENABLE_GPU=1 -DENABLE_EGLIMAGE=1 -DUSE_SKIA=1 -DENABLE_TASK_MANAGER=1 -DENABLE_EXTENSIONS=1 -DENABLE_PLUGIN_INSTALLATION=1 -DENABLE_PLUGINS=1 -DENABLE_SESSION_SERVICE=1 -DENABLE_THEMES=1 -DENABLE_BACKGROUND=1 -DENABLE_AUTOMATION=1 -DENABLE_GOOGLE_NOW=1 -DENABLE_LANGUAGE_DETECTION=1 -DENABLE_PRINTING=1 -DENABLE_CAPTIVE_PORTAL_DETECTION=1 -DENABLE_APP_LIST=1 -DENABLE_MANAGED_USERS=1 -DCC_IMPLEMENTATION=1 -DSK_BUILD_NO_IMAGE_ENCODE -DSK_DEFERRED_CANVAS_USES_GPIPE=1 '-DGR_GL_CUSTOM_SETUP_HEADER="GrGLConfig_chrome.h"' -DGR_AGGRESSIVE_SHADER_OPTS=1 -DSK_ENABLE_INST_COUNT=0 -DSK_USE_POSIX_THREADS -DU_USING_ICU_NAMESPACE=0 -DU_STATIC_IMPLEMENTATION -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -I../../Source/WebKit/chromium/third_party/icu/public/common -I../../Source/WebKit/chromium/third_party/icu/public/i18n -I../../Source/WebKit/chromium/third_party/khronos -I../../Source/WebKit/chromium/gpu -I../../Source/WebKit/chromium -I../../Source/WebKit/chromium/skia/config -I../../Source/WebKit/chromium/third_party/skia/src/core -I../../Source/WebKit/chromium/third_party/skia/include/config -I../../Source/WebKit/chromium/third_party/skia/include/core -I../../Source/WebKit/chromium/third_party/skia/include/effects -I../../Source/WebKit/chromium/third_party/skia/include/pdf -I../../Source/WebKit/chromium/third_party/skia/include/gpu -I../../Source/WebKit/chromium/third_party/skia/include/gpu/gl -I../../Source/WebKit/chromium/third_party/skia/include/pipe -I../../Source/WebKit/chromium/third_party/skia/include/ports -I../../Source/WebKit/chromium/third_party/skia/include/utils -I../../Source/WebKit/chromium/skia/ext -I../../Source/WebKit/chromium/third_party/skia/include/utils/mac -Igen/ui/gl -I../../Source/WebKit/chromium/third_party/mesa/MesaLib/include -I../../Source/Platform/chromium -Igen/webkit -I../../Source/Platform/chromium -Igen/webcore_headers -I../../Source/WebKit/chromium/third_party/npapi -I../../Source/WebKit/chromium/third_party/npapi/bindings -I../../Source/WebKit/chromium/v8/include -isysroot /Developer/SDKs/MacOSX10.6.sdk -O3 -gdwarf-2 -fvisibility=hidden -Werror -Wnewline-eof -mmacosx-version-min=10.6 -arch i386 -Wendif-labels -Wno-unused-parameter -Wno-missing-field-initializers -Wheader-hygiene -Wno-c++11-narrowing -Wno-reserved-user-defined-literal -Wno-char-subscripts -Wno-unused-function -Wno-covered-switch-default -fno-rtti -fno-exceptions -fvisibility-inlines-hidden -fno-threadsafe-statics -fcolor-diagnostics -fno-strict-aliasing -std=gnu++11  -c ../../Source/WebKit/chromium/cc/render_surface_filters.cc -o obj/source/webkit/chromium/cc/cc.render_surface_filters.o
> > ../../Source/WebKit/chromium/cc/render_surface_filters.cc:348:17: error: enumeration value 'FilterTypeSaturatingBrightness' not handled in switch [-Werror,-Wswitch]
> >         switch (op.type()) {
> >                 ^
> > ../../Source/WebKit/chromium/cc/render_surface_filters.cc:384:17: error: enumeration value 'FilterTypeSaturatingBrightness' not handled in switch [-Werror,-Wswitch]
> >         switch (op.type()) {
> >                 ^
> > 2 errors generated.
> > ninja: build stopped: subcommand failed.
> > 
> > http://build.webkit.org/builders/Chromium%20Mac%20Release/builds/55540/steps/compile-webkit/logs/stdio
> > 
> > I think we'll have to add a default: to the chromium side switch statement.
> 
> I think Florin already did that (will check).  Was this on the build.webkit.org builders?  Maybe they just need a Chromium DEPS roll.

Never mind, that was in cc_messages.cc.  Looks like we need some in render_surface_filters.cc as well.

[Florin Malita]

(In reply to comment #10)
> Never mind, that was in cc_messages.cc.  Looks like we need some in render_surface_filters.cc as well.

Yes, missed that somehow. Already landed http://build.chromium.org/p/chromium.webkit/changes/102436, rolling into WK right now.