Closed
Bug 450930
Opened 16 years ago
Closed 16 years ago
Only redraw when necessary
Categories
(Firefox for Android Graveyard :: General, defect, P1)
Tracking
(Not tracked)
RESOLVED
FIXED
fennec1.0a1
People
(Reporter: pavlov, Assigned: roc)
References
(Depends on 1 open bug)
Details
(Keywords: dev-doc-complete)
Attachments
(3 files, 8 obsolete files)
551 bytes,
text/html
|
Details | |
7.38 KB,
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
72.41 KB,
patch
|
roc
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
Right now we have some code that redraws the whole visible area every 15 seconds.
Flags: blocking-fennec1.0+
Reporter | ||
Comment 1•16 years ago
|
||
er, every 1 second
Reporter | ||
Comment 3•16 years ago
|
||
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.
Assignee | ||
Comment 4•16 years ago
|
||
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.
Reporter | ||
Comment 5•16 years ago
|
||
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?
Assignee | ||
Comment 6•16 years ago
|
||
(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.
Assignee | ||
Comment 7•16 years ago
|
||
(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.
Assignee | ||
Comment 8•16 years ago
|
||
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.
Assignee: pavlov → roc
Status: NEW → ASSIGNED
Comment 9•16 years ago
|
||
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)?
Reporter | ||
Comment 10•16 years ago
|
||
hey roc, is this patch missing some previous patch with things like NotifiyInvalidates? I fail to apply in nsPresContext.cpp/h, nsScrollFrame.cpp
Assignee | ||
Comment 11•16 years ago
|
||
That was just an hg misfire. Here's the real patch.
Attachment #337827 -
Attachment is obsolete: true
Assignee | ||
Comment 12•16 years ago
|
||
(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.
Assignee | ||
Comment 13•16 years ago
|
||
Argh, missed a file
Attachment #337960 -
Attachment is obsolete: true
Comment 14•16 years ago
|
||
(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?
Assignee | ||
Comment 15•16 years ago
|
||
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.
Attachment #337990 -
Flags: superreview?(mats.palmgren)
Attachment #337990 -
Flags: review?(Olli.Pettay)
Assignee | ||
Comment 16•16 years ago
|
||
(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.
Reporter | ||
Comment 17•16 years ago
|
||
Comment 18•16 years ago
|
||
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•16 years ago
|
||
{ &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•16 years ago
|
||
@@ -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);
Assignee | ||
Comment 21•16 years ago
|
||
(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.
Reporter | ||
Comment 22•16 years ago
|
||
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
Attachment #338032 -
Attachment is obsolete: true
Comment 23•16 years ago
|
||
(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•16 years ago
|
||
> 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•16 years ago
|
||
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?
Assignee | ||
Comment 26•16 years ago
|
||
(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•16 years ago
|
||
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?
Reporter | ||
Comment 28•16 years ago
|
||
roc's patch merged to tip
Attachment #337964 -
Attachment is obsolete: true
Attachment #337990 -
Attachment is obsolete: true
Attachment #337990 -
Flags: superreview?(mats.palmgren)
Attachment #337990 -
Flags: review?(Olli.Pettay)
Assignee | ||
Comment 29•16 years ago
|
||
(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.
Assignee | ||
Comment 30•16 years ago
|
||
(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.
Assignee | ||
Comment 31•16 years ago
|
||
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.
Assignee | ||
Comment 32•16 years ago
|
||
Updated to trunk again, updated to comments.
Attachment #338492 -
Attachment is obsolete: true
Attachment #338572 -
Flags: superreview?(mats.palmgren)
Attachment #338572 -
Flags: review?(Olli.Pettay)
Assignee | ||
Comment 33•16 years ago
|
||
Stuart, note that you'll have to change your patch to use 'MozAfterScroll' as the event name.
Comment 34•16 years ago
|
||
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.)
Attachment #338572 -
Flags: review?(Olli.Pettay) → review+
Reporter | ||
Comment 35•16 years ago
|
||
this still uses "MozDOMAfterPaint", but should be otherwise correct and ready to land once roc's patch lands
Attachment #338182 -
Attachment is obsolete: true
Attachment #338740 -
Flags: review?(gavin.sharp)
Comment 36•16 years ago
|
||
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
Attachment #338740 -
Flags: review?(gavin.sharp) → review+
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
Attachment #338572 -
Flags: superreview?(mats.palmgren) → superreview+
Assignee | ||
Comment 38•16 years ago
|
||
> 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•16 years ago
|
||
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.
Assignee | ||
Comment 40•16 years ago
|
||
(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.
Assignee | ||
Comment 41•16 years ago
|
||
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.
Attachment #338572 -
Attachment is obsolete: true
Attachment #338790 -
Flags: superreview+
Attachment #338790 -
Flags: review+
Assignee | ||
Comment 42•16 years ago
|
||
I popped my patch into the try server:
http://tinderbox.mozilla.org/showbuilds.cgi?tree=MozillaTry
"rocallahan@mozilla.com 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.
Assignee | ||
Comment 43•16 years ago
|
||
Looking at the main Tinderboxes, Linux Ts seems to jump around all over the place. So maybe we shouldn't be too concerned.
Assignee | ||
Comment 44•16 years ago
|
||
Pushed 9a46f2a17ddc. Watching Tinderbox.
Assignee | ||
Comment 45•16 years ago
|
||
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?
Reporter | ||
Comment 46•16 years ago
|
||
everything is checked in here. marking fixed.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Updated•16 years ago
|
Keywords: dev-doc-needed
Comment 47•16 years ago
|
||
I just tried the attached test on the latest nightly, and the event never fires. What am I missing?
Assignee | ||
Comment 48•16 years ago
|
||
What testcase? The mochittest is passing on Tinderbox.
Comment 49•16 years ago
|
||
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. :)
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•