Bug 80294 - [Qt] Interaction Engine suspends content during pageload
Summary: [Qt] Interaction Engine suspends content during pageload
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Allan Sandfeld Jensen
URL:
Keywords:
Depends on:
Blocks: 80165
  Show dependency treegraph
 
Reported: 2012-03-05 08:49 PST by Allan Sandfeld Jensen
Modified: 2012-03-06 09:55 PST (History)
6 users (show)

See Also:


Attachments
Patch (4.51 KB, patch)
2012-03-05 08:57 PST, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (4.87 KB, patch)
2012-03-06 06:24 PST, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Allan Sandfeld Jensen 2012-03-05 08:49:00 PST
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.
Comment 1 Allan Sandfeld Jensen 2012-03-05 08:57:17 PST
Created attachment 130145 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 2012-03-05 13:56:18 PST
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.
Comment 3 Allan Sandfeld Jensen 2012-03-06 01:54:36 PST
(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.
Comment 4 Simon Hausmann 2012-03-06 03:08:49 PST
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.
Comment 5 zalan 2012-03-06 05:28:18 PST
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/'.
Comment 6 Kenneth Rohde Christiansen 2012-03-06 05:29:09 PST
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
Comment 7 Allan Sandfeld Jensen 2012-03-06 05:45:19 PST
(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.
Comment 8 Allan Sandfeld Jensen 2012-03-06 06:24:45 PST
Created attachment 130370 [details]
Patch
Comment 9 Andras Becsi 2012-03-06 09:54:53 PST
Comment on attachment 130370 [details]
Patch

Clearing flags on attachment: 130370

Committed r109930: <http://trac.webkit.org/changeset/109930>
Comment 10 Andras Becsi 2012-03-06 09:55:01 PST
All reviewed patches have been landed.  Closing bug.