Bug 102037

Summary: [EFL] Refactor theme to choose whether to support foreground color of selection
Product: WebKit Reporter: Ryuan Choi <ryuan.choi>
Component: WebKit EFLAssignee: 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 Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Ryuan Choi 2012-11-12 19:42:39 PST
WebKit/Efl provides a way to change the foreground color of selection.
But, we did not support keeping the text color although text was selected.

Patch will be added.
Comment 1 Ryuan Choi 2012-11-12 19:56:44 PST
Created attachment 173800 [details]
Patch
Comment 2 Jinwoo Song 2012-11-12 20:08:55 PST
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/
Comment 3 Ryuan Choi 2012-11-12 21:28:29 PST
Created attachment 173810 [details]
Patch
Comment 4 Gyuyoung Kim 2012-11-12 21:46:26 PST
Comment on attachment 173810 [details]
Patch

It looks this patch may influence on pixel-test. Did you check this ?
Comment 5 Ryuan Choi 2012-11-12 23:46:19 PST
(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 6 Laszlo Gombos 2012-11-13 00:16:33 PST
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 7 Gyuyoung Kim 2012-11-13 00:23:32 PST
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; }
Comment 8 Ryuan Choi 2012-11-13 00:24:37 PST
Created attachment 173839 [details]
Patch
Comment 9 Ryuan Choi 2012-11-13 00:29:00 PST
(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.
Comment 10 Gyuyoung Kim 2012-11-13 02:02:33 PST
(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.
Comment 11 Ryuan Choi 2012-11-13 02:38:42 PST
Created attachment 173854 [details]
Patch
Comment 12 Ryuan Choi 2012-11-14 16:37:08 PST
Comment on attachment 173854 [details]
Patch

Thank you kangil.
Comment 13 WebKit Review Bot 2012-11-14 16:50:33 PST
Comment on attachment 173854 [details]
Patch

Clearing flags on attachment: 173854

Committed r134694: <http://trac.webkit.org/changeset/134694>
Comment 14 WebKit Review Bot 2012-11-14 16:50:37 PST
All reviewed patches have been landed.  Closing bug.
Comment 15 Yael 2012-11-15 19:16:37 PST
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?
Comment 16 WebKit Review Bot 2012-11-16 04:31:02 PST
Re-opened since this is blocked by bug 102481
Comment 17 Ryuan Choi 2012-11-19 04:12:37 PST
(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
Comment 18 Ryuan Choi 2012-11-19 04:34:41 PST
Created attachment 174953 [details]
Patch
Comment 19 Raphael Kubo da Costa (:rakuco) 2012-11-19 07:30:46 PST
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.
Comment 20 Ryuan Choi 2012-11-19 18:29:29 PST
(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.
Comment 21 Ryuan Choi 2012-11-20 09:08:06 PST
Created attachment 175229 [details]
Patch
Comment 22 Ryuan Choi 2012-11-28 04:33:17 PST
Created attachment 176455 [details]
Patch
Comment 23 Chris Dumez 2012-11-28 05:02:35 PST
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.
Comment 24 Ryuan Choi 2012-11-28 05:40:22 PST
Created attachment 176464 [details]
Patch
Comment 25 Ryuan Choi 2012-11-28 05:45:12 PST
(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 26 WebKit Review Bot 2012-11-28 06:24:05 PST
Comment on attachment 176464 [details]
Patch

Clearing flags on attachment: 176464

Committed r136005: <http://trac.webkit.org/changeset/136005>
Comment 27 WebKit Review Bot 2012-11-28 06:24:13 PST
All reviewed patches have been landed.  Closing bug.