[Qt][WK2] Infinite loop in PageViewportController
Created attachment 167982 [details] Patch
Comment on attachment 167982 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167982&action=review > Source/WebKit2/ChangeLog:9 > + which induce an infinite loop. introduce? > Source/WebKit2/ChangeLog:12 > + Also avoid an update cycle and return early in didRenderFrame if no > + change in scale or position has to be applied since animations on > + the page can trigger up to 60 DidRenderFrame messages per second. I'm not sure I understand what has to be avoided, is it because of the ViewportUpdateDeferrer? > Source/WebKit2/UIProcess/qt/PageViewportControllerClientQt.cpp:472 > + if (newSize == IntSize(m_pageItem->contentsSize().toSize())) > + return; Make sure that you run all API/qml tests with this change, I'm not sure if there would be cases where the contentsScaleCommitted signal emitted below might be skipped.
(In reply to comment #2) > (From update of attachment 167982 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=167982&action=review > > > Source/WebKit2/ChangeLog:9 > > + which induce an infinite loop. > > introduce? No, I meant induce: "cause, bring about or give rise to" > > > Source/WebKit2/ChangeLog:12 > > + Also avoid an update cycle and return early in didRenderFrame if no > > + change in scale or position has to be applied since animations on > > + the page can trigger up to 60 DidRenderFrame messages per second. > > I'm not sure I understand what has to be avoided, is it because of the ViewportUpdateDeferrer? Exactly, creating and releasing a ViewportUpdateDeferrer triggers an update response (didChangeContentsVisibility) from the client which ends in m_pageItem->update(). All this is unnecessary and can be avoided by the early return. > > > Source/WebKit2/UIProcess/qt/PageViewportControllerClientQt.cpp:472 > > + if (newSize == IntSize(m_pageItem->contentsSize().toSize())) > > + return; > > Make sure that you run all API/qml tests with this change, I'm not sure if there would be cases where the contentsScaleCommitted signal emitted below might be skipped. I ran the tests, and all of them pass. To me it seems that the below emission is obsolete as well since the tests do not rely on that aspect any more.
(In reply to comment #3) > > > Source/WebKit2/UIProcess/qt/PageViewportControllerClientQt.cpp:472 > > > + if (newSize == IntSize(m_pageItem->contentsSize().toSize())) > > > + return; > > > > Make sure that you run all API/qml tests with this change, I'm not sure if there would be cases where the contentsScaleCommitted signal emitted below might be skipped. > > I ran the tests, and all of them pass. To me it seems that the below emission is obsolete as well since the tests do not rely on that aspect any more. Ok, I was wrong about the obsoleteness, emitting contentsScaleCommitted even if the scale did not change is actually needed for the fit-to-width test, but only if the size changes, thus this patch does not interfere with that aspect.
(In reply to comment #3) > > I'm not sure I understand what has to be avoided, is it because of the ViewportUpdateDeferrer? > Exactly, creating and releasing a ViewportUpdateDeferrer triggers an update response (didChangeContentsVisibility) from the client which ends in m_pageItem->update(). All this is unnecessary and can be avoided by the early return. Humm this is ugly the deferrer shouls prevent stuff from happening, not causing anything especially when DeferUpdateAndSuspendContent isn't used. What if we start checking "if (suspendContentFlag == DeferUpdateAndSuspendContent)" before calling resumeContent() in ~ViewportUpdateDeferrer?
That syncVisibleContents() call should probably be included in the if as well, it also feels like it's called at the wrong place. I'm not even sure it is still necessary since PageViewportControllerClientQt::didResumeContent calls updateViewportController() itself.
(In reply to comment #5) > (In reply to comment #3) > > > I'm not sure I understand what has to be avoided, is it because of the ViewportUpdateDeferrer? > > Exactly, creating and releasing a ViewportUpdateDeferrer triggers an update response (didChangeContentsVisibility) from the client which ends in m_pageItem->update(). All this is unnecessary and can be avoided by the early return. > Humm this is ugly the deferrer shouls prevent stuff from happening, not causing anything especially when DeferUpdateAndSuspendContent isn't used. > What if we start checking "if (suspendContentFlag == DeferUpdateAndSuspendContent)" before calling resumeContent() in ~ViewportUpdateDeferrer? The problem with this is that then the DeferUpdate flag does not have any effect any more. It was used when correcting the contents to valid bounds immediately for example, to update the controller about the updated viewport visibility.
(In reply to comment #6) > That syncVisibleContents() call should probably be included in the if as well, it also feels like it's called at the wrong place. I'm not even sure it is still necessary since PageViewportControllerClientQt::didResumeContent calls updateViewportController() itself. Yes, it's because originally the ViewportDeferer was intended for client usage only, but didRenderFrame need's it now. So we might need to rethink the role of the update deferer.
Created attachment 168043 [details] Patch
Comment on attachment 168043 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=168043&action=review > Source/WebKit2/UIProcess/PageViewportController.cpp:127 > + if (m_clientContentsSize != contentsSize) { > + m_clientContentsSize = contentsSize; Better add a comment here why this is an optimization how there is m_ variable just for the sake of this check
Comment on attachment 168043 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=168043&action=review r=me if you can make Kenneth happy. > Source/WebKit2/UIProcess/PageViewportController.cpp:-58 > - // Make sure that tiles all around the viewport will be requested. You could move that comment to PageViewportControllerClientQt::didResumeContent which now takes care of it.
Committed r131034: <http://trac.webkit.org/changeset/131034>
Comment on attachment 168043 [details] Patch Clearing flags from attachment.