Bug 35142

Summary: [GTK] Some region checks in scroll are not required
Product: WebKit Reporter: Alejandro G. Castro <alex>
Component: WebKitGTKAssignee: Alejandro G. Castro <alex>
Severity: Normal CC: commit-queue, eric, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Description Flags
Proposed patch
Proposed patch none

Description Alejandro G. Castro 2010-02-19 02:56:01 PST
In the scroll method of ChromeClientGtk we are checking if the new region after scrolling the delta has parts that we already have in the window, in order to move them and invalidate the ones that are not already rendered. The gdk_window_move_region already does all that work so we are duplicating the effort.
Comment 1 Alejandro G. Castro 2010-02-19 03:00:25 PST
Created attachment 49062 [details]
Proposed patch

This patch removes the intersect check and the invalidate, leaving all the responsability to the gdk_window_move_region method.
Comment 2 Xan Lopez 2010-02-19 09:36:53 PST
Comment on attachment 49062 [details]
Proposed patch

Makes sense to me. I asked Alex to verify this works well in pre-CSW GTK+, and he said it does.

(BTW, typo in the ChangeLog, s/alredy/already/)
Comment 3 Eric Seidel (no email) 2010-02-19 16:23:19 PST
Attachment 49062 [details] was posted by a committer and has review+, assigning to Alejandro G. Castro for commit.
Comment 4 Alejandro G. Castro 2010-02-26 11:01:04 PST
This was committed in r55295, closing.
Comment 5 Alejandro G. Castro 2010-02-26 12:21:58 PST
Patch reverted (r55298), it was causing problems with the frames I have to review the patch.
Comment 6 Alejandro G. Castro 2010-03-09 01:15:25 PST
The refactoring has landed in the bug 34214, and we also have the tiled backing store 35146. Probably the best option is to implement the gtk+ support for the tiled backing store, I'm going to check it.
Comment 7 Alejandro G. Castro 2010-04-28 03:43:33 PDT
Created attachment 54545 [details]
Proposed patch

This patch avoids the invalidate and the operations related to it when we are using the region move function, it works with the frames.
Comment 8 Xan Lopez 2010-05-12 04:43:22 PDT
Comment on attachment 54545 [details]
Proposed patch

After talking with Alex we are going to add a comment explaining why we cannot rely only on gdk_moving_region, since its semantics do not quite match what we need in some cases (eg, when there are frames in the page).
Comment 9 WebKit Commit Bot 2010-05-12 23:15:19 PDT
Comment on attachment 54545 [details]
Proposed patch

Rejecting patch 54545 from commit-queue.

Unexpected failure when landing patch!  Please file a bug against webkit-patch.
Failed to run "['../WebKit/WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--build', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', 54545, '--test', '--parent-command=commit-queue', '--no-update']" exit_code: 1
Last 500 characters of output:
Fetching: https://bugs.webkit.org/show_bug.cgi?id=35142&ctype=xml
Processing 1 patch from 1 bug.
Cleaning working directory
Processing patch 54545 from bug 35142.
NOBODY (OOPS!) found in /Users/eseidel/Projects/CommitQueue/WebKit/gtk/ChangeLog does not appear to be a valid reviewer according to committers.py.
ERROR: /Users/eseidel/Projects/CommitQueue/WebKit/gtk/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).
Comment 10 Xan Lopez 2010-05-13 00:05:52 PDT
Comment on attachment 54545 [details]
Proposed patch

Second try.
Comment 11 Xan Lopez 2010-05-13 00:18:43 PDT
Any clue about these errors Eric? I seem to get them in all the commits I try to get in through the commit queue.
Comment 12 Xan Lopez 2010-05-13 00:19:30 PDT
(In reply to comment #11)
> Any clue about these errors Eric? I seem to get them in all the commits I try to get in through the commit queue.

Bah, nevermind, in this particular case it was a mistake, direct your attention to bug 31018.
Comment 13 Alejandro G. Castro 2010-05-13 02:21:53 PDT
The last patch it is not correct in some situations, think about a scroll that causes a region move of a small part of the bottom to the top, leaving a region in the middle, that central part is not invalidated in the gdk_window_move_region function.

Closing the bug as invalid, the initial small refactor makes no sense in this function.