Bug 85171 – [EFL] LayoutTestController does not support overriding WebKitLoadSiteIconsKey

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Chris Dumez]

The following test depends on overriding the "WebKitLoadSiteIconsKey" setting via LayoutTestController:
http/tests/misc/favicon-loads-with-icon-loading-override.html 

This functionality needs to be implemented in order to unskip this test case in EFL port.

[Chris Dumez]

Created attachment 139422 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

This patch seems to overlap with the one in bug 83007.

[Chris Dumez]

(In reply to comment #2)
> This patch seems to overlap with the one in bug 83007.

The patch at Bug 83007 does not support overriding "WebKitLoadSiteIconsKey" setting. If anything, the patches are complementary, not really overlapping. I don't see any issue with it, do you?

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #3)
> (In reply to comment #2)
> > This patch seems to overlap with the one in bug 83007.
> 
> The patch at Bug 83007 does not support overriding "WebKitLoadSiteIconsKey" setting. If anything, the patches are complementary, not really overlapping. I don't see any issue with it, do you?

They both implement overridePreference and toBool() slightly differently; wouldn't it make sense to land this one after 83007?

[Chris Dumez]

Ok, adding dependency on Bug 83007.

[Gyuyoung Kim]

Comment on attachment 139422 [details]
Patch

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

> Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.cpp:395
> +

Unneeded line.

> Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:666
> +static gboolean booleanFromValue(JSStringRef value)

Do not use unneeded glib type for efl port.

> Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:677
> +    else

If there is no logic when keyString is not "WebKitLoadSiteIconKey", is it better to do nothing until we support other keyStrings ? 

I don't see below code in webkit yet.
else
   notImplemented();

[Chris Dumez]

Created attachment 139749 [details]
Patch

Take feedback into consideration and make sure the patch is consistent with the one for Bug 83007.

Now, I don't think it matters which patch (Bug 83007 or this one) lands first.

[Chris Dumez]

Created attachment 139777 [details]
Patch

Make toBool() method inline and make sure the "WebKitLoadSiteIconsKey" setting is reset in DumpRenderTreeChrome::resetDefaultsToConsistentValues().

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 139777 [details]
Patch

This patch looks fine now. Bug 83007 has landed, so you'll need to rebase.

[Chris Dumez]

Created attachment 139953 [details]
Patch

Rebase on master.

[WebKit Review Bot]

Comment on attachment 139953 [details]
Patch

Rejecting attachment 139953 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
 Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.cpp
patching file Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.h
patching file Tools/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp
patching file Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Andreas Kl..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/12531147

[Chris Dumez]

Created attachment 140218 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 140218 [details]
Patch for landing

Clearing flags on attachment: 140218

Committed r116097: <http://trac.webkit.org/changeset/116097>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.