chromium/mac: let image layer data row order match skia
Created attachment 66915 [details] Patch
With this, images are no longer upside down when using the compositor with chrome mac. This is http://crbug.com/54834
What layout test does this fix? If none, please add one.
Comment on attachment 66915 [details] Patch See comment in bug. (If this fixes an existing layout test, add the info to the bug.)
This happens on a lot of images (every image with a css transform i believe), so I'm pretty sure this is covered by existing layout tests. The problem is that we don't run layout tests with compositing enabled yet, which is why this wasn't caught by tests.
(In reply to comment #5) > This happens on a lot of images (every image with a css transform i believe), so I'm pretty sure this is covered by existing layout tests. The problem is that we don't run layout tests with compositing enabled yet, which is why this wasn't caught by tests. Pls say something simple in the ChangeLog (and I'll gladly r+ it). Something like "Test: This will be caught by layout tests that have images with a css transform when the compositor is turned on for layout tests."
There is already Core Graphics-specific logic in ContentLayerChromium (see ContentLayerChromium::SharedValues::SharedValues(), fragmentShaderString) which performs a vertical flip. The origin difference between CG and Skia also affects scrolling; see LayerRendererChromium::drawLayers. Have these been taken into consideration with this patch?
Created attachment 66934 [details] Patch
(In reply to comment #7) > There is already Core Graphics-specific logic in ContentLayerChromium (see ContentLayerChromium::SharedValues::SharedValues(), fragmentShaderString) which performs a vertical flip. The origin difference between CG and Skia also affects scrolling; see LayerRendererChromium::drawLayers. Have these been taken into consideration with this patch? It hasn't. I just noticed that stuff looked wrong, and with this patch it looks right. After looking at things, it seems to be much cleaner to have the in-memory representation for skia and cg be identical and then remove all these tweaks (like we do in the software path). I've uploaded a revised patch that does this.
Comment on attachment 66934 [details] Patch Please wait with reviewing this in detail.
Comment on attachment 66934 [details] Patch I'm not sure why this is not necessary for ImageLayerChromium too, but ImageCG.cpp calls CGContextScaleCTM(context, 1, -1) as well, so I suppose CGImageRefs are already flipped for some reason. This change is ready for review.
Comment on attachment 66934 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=66934&action=prettypatch Thanks for cleaning this up. Would appreciate it if you could clean up two very minor things before commit. > WebCore/ChangeLog:20 > + (WebCore::ImageLayerChromium::updateContents): Could you regenerate the ChangeLog? ImageLayerChromium.cpp isn't touched in the diffs below. > WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:-87 > - " vec4 texColor = texture2D(s_texture, vec2(v_texCoord.x, 1.0 - v_texCoord.y)); \n" Could you please clean up the comment at the top of the fragmentShaderString too, in particular the reference to the differences in origin?
Created attachment 67500 [details] Patch
Comment on attachment 67500 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=67500&action=prettypatch Looks good to me. The commit queue might reject this because you cq+'d it before it was r+'d; if that happens just cq+ it again.
Comment on attachment 67500 [details] Patch Clearing flags on attachment: 67500 Committed r67493: <http://trac.webkit.org/changeset/67493>
All reviewed patches have been landed. Closing bug.