Bug 82860 - [EFL] Add setting API for author and user styles.
Summary: [EFL] Add setting API for author and user styles.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-02 00:10 PDT by Kamil Blank
Modified: 2012-04-20 02:23 PDT (History)
13 users (show)

See Also:


Attachments
patch (7.23 KB, patch)
2012-04-02 00:10 PDT, Kamil Blank
no flags Details | Formatted Diff | Diff
patch (7.24 KB, patch)
2012-04-02 00:15 PDT, Kamil Blank
no flags Details | Formatted Diff | Diff
patch (5.46 KB, patch)
2012-04-05 07:01 PDT, Kamil Blank
no flags Details | Formatted Diff | Diff
patch (5.47 KB, patch)
2012-04-05 08:48 PDT, Kamil Blank
dbates: review+
Details | Formatted Diff | Diff
patch (5.46 KB, patch)
2012-04-19 22:50 PDT, Kamil Blank
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kamil Blank 2012-04-02 00:10:01 PDT
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.
Comment 1 Kamil Blank 2012-04-02 00:15:07 PDT
Created attachment 135030 [details]
patch

Added missing bug id
Comment 2 Gyuyoung Kim 2012-04-02 02:30:07 PDT
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.
Comment 3 Kamil Blank 2012-04-04 00:58:02 PDT
(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?
Comment 4 Raphael Kubo da Costa (:rakuco) 2012-04-04 07:02:05 PDT
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.
Comment 5 Gyuyoung Kim 2012-04-04 20:13:54 PDT
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.
Comment 6 Kamil Blank 2012-04-05 06:59:46 PDT
(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
Comment 7 Kamil Blank 2012-04-05 07:01:19 PDT
Created attachment 135817 [details]
patch
Comment 8 Kamil Blank 2012-04-05 08:48:55 PDT
Created attachment 135834 [details]
patch

Patch rebased.
Comment 9 Raphael Kubo da Costa (:rakuco) 2012-04-05 08:58:15 PDT
Comment on attachment 135834 [details]
patch

Looks good, thanks. Don't forget to flip the r? flag for this patch.
Comment 10 Gyuyoung Kim 2012-04-05 21:37:53 PDT
Comment on attachment 135834 [details]
patch

Looks good to me too. Thanks.
Comment 11 Kamil Blank 2012-04-06 00:56:37 PDT
CC'ing reviewers.
Comment 12 Grzegorz Czajkowski 2012-04-10 00:42:46 PDT
LGTM.
Comment 13 Grzegorz Czajkowski 2012-04-17 02:49:06 PDT
Hello Ryosuke,
Could you review this patch? It already has positive informal review.
Comment 14 Daniel Bates 2012-04-19 16:18:44 PDT
Comment on attachment 135834 [details]
patch

Looks sane to me.
Comment 15 Daniel Bates 2012-04-19 16:21:22 PDT
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.
Comment 16 Daniel Bates 2012-04-19 16:24:58 PDT
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().
Comment 17 Kamil Blank 2012-04-19 22:48:52 PDT
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".
Comment 18 Kamil Blank 2012-04-19 22:50:12 PDT
Created attachment 138042 [details]
patch
Comment 19 Grzegorz Czajkowski 2012-04-19 23:31:36 PDT
Patch is ready for landing.
Kubo, Gyuyoung could you give cq+ ?
Comment 20 WebKit Review Bot 2012-04-20 00:27:42 PDT
Comment on attachment 138042 [details]
patch

Clearing flags on attachment: 138042

Committed r114722: <http://trac.webkit.org/changeset/114722>
Comment 21 WebKit Review Bot 2012-04-20 00:27:49 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 Dominik Röttsches (drott) 2012-04-20 01:13:14 PDT
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.
Comment 23 Dominik Röttsches (drott) 2012-04-20 01:50:37 PDT
As discussed on IRC, this might caused by a missing reset statement for the new setting in
void DumpRenderTreeChrome::resetDefaultsToConsistentValues()
Comment 24 Kamil Blank 2012-04-20 02:23:14 PDT
(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