[Qt] Interaction Engine suspends content during pageload

• Component: WebKit2
• Status: RESOLVED FIXED

[Allan Sandfeld Jensen]

Several calls to suspend and resume are made during page load. This should be entirely unnecessary and can only slow loading down.

The cause is the ViewportUpdateDeferrer that suspends and resumes content every time updates are deferred.

[Allan Sandfeld Jensen]

Created attachment 130145 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 130145 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130145&action=review

> Source/WebKit2/ChangeLog:6
> +        Only suspend content when viewport updates are deferred for an animation.

and interaction. This is also suspending during pan, which isn't really being animated.

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:64
> -    ViewportUpdateDeferrer(QtViewportInteractionEngine* engine)
> +    ViewportUpdateDeferrer(QtViewportInteractionEngine* engine, bool animation = false)

Have you looked whether the other ViewportUpdateDeferrer calls actually make any sense?

Maybe this class needs a clean up instead.

Also what happens if the last class destructing didn't have animation set to true? then we will never resume again.

[Allan Sandfeld Jensen]

(In reply to comment #2)
> (From update of attachment 130145 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130145&action=review
> 
> > Source/WebKit2/ChangeLog:6
> > +        Only suspend content when viewport updates are deferred for an animation.
> 
> and interaction. This is also suspending during pan, which isn't really being animated.
> 
That is what I meant. The pan set the animation flag.

> > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:64
> > -    ViewportUpdateDeferrer(QtViewportInteractionEngine* engine)
> > +    ViewportUpdateDeferrer(QtViewportInteractionEngine* engine, bool animation = false)
> 
> Have you looked whether the other ViewportUpdateDeferrer calls actually make any sense?
> 
> Maybe this class needs a clean up instead.
> 
I started doing that but realized that we end up sending more position updates. When the animation flag is not set, the class still postpones sending an update on position to the UI-class. This means a series of function calls changing zoom and then moving the viewport only ends up sending one update, instead of an update for each separate functions.

> Also what happens if the last class destructing didn't have animation set to true? then we will never resume again.

That is actually why the animation flag is not remembered. Instead the animation flags sets a flag on the engine that content has been suspended, then the last class checks that flag instead of whether it was an animation itself.

[Simon Hausmann]

Comment on attachment 130145 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130145&action=review

I think in general this looks good to me. I think the readability should be addressed (r-), but otherwise this is certainly an improvement :)

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:74
> +        if (animation) {
> +            engine->m_suspendedContent = true;
> +            emit engine->contentSuspendRequested();
> +        }

Would it make sense to move this code into a method in ViewportInteractionEngine, instead of modifying the state of the engine and emitting its signal from within another class?

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:202
> +    m_scrollUpdateDeferrer = adoptPtr(new ViewportUpdateDeferrer(this, true));

I'm not a fan of this API at least, because the boolean parameter creates unreadable code. If you look at this line of code in four months, it'll be rather difficult to remember what the meaning of "true" is. It may require a little more typing, but using an enum will make the code more readable.

[zalan]

the following assertion failure was observed during loading cnn.com

02:25:58 PM) carewolf: add it to https://bugs.webkit.org/show_bug.cgi?id=80294, I will open a new bug once that is closed, if it is separate issue
(02:26:21 PM) zalan: carewolf: sounds good.

