Bug 143566

Summary: [EFL] Scrollbar is not drawn on MiniBrowser.
Product: WebKit Reporter: Hunseop Jeong <hs85.jeong>
Component: WebKit EFLAssignee: Hunseop Jeong <hs85.jeong>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, gyuyoung.kim, lucas.de.marchi, ossy, ryuan.choi
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
WIP
none
Patch
none
Patch
none
WIP
none
WIP
none
Patch
none
WIP
none
Patch
none
Patch
none
Patch none

Description Hunseop Jeong 2015-04-09 08:53:56 PDT 
Normally, Scrollbar is drawn if the size of content is bigger than window.
But now scrollbar is not shown on MiniBrowser and WebKitTestRunner.
This bug seems to be fixed.
Comment 1 Hunseop Jeong 2015-04-16 21:39:23 PDT 
Created attachment 251002 [details]
WIP
Comment 2 Hunseop Jeong 2015-04-17 22:56:22 PDT 
Created attachment 251084 [details]
Patch
Comment 3 Hunseop Jeong 2015-04-21 05:27:11 PDT 
Created attachment 251227 [details]
Patch
Comment 4 Hunseop Jeong 2015-04-22 07:03:24 PDT 
Created attachment 251311 [details]
WIP
Comment 5 Hunseop Jeong 2015-04-23 21:42:08 PDT 
Created attachment 251535 [details]
WIP
Comment 6 Hunseop Jeong 2015-05-06 00:40:30 PDT 
Created attachment 252459 [details]
Patch
Comment 7 Hunseop Jeong 2015-05-06 00:46:08 PDT 
ERR<5259>:eo lib/eo/eo_ptr_indirection.x:294 _eo_obj_pointer_get() obj_id 0x8000001ae00000d8 is not pointing to a valid object. Maybe it has already been freed.
ERR<5259>:eo lib/eo/eo.c:1694 eo_data_scope_get() Obj (0x8000001ae00000d8) is an invalid ref.
ERR<5259>:ecore lib/ecore/ecore.c:690 _ecore_magic_fail() 
*** ECORE ERROR: Ecore Magic Check Failed!!!
*** IN FUNCTION: ecore_evas_free()
ERR<5259>:ecore lib/ecore/ecore.c:694 _ecore_magic_fail()   Input handle has already been freed!
ERR<5259>:ecore lib/ecore/ecore.c:703 _ecore_magic_fail() *** NAUGHTY PROGRAMMER!!!
*** SPANK SPANK SPANK!!!
*** Now go fix your code. Tut tut tut!
ERR<5259>:eo lib/eo/eo_ptr_indirection.x:294 _eo_obj_pointer_get() obj_id 0x80000019600000cc is not pointing to a valid object. Maybe it has already been freed.
ERR<5259>:eo lib/eo/eo.c:1694 eo_data_scope_get() Obj (0x80000019600000cc) is an invalid ref.
ERR<5259>:ecore lib/ecore/ecore.c:690 _ecore_magic_fail() 
*** ECORE ERROR: Ecore Magic Check Failed!!!
*** IN FUNCTION: ecore_evas_free()
ERR<5259>:ecore lib/ecore/ecore.c:694 _ecore_magic_fail()   Input handle has already been freed!
ERR<5259>:ecore lib/ecore/ecore.c:703 _ecore_magic_fail() *** NAUGHTY PROGRAMMER!!!
*** SPANK SPANK SPANK!!!
*** Now go fix your code. Tut tut tut!
ERR<5259>:eo lib/eo/eo_ptr_indirection.x:294 _eo_obj_pointer_get() obj_id 0x80000017c00000bf is not pointing to a valid object. Maybe it has already been freed.
ERR<5259>:eo lib/eo/eo.c:1694 eo_data_scope_get() Obj (0x80000017c00000bf) is an invalid ref.
ERR<5259>:ecore lib/ecore/ecore.c:690 _ecore_magic_fail() 
*** ECORE ERROR: Ecore Magic Check Failed!!!
*** IN FUNCTION: ecore_evas_free()
ERR<5259>:ecore lib/ecore/ecore.c:694 _ecore_magic_fail()   Input handle has already been freed!
ERR<5259>:ecore lib/ecore/ecore.c:703 _ecore_magic_fail() *** NAUGHTY PROGRAMMER!!!
*** SPANK SPANK SPANK!!!
*** Now go fix your code. Tut tut tut!
ERR<5259>:eo lib/eo/eo_ptr_indirection.x:294 _eo_obj_pointer_get() obj_id 0x80000016200000b2 is not pointing to a valid object. Maybe it has already been freed.
ERR<5259>:eo lib/eo/eo.c:1694 eo_data_scope_get() Obj (0x80000016200000b2) is an invalid ref.
ERR<5259>:ecore lib/ecore/ecore.c:690 _ecore_magic_fail() 
*** ECORE ERROR: Ecore Magic Check Failed!!!
*** IN FUNCTION: ecore_evas_free()


