Bug 125034 - [EFL] Implement scrollbarThickness for opaque scrollbar
Summary: [EFL] Implement scrollbarThickness for opaque scrollbar
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryuan Choi
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-11-30 18:02 PST by Ryuan Choi
Modified: 2013-11-30 23:03 PST (History)
5 users (show)

See Also:

Attachments
Patch (4.16 KB, patch)
2013-11-30 18:12 PST, Ryuan Choi 		no flags Details | Formatted Diff | Diff
Patch (4.31 KB, patch)
2013-11-30 18:44 PST, Ryuan Choi 		no flags Details | Formatted Diff | Diff
Patch (4.51 KB, patch)
2013-11-30 21:55 PST, Ryuan Choi 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Ryuan Choi 2013-11-30 18:02:08 PST 
WebKitEfl draws transparent scrollbar on the top of contents using edje.

This patch give a chance to support opaque scrollbar by edje.
Comment 1 Ryuan Choi 2013-11-30 18:12:13 PST 
Created attachment 218095 [details]
Patch
Comment 2 Ryuan Choi 2013-11-30 18:44:34 PST 
Created attachment 218097 [details]
Patch
Comment 3 Gyuyoung Kim 2013-11-30 21:28:04 PST 
Comment on attachment 218097 [details]
Patch

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

It looks this patch need to be verified if there is any layout test regression.

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:518
> +        ScrollbarThemeEfl* scrollbarTheme = static_cast<ScrollbarThemeEfl*>(ScrollbarTheme::theme());

Don't we need to use local variable as below ?
static_cast<ScrollbarThemeEfl*>(ScrollbarTheme::theme())->setScrollbarThickness(atoi(thickness));

> Source/WebCore/platform/efl/ScrollbarThemeEfl.h:39
> +    virtual int scrollbarThickness(ScrollbarControlSize = RegularScrollbar) { return m_scrollbarThickness; }

Missing OVERRIDE FINAL keyword.

> Source/WebCore/platform/efl/ScrollbarThemeEfl.h:41
> +    virtual void registerScrollbar(ScrollbarThemeClient*) { }

ditto.

> Source/WebCore/platform/efl/ScrollbarThemeEfl.h:42
> +    virtual void unregisterScrollbar(ScrollbarThemeClient*) { }

ditto.
Comment 4 Ryuan Choi 2013-11-30 21:55:55 PST 
Created attachment 218103 [details]
Patch
Comment 5 Ryuan Choi 2013-11-30 21:57:20 PST 
(In reply to comment #3)
> (From update of attachment 218097 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=218097&action=review
> 
> It looks this patch need to be verified if there is any layout test regression.
> 
Because this does not change with default theme, it must be fine.

> > Source/WebCore/platform/efl/RenderThemeEfl.cpp:518
> > +        ScrollbarThemeEfl* scrollbarTheme = static_cast<ScrollbarThemeEfl*>(ScrollbarTheme::theme());
> 
> Don't we need to use local variable as below ?
> static_cast<ScrollbarThemeEfl*>(ScrollbarTheme::theme())->setScrollbarThickness(atoi(thickness));
> 
Sure, fixed

> > Source/WebCore/platform/efl/ScrollbarThemeEfl.h:39
> > +    virtual int scrollbarThickness(ScrollbarControlSize = RegularScrollbar) { return m_scrollbarThickness; }
> 
> Missing OVERRIDE FINAL keyword.
> 
> > Source/WebCore/platform/efl/ScrollbarThemeEfl.h:41
> > +    virtual void registerScrollbar(ScrollbarThemeClient*) { }
> 
> ditto.
> 
> > Source/WebCore/platform/efl/ScrollbarThemeEfl.h:42
> > +    virtual void unregisterScrollbar(ScrollbarThemeClient*) { }
> 
> ditto.

Sure, I did.
Comment 6 Gyuyoung Kim 2013-11-30 22:03:04 PST 
Comment on attachment 218103 [details]
Patch

LGTM.
Comment 7 WebKit Commit Bot 2013-11-30 23:03:03 PST 
Comment on attachment 218103 [details]
Patch

Clearing flags on attachment: 218103

Committed r159897: <http://trac.webkit.org/changeset/159897>
Comment 8 WebKit Commit Bot 2013-11-30 23:03:06 PST 
All reviewed patches have been landed.  Closing bug.