Bug 93838

Summary: [EFL] Use vertical slider theme when the slider is vertical
Product: WebKit Reporter: KwangYong Choi <ky0.choi>
Component: WebKit EFLAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: gyuyoung.kim, kenneth, lucas.de.marchi, rakuco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: Linux   
Bug Depends on:    
Bug Blocks: 93449    
Attachments:
Description Flags
Patch
none
Patch none

Description KwangYong Choi 2012-08-13 06:35:40 PDT
Applied vertical slider theme to the vertical sliders. The height of the vertical slider is wrong when it's applied horizontal slider theme.

I changed the default width value of vertical slider as 129 px. All the other ports used 129 px as well. It does not change existing test result of range input type. But, it will pass the test which is range type of input with datalist.
Comment 1 KwangYong Choi 2012-08-21 03:25:53 PDT
Created attachment 159641 [details]
Patch

Modified to use vertical slider theme when the slider appearance is vertical. And do not resize slider when its size is not specified. It's natural to use the default value of renderer.

Updated expected result files. The size of the renderSlider is changed to 129x12 from 129x11 because it has the slider thumb 12x12 and it is not resized when the size is not specified.
Comment 2 Raphael Kubo da Costa (:rakuco) 2012-08-21 20:01:15 PDT
Comment on attachment 159641 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        And do not resize slider when its size is not specified. It's natural
> +        to use the default value of renderer.

What if you don't resize the slider at all, like the other ports seem to do? Do you get meaningful test results?

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:769
> +    const struct ThemePartDesc* desc;

I know this construction was already like this, but since you are touch this part you can leave out the `struct' part, it's an unneeded C-ism.
Comment 3 KwangYong Choi 2012-08-21 22:47:58 PDT
Comment on attachment 159641 [details]
Patch

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

>> Source/WebCore/ChangeLog:13
>> +        to use the default value of renderer.
> 
> What if you don't resize the slider at all, like the other ports seem to do? Do you get meaningful test results?

The size should be set to default value when the size is not specified. The height of horizontal slider is always 11 and the width of the vertical slider is always 11 without checking the size is greater than 0. It's always 11 even though the size of slider thumb is changed.

Render slider uses defaultTrackLength as 129. And, other ports show same width of vertical slider when I see the expected result. So, the size of slider on EFL should look same as the other ports.

The main problem of existing code is the height of the vertical slider is 11 because it is applied horizontal slider theme and resized by code.

>> Source/WebCore/platform/efl/RenderThemeEfl.cpp:769
>> +    const struct ThemePartDesc* desc;
> 
> I know this construction was already like this, but since you are touch this part you can leave out the `struct' part, it's an unneeded C-ism.

You mean, remove const, right?
Comment 4 Chris Dumez 2012-08-21 22:51:11 PDT
Comment on attachment 159641 [details]
Patch

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

>>> Source/WebCore/platform/efl/RenderThemeEfl.cpp:769
>>> +    const struct ThemePartDesc* desc;
>> 
>> I know this construction was already like this, but since you are touch this part you can leave out the `struct' part, it's an unneeded C-ism.
> 
> You mean, remove const, right?

No he means use:
ThemePartDesc* desc;

This is valid in C++.
Comment 5 KwangYong Choi 2012-08-21 22:52:38 PDT
(In reply to comment #4)
> (From update of attachment 159641 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159641&action=review
> 
> >>> Source/WebCore/platform/efl/RenderThemeEfl.cpp:769
> >>> +    const struct ThemePartDesc* desc;
> >> 
> >> I know this construction was already like this, but since you are touch this part you can leave out the `struct' part, it's an unneeded C-ism.
> > 
> > You mean, remove const, right?
> 
> No he means use:
> ThemePartDesc* desc;
> 
> This is valid in C++.

OK.
Comment 6 Raphael Kubo da Costa (:rakuco) 2012-08-21 22:59:08 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 159641 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=159641&action=review
> > 
> > >>> Source/WebCore/platform/efl/RenderThemeEfl.cpp:769
> > >>> +    const struct ThemePartDesc* desc;
> > >> 
> > >> I know this construction was already like this, but since you are touch this part you can leave out the `struct' part, it's an unneeded C-ism.
> > > 
> > > You mean, remove const, right?
> > 
> > No he means use:
> > ThemePartDesc* desc;
> > 
> > This is valid in C++.
> 
> OK.

Actually, const ThemePartDesc* desc ;)
Comment 7 KwangYong Choi 2012-08-21 23:01:06 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > (From update of attachment 159641 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=159641&action=review
> > > 
> > > >>> Source/WebCore/platform/efl/RenderThemeEfl.cpp:769
> > > >>> +    const struct ThemePartDesc* desc;
> > > >> 
> > > >> I know this construction was already like this, but since you are touch this part you can leave out the `struct' part, it's an unneeded C-ism.
> > > > 
> > > > You mean, remove const, right?
> > > 
> > > No he means use:
> > > ThemePartDesc* desc;
> > > 
> > > This is valid in C++.
> > 
> > OK.
> 
> Actually, const ThemePartDesc* desc ;)

But, is it really required? All the other parts using 'struct ThemePartDesc'. It breaks code consistency.
Comment 8 Raphael Kubo da Costa (:rakuco) 2012-08-21 23:03:17 PDT
(In reply to comment #7) 
> > Actually, const ThemePartDesc* desc ;)
> 
> But, is it really required? All the other parts using 'struct ThemePartDesc'. It breaks code consistency.

