Bug 78602 – [Qt] Page doesn't get repainted while panning is in progress

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Hugo Parente Lima]

If you pan a large webpage it will trigger a repaint only when the panning stop and the user will see large blank areas until the panning stop.

[Hugo Parente Lima]

Created attachment 126971 [details]
Patch

[WebKit Review Bot]

Attachment 126971 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets
Using index info to reconstruct a base tree...
<stdin>:1578: trailing whitespace.
        
<stdin>:1647: trailing whitespace.
    
<stdin>:1657: trailing whitespace.
    
<stdin>:1672: trailing whitespace.
        return 0;        
<stdin>:1674: trailing whitespace.
    
warning: squelched 7 whitespace errors
warning: 12 lines add whitespace errors.
Falling back to patching base and 3-way merge...
warning: too many files (created: 168776 deleted: 3), skipping inexact rename detection
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Auto-merging Source/WebKit2/ChangeLog
CONFLICT (content): Merge conflict in Source/WebKit2/ChangeLog
Auto-merging Tools/ChangeLog
CONFLICT (content): Merge conflict in Tools/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Hugo Parente Lima]

Created attachment 126980 [details]
Patch

[WebKit Review Bot]

Attachment 126980 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
From git://git.webkit.org/WebKit
   1475f6d..6640514  master     -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 107710 = 1475f6d06ac1bdc58f6b7b27ade48d967e113e10
r107712 = 66405148c5f7c19d2179ddbd3d3d5ee458d4b6d6
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Applying: [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets
Using index info to reconstruct a base tree...
<stdin>:1578: trailing whitespace.
        
<stdin>:1647: trailing whitespace.
    
<stdin>:1657: trailing whitespace.
    
<stdin>:1672: trailing whitespace.
        return 0;        
<stdin>:1674: trailing whitespace.
    
warning: squelched 7 whitespace errors
warning: 12 lines add whitespace errors.
Falling back to patching base and 3-way merge...
warning: too many files (created: 168776 deleted: 3), skipping inexact rename detection
Auto-merging LayoutTests/ChangeLog
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Auto-merging Source/WebKit2/ChangeLog
CONFLICT (content): Merge conflict in Source/WebKit2/ChangeLog
Auto-merging Tools/ChangeLog
CONFLICT (content): Merge conflict in Tools/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kenneth Rohde Christiansen]

Comment on attachment 126980 [details]
Patch

This is wrong, you do not need the page to be resumed in order to paint. you just need to emit the viewportTrajectoryVectorChanged and that should be it.

[Hugo Parente Lima]

Created attachment 127171 [details]
Patch

[WebKit Review Bot]

Attachment 127171 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Last 3072 characters of output:

	M	ChangeLog
	M	LayoutTests/platform/chromium-linux/tables/mozilla/bugs/bug11026-expected.png
	M	LayoutTests/platform/chromium-linux/tables/mozilla/bugs/bug10565-expected.png
	M	LayoutTests/platform/chromium-linux/tables/mozilla/bugs/bug1188-expected.png
	M	LayoutTests/platform/chromium/test_expectations.txt
	A	LayoutTests/platform/chromium-mac-snowleopard/tables/mozilla/bugs/bug11026-expected.png
	A	LayoutTests/platform/chromium-mac-snowleopard/tables/mozilla/bugs/bug10565-expected.png
	M	LayoutTests/platform/chromium-mac-snowleopard/tables/mozilla/bugs/bug1188-expected.png
	D	LayoutTests/platform/chromium-mac/tables/mozilla/bugs/bug11026-expected.png
	D	LayoutTests/platform/chromium-mac/tables/mozilla/bugs/bug10565-expected.png
	M	LayoutTests/platform/chromium-mac-leopard/tables/mozilla/bugs/bug11026-expected.png
	M	LayoutTests/platform/chromium-mac-leopard/tables/mozilla/bugs/bug10565-expected.png
	M	LayoutTests/platform/chromium-mac-leopard/tables/mozilla/bugs/bug1188-expected.png
	M	LayoutTests/platform/chromium-win/tables/mozilla/bugs/bug1188-expected.png
	M	LayoutTests/platform/chromium-win/tables/mozilla/bugs/bug11026-expected.png
	M	LayoutTests/platform/chromium-win/tables/mozilla/bugs/bug10565-expected.png
	A	LayoutTests/inspector/elements/resolve-node-blocked-expected.txt
	A	LayoutTests/inspector/elements/resolve-node-blocked.html
	M	LayoutTests/ChangeLog
	M	Source/cmake/FindCFLite.cmake
	M	Source/WebCore/ChangeLog
	M	Source/WebCore/inspector/InjectedScript.cpp
	M	Source/WebCore/inspector/InjectedScript.h
W: -empty_dir: trunk/LayoutTests/platform/chromium-mac/tables/mozilla/bugs/bug10565-expected.png
W: -empty_dir: trunk/LayoutTests/platform/chromium-mac/tables/mozilla/bugs/bug11026-expected.png
r107809 = 26c3dbf4d929380fdd9660a6218cc2abc86f8070 (refs/remotes/origin/master)
First, rewinding head to replay your work on top of it...
Applying: Web Inspector: crash when inspecting an element on a page with eval disabled by CSP
Using index info to reconstruct a base tree...
<stdin>:35: new blank line at EOF.
+
warning: 1 line adds whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Auto-merging Source/WebCore/inspector/InjectedScript.cpp
CONFLICT (content): Merge conflict in Source/WebCore/inspector/InjectedScript.cpp
Auto-merging Source/WebCore/inspector/InjectedScript.h
CONFLICT (content): Merge conflict in Source/WebCore/inspector/InjectedScript.h
Failed to merge in the changes.
Patch failed at 0001 Web Inspector: crash when inspecting an element on a page with eval disabled by CSP

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kenneth Rohde Christiansen]

