Bug 83007 – [EFL] EFL's LayoutTestController overridePreference implementation

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

[Mikhail Pozdnyakov]

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

[Dominik Röttsches (drott)]

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).

[Mikhail Pozdnyakov]

Created attachment 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()

[Leandro Pereira]

Comment on attachment 136650 [details]
Implementation of LayoutTestController::overridePreference + common ewk_view settings interface

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 Pozdnyakov]

(In reply to comment #3)
> (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-.

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.

[Dominik Röttsches (drott)]

(In reply to comment #3)
> (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-.

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.

[Rafael Antognolli]

(In reply to comment #4)
> (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.

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.

[Rafael Antognolli]

(In reply to comment #5)
> (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.

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

Regards,
Rafael

[Mikhail Pozdnyakov]

Created attachment 137249 [details]
Implementation of LayoutTestController::overridePreference + adding of 3 new ewk_view settings

[Gyuyoung Kim]

Comment on attachment 137249 [details]
Implementation of LayoutTestController::overridePreference + adding of 3 new ewk_view settings

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 ?

[Mikhail Pozdnyakov]

(In reply to comment #9)
> (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 ?

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.

[Mikhail Pozdnyakov]

Created attachment 137343 [details]
added better description of new ewk_view settings

[Rafael Antognolli]

(In reply to comment #11)
> Created an attachment (id=137343) [details]
> added better description of new ewk_view settings

The patch looks good to me.

Regards,
Rafael

[Mikhail Pozdnyakov]

Created attachment 137946 [details]
Implementation of LayoutTestController::overridePreference

[Mikhail Pozdnyakov]

(In reply to comment #13)
> Created an attachment (id=137946) [details]
> Implementation of LayoutTestController::overridePreference
The same as in previous version but rebased and splitted.

[Gyuyoung Kim]

Comment on attachment 137946 [details]
Implementation of LayoutTestController::overridePreference

Attachment 137946 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12456016

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 137946 [details]
Implementation of LayoutTestController::overridePreference

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)?

[Mikhail Pozdnyakov]

Created attachment 137957 [details]
Implementation of LayoutTestController::overridePreference

[WebKit Review Bot]

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.

[Mikhail Pozdnyakov]

Created attachment 137961 [details]
Implementation of LayoutTestController::overridePreference

[Mikhail Pozdnyakov]

(In reply to comment #16)
> (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.
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.

[Gyuyoung Kim]

Comment on attachment 137961 [details]
Implementation of LayoutTestController::overridePreference

Attachment 137961 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12466010

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 137961 [details]
Implementation of LayoutTestController::overridePreference

Looks fine, thanks.

[Eric Seidel (no email)]

Comment on attachment 137961 [details]
Implementation of LayoutTestController::overridePreference

LGTM.  Looks like this breaks EFL build though.  rs=me, if you fix the EFL bot.

[Gyuyoung Kim]

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.

[Dominik Röttsches (drott)]

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?

[Mikhail Pozdnyakov]

Created attachment 138077 [details]
Implementation of LayoutTestController::overridePreference + reset default values

[Mikhail Pozdnyakov]

(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.

[Gyuyoung Kim]

Comment on attachment 138077 [details]
Implementation of LayoutTestController::overridePreference + reset default values

Attachment 138077 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12469214

[Chris Dumez]

Mikhail, I'm adding support for the "WebKitLoadSiteIconsKey" setting overriding at Bug 85171. Our patches are complementary so I thought you should be aware.

[Mikhail Pozdnyakov]

Created attachment 139825 [details]
to be landed.

[WebKit Review Bot]

Comment on attachment 139825 [details]
to be landed.

Clearing flags on attachment: 139825

Committed r115843: <http://trac.webkit.org/changeset/115843>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.