| Summary: | [EFL] Add setting API for author and user styles. | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Kamil Blank <k.blank> | ||||||||||||
| Component: | WebKit EFL | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | dbates, d-r, g.czajkowski, gyuyoung.kim, kenneth, kling, lucas.de.marchi, morrita, mrobinson, rakuco, rniwa, tonikitoo, webkit.review.bot | ||||||||||||
| Priority: | P2 | ||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||
| Hardware: | PC | ||||||||||||||
| OS: | Linux | ||||||||||||||
| Attachments: |
|
||||||||||||||
Created attachment 135030 [details]
patch
Added missing bug id
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 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 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 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. (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 Created attachment 135817 [details]
patch
Created attachment 135834 [details]
patch
Patch rebased.
Comment on attachment 135834 [details]
patch
Looks good, thanks. Don't forget to flip the r? flag for this patch.
Comment on attachment 135834 [details]
patch
Looks good to me too. Thanks.
CC'ing reviewers. LGTM. Hello Ryosuke, Could you review this patch? It already has positive informal review. Comment on attachment 135834 [details]
patch
Looks sane to me.
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 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(). 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". Created attachment 138042 [details]
patch
Patch is ready for landing. Kubo, Gyuyoung could you give cq+ ? Comment on attachment 138042 [details] patch Clearing flags on attachment: 138042 Committed r114722: <http://trac.webkit.org/changeset/114722> All reviewed patches have been landed. Closing bug. 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. As discussed on IRC, this might caused by a missing reset statement for the new setting in void DumpRenderTreeChrome::resetDefaultsToConsistentValues() (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 |
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.