Bug 83007 - [EFL] EFL's LayoutTestController overridePreference implementation
: [EFL] EFL's LayoutTestController overridePreference implementation
Status: RESOLVED FIXED
: WebKit
WebKit EFL
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
: 84340 84451 84534
: 83900
  Show dependency treegraph
 
Reported: 2012-04-03 00:55 PST by
Modified: 2012-05-22 11:09 PST (History)


Attachments
Implementation of LayoutTestController::overridePreference + common ewk_view settings interface (31.08 KB, patch)
2012-04-11 03:07 PST, Mikhail Pozdnyakov
no flags Review Patch | Details | Formatted Diff | Diff
Implementation of LayoutTestController::overridePreference + adding of 3 new ewk_view settings (16.55 KB, patch)
2012-04-15 14:11 PST, Mikhail Pozdnyakov
no flags Review Patch | Details | Formatted Diff | Diff
added better description of new ewk_view settings (16.83 KB, patch)
2012-04-16 07:19 PST, Mikhail Pozdnyakov
no flags Review Patch | Details | Formatted Diff | Diff
Implementation of LayoutTestController::overridePreference (7.68 KB, patch)
2012-04-19 12:14 PST, Mikhail Pozdnyakov
gyuyoung.kim: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Implementation of LayoutTestController::overridePreference (7.34 KB, patch)
2012-04-19 13:12 PST, Mikhail Pozdnyakov
no flags Review Patch | Details | Formatted Diff | Diff
Implementation of LayoutTestController::overridePreference (7.34 KB, patch)
2012-04-19 13:18 PST, Mikhail Pozdnyakov
eric: review+
gyuyoung.kim: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Implementation of LayoutTestController::overridePreference + reset default values (8.30 KB, patch)
2012-04-20 06:08 PST, Mikhail Pozdnyakov
tonikitoo: review+
gyuyoung.kim: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
to be landed. (9.76 KB, patch)
2012-05-02 08:31 PST, Mikhail Pozdnyakov
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-04-03 00:55:00 PST
EFL's LayoutTestController needs overridePreference method implementation, which is necessary for unskipping following test cases:
    compositing/webgl/webgl-background-color.html

    compositing/webgl/webgl-no-alpha.html

    compositing/webgl/webgl-nonpremultiplied-blend.html

    compositing/webgl/webgl-reflection.html

    editing/selection/caret-mode-paragraph-keys-navigation.html

    fast/canvas/webgl

    fast/dom/Window/timer-resume-on-navigation-back.html

    fast/events/onunload-back-to-page-cache.html

    fast/events/pagehide-timeout.html

    fast/events/pageshow-pagehide-on-back-cached-with-frames.html

    fast/events/pageshow-pagehide-on-back-cached.html

    fast/events/tab-focus-anchor.html

    fast/frames/frame-crash-with-page-cache.html

    fast/harness/override-preferences-2.html

    fast/harness/override-preferences.html

    fast/harness/page-cache-crash-on-data-urls.html

    fast/harness/use-page-cache.html

    fast/history/back-forward-reset-after-error-handling.html

    fast/history/timed-refresh-in-cached-frame.html

    fast/images/animated-gif-restored-from-bfcache.html

    fast/loader/frames-with-unload-handlers-in-page-cache.html

    fast/loader/input-element-page-cache-crash.html

    fast/loader/scroll-position-restored-on-back.html

    fast/loader/stateobjects/popstate-fires-with-page-cache.html

    fast/loader/unschedule-relayout-after-unload.html

    fast/media/media-query-list-04.html

    fast/overflow/horizontal-scroll-after-back.html

    fast/parser/noscript-with-javascript-disabled.html

    fast/parser/noscript-with-javascript-enabled.html

    fast/parser/pre-html5-parser-quirks.html

    fast/repaint/no-caret-repaint-in-non-content-editable-element.html

    fast/spatial-navigation

    fast/text/zero-font-size.html

    fast/viewport/viewport-128.html

    http/tests/appcache/disabled.html

    http/tests/canvas/webgl/origin-clean-conformance.html

    http/tests/inspector/network/ping.html

    http/tests/media/reload-after-dialog.html

    http/tests/misc/favicon-loads-with-icon-loading-override.html

    http/tests/navigation/go-back-to-error-page.html

    http/tests/navigation/ping-cookie.html

    http/tests/navigation/ping-cross-origin.html

    http/tests/navigation/ping-same-origin.html

    loader/go-back-to-different-window-size.html

    media/restore-from-page-cache.html
