Bug 68458 – [EFL] Add DumpRenderTreeSupportEfl

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Raphael Kubo da Costa (:rakuco)]

[EFL] Add DumpRenderTreeSupportEfl

[Raphael Kubo da Costa (:rakuco)]

Created attachment 108041 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

Kenneth and tonikitoo, DRTSupportEfl is finally here. The patch looks big at first, but it's mostly moving functions which were already present from one place to another and adapting the code.

If you still would like me do split the patch, please let me know.

[WebKit Review Bot]

Attachment 108041 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebKit/efl/CMakeLists..." exit_code: 1

Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.h:31:  Ewk_History_Item is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #3)
> Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.h:31:  Ewk_History_Item is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
> Total errors found: 1 in 14 files

This is a false positive, Ewk_History_Item is actually called Ewk_History_Item.

[Raphael Kubo da Costa (:rakuco)]

Created attachment 108384 [details]
Rebase after r95659, export the class to make DRT link without SHARED_CORE

[WebKit Review Bot]

Attachment 108384 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebKit/efl/CMakeLists..." exit_code: 1

Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.h:31:  Ewk_History_Item is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Raphael Kubo da Costa (:rakuco)]

Created attachment 110371 [details]
Rebase on top of trunk

[WebKit Review Bot]

Attachment 110371 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebKit/efl/CMakeLists..." exit_code: 1

Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.h:31:  Ewk_History_Item is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Raphael Kubo da Costa (:rakuco)]

Created attachment 111003 [details]
Rebase on top of trunk

[Kenneth Rohde Christiansen]

Comment on attachment 111003 [details]
Rebase on top of trunk

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

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 111003 [details]
Rebase on top of trunk

Thank you so much for the review, this is the last remaining big DRT-related patch that needs to go in :)

View in context: https://bugs.webkit.org/attachment.cgi?id=111003&action=review

>> Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.cpp:150
>> +int DumpRenderTreeSupportEfl::numberOfPages(const Evas_Object* ewkFrame, float pageWidth, float pageHeight)
> 
> maybe this should just return unsigned?

PrintContext::numberOfPages returns an int.

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

So does PrintContext::pageNumberForElement.

[Raphael Kubo da Costa (:rakuco)]

Created attachment 111006 [details]
Fifth time is a charm. Follow Kenneth's suggestions.

[WebKit Review Bot]

Comment on attachment 111006 [details]
Fifth time is a charm. Follow Kenneth's suggestions.

Clearing flags on attachment: 111006

Committed r97464: <http://trac.webkit.org/changeset/97464>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.