ASSERTION FAILED: m_suspended
../../../../Source/WebCore/page/SuspendableTimer.cpp(76) : virtual void WebCore::SuspendableTimer::resume()
1   0x7fdfa287c907 /home/zbujtas/WebKit/WebKitBuild/Debug/lib/libQtWebKit.so.4(_ZN7WebCore16SuspendableTimer6resumeEv+0x3f) [0x7fdfa287c907]
2   0x7fdfa24406e2 /home/zbujtas/WebKit/WebKitBuild/Debug/lib/libQtWebKit.so.4(_ZN7WebCore22ScriptExecutionContext22resumeActiveDOMObjectsEv+0x130) [0x7fdfa24406e2]
3   0x7fdfa23935d4 /home/zbujtas/WebKit/WebKitBuild/Debug/lib/libQtWebKit.so.4(_ZN7WebCore8Document22resumeActiveDOMObjectsEv+0x1e) [0x7fdfa23935d4]
4   0x7fdfa282ef2c /home/zbujtas/WebKit/WebKitBuild/Debug/lib/libQtWebKit.so.4(_ZN7WebCore5Frame35resumeActiveDOMObjectsAndAnimationsEv+0xb6) [0x7fdfa282ef2c]
5   0x7fdfa2854462 /home/zbujtas/WebKit/WebKitBuild/Debug/lib/libQtWebKit.so.4(_ZN7WebCore4Page35resumeActiveDOMObjectsAndAnimationsEv+0x2a) [0x7fdfa2854462]
6   0x7fdfa201a6cf /home/zbujtas/WebKit/WebKitBuild/Debug/lib/libQtWebKit.so.4(_ZN6WebKit7WebPage35resumeActiveDOMObjectsAndAnimationsEv+0x29) [0x7fdfa201a6cf]
7   0x7fdfa205dae9 /home/zbujtas/WebKit/WebKitBuild/Debug/lib/libQtWebKit.so.4(_ZN7CoreIPC18callMemberFunctionIN6WebKit7WebPageEMS2_FvvEEEvRKNS_10Arguments0EPT_T0_+0x52) [0x7fdfa205dae9]
8   0x7fdfa205c32e /home/zbujtas/WebKit/WebKitBuild/Debug/lib/libQtWebKit.so.4(_ZN7CoreIPC13handleMessageIN8Messages7WebPage35ResumeActiveDOMObjectsAndAnimationsEN6WebKit7WebPageEMS5_FvvEEEvPNS_15ArgumentDecoderEPT0_T1_+0x4d) [0x7fdfa205c32e]
9   0x7fdfa2059dfe /home/zbujtas/WebKit/WebKitBuild/Debug/lib/libQtWebKit.so.4(_ZN6WebKit7WebPage24didReceiveWebPageMessageEPN7CoreIPC10ConnectionENS1_9MessageIDEPNS1_15ArgumentDecoderE+0xd40) [0x7fdfa2059dfe]
10  0x7fdfa201d801 /home/zbujtas/WebKit/WebKitBuild/Debug/lib/libQtWebKit.so.4(_ZN6WebKit7WebPage17didReceiveMessageEPN7CoreIPC10ConnectionENS1_9MessageIDEPNS1_15ArgumentDecoderE+0x121) [0x7fdfa201d801]
11  0x7fdfa2034f8a /home/zbujtas/WebKit/WebKitBuild/Debug/lib/libQtWebKit.so.4(_ZN6WebKit10WebProcess17didReceiveMessageEPN7CoreIPC10ConnectionENS1_9MessageIDEPNS1_15ArgumentDecoderE+0x2b0) [0x7fdfa2034f8a]
12  0x7fdfa2032a63 /home/zbujtas/WebKit/WebKitBuild/Debug/lib/libQtWebKit.so.4(_ZN6WebKit24WebConnectionToUIProcess17didReceiveMessageEPN7CoreIPC10ConnectionENS1_9MessageIDEPNS1_15ArgumentDecoderE+0x11b) [0x7fdfa2032a63]
13  0x7fdfa1e0f5b3 /home/zbujtas/WebKit/WebKitBuild/Debug/lib/libQtWebKit.so.4(_ZN7CoreIPC10Connection15dispatchMessageERNS0_7MessageINS_15ArgumentDecoderEEE+0x14b) [0x7fdfa1e0f5b3]
14  0x7fdfa1e0f78d /home/zbujtas/WebKit/WebKitBuild/Debug/lib/libQtWebKit.so.4(_ZN7CoreIPC10Connection16dispatchMessagesEv+0xaf) [0x7fdfa1e0f78d]
15  0x7fdfa1e19738 /home/zbujtas/WebKit/WebKitBuild/Debug/lib/libQtWebKit.so.4(_ZN3WTF15FunctionWrapperIMN7CoreIPC10ConnectionEFvvEEclEPS2_+0x58) [0x7fdfa1e19738]
16  0x7fdfa1e194f6 /home/zbujtas/WebKit/WebKitBuild/Debug/lib/libQtWebKit.so.4(_ZN3WTF17BoundFunctionImplINS_15FunctionWrapperIMN7CoreIPC10ConnectionEFvvEEEFvPS3_EEclEv+0x32) [0x7fdfa1e194f6]
17  0x7fdfa1f34e3c /home/zbujtas/WebKit/WebKitBuild/Debug/lib/libQtWebKit.so.4(_ZNK3WTF8FunctionIFvvEEclEv+0x72) [0x7fdfa1f34e3c]
18  0x7fdfa29167f6 /home/zbujtas/WebKit/WebKitBuild/Debug/lib/libQtWebKit.so.4(_ZN7WebCore7RunLoop11performWorkEv+0x74) [0x7fdfa29167f6]
19  0x7fdfa2bb8fea /home/zbujtas/WebKit/WebKitBuild/Debug/lib/libQtWebKit.so.4(_ZN7WebCore7RunLoop11TimerObject11performWorkEv+0x1c) [0x7fdfa2bb8fea]
20  0x7fdfa2bb9af7 /home/zbujtas/WebKit/WebKitBuild/Debug/lib/libQtWebKit.so.4(+0x2997af7) [0x7fdfa2bb9af7]
21  0x7fdf9c347637 /home/zbujtas/qt5/qtbase/lib/libQtCore.so.5(_ZN14QMetaCallEvent13placeMetaCallEP7QObject+0xc3) [0x7fdf9c347637]
22  0x7fdf9c3484b0 /home/zbujtas/qt5/qtbase/lib/libQtCore.so.5(_ZN7QObject5eventEP6QEvent+0x124) [0x7fdf9c3484b0]
23  0x7fdf9ce1c93c /home/zbujtas/qt5/qtbase/lib/libQtWidgets.so.5(_ZN19QApplicationPrivate13notify_helperEP7QObjectP6QEvent+0x17c) [0x7fdf9ce1c93c]
24  0x7fdf9ce19fec /home/zbujtas/qt5/qtbase/lib/libQtWidgets.so.5(_ZN12QApplication6notifyEP7QObjectP6QEvent+0x3f8) [0x7fdf9ce19fec]
25  0x7fdf9c315362 /home/zbujtas/qt5/qtbase/lib/libQtCore.so.5(_ZN16QCoreApplication14notifyInternalEP7QObjectP6QEvent+0x9e) [0x7fdf9c315362]
26  0x7fdf9c3190b3 /home/zbujtas/qt5/qtbase/lib/libQtCore.so.5(_ZN16QCoreApplication9sendEventEP7QObjectP6QEvent+0x51) [0x7fdf9c3190b3]
27  0x7fdf9c3163f6 /home/zbujtas/qt5/qtbase/lib/libQtCore.so.5(_ZN23QCoreApplicationPrivate16sendPostedEventsEP7QObjectiP11QThreadData+0x452) [0x7fdf9c3163f6]
28  0x7fdf9c315fa1 /home/zbujtas/qt5/qtbase/lib/libQtCore.so.5(_ZN16QCoreApplication16sendPostedEventsEP7QObjecti+0x2d) [0x7fdf9c315fa1]
29  0x7fdf9c37c428 /home/zbujtas/qt5/qtbase/lib/libQtCore.so.5(+0x25f428) [0x7fdf9c37c428]
30  0x7fdf9e2fba5d /lib/x86_64-linux-gnu/libglib-2.0.so.0(g_main_context_dispatch+0x1dd) [0x7fdf9e2fba5d]
31  0x7fdf9e2fc258 /lib/x86_64-linux-gnu/libglib-2.0.so.0(+0x45258) [0x7fdf9e2fc258]
WARNING: The web process experienced a crash on 'http://edition.cnn.com/'.

