[Webkit-unassigned] [Bug 68509] [EFL] Implementation of Unit Tests framework for the WebKit-Efl port

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=68509





--- Comment #6 from Krzysztof <k.czech at samsung.com>  2011-09-22 01:35:35 PST ---
(In reply to comment #5)
> (From update of attachment 108121 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108121&action=review
> 
> The patch looks OK overall (even though I've never used gtest).
> 
> However, there are coding style issues that need to be fixed (I see m_fooBar and m_foo_bar as member variables, there are the issues Gyuyoung pointed out). I would also prefer to use as many "native" C++ and WebKit types as possible, so you have bool instead of Eina_Bool where possible, and you use Strings instead of char*'s whose memory you need to manage manually. Speaking of memory management, you could also use OwnPtrs for the Evas_Objects and other pointers so you also don't need to manage their memory by hand.

I think it might be a problem using WebKit's OwnPtrs in unit tests context.
Tests are linked against WebKit library. Those internals like OwnPtr are not publicly exported. Other way I am not sure if is a good idea to mix Evas_Object's with smart pointers. How do you think ?
> 
> > Source/WebKit/efl/tests/src/UnitTestLauncher/EFLTestLauncher.cpp:32
> > +Eina_Bool EFLTestLauncher::init()
> 
> Shouldn't you have to call the respective _shutdown() functions as well?
> 
> > Source/WebKit/efl/tests/src/UnitTestLauncher/EFLTestView.cpp:73
> > +    m_url = createTestUrl(EFLUnitTests::Config::defaultTestPage);
> 
> Why the double assignment?
You mean one in initialization list and one above. In case something goes wrong with the above initialization m_url points at null. Other way is good to have default initialization.
> 
> > Source/WebKit/efl/tests/src/UnitTestLauncher/EFLTestView.cpp:84
> > +    m_url = createTestUrl(url);
> 
> Why the double assignment?
> 
> > Source/WebKit/efl/tests/src/UnitTestLauncher/EFLTestView.cpp:95
> > +    m_url = createTestUrl(url);
> 
> Why the double assignment?
> 
> > Source/WebKit/efl/tests/src/UnitTestLauncher/EFLTestView.cpp:106
> > +    m_url = createTestUrl(url);
> 
> Why the double assignment?
> 
> > Source/WebKit/efl/tests/src/UnitTestLauncher/EFLTestView.h:61
> > +protected:
> 
> Small nitpick: an extra empty line before this keyword would be nice
> 
> > Source/WebKit/efl/tests/src/UnitTestLauncher/EFLTestView.h:63
> > +private:
> 
> Ditto.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.