It's not _required_, as the code will not break if you leave it there. But it makes sense to clean up stuff when you are changing it (I personally prefer this approach to those sweeping commits which change a lot of lines at once).
Comment 9 Raphael Kubo da Costa (:rakuco) 2012-08-21 23:03:41 PDT
(In reply to comment #3)
> And, other ports show same width of vertical slider when I see the expected result. So, the size of slider on EFL should look same as the other ports.

What I wonder is why the other ports work without having to perform these calls in this method.
Comment 10 KwangYong Choi 2012-08-21 23:09:07 PDT
(In reply to comment #9)
> (In reply to comment #3)
> > And, other ports show same width of vertical slider when I see the expected result. So, the size of slider on EFL should look same as the other ports.
> 
> What I wonder is why the other ports work without having to perform these calls in this method.

(In reply to comment #8)
> (In reply to comment #7) 
> > > Actually, const ThemePartDesc* desc ;)
> > 
> > But, is it really required? All the other parts using 'struct ThemePartDesc'. It breaks code consistency.
> 
> It's not _required_, as the code will not break if you leave it there. But it makes sense to clean up stuff when you are changing it (I personally prefer this approach to those sweeping commits which change a lot of lines at once).

That's a good hint for me. May I file a bug for changing 'struct ThemePartDesc' to 'ThemePartDesc'?
Comment 11 Raphael Kubo da Costa (:rakuco) 2012-08-21 23:16:51 PDT
(In reply to comment #10)
> That's a good hint for me. May I file a bug for changing 'struct ThemePartDesc' to 'ThemePartDesc'?

That's exactly the sort of change I don't really like, as I mentioned in my comment ;) It's fixing what's not broken and uses time that could be invested in real bug fixing or implementing new features; but feel free to do that if you will, as it doesn't hurt either.
Comment 12 KwangYong Choi 2012-08-21 23:19:43 PDT
(In reply to comment #9)
> (In reply to comment #3)
> > And, other ports show same width of vertical slider when I see the expected result. So, the size of slider on EFL should look same as the other ports.
> 
> What I wonder is why the other ports work without having to perform these calls in this method.

The reason why I keep the resize code because it's already exist.

I think it's a platform specific thing. The slider can be displayed without resize even though its size is too small. In this case, render slider should be too small to display the slider inside it and it will affect layout of other elements.
Comment 13 KwangYong Choi 2012-08-21 23:22:48 PDT
(In reply to comment #11)
> (In reply to comment #10)
> > That's a good hint for me. May I file a bug for changing 'struct ThemePartDesc' to 'ThemePartDesc'?
> 
> That's exactly the sort of change I don't really like, as I mentioned in my comment ;) It's fixing what's not broken and uses time that could be invested in real bug fixing or implementing new features; but feel free to do that if you will, as it doesn't hurt either.

Oh, I have misunderstanding.

The exising code will be changed when I change there.
Comment 14 Raphael Kubo da Costa (:rakuco) 2012-08-21 23:23:58 PDT
(In reply to comment #12)
> (In reply to comment #9)
> > (In reply to comment #3)
> > > And, other ports show same width of vertical slider when I see the expected result. So, the size of slider on EFL should look same as the other ports.
> > 
> > What I wonder is why the other ports work without having to perform these calls in this method.
> 
> The reason why I keep the resize code because it's already exist.
> 
> I think it's a platform specific thing. The slider can be displayed without resize even though its size is too small. In this case, render slider should be too small to display the slider inside it and it will affect layout of other elements.

OK; my knowledge of this part isn't very large, so I trust your judgement here.
Comment 15 Gyuyoung Kim 2012-08-21 23:31:05 PDT
(In reply to comment #11)
> (In reply to comment #10)
> > That's a good hint for me. May I file a bug for changing 'struct ThemePartDesc' to 'ThemePartDesc'?
> 
> That's exactly the sort of change I don't really like, as I mentioned in my comment ;) It's fixing what's not broken and uses time that could be invested in real bug fixing or implementing new features; but feel free to do that if you will, as it doesn't hurt either.

KwangYoung, as far as I know, I was told WebKit culturally doesn't fix only style errors. But, I'm not sure if this is related to coding style. If you wanna fix them, I don't also mind. If possible, as Kubo said, this is better to fix with other bugs.
Comment 16 KwangYong Choi 2012-08-21 23:33:00 PDT
(In reply to comment #15)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > That's a good hint for me. May I file a bug for changing 'struct ThemePartDesc' to 'ThemePartDesc'?
> > 
> > That's exactly the sort of change I don't really like, as I mentioned in my comment ;) It's fixing what's not broken and uses time that could be invested in real bug fixing or implementing new features; but feel free to do that if you will, as it doesn't hurt either.
> 
> KwangYoung, as far as I know, I was told WebKit culturally doesn't fix only style errors. But, I'm not sure if this is related to coding style. If you wanna fix them, I don't also mind. If possible, as Kubo said, this is better to fix with other bugs.

OK. Thank you for your advice.
Comment 17 KwangYong Choi 2012-08-21 23:35:41 PDT
Created attachment 159869 [details]
Patch

Using "const ThemePartDesc* desc;"
Comment 18 KwangYong Choi 2012-08-21 23:36:48 PDT
Why this patch makes 404 error? :(
Comment 19 Raphael Kubo da Costa (:rakuco) 2012-08-21 23:38:12 PDT
Comment on attachment 159869 [details]
Patch

Looks good, thanks.
Comment 20 Thiago Marcos P. Santos 2012-08-23 00:33:22 PDT
Comment on attachment 159869 [details]
Patch

LGTM
Comment 21 WebKit Review Bot 2012-08-23 00:54:20 PDT
Comment on attachment 159869 [details]
Patch

Clearing flags on attachment: 159869

Committed r126401: <http://trac.webkit.org/changeset/126401>
Comment 22 WebKit Review Bot 2012-08-23 00:54:25 PDT
All reviewed patches have been landed.  Closing bug.