If, during handling of a reflow-on-zoom event, another reflow-on-zoom event is triggered, the events begin to build up in a queue. We should try to fix this more intelligently, so that a series of events doesn't cause an inordinate lag to the user. An example would be to pinch-zoom on the url testcase, then pinch-zoom out, then pinch-zoom in to a different location, then pinch-zoom out, etc... ad nauseum. Right now, we handle each of these events in browser.js individually, where we should try and jump to the end of the queue (i.e. skip the changing of max line box width when another max line box width change is in the queue).

I'm not _exactly_ sure how to check if there is another event in the pipeline right now, but this should be something fairly easy we can do to drastically improve reflow-on-zoom performance.

I was thinking about what would be the best way to do this. The best I could come up with was that when browser.js is to have the PZC keep a generation counter for the number of pinches that have occurred, and to send the generation number with any pinch events to browser.js. Then when browser.js is about to process a pinch, it can synchronously query Java to get the latest generation number, and if it is newer than the pinch it is about to process, it drops that pinch and waits for the next one (which should already be queued). It's not the cleanest solution, though, and will give me nightmares later when I'm unifying the PZC with the B2G version.

That makes my stomach churn as well. I think the problem isn't necessarily pinch zooms queuing up, but reflows queuing up. The thing to do here would seem to be to cancel a reflow when it has become obvious that the results of that reflow are no longer needed. Given that we have interruptable reflows, we should be able to process the queue of gesture events and when one calls for yet another reflow cancel any pending reflows.

Similar to my comment on this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=836565#c7 We seem to be hypothesizing about what is causing these issues (events, reflows, invalidation, aliens?) Please attach a profile that indicates the operations that occur and the %time spent in each operation.

This patch adds a timer after reflow-on-zoom happens to prevent a buildup of events if a user performs multiple pinch gestures in quick succession. The amount of time to wait before allowing reflow-on-zoom to continue is controlled by a pref that defaults to 500 ms. We might want to play with this a bit, especially once the double-tap to reflow lands.

Doesn't this patch mean that the *first* pinch in a quick series of pinches will trigger the reflow, rather than the *last* pinch? i.e. if the user pinches to 500px wide, and then immediately pinches again to 200px wide, the end result will be that the text will be laid out with a max line width of 500px, right?

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #6) > Doesn't this patch mean that the *first* pinch in a quick series of pinches > will trigger the reflow, rather than the *last* pinch? i.e. if the user > pinches to 500px wide, and then immediately pinches again to 200px wide, the > end result will be that the text will be laid out with a max line width of > 500px, right? Yes, that's correct. I'm not entirely sure how we could do it with the *last* one, since we can't really cancel a reflow that's in progress. We'd have to queue additional reflows if one is already in progress, and then just jump to the last one, I guess. The problem, though, is that we could have quite a few of these. Returning (IMHO) with a slightly wrong size for the max line box width is better than hanging indefinitely, though...

The way I'm thinking of to do it is like this: performReflowOnZoom: function(...) { ... let viewportWidth = ...; gDesiredMaxLineWidth = viewportWidth - 15; if (gReflowPending) { clearTimeout(gReflowPending); } gReflowPending = setTimeout(doChangeMaxLineBoxWidth, 500); } doChangeMaxLineBoxWidth: function() { gReflowPending = null; docViewer.changeMaxLineBoxWidth(gDesiredMaxLineWidth); } So basically, when we get the call to performReflowOnZoom, we calculate what the desired max line width is, but don't actually set it for another 500ms (or whatever delay). If, during that time, we get additional pinches, we keep updating our "desired" max line width and deferring the actual setting of the max line width. Then, once the 500ms have expired with no more calls to performReflowOnZoom, we do the actual (expensive) setting of the line box width. What do you think?

Yeah, I think this is a better version of what I was trying to do. Let me implement it and see how it works.

Ah, right... So, now I remember why this doesn't work. I thought I had tried it that way before doing it the way I did, and I couldn't remember why. So, as in the other bug that I requested review on, the order of events is important here. We do the reflow on zoom inside of setViewport. But, the position maintenance happens in onPinchFinish, which is called after setViewport for a given pinch event. So, if we do it the way you suggest in comment 8, we run into a problem where the position maintenance code executes before the reflow-on-zoom has had a chance to run, meaning that we don't correctly maintain position after reflow has happened. I'm looking into this, though, to see if there is a way we could still get it to work by reordering when the reflow position maintenance code executes...

