I want to simplify our view update / invalidation handling. That entails getting rid of view update batches, handling view resizes and invalidation flushes from the refresh driver, and getting rid of all synchronous refreshes. I'll write up a better description tomorrow and just put up a wip patch for now.

We probably do still need to keep the Flush_Display stuff, no? And probably make windowutils use it?

Also, would we ever have more than one presshell being an invalidation flush observer in practice? It needs to be the presshell for a root vm, right?

What if a non-root presshell has a popup widget?

What about it? Popups don't get a separate viewmanager, right?

So wouldn't the view manager for that non-root presshell that contains the popup need to do the painting itself?

Well, maybe. Right now in the patch all the refresh driver does is call TriggerRefresh on the view managers of all the presshells registered with it, and TriggerRefresh always forwards to the root VM.

And to be clear, TriggerRefresh just invalidates native widgets; it does NOT do any actual painting. Again, with this patch.

(In reply to comment #1) > We probably do still need to keep the Flush_Display stuff, no? I don't know what the use case for it is. I see that our reftest system uses windowutils.processUpdates, but I don't understand why it needs it yet. I'm going to find out now. Not sure about the root presshell view manager stuff yet.

(In reply to comment #8) > I see that our reftest system uses > windowutils.processUpdates, but I don't understand why it needs it yet. I think it doesn't need it. MozAfterPaint events and drawWindow results are completely independent from native widget drawing.

This sounds great!

We should consider flushing invalidates onload; otherwise there can be a 20ms pause between when onload fires and when things start to paint.

But we also want to not flush layout onload? Wouldn't that be a bad combination?

(In reply to comment #11) > We should consider flushing invalidates onload; otherwise there can be a 20ms > pause between when onload fires and when things start to paint. Why do we care about that? If somebody removes an iframe before it had a chance to paint, as is the case in bug 601332, isn't that his fault?

> But we also want to not flush layout onload? We don't want to flush layout before firing the onload event. I think we should in fact flush layout and invalidates right after firing the onload event, so the user gets to see the page right then. There are perceived performance benefits there, unlike the flush before onload. > isn't that his fault? Assigning fault on the web is usually pointless; the upshot in bug 601332 is that we look like we're hung and other browsers don't. Or that we take too long to paint things, if someone digs a little bit (which happens to be true!).

(In reply to comment #14) > the upshot in bug 601332 is that we look like we're hung OK, that's not good. Maybe we can fix that differently without flushing more than 60 times a second... I'll look into it.

Yeah, I'm not quite sure what the right solution for that situation is... The case when close() is called there (so onload fires) would be fixed by my proposal in comment 11, but the case when close() isn't called is hard.

(In reply to comment #9) > (In reply to comment #8) > > I see that our reftest system uses > > windowutils.processUpdates, but I don't understand why it needs it yet. > > I think it doesn't need it. MozAfterPaint events and drawWindow results are > completely independent from native widget drawing. For future reference, roc came to the same conclusion in bug 617152 comment 22.

Here's a more up to date patch that also mixes in lots of cleanup. It worked pretty well a few months ago, apart from very few test failures and a ts_shutdown regression. Unfortunately, on current trunk it results in window sizing weirdness and assertions, which I haven't had time to track down yet.

Markus, if you don't have time to work on this very much, maybe Mats could help.

Patches are almost ready for review; I've sent them to try one last time: https://tbpl.mozilla.org/?tree=Try&rev=01b32e3f1328

I'm not sure if this patch won't break anything. Plugin geometry updates are currently triggered from two sources: (1) around painting (either from WillPaint or from DidPaint) and (2) from a runnable. The purpose of (2) is to make sure that the update happens even if no painting occurs. But can we even hit that case? Maybe when a plugin moves offscreen or in a background tab? And what happens if we fail to do the plugin geometry update? Would it cause problems to only do the plugin geometry update when the plugin comes back onscreen (which will definitely trigger a paint)? The problem with triggering a plugin geometry update from a runnable is that the event might fire before the paint happens, so we'd end up with two paints. Another problem with SynchronousPluginGeometryUpdate() is that it uses a synchronous repaint, and I want to remove all sync paint APIs.

I think you removed the last caller (reftests) in bug 617152.

Have you tested if this regresses things like bug 635465 on Windows?

Have you checked if this regresses things like bug 635465 on Windows?

All updates now behave like NS_VMREFRESH_NO_SYNC, so the flag has become useless.

(this needs to be applied before part 11)

There's a comment above UpdateWidgetArea which justifies the existence of this argument, but in reality all callers just call ->GetWidget() on the view they pass in.

This is the most important part because it completes the connection to the refresh driver. This patch changes the meaning of several things. Most importantly, the result of IsRefreshEnabled() is now treated completely differently. Before this patch, while refresh was enabled (i.e. outside of any view update batch), invalidations were instantly passed to the widget, and widget geometry was update synchronously. While refresh was disabled (i.e. during a view update batch), widget geometry changes and invalidations were stored on the view, and only synced to the widget at the end of the outermost view update batch. So IsRefreshEnabled() meant all of "painting is allowed at the moment", "invalidations on the widget are allowed at the moment", and "widget geometry changes are expected to be synchronous". Now, IsRefreshEnabled() only means "widget geometry changes are expected to be synchronous". Since widget geometry changes can cause synchronous painting, this also means that painting is allowed during that time. Whether invalidations are stored on the view or on the widget doesn't depend on IsRefreshEnabled() any more. The new rules are: (1) All invalidations are stored on the view, and (2) they're only flushed to the widget when ProcessPendingUpdates() is called (which only happens via the refresh driver).

Most users of update view batches only wanted the paint coalescing feature, which is now the default even outside update view batches. So most uses of them can be removed. They're only necessary at times when painting is absolutely forbidden, e.g. during reflow.

This is a behavior change that maybe doesn't belong into this bug, but it's a caller that I didn't want to keep updating. I think it's completely unnecessary.

Unrelated cleanup which I stumbled upon while I was near.

Updates are now never synchronous, and asynchronous view updates are called invalidations in my book.

(In reply to Timothy Nikkel (:tn) from comment #25) > Have you tested if this regresses things like bug 635465 on Windows? Not yet, thanks for the pointer.

It does regress bug 635465 on Windows. :(

Ok so bug 635465 comment 12 seems like what we want to do to fix that. I just haven't gotten around to doing it yet. Do you want to take a stab at that?

(In reply to Markus Stange from comment #44) > It does regress bug 635465 on Windows. :( On OS X, too. Fortunately. (In reply to Timothy Nikkel (:tn) from comment #45) > Ok so bug 635465 comment 12 seems like what we want to do to fix that. I > just haven't gotten around to doing it yet. Do you want to take a stab at > that? Sure.

Comment on attachment 564231 [details] [diff] [review] part 1, v1: no sync plugin geometry update Review of attachment 564231 [details] [diff] [review]: ----------------------------------------------------------------- Imagine there's a windowed plugin covering the whole content window and it gets moved or removed. The plugin's widget sits there and blocks all repainting of the browser content. Assuming nothing else happens, no Gecko painting occurs, and without this code, we'll never update plugin geometry and update the plugin widget. At least, that's why I wrote this code. The reason the repaint is synchronous here is because it's important to make the plugin geometry update happen as quickly as possible. We can take this out if, after moving all painting to the refresh driver, we also do all the plugin geometry updates from the widget driver and they happen every time the refresh driver runs, even if the invalid areas are covered by a plugin widget. So this shouldn't be hard to fix, and it's OK to take this patch *if* we fix things up at the end of the patch queue, like I have suggested.

Comment on attachment 564261 [details] [diff] [review] part 13, v1: flush invalidations from the refresh driver instead of from view update batches Review of attachment 564261 [details] [diff] [review]: ----------------------------------------------------------------- We want to get rid of views, so it would actually be good to move storage of invalidations further out, probably to the frames corresponding to displayroots (to the callers of nsFrame::InvalidateRoot, actually). I'm not sure whether it makes sense to do that in this bug, or defer it. What do you think?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #47) > Imagine there's a windowed plugin covering the whole content window and it > gets moved or removed. The plugin's widget sits there and blocks all > repainting of the browser content. Assuming nothing else happens, no Gecko > painting occurs, and without this code, we'll never update plugin geometry > and update the plugin widget. I see, thank you. > We can take this out if, after moving all painting to the refresh driver, we > also do all the plugin geometry updates from the widget driver and they > happen every time the refresh driver runs, even if the invalid areas are > covered by a plugin widget. So this shouldn't be hard to fix, and it's OK to > take this patch *if* we fix things up at the end of the patch queue, like I > have suggested. OK, I'll do that. (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #48) > I'm not sure whether it makes sense to do that in this bug, or defer it. > What do you think? I'd rather defer it. And then ask you to do it. :-)

Comment on attachment 564231 [details] [diff] [review] part 1, v1: no sync plugin geometry update Review of attachment 564231 [details] [diff] [review]: ----------------------------------------------------------------- This is OK given you've promised to fix this in another patch :-)

One thing I'm a little concerned about is possibly adding latency to user actions where we're removing synchronous painting, like in scrolling. We may need to address this by creating a way to trigger immediate running of the refresh driver at the end of the current event; we'll see.

Comment on attachment 564237 [details] [diff] [review] part 5, v1: remove invalidate event stuff Review of attachment 564237 [details] [diff] [review]: ----------------------------------------------------------------- This patch effectively makes VMREFRESH_DEFERRED a no-op. I think you should remove it in this patch. I believe the uses of VMREFRESH_DEFERRED are fine except possibly for this one: http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/base/nsEditor.cpp#4231 but I think it's OK to just remove it. I believe that's the only real use of eEditorUseAsyncUpdatesMask, so we should remove that too.

ooops I see you're doing that in later patches!

Comment on attachment 564237 [details] [diff] [review] part 5, v1: remove invalidate event stuff Review of attachment 564237 [details] [diff] [review]: ----------------------------------------------------------------- This patch effectively makes VMREFRESH_DEFERRED a no-op. I think you should remove it in this patch. I believe the uses of VMREFRESH_DEFERRED are fine except possibly for this one: http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/base/nsEditor.cpp#4231 but I think it's OK to just remove it. I believe that's the only real use of eEditorUseAsyncUpdatesMask, so we should remove that too.

Sorry about that spurious comment, Splinter error.

Comment on attachment 564247 [details] [diff] [review] part 11, v1: set up a connection between view manager and refresh driver Review of attachment 564247 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsRefreshDriver.h @@ +165,5 @@ > + void RemoveInvalidationFlushObserver(nsIPresShell* aShell) { > + if (mInvalidationFlushObserver == aShell) { > + mInvalidationFlushObserver = nsnull; > + } > + } Instead of treating the presshell as an observer, why not just always call mPresContext->GetShell()->GetViewManager()->ProcessPendingUpdates()? Seems a lot simpler.

Comment on attachment 564261 [details] [diff] [review] part 13, v1: flush invalidations from the refresh driver instead of from view update batches Review of attachment 564261 [details] [diff] [review]: ----------------------------------------------------------------- Let's rename IsRefreshEnabled to IsPaintingAllowed.

Comment on attachment 564262 [details] [diff] [review] part 14, v1: reorganize update view batches Review of attachment 564262 [details] [diff] [review]: ----------------------------------------------------------------- This is actually a little scary since you're allowing paints in a lot more places, if someone happens to flush layout which used to be inside a view-update-batch. I wonder what breaks if we always delay widget geometry updates until the refresh driver runs. That would be safest and simplest.

Comment on attachment 564265 [details] [diff] [review] part 16, v1: separate NS_WILL_PAINT and NS_PAINT handling; only flush again if no NS_WILL_PAINT event has been sent by the platform Review of attachment 564265 [details] [diff] [review]: ----------------------------------------------------------------- We really need to file bugs to get all platforms to dispatch WILL_PAINT/DID_PAINT...

Comment on attachment 564261 [details] [diff] [review] part 13, v1: flush invalidations from the refresh driver instead of from view update batches Review of attachment 564261 [details] [diff] [review]: ----------------------------------------------------------------- ::: view/src/nsViewManager.cpp @@ +458,5 @@ > + nsViewManager* rootVM = RootViewManager(); > + rootVM->mHasPendingUpdates = true; > + if (rootVM->mObserver) { > + nsCOMPtr<nsIPresShell> shell = do_QueryInterface(rootVM->mObserver); > + shell->ScheduleInvalidationFlush(); Really shell->ScheduleInvalidationFlush() should be in nsIViewObserver.

Also, I realized there's a problem here due to view manager invalidation being global for the entire document tree, whereas we have separate nsRefreshDrivers for each document. I think these patches will *work*, but we could be increasing latency due to a content document's refresh driver causing invalidation, but that invalidation not being applied to the widget until the root document's refresh driver has run (followed by the actual paint). I'm not sure how serious a problem this is.

So, there's a few more things I think we should do in this area. They don't have to be done in this bug, but we should prepare for them. -- Connect refresh drivers in the same document tree together to avoid the problem in the previous comment. Really we should have only one timer for the entire document tree, have the root document's refresh driver be solely responsible for viewmanager invalidation flushing, and have it propagate notifications down the entire tree of refresh drivers (possibly optimizing to skip refresh driver subtrees that do not need to run). -- Paint synchronously in the root document refresh driver after it has flushed everything. This should reduce layout flushing because we're less likely to have dirtied the layout again before the painting triggered by the refresh driver happens. (This means we probably don't want patch 3 to land.) It could also reduce latency. -- When receiving a paint event other than during that synchronous paint in the refresh driver, just trigger an immediate run of the refresh driver and don't do anything else. (This might be a bit tricky if the synchronous paint of the refresh driver interferes with the OS paint event; we may need to make the refresh driver run slightly asynchronously.) -- Then the refresh driver will take care of all flushing before painting, so we don't need to flush in the view system. -- Then we may be able to avoid OS-level invalidates completely, by making our synchronous paint just paint directly instead of in response to an OS paint event.

Note that there isn't really a "tree" of refresh drivers. There's one for the toplevel chrome window and one for every root of a content docshell tree. That's it.

Oh right, even better, that would make step 1 in comment #62 easier.

Hmm, but we don't want all chrome painting to be gated on content document flushing (not that it matters with e10s, but still). hmm.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #51) > One thing I'm a little concerned about is possibly adding latency to user > actions where we're removing synchronous painting, like in scrolling. We may > need to address this by creating a way to trigger immediate running of the > refresh driver at the end of the current event; we'll see. I agree that it would make sense to skip the delay before the first timer firing, if the timer is not running already. (Otherwise the delay between two subsequent refresh driver runs would be unnecessarily short.) (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #56) > ::: layout/base/nsRefreshDriver.h > @@ +165,5 @@ > > + void RemoveInvalidationFlushObserver(nsIPresShell* aShell) { > > + if (mInvalidationFlushObserver == aShell) { > > + mInvalidationFlushObserver = nsnull; > > + } > > + } > > Instead of treating the presshell as an observer, why not just always call > mPresContext->GetShell()->GetViewManager()->ProcessPendingUpdates()? Seems a > lot simpler. Good idea. But the refresh driver still needs a method that tells it to start the timer, and a way to remember that something is waiting for an invalidation flush (so that we don't take the early exit in nsRefreshDriver::Notify). So it might not be that much simpler in the end. I'll give it a try. (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #58) > Comment on attachment 564262 [details] [diff] [review] [diff] [details] [review] > part 14, v1: reorganize update view batches > > Review of attachment 564262 [details] [diff] [review] [diff] [details] [review]: > ----------------------------------------------------------------- > > This is actually a little scary since you're allowing paints in a lot more > places, if someone happens to flush layout which used to be inside a > view-update-batch. Right. For some reason I only thought of popup widgets here, and not of child widgets (whose geometry changes might also cause sync painting)... I need to think about this again. > I wonder what breaks if we always delay widget geometry updates until the > refresh driver runs. That would be safest and simplest. That's what I tried first, but then I got all kinds of test failures involving popups. I've forgotten all the details (this was a year ago), so I'll fire off another try run with that change. But I think it was due to nsMenuPopupFrame.cpp or nsXULPopupManager.cpp working with widget coordinates instead of view coordinates. But maybe all that has changed in the meantime. (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #62) > (This might be a bit tricky if the synchronous paint > of the refresh driver interferes with the OS paint event; we may need to > make the refresh driver run slightly asynchronously.) And do nothing during the OS paint? That might work, but it's not very pretty. Or we could just do a refresh driver run without triggering the sync OS paint, and then do the paint outside the refresh driver directly from the OS paint. In any case we'd need to detect recursion in two places. With async painting we wouldn't need any recursion detection. We might have two subsequent refresh driver runs (one from the refresh driver timer, and one from inside the triggered OS paint), but if the second run has nothing to flush, that might be cheap enough to not make a difference. > -- Then we may be able to avoid OS-level invalidates completely, by making > our synchronous paint just paint directly instead of in response to an OS > paint event. But then we couldn't coalesce our own paints with OS-triggered paints, could we? And we'd have two different code paths entering painting. Do we have evidence of cases where asynchronous painting and going through OS-level invalidates induces noticeable delays?

> > -- Then we may be able to avoid OS-level invalidates completely, by making > > our synchronous paint just paint directly instead of in response to an OS > > paint event. > > But then we couldn't coalesce our own paints with OS-triggered paints, could > we? And we'd have two different code paths entering painting. I believe OS-triggered paints are relatively rare, especially on modern composited systems where the OS is usually retaining the contents of our window anyway. > Do we have evidence of cases where asynchronous painting and going through > OS-level invalidates induces noticeable delays? No, but no-one's looked. We have had a lot of problems with event prioritization involving paint events, which would become non-issues if we simply painted ourselves off a timer. > With async painting we wouldn't need any recursion detection. We might have two > subsequent refresh driver runs (one from the refresh driver timer, and one from > inside the triggered OS paint), but if the second run has nothing to flush, that > might be cheap enough to not make a difference. While animations are running, the second run always has something to do.

Will this fix bug 693707?

Probably. You can test with this build: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mstange@themasta.com-d776b413eb0b/try-macosx64/firefox-10.0a1.en-US.mac.dmg

Comment on attachment 566887 [details] [diff] [review] part 13, v2 Review of attachment 566887 [details] [diff] [review]: ----------------------------------------------------------------- ::: view/src/nsViewManager.cpp @@ +790,5 @@ > + } > + } > + > + if (!didResize) { > + //NS_ASSERTION(view->IsEffectivelyVisible(), "painting an invisible view"); Is this assertion valid? I think not, it seems to me the flush could change the view visibility. Just take it out.

(In reply to Markus Stange from comment #66) > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #58) > > Comment on attachment 564262 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review] > > part 14, v1: reorganize update view batches > > > > Review of attachment 564262 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]: > > ----------------------------------------------------------------- > > > > This is actually a little scary since you're allowing paints in a lot more > > places, if someone happens to flush layout which used to be inside a > > view-update-batch. > > Right. For some reason I only thought of popup widgets here, and not of > child widgets (whose geometry changes might also cause sync painting)... I > need to think about this again. > > > I wonder what breaks if we always delay widget geometry updates until the > > refresh driver runs. That would be safest and simplest. > > That's what I tried first, but then I got all kinds of test failures > involving popups. I've forgotten all the details (this was a year ago), so > I'll fire off another try run with that change. But I think it was due to > nsMenuPopupFrame.cpp or nsXULPopupManager.cpp working with widget > coordinates instead of view coordinates. But maybe all that has changed in > the meantime. These are the only issues remaining to be addressed, AFAIK...

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #73) > > But I think it was due to > > nsMenuPopupFrame.cpp or nsXULPopupManager.cpp working with widget > > coordinates instead of view coordinates. But maybe all that has changed in > > the meantime. My patches in bug 668437 change this.

What's the status on this? Its been very quiet since the 24th.

I still see > 120Hz repaints on mac. We should really fix this.

Giving to Boris to finish addressing comments and get this landed!

> Let's rename IsRefreshEnabled to IsPaintingAllowed. Done. > We really need to file bugs to get all platforms to dispatch WILL_PAINT/DID_PAINT... Which platforms need those bugs? > I wonder what breaks if we always delay widget geometry updates until the refresh driver > runs. Kicked off a try build for that.

(In reply to Boris Zbarsky (:bz) from comment #78) > > We really need to file bugs to get all platforms to dispatch WILL_PAINT/DID_PAINT... > > Which platforms need those bugs? Windows, Mac and GTK support WILL_PAINT. So Android, Qt, PuppetWidget and OS/2 need WILL_PAINT. Windows, GTK and PuppetWidget support DID_PAINT. So Android, Qt, OS/2 and Mac need DID_PAINT.

Try run for ff2e449c4945 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=ff2e449c4945 Results (out of 209 total builds): success: 171 warnings: 37 failure: 1 Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bzbarsky@mozilla.com-ff2e449c4945

Sorry for the silence. Here's a try push with more up-to-date patches: https://tbpl.mozilla.org/?tree=Try&rev=51b7bb1988ff Here's an older one with the same patches but an older mozilla-central base: https://tbpl.mozilla.org/?tree=Try&rev=7a371ec21c6f There are some mochitest-other failures on Mac which I can't reproduce. The mochitest-1 failure is tnikkel's test from bug 592954 which unfortunately isn't fixed by the flush I added for bug 635465 in https://hg.mozilla.org/try/rev/965f95d8d123 . The main changes in that try push compared to the patches that are already attached here are addressing comment 58; i.e. making all widget geometry changes asynchronous. That happens in https://hg.mozilla.org/try/rev/b4ecd98e61de and https://hg.mozilla.org/try/rev/39130b9def6e . (In reply to Timothy Nikkel (:tn) from comment #74) > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #73) > > > But I think it was due to > > > nsMenuPopupFrame.cpp or nsXULPopupManager.cpp working with widget > > > coordinates instead of view coordinates. But maybe all that has changed in > > > the meantime. > > My patches in bug 668437 change this. The three places that still needed fixing were nsPopupBoxObject::GetOuterScreenRect, nsResizerFrame::HandleEvent and nsTitleBarFrame::HandleEvent. https://hg.mozilla.org/try/rev/5332a0aa1ded The remaining popup test failures which I could reproduce locally were caused by popup move and resize events that fire at times when the current code doesn't expect them. https://hg.mozilla.org/try/rev/4c7619d98fce prevents them from firing on Mac for popups without titlebars, but that's probably not the right fix. In fact I'm not sure what should be the correct behavior here. For example, in one case a test opened a popup, operated on it, and closed it. When a popup is closed, its view is resized to 0x0. When that resize was synced to the widget, the resulting resize event set width="0" height="0" attributes on the popup in nsXULPopupManager::PopupResized, and when the popup was reopened it was too small. Maybe we shouldn't fire move and resize events while a widget is closed, but I think that wouldn't fix all problems. For example, in the async widget visibility world, when a popup is closed and reopened without a view manager flush in between, the widget is never hidden. https://hg.mozilla.org/try/rev/2e095cf5e2e1 isn't really necessary if we block popup move events, but it will be if we decide that we don't want the blocking. Boris, do you want to continue with the old patches or the new ones? Should I attach my new patches here, or should we save the async view geometry work for another bug?

Try run for 1c3a590d6f11 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=1c3a590d6f11 Results (out of 211 total builds): success: 183 warnings: 28 Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bzbarsky@mozilla.com-1c3a590d6f11

<sigh>. So what I have locally is a copy of the patches in this bug but merged to tip, with the following changes: 1) Rename IsRefreshEnabled to IsPaintingAllowed 2) Make sure that ResetWidgetBounds only resizes widgets when called from ProcessPendingUpdates. It's a somewhat less invasive change than https://hg.mozilla.org/try/rev/b4ecd98e61de and can be found at https://hg.mozilla.org/try/rev/9719f8c1acc7 3) To work around the obvious fails that result in Moth popup tests with jut #2, I added some code that flushes out widget resizes when a Flush_Layout happens on the presshell. That's found in the same changeset as #2. I'm not making any attempts to do async widget visibility changes. With those changes, I get the try run in comment 82. It looks like there's some random orange, but not anything obviously caused by these patches. So if we think that the flush described above and sync visibility changes are safe, I would tend to think we should land that and worry about async visibility changes and weaning popups off widget coordinates in other bugs. Thoughts?

I like Boris's approach. It sounds pretty safe. (In reply to Markus Stange from comment #49) > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #47) > > We can take this out if, after moving all painting to the refresh driver, we > > also do all the plugin geometry updates from the widget driver and they > > happen every time the refresh driver runs, even if the invalid areas are > > covered by a plugin widget. So this shouldn't be hard to fix, and it's OK to > > take this patch *if* we fix things up at the end of the patch queue, like I > > have suggested. > > OK, I'll do that. Has this been done? I don't see it.

So I'm actually still seeing a bunch of orange on my try push that does not in fact look random now that I've respun some builds. :(

Current progress update: Most of the orange is fixed. The remaining issues are a Windows debug accessibility test leak that's caused by part 13 of the patch queue and a Linux reftest fail that roc and I are still debugging. Next up, looking into addressing comment 47.

bz, I think we are about to remove support for windowed plugins so we get async painting, so comment 47 might be moot.

No we aren't!

jst can you clarify? Maybe I misunderstood our plan for asynchronous painting but my impression was we will force windowless mode for all plugin objects on a page in the future (or well the flash plugin will and we will deprecate and drop support for windowed mode).

Many plugins don't support windowless mode, or won't be updated to use our new async rendering APIs in a timely manner. So we can't drop support for windowed plugins without completely dropping support for lots of plugins (everything except Flash, probably). Tempting as that is, I don't think we can do it. (Making them all click-to-play might be reasonable though.)

In practice anything but flash and maybe silverlight are mostly irrelevant. We can make it click to play and deprecated. It's definitely not something we should focus on supporting or even optimize for.

This discussion belongs in another forum. But I'm confident we won't be completely dropping Java support anytime soon. We won't even be dropping windowed Flash support anytime soon, since that's gated on us shipping our new API, Adobe implementing support for it, and updating the vast majority of Flash users to a version supporting the API. We certainly don't want this bug to be waiting for that!

(In reply to Andreas Gal :gal from comment #89) > jst can you clarify? Maybe I misunderstood our plan for asynchronous > painting but my impression was we will force windowless mode for all plugin > objects on a page in the future (or well the flash plugin will and we will > deprecate and drop support for windowed mode). The long term plan is certainly to drop windowed plugin support, but how quickly we can do that is unclear. What seems clear from discussion with flash engineers is that we can very soon start forcing flash into windowless mode and that way get async painting for all flash no matter what the webpage requests. Whether that's something we do in the browser or if that's something flash does internally is tbd.

(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #93) > The long term plan is certainly to drop windowed plugin support... This will increase the visibility of bug 686673, so hopefully it can be fixed before then.

Adding [Snappy] search string. Synching invalidates to the refresh driver should improve perceived performance.

Progress update: The linux reftest I mention in comment 86 is fixed, and I've figured out how to work around the a11y fail with a change to the setTimeout value in the test for now. Planning on comment 47 work tomorrow.

I also pushed the relevant mq to >http://hg.mozilla.org/users/bzbarsky_mozilla.com/async-painting-queue/> in case people want to look. My apologies for the extraneous files there...

OK. So I've been staring at comment 47 for a bit. Is this just a matter of calling nsRootPresContext::UpdatePluginGeometry on every refresh tick? I'm not sure whether the "even if the invalid areas are covered by a plugin widget" part from comment 47 means that UpdatePluginGeometry is not OK to use... If this is just a matter of UpdatePluginGeometry, then I assume that the only reason we need to add it is that refresh driver is doing interruptible layout flushes and PresShell::FlushPendingNotifications only updates plugin geometry on non-interruptible ones? On the assumption that this is true, going to post a patch to add the UpdatePluginGeometry call....

(In reply to Boris Zbarsky (:bz) from comment #101) > If this is just a matter of UpdatePluginGeometry, then I assume that the > only reason we need to add it is that refresh driver is doing interruptible > layout flushes and PresShell::FlushPendingNotifications only updates plugin > geometry on non-interruptible ones? Currently FlushPendingNotifications, via PresShell::DoReflow, only calls RequestUpdatePluginGeometry, which runs SynchronousPluginGeometryUpdate off an event. And that tries to trigger a repaint which will update the plugin geometry in WillPaint. The goal is to have plugin geometry changes happen as close as possible in time to the actual repainting of the window. The change in part 1 means that we just wait for the next paint of the presshell to do the geometry changes, which might never happen, as described in comment #47. Your patch in part 23 will ensure that plugin geometry gets updated. I wonder if it will increase the delay between updating plugin geometry and repainting the window, though. Eventually (or maybe soon) I want the refresh driver to synchronously paint the window, which would probably fix it (especially if we update the layer tree update the plugin geometry, then just composite the layer tree). Perhaps a better alternative to part 23 would be to just use a timer or something to ensure that the UpdatePluginGeometry eventually happens if no paint is forthcoming.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #103) > (In reply to Boris Zbarsky (:bz) from comment #101) > > If this is just a matter of UpdatePluginGeometry, then I assume that the > > only reason we need to add it is that refresh driver is doing interruptible > > layout flushes and PresShell::FlushPendingNotifications only updates plugin > > geometry on non-interruptible ones? Oh, I see what you mean now. That UpdatePluginGeometry call was added later, at least in my thought processes, to ensure plugin geometry is up-to-date when plugin tests talk to the plugin after flushing. It's fortunate that the refresh driver currently doesn't trigger this.

> Windows, Mac and GTK support WILL_PAINT. So Android, Qt, PuppetWidget and > OS/2 need WILL_PAINT. Filed bug 710765, bug 710767, bug 710769, bug 710770. > Windows, GTK and PuppetWidget support DID_PAINT. So Android, Qt, OS/2 and > Mac need DID_PAINT. Filed bug 710777, bug 710774, bug 710772, bug 710773.

Comment on attachment 581818 [details] [diff] [review] Timer to update plugin geometry, actually passing tests Review of attachment 581818 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsPresContext.cpp @@ +2615,5 @@ > + // so set it for double the refresh driver interval. > + mUpdatePluginGeometryTimer = do_CreateInstance("@mozilla.org/timer;1"); > + if (mUpdatePluginGeometryTimer) { > + mUpdatePluginGeometryTimer-> > + InitWithFuncCallback(UpdatePluginGeometryCallback, this, 32, Can we actually expose DEFAULT_FRAME_RATE and use it here instead of having a magic number?

Will land after the branchpoint so this has a full cycle of bake time.

Have we resolved comment 25?

Using the win32 build from what appears to be the latest try push, the bug 635465 testcase doesn't appear to work correctly. When I try to hover over the second link, it disappears. The testcase works on trunk, of course. https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bzbarsky@mozilla.com-602317873a2c/

> Have we resolved comment 25? Gah. I assumed that passing tests would handle that, but apparently not... I'll look into that, I guess. :(

I tried and failed to write a test for that, sorry.

Comment on attachment 583973 [details] [diff] [review] part 24. Flush on every mousemove, because otherwise we can end up with mouse events (mousemove, mousein, mouseout) dispatched to the wrong elements. >+ // Flush pending layout changes, so that later mouse move events >+ // will go to the right nodes. >+ FlushPendingEvents(aPresContext); Strange method name, but not your fault.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c631f9c3e9a9 https://hg.mozilla.org/integration/mozilla-inbound/rev/df447fcc75f4 https://hg.mozilla.org/integration/mozilla-inbound/rev/0787c2f0c45f https://hg.mozilla.org/integration/mozilla-inbound/rev/e9834142dbbd https://hg.mozilla.org/integration/mozilla-inbound/rev/5b25cab4ad9d https://hg.mozilla.org/integration/mozilla-inbound/rev/88ebe1a18f0d https://hg.mozilla.org/integration/mozilla-inbound/rev/aba624563ee7 https://hg.mozilla.org/integration/mozilla-inbound/rev/b3fa93cdc185 https://hg.mozilla.org/integration/mozilla-inbound/rev/583ac6c21fbe https://hg.mozilla.org/integration/mozilla-inbound/rev/42fde5b5e098 https://hg.mozilla.org/integration/mozilla-inbound/rev/fc0f7ad1de41 https://hg.mozilla.org/integration/mozilla-inbound/rev/84a70e2a270e https://hg.mozilla.org/integration/mozilla-inbound/rev/4ce03bf986e7 https://hg.mozilla.org/integration/mozilla-inbound/rev/51699a063cea https://hg.mozilla.org/integration/mozilla-inbound/rev/02cfda93f548 https://hg.mozilla.org/integration/mozilla-inbound/rev/1116117b73cc https://hg.mozilla.org/integration/mozilla-inbound/rev/2147078591dc https://hg.mozilla.org/integration/mozilla-inbound/rev/05a23c8d3acb https://hg.mozilla.org/integration/mozilla-inbound/rev/01db2746f7d2 https://hg.mozilla.org/integration/mozilla-inbound/rev/3e1a744fba1e https://hg.mozilla.org/integration/mozilla-inbound/rev/2a36941a6991 https://hg.mozilla.org/integration/mozilla-inbound/rev/09f0d4cc58cc https://hg.mozilla.org/integration/mozilla-inbound/rev/9cb424c5127d https://hg.mozilla.org/integration/mozilla-inbound/rev/1ac4cb2e7c32

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/25224a78f895 Remember a try push a few days (or weeks, dunno) ago, that I starred for you, but instead of starring the Android XUL reftest-3, I retriggered it three or four times, and every one was orange? That must have been part of the patches for this, since I remember those failures as having been svg/as-image/canvas-drawImage-simple-1a.html and svg/as-image/canvas-drawImage-origin-clean-1.html, same as this hit on inbound, and that was supposed to be an orange "look at me" flag. I don't remember it having had the 14 to 19 failures in canvas reftests in reftest-2, though, so those may have been added by later parts that weren't in that try push yet, though they were in today's try push.

Less of a problem, but you seem to be making the intermittent unexpected pass of bug 636379 much less intermittent - 5 of the 8 runs while you were in unexpectedly passed.

OK, so the Android reftest issue shows up first in the patch that actually moves invalidates to the refresh driver. The reftest screenshots show that some things are just not being painted, at least by the time the screenshot happens. I assume this is something to do with out of process rendering. Chris, any idea what I should be looking for here?

The failures were in canvas drawing? Hmm, I'm not sure. The major difference in the reftest harness between the same-process and multi-process is that in multi-process, the content process doesn't snapshot window contents, but rather asks the chrome process to do so. And before the chrome process can snapshot, the content process has to "flush" all pending layers updates, i.e. force shadow layer updates to propagate to the chrome process. The code to do that is http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/reftest-content.js#675 I'm not too familiar with the patches here, but maybe something changed wrt CanvasLayer behavior, maybe in incomplete layer transactions? I can't help more without understanding these patches better, but that's where I would start looking. Note too that you can run these tests on desktop with |make -C $objdir reftest-ipc|, so you should be able to bisect over the patches here. You'll likely have the most success on linux-x11. I can also send a mozconfig that should work there. (But of course, if there's something timing related here too, that might not repro reliably on desktop :/.)

> The failures were in canvas drawing? Yes. The main change in these patches is that invalidates are flushed to the widget layer off the refresh driver, instead of synchronously. As far as bisecting, I did that on try already. The patch that does the actual switch to using the refresh driver is what causes the issue. Will look at that link. Thanks!

(In reply to Boris Zbarsky (:bz) from comment #122) > The main change in these patches is that invalidates are flushed to the > widget layer off the refresh driver, instead of synchronously. > How does that interact with drawWindow(USE_WIDGET_LAYERS)?

> How does that interact with drawWindow(USE_WIDGET_LAYERS)? Good question. Will check.

OK, I can reproduce _one_ of the failures involved using reftest-ipc, I think. Maybe. I added code in drawWindow to flush out the invalidates by calling presShell->GetViewManager()->ProcessPendingUpdates() when drawing with USE_WIDGET_LAYERs, but that didn't help. Turning off empty transactions for canvas by both commenting out the check for canvas in nsIFrame::InvalidateLayer and adding "&& 0" to the check for isRetainingManager in PresShell::Paint didn't help.

I tried adding an invalidate flush in RenderFrameParent::ShadowLayersUpdated but I don't see why that would be needed; it's mostly flailing...

Oh, and it doesn't obviously fix things locally, though I pushed it to try just in case.

The flush in RenderFrameParent::ShadowLayersUpdated shouldn't be needed. In general, what's supposed to happen is - SynchronizeForSnapshot(): force content-process paint, in order to force PLayers:Update message to be sent. * is PLayers:Update being sent? (http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/ShadowLayers.cpp#324) * if so, are the pixels in the buffer of the CanvasLayer in the layer tree correct? If PLayers:Update *isn't* being sent, then something is probably bailing out early. I'm not sure why; the best guess previously was empty transactions. If PLayers:Update *is* being sent, but the CanvasLayers in these tests have the wrong pixels, then there's likely a bug in some invalidation/repainting logic. PuppetWidget would seem to be most suspect. (It's extremely unlikely that PLayers:Update is being sent with the correct CanvasLayer pixels, but there's some bug in the chrome process.)

> * if so, are the pixels in the buffer of the CanvasLayer in the layer tree correct? How would I tell? Keep in mind that I still can't reproduce this locally, at least on Mac, so I'm having to use tryserver for all my debugging....

Easiest would be to use DumpAsDataURL() [1] after the UpdateSurface() call here [2]. I'm not sure that stdout will make it to the tinderbox log though, so you might need to dump it somewhere else :(, and/or with a magical prefix. [1] http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxASurface.h#242 [2] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/basic/BasicLayers.cpp#2691

OK, so getting things into the log is rocket science. I can do it if I'm willing to lose part of my data to the reftest logging code itself, but I haven't managed to actually get my data into the log sanely. I tried building fennec, and I can install it on my tablet, but it seems like running reftest only works on rooted devices or something. It totally fails with permissions errors on my (non-rooted) tablet. I'm somewhat at a loss as to where to go from here. :(

Not being able to run reftests on an unrooted device smells like a bug to me, if that's the problem. Maybe jmaher can help with that tomorrow?

OK, I finally have reftests running. At first glance, I'm getting calls to SendUpdate, and the two data: URLs that are dumped in EndTransaction both look correct. I'll try to dig more into the exact timing of things here if I can, but probably not until Wednesday.

I've spun off part 24 into bug 716549.

Sorry is this issue described here dependent on this https://bugzilla.mozilla.org/show_bug.cgi?id=702485 compared to the chromeium behavior ?

That's a completely unrelated issue.

After much log-wrangling and such with Chris, we found bug 718150. Fixing that made at least one of the failing tests pass for me; I'll check the others.

Yep, the patch for bug 718150 fixed the android failures. Now looking at the other issues that seem to have popped up in the interim...

Second time is the charm, right? https://hg.mozilla.org/integration/mozilla-inbound/rev/b820017dc738 https://hg.mozilla.org/integration/mozilla-inbound/rev/7dd97ae4452d https://hg.mozilla.org/integration/mozilla-inbound/rev/487d669f7973 https://hg.mozilla.org/integration/mozilla-inbound/rev/3b7183745bb9 https://hg.mozilla.org/integration/mozilla-inbound/rev/a0238d9206c2 https://hg.mozilla.org/integration/mozilla-inbound/rev/e83f53b3302e https://hg.mozilla.org/integration/mozilla-inbound/rev/a4e8d5f7c9be https://hg.mozilla.org/integration/mozilla-inbound/rev/04292f9ff363 https://hg.mozilla.org/integration/mozilla-inbound/rev/61a1f0b28812 https://hg.mozilla.org/integration/mozilla-inbound/rev/ac01aec1a983 https://hg.mozilla.org/integration/mozilla-inbound/rev/4616fa6c1dc7 https://hg.mozilla.org/integration/mozilla-inbound/rev/2d5e31af7e3e https://hg.mozilla.org/integration/mozilla-inbound/rev/4b492a0e51f5 https://hg.mozilla.org/integration/mozilla-inbound/rev/d7dcde1032ed https://hg.mozilla.org/integration/mozilla-inbound/rev/a35342556482 https://hg.mozilla.org/integration/mozilla-inbound/rev/6d9721de770c https://hg.mozilla.org/integration/mozilla-inbound/rev/71d77495b606 https://hg.mozilla.org/integration/mozilla-inbound/rev/b9e630462207 https://hg.mozilla.org/integration/mozilla-inbound/rev/05dff2f69c0c https://hg.mozilla.org/integration/mozilla-inbound/rev/88b93c358499 https://hg.mozilla.org/integration/mozilla-inbound/rev/0d025e2e0134 https://hg.mozilla.org/integration/mozilla-inbound/rev/8988bb111202 https://hg.mozilla.org/integration/mozilla-inbound/rev/399dace827fa https://hg.mozilla.org/integration/mozilla-inbound/rev/5e6e63f3aed8

And edmorley merged it to m-c: https://hg.mozilla.org/mozilla-central/rev/b820017dc738 https://hg.mozilla.org/mozilla-central/rev/7dd97ae4452d https://hg.mozilla.org/mozilla-central/rev/487d669f7973 https://hg.mozilla.org/mozilla-central/rev/3b7183745bb9 https://hg.mozilla.org/mozilla-central/rev/a0238d9206c2 https://hg.mozilla.org/mozilla-central/rev/e83f53b3302e https://hg.mozilla.org/mozilla-central/rev/a4e8d5f7c9be https://hg.mozilla.org/mozilla-central/rev/04292f9ff363 https://hg.mozilla.org/mozilla-central/rev/61a1f0b28812 https://hg.mozilla.org/mozilla-central/rev/ac01aec1a983 https://hg.mozilla.org/mozilla-central/rev/4616fa6c1dc7 https://hg.mozilla.org/mozilla-central/rev/2d5e31af7e3e https://hg.mozilla.org/mozilla-central/rev/4b492a0e51f5 https://hg.mozilla.org/mozilla-central/rev/d7dcde1032ed https://hg.mozilla.org/mozilla-central/rev/a35342556482 https://hg.mozilla.org/mozilla-central/rev/6d9721de770c https://hg.mozilla.org/mozilla-central/rev/71d77495b606 https://hg.mozilla.org/mozilla-central/rev/b9e630462207 https://hg.mozilla.org/mozilla-central/rev/05dff2f69c0c https://hg.mozilla.org/mozilla-central/rev/88b93c358499 https://hg.mozilla.org/mozilla-central/rev/0d025e2e0134 https://hg.mozilla.org/mozilla-central/rev/8988bb111202 https://hg.mozilla.org/mozilla-central/rev/399dace827fa https://hg.mozilla.org/mozilla-central/rev/5e6e63f3aed8

Backed out on Aurora 12 in bug 722641.

Marking this REOPENED until we get the code landed again.

The code is just fine on m-c. It was only backed out on Aurora 12.