Bug 93838

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 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 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 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++.

(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.

(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 ;)

(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.

(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).

(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 #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'?

(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.

(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.

(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.

(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.

(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.

(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.

Created attachment 159869 [details] Patch Using "const ThemePartDesc* desc;"

Why this patch makes 404 error? :(

Comment on attachment 159869 [details] Patch Looks good, thanks.

Comment on attachment 159869 [details] Patch LGTM

Comment on attachment 159869 [details] Patch Clearing flags on attachment: 159869 Committed r126401: <http://trac.webkit.org/changeset/126401>

All reviewed patches have been landed. Closing bug.