It is the first step to support for web inspector in WebKit-EFL DRT.
Created attachment 145017 [details] Patch
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.
Comment on attachment 145017 [details] Patch Attachment 145017 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12864187
(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.
(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!
Created attachment 145047 [details] Patch
Comment on attachment 145047 [details] Patch Attachment 145047 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12860328
Created attachment 151398 [details] Patch
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
(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"
Created attachment 151428 [details] Patch
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?
(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 ...)
Created attachment 151612 [details] Patch
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.
(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.
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.
(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.
(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.
Created attachment 152475 [details] Patch
(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.
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 ?
Created attachment 152934 [details] Patch
Comment on attachment 152934 [details] Patch Looks good to me now.
Comment on attachment 152934 [details] Patch Clearing flags on attachment: 152934 Committed r122952: <http://trac.webkit.org/changeset/122952>
All reviewed patches have been landed. Closing bug.
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.
(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)