Bug 83030 - [EFL] DRT should support LayoutTestController's dumpFrameLoadCallbacks()
Summary: [EFL] DRT should support LayoutTestController's dumpFrameLoadCallbacks()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on: 84155
Blocks: 83281 83669 84052
  Show dependency treegraph
 
Reported: 2012-04-03 07:27 PDT by Chris Dumez
Modified: 2012-05-06 22:50 PDT (History)
5 users (show)

See Also:


Attachments
Proposed patch (20.40 KB, patch)
2012-04-03 07:35 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Proposed patch (20.20 KB, patch)
2012-04-05 06:48 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Proposed patch (21.17 KB, patch)
2012-04-10 04:16 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Proposed patch (24.03 KB, patch)
2012-04-12 11:31 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Proposed patch (19.68 KB, patch)
2012-04-16 10:34 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Proposed patch (19.88 KB, patch)
2012-04-16 23:36 PDT, Chris Dumez
tonikitoo: review+
tonikitoo: commit-queue-
Details | Formatted Diff | Diff
Proposed patch (21.33 KB, patch)
2012-04-17 07:24 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Proposed patch (19.92 KB, patch)
2012-04-17 08:48 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2012-04-03 07:27:25 PDT
EFL's DumpRenderTree needs to support LayoutTestController's dumpFrameLoadCallbacks(). This will allow removal of several test cases from the skip list.
Comment 1 Chris Dumez 2012-04-03 07:35:19 PDT
Created attachment 135326 [details]
Proposed patch
Comment 2 Raphael Kubo da Costa (:rakuco) 2012-04-04 07:58:50 PDT
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.
Comment 3 Chris Dumez 2012-04-05 06:48:24 PDT
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 4 Raphael Kubo da Costa (:rakuco) 2012-04-05 18:58:45 PDT
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?
Comment 5 Chris Dumez 2012-04-06 00:00:36 PDT
(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?
Comment 6 Raphael Kubo da Costa (:rakuco) 2012-04-06 04:33:49 PDT
(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.
Comment 7 Chris Dumez 2012-04-10 04:16:55 PDT
Created attachment 136426 [details]
Proposed patch

Update patch based on feedback.
Comment 8 Raphael Kubo da Costa (:rakuco) 2012-04-10 22:58:12 PDT
Comment on attachment 136426 [details]
Proposed patch

Looks fine, thanks!
Comment 9 Chris Dumez 2012-04-11 08:27:42 PDT
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.
Comment 10 Chris Dumez 2012-04-12 11:31:14 PDT
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 11 Raphael Kubo da Costa (:rakuco) 2012-04-12 11:46:38 PDT
Comment on attachment 136939 [details]
Proposed patch

Looks good to me.
Comment 12 Antonio Gomes 2012-04-16 07:10:56 PDT
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?
Comment 13 Chris Dumez 2012-04-16 10:34:13 PDT
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().
Comment 14 Chris Dumez 2012-04-16 11:00:02 PDT
Splitted with #84052.
Comment 15 Gyuyoung Kim 2012-04-16 23:05:25 PDT
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.
Comment 16 Chris Dumez 2012-04-16 23:36:45 PDT
Created attachment 137484 [details]
Proposed patch

Add missing description to Tools/ChangeLog.
Comment 17 Antonio Gomes 2012-04-17 06:49:56 PDT
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
Comment 18 Chris Dumez 2012-04-17 07:24:12 PDT
Created attachment 137533 [details]
Proposed patch

Ready patch for landing.
Comment 19 WebKit Review Bot 2012-04-17 08:02:54 PDT
Comment on attachment 137533 [details]
Proposed patch

Clearing flags on attachment: 137533

Committed r114376: <http://trac.webkit.org/changeset/114376>
Comment 20 WebKit Review Bot 2012-04-17 08:03:00 PDT
All reviewed patches have been landed.  Closing bug.
Comment 21 Chris Dumez 2012-04-17 08:21:49 PDT
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 :(
Comment 22 Raphael Kubo da Costa (:rakuco) 2012-04-17 08:32:14 PDT
(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.
Comment 23 Chris Dumez 2012-04-17 08:48:28 PDT
Created attachment 137545 [details]
Proposed patch

Uploading the right patch this time, ready to commit. Really sorry about the mix up.
Comment 24 WebKit Review Bot 2012-04-17 09:13:40 PDT
Comment on attachment 137545 [details]
Proposed patch

Clearing flags on attachment: 137545

Committed r114386: <http://trac.webkit.org/changeset/114386>
Comment 25 WebKit Review Bot 2012-04-17 09:13:46 PDT
All reviewed patches have been landed.  Closing bug.