Comment on attachment 127171 [details]
Patch

r- wrong solution

[Kenneth Rohde Christiansen]

Created attachment 128465 [details]
Patch

[Simon Hausmann]

Comment on attachment 128465 [details]
Patch

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

A couple of questions first :)

> Source/WebKit2/UIProcess/DrawingAreaProxy.h:97
> +    virtual void setVisibleContentRectTrajectoryVector(const WebCore::IntRect& visibleContentsRect, const WebCore::FloatPoint&) { }

Hm, if you add the variable name to the first argument, why not the second?

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:206
> +    connect(m_flickProvider, SIGNAL(contentXChanged()), SLOT(flickableMovingPositionUpdate()), Qt::DirectConnection);
> +    connect(m_flickProvider, SIGNAL(contentYChanged()), SLOT(flickableMovingPositionUpdate()), Qt::DirectConnection);

IMHO we should leave out the DirectConnection parameters from connect() unless we are knowingly in a threaded-context.

> Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.cpp:-424
> -    if (rect == m_visibleContentsRect && scale == m_contentsScale)
> -        return;

Hm, why did you remove the check? Is there a reason why schedulerLayerFlush() needs to be called even if the parameters haven't changed?

> Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.cpp:434
> +void LayerTreeHostQt::setVisibleContentRectTrajectoryVector(const IntRect& rect, const FloatPoint& trajectoryVector)

Just thinking, should it be called setVisibleContentRectAndTrajectoryVector?

[Kenneth Rohde Christiansen]

(In reply to comment #10)
> (From update of attachment 128465 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=128465&action=review
> 
> A couple of questions first :)
> 
> > Source/WebKit2/UIProcess/DrawingAreaProxy.h:97
> > +    virtual void setVisibleContentRectTrajectoryVector(const WebCore::IntRect& visibleContentsRect, const WebCore::FloatPoint&) { }
> 
> Hm, if you add the variable name to the first argument, why not the second?

Maybe I was too mechanical :-)

> 
> > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:206
> > +    connect(m_flickProvider, SIGNAL(contentXChanged()), SLOT(flickableMovingPositionUpdate()), Qt::DirectConnection);
> > +    connect(m_flickProvider, SIGNAL(contentYChanged()), SLOT(flickableMovingPositionUpdate()), Qt::DirectConnection);
> 
> IMHO we should leave out the DirectConnection parameters from connect() unless we are knowingly in a threaded-context.

OK, though andras added them all over

> 
> > Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.cpp:-424
> > -    if (rect == m_visibleContentsRect && scale == m_contentsScale)
> > -        return;
> 
> Hm, why did you remove the check? Is there a reason why schedulerLayerFlush() needs to be called even if the parameters haven't changed?

Because two methods are used...so if I want to do this check I will need to store the trajectory vector as well and compare it. Not really worth it.

> 
> > Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.cpp:434
> > +void LayerTreeHostQt::setVisibleContentRectTrajectoryVector(const IntRect& rect, const FloatPoint& trajectoryVector)
> 
> Just thinking, should it be called setVisibleContentRectAndTrajectoryVector?

I didn't want to do the renaming, but I would suggest that we go for

LayerTreeHostQt::setVisibleContentRectForPanning(const FloatPoint& trajectoryVector)
LayerTreeHostQt::setVisibleContentRectForScaling(const IntRect& rect, float scale)

[Kenneth Rohde Christiansen]

LayerTreeHostQt::setVisibleContentRectForPanning(const IntRect& rect, const FloatPoint& trajectoryVector)
LayerTreeHostQt::setVisibleContentRectForScaling(const IntRect& rect, float scale)

I mean :-)

[Simon Hausmann]

Comment on attachment 128465 [details]
Patch

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

>>> Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.cpp:434
>>> +void LayerTreeHostQt::setVisibleContentRectTrajectoryVector(const IntRect& rect, const FloatPoint& trajectoryVector)
>> 
>> Just thinking, should it be called setVisibleContentRectAndTrajectoryVector?
> 
> I didn't want to do the renaming, but I would suggest that we go for
> 
> LayerTreeHostQt::setVisibleContentRectForPanning(const FloatPoint& trajectoryVector)
> LayerTreeHostQt::setVisibleContentRectForScaling(const IntRect& rect, float scale)

_Those_ sounds like nice and descriptive names :)

[Kenneth Rohde Christiansen]

Created attachment 128468 [details]
Patch

[Kenneth Rohde Christiansen]

Landed in r108625.

[Hugo Parente Lima]

The Flickable component has movementStarted() and movementEnded() signals, I think they can be used to avoid multiple calls to flickableMovingStateChanged() due to multiple emissions of movingChanged().

[Kenneth Rohde Christiansen]

(In reply to comment #16)
> The Flickable component has movementStarted() and movementEnded() signals, I think they can be used to avoid multiple calls to flickableMovingStateChanged() due to multiple emissions of movingChanged().

Up to doing a patch? :-)

[Hugo Parente Lima]

Yes, I can do that.