847890 - paint flashing for content only (maybe for only one tab)

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Paul Rouget [:paul]]

It would be great to be able to turn on paint flashing from the devtools (see bug 723281). But paint flashing is for the whole browser, and not specific to one tab.

Could we enable paint flashing for content only?
And only for one tab? (this is optional, from the devtools, we could disable paint flashing when a new tab is selected).

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes.

Comment 2

[Paul Rouget [:paul]]

This is something that has been requested several times for the devtools.

How much work is needed?

If nobody has time to work on that, I can take care of it if mentored.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Paul Rouget [:paul] from comment #2)
> If nobody has time to work on that, I can take care of it if mentored.

Please; it'll be fun :-).

I think the way to do this is to pass containerLayerFrame from FrameLayerBuilder::DrawThebesLayer into FrameLayerBuilder::FlashPaint. Then, if you want to flash content only, test containerLayerFrame->PresContext()->IsChrome().

Controlling it by flipping prefs doesn't seem like a great API though, but nothing obviously better springs to mind at the moment.

Comment 4

[Boris Zbarsky [:bzbarsky]]

If the prescontext is available there's no need for prefs.  Just a flag on the document that can be set via windowutils or something, and check the flag on the prescontext's document.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

But you'd really want the flag to control all descendant documents. I'm not sure we want to walk the document ancestors from every FlashPaint.

Maybe if we store the flag on nsRefreshDriver? That sounds like the right idea. We can get the toplevel content document's refresh driver efficiently.

Comment 6

