Bug 143566

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.