Bug 32989 - [Qt] DRT: Frame loader callbacks differ from the Mac port
Summary: [Qt] DRT: Frame loader callbacks differ from the Mac port
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P3 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-12-28 12:11 PST by Jakub Wieczorek
Modified: 2009-12-29 00:26 PST (History)
3 users (show)

See Also:


Attachments
proposed patch (7.30 KB, patch)
2009-12-28 12:15 PST, Jakub Wieczorek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jakub Wieczorek 2009-12-28 12:11:14 PST
In FrameLoaderClientQt, there is a couple of methods that print debug messages for the DRT, while the Mac equivalents do not.
To match the expected results for tests that dump frame loader callbacks, those messages should be removed.
Comment 1 Jakub Wieczorek 2009-12-28 12:15:54 PST
Created attachment 45567 [details]
proposed patch
Comment 2 WebKit Review Bot 2009-12-28 12:19:00 PST
style-queue ran check-webkit-style on attachment 45567 [details] without any errors.
Comment 3 Antonio Gomes 2009-12-28 12:32:19 PST
jakub, although "proposed patch" gets good results (many tests can be unskipped), I did not like the solution very much, mainly because these messages you are removing are useful for debugging.

A better solution would be stop calling "qt_dump_frame_loader" (see quoted code below), imo.

void DumpRenderTree::open(const QUrl& aurl)
{
    (...)
    qt_dump_frame_loader(url.toString().contains("loading/"));
    setTextOutputEnabled(true);
    m_page->mainFrame()->load(url);
}


please consider this as well ...
Comment 4 Jakub Wieczorek 2009-12-28 12:47:08 PST
(In reply to comment #3)
> jakub, although "proposed patch" gets good results (many tests can be
> unskipped), I did not like the solution very much, mainly because these
> messages you are removing are useful for debugging.

I understand the point about having these messages for the sake of debugging but should they prevent the Qt port from passing loader tests against the cross-platform expected results?

Do you think it would be fine to leave those messages commented out?

> A better solution would be stop calling "qt_dump_frame_loader" (see quoted code
> below), imo.
> 
> void DumpRenderTree::open(const QUrl& aurl)
> {
>     (...)
>     qt_dump_frame_loader(url.toString().contains("loading/"));
>     setTextOutputEnabled(true);
>     m_page->mainFrame()->load(url);
> }
> 
> 
> please consider this as well ...

I'm sorry, I didn't get this. Can you elaborate a bit more please?
Comment 5 Antonio Gomes 2009-12-28 12:56:21 PST
> I'm sorry, I didn't get this. Can you elaborate a bit more please?

by looking at the messages, they are mostly enclosed by an "if". e.g.:

void FrameLoaderClientQt::dispatchDidHandleOnloadEvents()
{
    // don't need this one
    if (dumpFrameLoaderCallbacks)
        printf("%s - didHandleOnloadEventsForFrame\n", qPrintable(drtDescriptionSuitableForTestResult(m_frame)));
}


"dumpFrameLoaderCallbacks" is a global var exported to DRT via "extern".

Now in the DumpRenderThree::open code formely quoted, see "qt_dump_frame_loader" method call specifically. It is the method that enables dumping these debug message you want to remove. So another option would instead of removing all the messages from the code, we could just stop calling "qt_dump_frame_loader" at DumpRenderThree::open, then they would not dump and debuggers would still be happy :)

make more sense now ?
Comment 6 Jakub Wieczorek 2009-12-28 13:06:27 PST
(In reply to comment #5)
> > I'm sorry, I didn't get this. Can you elaborate a bit more please?
> 
> by looking at the messages, they are mostly enclosed by an "if". e.g.:
> 
> void FrameLoaderClientQt::dispatchDidHandleOnloadEvents()
> {
>     // don't need this one
>     if (dumpFrameLoaderCallbacks)
>         printf("%s - didHandleOnloadEventsForFrame\n",
> qPrintable(drtDescriptionSuitableForTestResult(m_frame)));
> }
> 
> 
> "dumpFrameLoaderCallbacks" is a global var exported to DRT via "extern".
> 
> Now in the DumpRenderThree::open code formely quoted, see
> "qt_dump_frame_loader" method call specifically. It is the method that enables
> dumping these debug message you want to remove. So another option would instead
> of removing all the messages from the code, we could just stop calling
> "qt_dump_frame_loader" at DumpRenderThree::open, then they would not dump and
> debuggers would still be happy :)
> 
> make more sense now ?

It is not about removing all of the frame loader callback messages but only those that are now printed as part of the DRT output in the Qt port but are not in the Mac and Win ports. We still want to dump all of the remaining callbacks. This is to match the loading test expectations.
Comment 7 Eric Seidel (no email) 2009-12-29 00:09:56 PST
Comment on attachment 45567 [details]
proposed patch

Looks like at least some of these were removed in bug 16853.

This looks sane to me.
Comment 8 WebKit Commit Bot 2009-12-29 00:26:42 PST
Comment on attachment 45567 [details]
proposed patch

Clearing flags on attachment: 45567

Committed r52612: <http://trac.webkit.org/changeset/52612>
Comment 9 WebKit Commit Bot 2009-12-29 00:26:47 PST
All reviewed patches have been landed.  Closing bug.