[Kenneth Rohde Christiansen]

Comment on attachment 130145 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130145&action=review

553	    ViewportUpdateDeferrer guard(this);
554	    ensureContentWithinViewportBoundary();

^ this one might also animate (do a bounce back).

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:72
> +            engine->m_suspendedContent = true;

m_didSuspendContent would be more clear

>> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:202
>> +    m_scrollUpdateDeferrer = adoptPtr(new ViewportUpdateDeferrer(this, true));
> 
> I'm not a fan of this API at least, because the boolean parameter creates unreadable code. If you look at this line of code in four months, it'll be rather difficult to remember what the meaning of "true" is. It may require a little more typing, but using an enum will make the code more readable.

I don't like calling it animating as it is true for just normal panning, so why not use DeferGeometryChanges, DeferGeometryAndContentsChanges or so

[Allan Sandfeld Jensen]

(In reply to comment #6)
> (From update of attachment 130145 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130145&action=review
> 
> 553        ViewportUpdateDeferrer guard(this);
> 554        ensureContentWithinViewportBoundary();
> 
> ^ this one might also animate (do a bounce back).
> 
If it doesn't allocate the ViewportUpdateDeferrer using new, and then later delete it when the animation is over, then the updates will have to complete inside this call. If it does animate then ViewportUpdateDeferrer is being used incorrectly.

[Allan Sandfeld Jensen]

Created attachment 130370 [details]
Patch

[Andras Becsi]

Comment on attachment 130370 [details]
Patch

Clearing flags on attachment: 130370

Committed r109930: <http://trac.webkit.org/changeset/109930>

[Andras Becsi]

All reviewed patches have been landed.  Closing bug.