------- Comment #1 From 2012-04-05 05:50:32 PST -------
platform/mac/fast/repaint/region-painting-invalidation-expected.png can be rebaselined and taken out of test_expectations.txt as well after this one works. (I am currently putting it there).
------- Comment #2 From 2012-04-11 03:07:13 PST -------
Created an attachment (id=136650) [details]
Implementation of LayoutTestController::overridePreference + common ewk_view settings interface

Provided common getter and setter interface for ewk_view settings.
Added 3 new ewk_view settings: EWK_VIEW_SETTING_ENABLE_WEBGL, EWK_VIEW_SETTING_HYPERLINK_AUDITING, EWK_VIEW_SETTING_TAB_TO_LINKS.
Added EWK_VIEW_SETTING_TAB_TO_LINKS ewk_view setting support to ChromeClientEfl::keyboardUIMode().
Added implementation of LayoutTestController::overridePreference()
------- Comment #3 From 2012-04-11 06:33:12 PST -------
(From update of attachment 136650 [details])
The ewk_view_setting_{get,set}() stuff isn't very EFL-like, and is yet another place to maintain when adding/removing settings. I don't like this. 

One could easily write LayoutTestController::overridePreference() by calling directly the already existing ewk_view_setting family of functions, without sacrificing readability at the call site.

