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.
Created attachment 45567 [details] proposed patch
style-queue ran check-webkit-style on attachment 45567 [details] without any errors.
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 ...
(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?
> 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 ?
(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 on attachment 45567 [details] proposed patch Looks like at least some of these were removed in bug 16853. This looks sane to me.
Comment on attachment 45567 [details] proposed patch Clearing flags on attachment: 45567 Committed r52612: <http://trac.webkit.org/changeset/52612>
All reviewed patches have been landed. Closing bug.