In the current setup, we dump pixel results from the web process, bypassing the Texture Mapper. In order to correctly dump the pixels for 3D transforms we have to do the painting in the UI process, the same as was done for Qt. A patch is coming.
Created attachment 175510 [details] Patch Generate a snapshot of the view in the UI process instead of the web process. We have to use Texture Mapper in order to correctly paint 3D transforms etc. Since I could not find an efl based API that will give me a snapshot, and I cannot use both evas and openGl API in the same file, I created a new file, in which I use openGL to get a snapshot.
Comment on attachment 175510 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175510&action=review > Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:45 > + return viewImpl->getSnapshot(); takeShapshot? > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:978 > + } Is this right with those ifdefs?
(In reply to comment #2) > (From update of attachment 175510 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=175510&action=review > > > Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:45 > > + return viewImpl->getSnapshot(); > > takeShapshot? > ok > > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:978 > > + } > > Is this right with those ifdefs? yes. both '{' and '}' are inside #if USE(ACCELERATED_COMPOSITING)
Comment on attachment 175510 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175510&action=review > Source/WebKit2/UIProcess/API/efl/SnapshotGL.cpp:24 > +#include <GL/gl.h> Unfortunately this will not work for opengl ES. For GL ES2 it will be GLES2/gl2.h, for GL ES1 it will be GLES/gl.h I think there should be single place for this, may be OpenGLShims.h ?
(In reply to comment #4) > (From update of attachment 175510 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=175510&action=review > > > Source/WebKit2/UIProcess/API/efl/SnapshotGL.cpp:24 > > +#include <GL/gl.h> > > Unfortunately this will not work for opengl ES. > For GL ES2 it will be GLES2/gl2.h, for GL ES1 it will be GLES/gl.h > > I think there should be single place for this, may be OpenGLShims.h ? I got a tip from Carsten Haitzler on the enlightement mailing list about some ecore_x API that I can use. I will give that a try first.
Created attachment 175907 [details] Patch Address comment #2 and comment #4. XShmGetImage is failing to grab the image, so I resorted to glReadPixels, which is what the Qt port does.
Attachment 175907 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:64: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 175907 [details] Patch Attachment 175907 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14981481
Created attachment 175910 [details] Patch Forgot to remove one of the X11 headers. sorry :(
Attachment 175910 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedBackingStore.cpp:122: One space before end of line comments [whitespace/comments] [5] Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedBackingStore.cpp:122: Should have a space between // and comment [whitespace/comments] [4] Total errors found: 2 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 175911 [details] Patch
Comment on attachment 175911 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175911&action=review > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:1034 > + delete buffer; delete[] buffer ? > Source/WebKit2/UIProcess/API/efl/SnapshotGL.cpp:34 > + unsigned char* buffer = new unsigned char[width * height * 4]; allocated with operator new[]. > Source/WebKit2/UIProcess/API/efl/SnapshotGL.cpp:39 > + unsigned* buf = (unsigned*)buffer; Please void C style cast > Source/WebKit2/UIProcess/API/efl/SnapshotGL.cpp:41 > + for (int i = 0; i < height / 2; i++) { we usually prefer preincrement when possible > Source/WebKit2/UIProcess/API/efl/SnapshotGL.cpp:42 > + for (int j = 0; j < width; j++) { ditto > Tools/WebKitTestRunner/efl/PlatformWebViewEfl.cpp:138 > + RefPtr<cairo_surface_t> surface = WKImageCreateCairoSurface(WKViewGetSnapshot(toAPI(m_view))); You need to adopt. > Tools/WebKitTestRunner/efl/PlatformWebViewEfl.cpp:147 > + return adoptWK(WKImageCreateFromCairoSurface(surface.get(), options)); I did not understand why we converted to a cairo surface and then back to a WKImageRef. Why not return WKViewGetSnapshot(toAPI(m_view)) directly? > Tools/WebKitTestRunner/efl/TestInvocationEfl.cpp:85 > + cairo_t* context = cairo_create(surface); Smart pointer? > Tools/WebKitTestRunner/efl/TestInvocationEfl.cpp:121 > + cairo_surface_t* surface = 0; Smart pointer?
(In reply to comment #12) Thank you for your comments :) > (From update of attachment 175911 [details]) > we usually prefer preincrement when possible > Can you please tell me which coding guideline says this? > I did not understand why we converted to a cairo surface and then back to a WKImageRef. Why not return WKViewGetSnapshot(toAPI(m_view)) directly? I didn't know if it is ok to return a cairo type in our API. If that is ok, then there is no need for the multiple conversions.
Comment on attachment 175911 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175911&action=review >> Source/WebKit2/UIProcess/API/efl/SnapshotGL.cpp:41 >> + for (int i = 0; i < height / 2; i++) { > > we usually prefer preincrement when possible It is not documented in the WebKit style guidelines document but Benjamin Poulain commented about this about on one of my patches: https://bugs.webkit.org/show_bug.cgi?id=101257#c22 I also landed a cleanup patch regarding this in: https://bugs.webkit.org/show_bug.cgi?id=101859 While this makes no difference for primitive types, it usually does for complex types. Therefore, it is good practice to use prefix operator whenever possible. >> Tools/WebKitTestRunner/efl/PlatformWebViewEfl.cpp:147 >> + return adoptWK(WKImageCreateFromCairoSurface(surface.get(), options)); > > I did not understand why we converted to a cairo surface and then back to a WKImageRef. Why not return WKViewGetSnapshot(toAPI(m_view)) directly? What I mean is WKViewGetSnapshot() returns a WKImageRef and this function returns a WKImageRef. Why not return the output of WKViewGetSnapshot() directly? Instead you are creating a surface using WKImageCreateCairoSurface() from the WKImage, then converting the surface back to a WKImage. I did not understand the reason for this.
(In reply to comment #12) > (From update of attachment 175911 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=175911&action=review > > > Tools/WebKitTestRunner/efl/TestInvocationEfl.cpp:85 > > + cairo_t* context = cairo_create(surface); > > Smart pointer? > > > Tools/WebKitTestRunner/efl/TestInvocationEfl.cpp:121 > > + cairo_surface_t* surface = 0; > > Smart pointer? TestInvocationEfl.cpp is almost a copy of TestInvocationCairo.cpp, with some modifications specific to EFL. I was hoping to get some comments regarding if people prefer that the file is split, or to add the EFL specific bits to TestInvocationCairo.cpp. I think your comments are result of copy/pasted code from TestInvocationCairo.cpp.
Adding a few GTK guys to see if they have an opinion regarding the addition of EFL specific code in TestInvocationCairo.cpp (https://bugs.webkit.org/show_bug.cgi?id=102833#c15).
Created attachment 176066 [details] Patch Address comment #12.
Comment on attachment 176066 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176066&action=review > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:1035 > + delete[] buffer; > + return image; I prefer newline before return > Source/WebKit2/UIProcess/API/efl/SnapshotGL.cpp:37 > + // Now flip it Bad comment and ends without punctuation mark > Source/WebKit2/UIProcess/API/efl/SnapshotGL.h:21 > +#ifndef SnapshotGL_h > +#define SnapshotGL_h what about SnapshotImageGL ?
Created attachment 176113 [details] Patch Address comment #18.
Comment on attachment 176113 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176113&action=review > Source/WebKit2/PlatformEfl.cmake:45 > + UIProcess/API/efl/SnapshotImageGL.cpp Wrong alphabetical order. > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:1026 > + return WKImageCreateFromCairoSurface(surface.get(), 0); If you don't use local variable, I think you can remove "{", "}" in this function. return WKImageCreateFromCairoSurface(createSurfaceForImage(sd->image).get(), 0); Or, because you define *surface* variable two places, I think you can declare it first on 1022 line. > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:1030 > + Evas_Native_Surface* ns = evas_object_image_native_surface_get(sd->image); I prefer to use full name. e.g) naviveSurface. > Source/WebKit2/UIProcess/API/efl/SnapshotImageGL.cpp:21 > +#if USE(ACCELERATED_COMPOSITING) IMO, it would be better if you move this macro to above #if USE(OPENGL_ES_2)
Created attachment 176279 [details] Patch Address comment #20.
Comment on attachment 176279 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176279&action=review > Tools/WebKitTestRunner/efl/TestInvocationEfl.cpp:45 > +void computeMD5HashStringForCairoSurface(cairo_surface_t* surface, char hashString[33]) There seems to be quite a bit of duplication with TestInvocationCairo.cpp. I really wish we could avoid that. Personally, I would add #ifdefs in TestInvocationCairo.cpp to git EFL port's need. This would avoid the duplication. Any other opinion on this?
(In reply to comment #22) > (From update of attachment 176279 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=176279&action=review > > > Tools/WebKitTestRunner/efl/TestInvocationEfl.cpp:45 > > +void computeMD5HashStringForCairoSurface(cairo_surface_t* surface, char hashString[33]) > > There seems to be quite a bit of duplication with TestInvocationCairo.cpp. I really wish we could avoid that. > > Personally, I would add #ifdefs in TestInvocationCairo.cpp to git EFL port's need. This would avoid the duplication. > Any other opinion on this? That sounds like the proper thing to do indeed
Created attachment 176329 [details] Patch Removed TestInvocationEFL.cpp
Looks like I cannot include WebCore headers in webkit-gtk, so I cannot use RefCairoPtr in common code. Tools/WebKitTestRunner/cairo/Programs_WebKitTestRunner-TestInvocationCairo.o:TestInvocationCairo.cpp:function WTR::TestInvocation::dumpPixelsAndCompareWithExpected(OpaqueWKImage const*, OpaqueWKArray const*): error: undefined reference to 'void WTF::derefIfNotNull<_cairo_surface>(_cairo_surface*)' Tools/WebKitTestRunner/cairo/Programs_WebKitTestRunner-TestInvocationCairo.o:TestInvocationCairo.cpp:function WTR::TestInvocation::dumpPixelsAndCompareWithExpected(OpaqueWKImage const*, OpaqueWKArray const*): error: undefined reference to 'void WTF::derefIfNotNull<_cairo>(_cairo*)' Tools/WebKitTestRunner/cairo/Programs_WebKitTestRunner-TestInvocationCairo.o:TestInvocationCairo.cpp:function WTR::TestInvocation::dumpPixelsAndCompareWithExpected(OpaqueWKImage const*, OpaqueWKArray const*): error: undefined reference to 'void WTF::derefIfNotNull<_cairo>(_cairo*)' Tools/WebKitTestRunner/cairo/Programs_WebKitTestRunner-TestInvocationCairo.o:TestInvocationCairo.cpp:function WTR::TestInvocation::dumpPixelsAndCompareWithExpected(OpaqueWKImage const*, OpaqueWKArray const*): error: undefined reference to 'void WTF::derefIfNotNull<_cairo_surface>(_cairo_surface*)' collect2: error: ld returned 1 exit status make[1]: *** [Programs/WebKitTestRunner] Error 1 make[1]: Leaving directory `/home/phil/gst/jhbuild/makes/WebKit/WebKitBuild/Release' make: *** [all] Error 2 Failed to build WebKit using 'make'!
(In reply to comment #25) > Looks like I cannot include WebCore headers in webkit-gtk, so I cannot use RefCairoPtr in common code. > > Tools/WebKitTestRunner/cairo/Programs_WebKitTestRunner-TestInvocationCairo.o:TestInvocationCairo.cpp:function WTR::TestInvocation::dumpPixelsAndCompareWithExpected(OpaqueWKImage const*, OpaqueWKArray const*): error: undefined reference to 'void WTF::derefIfNotNull<_cairo_surface>(_cairo_surface*)' > Tools/WebKitTestRunner/cairo/Programs_WebKitTestRunner-TestInvocationCairo.o:TestInvocationCairo.cpp:function WTR::TestInvocation::dumpPixelsAndCompareWithExpected(OpaqueWKImage const*, OpaqueWKArray const*): error: undefined reference to 'void WTF::derefIfNotNull<_cairo>(_cairo*)' > Tools/WebKitTestRunner/cairo/Programs_WebKitTestRunner-TestInvocationCairo.o:TestInvocationCairo.cpp:function WTR::TestInvocation::dumpPixelsAndCompareWithExpected(OpaqueWKImage const*, OpaqueWKArray const*): error: undefined reference to 'void WTF::derefIfNotNull<_cairo>(_cairo*)' > Tools/WebKitTestRunner/cairo/Programs_WebKitTestRunner-TestInvocationCairo.o:TestInvocationCairo.cpp:function WTR::TestInvocation::dumpPixelsAndCompareWithExpected(OpaqueWKImage const*, OpaqueWKArray const*): error: undefined reference to 'void WTF::derefIfNotNull<_cairo_surface>(_cairo_surface*)' > collect2: error: ld returned 1 exit status > make[1]: *** [Programs/WebKitTestRunner] Error 1 > make[1]: Leaving directory `/home/phil/gst/jhbuild/makes/WebKit/WebKitBuild/Release' > make: *** [all] Error 2 > > Failed to build WebKit using 'make'! This would probably fix it: diff --git a/Tools/WebKitTestRunner/GNUmakefile.am b/Tools/WebKitTestRunner/GNUmakefile.am index 3706003..b1dfb98 100644 --- a/Tools/WebKitTestRunner/GNUmakefile.am +++ b/Tools/WebKitTestRunner/GNUmakefile.am @@ -52,6 +52,7 @@ Programs_WebKitTestRunner_CFLAGS = $(global_cflags) Programs_WebKitTestRunner_LDADD = \ libjavascriptcoregtk-@WEBKITGTK_API_MAJOR_VERSION@.@WEBKITGTK_API_MINOR_VERSION@.la \ + libWebCoreGtk.la \ libwebkit2gtk-@WEBKITGTK_API_MAJOR_VERSION@.@WEBKITGTK_API_MINOR_VERSION@.la \ $(GLOBALDEPS_LIBS) \ $(CAIRO_LIBS) \ Unless there is a specific reason not to have WebKitTestRunner link against WebCore library?
Created attachment 176336 [details] Patch Removed smart pointer usage from TestInvocationCairo.cpp. They break the GTK build.
Comment on attachment 176336 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176336&action=review > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:1026 > + return WKImageCreateFromCairoSurface(createSurfaceForImage(sd->image).get(), 0); > +#if USE(ACCELERATED_COMPOSITING) newline after return please > Source/WebKit2/UIProcess/API/efl/SnapshotImageGL.cpp:6 > +/* > + Copyright (C) 2012 Intel Corporation. All rights reserved. > + > + This library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Library General Public > + License as published by the Free Software Foundation; either why not * ? and why not BSD license? I think we prefer that > Tools/WebKitTestRunner/cairo/TestInvocationCairo.cpp:110 > +void TestInvocation::forceRepaintDoneCallback(WKErrorRef, void *context) didFinishRepaint would be a nicer name > Tools/WebKitTestRunner/cairo/TestInvocationCairo.cpp:112 > + static_cast<TestInvocation*>(context)->m_gotRepaint = true; gotRepaint is a terrible name. why not m_didRepaint > Tools/WebKitTestRunner/cairo/TestInvocationCairo.cpp:136 > + > + if (m_gotRepaint) > + surface = WKImageCreateCairoSurface(TestController::shared().mainWebView()->windowSnapshotImage().get()); > + else { > + m_error = true; > + m_errorMessage = "Timed out waiting for repaint\n"; > + m_webProcessIsUnrensponsive = true; > + return; > + } no. if (!m_didRepaint) { ... return; } cairo_surface_t surface = WKImageCreateCairoSurface(TestController::shared().mainWebView()->windowSnapshotImage().get()); > Tools/WebKitTestRunner/efl/PlatformWebViewEfl.cpp:132 > + Ecore_Evas* ee = ecore_evas_ecore_evas_get(evas_object_evas_get(m_view)); > + > + ASSERT(ee); no newline before ASSERT, it belong to ee
(In reply to comment #28) > (From update of attachment 176336 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=176336&action=review > > > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:1026 > > + return WKImageCreateFromCairoSurface(createSurfaceForImage(sd->image).get(), 0); > > +#if USE(ACCELERATED_COMPOSITING) > > newline after return please > ok > > Source/WebKit2/UIProcess/API/efl/SnapshotImageGL.cpp:6 > > +/* > > + Copyright (C) 2012 Intel Corporation. All rights reserved. > > + > > + This library is free software; you can redistribute it and/or > > + modify it under the terms of the GNU Library General Public > > + License as published by the Free Software Foundation; either > > why not * ? and why not BSD license? I think we prefer that > I copied the license of EwkViewImpl, I'll change that. > > Tools/WebKitTestRunner/cairo/TestInvocationCairo.cpp:110 > > +void TestInvocation::forceRepaintDoneCallback(WKErrorRef, void *context) > > didFinishRepaint would be a nicer name > The functions names are not introduced in this patch. I can rename them as a followup > > Tools/WebKitTestRunner/cairo/TestInvocationCairo.cpp:112 > > + static_cast<TestInvocation*>(context)->m_gotRepaint = true; > > gotRepaint is a terrible name. why not m_didRepaint > copy/paste from Qt :) I'll rename > > Tools/WebKitTestRunner/cairo/TestInvocationCairo.cpp:136 > > + > > + if (m_gotRepaint) > > + surface = WKImageCreateCairoSurface(TestController::shared().mainWebView()->windowSnapshotImage().get()); > > + else { > > + m_error = true; > > + m_errorMessage = "Timed out waiting for repaint\n"; > > + m_webProcessIsUnrensponsive = true; > > + return; > > + } > > no. > > if (!m_didRepaint) { > ... > return; > } > same as before, copy/paste from Qt :) Will change. > cairo_surface_t surface = WKImageCreateCairoSurface(TestController::shared().mainWebView()->windowSnapshotImage().get()); > > > Tools/WebKitTestRunner/efl/PlatformWebViewEfl.cpp:132 > > + Ecore_Evas* ee = ecore_evas_ecore_evas_get(evas_object_evas_get(m_view)); > > + > > + ASSERT(ee); > > no newline before ASSERT, it belong to ee ok
Created attachment 176347 [details] Patch Address part of comment #28.
Comment on attachment 176347 [details] Patch Clearing flags on attachment: 176347 Committed r135935: <http://trac.webkit.org/changeset/135935>
All reviewed patches have been landed. Closing bug.
This patch seems to have caused 6 failures: http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug%20WK2/builds/6416 We have blurry text on scaled layers and a few other issues.
(In reply to comment #33) > This patch seems to have caused 6 failures: > http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug%20WK2/builds/6416 > > We have blurry text on scaled layers and a few other issues. There are over 100 tests that have become flaky ([ IMAGE PASS ]) because of this
Not sure how it is related but fast/dom/shadow/input-with-validation.html seems to have started crashing reliably after this commit on WK2 debug
(In reply to comment #35) > Not sure how it is related but fast/dom/shadow/input-with-validation.html seems to have started crashing reliably after this commit on WK2 debug We have these bugs to track the crash: https://bugs.webkit.org/show_bug.cgi?id=98026 https://bugs.webkit.org/show_bug.cgi?id=99886
Causes masive reftests flakiness, this needs to be solved urgently.
(In reply to comment #37) > Causes masive reftests flakiness, this needs to be solved urgently. Unfortunately, many of the failures are real failures. I was looking today e.g. the css/sticky results and the actual results match what I see in MiniBrowser. Which means we have bugs that need to be fixed.
(In reply to comment #37) > Causes masive reftests flakiness, this needs to be solved urgently. Thanks to kbalazs, who helped me analyze some of the flaky tests. It seems that we have a bug in the way we draw the scrollbars. If you look at the expected results images, they are completely wrong in many tests. I will file a separate bug about that.
(In reply to comment #39) > (In reply to comment #37) > > Causes masive reftests flakiness, this needs to be solved urgently. > > Thanks to kbalazs, who helped me analyze some of the flaky tests. It seems that we have a bug in the way we draw the scrollbars. If you look at the expected results images, they are completely wrong in many tests. I will file a separate bug about that. https://bugs.webkit.org/show_bug.cgi?id=103641
(In reply to comment #38) > (In reply to comment #37) > > Causes masive reftests flakiness, this needs to be solved urgently. > > Unfortunately, many of the failures are real failures. > I was looking today e.g. the css/sticky results and the actual results match what I see in MiniBrowser. Which means we have bugs that need to be fixed. Many tests don't seem that way. As an example (and this one seems common) fast/forms/input-first-letter-edit.html is just empty (totally white) when it fails. If you look at the test, it is highly unlikely to be a real failure: it's basically just <body> <input type="text" id="target" /> </body> with a bit of script that 'types' text into the input. I don't think that really renders as an empty page.
Closing this bug because the EFL port has been removed from trunk. If you feel this bug applies to a different upstream WebKit port and was closed in error, please either update the title and reopen the bug, or leave a comment to request this.