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.
Created attachment 251002 [details] WIP
Created attachment 251084 [details] Patch
Created attachment 251227 [details] Patch
Created attachment 251311 [details] WIP
Created attachment 251535 [details] WIP
Created attachment 252459 [details] Patch
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 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 ?
Created attachment 256355 [details] WIP
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 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.
Created attachment 256563 [details] Patch
(In reply to comment #12) > Created attachment 256563 [details] > Patch Looks fine to me.
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.
Created attachment 256764 [details] Patch
(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 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().
Created attachment 256819 [details] Patch
(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.
(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.
(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 on attachment 256819 [details] Patch Clearing flags on attachment: 256819 Committed r186829: <http://trac.webkit.org/changeset/186829>
All reviewed patches have been landed. Closing bug.