EFL's DumpRenderTree needs to support LayoutTestController's dumpFrameLoadCallbacks(). This will allow removal of several test cases from the skip list.
Created attachment 135326 [details] Proposed patch
Comment on attachment 135326 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=135326&action=review Repeating what I said on IRC for future-proofness: this approach is fine, but it ends up emitting duplicate signals in cases in which we already have, for example, ewk_frame_load_firstlayout_nonempty_finished and ewk_frame_load_provisional. Even though I like the enum approach better, adopting it will probably break existing code hard, so it'd be good to leverage the work that's already done here. > Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp:341 > + if ((loadStatus == EWK_FRAME_LOAD_FINISHED || loadStatus == EWK_FRAME_LOAD_FAILED) > + && frame == topLoadingFrame) WebKit in general doesn't have a hard character limit, so putting the if in a single line is just fine.
Created attachment 135814 [details] Proposed patch Reuse existing signals as suggested and remove the one with enumeration. Please note that I had to emit the "load,finished" signal from dispatchDidFailLoad() / dispatchDidFinishLoad(), instead of postProgressFinishedNotification(). Otherwise, I miss a "didFinishLoadForFrame" in some tests.
Comment on attachment 135814 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=135814&action=review Looks almost good; please also explain in the ChangeLog the reason for moving the ewk_frame_load_{finished,error} signals. > Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:-244 > - if (m_loadError.isNull()) > - ewk_frame_load_finished(m_frame, 0, 0, 0, 0, 0); > - else { > - ewk_frame_load_finished(m_frame, > - m_loadError.domain().utf8().data(), > - m_loadError.errorCode(), > - m_loadError.isCancellation(), > - m_loadError.localizedDescription().utf8().data(), > - m_loadError.failingURL().utf8().data()); > - } Please add a notImplemented() call here. > Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:769 > if (!shouldFallBack(err)) > return; This check has no effect now; shouldn't you call ewk_frame_load_error after this check?
(In reply to comment #4) > > Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:769 > > if (!shouldFallBack(err)) > > return; > > This check has no effect now; shouldn't you call ewk_frame_load_error after this check? Some tests did not pass due to missing load errors, so I had to move the error signaling before the shouldFallBack() check. Note that the GTK port is doing the same. However, they have code after the shouldFallback() check. Maybe I should add a notImplemented() after the check for clarity?
(In reply to comment #5) > (In reply to comment #4) > > > Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:769 > > > if (!shouldFallBack(err)) > > > return; > > > > This check has no effect now; shouldn't you call ewk_frame_load_error after this check? > > Some tests did not pass due to missing load errors, so I had to move the error signaling before the shouldFallBack() check. Note that the GTK port is doing the same. However, they have code after the shouldFallback() check. Maybe I should add a notImplemented() after the check for clarity? The GTK port seems to automatically load a page with an error message, which is something we usually leave up to the API users. I guess you can just remove the call.
Created attachment 136426 [details] Proposed patch Update patch based on feedback.
Comment on attachment 136426 [details] Proposed patch Looks fine, thanks!
Comment on attachment 136426 [details] Proposed patch Removing flags since it seems to cause regressions. I'll debug this tomorrow and update the patch then.
Created attachment 136939 [details] Proposed patch The previous tests was causing timeouts in the following tests: * http/tests/eventsource/existent-eventsource-status-error-iframe-crash.html * http/tests/eventsource/existent-eventsource-status-error-iframe-crash.html * http/tests/eventsource/non-existent-eventsource-status-error-iframe-crash.html * http/tests/misc/location-replace-crossdomain.html * http/tests/misc/cached-scripts.html * http/tests/misc/onload-remove-iframe-crash-2.html * fast/dom/HTMLDocument/frameless-location-bugzilla10837.html * fast/dom/gc-6.html * fast/dom/attr_dead_doc.html * fast/dom/frame-loading-via-document-write.html * fast/dom/Window/Location/window-shadow-location-using-js-object-with-toString.html * fast/dom/Window/closure-access-after-navigation-iframe.html * fast/dom/constructors-cached-navigate.html * fast/dom/HTMLObjectElement/object-as-frame.html * fast/dom/HTMLHeadElement/head-check.html * fast/dom/NodeList/adoptNode-node-list-cache.html * fast/dom/DOMImplementation/createDocument-with-used-doctype.html * fast/dom/gc-7.html * fast/css-generated-content/positioned-div-with-floating-after-content-crash.html * fast/css/user-stylesheet-crash.html * fast/css/stylesheet-candidate-nodes-crash.xhtml I updated the "load,started" signal that is emitted on the ewk_view so that the ewk_frame which started loading is passed to the listeners. This way I can get rid of the following hack in EFL's DRT: -void DumpRenderTreeChrome::onLoadStarted(void*, Evas_Object* view, void*) +void DumpRenderTreeChrome::onLoadStarted(void*, Evas_Object* view, void* eventInfo) { - // FIXME: we actually need the frame related to this event - Evas_Object* frame = ewk_view_frame_main_get(view); + Evas_Object* frame = static_cast<Evas_Object*>(eventInfo); After this fix, I can no longer experience any timeouts in the tests.
Comment on attachment 136939 [details] Proposed patch Looks good to me.
Comment on attachment 136939 [details] Proposed patch it looks like you are doing more than what the bug title says. Maybe splitting the patch in two?
Created attachment 137364 [details] Proposed patch Splitted the patch as advised. I will open another bug report for the "load,finished" emitting from dispatchDidFailLoad() instead of postProgressFinishedNotification().
Splitted with #84052.
Comment on attachment 137364 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=137364&action=review > Tools/ChangeLog:7 > + Though you modify codes considerably, you don't write any descriptions.
Created attachment 137484 [details] Proposed patch Add missing description to Tools/ChangeLog.
Comment on attachment 137484 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=137484&action=review it is good to go,with some minor nits. please reupload with them fixed, and "Reviewed by Antonio Gomes" in the changelogs and only set cq? > Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp:242 > Eina_Bool DumpRenderTreeChrome::processWork(void* data) I think you can omit 'data' or use param_unused > Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp:259 > + ecore_idler_add(processWork, 0); ecore_idler_add(processWork, 0 /*frame*/) maybe would be clearer
Created attachment 137533 [details] Proposed patch Ready patch for landing.
Comment on attachment 137533 [details] Proposed patch Clearing flags on attachment: 137533 Committed r114376: <http://trac.webkit.org/changeset/114376>
All reviewed patches have been landed. Closing bug.
I'm really sorry, it appears I uploaded a wrong patch after tonikitoo's review. Would it be possible to revert the commit so that I can reupload a clean one? The patch that landed is an older version which has regressions :(
(In reply to comment #21) > I'm really sorry, it appears I uploaded a wrong patch after tonikitoo's review. Would it be possible to revert the commit so that I can reupload a clean one? The patch that landed is an older version which has regressions :( Rolling out r114376 in bug 84155.
Created attachment 137545 [details] Proposed patch Uploading the right patch this time, ready to commit. Really sorry about the mix up.
Comment on attachment 137545 [details] Proposed patch Clearing flags on attachment: 137545 Committed r114386: <http://trac.webkit.org/changeset/114386>