Informal r-.
------- Comment #4 From 2012-04-11 07:10:34 PST -------
(In reply to comment #3)
> (From update of attachment 136650 [details] [details])
> The ewk_view_setting_{get,set}() stuff isn't very EFL-like, and is yet another place to maintain when adding/removing settings. I don't like this. 
> 
> One could easily write LayoutTestController::overridePreference() by calling directly the already existing ewk_view_setting family of functions, without sacrificing readability at the call site.
> 
> Informal r-.

Not all the necessary settings are currently present in ewk_view_setting family. 
Keeping the old approach would require adding of 6 new functions to the ewk_view interface (for 3 new settings). Solving of https://bugs.webkit.org/show_bug.cgi?id=83687 will also require adding of 2 more functions and so on.. 
Isn't it better to have a common setter/getter interface for all the settings?
The whole old ewk_view_setting family could be removed and it would significantly  decrease the size of ewk_view header file and improve its readability.
------- Comment #5 From 2012-04-11 07:24:48 PST -------
(In reply to comment #3)
> (From update of attachment 136650 [details] [details])
> The ewk_view_setting_{get,set}() stuff isn't very EFL-like, and is yet another place to maintain when adding/removing settings. I don't like this. 
> 
> One could easily write LayoutTestController::overridePreference() by calling directly the already existing ewk_view_setting family of functions, without sacrificing readability at the call site.
> 
> Informal r-.

Mikhail's approach has been up for discussion before this patch was submitted, pls compare: https://lists.webkit.org/pipermail/webkit-efl/2012-April/000158.html 
Feedback there had been rather positive.

As you say, it is or becomes a "family" of functions, but a large one, given that there would be about 8 more functions to be added at least.

Could you reconsider your r-? It may not be 100% EFL style, but IMHO it's an improvement over adding more individual functions.

So, in a next step, the existing family of functions would be migrated to this approach.
------- Comment #6 From 2012-04-11 07:47:23 PST -------
(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 136650 [details] [details] [details])
> > The ewk_view_setting_{get,set}() stuff isn't very EFL-like, and is yet another place to maintain when adding/removing settings. I don't like this. 
> > 
> > One could easily write LayoutTestController::overridePreference() by calling directly the already existing ewk_view_setting family of functions, without sacrificing readability at the call site.
> > 
> > Informal r-.
> 
> Not all the necessary settings are currently present in ewk_view_setting family. 
> Keeping the old approach would require adding of 6 new functions to the ewk_view interface (for 3 new settings). Solving of https://bugs.webkit.org/show_bug.cgi?id=83687 will also require adding of 2 more functions and so on.. 
> Isn't it better to have a common setter/getter interface for all the settings?
> The whole old ewk_view_setting family could be removed and it would significantly  decrease the size of ewk_view header file and improve its readability.

Is it adding more functions for settings that bad?

If we use one function to set all the properties we are not going to reduce the amount of code, just the size of the header, but I don't think we really need that. The chromium port, as an example, has one function per setting too, similar to us.

I also don't think we should move webkit-efl exposed API away from the EFL coding style. This would just make things less consistent with the rest of the libraries.
------- Comment #7 From 2012-04-11 07:48:28 PST -------
(In reply to comment #5)
> (In reply to comment #3)
> > (From update of attachment 136650 [details] [details] [details])
> > The ewk_view_setting_{get,set}() stuff isn't very EFL-like, and is yet another place to maintain when adding/removing settings. I don't like this. 
> > 
> > One could easily write LayoutTestController::overridePreference() by calling directly the already existing ewk_view_setting family of functions, without sacrificing readability at the call site.
> > 
> > Informal r-.
> 
> Mikhail's approach has been up for discussion before this patch was submitted, pls compare: https://lists.webkit.org/pipermail/webkit-efl/2012-April/000158.html 
> Feedback there had been rather positive.

Ok, we can move the discussion back to the ML if necessary. I'm going to answer to that thread too.

Regards,
Rafael
------- Comment #8 From 2012-04-15 14:11:26 PST -------
Created an attachment (id=137249) [details]
Implementation of LayoutTestController::overridePreference + adding of 3 new ewk_view settings
------- Comment #9 From 2012-04-15 19:09:08 PST -------
(From update of attachment 137249 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=137249&action=review

> Source/WebKit/efl/ewk/ewk_view.cpp:212
> +        bool hyperlinkAuditingEnabled : 1;

I'm not sure whether we need to add new API for hyperlinkAuditing setting. Are you sure ?

> Source/WebKit/efl/ewk/ewk_view.cpp:222
> +        bool tabsToLinks : 1;

ditto.

> Source/WebKit/efl/ewk/ewk_view.cpp:223
> +        bool webGLEnabled : 1;

We already have compile option for WebGL. When do we need to turn on/off webgl dynamically ?
------- Comment #10 From 2012-04-16 00:56:41 PST -------
(In reply to comment #9)
> (From update of attachment 137249 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137249&action=review
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:212
> > +        bool hyperlinkAuditingEnabled : 1;
> 
> I'm not sure whether we need to add new API for hyperlinkAuditing setting. Are you sure ?
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:222
> > +        bool tabsToLinks : 1;
> 
> ditto.
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:223
> > +        bool webGLEnabled : 1;
> 
> We already have compile option for WebGL. When do we need to turn on/off webgl dynamically ?

Another solution could be off course to put these settings to DumpRenderTreeSupportEfl(just for enabling LayoutTestController overridePreference), however 
these settings are exported in other ports: GTK, Qt, mac, win, so 
I decided to keep efl port consistent with other ones.
------- Comment #11 From 2012-04-16 07:19:39 PST -------
Created an attachment (id=137343) [details]
added better description of new ewk_view settings
------- Comment #12 From 2012-04-19 07:14:35 PST -------
(In reply to comment #11)
> Created an attachment (id=137343) [details] [details]
> added better description of new ewk_view settings

The patch looks good to me.

Regards,
Rafael
------- Comment #13 From 2012-04-19 12:14:10 PST -------
Created an attachment (id=137946) [details]
Implementation of LayoutTestController::overridePreference
------- Comment #14 From 2012-04-19 12:19:00 PST -------
(In reply to comment #13)
> Created an attachment (id=137946) [details] [details]
> Implementation of LayoutTestController::overridePreference
The same as in previous version but rebased and splitted.
------- Comment #15 From 2012-04-19 12:24:30 PST -------
(From update of attachment 137946 [details])
Attachment 137946 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12456016
------- Comment #16 From 2012-04-19 12:40:17 PST -------
(From update of attachment 137946 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=137946&action=review

> Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:652
> +static bool toBool(JSStringRef value)

Could be inline.

> Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:655
> +    const JSC::UString& uVal = value->ustring();
> +    return (uVal == "true" || uVal == "1");

You can simplify this snippet by using the equals() overloads defined in JSStringUtils.cpp:
  return equals(value, "true") || equals(value, "1");

> Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:658
> +static int toInt(JSStringRef value)

Why not atoi(3)?
------- Comment #17 From 2012-04-19 13:12:33 PST -------
Created an attachment (id=137957) [details]
Implementation of LayoutTestController::overridePreference
------- Comment #18 From 2012-04-19 13:14:37 PST -------
Attachment 137957 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:664:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #19 From 2012-04-19 13:18:50 PST -------
Created an attachment (id=137961) [details]
Implementation of LayoutTestController::overridePreference
------- Comment #20 From 2012-04-19 13:25:58 PST -------
(In reply to comment #16)
> (From update of attachment 137946 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137946&action=review
> 
> > Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:652
> > +static bool toBool(JSStringRef value)
> 
> Could be inline.
Fixed.
> 
> > Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:655
> > +    const JSC::UString& uVal = value->ustring();
> > +    return (uVal == "true" || uVal == "1");
>
Doesn't it bring 1 more conversion for "val" to UString?
Fixed.

> You can simplify this snippet by using the equals() overloads defined in JSStringUtils.cpp:
>   return equals(value, "true") || equals(value, "1");
> 
> > Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:658
> > +static int toInt(JSStringRef value)
> 
> Why not atoi(3)?
I had impression (probably wrong) that Eina_Value is an "official" mean for converting one type to another in EFL.
Fixed.
------- Comment #21 From 2012-04-19 13:33:55 PST -------
(From update of attachment 137961 [details])
Attachment 137961 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12466010
------- Comment #22 From 2012-04-19 13:57:08 PST -------
(From update of attachment 137961 [details])
Looks fine, thanks.
------- Comment #23 From 2012-04-19 14:14:52 PST -------
(From update of attachment 137961 [details])
LGTM.  Looks like this breaks EFL build though.  rs=me, if you fix the EFL bot.
------- Comment #24 From 2012-04-19 14:46:59 PST -------
This patch is depended on Bug 84340. However, Bug 84340 needs to be splited into 3 bugs. When you land patches of Bug 84340, I will set cq+ for this patch.
------- Comment #25 From 2012-04-20 04:16:38 PST -------
We had an issue in bug 82860, fixed in 84430, with missing resets in DRT. Can you check that your new settings are reset before each test?
------- Comment #26 From 2012-04-20 06:08:35 PST -------
Created an attachment (id=138077) [details]
Implementation of LayoutTestController::overridePreference + reset default values
------- Comment #27 From 2012-04-20 06:10:23 PST -------
(In reply to comment #25)
> We had an issue in bug 82860, fixed in 84430, with missing resets in DRT. Can you check that your new settings are reset before each test?

Thanks for the hint! Corrected in the new patch.
------- Comment #28 From 2012-04-20 06:16:18 PST -------
(From update of attachment 138077 [details])
Attachment 138077 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12469214
------- Comment #29 From 2012-05-01 00:28:18 PST -------
Mikhail, I'm adding support for the "WebKitLoadSiteIconsKey" setting overriding at Bug 85171. Our patches are complementary so I thought you should be aware.
------- Comment #30 From 2012-05-02 08:31:24 PST -------
Created an attachment (id=139825) [details]
to be landed.
------- Comment #31 From 2012-05-02 09:51:17 PST -------
(From update of attachment 139825 [details])
Clearing flags on attachment: 139825

Committed r115843: <http://trac.webkit.org/changeset/115843>
------- Comment #32 From 2012-05-02 09:51:24 PST -------
All reviewed patches have been landed.  Closing bug.