The shader in ContentLayerChromium assumes that color is in BGRA. It should honor the platform setting so that for the platform using RGBA, the color will be correct.
Created attachment 75082 [details] patch
Comment on attachment 75082 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=75082&action=review The code change looks fine (though the duplicated shader is a little unfortunate) but the OOPS line about no new tests needs to be removed and the patch regenerated or the commit queue will reject it. > WebCore/ChangeLog:8 > + No new tests. (OOPS!) This line needs to be deleted.
Created attachment 75088 [details] Patch
Comment on attachment 75088 [details] Patch Looks good.
Comment on attachment 75088 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=75088&action=review > WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:97 > + m_contentShaderProgram = createShaderProgram(m_context, vertexShaderString, SK_B32_SHIFT ? rgbaFragmentShaderString : bgraFragmentShaderString); As far as I can tell from SkColorPriv.h SK_B32_SHIFT is always non-zero, regardless of little or big endian. Did you mean to check SK_R32_SHIFT instead?
This is actually not for the big endian or little endian. In chrome/skia/config/SkUserConfig.h, it tries to set the packing to BGRA instead of the default RGBA. So if the platform uses SkUserConfig.h, it uses BGRA packing. If the platform doesn't use the SkUserConfig.h, it uses the default RGBA packing. (In reply to comment #5) > (From update of attachment 75088 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=75088&action=review > > > WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:97 > > + m_contentShaderProgram = createShaderProgram(m_context, vertexShaderString, SK_B32_SHIFT ? rgbaFragmentShaderString : bgraFragmentShaderString); > > As far as I can tell from SkColorPriv.h SK_B32_SHIFT is always non-zero, regardless of little or big endian. Did you mean to check SK_R32_SHIFT instead?
(In reply to comment #6) > This is actually not for the big endian or little endian. In chrome/skia/config/SkUserConfig.h, it tries to set the packing to BGRA instead of the default RGBA. So if the platform uses SkUserConfig.h, it uses BGRA packing. If the platform doesn't use the SkUserConfig.h, it uses the default RGBA packing. > Ah, ok. Makes sense. Thanks. > (In reply to comment #5) > > (From update of attachment 75088 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=75088&action=review > > > > > WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:97 > > > + m_contentShaderProgram = createShaderProgram(m_context, vertexShaderString, SK_B32_SHIFT ? rgbaFragmentShaderString : bgraFragmentShaderString); > > > > As far as I can tell from SkColorPriv.h SK_B32_SHIFT is always non-zero, regardless of little or big endian. Did you mean to check SK_R32_SHIFT instead?
The commit-queue encountered the following flaky tests while processing attachment 75088 [details]: fast/workers/storage/use-same-database-in-page-and-workers.html http/tests/appcache/foreign-fallback.html Please file bugs against the tests. These tests were authored by ap@webkit.org and dumi@chromium.org. The commit-queue is continuing to process your patch.
Comment on attachment 75088 [details] Patch Rejecting patch 75088 from commit-queue. Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'build', '--no-clean', '--no-update', '--build-style=both']" exit_code: 2 Last 500 characters of output: 86_64 c++ com.apple.compilers.gcc.4_2 CompileC /Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/x86_64/IndentOutdentCommand.o /Projects/CommitQueue/WebCore/editing/IndentOutdentCommand.cpp normal x86_64 c++ com.apple.compilers.gcc.4_2 CompileC /Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/x86_64/ImageDocument.o /Projects/CommitQueue/WebCore/html/ImageDocument.cpp normal x86_64 c++ com.apple.compilers.gcc.4_2 (21 failures) Full output: http://queues.webkit.org/results/6415104
That build failure looks unrelated to this change. Trying again.
I do have a style change for this CL. If it is not committed yet, can I provide another patch?
(In reply to comment #11) > I do have a style change for this CL. If it is not committed yet, can I provide another patch? Go ahead. It looks like the commit queue has not been running due to redness in the tree. I'll remove the cq+ and we can hope that the initial patch doesn't get landed; if it does, I'll deal with the merge by hand.
The commit-queue encountered the following flaky tests while processing attachment 75088 [details]: fast/workers/storage/use-same-database-in-page-and-workers.html Please file bugs against the tests. These tests were authored by dumi@chromium.org. The commit-queue is continuing to process your patch.
Created attachment 75244 [details] Patch
Comment on attachment 75244 [details] Patch Looks fine. I'm going to land this by hand.
Committed r73046: <http://trac.webkit.org/changeset/73046>
http://trac.webkit.org/changeset/73046 might have broken GTK Linux 32-bit Release The following tests are not passing: media/controls-drag-timebar.html
There was a compiler warning on Mac OS X in the original patch that Darin helpfully fixed up in http://trac.webkit.org/changeset/73053 .