.
Created attachment 93331 [details] Patch
Comment on attachment 93331 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=93331&action=review I had a quick look, and this look sane ;) Just a few comments. > Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:176 > + if (!m_eventSender) > + m_eventSender = new EventSender(this); Can't this be initialized in the constructor of the page to simplify stuff a bit? > Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:787 > + // Each page has its own event sender object, as it must install an event filter in that page's view I don't think you need a comment here. > Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:788 > + if (frame->page()) Is this check really necessary? When is a frame be detached from the page?
Created attachment 93468 [details] Patch
Created attachment 93471 [details] Patch
Comment on attachment 93471 [details] Patch Clearing flags on attachment: 93471 Committed r86447: <http://trac.webkit.org/changeset/86447>
All reviewed patches have been landed. Closing bug.
Created attachment 93557 [details] Patch
Reopened to fix regressions. Benjamin - I think this fixes the leak we discussed last night.
Comment on attachment 93557 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=93557&action=review > Source/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.h:110 > + static void setView(QWebPage*, QWidget* view); What would you think about: -we get rid of this -the WebPage allocate its own view in the constructor. > Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:179 > + if (!m_eventSender) > + m_eventSender = new EventSender(this); > + return m_eventSender; Can we skip the lazy allocation? > Tools/DumpRenderTree/qt/DumpRenderTreeQt.h:219 > + EventSender* m_eventSender; Why not allocate the event sender with the object: EventSender* m_eventSender; -> EventSender m_eventSender; This way we don't have to care about the memory. If that can't be done for some reason, please use a smart pointer so that there is no need to delete m_eventSender in the destructor.
Created attachment 93578 [details] Patch
Comment on attachment 93578 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=93578&action=review Looks good, please add the assertion so we make sure we never leak the QWidget. Let's hope this path will have a better fate than the previous one :) R+ > Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:162 > + DumpRenderTreeSupportQt::setView(this, new QWidget(qobject_cast<QWidget*>(parent))); Please add: ASSERT(qobject_cast<QWidget*>(parent)) before this line. > Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:-467 > - m_eventSender = new EventSender(m_page); Do we still need m_eventSender at all now that we have m_page->eventSender()?
Committed r86504: <http://trac.webkit.org/changeset/86504>
It was rolled out. Reopening ...
Created attachment 93682 [details] Patch
I'm testing this patch in the VM, I'll post the results here. Note that the change failed the last time because DRT needs event sender to be intialized lazily. I'm not sure why this is yet.
This runs cleanly on the VM. Kenneth/Andreas, Benjamin asked that someone else take a look - (I think I've broken his spirit :) ). Would you mind reviewing?
Comment on attachment 93682 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=93682&action=review There is no information in this bug about the regressions that the second patch fixed and about why the latest landed patch was rolled out. This information would probably help in the review. > LayoutTests/ChangeLog:3 > + Reviewed by Benjamin Poulain. You should probably remove this until the patch gets actually reviewed and r+'ed. > Source/WebKit/qt/ChangeLog:3 > + Reviewed by Benjamin Poulain. Ditto. > Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:139 > + , m_eventSender(0) Could you use a smart pointer for m_eventSender? Is m_eventSender leaking? Is it intentional? > Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:166 > + ASSERT(qobject_cast<QWidget*>(parent)); If the parameter parent must be a QWidget* why isn't it declared as QWidget* instead of QObject*? Then you could avoid the cast and let the compiler check the type for you. > Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:167 > + DumpRenderTreeSupportQt::setView(this, new QWidget(qobject_cast<QWidget*>(parent))); How was it working before without this line? Was page.setView been called somewhere? > Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:182 > + if (!m_eventSender) > + m_eventSender = new EventSender(this); > + return m_eventSender; It seems that in current version the event sender is not created lazily, it is created in DumpRenderTree constructor just after creating m_page. Why is it needed now?
(In reply to comment #17) > > > Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:139 > > + , m_eventSender(0) > > Could you use a smart pointer for m_eventSender? > Is m_eventSender leaking? Is it intentional? No, that was accidental. > > > Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:182 > > + if (!m_eventSender) > > + m_eventSender = new EventSender(this); > > + return m_eventSender; > > It seems that in current version the event sender is not created lazily, it is created in DumpRenderTree constructor just after creating m_page. > Why is it needed now? Now each webpage has its own EventSender object, before there was one for all pages. I have found that if I don't create it lazily, some tests that use EventSender will time out - because EventSender hangs in the event loop in replaySavedEvents(). My guess is that this suggests that QWebPage needs to be in a certain state before EventSender calls installEventFilter() on its view in it's constructor. That's as far as I've got in understanding why. > > Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:166 > > + ASSERT(qobject_cast<QWidget*>(parent)); > > If the parameter parent must be a QWidget* why isn't it declared as QWidget* instead of QObject*? > Then you could avoid the cast and let the compiler check the type for you. The parent needs to be a QObject so that the view doesn't display. I need to cast it to a QWidget here so that it can be the parent of the 'dummy' non-displayed view and free it when the parent is destroyed. > > > Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:167 > > + DumpRenderTreeSupportQt::setView(this, new QWidget(qobject_cast<QWidget*>(parent))); > > How was it working before without this line? Was page.setView been called somewhere? > No, it wasn't called anywhere explicitly - instead it was called by QWebPage's constructor. So the parent QObject (m_mainView) was the view() for DRT's only EventSender. Now each page has a unique EventSender with it's own view. This allows each page to have its own event filter.
Created attachment 94103 [details] Patch
Comment on attachment 94103 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94103&action=review > Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:166 > + ASSERT(qobject_cast<QWidget*>(parent)); I still think that this is wrong because the type of the parameter parent should be QWidget*. QWebPage's constructor parameter type is QObject* because it does not require it to be a QWidget*, but you obviously do. The cast in the first line of QWebPage's constructor is exactly equal to this one. If this one is required to succeed then that one will succeed as well. > Tools/DumpRenderTree/qt/DumpRenderTreeQt.h:218 > + EventSender* m_eventSender; I think that to use a smart pointer would be preferable, but it is not a problem if m_eventSender is not leaking.
(In reply to comment #18) > (In reply to comment #17) > > > > > > Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:182 > > > + if (!m_eventSender) > > > + m_eventSender = new EventSender(this); > > > + return m_eventSender; > > > > It seems that in current version the event sender is not created lazily, it is created in DumpRenderTree constructor just after creating m_page. > > Why is it needed now? > > Now each webpage has its own EventSender object, before there was one for all pages. I have found that if I don't create it lazily, some tests that use EventSender will time out - because EventSender hangs in the event loop in replaySavedEvents(). My guess is that this suggests that QWebPage needs to be in a certain state before EventSender calls installEventFilter() on its view in it's constructor. That's as far as I've got in understanding why. I have seen that WebPage instances are created by two methods: DumpRenderTree::DumpRenderTree() and DumpRenderTree::createWindow(). In the original code just DumpRenderTree::DumpRenderTree() creates an EventSender instance, so no EventSender instance was created when a WebPage was created by DumpRenderTree::createWindow(). In your patch an EventSender instance is created for every WebPage instance. Could it be a problem? Could it be the reason for those tests to timeout? Trying to help. :)
(In reply to comment #21) > > I have seen that WebPage instances are created by two methods: DumpRenderTree::DumpRenderTree() and DumpRenderTree::createWindow(). > In the original code just DumpRenderTree::DumpRenderTree() creates an EventSender instance, so no EventSender instance was created when a WebPage was created by DumpRenderTree::createWindow(). > > In your patch an EventSender instance is created for every WebPage instance. > > Could it be a problem? It's deliberate that each Page gets its own EventSender - this is what will allow the unskipped test to pass. I think the problem is that if I attempt to initialize EventSender when the WebPage is being constructed it's too early, for some reason. :-) > Could it be the reason for those tests to timeout? The tests that are timing out don't use createWindow(). As I say, I think it's because installing an Event Filter on the page's view while the page is getting constructed is too early.
Comment on attachment 94103 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94103&action=review I pointed out another issue in a previous review round. > Tools/ChangeLog:11 > + The support for this test added to the unix test plugin here > + may allow other platforms to pass it (nearly everyone > + skips it). On Qt it required a bit of trickiness with > + the page's EventSender object to get it working fully though, > + so only unskipping Qt here. This description does not say anything about the changes introduced by this patch. By reading this change log it should be possible to figure out what is the problem with the current implementation, what are the changes that this patch introduces and how they solve the problem. What, why and how are usually the questions for the change log to answer. :)
(In reply to comment #22) > (In reply to comment #21) > > > > I have seen that WebPage instances are created by two methods: DumpRenderTree::DumpRenderTree() and DumpRenderTree::createWindow(). > > In the original code just DumpRenderTree::DumpRenderTree() creates an EventSender instance, so no EventSender instance was created when a WebPage was created by DumpRenderTree::createWindow(). > > > > In your patch an EventSender instance is created for every WebPage instance. > > > > Could it be a problem? > > It's deliberate that each Page gets its own EventSender - this is what will allow the unskipped test to pass. I think the problem is that if I attempt to initialize EventSender when the WebPage is being constructed it's too early, for some reason. :-) I see that it was intentional to move m_eventSender to class WebPage but it is not clear why, and it is not clear that all WebPage will indeed have its EventSender instance because of the lazy construction. > > > Could it be the reason for those tests to timeout? > > The tests that are timing out don't use createWindow(). As I say, I think it's because installing an Event Filter on the page's view while the page is getting constructed is too early. Lazy construction is adding complexity and it would be nice to know if it is really needed and why. We had some bad side effects with previous patches that were landed before and I do not feel confident about landing a new one without knowing what is going on there. I would really like to know what is going on with those tests. What are the tests that were failing? Could you provide a link to the test results?
Created attachment 94491 [details] Patch
(In reply to comment #25) > Created an attachment (id=94491) [details] > Patch Uploaded this before I saw your second comment. It's clear the mystery behind the lazy initialization is a blocker for this bug so I'll get to the bottom of it. Thanks for the quick feedback.
(In reply to comment #24) > > Lazy construction is adding complexity and it would be nice to know if it is really needed and why. We had some bad side effects with previous patches that were landed before and I do not feel confident about landing a new one without knowing what is going on there. > > I would really like to know what is going on with those tests. What are the tests that were failing? Could you provide a link to the test results? OK, the reason lazy initialization of EventSender is needed (or careful initialization outside WebPage's constructor) is because view->setPage() currently gets called after the WebPage is created. QWebView::setPage() calls QWebPage::setView() so a new parent view of the page is reinstated - which is bad if you have called EventSender in the page's constructor after setting the QWebView to a custom, windowless view in DumpRenderTreeSupportQt::setView(). The result is that EventSender is listening to a view that doesn't get events anymore. This will be easy enough to to clean up, but this bug is still blocked by 61376, which is more puzzling.
Created attachment 111133 [details] Patch
=== Bulk closing of Qt bugs === If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary. If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.