450930 - Only redraw when necessary

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Stuart Parmenter]

Right now we have some code that redraws the whole visible area every 15 seconds.

Comment 1

[Stuart Parmenter]

er, every 1 second

Comment 2

[Stuart Parmenter]

er, every 1 second

Comment 3

[Stuart Parmenter]

We have some code in fennec that redraws the whole <canvas> from the hidden <browser> element every 100ms based on the current scroll position and zoom factor.  We need a mechanism to repaint the right parts of the canvas only as necessary.  We should hook this up by having the view manager(s) for the browser element's presshell(s) send notifications outward covering the entire browser element (things "offscreen" as well as on).  Then clip those based on the visible area based on the location and zoom factor.  Perhaps adding a browser onpaint() method would be nice.

Comment 4

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

I'm thinking of dispatching a MozDOMAfterPaint DOM event that is fired asynchronously at 'window'. It would have an attribute with a ClientRectList describing the area that has changed.

http://mxr.mozilla.org/mozilla-central/source/dom/public/idl/base/nsIDOMClientRectList.idl

Always firing this event would incur some overhead but I'm not sure how much. If the overhead is significant I'd prefer to optimize the event dispatching path (e.g. by remembering whether anyone has ever registered an event handler for this kind of event).

This is something that would be generally useful for the Web IMHO.

Is asynchronous dispatch of this event going to be adequate for Fennec's needs? It means there could be some delay between the paint and the event being processed. But firing it synchronously could cause all kinds of problems.

Comment 5

[Stuart Parmenter]

roc: I think async should be OK, as long as they aren't too far apart.  queuing them up should help, and the interesting test cases will be heavy dhtml pages to see if they repaint fast enough, but I think they should be fine.

would it be possible to query the current damage region to repaint as-needed if we had some spots that needed to update very frequently?

Comment 6

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

