Bug 61974

Created attachment 95824 [details] Patch

Comment on attachment 95824 [details] Patch 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?

Created attachment 98011 [details] Patch

Created attachment 98178 [details] Fix the return value for some functions which return JSValueRefs

Comment on attachment 98178 [details] Fix the return value for some functions which return JSValueRefs View in context: https://bugs.webkit.org/attachment.cgi?id=98178&action=review > Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:57 > +void LayoutTestController::addDisallowedURL(JSStringRef url) We usually omit argument names if they are not used in the function body. > Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:401 > + char* tmpDirEnv = getenv("TMPDIR"); > + if (!tmpDirEnv) > + tmpDirEnv = getenv("TEMP"); > + if (!tmpDirEnv) > + tmpDirEnv = const_cast<char*>("/tmp"); It's a bad code. We must not cast a string literal to char*. The first patch used "const char*" for tmpDirEnv. It was better.

Created attachment 98722 [details] New patch following Kent's suggestions

Comment on attachment 98722 [details] New patch following Kent's suggestions View in context: https://bugs.webkit.org/attachment.cgi?id=98722&action=review > Tools/ChangeLog:11 > + [EFL] DRT: Add LayoutTestControllerEfl. > + Only the core features are implemented. Most functions are actually > + stubs and will be implemented as soon as its features are needed. > + Build system changes will be made as soon as DRT is completely > + upstreamed. > + https://bugs.webkit.org/show_bug.cgi?id=61974 > + Typical ChangeLog format is: Reviewed by ... <Summary> <Bug URL> <Descption> > Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:179 > + WTF::String absoluteUrl = WTF::String::fromUTF8(ewk_frame_uri_get(mainFrame)); > + absoluteUrl.append(url->characters(), url->length()); > + WTF::String targetString(target->characters(), target->length()); We don't need to add WTF:: prefix to String like OwnFastMallocPtr<> doesn't need WTF::. > Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:404 > + WTF::String databasePath; > + > + if (getenv("TMPDIR")) > + databasePath = WTF::String::fromUTF8(getenv("TMPDIR")); > + else if (getenv("TEMP")) > + databasePath = WTF::String::fromUTF8(getenv("TEMP")); > + else > + databasePath = WTF::String::fromUTF8("/tmp"); ditto. WTF:: is not needed.

Created attachment 98725 [details] Replace WTF::String with String, improve the ChangeLog

Created attachment 98754 [details] Make it build after the recent changes to LayoutTestController.h

Comment on attachment 98754 [details] Make it build after the recent changes to LayoutTestController.h Clearing flags on attachment: 98754 Committed r90361: <http://trac.webkit.org/changeset/90361>

All reviewed patches have been landed. Closing bug.