Bug 87935 – [EFL][DRT] Add support for Web Inspector in WebKit-EFL DRT

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

[Seokju Kwon]

It is the first step to support for web inspector in WebKit-EFL DRT.

[Seokju Kwon]

Created attachment 145017 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 145017 [details]
Patch

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

> Source/WebKit/efl/ChangeLog:8
> +        It is the first step to support for Web Inspector in WebKit-EFL DRT

Please write patch description for each ChangeLog more detail. In addition, put a period at the end line.

> Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp:150
> +    evas_object_focus_set(inspector, EINA_TRUE);

Use true instead of EINA_TRUE.

> Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp:590
> +    Evas_Object** view = static_cast<Evas_Object **>(eventInfo);

Should you need to use ** pointer in here ?

> LayoutTests/ChangeLog:8
> +        It is the first step to support for Web Inspector in WebKit-EFL DRT

This is not proper description for this change. Re-write description.

> LayoutTests/platform/efl/Skipped:300
> +#inspector

Remove this all instead of comment.

[Gyuyoung Kim]

Comment on attachment 145017 [details]
Patch

Attachment 145017 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12864187

[Seokju Kwon]

(In reply to comment #3)
> (From update of attachment 145017 [details])
> Attachment 145017 [details] did not pass efl-ews (efl):
> Output: http://queues.webkit.org/results/12864187

ewk_view_web_inspector_show() and ewk_view_web_inspector_close() are implemented in another patch. (See Bug 83865)
This patch depends on it.

[Seokju Kwon]

(In reply to comment #2)
> (From update of attachment 145017 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145017&action=review
> 
> > Source/WebKit/efl/ChangeLog:8
> > +        It is the first step to support for Web Inspector in WebKit-EFL DRT
>
> Please write patch description for each ChangeLog more detail. In addition, put a period at the end line.
Done!
 
> > Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp:150
> > +    evas_object_focus_set(inspector, EINA_TRUE);
> 
> Use true instead of EINA_TRUE.
Done!

> > Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp:590
> > +    Evas_Object** view = static_cast<Evas_Object **>(eventInfo);
> 
> Should you need to use ** pointer in here ?
As you know, Evas smart callback can not return a value. And Use ** pointer for returning a pointer from callback function. 

> > LayoutTests/ChangeLog:8
> > +        It is the first step to support for Web Inspector in WebKit-EFL DRT
> 
> This is not proper description for this change. Re-write description.
Done!

> > LayoutTests/platform/efl/Skipped:300
> > +#inspector
> 
> Remove this all instead of comment.
Done!

[Seokju Kwon]

Created attachment 145047 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 145047 [details]
Patch

Attachment 145047 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12860328

[Seokju Kwon]

Created attachment 151398 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 151398 [details]
Patch

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

> Tools/DumpRenderTree/efl/DumpRenderTreeView.cpp:69
> +    if (newMessage.contains("Localized string") || (newMessage.contains("Protocol Error: the message is for non-existing domain 'Profiler'")))

nit : Why do you use unneeded ( ) in second condition ?

if (newMessage.contains("Localized string") || newMessage.contains("Protocol Error: the message is for non-existing domain 'Profiler'")) ?

> Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:770
> +    // Waits until web inspector resources are loaded

In code level, I don't understand what do you need to wait ?

> Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:773
> +        ecore_main_loop_begin();

I wonder if this is meaningful? Because ecore_main_loop_begin() is already called at the beginning of layout test.

http://trac.webkit.org/browser/trunk/Tools/DumpRenderTree/efl/DumpRenderTree.cpp#L257

[Seokju Kwon]

(In reply to comment #9)
> (From update of attachment 151398 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151398&action=review
> 
> > Tools/DumpRenderTree/efl/DumpRenderTreeView.cpp:69
> > +    if (newMessage.contains("Localized string") || (newMessage.contains("Protocol Error: the message is for non-existing domain 'Profiler'")))
> 
> nit : Why do you use unneeded ( ) in second condition ?
> 
> if (newMessage.contains("Localized string") || newMessage.contains("Protocol Error: the message is for non-existing domain 'Profiler'")) ?

I will remove it.

> 
> > Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:770
> > +    // Waits until web inspector resources are loaded
> 
> In code level, I don't understand what do you need to wait ?
> 
> > Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:773
> > +        ecore_main_loop_begin();
> 
> I wonder if this is meaningful? Because ecore_main_loop_begin() is already called at the beginning of layout test.
> 
> http://trac.webkit.org/browser/trunk/Tools/DumpRenderTree/efl/DumpRenderTree.cpp#L257

Test files are using 'WebInspector' object. It is implemented in resources of web inspector(frontend). So Dumprendertree should wait until resources are loaded totally('load, finished'). If not, drt fails tests with error message below.
"ReferenceError: Can't find variable: WebInspector"

[Seokju Kwon]

Created attachment 151428 [details]
Patch

[Chris Dumez]

Comment on attachment 151428 [details]
Patch

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

> Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.cpp:529
> +#if ENABLE(INSPECTOR)

This should be at the beginning of the function, not in the middle.

> Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp:153
> +    evas_object_focus_set(inspectorView, EINA_TRUE);

I believe you should use "true" here instead of EINA_TRUE, as per guidelines.

> Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp:167
> +    if (inspectorView) {

You already return earlier if inspectorView is NULL so it seems this check is not needed, right?

> Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:770
> +    // Waits until web inspector resources are loaded

I don't understand why this is needed, could you explain?

[Seokju Kwon]

(In reply to comment #12)
> (From update of attachment 151428 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151428&action=review
> 
> > Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.cpp:529
> > +#if ENABLE(INSPECTOR)
> 
> This should be at the beginning of the function, not in the middle.
> 
> > Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp:153
> > +    evas_object_focus_set(inspectorView, EINA_TRUE);
> 
> I believe you should use "true" here instead of EINA_TRUE, as per guidelines.
> 
> > Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp:167
> > +    if (inspectorView) {
> 
> You already return earlier if inspectorView is NULL so it seems this check is not needed, right?
>
I will fix all that you mentioned. 
 
> > Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:770
> > +    // Waits until web inspector resources are loaded
> 
> I don't understand why this is needed, could you explain?

Like what I said on comment #10, Drt have to wait until 'inspector.html' called web inspector is loaded totally. Because some objects in web inspector are called in test files (like LayoutTests/http/tests/inspector/inspector-test.js, LayoutTests/http/tests/inspector/console-test.js ...)

[Seokju Kwon]

Created attachment 151612 [details]
Patch

[Chris Dumez]

Comment on attachment 151612 [details]
Patch

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

> Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:771
> +    Evas_Object* inspectorView = ewk_view_web_inspector_view_get(browser->mainView());

If we really need those 3 lines, then LGTM. I was just wondering because the Qt port code seems simpler here.

[Seokju Kwon]

(In reply to comment #15)
> (From update of attachment 151612 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151612&action=review
> 
> > Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:771
> > +    Evas_Object* inspectorView = ewk_view_web_inspector_view_get(browser->mainView());
> 
> If we really need those 3 lines, then LGTM. I was just wondering because the Qt port code seems simpler here.

Some tests are not passing without those 3 line.
(Error messgae : "ReferenceError: Can't find variable: WebInspector")
And I got the hint at https://bugs.webkit.org/show_bug.cgi?id=80235.
Thank you for your review.

[Gyuyoung Kim]

Comment on attachment 151398 [details]
Patch

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

>> Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:773
>> +        ecore_main_loop_begin();
> 
> I wonder if this is meaningful? Because ecore_main_loop_begin() is already called at the beginning of layout test.
> 
> http://trac.webkit.org/browser/trunk/Tools/DumpRenderTree/efl/DumpRenderTree.cpp#L257

I'm not clear this yet. Does inspector call ecore_main_loop_begin() / _quit() first before calling it from runTest() of DumpRenderTree.cpp ? Could you explain the reason again ?

In addition, I don't like to call ecore_main_loop_begin() / _quit() from different files. If you call this in LayoutTestControllerEfl.cpp, it is good to call ecore_main_loop_quit() in here as well. I think this is good to debug.

[Seokju Kwon]

(In reply to comment #17)
> (From update of attachment 151398 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151398&action=review
> 
> >> Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:773
> >> +        ecore_main_loop_begin();
> > 
> > I wonder if this is meaningful? Because ecore_main_loop_begin() is already called at the beginning of layout test.
> > 
> > http://trac.webkit.org/browser/trunk/Tools/DumpRenderTree/efl/DumpRenderTree.cpp#L257
> 
> I'm not clear this yet. Does inspector call ecore_main_loop_begin() / _quit() first before calling it from runTest() of DumpRenderTree.cpp ? Could you explain the reason again ?
> 
It can't test the inspector without page(inspector.html) has finished loading on the view of inspector.
I think that it can't complete loading before it loads testURL on EFL Port. So it should wait until the page has finished loading with ecore_main_loop_begin() / _quit().

> In addition, I don't like to call ecore_main_loop_begin() / _quit() from different files. If you call this in LayoutTestControllerEfl.cpp, it is good to call ecore_main_loop_quit() in here as well. I think this is good to debug.

I agree with you. I will move it.

[Gyuyoung Kim]

(In reply to comment #18)

> > I'm not clear this yet. Does inspector call ecore_main_loop_begin() / _quit() first before calling it from runTest() of DumpRenderTree.cpp ? Could you explain the reason again ?
> > 

> It can't test the inspector without page(inspector.html) has finished loading on the view of inspector.
> I think that it can't complete loading before it loads testURL on EFL Port. So it should wait until the page has finished loading with ecore_main_loop_begin() / _quit().

Do you mean you need to load inspector.html first before loading testURL by page instance ? If so, I can understand this. Please update patch according to comment  #17.

[Seokju Kwon]

Created attachment 152475 [details]
Patch

[Seokju Kwon]

(In reply to comment #19)
> (In reply to comment #18)
> 
> > > I'm not clear this yet. Does inspector call ecore_main_loop_begin() / _quit() first before calling it from runTest() of DumpRenderTree.cpp ? Could you explain the reason again ?
> > > 
> 
> > It can't test the inspector without page(inspector.html) has finished loading on the view of inspector.
> > I think that it can't complete loading before it loads testURL on EFL Port. So it should wait until the page has finished loading with ecore_main_loop_begin() / _quit().
> 
> Do you mean you need to load inspector.html first before loading testURL by page instance ? If so, I can understand this. Please update patch according to comment  #17.

Yes. That's right. Thank you for your understanding.

[Gyuyoung Kim]

Comment on attachment 152475 [details]
Patch

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

Looks better than before.

> Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp:173
> +    // Waits until the page has finished loading

Could you write why we need to wait for finishing page loading here ?

[Seokju Kwon]

Created attachment 152934 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 152934 [details]
Patch

Looks good to me now.

[WebKit Review Bot]

Comment on attachment 152934 [details]
Patch

Clearing flags on attachment: 152934

Committed r122952: <http://trac.webkit.org/changeset/122952>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dominik Röttsches (drott)]

Seokju, good to see this enabled. Thank you.

Can you check the failing test cases on the bot? Maybe we need some new baselines here?
http://build.webkit.org/results/EFL%20Linux%2064-bit%20Debug/r122954%20(2963)/results.html

Please run-webkit-tests locally before unskipping a set of tests.

[Seokju Kwon]

(In reply to comment #27)
> Seokju, good to see this enabled. Thank you.
> 
> Can you check the failing test cases on the bot? Maybe we need some new baselines here?
> http://build.webkit.org/results/EFL%20Linux%2064-bit%20Debug/r122954%20(2963)/results.html
> 
> Please run-webkit-tests locally before unskipping a set of tests.

Add them to Skipped list temporarily because of the bot red.
And then I will move it to TestExpectations with bug id.
(https://bugs.webkit.org/show_bug.cgi?id=91624)