[webkit-reviews] review denied: [Bug 80294] [Qt] Interaction Engine suspends content during pageload : [Attachment 130145] Patch

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has denied Allan Sandfeld Jensen
<allan.jensen at nokia.com>'s request for review:
Bug 80294: [Qt] Interaction Engine suspends content during pageload
https://bugs.webkit.org/show_bug.cgi?id=80294

Attachment 130145: Patch
https://bugs.webkit.org/attachment.cgi?id=130145&action=review

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
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.