Ok, here's a better version, implementing what you were previously talking about, kats. I think that this is slightly better, but I'm worried about the 500ms delay that's being added in to EVERY reflow-on-zoom operation. Perhaps we should make it something like 250ms instead. There's definitely a tradeoff here - we're actually not performing the reflow until this timer expires, which adds this to the total time the reflow takes every time we do it, but on the other hand, shorter values for it could cause the successive queue to build up, since we're not allowing as much time between what we consider to be "successive" gestures.

Comment on attachment 731206 [details] [diff] [review] b836568 (v2) Review of attachment 731206 [details] [diff] [review]: ----------------------------------------------------------------- Some comments below but overall this looks much better. I agree that adding 500ms to every reflow is going to be kind of painful though - maybe we can mitigate some of the user perception issue with appropriate UI feedback at the setTimeout call point? ::: mobile/android/chrome/content/browser.js @@ +115,5 @@ > +} > + > +function doDelayedPositionMaintenance() { > + let range = BrowserApp.selectedTab._mReflozPoint.range; > + BrowserEventHandler._zoomInAndSnapToRange(range); Instead of having this as a separate function running on its own independent setTimeout timer, can you just move this code into the end of doChangeMaxLineBoxWidth? @@ +2452,5 @@ > // We add in a bit of fudge just so that the end characters don't accidentally > // get clipped. 15px is an arbitrary choice. > + gDesiredMaxLineBoxWidth = viewportWidth - 15; > + let reflozTimeout = Services.prefs.getIntPref("browser.zoom.reflowZoom.reflowTimeout"); > + gReflowPending = setTimeout(doChangeMaxLineBoxWidth, reflozTimeout); According to the setTimeout documentation on MDN, you can pass extra arguments that will get passed to the function. If that works, you could something like: setTimeout(doChangeMaxLineBoxWidth, reflozTimeout, viewportWidth - 15); and change doChangeMaxLineBoxWidth to take a parameter which would be the desired max line box width. Not a big deal but it'll eliminate the need to keep gDesiredMaxLineBoxWidth as a global variable. @@ +3925,5 @@ > > _zoomInAndSnapToRange: function(aRange) { > + if (gReflowPending) { > + setTimeout(doDelayedPositionMaintenance, 250); > + return; See above. If you call _zoomInAndSnapToRange directly at the end of doChangeMaxLineBoxWidth then you shouldn't need this setTimeout. You can probably kill the onPinchFinish method entirely as well.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #12) > Comment on attachment 731206 [details] [diff] [review] > b836568 (v2) > > Review of attachment 731206 [details] [diff] [review]: > ----------------------------------------------------------------- > > Some comments below but overall this looks much better. I agree that adding > 500ms to every reflow is going to be kind of painful though - maybe we can > mitigate some of the user perception issue with appropriate UI feedback at > the setTimeout call point? Yeah, I think this is what we need. However, as this will probably need to be looked over by UX, I'm tracking this in a different bug - bug 861362. > Instead of having this as a separate function running on its own independent > setTimeout timer, can you just move this code into the end of > doChangeMaxLineBoxWidth? Changed. > According to the setTimeout documentation on MDN, you can pass extra > arguments that will get passed to the function. If that works, you could > something like: > > setTimeout(doChangeMaxLineBoxWidth, reflozTimeout, viewportWidth - 15); > > and change doChangeMaxLineBoxWidth to take a parameter which would be the > desired max line box width. Not a big deal but it'll eliminate the need to > keep gDesiredMaxLineBoxWidth as a global variable. Changed. > See above. If you call _zoomInAndSnapToRange directly at the end of > doChangeMaxLineBoxWidth then you shouldn't need this setTimeout. You can > probably kill the onPinchFinish method entirely as well. Done, and removed onPinchFinish.

Comment on attachment 740433 [details] [diff] [review] b836568 (v3) Review of attachment 740433 [details] [diff] [review]: ----------------------------------------------------------------- Much cleaner, thanks! ::: mobile/android/chrome/content/browser.js @@ +149,5 @@ > function sendMessageToJava(aMessage) { > return getBridge().handleGeckoMessage(JSON.stringify(aMessage)); > } > > +function doChangeMaxLineBoxWidth (aWidth) { nit: remove the space before "("

Inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/950e522a7937