Last Comment Bug 764188 - Use a better event for trigger java screenshots
: Use a better event for trigger java screenshots
Status: RESOLVED FIXED
[games:p1]
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: Firefox 16
Assigned To: Jeff Muizelaar [:jrmuizel]
:
Mentors:
Depends on:
Blocks: gecko-games
  Show dependency treegraph
 
Reported: 2012-06-12 15:35 PDT by Jeff Muizelaar [:jrmuizel]
Modified: 2013-11-06 14:12 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
The basic idea (1.05 KB, patch)
2012-06-12 15:41 PDT, Jeff Muizelaar [:jrmuizel]
no flags Details | Diff | Splinter Review
Patch (v1) (2.07 KB, patch)
2012-06-22 12:20 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
Use the frame tree generation number (3.44 KB, patch)
2012-06-29 12:30 PDT, Jeff Muizelaar [:jrmuizel]
no flags Details | Diff | Splinter Review
Use the frame tree generation number to avoid unnecessary screenshotting (3.84 KB, patch)
2012-07-04 12:37 PDT, Jeff Muizelaar [:jrmuizel]
no flags Details | Diff | Splinter Review
Use BeginUpdate instead of incrementing manually (3.37 KB, patch)
2012-07-06 18:55 PDT, Jeff Muizelaar [:jrmuizel]
dbaron: review+
Details | Diff | Splinter Review
Add global frame tree generation (3.78 KB, patch)
2012-07-06 20:33 PDT, Jeff Muizelaar [:jrmuizel]
jmuizelaar: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta-
Details | Diff | Splinter Review

Description Jeff Muizelaar [:jrmuizel] 2012-06-12 15:35:15 PDT
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] 2012-06-12 15:41:59 PDT
Created attachment 632449 [details] [diff] [review]
The basic idea
Comment 2 Jeff Muizelaar [:jrmuizel] 2012-06-20 13:19:48 PDT
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] 2012-06-20 13:54:47 PDT
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] 2012-06-20 14:11:33 PDT
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] 2012-06-22 07:35:28 PDT
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 :Ehsan Akhgari 2012-06-22 08:19:56 PDT
(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] 2012-06-22 08:52:22 PDT
(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 :Ehsan Akhgari 2012-06-22 12:20:08 PDT
Created attachment 635860 [details] [diff] [review]
Patch (v1)
Comment 9 Jeff Muizelaar [:jrmuizel] 2012-06-29 12:30:12 PDT
Created attachment 637970 [details] [diff] [review]
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] 2012-06-29 12:31:04 PDT
This patch successfully stops all screenshot notification on a page like reddit commments where the page is static.
Comment 11 Jeff Muizelaar [:jrmuizel] 2012-07-04 12:37:31 PDT
Created attachment 639159 [details] [diff] [review]
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-7 (busy September 14-25) 2012-07-06 16:21:12 PDT
Is it not sufficient to hook into nsCSSFrameConstructor::BeginUpdate like the two other existing generation counters do?
Comment 13 :Ehsan Akhgari 2012-07-06 16:25:01 PDT
(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] 2012-07-06 18:55:37 PDT
Created attachment 639895 [details] [diff] [review]
Use BeginUpdate instead of incrementing manually
Comment 15 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-07-06 19:23:31 PDT
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-7 (busy September 14-25) 2012-07-06 19:24:09 PDT
And ideally there'd be a comment pointing the other direction from nsPresContext::GetDOMGeneration's declaration in nsPresContext.h.
Comment 17 Jeff Muizelaar [:jrmuizel] 2012-07-06 19:58:44 PDT
(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] 2012-07-06 20:33:17 PDT
Created attachment 639917 [details] [diff] [review]
Add global frame tree generation
Comment 19 Jeff Muizelaar [:jrmuizel] 2012-07-06 20:34:26 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/002efc873e32
Comment 20 Ryan VanderMeulen [:RyanVM] 2012-07-07 12:00:09 PDT
https://hg.mozilla.org/mozilla-central/rev/002efc873e32
Comment 21 Jeff Muizelaar [:jrmuizel] 2012-07-09 06:10:05 PDT
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] 2012-07-09 14:12:05 PDT
(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] 2012-07-09 17:59:37 PDT
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] 2012-07-10 10:31:51 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/f94c4dfc1069

Note You need to log in before you can comment on or make changes to this bug.