Summary: | [EFL] Refactor theme to choose whether to support foreground color of selection | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryuan Choi <ryuan.choi> | ||||||||||||||||||
Component: | WebKit EFL | Assignee: | Ryuan Choi <ryuan.choi> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | gyuyoung.kim, lucas.de.marchi, rakuco, webkit.review.bot, yael | ||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||
Bug Depends on: | 102481 | ||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||
Attachments: |
|
Description
Ryuan Choi
2012-11-12 19:42:39 PST
Created attachment 173800 [details]
Patch
Comment on attachment 173800 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173800&action=review > Source/WebCore/ChangeLog:10 > + which isselected. s/isselected/is selected/ Created attachment 173810 [details]
Patch
Comment on attachment 173810 [details]
Patch
It looks this patch may influence on pixel-test. Did you check this ?
(In reply to comment #4) > (From update of attachment 173810 [details]) > It looks this patch may influence on pixel-test. Did you check this ? I will check, but it should not change anything. Default theme provides same foreground color. Comment on attachment 173810 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173810&action=review Any idea how to test this code new code path ? > Source/WebCore/ChangeLog:8 > + Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!). You should remove this line. Comment on attachment 173810 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173810&action=review > Source/WebCore/ChangeLog:8 > + Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!). Remove this comment. > Source/WebCore/platform/efl/RenderThemeEfl.h:109 > + virtual bool supportsSelectionForegroundColors() const; I prefer to put same functions in a place. So, it looks this can be moved to 'bool supportsControlTints()' below. virtual bool supportsControlTints() const { return true; } Created attachment 173839 [details]
Patch
(In reply to comment #6) > (From update of attachment 173810 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=173810&action=review > > Any idea how to test this code new code path ? > > > Source/WebCore/ChangeLog:8 > > + Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!). > > You should remove this line. Oops it's mistake. removed.(In reply to comment #7) > (From update of attachment 173810 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=173810&action=review > > > Source/WebCore/ChangeLog:8 > > + Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!). > > Remove this comment. Done, sorry. > > > Source/WebCore/platform/efl/RenderThemeEfl.h:109 > > + virtual bool supportsSelectionForegroundColors() const; > > I prefer to put same functions in a place. So, it looks this can be moved to 'bool supportsControlTints()' below. > > virtual bool supportsControlTints() const { return true; } I tried to keep the similar position with base class. But I can move this if you want. (In reply to comment #9) > I tried to keep the similar position with base class. > But I can move this if you want. Yes, I prefer it. Created attachment 173854 [details]
Patch
Comment on attachment 173854 [details]
Patch
Thank you kangil.
Comment on attachment 173854 [details] Patch Clearing flags on attachment: 173854 Committed r134694: <http://trac.webkit.org/changeset/134694> All reviewed patches have been landed. Closing bug. The API test ewk_test2_view is asserting on _ASSERT_ON_RELEASE_RETURN_VAL(ok, false, "Could not get color class '%s'\n", colorClass); Do we need to change the test after this patch? Re-opened since this is blocked by bug 102481 (In reply to comment #15) > The API test ewk_test2_view is asserting on _ASSERT_ON_RELEASE_RETURN_VAL(ok, false, "Could not get color class '%s'\n", colorClass); > > Do we need to change the test after this patch? I missed to change big_button.edj not to have unnecessary warning. But I think that it would be just additional warning, not fail case of API test. Anyway, I will make patch including bug_button.edj Created attachment 174953 [details]
Patch
Laszlo's question in comment #6 has not been answered yet -- any idea how to test this? It's still not clear to me from the ChangeLog what this is actually fixing. (In reply to comment #19) > Laszlo's question in comment #6 has not been answered yet -- any idea how to test this? It's still not clear to me from the ChangeLog what this is actually fixing. Sorry, I didn't catch it. For test, I don't have idea now. IMHO, to tests this, we make pixel tests for API tests (or any framework to test theme file). It's because this changes just affect painting of selected paragraph. WebCore choose foreground color of selection if it was given. If not, it will paint default color of selected text. But, we can not have a way not to give foreground color to WebCore. So, this patch adds a new feature to disable foreground color to paint default color of selected text. I will consider to update change log. Thanks. Created attachment 175229 [details]
Patch
Created attachment 176455 [details]
Patch
Comment on attachment 176455 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176455&action=review > Source/WebCore/ChangeLog:12 > + However, EFL port does not have a way to disable supports of foreground "support" > Source/WebCore/ChangeLog:14 > + active and inactive to foreground and background in order to disable supports ditto > Source/WebKit2/ChangeLog:8 > + * UIProcess/API/efl/tests/resources/big_button_theme.edj: Recreated. We really should not have a binary in our tree. Why don't you upload the edc instead and compile it with the others? At the moment, you are the only one who can update this file. Created attachment 176464 [details]
Patch
(In reply to comment #23) > (From update of attachment 176455 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=176455&action=review > > > Source/WebCore/ChangeLog:12 > > + However, EFL port does not have a way to disable supports of foreground > > "support" > > > Source/WebCore/ChangeLog:14 > > + active and inactive to foreground and background in order to disable supports > > ditto > Thank you, I fixed. > > Source/WebKit2/ChangeLog:8 > > + * UIProcess/API/efl/tests/resources/big_button_theme.edj: Recreated. > > We really should not have a binary in our tree. Why don't you upload the edc instead and compile it with the others? > At the moment, you are the only one who can update this file. Good point. I agree with you. I created Bug 103511 for it and removed changes from this bug because it does not make regression and print additional warning anymore. Comment on attachment 176464 [details] Patch Clearing flags on attachment: 176464 Committed r136005: <http://trac.webkit.org/changeset/136005> All reviewed patches have been landed. Closing bug. |