Bug 85171 - [EFL] LayoutTestController does not support overriding WebKitLoadSiteIconsKey
Summary: [EFL] LayoutTestController does not support overriding WebKitLoadSiteIconsKey
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-30 00:29 PDT by Chris Dumez
Modified: 2012-05-06 22:55 PDT (History)
4 users (show)

See Also:


Attachments
Patch (5.92 KB, patch)
2012-04-30 00:38 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (5.87 KB, patch)
2012-05-01 22:51 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (6.69 KB, patch)
2012-05-02 02:55 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (6.49 KB, patch)
2012-05-02 22:26 PDT, Chris Dumez
kling: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (6.69 KB, patch)
2012-05-04 07:10 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2012-04-30 00:29:37 PDT
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.
Comment 1 Chris Dumez 2012-04-30 00:38:40 PDT
Created attachment 139422 [details]
Patch
Comment 2 Raphael Kubo da Costa (:rakuco) 2012-04-30 18:33:14 PDT
This patch seems to overlap with the one in bug 83007.
Comment 3 Chris Dumez 2012-05-01 00:25:30 PDT
(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?
Comment 4 Raphael Kubo da Costa (:rakuco) 2012-05-01 06:50:24 PDT
(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?
Comment 5 Chris Dumez 2012-05-01 08:49:58 PDT
Ok, adding dependency on Bug 83007.
Comment 6 Gyuyoung Kim 2012-05-01 18:46:03 PDT
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();
Comment 7 Chris Dumez 2012-05-01 22:51:06 PDT
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.
Comment 8 Chris Dumez 2012-05-02 02:55:13 PDT
Created attachment 139777 [details]
Patch

Make toBool() method inline and make sure the "WebKitLoadSiteIconsKey" setting is reset in DumpRenderTreeChrome::resetDefaultsToConsistentValues().
Comment 9 Raphael Kubo da Costa (:rakuco) 2012-05-02 12:35:35 PDT
Comment on attachment 139777 [details]
Patch

This patch looks fine now. Bug 83007 has landed, so you'll need to rebase.
Comment 10 Chris Dumez 2012-05-02 22:26:01 PDT
Created attachment 139953 [details]
Patch

Rebase on master.
Comment 11 WebKit Review Bot 2012-05-04 06:51:06 PDT
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
Comment 12 Chris Dumez 2012-05-04 07:10:22 PDT
Created attachment 140218 [details]
Patch for landing
Comment 13 WebKit Review Bot 2012-05-04 07:31:57 PDT
Comment on attachment 140218 [details]
Patch for landing

Clearing flags on attachment: 140218

Committed r116097: <http://trac.webkit.org/changeset/116097>
Comment 14 WebKit Review Bot 2012-05-04 07:32:03 PDT
All reviewed patches have been landed.  Closing bug.