[webkit-reviews] review granted: [Bug 68458] [EFL] Add DumpRenderTreeSupportEfl : [Attachment 111003] Rebase on top of trunk

[bugzilla-daemon at webkit.org]

Kenneth Rohde Christiansen <kenneth at webkit.org> has granted Raphael Kubo da
Costa <kubo at profusion.mobi>'s request for review:
Bug 68458: [EFL] Add DumpRenderTreeSupportEfl
https://bugs.webkit.org/show_bug.cgi?id=68458

Attachment 111003: Rebase on top of trunk
https://bugs.webkit.org/attachment.cgi?id=111003&action=review

------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=111003&action=review


Pretty nice and definitely an improvement

> Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.cpp:55
> +DumpRenderTreeSupportEfl::DumpRenderTreeSupportEfl()
> +{
> +}
> +
> +DumpRenderTreeSupportEfl::~DumpRenderTreeSupportEfl()
> +{
> +}

Why not just define in the header?

> Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.cpp:77
> +    WebCore::Frame* frame = EWKPrivate::coreFrame(ewkFrame);
> +
> +    if (frame)
> +	   frame->tree()->clearName();

This can be done in two lines

> Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.cpp:85
> +    WebCore::Frame* frame = EWKPrivate::coreFrame(ewkFrame);
> +
> +    if (frame)
> +	   frame->loader()->setOpener(0);

here as well

if (WebCore::Frame* frame = EWKPrivate::coreFrame(ewkFrame))
    frame->loader()->setOpener(0);

> Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.cpp:150
> +int DumpRenderTreeSupportEfl::numberOfPages(const Evas_Object* ewkFrame,
float pageWidth, float pageHeight)

maybe this should just return unsigned?

> Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.cpp:160
> +int DumpRenderTreeSupportEfl::numberOfPagesForElementId(const Evas_Object*
ewkFrame, const char* elementId, float pageWidth, float pageHeight)

here as well

> Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.cpp:236
> +    WebCore::Frame* frame = EWKPrivate::coreFrame(ewkFrame);
> +
> +    if (!frame)
> +	   return 0;
> +
> +    return frame->domWindow()->pendingUnloadEventListeners();

if (WebCore::Frame* frame = EWKPrivate::coreFrame(ewkFrame))
    return frame->domWindow()->pendingUnloadEventListeners();

return 0;

> Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.cpp:372
> +	   Ewk_History_Item* kid =
ewk_history_item_new_from_core(children[i].get());

You must be "kidding" me :-)

> Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.h:42
> +    DumpRenderTreeSupportEfl();
> +    ~DumpRenderTreeSupportEfl();

Add the { } here