Bug 82860 – [EFL] Add setting API for author and user styles.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Kamil Blank]

Created attachment 135029 [details]
patch

Add setting API for author and user styles.
Patch contains:
+ API for EWK
+ Enable layout test connected with seAuthorAndUserStylesEnabled.
+ DRT: Implementation of setAuthorAndUserStylesEnabled.

[Kamil Blank]

Created attachment 135030 [details]
patch

Added missing bug id

[Gyuyoung Kim]

Comment on attachment 135030 [details]
patch

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

> Source/WebKit/efl/ewk/ewk_view.h:1950
> +EAPI Eina_Bool    ewk_view_setting_enable_author_and_user_styles_get(const Evas_Object *o);

I'm not sure if we need to add this API for application. It seems to me that this setting feature is only added by chromium port. Could you let me know why we need to add this API ?
If we don't need to add this API, in my opinion, you can add this feature to DumpRenderTreeSupportEFL.

[Kamil Blank]

(In reply to comment #2)
> (From update of attachment 135030 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135030&action=review
> 
> > Source/WebKit/efl/ewk/ewk_view.h:1950
> > +EAPI Eina_Bool    ewk_view_setting_enable_author_and_user_styles_get(const Evas_Object *o);
> 
> I'm not sure if we need to add this API for application. It seems to me that this setting feature is only added by chromium port. Could you let me know why we need to add this API ?
> If we don't need to add this API, in my opinion, you can add this feature to DumpRenderTreeSupportEFL.

In general I added this setting to pass layout test. If you feel that it's not needed to publish such an API I can change it. What is others opinion?

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 135030 [details]
patch

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

> LayoutTests/ChangeLog:8
> +        Enable test connected with seAuthorAndUserStylesEnabled.

Typo in the function name.

> LayoutTests/ChangeLog:11
> +        * platform/efl/fast/css/disabled-author-styles-expected.txt: Added.

Just to confirm, this was created via run-webkit-tests (ie. it used the dependencies built with jhbuild), right?

>>> Source/WebKit/efl/ewk/ewk_view.h:1950
>>> +EAPI Eina_Bool    ewk_view_setting_enable_author_and_user_styles_get(const Evas_Object *o);
>> 
>> I'm not sure if we need to add this API for application. It seems to me that this setting feature is only added by chromium port. Could you let me know why we need to add this API ?
>> If we don't need to add this API, in my opinion, you can add this feature to DumpRenderTreeSupportEFL.
> 
> In general I added this setting to pass layout test. If you feel that it's not needed to publish such an API I can change it. What is others opinion?

Personally, it's unclear to me what "enabling author and user styles" actually means, and the Qt and Win ports don't expose this functionality to end users.

[Gyuyoung Kim]

Comment on attachment 135030 [details]
patch

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

>>>> Source/WebKit/efl/ewk/ewk_view.h:1950
>>>> +EAPI Eina_Bool    ewk_view_setting_enable_author_and_user_styles_get(const Evas_Object *o);
>>> 
>>> I'm not sure if we need to add this API for application. It seems to me that this setting feature is only added by chromium port. Could you let me know why we need to add this API ?
>>> If we don't need to add this API, in my opinion, you can add this feature to DumpRenderTreeSupportEFL.
>> 
>> In general I added this setting to pass layout test. If you feel that it's not needed to publish such an API I can change it. What is others opinion?
> 
> Personally, it's unclear to me what "enabling author and user styles" actually means, and the Qt and Win ports don't expose this functionality to end users.

I think it is better to move this functionality to DumpRenderTreeSupportEfl for only testing. Then, if application needs to use this functionality, we can move this to API layer in future again.

[Kamil Blank]

(In reply to comment #4)
> (From update of attachment 135030 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135030&action=review
> 
> > LayoutTests/ChangeLog:8
> > +        Enable test connected with seAuthorAndUserStylesEnabled.
> 
> Typo in the function name.
Thanks. I'll fix it.

> 
> > LayoutTests/ChangeLog:11
> > +        * platform/efl/fast/css/disabled-author-styles-expected.txt: Added.
> 
> Just to confirm, this was created via run-webkit-tests (ie. it used the dependencies built with jhbuild), right?
Right :)

I'll send new patch with functionality moved to DumpRenderTreeSupportEfl

[Kamil Blank]

Created attachment 135817 [details]
patch

[Kamil Blank]

Created attachment 135834 [details]
patch

Patch rebased.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 135834 [details]
patch

Looks good, thanks. Don't forget to flip the r? flag for this patch.

[Gyuyoung Kim]

Comment on attachment 135834 [details]
patch

Looks good to me too. Thanks.

[Kamil Blank]

CC'ing reviewers.

[Grzegorz Czajkowski]

LGTM.

[Grzegorz Czajkowski]

Hello Ryosuke,
Could you review this patch? It already has positive informal review.

[Daniel Bates]

Comment on attachment 135834 [details]
patch

Looks sane to me.

[Daniel Bates]

Comment on attachment 135834 [details]
patch

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

> Source/WebKit/efl/ChangeLog:9
> +        Add missing implementation setAuthorAndUserStylesEnabled to EFL's
> +        DumpRenderTreeSupport.
> +
> +        Reviewed by NOBODY (OOPS!).

Nit: The format of this change log entry is inconsistent with the format used in the other change log entries in this patch. To be consistent, the "Reviewed by" line should come after the bug URL.

[Daniel Bates]

Comment on attachment 135834 [details]
patch

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

> Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:353
> +void LayoutTestController::setAuthorAndUserStylesEnabled(bool flag)

Nit: You may want to consider substituting "enable" for "flag" for the parameter name so as to be consistent with the name of this parameter in DumpRenderTreeSupportEfl::setAuthorAndUserStylesEnabled().

[Kamil Blank]

Thank you for review!

(In reply to comment #15)
> (From update of attachment 135834 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135834&action=review
> 
> > Source/WebKit/efl/ChangeLog:9
> > +        Add missing implementation setAuthorAndUserStylesEnabled to EFL's
> > +        DumpRenderTreeSupport.
> > +
> > +        Reviewed by NOBODY (OOPS!).
> 
> Nit: The format of this change log entry is inconsistent with the format used in the other change log entries in this patch. To be consistent, the "Reviewed by" line should come after the bug URL.

Thanks. I'll fix it!

(In reply to comment #16)
> (From update of attachment 135834 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135834&action=review
> 
> > Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:353
> > +void LayoutTestController::setAuthorAndUserStylesEnabled(bool flag)
> 
> Nit: You may want to consider substituting "enable" for "flag" for the parameter name so as to be consistent with the name of this parameter in DumpRenderTreeSupportEfl::setAuthorAndUserStylesEnabled().

I'll leave it as it is as almost all functions inside Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp use name "flag".

[Kamil Blank]

Created attachment 138042 [details]
patch

[Grzegorz Czajkowski]

Patch is ready for landing.
Kubo, Gyuyoung could you give cq+ ?

[WebKit Review Bot]

Comment on attachment 138042 [details]
patch

Clearing flags on attachment: 138042

Committed r114722: <http://trac.webkit.org/changeset/114722>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dominik Röttsches (drott)]

After landing this one, test failures spike
http://build.webkit.org/builders/EFL%20Linux%20Debug/builds/326
http://build.webkit.org/builders/EFL%20Linux%20Release/builds/35686

from (debug)
"Failed 124 failures 43 flakes 3 missing results" to 
"Failed 202 failures 191 flakes 3 missing results"

from (release):
"Failed 141 failures 130 flakes 3 missing results" to
"Failed 217 failures 193 flakes 3 missing results"

Kamil, can you please do the appropriate rebaselining if required or check whether this patch introduces regressions. I think we made good progress on bringing the failure numbers down. I wouldn't like to see this trend reverted.

[Dominik Röttsches (drott)]

As discussed on IRC, this might caused by a missing reset statement for the new setting in
void DumpRenderTreeChrome::resetDefaultsToConsistentValues()

[Kamil Blank]

(In reply to comment #23)
> As discussed on IRC, this might caused by a missing reset statement for the new setting in
> void DumpRenderTreeChrome::resetDefaultsToConsistentValues()

This will be fixed here:
https://bugs.webkit.org/show_bug.cgi?id=84430