Bug 83030 – [EFL] DRT should support LayoutTestController's dumpFrameLoadCallbacks()

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Chris Dumez]

EFL's DumpRenderTree needs to support LayoutTestController's dumpFrameLoadCallbacks(). This will allow removal of several test cases from the skip list.

[Chris Dumez]

Created attachment 135326 [details]
Proposed patch

[Raphael Kubo da Costa (:rakuco)]

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.

[Chris Dumez]

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.

[Raphael Kubo da Costa (:rakuco)]

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?

[Chris Dumez]

(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?

[Raphael Kubo da Costa (:rakuco)]

(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.

[Chris Dumez]

Created attachment 136426 [details]
Proposed patch

Update patch based on feedback.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 136426 [details]
Proposed patch

Looks fine, thanks!

[Chris Dumez]

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.

[Chris Dumez]

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.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 136939 [details]
Proposed patch

Looks good to me.

[Antonio Gomes]

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?

[Chris Dumez]

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().

[Chris Dumez]

Splitted with #84052.

[Gyuyoung Kim]

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.

[Chris Dumez]

Created attachment 137484 [details]
Proposed patch

Add missing description to Tools/ChangeLog.

[Antonio Gomes]

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

[Chris Dumez]

Created attachment 137533 [details]
Proposed patch

Ready patch for landing.

[WebKit Review Bot]

Comment on attachment 137533 [details]
Proposed patch

Clearing flags on attachment: 137533

Committed r114376: <http://trac.webkit.org/changeset/114376>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

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 :(

[Raphael Kubo da Costa (:rakuco)]

(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.

[Chris Dumez]

Created attachment 137545 [details]
Proposed patch

Uploading the right patch this time, ready to commit. Really sorry about the mix up.

[WebKit Review Bot]

Comment on attachment 137545 [details]
Proposed patch

Clearing flags on attachment: 137545

Committed r114386: <http://trac.webkit.org/changeset/114386>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.