[webkit-reviews] review denied: [Bug 68509] [EFL] Implementation of Unit Tests framework for the WebKit-Efl port : [Attachment 108121] Patch

[bugzilla-daemon at webkit.org]

Raphael Kubo da Costa <kubo at profusion.mobi> has denied Krzysztof
<k.czech at samsung.com>'s request for review:
Bug 68509: [EFL] Implementation of Unit Tests framework for the WebKit-Efl port
https://bugs.webkit.org/show_bug.cgi?id=68509

Attachment 108121: Patch
https://bugs.webkit.org/attachment.cgi?id=108121&action=review

------- Additional Comments from Raphael Kubo da Costa <kubo at profusion.mobi>
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.

> 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?

> 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.