Many upper error message occurred after terminating the minibrowser.
I don't know why this error message occurred.
Could you give me the guide to remove the error message?
I use the EflUniquePtr for Ecore_evas and Evas_Object.
Comment 8 Ryuan Choi 2015-07-07 05:34:46 PDT 
Comment on attachment 252459 [details]
Patch

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

> Source/WebCore/platform/efl/ScrollbarThemeEfl.h:58
> +    void setThemePath(const String&);

I don't think that we can set theme path to scrollbar on time.

Can we just use RenderThemeEfl ?
Comment 9 Hunseop Jeong 2015-07-07 22:34:30 PDT 
Created attachment 256355 [details]
WIP
Comment 10 Hunseop Jeong 2015-07-07 23:21:29 PDT 
Comment on attachment 256355 [details]
WIP

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

> Source/WebCore/platform/efl/ScrollbarThemeEfl.cpp:130
> +void ScrollbarThemeEfl::loadThemeIfNeeded(Scrollbar& scrollbar)
> +{
> +    if (m_theme)
> +        return;
> +
> +    ScrollView* view = scrollbar.parent();
> +    if (!view)
> +        return;
> +
> +    if (!is<FrameView>(view))
> +        return;
> +
> +    Page* page = downcast<FrameView>(view)->frame().page();
> +    if (!page)
> +        return;
> +
> +    m_theme = static_cast<RenderThemeEfl*>(&page->theme());
> +}

ryuan, 

How about this approach?

Get the RenderThemeEfl from the scrollbar, Drawing the scrollbar using the RenderThemeEfl.
If you agree, I will proceed to implement the scrollbar.
Comment 11 Ryuan Choi 2015-07-08 23:41:58 PDT 
Comment on attachment 256355 [details]
WIP

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

> Source/WebCore/platform/efl/DefaultTheme/widget/scrollbar/scrollbar.edc:77
> +            name: "reset";

Is it still required?

> Source/WebCore/platform/efl/DefaultTheme/widget/scrollbar/scrollbar.edc:155
> +               state: "default" 0.0;

you write state two time.

> Source/WebCore/platform/efl/DefaultTheme/widget/scrollbar/scrollbar.edc:161
> +         name: "horizontal.thumb";

indentation looks wrong.

> Source/WebCore/platform/efl/DefaultTheme/widget/scrollbar/scrollbar.edc:192
> +            name: "reset";

Ditto.

>> Source/WebCore/platform/efl/ScrollbarThemeEfl.cpp:130
>> +}
> 
> ryuan, 
> 
> How about this approach?
> 
> Get the RenderThemeEfl from the scrollbar, Drawing the scrollbar using the RenderThemeEfl.
> If you agree, I will proceed to implement the scrollbar.

I think that it's better than before.

Although I am not sure whether it is right to access FrameView and Page here,
I think that we don't have another choice until we refactor RenderTheme.

