Bug 35142 – [GTK] Some region checks in scroll are not required

• Status: RESOLVED
• Resolution: INVALID
• Component: WebKitGTK

[Alejandro G. Castro]

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.

[Alejandro G. Castro]

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.

[Xan Lopez]

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/)

[Eric Seidel (no email)]

Attachment 49062 [details] was posted by a committer and has review+, assigning to Alejandro G. Castro for commit.

[Alejandro G. Castro]

This was committed in r55295, closing.

[Alejandro G. Castro]

Patch reverted (r55298), it was causing problems with the frames I have to review the patch.

[Alejandro G. Castro]

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.

[Alejandro G. Castro]

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.

[Xan Lopez]

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).

[WebKit Commit Bot]

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:
=54545&action=edit
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).

[Xan Lopez]

Comment on attachment 54545 [details]
Proposed patch

Second try.

[Xan Lopez]

Any clue about these errors Eric? I seem to get them in all the commits I try to get in through the commit queue.

[Xan Lopez]

(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.

[Alejandro G. Castro]

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.