AfterPaint currently fires anytime we scroll. This is not that helpful. The current plan is to try to use the style system to notify us when things change.

So I did some basic testing of this and it seems to work as advertised. We still, as expected, get these change notifications on link hover. This would cause us to repaint the entire screenshot if we only used this event. However, the right thing to do might be to use the combination.

It looks like there may be some problems with this approach. On this page: http://techcrunch.com/2012/04/19/mozilla-ceo-first-boot-to-gecko-devices-will-be-sold-in-brazil/ We get constant AppendChild calls which trigger this path.

I guess that should be http://m.techcrunch.com/2012/04/19/mozilla-ceo-first-boot-to-gecko-devices-will-be-sold-in-brazil/

It looks like the best thing to do here is use a frame tree generation number. When we get an after paint notification we check if the frame tree has changed since we last painted. If it has then we trigger the screenshot, if not, I believe we can safely ignore the AfterPaint notification.

(In reply to Jeff Muizelaar [:jrmuizel] from comment #3) > It looks like there may be some problems with this approach. On this page: > http://techcrunch.com/2012/04/19/mozilla-ceo-first-boot-to-gecko-devices- > will-be-sold-in-brazil/ > > We get constant AppendChild calls which trigger this path. To be perfectly clear on this, this will not be a bullet proof idea. There are tons of cases where the page can trigger layouts over and over again, without making anything actually change on the screen, and in many cases we're unable to detect this kind of change unless we've done almost all of the work associated with handling the dynamic change.

(In reply to Ehsan Akhgari [:ehsan] from comment #6) > (In reply to Jeff Muizelaar [:jrmuizel] from comment #3) > > It looks like there may be some problems with this approach. On this page: > > http://techcrunch.com/2012/04/19/mozilla-ceo-first-boot-to-gecko-devices- > > will-be-sold-in-brazil/ > > > > We get constant AppendChild calls which trigger this path. > > To be perfectly clear on this, this will not be a bullet proof idea. There > are tons of cases where the page can trigger layouts over and over again, > without making anything actually change on the screen, and in many cases > we're unable to detect this kind of change unless we've done almost all of > the work associated with handling the dynamic change. I think that will be fine if use this as mechanism for avoiding false positives from the current method. i.e. tracking document changes and paints both have false positives (we don't miss any changes that we would want to paint) and no false negatives as far as I know. However, each method has different false positives. If we combine these methods, we should be able to reduce our false positive rate to eliminate the unique false positives of each method. For example, scrolling causes painting false positives but not document changes so we should be able to ignore them. Unpainted document changes can also be ignored because we can catch them the next time we paint. The combination won't eliminate all false positives but it should be strictly better than what we have now and might just be good enough.

This uses the frame tree generation number to avoid taking screenshots when the frame tree hasn't changed. I also changed the frame tree generation to be a global. While ugly, this made things much easier because it exactly matches the lifetime of the event handler which is also global. It also fixes the problem of iframe generations being different.

This patch successfully stops all screenshot notification on a page like reddit commments where the page is static.

This is the same as the last patch with some additional comments added.

Is it not sufficient to hook into nsCSSFrameConstructor::BeginUpdate like the two other existing generation counters do?

(In reply to David Baron [:dbaron] from comment #12) > Is it not sufficient to hook into nsCSSFrameConstructor::BeginUpdate like > the two other existing generation counters do? You're right, we should do that instead.

Comment on attachment 639895 [details] [diff] [review] Use BeginUpdate instead of incrementing manually How about s/GenerationNumberForAndroidScreenshot/GlobalGenerationNumber/g ? And maybe add Global to the method to match, with a comment explaining that it's different from nsPresContext::GetDOMGeneration() in that it's a global rather than per-document? r=dbaron with that or something like it

And ideally there'd be a comment pointing the other direction from nsPresContext::GetDOMGeneration's declaration in nsPresContext.h.

(In reply to David Baron [:dbaron] from comment #15) > Comment on attachment 639895 [details] [diff] [review] > Use BeginUpdate instead of incrementing manually > > How about s/GenerationNumberForAndroidScreenshot/GlobalGenerationNumber/g ? > And maybe add Global to the method to match, with a comment explaining that > it's different from nsPresContext::GetDOMGeneration() in that it's a global > rather than per-document? > > r=dbaron with that or something like it That works well for me. I had also considered the name GlobalGenerationNumber, so I'll go with that.

Comment on attachment 639917 [details] [diff] [review] Add global frame tree generation [Approval Request Comment] Bug caused by (feature/regressing bug #): User impact if declined: Increased unnecessary screenshoting. Screenshotting is one of the biggest causes of checkerboarding on mobile today and this should help reduce some of that. Testing completed (on m-c, etc.): Only been on m-c for a little while. I'm going to do some more testing today. Risk to taking this patch (and alternatives if risky): Low risk. This is basically mobile only and the worst impact it should cause would be not screenshoting when we should. String or UUID changes made by this patch: None

(In reply to Jeff Muizelaar [:jrmuizel] from comment #21) > User impact if declined: Increased unnecessary screenshoting. Screenshotting > is one of the biggest causes of checkerboarding on mobile today and this > should help reduce some of that. Do we have data to back up the % difference we'll get here? The reward would need to overcome the risk you've outlined below > Risk to taking this patch (and alternatives if risky): Low risk. This is > basically mobile only and the worst impact it should cause would be not > screenshoting when we should. which is actually significant since we've already set expectations with 14.0 (and users won't notice if we don't raise the bar in 14.0.1 here).

Comment on attachment 639917 [details] [diff] [review] Add global frame tree generation This will miss our final 14.0.1 beta, and was not critical for release.