> Source/WebCore/platform/efl/ScrollbarThemeEfl.h:40
> +class ScrollbarThemeEfl : public ScrollbarThemeComposite {

why do you remove final?

> Source/WebCore/platform/efl/ScrollbarThemeEfl.h:42
> +    virtual ~ScrollbarThemeEfl();

If this is final, we can remove virtual. and how about `~ScrollbarThemeEfl() = default` ?

> Source/WebCore/platform/efl/ScrollbarThemeEfl.h:44
> +    virtual int scrollbarThickness(ScrollbarControlSize = RegularScrollbar) override;

I am not sure, didn't webkit decide skip `virtual` keyword since `override` keyword was introduced.
Comment 12 Hunseop Jeong 2015-07-09 21:04:46 PDT 
Created attachment 256563 [details]
Patch
Comment 13 Ryuan Choi 2015-07-13 17:11:43 PDT 
(In reply to comment #12)
> Created attachment 256563 [details]
> Patch

Looks fine to me.
Comment 14 Gyuyoung Kim 2015-07-13 22:38:04 PDT 
Comment on attachment 256563 [details]
Patch

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

> Source/WebCore/platform/efl/DefaultTheme/widget/scrollbar/scrollbar.edc:3
> +    Copyright (C) 2015 Samsung Electronics.

Missing "All rights reserved." at the end of line.

> Source/WebCore/platform/efl/DefaultTheme/widget/scrollbar/scrollbar.edc:24
> +                  * If wants to draw on top, just overflow usign edje's rel1/rel2

s/usign/using/g

> Source/WebCore/platform/efl/DefaultTheme/widget/scrollbar/scrollbar.edc:78
> +                  * If wants to draw on top, just overflow usign edje's rel1/rel2

ditto.

> Source/WebCore/platform/efl/DefaultTheme/widget/scrollbar/scrollbar.edc:171
> +                  * If wants to draw on top, just overflow usign edje's rel1/rel2

ditto.

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:354
> +    if (!entry)

I think we should get *entry*. It would be good if we generate a crash using assert when entry is null.

> Source/WebCore/platform/efl/RenderThemeEfl.h:183
> +    bool paintThemePart(const GraphicsContext&, FormType, const IntRect&);

One question. What is difference between this new *paintThemePart* and existing one ?

> Source/WebCore/platform/efl/ScrollbarThemeEfl.cpp:46
> +static const int cScrollbarThickness = 10;

What does "c" prefix mean ? It looks you just copy win port implementation. If so, win port can use *c* prefix because it means *CoreUI*. However we doesn't.

> Source/WebCore/platform/efl/ScrollbarThemeEfl.cpp:47
> +static const int cScrollbarThumbMin = 2;

ditto.

> Source/WebCore/platform/efl/ScrollbarThemeEfl.cpp:71
> +{

Add notImplemented().

> Source/WebCore/platform/efl/ScrollbarThemeEfl.cpp:77
> +    return IntRect();

ditto.
Comment 15 Hunseop Jeong 2015-07-14 00:43:14 PDT 
Created attachment 256764 [details]
Patch
Comment 16 Hunseop Jeong 2015-07-14 00:52:21 PDT 
(In reply to comment #14)
> Comment on attachment 256563 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=256563&action=review
> 
> > Source/WebCore/platform/efl/DefaultTheme/widget/scrollbar/scrollbar.edc:3
> > +    Copyright (C) 2015 Samsung Electronics.
> 
> Missing "All rights reserved." at the end of line.
Done.
> 
> > Source/WebCore/platform/efl/DefaultTheme/widget/scrollbar/scrollbar.edc:24
> > +                  * If wants to draw on top, just overflow usign edje's rel1/rel2
> 
> s/usign/using/g
Done
> 
> > Source/WebCore/platform/efl/DefaultTheme/widget/scrollbar/scrollbar.edc:78
> > +                  * If wants to draw on top, just overflow usign edje's rel1/rel2
> 
> ditto.
> 
> > Source/WebCore/platform/efl/DefaultTheme/widget/scrollbar/scrollbar.edc:171
> > +                  * If wants to draw on top, just overflow usign edje's rel1/rel2
> 
> ditto.
> 
> > Source/WebCore/platform/efl/RenderThemeEfl.cpp:354
> > +    if (!entry)
> 
> I think we should get *entry*. It would be good if we generate a crash using
> assert when entry is null.
Done.
> 
> > Source/WebCore/platform/efl/RenderThemeEfl.h:183
> > +    bool paintThemePart(const GraphicsContext&, FormType, const IntRect&);
> 
> One question. What is difference between this new *paintThemePart* and
> existing one ?
Existing *paintThemePart* need the renderObject to apply the Edje State.
But *ScrollbarThemeEfl* didn't pass the RenderObject to draw the scrollbar.
So I create the new function to draw the scrollbar.
> 
> > Source/WebCore/platform/efl/ScrollbarThemeEfl.cpp:46
> > +static const int cScrollbarThickness = 10;
> 
> What does "c" prefix mean ? It looks you just copy win port implementation.
> If so, win port can use *c* prefix because it means *CoreUI*. However we
> doesn't.
You are right,, I copied the *c* prefix from mac port.
I changed the variable name correctly.
> 
> > Source/WebCore/platform/efl/ScrollbarThemeEfl.cpp:47
> > +static const int cScrollbarThumbMin = 2;
> 
> ditto.
> 
> > Source/WebCore/platform/efl/ScrollbarThemeEfl.cpp:71
> > +{
> 
> Add notImplemented().
Done.
> 
> > Source/WebCore/platform/efl/ScrollbarThemeEfl.cpp:77
> > +    return IntRect();
> 
> ditto.
Comment 17 Gyuyoung Kim 2015-07-14 01:12:11 PDT 
Comment on attachment 256764 [details]
Patch

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

> Source/WebCore/platform/efl/RenderThemeEfl.h:183
> +    bool paintThemePart(const GraphicsContext&, FormType, const IntRect&);

It would be good if we place this function just below the existing paintThemePart().
Comment 18 Hunseop Jeong 2015-07-14 19:05:39 PDT 
Created attachment 256819 [details]
Patch
Comment 19 Hunseop Jeong 2015-07-14 19:52:11 PDT 
(In reply to comment #17)
> Comment on attachment 256764 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=256764&action=review
> 
> > Source/WebCore/platform/efl/RenderThemeEfl.h:183
> > +    bool paintThemePart(const GraphicsContext&, FormType, const IntRect&);
> 
> It would be good if we place this function just below the existing
> paintThemePart().

Done. Thanks for the review.
Comment 20 Gyuyoung Kim 2015-07-14 20:10:39 PDT 
(In reply to comment #17)
> Comment on attachment 256764 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=256764&action=review
> 
> > Source/WebCore/platform/efl/RenderThemeEfl.h:183
> > +    bool paintThemePart(const GraphicsContext&, FormType, const IntRect&);
> 
> It would be good if we place this function just below the existing
> paintThemePart().

My comment is not fixed yet.
Comment 21 Hunseop Jeong 2015-07-14 20:28:24 PDT 
(In reply to comment #20)
> (In reply to comment #17)
> > Comment on attachment 256764 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=256764&action=review
> > 
> > > Source/WebCore/platform/efl/RenderThemeEfl.h:183
> > > +    bool paintThemePart(const GraphicsContext&, FormType, const IntRect&);
> > 
> > It would be good if we place this function just below the existing
> > paintThemePart().
> 
> My comment is not fixed yet.

Oh,, *paintThemePart* have to be placed in public scope to access this function in ScrollbarThemeEfl.
Comment 22 WebKit Commit Bot 2015-07-14 21:17:39 PDT 
Comment on attachment 256819 [details]
Patch

Clearing flags on attachment: 256819

Committed r186829: <http://trac.webkit.org/changeset/186829>
Comment 23 WebKit Commit Bot 2015-07-14 21:17:46 PDT 
All reviewed patches have been landed.  Closing bug.