Bug 125034

Summary: [EFL] Implement scrollbarThickness for opaque scrollbar
Product: WebKit Reporter: Ryuan Choi <ryuan.choi>
Component: WebKit EFLAssignee: Ryuan Choi <ryuan.choi>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, gyuyoung.kim, lucas.de.marchi, rakuco
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

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.