Bug 98886 – [Qt][WK2] Avoid unnecessary calls in PageViewportController

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Andras Becsi]

[Qt][WK2] Infinite loop in PageViewportController

[Andras Becsi]

Created attachment 167982 [details]
Patch

[Jocelyn Turcotte]

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.

[Andras Becsi]

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

[Andras Becsi]

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

[Jocelyn Turcotte]

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

[Jocelyn Turcotte]

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.

[Andras Becsi]

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

[Andras Becsi]

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

[Andras Becsi]

Created attachment 168043 [details]
Patch

[Kenneth Rohde Christiansen]

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

[Jocelyn Turcotte]

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.

[Andras Becsi]

Committed r131034: <http://trac.webkit.org/changeset/131034>

[Andras Becsi]

Comment on attachment 168043 [details]
Patch

Clearing flags from attachment.