Bug 6388 - REGRESSION: Incomplete repaint when dragging the map on Google maps
Summary: REGRESSION: Incomplete repaint when dragging the map on Google maps
Status: VERIFIED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P1 Major
Assignee: Nobody
URL: http://maps.google.com/
Keywords:
Depends on:
Blocks:
 
Reported: 2006-01-05 14:06 PST by mitz
Modified: 2006-01-31 21:20 PST (History)
1 user (show)

See Also:


Attachments
Reduced testcase (536 bytes, text/html)
2006-01-05 16:10 PST, mitz
no flags Details
Possible fix (2.23 KB, patch)
2006-01-24 15:55 PST, mitz
darin: review-
Details | Formatted Diff | Diff
Updated patch (4.61 KB, patch)
2006-01-25 08:27 PST, mitz
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2006-01-05 14:06:14 PST
If you drag the map slowly straight down (or straight to the right), it "smears".
Comment 1 mitz 2006-01-05 14:17:51 PST
The regression happened between Nov 6 and Nov 8 so I'm guessing it's from fixing bug 5633.
Comment 2 mitz 2006-01-05 16:10:35 PST
Created attachment 5506 [details]
Reduced testcase

The optimized repaint code thinks that the image only resized, rather than
moved.
Comment 3 Alice Liu 2006-01-11 13:25:43 PST
<rdar://problem/4405688>
Comment 4 mitz 2006-01-24 15:55:40 PST
Created attachment 5931 [details]
Possible fix

This fixes the Google maps site and the testcase. I think the bug (mistaking a move for a resize due to clipping) can only happen to objects that have a layer, since otherwise repaintDuringLayoutIfMoved handles it.

If the patch is good, please r- and I'll resubmit with a ChangeLog entry and a manual test (unfortunately repaint issues can't be tested automatically).
Comment 5 Darin Adler 2006-01-24 19:29:21 PST
Comment on attachment 5931 [details]
Possible fix

Looks good. I think you can move on to the next step on this one.
Comment 6 Dave Hyatt 2006-01-24 20:19:28 PST
Could you explain this further?  I don't see why these extra fields are necessary.
Comment 7 Dave Hyatt 2006-01-24 20:23:25 PST
This is marked as a regression.  Was it caused by the checkin I did a month or two ago to optimize repainting when a block resized but didn't move?
Comment 8 mitz 2006-01-24 22:15:24 PST
(In reply to comment #7)
> This is marked as a regression.  Was it caused by the checkin I did a month or
> two ago to optimize repainting when a block resized but didn't move?

Yes. There are several of those, some fixed, some still not.

(In reply to comment #6)
> Could you explain this further?  I don't see why these extra fields are
> necessary.
 
Using absolute repaint rects, all repaintAfterLayoutIfNeeded sees is a resize, due to clipping. This extra info is what's needed to break the move/resize ambiguity.

There is at least one other case where repaintAfterLayoutIfNeeded lacks enough information to determine what really changed (see bug 6770), so perhaps repaintAfterLayoutIfNeeded will need to take extra arguments (e.g. the object's unclipped rect in absolute coordinates), in which case layers will need to cache that info.
Comment 9 Dave Hyatt 2006-01-24 22:50:55 PST
Ok, yeah I see now.  I think I might rename the member variables m_repaintX and m_repaintY or something like that, since they are really just cached values for the repaint check step.
Comment 10 mitz 2006-01-25 08:27:36 PST
Created attachment 5946 [details]
Updated patch

Renamed member variables and added ChangeLog and manual test.
Comment 11 Darin Adler 2006-01-25 09:38:47 PST
Comment on attachment 5946 [details]
Updated patch

Nice. Lets go with this. r=me

Onward to the other repaint problems. :-)
Comment 12 mitz 2006-01-27 01:02:13 PST
Verified in r12402 nightly
Comment 13 Eric Seidel (no email) 2006-01-31 21:20:54 PST
Removing Regression keyword from bugs already fixed.