Bug 78602 - [Qt] Page doesn't get repainted while panning is in progress
Summary: [Qt] Page doesn't get repainted while panning is in progress
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Kenneth Rohde Christiansen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-14 06:41 PST by Hugo Parente Lima
Modified: 2012-02-23 09:59 PST (History)
4 users (show)

See Also:


Attachments
Patch (12.29 KB, patch)
2012-02-14 07:00 PST, Hugo Parente Lima
no flags Details | Formatted Diff | Diff
Patch (12.31 KB, patch)
2012-02-14 08:17 PST, Hugo Parente Lima
no flags Details | Formatted Diff | Diff
Patch (12.26 KB, patch)
2012-02-15 06:08 PST, Hugo Parente Lima
no flags Details | Formatted Diff | Diff
Patch (18.47 KB, patch)
2012-02-23 05:50 PST, Kenneth Rohde Christiansen
no flags Details | Formatted Diff | Diff
Patch (18.44 KB, patch)
2012-02-23 06:05 PST, Kenneth Rohde Christiansen
hausmann: review+
hausmann: commit-queue+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hugo Parente Lima 2012-02-14 06:41:55 PST
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.
Comment 1 Hugo Parente Lima 2012-02-14 07:00:34 PST
Created attachment 126971 [details]
Patch
Comment 2 WebKit Review Bot 2012-02-14 07:03:34 PST
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.
Comment 3 Hugo Parente Lima 2012-02-14 08:17:10 PST
Created attachment 126980 [details]
Patch
Comment 4 WebKit Review Bot 2012-02-14 08:19:56 PST
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.
Comment 5 Kenneth Rohde Christiansen 2012-02-15 04:48:32 PST
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.
Comment 6 Hugo Parente Lima 2012-02-15 06:08:08 PST
Created attachment 127171 [details]
Patch
Comment 7 WebKit Review Bot 2012-02-15 06:11:06 PST
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.
Comment 8 Kenneth Rohde Christiansen 2012-02-23 05:44:41 PST
Comment on attachment 127171 [details]
Patch

r- wrong solution
Comment 9 Kenneth Rohde Christiansen 2012-02-23 05:50:31 PST
Created attachment 128465 [details]
Patch
Comment 10 Simon Hausmann 2012-02-23 05:59:56 PST
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?
Comment 11 Kenneth Rohde Christiansen 2012-02-23 06:02:55 PST
(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)
Comment 12 Kenneth Rohde Christiansen 2012-02-23 06:04:50 PST
LayerTreeHostQt::setVisibleContentRectForPanning(const IntRect& rect, const FloatPoint& trajectoryVector)
LayerTreeHostQt::setVisibleContentRectForScaling(const IntRect& rect, float scale)

I mean :-)
Comment 13 Simon Hausmann 2012-02-23 06:05:22 PST
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 :)
Comment 14 Kenneth Rohde Christiansen 2012-02-23 06:05:40 PST
Created attachment 128468 [details]
Patch
Comment 15 Kenneth Rohde Christiansen 2012-02-23 07:11:53 PST
Landed in r108625.
Comment 16 Hugo Parente Lima 2012-02-23 09:52:17 PST
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().
Comment 17 Kenneth Rohde Christiansen 2012-02-23 09:57:01 PST
(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? :-)
Comment 18 Hugo Parente Lima 2012-02-23 09:59:07 PST
Yes, I can do that.