Bug 50189 – [chromium] ContentLayerChromium shader should honor the platform Sk_x_SHIFT value instead of assuming BGRA color

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Grace Kloba]

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.

[Grace Kloba]

Created attachment 75082 [details]
patch

[Kenneth Russell]

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.

[Grace Kloba]

Created attachment 75088 [details]
Patch

[Kenneth Russell]

Comment on attachment 75088 [details]
Patch

Looks good.

[Vangelis Kokkevis]

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?

[Grace Kloba]

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?

[Vangelis Kokkevis]

(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?

[WebKit Commit Bot]

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 [e-mail] and [e-mail].  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

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

[Kenneth Russell]

That build failure looks unrelated to this change. Trying again.

[Grace Kloba]

I do have a style change for this CL. If it is not committed yet, can I provide another patch?

[Kenneth Russell]

(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.

[WebKit Commit Bot]

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 [e-mail].  The commit-queue is continuing to process your patch.

[Grace Kloba]

Created attachment 75244 [details]
Patch

[Kenneth Russell]

Comment on attachment 75244 [details]
Patch

Looks fine. I'm going to land this by hand.

[Kenneth Russell]

Committed r73046: <http://trac.webkit.org/changeset/73046>

[WebKit Review Bot]

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

[Kenneth Russell]

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 .