Bug 61974 – [EFL] DRT: Add LayoutTestControllerEfl.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Leandro Pereira]

[EFL] DRT: Add LayoutTestControllerEfl.

[Leandro Pereira]

Created attachment 95824 [details]
Patch

[Eric Seidel (no email)]

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?

[Raphael Kubo da Costa (:rakuco)]

Created attachment 98011 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

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

[Kent Tamura]

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.

[Raphael Kubo da Costa (:rakuco)]

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

[Kent Tamura]

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.

[Raphael Kubo da Costa (:rakuco)]

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

[Raphael Kubo da Costa (:rakuco)]

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

[WebKit Review Bot]

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>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.