[Paul Rouget [:paul]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #3)
> (In reply to Paul Rouget [:paul] from comment #2)
> > If nobody has time to work on that, I can take care of it if mentored.
> 
> Please; it'll be fun :-).

Let's do that then :)

> I think the way to do this is to pass containerLayerFrame from
> FrameLayerBuilder::DrawThebesLayer into FrameLayerBuilder::FlashPaint. Then,
> if you want to flash content only, test
> containerLayerFrame->PresContext()->IsChrome().

It works.

> Maybe if we store the flag on nsRefreshDriver? That sounds like the right
> idea. We can get the toplevel content document's refresh driver efficiently.

Can you give me some pointers?

Comment 7

[Boris Zbarsky [:bzbarsky]]

So basically we'd use windowutils, but instead of setting a flag on the document, you'd do it on doc->GetShell()->GetPresContext()->RefreshDriver() (with a null-check on the GetShell()).

Then in FlashPaint you'd check the flag on containerLayerFrame->PresContext()->RefreshDriver().

Comment 8

[Paul Rouget [:paul]]

Attachment: patch v0.1

This appears to work well.

To test this code, starts a chrome scratchpad (https://developer.mozilla.org/en-US/docs/Tools/Scratchpad#Using_Scratchpad_to_access_Firefox_internals) and type this code:


// Flash content
var w = gBrowser.selectedTab.linkedBrowser.contentWindow;
var u = w.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils);
u.enablePaintFlashing();

// Flash chrome
var u = window.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils);
u.enablePaintFlashing();

Next steps:
- address feedback
- add comments (the FIXMEs)
- kill the nglayout.debug.paint_flashing pref (does it make sense?)
- test (how do I test that?)
- expose this feature as a command in the devtools command line (for chrome and content)

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 722727 [details] [diff] [review]
patch v0.1

Review of attachment 722727 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/interfaces/base/nsIDOMWindowUtils.idl
@@ +1341,5 @@
> +    * FIXME
> +    */
> +   void enablePaintFlashing();
> +   void disablePaintFlashing();
> +   boolean isPaintFlashingEnabled();

This should be
  attribute boolean paintFlashing;
which you'll implement with SetPaintFlashing and GetPaintFlashing methods.

::: layout/base/nsRefreshDriver.h
@@ +75,5 @@
>  
>    /**
> +   * FIXME
> +   */
> +  bool paintFlashing;

This should be protected/private next to the other fields, and you should expose a SetPaintFlashing setter method and a GetPaintFlashing getter method, which can be inline. Also, call it mPaintFlashing to follow naming conventions.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Paul Rouget [:paul] from comment #8)
> - kill the nglayout.debug.paint_flashing pref (does it make sense?)

I think it would make sense to keep the pref and use it as the initial value for mPaintFlashing.

> - test (how do I test that?)

Mochitest-chrome using WindowSnapshot.js and paint_listener.js. E.g. you could take a snapshot of a content IFRAME, change something in the frame, wait for paint, change it back, wait for paint again, take another snapshot, and verify that the snapshots are different. (Maybe repeat until they are different, to avoid low probability that we're unlucky with the random color choices!)

Comment 11

[Paul Rouget [:paul]]

Attachment: patch v0.2

Addressed previous comments.

Is accessing the pref in nsRefreshDriver::nsRefreshDriver() ok in term of performance?

I didn't manage to make the test working. I'm not sure why: the 2 screenshots are identical, even though one has been taken after paintflashing.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Paul Rouget [:paul] from comment #11)
> I didn't manage to make the test working. I'm not sure why: the 2
> screenshots are identical, even though one has been taken after
> paintflashing.

The test should be using SpecialPowers; you shouldn't be using enablePrivilege.

It might not be working due to our ColorLayer optimization; paint flashing only happens for ThebesLayers. Try putting some text or other content in the iframe.

Yeah, so a heads-up on that: paint flashing won't happen when we do certain optimizations. E.g. a single CSS-transformed <img> being animated shouldn't cause paint flashing.

Comment 13

[Paul Rouget [:paul]]

Attachment: patch v1

Comment 14

[Matt Woodrow (:mattwoodrow)]

This patch makes the pref only read during creation of the refresh driver. I frequently toggle paint flashing temporarily, is it possible to keep this working?

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think it'll work if you reload the document after toggling the pref. Is that OK?

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 723732 [details] [diff] [review]
patch v1

Review of attachment 723732 [details] [diff] [review]:
-----------------------------------------------------------------

r+ assuming the test works.

Comment 17

[Matt Woodrow (:mattwoodrow)]

Ah, good point, too many refresh drivers (I was thinking of the per-window one that drives painting). That will be fine, yeah.

Comment 18

[Paul Rouget [:paul]]

When paint flashing is turned off (.paintflashing = false), I'd like to redraw the whole frame to get rid of the added colors. How can I do that?

Comment 19

[Paul Rouget [:paul]]

Attachment: patch v1.1

Thanks a lot for your help and the r+.

A minor addition: clearing the colors once paint flashing is disabled.
I use:
> presContext->PresShell()->GetRootFrame()->InvalidateFrameSubtree();

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 723826 [details] [diff] [review]
patch v1.1

Review of attachment 723826 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsDOMWindowUtils.cpp
@@ +3216,5 @@
> +  nsPresContext* presContext = GetPresContext();
> +  if (presContext) {
> +    presContext->RefreshDriver()->SetPaintFlashing(aPaintFlashing);
> +    if (!aPaintFlashing) {
> +      presContext->PresShell()->GetRootFrame()->InvalidateFrameSubtree();

It is possible for GetRootFrame to return null. Please add a null check here and just do nothing if it's null.

Comment 21

[Paul Rouget [:paul]]

Attachment: patch to land

Comment 22

[Paul Rouget [:paul]]

Vivien, I'm not sure, but that might have an impact on the debug mode in B2G.

Comment 23

[Paul Rouget [:paul]]

https://hg.mozilla.org/integration/fx-team/rev/eaf182f23c3a

Comment 24

[Paul Rouget [:paul]]

https://hg.mozilla.org/mozilla-central/rev/eaf182f23c3a

Comment 25

[Ed Morley [:emorley]]

Backed out for test_bug847890_paintFlashing.html permaorange on opt Windows7/XP:
https://tbpl.mozilla.org/php/getParsedLog.php?id=20590180&tree=Firefox
https://tbpl.mozilla.org/php/getParsedLog.php?id=20596231&tree=Firefox
https://tbpl.mozilla.org/php/getParsedLog.php?id=20590287&tree=Firefox
https://tbpl.mozilla.org/php/getParsedLog.php?id=20596253&tree=Firefox

Weird thing is that the same csets merged back to fx-team, where it is green.

80% failure rather than 100% and just been unlucky with retriggers? Actually a bad binary and needed a clobber? Who knows! Either way, this is holding m-c closed, so backed out for now.

(Note if this turns out to have needed a clobber, please can we file a bug to help us fix the buildsystem & the re-landing here will need to touch the clobber file).

Comment 26

[Ed Morley [:emorley]]

Backout:
https://hg.mozilla.org/mozilla-central/rev/53f7536d1ec5

Comment 27

[Paul Rouget [:paul]]

https://hg.mozilla.org/mozilla-central/rev/3af46f38ba7f