764188 - Use a better event for trigger java screenshots

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Attachment: The basic idea

Comment 2

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 4

[Jeff Muizelaar [:jrmuizel]]

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

Comment 5

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 6

[(no longer active)]

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

Comment 7

[Jeff Muizelaar [:jrmuizel]]

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

Comment 8

[(no longer active)]

Attachment: Patch (v1)

Comment 9

[Jeff Muizelaar [:jrmuizel]]

Attachment: Use the frame tree generation number

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.

Comment 10

[Jeff Muizelaar [:jrmuizel]]

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

Comment 11

[Jeff Muizelaar [:jrmuizel]]

Attachment: Use the frame tree generation number to avoid unnecessary screenshotting

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

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 13

[(no longer active)]

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

[Jeff Muizelaar [:jrmuizel]]

Attachment: Use BeginUpdate instead of incrementing manually

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 17

[Jeff Muizelaar [:jrmuizel]]

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

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add global frame tree generation

Comment 19

[Jeff Muizelaar [:jrmuizel]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/002efc873e32

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/002efc873e32

Comment 21

[Jeff Muizelaar [:jrmuizel]]

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

Comment 22

[Alex Keybl [:akeybl]]

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

[Alex Keybl [:akeybl]]

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.

Comment 24

[Jeff Muizelaar [:jrmuizel]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f94c4dfc1069