Bug 53644 – [GTK] editing/deleting/5408255.html results are incorrect

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Martin Robinson]

This test produces incorrect results. The test text says that the box should be empty, but it still contains the list after it has run. This can be observed by looking at the test dump and pixel output.

[Zan Dobersek]

Created attachment 87881 [details]
Actual result in r82691

Running this test in a release 64-bit build generates attached result. The result seems right - the list is removed with no errors popping out (as they seemed to, based on the current generated results).

I think the text and pixel results should be reset and this test unskipped.

[Martin Robinson]

Committed r82708: <http://trac.webkit.org/changeset/82708>

[Martin Robinson]

(In reply to comment #1)
> Running this test in a release 64-bit build generates attached result. The result seems right - the list is removed with no errors popping out (as they seemed to, based on the current generated results).

It definitely appears to be passing now. I've added results and unskipped the test. Thanks!

[Martin Robinson]

Reverted r82708 for reason:

This tests fails consistently when run along with other tests

Committed r82743: <http://trac.webkit.org/changeset/82743>

[Martin Robinson]

It appears this test fails when run along with the other tests. Can you confirm this Zan?

[Zan Dobersek]

(In reply to comment #5)
> It appears this test fails when run along with the other tests. Can you confirm this Zan?

I can confirm, this test fails in debug build.

Comparing the test between the release and debug builds (in GtkLauncher), the delete button does not seem to appear in debug build, therefor there's no way of deleting the list and the failure occurs.

[Zan Dobersek]

Looking a bit into it, a deletion UI is started being created in DeleteButtonController::createDeletionUI[1] but then stops because platform resource (deleteButton) is not properly loaded so the function returns[2]. I'll see why that occurs and try to fix it.


[1] http://trac.webkit.org/browser/trunk/Source/WebCore/editing/DeleteButtonController.cpp#L187
[2] http://trac.webkit.org/browser/trunk/Source/WebCore/editing/DeleteButtonController.cpp#L245

[Zan Dobersek]

(In reply to comment #7)
> Looking a bit into it, a deletion UI is started being created in DeleteButtonController::createDeletionUI[1] but then stops because platform resource (deleteButton) is not properly loaded so the function returns[2]. I'll see why that occurs and try to fix it.
> 

Here's why test didn't pass with a debug build - as said, the platform resource, in this case 'deleteButton.png', did not properly load because platform resources in Gtk port are loaded from DATA_DIR, but I didn't install the debug build so DATA_DIR didn't actually exist.

On the other hand, I did install the release build (with a non-default prefix, so the debug build couldn't load the resources despite being configured with a default prefix), so the test did pass in this case.

The solution to passing this test would be to make the platform resources available in correct spots on the buildbots.

[Martin Robinson]

(In reply to comment #8)
> The solution to passing this test would be to make the platform resources available in correct spots on the buildbots.

It would be nice if the code was smart enough to find the resources in the build or source directory if they are not found in the installation directory.

[Zan Dobersek]

Created attachment 118807 [details]
Patch

Set an environment variable that points to resources in source checkout when testing

[Martin Robinson]

Comment on attachment 118807 [details]
Patch

Thanks for looking into this!

Instead of adding another environment variable, could you look for the resources relative to the executable path? This is nice because many times we want to run DumpRenderTree outside the harness (for attaching gdb, etc).

Another approach is to find the resources relative to WEBKIT_TOP_LEVEL and we can make sure that the harness defines WEBKIT_TOP_LEVEL if it isn't in the environment.

[Philippe Normand]

Comment on attachment 118807 [details]
Patch

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

> Source/WebCore/platform/graphics/gtk/ImageGtk.cpp:78
>  static const char* getWebKitDataDirectory()

I think it would make sense to return a smart pointer here now.

> Source/WebCore/platform/graphics/gtk/ImageGtk.cpp:151
> +        GOwnPtr<gchar> glibFileName(g_build_filename(getWebKitDataDirectory(), imageName.get(), NULL));

I think this introduces a leak of the string returned by getWebKitDataDirectory().

[Zan Dobersek]

Created attachment 120476 [details]
WIP Patch

This is just a WIP patch on which I'd like some comments.

I've introduced a new macro, TOP_LEVEL_DIR, in Tools/GNUmakefile.am. It is later used in DumpRenderTree.cpp as a return value when calling getTopLevelPath. Also in that function, if environment variable WEBKIT_TOP_LEVEL is not set, it is set to the value of the TOP_LEVEL_DIR macro.

This nullifies the need of setting the WEBKIT_TOP_LEVEL variable in testing environment, so this is not done anymore in the GtkPort in webkitpy.

In ImageGtk.cpp, resource is loaded from the checkout tree if WEBKIT_TOP_LEVEL is set (now only if DumpRenderTree is run) or from DATA_DIR otherwise. Smart pointer is not used at this moment as I believe there's no need for it based on the path acquiring that is used in this patch.

This patch leaves the resource loading on Windows platform pretty broken. I'm not able to test the changes for this platform, so the best I can do is a blind guess at how things should be handled.

I'd like to hear some comments on this patch and especially the Windows issue, and because this patch is more of a WIP/RFC, I'm not setting any flags on this patch.

[Martin Robinson]

Comment on attachment 120476 [details]
WIP Patch

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

Okay. This seems like a nice fix. r- here because it should be possible to get something close-to-right on Windows without building and because this breaks WebKitTestRunner. Let me know if you need tips for testing WKTR.

> Source/WebCore/platform/graphics/gtk/ImageGtk.cpp:51
> +static const char* getDataPathForResource()

You need to fix this up a bit. The signature should be: static char* getDataPathForResource(const char* resource). Later on you should use the resource name to find the image.

> Source/WebCore/platform/graphics/gtk/ImageGtk.cpp:78
> +static char* getDataPathForResource(char* resource)

You should probably rename this to something like getPathToImage now. The argument should be a const char*.

> Tools/GNUmakefile.am:76
> +	-DTOP_LEVEL_DIR=\"${shell pwd}/${srcdir}\" \

You need to do something similar for WebKitTestRunner too if you are going to no longer set this environment variable.

[Zan Dobersek]

Created attachment 123588 [details]
Patch

Modifies the Windows code as well as possible and also defines the macro in WKTR's injected bundle. Note that this test still doesn't pass in WebKitTestRunner as currently deletion UI is not shown in WebKit2 - http://trac.webkit.org/browser/trunk/Source/WebKit2/WebProcess/WebCoreSupport/WebEditorClient.cpp#L68

[Martin Robinson]

Comment on attachment 123588 [details]
Patch

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

Looks nice, but I have a small concern about the Windows stuff.

> Source/WebCore/platform/graphics/gtk/ImageGtk.cpp:74
> +        strcat(dataDirectory, "\\share\\webkitgtk-"WEBKITGTK_API_VERSION_STRING"\\images\\");
>      }
> -    strcat(dataDirectory, "\\share");
>  
> -    return dataDirectory;
> +    char* imageResourcePath = new char[PATH_MAX];
> +    strcat(imageResourcePath, dataDirectory);
> +    strcat(imageResourcePath, resource);

Doesn't this change the behavior a bit here? Before it would look for $moduleDirectory\share\image.png and now it's looking for $moduleDirectory\share\webkitgtk-3.0\images\image.png? In this case $moduleDirectory is the location of the DLL file.

[Zan Dobersek]

Comment on attachment 123588 [details]
Patch

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

>> Source/WebCore/platform/graphics/gtk/ImageGtk.cpp:74
>> +    strcat(imageResourcePath, resource);
> 
> Doesn't this change the behavior a bit here? Before it would look for $moduleDirectory\share\image.png and now it's looking for $moduleDirectory\share\webkitgtk-3.0\images\image.png? In this case $moduleDirectory is the location of the DLL file.

Previously, the getWebKitDataDirectory function would just return the data directory, being either $moduleDirectory\share or DATA_DIR. This path would then be used in Image::loadPlatformResource, which would, in the old way, build a filename, starting with the data directory and appending the "webkitgtk-"WEBKITGTK_API_VERSION_STRING, "images" and finally the image name (whatever the OS).

Now, getPathToImageResource is responsible to deliver the complete path to the required image resource, so on Windows we must further append the required path parts to get the complete and proper path.

[Martin Robinson]

Comment on attachment 123588 [details]
Patch

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

Thanks for fixing this!

>>> Source/WebCore/platform/graphics/gtk/ImageGtk.cpp:74
>>> +    strcat(imageResourcePath, resource);
>> 
>> Doesn't this change the behavior a bit here? Before it would look for $moduleDirectory\share\image.png and now it's looking for $moduleDirectory\share\webkitgtk-3.0\images\image.png? In this case $moduleDirectory is the location of the DLL file.
> 
> Previously, the getWebKitDataDirectory function would just return the data directory, being either $moduleDirectory\share or DATA_DIR. This path would then be used in Image::loadPlatformResource, which would, in the old way, build a filename, starting with the data directory and appending the "webkitgtk-"WEBKITGTK_API_VERSION_STRING, "images" and finally the image name (whatever the OS).
> 
> Now, getPathToImageResource is responsible to deliver the complete path to the required image resource, so on Windows we must further append the required path parts to get the complete and proper path.

Indeed you're right!

[WebKit Review Bot]

Comment on attachment 123588 [details]
Patch

Clearing flags on attachment: 123588

Committed r105686: <http://trac.webkit.org/changeset/105686>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.