[webkit-reviews] review denied: [Bug 61974] [EFL] DRT: Add LayoutTestControllerEfl. : [Attachment 95824] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Leandro Pereira
<leandro at profusion.mobi>'s request for review:
Bug 61974: [EFL] DRT: Add LayoutTestControllerEfl.
https://bugs.webkit.org/show_bug.cgi?id=61974

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

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=95824&action=review

Smart pointers would make this patch infinitely better.

> Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:91
> +    free(idAsString);

Can't we use some sort of smart pointer here?

> Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:95
> +    JSRetainPtr<JSStringRef> returnValue(Adopt,
JSStringCreateWithUTF8CString(counterValue));
> +    free(counterValue);
> +    return returnValue;

A smart pointer here would turn these 3 lines into 1. :)

> Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:127
> +    int pageNumber = ewk_frame_page_number_by_element_id_get(mainFrame,
idAsString, pageWidth, pageHeight);
> +    free(idAsString);
> +    return pageNumber;

Again, a smart pointer turns 3 lines into 1...

> Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:155
> +    Ewk_History* history;

WebKit avoids this old C-style declare-at-top-of-block, but maybe EFL
recommends it?	Again, this is a c++ file, not c.

> Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:156
> +    int count;

This isn't needed.

> Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:158
> +    history = ewk_view_history_get(browser);

I woudl have declared history on the same line as you initialized this.

> Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:163
> +    count = ewk_history_back_list_length(history) +
ewk_history_forward_list_length(history);
> +    return count;

The local doesn't buy you anything here.

> Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:176
> +    waitForPolicy = false;

What scope is waitForPolicy?  Is that a global?  Or a member?

> Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:212
> +    Ewk_Cookie_Policy policy;
> +
> +    if (alwaysAcceptCookies)
> +	   policy = EWK_COOKIE_JAR_ACCEPT_ALWAYS;
> +    else
> +	   policy = EWK_COOKIE_JAR_ACCEPT_NEVER;

This could easily all be a single-line ternary expression.

> Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:275
> +    free(userStyleSheet);

Huh?  I take it userStyleSheet is a global?  Can't we use a smart pointer here
so we don't have to manually free... and possibly double-free?

> Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:303
> +    waitToDumpWatchdog = 0;

Does it need to be deleted/freed as well?

> Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:359
> +    // FIXME: implement

We often use the notImplemented(); function instead, but this is fine.

> Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:416
> +	   const char* tmpDirEnv = static_cast<const char*>(getenv("TMPDIR"));
> +	   if (!tmpDirEnv) {
> +	       tmpDirEnv = static_cast<const char*>(getenv("TEMP"));
> +	       if (!tmpDirEnv)
> +		   tmpDirEnv = "/tmp";

Do we have to free the result of getenv?

> Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:469
> +    Eina_Bool caseSensitive = EINA_TRUE;
> +    Eina_Bool forward = EINA_TRUE;
> +    Eina_Bool wrap = EINA_FALSE;

Do we really need our own bool?  I suspect EFL is a C-API, and C doesn't have a
"bool" type, but since we're writing c++, can't we just use bool and
true/false?