Bug 61974 - [EFL] DRT: Add LayoutTestControllerEfl.
Summary: [EFL] DRT: Add LayoutTestControllerEfl.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Leandro Pereira
URL:
Keywords:
Depends on: 62034 62877
Blocks: 61962
  Show dependency treegraph
 
Reported: 2011-06-02 15:38 PDT by Leandro Pereira
Modified: 2011-07-04 05:18 PDT (History)
2 users (show)

See Also:


Attachments
Patch (29.78 KB, patch)
2011-06-02 15:43 PDT, Leandro Pereira
eric: review-
Details | Formatted Diff | Diff
Patch (28.82 KB, patch)
2011-06-21 10:55 PDT, Raphael Kubo da Costa (:rakuco)
no flags Details | Formatted Diff | Diff
Fix the return value for some functions which return JSValueRefs (28.91 KB, patch)
2011-06-22 09:02 PDT, Raphael Kubo da Costa (:rakuco)
no flags Details | Formatted Diff | Diff
New patch following Kent's suggestions (28.03 KB, patch)
2011-06-27 07:28 PDT, Raphael Kubo da Costa (:rakuco)
no flags Details | Formatted Diff | Diff
Replace WTF::String with String, improve the ChangeLog (27.99 KB, patch)
2011-06-27 07:58 PDT, Raphael Kubo da Costa (:rakuco)
no flags Details | Formatted Diff | Diff
Make it build after the recent changes to LayoutTestController.h (27.23 KB, patch)
2011-06-27 11:23 PDT, Raphael Kubo da Costa (:rakuco)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Leandro Pereira 2011-06-02 15:38:12 PDT
[EFL] DRT: Add LayoutTestControllerEfl.
Comment 1 Leandro Pereira 2011-06-02 15:43:49 PDT
Created attachment 95824 [details]
Patch
Comment 2 Eric Seidel (no email) 2011-06-10 12:27:40 PDT
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?
Comment 3 Raphael Kubo da Costa (:rakuco) 2011-06-21 10:55:04 PDT
Created attachment 98011 [details]
Patch
Comment 4 Raphael Kubo da Costa (:rakuco) 2011-06-22 09:02:15 PDT
Created attachment 98178 [details]
Fix the return value for some functions which return JSValueRefs
Comment 5 Kent Tamura 2011-06-23 02:42:14 PDT
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.
Comment 6 Raphael Kubo da Costa (:rakuco) 2011-06-27 07:28:34 PDT
Created attachment 98722 [details]
New patch following Kent's suggestions
Comment 7 Kent Tamura 2011-06-27 07:52:37 PDT
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.
Comment 8 Raphael Kubo da Costa (:rakuco) 2011-06-27 07:58:09 PDT
Created attachment 98725 [details]
Replace WTF::String with String, improve the ChangeLog
Comment 9 Raphael Kubo da Costa (:rakuco) 2011-06-27 11:23:25 PDT
Created attachment 98754 [details]
Make it build after the recent changes to LayoutTestController.h
Comment 10 WebKit Review Bot 2011-07-04 05:18:47 PDT
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>
Comment 11 WebKit Review Bot 2011-07-04 05:18:52 PDT
All reviewed patches have been landed.  Closing bug.