Bug 83007 - [EFL] EFL's LayoutTestController overridePreference implementation
: [EFL] EFL's LayoutTestController overridePreference implementation
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebKit EFL
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Mikhail Pozdnyakov
:
Depends on: 84340 84451 84534
Blocks: 83900
  Show dependency treegraph
 
Reported: 2012-04-03 00:55 PDT by Mikhail Pozdnyakov
Modified: 2012-05-22 11:09 PDT (History)
8 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Mikhail Pozdnyakov 2012-04-03 00:55:00 PDT
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 Dominik Röttsches (drott) 2012-04-05 05:50:32 PDT
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 Mikhail Pozdnyakov 2012-04-11 03:07:13 PDT
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()
Comment 3 Leandro Pereira 2012-04-11 06:33:12 PDT
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-.
Comment 4 Mikhail Pozdnyakov 2012-04-11 07:10:34 PDT
(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.
Comment 5 Dominik Röttsches (drott) 2012-04-11 07:24:48 PDT
(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.
Comment 6 Rafael Antognolli 2012-04-11 07:47:23 PDT
(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.
Comment 7 Rafael Antognolli 2012-04-11 07:48:28 PDT
(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
Comment 8 Mikhail Pozdnyakov 2012-04-15 14:11:26 PDT
Created attachment 137249 [details]
Implementation of LayoutTestController::overridePreference + adding of 3 new ewk_view settings
Comment 9 Gyuyoung Kim 2012-04-15 19:09:08 PDT
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 ?
Comment 10 Mikhail Pozdnyakov 2012-04-16 00:56:41 PDT
(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.
Comment 11 Mikhail Pozdnyakov 2012-04-16 07:19:39 PDT
Created attachment 137343 [details]
added better description of new ewk_view settings
Comment 12 Rafael Antognolli 2012-04-19 07:14:35 PDT
(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
Comment 13 Mikhail Pozdnyakov 2012-04-19 12:14:10 PDT
Created attachment 137946 [details]
Implementation of LayoutTestController::overridePreference
Comment 14 Mikhail Pozdnyakov 2012-04-19 12:19:00 PDT
(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.
Comment 15 Gyuyoung Kim 2012-04-19 12:24:30 PDT
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
Comment 16 Raphael Kubo da Costa (:rakuco) 2012-04-19 12:40:17 PDT
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)?
Comment 17 Mikhail Pozdnyakov 2012-04-19 13:12:33 PDT
Created attachment 137957 [details]
Implementation of LayoutTestController::overridePreference
Comment 18 WebKit Review Bot 2012-04-19 13:14:37 PDT
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 Mikhail Pozdnyakov 2012-04-19 13:18:50 PDT
Created attachment 137961 [details]
Implementation of LayoutTestController::overridePreference
Comment 20 Mikhail Pozdnyakov 2012-04-19 13:25:58 PDT
(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.
Comment 21 Gyuyoung Kim 2012-04-19 13:33:55 PDT
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
Comment 22 Raphael Kubo da Costa (:rakuco) 2012-04-19 13:57:08 PDT
Comment on attachment 137961 [details]
Implementation of LayoutTestController::overridePreference

Looks fine, thanks.
Comment 23 Eric Seidel 2012-04-19 14:14:52 PDT
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.
Comment 24 Gyuyoung Kim 2012-04-19 14:46:59 PDT
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 Dominik Röttsches (drott) 2012-04-20 04:16:38 PDT
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 Mikhail Pozdnyakov 2012-04-20 06:08:35 PDT
Created attachment 138077 [details]
Implementation of LayoutTestController::overridePreference + reset default values
Comment 27 Mikhail Pozdnyakov 2012-04-20 06:10:23 PDT
(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 Gyuyoung Kim 2012-04-20 06:16:18 PDT
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
Comment 29 Chris Dumez 2012-05-01 00:28:18 PDT
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 Mikhail Pozdnyakov 2012-05-02 08:31:24 PDT
Created attachment 139825 [details]
to be landed.
Comment 31 WebKit Review Bot 2012-05-02 09:51:17 PDT
Comment on attachment 139825 [details]
to be landed.

Clearing flags on attachment: 139825

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