(In reply to comment #5)
> would it be possible to query the current damage region to repaint as-needed if
> we had some spots that needed to update very frequently?

We could add a separate API for that, but it would be hard to use correctly. I think this DOM event would fire at least as fast as a timeout, so I'm not sure how you'd use the damage region API.

My current thinking is that this DOM event would *not* fire on repainting performed by subframes. For the Web, that could be a cross-origin information leak, and for Fennec you can use a chrome event handler to catch all repainting performed by subframes.

Another issue is exactly what painting will be reported. For Fennec, we definitely want to know about painting that happened in a window that's behind other windows. But do we want to know about invalidation of content that's clipped out or scrolled out of view? Probably not. I'll have to think about how to define this.

Comment 7

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

(In reply to comment #6)
> My current thinking is that this DOM event would *not* fire on repainting
> performed by subframes. For the Web, that could be a cross-origin information
> leak, and for Fennec you can use a chrome event handler to catch all repainting
> performed by subframes.

This actually isn't going to work. Subframes that are transformed by CSS transforms or SVG foreignobject make it impossibly difficult for a chrome event handler to figure out what a repaint in a subframe actually means for the toplevel document. So I'll have to bubble those up to the top level.

Now, the tricky part is, I don't want to fire a DOM event at a toplevel content window when one of its subframes invalidates, because that's an information leak.

So my current plan is to track "invalidation in subframes" separately from "invalidation in this document". There will be two "invalid regions" tracked in each presContext. When a region goes from empty to nonempty we'll spawn an XPCOM event to asynchronously dispatch a DOM event. When we dispatch the DOM event, if the "invalidation in this document" region is empty and the document is not trusted, we will target the event at the chrome event handler instead of the window. When we read the event's dirty rect list, if the caller is trusted then we return the union of the two regions, otherwise we just return the "invalidation in this document" region.

> Another issue is exactly what painting will be reported. For Fennec, we
> definitely want to know about painting that happened in a window that's behind
> other windows. But do we want to know about invalidation of content that's
> clipped out or scrolled out of view? Probably not. I'll have to think about
> how to define this.

What I have right now is that clipping and scrolled-out-of-view limit what's reported, but z-ordering both inside and outside the content window does not.

Comment 8

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

Attachment: fix, WIP

OK, here's where I'm at. This almost works. The mochitest currently has one failure; when we're privileged we should be able to see invalidation happening in an iframe, but we can't. Need to debug that.

Also, this needs performance testing. If it causes significant overhead during rapid painting, then we need to find a way to fix that.

But the API is basically here. You should be able to build on this.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 337827 [details] [diff] [review]
fix, WIP

> void
>+nsPresContext::FireDOMPaintEvent()
>+{
>+  nsCOMPtr<nsIDocShell> docShell(do_QueryReferent(mContainer));
>+  if (!docShell)
>+    return;
>+  nsCOMPtr<nsPIDOMWindow> ourWindow = do_GetInterface(docShell);
>+  nsISupports* eventTarget = ourWindow;
>+  if (mSameDocDirtyRegion.IsEmpty() && !IsChrome()) {
>+    // Don't tell the window about this event, it should not know that
>+    // something happened in a subdocument
>+    eventTarget = ourWindow->GetChromeEventHandler();
>+  }
>+
>+  nsNotifyPaintEvent event(PR_TRUE, NS_AFTERPAINT, mSameDocDirtyRegion,
>+                           mCrossDocDirtyRegion);
>+  // Empty our regions now in case dispatching the event causes more damage
>+  // (hopefully it won't, or we're likely to get an infinite loop! At least
>+  // it won't be blocking app execution though).
>+  mSameDocDirtyRegion.SetEmpty();
>+  mCrossDocDirtyRegion.SetEmpty();
>+  event.target = do_QueryInterface(ourWindow);
>+  nsEventDispatcher::Dispatch(ourWindow, this, &event);
>+}
I don't understand this. What is eventTarget?
Should you dispatch the event to eventTarget, not to ourWindow?

Is it always the top-level content window which dispatches these events? Or could/do iframes dispatch their own
events (in which case event propagates from iframe to chromehandler, not to top window)?

Comment 10

[Stuart Parmenter]

hey roc, is this patch missing some previous patch with things like NotifiyInvalidates?   I fail to apply in nsPresContext.cpp/h, nsScrollFrame.cpp

Comment 11

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

Attachment: fix WIP v2

That was just an hg misfire. Here's the real patch.

Comment 12

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

(In reply to comment #9)
> I don't understand this. What is eventTarget?
> Should you dispatch the event to eventTarget, not to ourWindow?

Yes, that's a mistake, I want to dispatch to eventTarget.

> Is it always the top-level content window which dispatches these events? Or
> could/do iframes dispatch their own
> events (in which case event propagates from iframe to chromehandler, not to top
> window)?

iframes can dispatch events, and what you describe is the desired behaviour.

Comment 13

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

Attachment: fix WIP v3

Argh, missed a file

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #12)
> iframes can dispatch events, and what you describe is the desired behaviour.
Ok, and that is what happens anyway (using normal event dispatch). Is the special dispatch-to-chrome-handler needed? Or are there cases when top-level content window is notified that iframe is painted, but iframe doesn't get that info.
Or perhaps the dirty region handling needs that parent dispatches such event?

Comment 15

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

Attachment: fix v4

Fixed a leak. Added some more tests, which all pass. Basically good to go.

Only thing not addressed yet is performance testing. There are various ways to disable this when it's not needed if perf is an issue.

Comment 16

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

(In reply to comment #14)

The important thing here is that we sometimes want to only notify the chrome event handler, not the window itself. In particular, invalidation occurring in a subdocument should not be reported to the content window, only to the chrome event handler.

Comment 17

[Stuart Parmenter]

Attachment: v0.5 for fennec to use roc's patch

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Actually, would it make sense to just dispatch a simple "paint" or "MozPaint" or 
"MozAfterPaint" event and at that point the information about what was painted could be available in WindowUtils of the event target's window?
That way there wasn't reason to create yet another event type, and
nsDOMNotifyPaintEvent::GetClientRects and nsDOMNotifyPaintEvent::GetBoundingClientRect work anyway like static methods, depending just on prescontext.
Or do we really want to expose the event *and* the paint information to web content?

Comment 19

[Oleg Romashin (:romaxa)]

 { &nsGkAtoms::onvolumechange, { NS_VOLUMECHANGE, ventNameType_HTML }},
 #endif //MOZ_MEDIA
+{ &nsGkAtoms::onMozDOMAfterPaint,{ NS_AFTERPAINT, EventNameType_HTML }}
This will fail if MOZ_MEDIA not defined.


-{ &nsGkAtoms::onvolumechange, { NS_VOLUMECHANGE, ventNameType_HTML }},
+{ &nsGkAtoms::onvolumechange, { NS_VOLUMECHANGE, ventNameType_HTML }}
 #endif //MOZ_MEDIA
+ ,{ &nsGkAtoms::onMozDOMAfterPaint,{ NS_AFTERPAINT, EventNameType_HTML }}

Comment 20

[Oleg Romashin (:romaxa)]

@@ -3610,17 +3610,17 @@ nsIFrame::InvalidateWithFlags(const nsRe
  if (nsSVGIntegrationUtils::UsingEffectsForFrame(this)) { 
     nsRect r = nsSVGIntegrationUtils::GetInvalidAreaForChangedSource(this, 
             aDamageRect + nsPoint(aX, aY));
-    GetParent()->InvalidateInternal(r, mRect.x, mRect.y, this, aImmediate); 
+    GetParent()->InvalidateInternal(r, mRect.x, mRect.y, this, aFlags);

Comment 21

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

(In reply to comment #18)
> Actually, would it make sense to just dispatch a simple "paint" or "MozPaint"
> or 
> "MozAfterPaint" event and at that point the information about what was painted
> could be available in WindowUtils of the event target's window?

Then when do you clear those stored dirty areas? You really want to clear them before firing the event, in case the event does more invalidation (you don't want the new invalidation to get mixed up with the area already reported, especially if there are multiple listeners). So you'd need two sets of dirty areas, the "area reporting now" and "next dirty area". Right now we have that, the "area reporting now" is just stored in the event.

Having the data which is really part of the event be stored globally is ugly IMHO.

> That way there wasn't reason to create yet another event type, and
> nsDOMNotifyPaintEvent::GetClientRects and
> nsDOMNotifyPaintEvent::GetBoundingClientRect work anyway like static methods,
> depending just on prescontext.

It really shouldn't be this hard to create a new event type. I would hate to work around that problem by exposing an ugly API to content/chrome.

> Or do we really want to expose the event *and* the paint information to web
> content?

I do. People have asked for it. It's useful for debugging Web apps and logging timing data.

Comment 22

[Stuart Parmenter]

Attachment: v0.9 for fennec to use roc's patch

ok, this hooks things up to redraw with fennec.  only 4 real issues left: need to fix up rounding a bit more, and sometimes we're unable to get the canvas's width and height which causes us to draw funny sometimes.  we need to clear the canvas when a new page starts loading and we need to cancel the event listener when we switch to another tab

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #21)
> I do. People have asked for it. It's useful for debugging Web apps and logging
> timing data.
Ok, that was the most important part. I'll review the patch asap.

Comment 24

[Oleg Romashin (:romaxa)]

> Created an attachment (id=338182) [details]

Looks very good, but some strange things happen after loading "linux.org.ru"... all interactions and scrolling works very randomly...
May be it is because there are dynamic text frame on the left side?

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 337990 [details] [diff] [review]
fix v4

>+[scriptable, uuid(dec5582e-5cea-412f-bf98-6b27480fb46a)]
>+interface nsIDOMNotifyPaintEvent : nsIDOMEvent
>+{
>+  /**
>+   * Stores a list of rectangles which are affected.
>+   */
>+  readonly attribute nsIDOMClientRectList clientRects;
>+  /**
>+   * Stores the bounding box of the rectangles which are affected.
>+   */
>+  readonly attribute nsIDOMClientRect boundingClientRect;

If you could make this to have initNotifyPaintEvent method with clientRects and boundingClientRect as parameters
(and maybe paintTarget too, so that special targeting wasn't needed when dispatching), then you don't
need to add new event type to nsGUIEvent.h, nor to nsContentUtils and there is no need to change 
nsDOMEvent.h|cpp either. And .clientRects and .boundingClientRect would always return the same value, which
wouldn't depend on the current state of prescontext. 
Then when creating the event, use normal document.createEvent();event.initXXX();target.dispatchEvent(event)

I'd like to avoid to add new things to nsGUIEvent, if possible.

How often do we dispatch these events?

Comment 26

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

(In reply to comment #25)
> If you could make this to have initNotifyPaintEvent method with clientRects and
> boundingClientRect as parameters
> (and maybe paintTarget too, so that special targeting wasn't needed when
> dispatching), then you don't
> need to add new event type to nsGUIEvent.h, nor to nsContentUtils and there is
> no need to change 
> nsDOMEvent.h|cpp either. And .clientRects and .boundingClientRect would always
> return the same value, which
> wouldn't depend on the current state of prescontext. 

clientRects and boundingClientRect have to return different values depending on whether the caller is trusted or not. How would that fit in here?

> How often do we dispatch these events?

Every time we paint. So I'd like to make it as cheap as possible, which probably means not constructing a heap-allocated list of heap-allocated ClientRect objects unless we really have to...

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 337990 [details] [diff] [review]
fix v4

>diff --git a/content/base/src/nsContentUtils.cpp b/content/base/src/nsContentUtils.cpp
>--- a/content/base/src/nsContentUtils.cpp
>+++ b/content/base/src/nsContentUtils.cpp
>@@ -476,6 +476,7 @@ nsContentUtils::InitializeEventTable() {
>     { &nsGkAtoms::ondurationchange,              { NS_DURATIONCHANGE, EventNameType_HTML }},
>     { &nsGkAtoms::onvolumechange,                { NS_VOLUMECHANGE, EventNameType_HTML }},
> #endif //MOZ_MEDIA
>+    { &nsGkAtoms::onMozDOMAfterPaint,            { NS_AFTERPAINT, EventNameType_HTML }}
Maybe EventNameType_None.
And I'd call the event "MozAfterPaint".

>+NS_IMETHODIMP
>+nsDOMNotifyPaintEvent::GetClientRects(nsIDOMClientRectList** aResult)
...
>+  *aResult = rectList.forget().get();
Why not .swap() ?

>+void
>+nsPresContext::NotifyInvalidation(const nsRect& aRect, PRBool aIsCrossDoc)
>+{
>+  if (aRect.IsEmpty())
>+    return;
>+
>+  if (mSameDocDirtyRegion.IsEmpty() && mCrossDocDirtyRegion.IsEmpty()) {
>+    // No event is pending. Dispatch one now.
>+    nsCOMPtr<nsIRunnable> ev =
>+      new nsRunnableMethod<nsPresContext>(this,
>+                                          &nsPresContext::FireDOMPaintEvent);
>+    NS_DispatchToCurrentThread(ev);

I think we don't want to dispatch events if there are no listeners for it anywhere
current or ancestor windows. Adding a flag to nsPIDOMWindow and marking it in
ELM::AddEventListener should work ok. Then iterating through all the windows.
Have to remember to copy the flag when adopting nodes.
That can be a followup bug, if this causes performance problems. But would be great
to test even before landing this.

Also I think we don't want to fire events too often - not sure what is the best way to limit the frequency.
attachment 338182 [details] [diff] [review] does quite a bit work for each event.

> void
> nsListControlFrame::InvalidateInternal(const nsRect& aDamageRect,
>                                        nscoord aX, nscoord aY, nsIFrame* aForChild,
>-                                       PRBool aImmediate)
>-{
>-  if (!IsInDropDownMode())
>-    nsHTMLScrollFrame::InvalidateInternal(aDamageRect, aX, aY, this, aImmediate);
>-  InvalidateRoot(aDamageRect, aX, aY, aImmediate);
>+                                       PRUint32 aFlags)
>+{
>+  if (!IsInDropDownMode()) {
>+    nsHTMLScrollFrame::InvalidateInternal(aDamageRect, aX, aY, this, aFlags);
>+    return;
>+  }
>+  InvalidateRoot(aDamageRect + nsPoint(aX, aY), aFlags);
> }
The |return| changes behavior. I guess that is what you want?

Comment 28

[Stuart Parmenter]

Attachment: fix v4.1

roc's patch merged to tip

Comment 29

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

(In reply to comment #27)
> Maybe EventNameType_None.
> And I'd call the event "MozAfterPaint".

OK

> >+NS_IMETHODIMP
> >+nsDOMNotifyPaintEvent::GetClientRects(nsIDOMClientRectList** aResult)
> ...
> >+  *aResult = rectList.forget().get();
> Why not .swap() ?

OK

> I think we don't want to dispatch events if there are no listeners for it
> anywhere
> current or ancestor windows. Adding a flag to nsPIDOMWindow and marking it in
> ELM::AddEventListener should work ok. Then iterating through all the windows.
> Have to remember to copy the flag when adopting nodes.
> That can be a followup bug, if this causes performance problems. But would be
> great
> to test even before landing this.

Yes. But how would this flag work if the listener is a chrome event handler that's listening for events on all subframes?

> Also I think we don't want to fire events too often - not sure what is the best
> way to limit the frequency.
> attachment 338182 [details] [diff] [review] does quite a bit work for each event.

That is true. However, you could use JS timeouts to coalesce too-frequent events (pushing the bounding-rect and/or client-rects onto an array), and implementing coalescing there gives authors full control over the policy, which is nice.

> > void
> > nsListControlFrame::InvalidateInternal(const nsRect& aDamageRect,
> >                                        nscoord aX, nscoord aY, nsIFrame* aForChild,
> >-                                       PRBool aImmediate)
> >-{
> >-  if (!IsInDropDownMode())
> >-    nsHTMLScrollFrame::InvalidateInternal(aDamageRect, aX, aY, this, aImmediate);
> >-  InvalidateRoot(aDamageRect, aX, aY, aImmediate);
> >+                                       PRUint32 aFlags)
> >+{
> >+  if (!IsInDropDownMode()) {
> >+    nsHTMLScrollFrame::InvalidateInternal(aDamageRect, aX, aY, this, aFlags);
> >+    return;
> >+  }
> >+  InvalidateRoot(aDamageRect + nsPoint(aX, aY), aFlags);
> > }
> The |return| changes behavior. I guess that is what you want?

Yes. The previous lack of return was a (mostly harmless) bug.

Comment 30

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

(In reply to comment #29)
> > >+NS_IMETHODIMP
> > >+nsDOMNotifyPaintEvent::GetClientRects(nsIDOMClientRectList** aResult)
> > ...
> > >+  *aResult = rectList.forget().get();
> > Why not .swap() ?
> 
> OK

Actually that doesn't work because rectList is an nsRefPtr<nsClientRectList>, not an nsIDOMClientRectList.

Comment 31

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

Attachment: performance testcase

This testcase is my attempt to get a worst-case scenario for the overhead of this feature. We just repaint a small area about as fast as we possibly can (up to the limits of JS timers).

I ran this in my Linux-opt build and sysprof says we're spending 0.6% of the time under nsPresContext::FireDOMPaintEvent. I'm not sure whether that's enough to worry about, but I lean towards not worrying about it, assuming this really is the worst case.

Comment 32

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

Attachment: fix v5

Updated to trunk again, updated to comments.

Comment 33

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

Stuart, note that you'll have to change your patch to use 'MozAfterScroll' as the event name.

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 338572 [details] [diff] [review]
fix v5

>+    case NS_NOTIFYPAINT_EVENT:
>+    {
>+      newEvent =
>+        new nsNotifyPaintEvent(PR_FALSE, msg,
>+                               ((nsNotifyPaintEvent*) mEvent)->sameDocRegion,
>+                               ((nsNotifyPaintEvent*) mEvent)->crossDocRegion);
static_cast<>

>diff --git a/layout/base/tests/Makefile.in.orig b/layout/base/tests/Makefile.in.orig
This file shouldn't be here, I think.

(About the chromehandler thing, if the optimization is needed;
if chromehandler isn't a DOM node or window, QIing to nsPIDOMEventTarget
and getting ELM from it and asking HasListenersFor(NS_LITERAL_STRING("MozAfterPaing"))
should work.)

Comment 35

[Stuart Parmenter]

Attachment: v1.0 for fennec to use roc's patch

this still uses "MozDOMAfterPaint", but should be otherwise correct and ready to land once roc's patch lands

Comment 36

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 338740 [details] [diff] [review]
v1.0 for fennec to use roc's patch

>diff --git a/chrome/content/deckbrowser.xml b/chrome/content/deckbrowser.xml

>       <method name="updateCanvasState">

>-          if (aNewDoc)
>+          if (aNewDoc) {

Just get rid of this parameter, and remove all the callers that don't pass true, since the aNewDoc==false case is just a no-op now.

>+            // FIXME: canvas needs to know it's actual width/height
>+            var rect = this._canvas.getBoundingClientRect();
>+            var w = rect.right - rect.left;
>+            var h = rect.bottom - rect.top;

This shouldn't be needed... if the canvas isn't already sized correctly, there's nothing to clearRect anyways (browserToCanvas takes care of doing it initially).
 
>+      <field name="browserRedrawHandler">

>+          handleEvent: function (aEvent) {
>+            let self = this.deckbrowser;

nit: get rid of this and just use this.deckbrowser directly?

>       <method name="_browserToCanvas">

>-          ctx.clearRect(0,0,w,h);
>+          //ctx.clearRect(0,0,w,h);

Just remove this?

>+      <method name="_browserToCanvas2">

This should have a better name... _redrawRect?

>-          if (isNaN(w) || isNaN(h))
>-            throw "Can't get content width/height";
>-
>+          if (isNaN(w) || isNaN(h)) {
>+            return [this._canvas.width, this._canvas.height];
>+          }

Ew, unnecessary brackets.

You can get rid of this code too:
http://hg.mozilla.org/mobile-browser/annotate/5fbad643ccf0/chrome/content/browser.js#l359

Comment 37

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

Comment on attachment 338572 [details] [diff] [review]
fix v5

Stuart asked me over IRC to take this superreview request, so...

In nsContentUtils.cpp, nsDOMEvent.cpp, and nsDOMEvent.h, please switch 
to not having any leading commas at all (by changing the comma situation
at the other ifdef boundaries) or add before the MOZ_SVG ifdef.

In nsDOMNotifyPaintEvent.h, please only use one _ at the end of the
include guard; names with two consecutive underscores are reserved for
the implementation.

Should nsIDOMNotifyPaintEvent.idl say something about what coordinate
system is used?  Should it also say something about the cross-origin
restrictions?

The second change in nsDOMClassInfo.cpp should match the ordering
of the entry in the first change and in the header; otherwise I think
you risk crashes.  (And the other two are adding to the end, which is
good.)

Please undo the license header munging in nsPresContext.cpp.

nsPresContext::FireDOMPaintEvent is a bit confusing; it might be worth
commenting that;
 * events sent to the window get propagated to the chrome event handler
 * event.target is intentionally always the window, and thus sometimes
   differs from eventTarget.

Please don't hg add and commit layout/base/tests/Makefile.in.orig

In the test, could you make the element with id="display" a div rather
than a p, so that the actual document tree matches the indentation?
(The start tag for the div following implicitly closes the p.)

In the test, would you mind putting runTest1, runTest2, and runTest3 in
order, rather than backwards?  It's JavaScript, so that should be ok.
(And maybe triggerPaint before check, twice.)

In nsIFrame.h, could you document INVALIDATE_CROSS_DOC with its correct
name (not INVALIDATE_CROSS_FRAME), and document INVALIDATE_NOTIFY_ONLY ?

In nsIFrame.h, could you rename the parameter to
InvalidateInternalAfterResize to be aFlags rather than aImmediate?

Do you need to set the cross-document invalidation flag in
nsSVGOuterSVGFrame::Paint (#ifdef XP_MACOSX code)?  If not, why not?

In nsGUIEvent.h, please fix the comment that says "pagetransition
events" to say something correct.

sr=dbaron with that

Comment 38

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

> Do you need to set the cross-document invalidation flag in
> nsSVGOuterSVGFrame::Paint (#ifdef XP_MACOSX code)?  If not, why not?

I suppose I should. Good catch.

I'll address all your comments, shouldn't be hard.

Comment 39

[Mats Palmgren (inactive)]

In nsIFrame.h, remove the old UUID comment.

In nsSVGForeignObjectFrame.cpp,
+  nsRegion* region = (aFlags & INVALIDATE_CROSS_DOC)
+    ? &mSameDocDirtyRegion : &mCrossDocDirtyRegion;

Looks odd - should it be the opposite?

In nsDOMNotifyPaintEvent.cpp,
+  if ( aEvent ) {

Remove the spaces around aEvent please.

+  if (mEventIsInternal) {
+    if (mEvent->eventStructType == NS_NOTIFYPAINT_EVENT) {

For internal events you can just assert it's a NS_NOTIFYPAINT_EVENT?

+NS_IMETHODIMP
+nsDOMNotifyPaintEvent::GetClientRects(nsIDOMClientRectList** aResult)
+{
+  *aResult = nsnull;

This assignment isn't necessary, the caller shouldn't use the result
in case of an error.

In nsDOMNotifyPaintEvent.h,
+  ~nsDOMNotifyPaintEvent();

Add an explicit "virtual" for clarity and consistency with other
nsDOM*Event classes.

Comment 40

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

(In reply to comment #39)
> In nsIFrame.h, remove the old UUID comment.
> 
> In nsSVGForeignObjectFrame.cpp,
> +  nsRegion* region = (aFlags & INVALIDATE_CROSS_DOC)
> +    ? &mSameDocDirtyRegion : &mCrossDocDirtyRegion;
> 
> Looks odd - should it be the opposite?

Actually yes, it should. I guess the test needs to be better...

> In nsDOMNotifyPaintEvent.cpp,
> +  if ( aEvent ) {
> 
> Remove the spaces around aEvent please.

OK

> +  if (mEventIsInternal) {
> +    if (mEvent->eventStructType == NS_NOTIFYPAINT_EVENT) {
> 
> For internal events you can just assert it's a NS_NOTIFYPAINT_EVENT?

Maybe, but other events don't do that.

> +NS_IMETHODIMP
> +nsDOMNotifyPaintEvent::GetClientRects(nsIDOMClientRectList** aResult)
> +{
> +  *aResult = nsnull;
> 
> This assignment isn't necessary, the caller shouldn't use the result
> in case of an error.

OK

> In nsDOMNotifyPaintEvent.h,
> +  ~nsDOMNotifyPaintEvent();
> 
> Add an explicit "virtual" for clarity and consistency with other
> nsDOM*Event classes.

Sure.

Comment 41

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

Attachment: fix v6

Updated to comments. I also added a test involving foreignObject. This is ready for checkin. Stuart, feel free to land if I don't get to it first.

Comment 42

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

I popped my patch into the try server:
http://tinderbox.mozilla.org/showbuilds.cgi?tree=MozillaTry
"[e-mail] 1221546086"

Unfortunately Windows was busted (not by me, I think). Mac numbers looked fine. Linux may be showing a Ts regression. Hard to say, I don't know what the noise range is. It would be surprising if this hurt Ts but not Tp, however --- Tp paints more. Maybe the throbber invalidation showing up?

I'm trying another build to see if the regression is repeatable.

Comment 43

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

Looking at the main Tinderboxes, Linux Ts seems to jump around all over the place. So maybe we shouldn't be too concerned.

Comment 44

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

Pushed 9a46f2a17ddc. Watching Tinderbox.

Comment 45

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

Looking at a bunch of graphs, things seem to be basically fine except there's pretty clearly a Tdhtml regression on Mac:
http://graphs.mozilla.org/#show=394910,394919,394927,911649,1431219&sel=1221433671,1221774701
The Mac builder picked up my changeset for the build starting at 3:27 on the 18th.

So, back out or add the optimization on top?

Comment 46

[Stuart Parmenter]

everything is checked in here.  marking fixed.

Comment 47

[Eric Shepherd [:sheppy]]

I just tried the attached test on the latest nightly, and the event never fires.  What am I missing?

Comment 48

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

What testcase? The mochittest is passing on Tinderbox.

Comment 49

[Eric Shepherd [:sheppy]]

I'm apparently hallucinating, as there isn't an attached test case that resembles the one I tried running earlier.  I'm very confused.  Sorry. :)