Closed Bug 701623 Opened 12 years ago Closed 12 years ago
Using dirty rects for repainting causes corruption
See cnn.com; the header gets duplicated.
OS: Mac OS X → Android
Priority: -- → P3
Hardware: x86 → ARM
I've been hearing lots of reports of this anecdotally. I think we should consider making this higher priority.
Here's a patch for this. It combines several things, that all contribute to fix this bug in most cases: * Paints all dirty rects. * Moves pan logic into the widget code and bundles it with the paint events, so that we don't have races. It also refactors some of the GL tiles code, for good measure. There are still redraw issues when moving from page to page, but this is much better in my experience. I don't see the header duplicated on cnn.com, for example.
Assignee: nobody → pwalton
Status: NEW → ASSIGNED
Attachment #574806 - Flags: review?(chrislord.net)
Actually, on further thought, the translate ought to be ok. I'm actually not sure why you've set the window width/height so large? Surely it just needs to be (tileWidth + (tileWidth - viewportWidth)) x (tileHeight + (tileHeight - viewportHeight)) (i.e. the tile size plus the largest possible viewport offset)?
And on even closer inspection, I didn't realise this patch removed setting of the scroll coordinates - which explains the need for a massive window, why it removes race conditions, etc. I don't think this is going to work unless we have a way to separate viewport size from element size. Because the page's width depends on the viewport width, we can't really size the page at the document width - so we can't really disable horizontal scrolling. Any scroll offset will break this patch. The part of this patch that sets width/height attributes on the browser doesn't work - a child element of a deck will always assume the size of the deck, regardless of its width/height. Really, I think we want the displayport-style method; We have a render area (the displayport), we have an element which is a sub-rectangle of this area (the viewport), and we scroll the document to expose the document area (and optionally move the viewport to wherever we believe is most optimal). I think we'd be better off finding a way to send the metrics for these transforms (as is done in JS in the displayport patch) synchronously with the endDrawing call. Doing the viewport offset as is done in this patch is probably the way to go, we also, I suppose, need to find a reliable way of getting the browser frame so that we can send the scroll coordinates as well.
(In reply to Chris Lord [:cwiiis] from comment #5) > And on even closer inspection, I didn't realise this patch removed setting > of the scroll coordinates - which explains the need for a massive window, > why it removes race conditions, etc. > > I don't think this is going to work unless we have a way to separate > viewport size from element size. > > Because the page's width depends on the viewport width, we can't really size > the page at the document width - so we can't really disable horizontal > scrolling. Any scroll offset will break this patch. > > The part of this patch that sets width/height attributes on the browser > doesn't work - a child element of a deck will always assume the size of the > deck, regardless of its width/height. > > Really, I think we want the displayport-style method; We have a render area > (the displayport), we have an element which is a sub-rectangle of this area > (the viewport), and we scroll the document to expose the document area (and > optionally move the viewport to wherever we believe is most optimal). > > I think we'd be better off finding a way to send the metrics for these > transforms (as is done in JS in the displayport patch) synchronously with > the endDrawing call. > > Doing the viewport offset as is done in this patch is probably the way to > go, we also, I suppose, need to find a reliable way of getting the browser > frame so that we can send the scroll coordinates as well. I guess we have these options: 1. Continue down the road of trying to squash duplicate frames and keep all the synchronization in the Java side (by dropping frames, etc). 2. This patch's approach: move all the panning code into widget. 3. Remove the paint code in widget. Have painting strictly controlled by browser.js, which adds the tile offset and zoom factor to each paint event before sending it off to Java. (1) is a real nightmare, as we've discovered. It's also wasteful for performance. (2) seems to work (and better than (1); this patch really makes a big difference), but it does have the downsides you mention (breaking scrolling, massive window leading to ugly XUL), and zoom is going to be a bit annoying since we have to poke the PresShell, which isn't something that's straightforward to do from widget. (3) is attractive for a lot of reasons -- it removes races while keeping the CSS transform approach. Is this what you were trying to do with MozAfterPaint? How did it go?
(3) is what I tried, but it ends up that MozAfterPaint isn't tied directly to window draws, so what would often happen is; MozAfterPaint <viewport metrics 1> MozAfterPaint <viewport metrics 2> endDrawing <viewport metrics 1> endDrawing <viewport metrics 2> or even, two MozAfterPaints and one endDrawing, the other way round, and various other combinations - basically, it doesn't work :( I think a fourth option, - Do the viewport offset how you've done it in this patch - but never change it. Always assume that the viewport offset places the viewport in the centre of the display port. If we do it how you've done here, changing it means we have to force Gecko to redraw everything, so it would outweigh the benefit of having a larger buffered area at the edges of the page. - Size the window so that it's (tileWidth + (tileWidth - viewportWidth)) x (tileHeight + (tileHeight - viewportHeight)), as mentioned in the above comment. This should mitigate any problems with setting the viewport transform via a translate that the display list builder is unaware of. - Before calling endDrawing, in the native code, read the scroll position of the browser frame and send the coordinates back with the draw call. We should be able to do this - we may want to mark the browser frame somehow in JS so it's easy to find in the native code (I don't know how to do this, but in the worst case, we could add something to nsIDOMWindowUtils) - Continue to send scroll requests/viewport resizes to browser.js, and continue sending page-size back from browser.js. Page size needn't be perfectly in sync, so there's no need to complicate things further. We could add this in the native code if it's easy, however. Any thoughts on that?
(In reply to Chris Lord [:cwiiis] from comment #7) > (3) is what I tried, but it ends up that MozAfterPaint isn't tied directly > to window draws, so what would often happen is; > > MozAfterPaint <viewport metrics 1> > MozAfterPaint <viewport metrics 2> > endDrawing <viewport metrics 1> > endDrawing <viewport metrics 2> > > or even, two MozAfterPaints and one endDrawing, the other way round, and > various other combinations - basically, it doesn't work :( This doesn't seem right. Are you sending widget-level paint events other than outside these endDrawing calls? nsViewManager::Refresh has an nsAutoScriptBlocker. nsPresShell::Paint via nsPresContext::NotifyDidPaintForSubtree uses AddScriptRunner to dispatch the MozAfterPaint event(s). Those runnables should run when nsViewManager::Refresh releases its scriptblocker, i.e., during the dispatch of the paint event from the widget. > Any thoughts on that? Sounds reasonable.
This should now be fixed with the patch in bug 703821.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment on attachment 574806 [details] [diff] [review] Proposed patch. Obsoleting patch and removing feedback request. (In reply to Patrick Walton (:pcwalton) from comment #9) > This should now be fixed with the patch in bug 703821.
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.