Bug 32989 – [Qt] DRT: Frame loader callbacks differ from the Mac port

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jakub Wieczorek]

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.

[Jakub Wieczorek]

Created attachment 45567 [details]
proposed patch

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45567 [details] without any errors.

[Antonio Gomes]

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

[Jakub Wieczorek]

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

[Antonio Gomes]

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

[Jakub Wieczorek]

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

[Eric Seidel (no email)]

Comment on attachment 45567 [details]
proposed patch

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

This looks sane to me.

[WebKit Commit Bot]

Comment on attachment 45567 [details]
proposed patch

Clearing flags on attachment: 45567

Committed r52612: <http://trac.webkit.org/changeset/52612>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.