Closed Bug 847890 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Graphics, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla22
Tracking Status
firefox22 --- fixed

People

(Reporter: paul, Assigned: paul)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 3 obsolete files)

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).
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.
(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.
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.
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.
(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?
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().
Attached patch patch v0.1 (obsolete) — Splinter Review
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)
Assignee: nobody → paul
Status: NEW → ASSIGNED
Attachment #722727 - Flags: feedback?(roc)
Attachment #722727 - Flags: feedback?(bzbarsky)
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.
(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!)
Attached patch patch v0.2 (obsolete) — Splinter Review
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.
Attachment #722727 - Attachment is obsolete: true
Attachment #722727 - Flags: feedback?(roc)
Attachment #722727 - Flags: feedback?(bzbarsky)
Attachment #723644 - Flags: feedback?(roc)
(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.
Attached patch patch v1 (obsolete) — Splinter Review
Attachment #723644 - Attachment is obsolete: true
Attachment #723732 - Flags: review?(roc)
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?
I think it'll work if you reload the document after toggling the pref. Is that OK?
Comment on attachment 723732 [details] [diff] [review]
patch v1

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

r+ assuming the test works.
Attachment #723732 - Flags: review?(roc) → review+
Ah, good point, too many refresh drivers (I was thinking of the per-window one that drives painting). That will be fine, yeah.
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?
Attached patch patch v1.1Splinter Review
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();
Attachment #723826 - Flags: review?(roc)
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.
Attachment #723826 - Flags: review?(roc) → review+
Attached patch patch to landSplinter Review
Attachment #723732 - Attachment is obsolete: true
Blocks: 723281
Vivien, I'm not sure, but that might have an impact on the debug mode in B2G.
Keywords: checkin-needed
Keywords: checkin-needed
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/eaf182f23c3a
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/eaf182f23c3a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
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).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/3af46f38ba7f
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.