Summary: | [EFL] DRT should support LayoutTestController's dumpFrameLoadCallbacks() | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||||||||||
Component: | WebKit EFL | Assignee: | Chris Dumez <cdumez> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | gyuyoung.kim, lucas.de.marchi, rakuco, tonikitoo, webkit.review.bot | ||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||
Bug Depends on: | 84155 | ||||||||||||||||||||
Bug Blocks: | 83281, 83669, 84052 | ||||||||||||||||||||
Attachments: |
|
Description
Chris Dumez
2012-04-03 07:27:25 PDT
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> All reviewed patches have been landed. Closing bug. |