I'm planning to eliminate the use of native child widgets in Gecko, so that all content is rendered in one native widget, except of course for windowed plugins, which will still need their own widgets. To get there, we have to first hoist plugin windows to be children of Gecko's topmost widget. That's what this bug is about. The obvious problem is that we currently rely on our hierarchy of native child widgets to clip and scroll plugin windows. I propose to replace that with platform-independent code in nsPresShell that computes, for each plugin window, the region of that window that should be visible, and also platform-dependent code in nsIWidget implementations to clip a window to a particular region. GTK and Windows have native APIs for this. For Mac, the plugin implementation can clip the plugin to a particular rect, so I'll just use that to clip to the bounding box of the desired region. I'm going beyond what we do today because the visible region computation uses frame display lists and takes full account of CSS z-order and also takes account of opaque content above the plugin(s) in z-order. So if you have, say, a DIV with a background color positioned over a Flash object, we'll punch a hole in the Flash object's window to ensure the DIV is visible. This will actually address some site compatibility problems. Another obvious question is how well this will work when we scroll, because we won't be able to scroll pixels and child widgets in one atomic operation the way we do today. In fact we have to juggle scrolling pixels, moving plugin windows, and adjusting their clipping regions and visibility state. With my current patch (GTK only right now) I think the results are already acceptable, although they can be improved. The trick is to a) update plugin geometries as soon as possible after scrolling, and before the view manager does its Composite(), so we force a synchronous paint of areas damaged by plugin geometry changes, and b) set the window's clip region to the intersection of its old and new clip regions before we move it, and set the new clip region after we move it. The latter strategy avoids problems when the window is scrolling in or out of view. Consider that just setting the new clip region after moving the window causes problems when the window is being scrolled out of view, but setting the new clip region before moving the window causes problems when the window is being scrolled into view.

This is my current patch. It needs Mac and Windows support before I can even think about landing it. I'm going to work on the Mac side at home. Windows should be fairly easy, very much like the GTK2 implementation, although I'll have to track someone down to test it for me.

testcase for "DIV over plugin" and scrolling

Cleaned up a bit, plus some entirely untested code for Windows support.

Why don't you use BeginResizingChildren (that is, ::DeferWindowPos family) or ::LockWindowUpdate for Windows?

We probably should. I designed this nsIWidget API specifically to support platform-specific hacks like that. Someone needs to experiment on Windows to find out which approach works best. If you can help, that'd be great :-).

Sorry, the previous patch had some parts missing (gfx and modules/plugin changes). This is the complete patch.

Oops. The "Full Patch" actually is missing changes to view/. Here are those changes.

The Mac stuff seems to work somewhat, but has bugs. I also need to port the Mac bits to Cocoa. I still need help to test this on Windows.

Will this fix be in FF 2.0 release?

(In reply to comment #11) > Will this fix be in FF 2.0 release? No, FF2.0 will be released in a few days.

In view of the call for Windows testing, I had a go at merging the now rather bitrotted patch to TIP. Unfortunately when trying to load the testcase Minefield hangs. A quick look at www.adobe.com shows the flash on the homepage scrolling fine. Dunno if this is an issue with the patch or an issue with my merging conflicts. Could well be the latter.

It might also be that one or more of the plugin DLLs that I copied from Firefox 2.0 don't work with Minefield. The installer failed when Minefield tried to fetch the required plugin.

Comment on attachment 226363 [details] [diff] [review] Full patch, now including a widget/mac implementation What's with this patch? I thought I'd have a go at applying the patch to the source as it was back when the diff was made to see if it worked on Windows back then. Frustratingly patch just doesn't seem to be able to apply cleanly. I'm getting hunks failing despite checking that the revision of each file that's failing is the same as the revision the patch claims to have been against.

(In reply to comment #13) > In view of the call for Windows testing, I had a go at merging the now rather > bitrotted patch to TIP. Unfortunately when trying to load the testcase > Minefield hangs. Yeah, I'm seeing it too with the patch, Firefox hangs when I load the testcase with the patch applied.

Robert, I'm still not sure what's up with your original patch, but here it is fixed up to apply to the source as it was at the time you took the diff (MOZ_CO_DATE="30 May 2006 20:57:07").

Building a MOZ_CO_DATE="30 May 2006 20:57:07" tree with the above patch (cairo build) I don't hang, but I get the following output and a completely empty (grey) content area. (This is a debug build, I'll try release later in case the plugin has problems with debug builds.) HELLO, I HAVE BEEN CALLED For application/x-shockwave-flash found plugin C:\mozilla\trees\plugin-view-test\debug\dist\bin\plugins\NPSWF32.dll HELLO, I HAVE BEEN CALLED HELLO, I HAVE BEEN CALLED HELLO, I HAVE BEEN CALLED HELLO, I HAVE BEEN CALLED HELLO, I HAVE BEEN CALLED HELLO, I HAVE BEEN CALLED HELLO, I HAVE BEEN CALLED HELLO, I HAVE BEEN CALLED HELLO, I HAVE BEEN CALLED Clip 0569B660(Area(div)(17)) (0,0,13305,6660) CanvasBackground 05676640(Canvas(html)(-1)) (0,0,13305,9420) uniform Clip 00000000() (1080,960,5715,7215) Plugin 0569B840(ObjectFrame(embed)(19)) (1080,5160,7470,975) opaque Background 0569AAA0(ScrollbarFrame(scrollbar)(-1)) (6795,960,285,7215) Background 0569B660(Area(div)(17)) (3000,0,3000,7500) opaque uniform ###!!! ASSERTION: Attempt to decrement focus controller's suppression when no suppression active! : 'PR_FALSE', file c:/mozilla/trees/plugin-view-test/mozilla/dom/src/base/nsFocusController.cpp, line 501 HELLO, I HAVE BEEN CALLED

I talked to afri and I think you can hack around the problems for your needs without taking this patch. Of course I'd still be happy if you can debug this :-)

Oh! I forgot to reply to that, sorry. I also forgot to say that I went through my update of your patch with a fine tooth comb, then did the update again in a different way. Both times I got the same result so I'm positive I got the update to tip right. I should add that I was doing a brain dead update, by which I mean resolving conflicts and making obvious simple changes, but without understanding how the patch works and therefore not taking into account changes to the tree that might required bigger changes to the patch. I still have the patch in one of my trees, and until very recently (Eli's units rewrite for example) I hadn't had any conflicts. I've resolved all the recent conflicts bar two which I've highlighted with "XXXjwatt" comments inline in this patch. Probably not too hard to figure out but at the moment I want to get on with other things, yet I don't want to forget to attach this patch so here it is. ;-)

I've looked at the XXXjwatt comments in your patch, and found that you are using t2p extensively. Is this still correct after the unit fixing? I don't think so. Maybe you want to use the new app units to dev units conversion function (I don't recall its name right now).

For various reasons I ended up starting over. I'll attach a series of patches here, but they will have to land simultaneously with the patches for bug 352093, because eliminating plugin flicker while scrolling depends on removing child widgets from the content area (which depends on this!).

I'm going to introduce an nsRootPresContext class which is used for prescontexts which are at the top of a view hierarchy. In particular an nsRootPresContext is responsible for managing plugins. nsDocumentViewer needs to know what sort of prescontext to create, so I need to lift the code that decides whether we should be at the top of the view hierarchy out of MakeWindow so it can be called before we create the prescontext. This patch does that, creating FindContainerView which returns null if we should be the root of a view manager hierarchy, or the parent view we should hook up to if we shouldn't be a root.

Introduce nsRootPresContext, which is no different from nsPresContext for now, and create it when necessary. Also change the return type of nsPresContext::RootPresContext().

Comment on attachment 385564 [details] [diff] [review] Part 1: factor out FindContainerView I need to think about who's going to review stuff here.

This patch introduces a native widget clip region API so that we can clip native widgets associated with plugins. The format is a little unusual; instead of some sort of SetClipRegion on each widget, you can set the clip regions of a set of children with the same parent all at once. You also set their bounds at the same time. The need for this list will become clear near the end of bug 352093 --- we'll pass the same list into the Scroll method so we can fix widget geometries and clipping while we scroll. This patch also modifies nsViewManager so that it takes clipped widgets into account properly when it's excluding areas that are covered by widgets.

nsViewManager::AddCoveringWidgetsToOpaqueRegion tries to exclude from the paint region areas which are covered by child widgets. It's a pain to maintain, and completely unnecessary on Windows since Windows already does it for us. Now that all platforms pass down a paint region in the NS_PAINT event, we can just make this the platform's responsibility. The nsViewManager changes only remove code, and the rest is Windows and GTK widgetry.

Comment on attachment 385567 [details] [diff] [review] Part 4: Remove nsViewManager::AddCoveringWidgetsToOpaqueRegion Er, Mac and GTK widgetry. So giving the primary review to Karl and the secondary review to Josh for the Mac bits.

We're going to use display lists to compute the clip regions of each plugin. Currently people place HTML content over windowed plugins by putting it in (or over) content which creates a native widget, like an IFRAME. Removing widgets from IFRAMEs breaks that, so to make that content appear we're going to exclude areas of opaque content covering a plugin from the plugin's clip region. To do that reliably we need to disable the performance-enhancing simplifications of visible regions that the display list system does. This does mean that if someone covers a plugin with thousands of opaque DIVs, we'll lose. I think that's OK for now.

I'm going to need to #include "nsDisplayList.h" in nsObjectFrame.h, and that breaks various things (we include nsObjectFrame.h in weird places) because nsDisplayList.h #includes "nsLayoutUtils.h" which can't be included everywhere. The simplest thing to do seemed to be to just unlinine IsMovingFrame, which is the only thing that needs nsLayoutUtils.h. A few files needed nsLayoutUtils and were currently depending on getting it through nsDisplayList, so they need to #include nsLayoutUtils.h explicitly now.

We need a method like nsLayoutUtils::IsProperAncestorFrameCrossDoc, but that returns true when the specified ancestor is the frame itself.

We need to be able to snap an appunits point to the nearest pixel corner.

This is the main patch to fix this bug. It does the following: -- All windowed plugins are registered with their nsRootPresContext. -- Creates nsRootPresContext::UpdatePluginGeometry to reposition and set clip regions for all visible plugins that have a given frame as an ancestor. It does this by building a display list and traversing the resulting display list, looking for nsDisplayPlugin items that indicate a plugin should be visible. When no nsDisplayPlugin is found for a windowed plugin, we hide that plugin (for some reason or another, it's not visible). -- To do this, nsDisplayPlugin is exported in nsObjectFrame.h. It provides a method that (via the frame) computes the desired position and clip region of the plugin's widget, if any. -- nsObjectFrame no longer has its widget managed by its view. Instead nsObjectFrame owns the widget directly. However, the widget still has a HasViewFor wrapper pointing to nsObjectFrame's view so that events in the widget can be dispatched to the view. This is especially important on Mac, where plugin views actually still paint through Gecko. The widget is always a direct child of the root widget for the root prescontext. Its size and position are always set through UpdatePluginGeometry. -- nsDisplayPlugin sets IsOpaque properly so that if you have overlapping plugins, the one that's underneath has the top one's area clipped out. The resulting effect is that we see the correct z-ordering (without actually having to z-order native widgets properly). -- Call UpdatePluginGeometry when we paint due to a style change, when we reflow, when we scroll, and when we instantiate a plugin. -- UpdatePluginGeometry works by first collecting all the plugin widget geometry changes into a list, in nsRootPresContext::GetPluginGeometryUpdates, and then calling nsIWidget::ConfigureChildren to make them happen. This is a bit cumbersome for this stage, but we'll need GetPluginGeometryUpdates when we get to passing the widget configuration changes into nsIWidget::Scroll in bug 352093. -- nsViewManager has to be updated to handle the fact that we can now get paint events for views that don't seem to have a widget associated with them. It can also encounter widgets whose view does't know about the widget. In UpdateWidgetArea we handle this by passing in the widget as well as the view in case the view doesn't know about the widget. (This asymmetric relationship is a bit annoying, but it will go away when views go away.) With this patch, this bug is basically done. However there is an unacceptable regression: scrolling is handled very naively here. Plugins don't scroll smoothly with their surrounding content, the content moves first and then the plugin(s). That is fixed by the work I will attach in bug 352093.

Oops, I found part of another patch that logically belongs here. We need to change nsChildView::GetPluginClipRect to account for the clip region.

In part 9, the widget parameter passed to UpdateWidgetArea was just aWidgetView->GetNearestWidget in most cases, which is just what used to be in UpdateWidgetArea hoisted out. But in every one of those cases, the view is either a DisplayRoot or we already checked it has a widget. In both cases we can just use GetWidget instead of GetNearestWidget, and we should, because if the view didn't have a widget we'd be broken because we're ignoring the offset from the view to the widget. (If the view is a DisplayRoot and doesn't have a widget, then there is no relevant widget for us to update.)

These are some tests for complex plugin clipping. It includes tests for plugins partially covered by opaque HTML content, tests for clip region changes due to scrolling, tests for clip region changes due to z-index changes, and tests for correct plugin clip regions in the presence of subpixel geometry. It refactors the existing test_plugin_clipping to share code with the new tests.

BTW those tests are ignored on Mac. The Mac test plugin cannot report a complex clip region, since there isn't really any such thing since the Mac plugin doesn't know of any associated native window we could set it on. (Mac plugins draw fine in any case, because we control their drawing.)

For the next patch I'm going to attach, we need the functionality of nsDisplayBackground::IsOpaque. So let's make it a public static method.

I found that one site (Youtube IIRC) was positioning content over a plugin by creating a transparent IFRAME over the plugin, and then on top of that, positioning a table with an opaque background. Tables don't use nsDisplayBackground, so we need to do a little bit of work to make them report that they're opaque.

Is there a hg tree/branch with these changes? It would be a lot easier to work with that than having to apply multi-part patches.

Comment on attachment 385566 [details] [diff] [review] Part 3: create a native widget clip region API CreatePolyPolygonRgn will be much faster than repeated CreateRectRgn & CombineRgn & DeleteObject.

Try-server builds here: https://build.mozilla.org/tryserver-builds/rocallahan@mozilla.com-try-f9bf8fa4178/ although these patches have some fixes that aren't in those builds. I haven't quite figured out the best way to publish my patch queue. (In reply to comment #41) > (From update of attachment 385566 [details] [diff] [review]) > CreatePolyPolygonRgn will be much faster than repeated CreateRectRgn & > CombineRgn & DeleteObject. By far the most common case is that the number of rectangles is 1. If there was a CreatePolyRectRgn I'd use it, but I'd like to see a testcase where it really matters, and a benchmark showing CreatePolyPolygonRgn wins, before writing that more complicated code.

(In reply to comment #42) > Try-server builds here: > https://build.mozilla.org/tryserver-builds/rocallahan@mozilla.com-try-f9bf8fa4178/ > although these patches have some fixes that aren't in those builds. I haven't > quite figured out the best way to publish my patch queue. What about an HG branch on top of mozilla-central?

Comment on attachment 385567 [details] [diff] [review] Part 4: Remove nsViewManager::AddCoveringWidgetsToOpaqueRegion + NSView* view = [subviews objectAtIndex: i]; We don't put a space between ":" and arguments in Obj-C messages. + rgn->Subtract(frame.origin.x, frame.origin.y, frame.size.width, frame.size.height); Should make sure the frame coords are in the same system as the region you're subtracting from (not flipped, for example), I assume you tested that.

Make Kimura-san happy by avoiding lots of CombineRegion calls to build up regions.

To make Mac work consistently with other platforms, we should set the Mac plugin clip rect eagerly as soon as we've determined the correct widget geometry.

With the patch I just attached, we don't need to wait anymore for internal plugin geometry (in particular the Mac clipRect) to sync up, so we can simplify the plugin clipping tests a fair bit. And with the patches in bug 501295, we can enable the tests for Windows.

Comment on attachment 385957 [details] [diff] [review] Part 3 v3: Use ExtCreateRegion Thank you. Unfortunately, however, ExtCreateRegion doesn't allow the rectangles to overlap per MSDN. http://msdn.microsoft.com/en-us/library/dd162940%28VS.85%29.aspx That's why I recommended CreatePolyPolygonRgn. I don't think it will be much more complicate because every polygon can simulate each rectangle. That said, I will not persist for now because I can't prepare the benchmark right away.

These rectangles don't overlap.

Comment on attachment 385957 [details] [diff] [review] Part 3 v3: Use ExtCreateRegion Review from Boris on the view manager bits, Josh on the Mac impl (which doesn't really do anything), Karl on the GTK2 impl, Jim on the Windows impl.

Comment on attachment 385957 [details] [diff] [review] Part 3 v3: Use ExtCreateRegion Josh for Mac

Comment on attachment 385957 [details] [diff] [review] Part 3 v3: Use ExtCreateRegion Karl for GTK2

Comment on attachment 385957 [details] [diff] [review] Part 3 v3: Use ExtCreateRegion Jim for Windows

Comment on attachment 385957 [details] [diff] [review] Part 3 v3: Use ExtCreateRegion + // XXXroc should this be !outClipRect.IsEmpty()? Did you mean to resolve this or leave it as-is for now?

I wasn't sure whether I should change it. Continuing to return PR_TRUE is the conservative thing to do, but perhaps we could do better. It's fine to land as-is IMHO.

Comment on attachment 385567 [details] [diff] [review] Part 4: Remove nsViewManager::AddCoveringWidgetsToOpaqueRegion This seems OK as a reimplementation of existing behavior, but: + for (nsIWidget* kid = mFirstChild; kid; kid = kid->GetNextSibling()) { The nsIWidget hierarchy seems strange to me (in the GTK implementation at least). Child windows are not registered as children of their parent when created. However, if the child window is created through the nsIWidget (not nsNativeWidget) version of nsWindow::Create() and SetZIndex() is called on the child, then the child gets registered on its parent. (When this does happen, there are strong references each way so one of the nsIWidgets needs to be explicitly Destroy()ed.) I'm not sure how important this optimization is, but I doubt its general effectiveness in its current state. I guess AddCoveringWidgetsToOpaqueRegion didn't work well either, but there doesn't seem much point keeping the code if it doesn't work. The hierarchy does exist in the GdkWindows through gdk_window_peek_children() (as long as they have not been destroyed, which is when we care) and get_window_for_gdk_window() can be used to get back to the nsWindow.

I guess I'll reimplement this using gdk_window_peek_children.

Comment on attachment 385957 [details] [diff] [review] Part 3 v3: Use ExtCreateRegion Mac code looks fine.

Comment on attachment 385958 [details] [diff] [review] Part 10.5: eagerly fix up Mac clip rect +static PLDHashOperator +PluginDidSetGeometryEnumerator(nsPtrHashKey<nsObjectFrame>* aEntry, void* userArg) +{ + nsObjectFrame* f = aEntry->GetKey(); + f->DidSetWidgetGeometry(); If ->GetKey() can return a NULL pointer than we need a null check.

It can't.

Comment on attachment 385564 [details] [diff] [review] Part 1: factor out FindContainerView r+sr=bzbarsky

Comment on attachment 385565 [details] [diff] [review] Part 2: introduce nsRootPresContext >+++ b/layout/base/nsPresContext.cpp >+nsRootPresContext::nsRootPresContext(nsIDocument* aDocument, >+ nsPresContextType aType) Line up the args here, and r+sr=bzbarsky

(In reply to comment #44) > (From update of attachment 385567 [details] [diff] [review]) > + rgn->Subtract(frame.origin.x, frame.origin.y, frame.size.width, > frame.size.height); > > Should make sure the frame coords are in the same system as the region you're > subtracting from (not flipped, for example), I assume you tested that. isFlipped means that getRectsBeingDrawn returned rectangles with the origin at the top-left of the view, so rgn is also in that coordinate system. Doesn't [view frame] for the child views also return coordinates in the parent's coordinate system? It doesn't seem to be documented.

Comment on attachment 385957 [details] [diff] [review] Part 3 v3: Use ExtCreateRegion >+++ b/view/src/nsViewManager.cpp >@@ -537,49 +537,45 @@ void nsViewManager::AddCoveringWidgetsTo >+ aRgn.Or(aRgn, rr); Is it worth worrying about bounding the complexity of aRgn? Viewmanager changes look fine. I didn't really read the rest of the patch, but I don't understand why you use: > + const Configuration* configuration = &aConfigurations[i]; instead of: const Configuration& configuration = aConfigurations[i];

(In reply to comment #65) > (From update of attachment 385957 [details] [diff] [review]) > >+++ b/view/src/nsViewManager.cpp > >@@ -537,49 +537,45 @@ void nsViewManager::AddCoveringWidgetsTo > >+ aRgn.Or(aRgn, rr); > > Is it worth worrying about bounding the complexity of aRgn? I'm deliberately not bounding it, because in this case that would actually alter the visible appearance and I'm reluctant to do that in an unpredictable way. > Viewmanager changes look fine. I didn't really read the rest of the patch, but > I don't understand why you use: > > > + const Configuration* configuration = &aConfigurations[i]; > > instead of: > > const Configuration& configuration = aConfigurations[i]; No reason, they feel about the same to me.

> No reason, they feel about the same to me. I'd somewhat prefer the Configuration& version, then.

Comment on attachment 385957 [details] [diff] [review] Part 3 v3: Use ExtCreateRegion +#include <gtk/gtkprivate.h> I was a bit surprised to see that this header gets installed. + // Don't let GTK mess with the shapes of our GdkWindows + GTK_PRIVATE_SET_FLAG(GTK_WIDGET(mContainer), GTK_HAS_SHAPE_MASK); The only non-invasive way to deal with this that I can think of is to listen for the set-style signal on the container widgets and reapply all the shapes that have just been removed. That would be more code though and less effective. How often were the shapes being removed? + nsresult SetWindowClipRegion(const nsTArray<nsIntRect>& aRects); The windows version of this is protected. It seems that the gtk version can also be protected. + if (configuration->mClipRegion.IsEmpty()) { + w->Show(PR_FALSE); + } + if (!configuration->mClipRegion.IsEmpty()) { + w->Show(PR_TRUE); + } How important is this optimization? Two possible issues: 1. A windowed plugin may unexpectedly lose a grab when it becomes unviewable. (Perhaps a plugin is unlikely to become obscured while it has a grab, though?) 2. This seems to be interfering with the shown/hidden state of the window * It changes what nsIWidget::IsVisible() returns. * I fear it may cause a window to be shown that is not intended to be shown. If the optimization is important, then modifying AreBoundsSane() to consider the clip region (and probably change the method name) and using mNeedsShow, mIsShown, and NativeShow() would fix issue 2 (though it would be more complicated, unfortunately). It looks like showing or hiding the window always effects an Xlib call, irrespective of the current state, so a check on whether a change in state is necessary would probably be worthwhile. + if (!mDrawingarea) + return NS_ERROR_UNEXPECTED; mDrawingarea will be null if the nsIWidget has been Destroy()ed, which may not be completely unexpected.

(In reply to comment #68) > (From update of attachment 385957 [details] [diff] [review]) > +#include <gtk/gtkprivate.h> > > I was a bit surprised to see that this header gets installed. > > + // Don't let GTK mess with the shapes of our GdkWindows > + GTK_PRIVATE_SET_FLAG(GTK_WIDGET(mContainer), GTK_HAS_SHAPE_MASK); > > The only non-invasive way to deal with this that I can think of is to listen > for the set-style signal on the container widgets and reapply all the shapes > that have just been removed. That would be more code though and less > effective. I think it would also create flicker. > How often were the shapes being removed? I'm not sure. > + nsresult SetWindowClipRegion(const nsTArray<nsIntRect>& aRects); > > The windows version of this is protected. > It seems that the gtk version can also be protected. OK. > + if (configuration->mClipRegion.IsEmpty()) { > + w->Show(PR_FALSE); > + } > > + if (!configuration->mClipRegion.IsEmpty()) { > + w->Show(PR_TRUE); > + } > > How important is this optimization? > Two possible issues: > > 1. A windowed plugin may unexpectedly lose a grab when it becomes unviewable. > (Perhaps a plugin is unlikely to become obscured while it has a grab, > though?) Hmm, perhaps. My scrolling patch in the other bug is much more likely to trigger this problem, although again, we probably shouldn't be scrolling while the plugin has a grab. > 2. This seems to be interfering with the shown/hidden state of the window > > * It changes what nsIWidget::IsVisible() returns. > > * I fear it may cause a window to be shown that is not intended to be shown. > > If the optimization is important, then modifying AreBoundsSane() to consider > the clip region (and probably change the method name) and using mNeedsShow, > mIsShown, and NativeShow() would fix issue 2 (though it would be more > complicated, unfortunately). I was hoping not to have to get into that, on the grounds that only plugins are affected here and we don't explicitly manage the hiding or showing of plugin windows in any other way. > It looks like showing or hiding the window always effects an Xlib call, > irrespective of the current state, so a check on whether a change in state is > necessary would probably be worthwhile. OK. > + if (!mDrawingarea) > + return NS_ERROR_UNEXPECTED; > > mDrawingarea will be null if the nsIWidget has been Destroy()ed, which may not > be completely unexpected. OK, I'll change that.

For Part 3 v3: Use ExtCreateRegion - + // MSDN says we should do this before moving or resizing the window + // See http://msdn.microsoft.com/en-us/library/aa930600.aspx + // We put the region back just below, anyway. + ::SetWindowRgn(w->mWnd, NULL, FALSE); This is going to blow away transparency clips on panels and maybe popups, unless other changes restrict the clip changes to certain window types that wouldn't have these set - http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsWindow.cpp#1233 Maybe use Clear and SetThemeRegion pre and post instead? Also, looks like this was posted before the nsWindow reorg. Can we make sure and get everything updated (proper placements of the code in nsWindow.h /nsWindow.cpp).

(In reply to comment #70) > This is going to blow away transparency clips on panels and maybe popups, > unless other changes restrict the clip changes to certain window types that > wouldn't have these set - > > http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsWindow.cpp#1233 > > Maybe use Clear and SetThemeRegion pre and post instead? This will only be used on plugins, or at least child windows, as per the documentation in nsIWidget and the fact that ConfigureChildren has to be called on the parent widget, so I don't think that should be necessary. > Also, looks like this was posted before the nsWindow reorg. Can we make sure > and get everything updated (proper placements of the code in nsWindow.h > /nsWindow.cpp). It's updated in my tree. I'll attach the new patch.

(In reply to comment #68) > (From update of attachment 385957 [details] [diff] [review]) > +#include <gtk/gtkprivate.h> > > I was a bit surprised to see that this header gets installed. > > + // Don't let GTK mess with the shapes of our GdkWindows > + GTK_PRIVATE_SET_FLAG(GTK_WIDGET(mContainer), GTK_HAS_SHAPE_MASK); > > The only non-invasive way to deal with this that I can think of is to listen > for the set-style signal on the container widgets and reapply all the shapes > that have just been removed. That would be more code though and less > effective. > > How often were the shapes being removed? Karl and I talked about this issue and I think I convinced him that we really need GTK to expose a gtk_window_shape_combine_region API, and we'll have to use GTK_PRIVATE_SET_FLAG until then. He did convince me, though, to stop hiding and showing widgets when their regions become empty/non-empty.

(In reply to comment #72) It sounded like the shapes were not being removed so often that flicker would be a big problem. I think using the set-style signal would probably work as I get the impression that shapes are being removed because styles are expected to set shapes. A gtk_widget_shape_combine_region API is actually not quite what we need because we only set shape regions on descendant windows in the GtkWidget. What we really need is a means to tell GTK not to mess with the shapes of GdkWindows on our GtkWidget. That is precisely what GTK_HAS_SHAPE_MASK is, so that's the main reason why I'm not objecting to using this.

Changed to iterate through the GdkWindow children.

(In reply to comment #66) > (In reply to comment #65) > > (From update of attachment 385957 [details] [diff] [review] [details]) > > >+++ b/view/src/nsViewManager.cpp > > >@@ -537,49 +537,45 @@ void nsViewManager::AddCoveringWidgetsTo > > >+ aRgn.Or(aRgn, rr); > > > > Is it worth worrying about bounding the complexity of aRgn? > > I'm deliberately not bounding it, because in this case that would actually > alter the visible appearance and I'm reluctant to do that in an unpredictable > way. This comment was completely wrong. I'm not bounding the complexity of this region because this code is removed in Part 4.

Updated for comments

To get the tests to work well, I have to make sure that the last clip rect set is reasonable. Currently, the clip rect is always empty except while we're painting or briefly after we've scrolled. Is that intentional? I don't see how. This version of the patch removes ePluginPaintIgnore and makes HandleEvent and Notify(timer) pass ePluginPaintEnable instead. If this doesn't make sense, I'd like to know why the plugin clip rect needs to be empty most of the time (and we'll probably just have to disable the tests for the clip rect).

Comment on attachment 385568 [details] [diff] [review] Part 5: support computation of accurate visible regions in the display list system >+ * Call this setter makes us compute accurate visible regions at the cost >+ * of performance if regions get very complex. >+ * s/Call/Calling/ drop the extra blank line at the end r=dbaron

Comment on attachment 385570 [details] [diff] [review] Part 7: Create nsLayoutUtils::IsAncestorFrameCrossDoc Maybe comment that it differs from IsProperAncestorFrameCrossDoc only in that it returns true when aFrame == aAncestorFrame, and add the reverse comment for IsProper...Doc ? Also, please reorder your patch queue so that this comes before patch 6, since patch 6 depends on this one. (It helps bisecting if there are fewer revisions that don't compile.) r=dbaron with that

Comment on attachment 385569 [details] [diff] [review] Part 6: Uninline IsMovingFrame r=dbaron given that the patch order is swapped with part 7.

Comment on attachment 385571 [details] [diff] [review] Part 8: create nsPoint::ToNearestPixels >+nsIntPoint >+nsPoint::ToNearestPixels(nscoord aAppUnitsPerPixel) const { Probably nice to write "inline nsIntPoint ..." here. r=dbaron

(In reply to comment #79) > (From update of attachment 385570 [details] [diff] [review]) > Maybe comment that it differs from IsProperAncestorFrameCrossDoc only in that > it returns true when aFrame == aAncestorFrame, and add the reverse comment for > IsProper...Doc ? Sure. > Also, please reorder your patch queue so that this comes before patch 6, since > patch 6 depends on this one. (It helps bisecting if there are fewer revisions > that don't compile.) Will do.

Comment on attachment 386134 [details] [diff] [review] Part 9 v2: fix nsViewManager::UpdateWidgetArea to handle plugin widgets correctly per-platform Here are my comments on the layout/base part of patch 9: You need to either: * make the destructor of nsPresContext virtual (and for style, explicitly mark the destructor of nsRootPresContext as virtual too), or * reimplement release on nsRootPresContext (there might not even be an appropriate macro) so that mRegisteredPlugins's destructor gets called and so that your assertion in ~nsRootPresContext gets tested. (I'm glad you have the assertion in ~nsRootPresContext; I was about to write a comment saying you should add exactly that assertion.) + /** + * Registers a plugin to receive geometry updates (position and clip + * region) so it can update its widget. + */ + void RegisterPluginForGeometryUpdates(nsObjectFrame* aPlugin); Comment that callers are required to unregister before the frame is destroyed? + void GetPluginGeometryUpdates(nsIFrame* aChangedRoot, + nsTArray<nsIWidget::Configuration>* aConfigurations); Make this private? nsPresContext.cpp: (applies to patch 2 as well) Could you put the nsRootPresContext stuff at the end of the file rather than not-quite-at-the-end? +#ifdef DEBUG +#include <stdio.h> Don't ifdef an include by an condition other than the one under which the header is available. (And just put it up at the top, please.) +void +nsRootPresContext::UpdatePluginGeometry(nsIFrame* aChangedRoot) +{ Could you add some assertions here to check that aChangedRoot is (1) the root frame of (2) a pres context that's inside this one? Moving backwards: + if (fBounds.Intersects(closure->mChangedRect) || + nsLayoutUtils::IsAncestorFrameCrossDoc(closure->mChangedRoot, f)) { I don't understand why this condition is what it is. Could you add a comment to explain? (I could understand wanting the set of frames that match one of the conditions or the set that match the other, but I'm not quite sure yet what you're going to do with the set of frames that match either one of them. Maybe I'll be enlightened by further reading, though.) Or did you mean && ? + if (aClosure->mAffectedPlugins.GetEntry(f)) { + displayPlugin->GetWidgetConfiguration(aBuilder, + aClosure->mOutputConfigurations); + // we've dealt with this plugin now + aClosure->mAffectedPlugins.RemoveEntry(f); + } If you save the result of GetEntry you can (and probably should) use RawRemoveEntry. (I think you don't care about not resizing the table since the table is short-lived.) + // For now, just hide all windowed plugins in transformed content. + // Fall through to default. GetList below will return null, since + // the transform list isn't exposed that way, and we won't see + // any nsDisplayPlugins in or under the list, so we'll hide the + // plugin(s). It seems a little problematic to do this only here. Couldn't the plugins end up involved in opacity analysis (well, probably not) or event handling (more likely)? Can we just prevent them from being put on the display list instead, and rely on that here? (The code should probably have comments pointing to the other places that rely on the behavior, though.)

(In reply to comment #83) > You need to either: > * make the destructor of nsPresContext virtual (and for style, explicitly > mark the destructor of nsRootPresContext as virtual too), or It subclasses nsIObserver, doesn't that mean the destructor is already virtual? I'll mark it and the nsRootPresContext destructor virtual though. > Comment that callers are required to unregister before the frame is > destroyed? Sure > + void GetPluginGeometryUpdates(nsIFrame* aChangedRoot, > + nsTArray<nsIWidget::Configuration>* > aConfigurations); > > Make this private? No, we'll call it elsewhere later (in the scrolling patch in bug 352093). > (applies to patch 2 as well) Could you put the nsRootPresContext stuff > at the end of the file rather than not-quite-at-the-end? OK > +#ifdef DEBUG > +#include <stdio.h> > > Don't ifdef an include by an condition other than the one under which > the header is available. (And just put it up at the top, please.) OK > +void > +nsRootPresContext::UpdatePluginGeometry(nsIFrame* aChangedRoot) > +{ > > Could you add some assertions here to check that aChangedRoot is (1) the > root frame of (2) a pres context that's inside this one? It's not necessarily a root frame. It's the root of a frame subtree whose frames may have moved or otherwise changed. (The aChangedRoot itself is not allowed to have changed its size, position or overflow area.) I can write a better comment in nsPresContext.h. Should I change the name to aChangedSubtree? > Moving backwards: > + if (fBounds.Intersects(closure->mChangedRect) || > + nsLayoutUtils::IsAncestorFrameCrossDoc(closure->mChangedRoot, f)) { > > I don't understand why this condition is what it is. Could you add a > comment to explain? (I could understand wanting the set of frames that > match one of the conditions or the set that match the other, but I'm not > quite sure yet what you're going to do with the set of frames that match > either one of them. Maybe I'll be enlightened by further reading, > though.) We're identifying the plugins that may have been affected by changes to the frame subtree rooted at aChangedRoot. Any plugin that overlaps the overflow area of aChangedRoot could have its clip region affected because it might be covered (or uncovered) by changes to the subtree. Plugins in the subtree might have changed position and/or size, and they might not be in aChangedRoot's overflow area (because they're being clipped by an ancestor in the subtree). I'll add this as a comment. > If you save the result of GetEntry you can (and probably should) use > RawRemoveEntry. (I think you don't care about not resizing the table > since the table is short-lived.) OK. > + // For now, just hide all windowed plugins in transformed content. > + // Fall through to default. GetList below will return null, since > + // the transform list isn't exposed that way, and we won't see > + // any nsDisplayPlugins in or under the list, so we'll hide the > + // plugin(s). > > It seems a little problematic to do this only here. Couldn't the > plugins end up involved in opacity analysis (well, probably not) or > event handling (more likely)? Can we just prevent them from being put > on the display list instead, and rely on that here? (The code should > probably have comments pointing to the other places that rely on the > behavior, though.) You're right. I can create a separate patch to stop plugins from going on the display list, and take out this comment.

(In reply to comment #84) > It subclasses nsIObserver, doesn't that mean the destructor is already virtual? No. XPCOM interfaces don't have virtual destructors, and many classes that implement them don't need virtual destructors either; the only caller of the destructor should normally be Release, which is virtual, so implementations that don't expect derived classes don't need virtual destructors. > > +void > > +nsRootPresContext::UpdatePluginGeometry(nsIFrame* aChangedRoot) > > +{ > > > > Could you add some assertions here to check that aChangedRoot is (1) the > > root frame of (2) a pres context that's inside this one? > > It's not necessarily a root frame. It's the root of a frame subtree whose > frames may have moved or otherwise changed. (The aChangedRoot itself is not > allowed to have changed its size, position or overflow area.) I can write a > better comment in nsPresContext.h. Should I change the name to aChangedSubtree? Er, right. Maybe the name change would be good. > We're identifying the plugins that may have been affected by changes to the > frame subtree rooted at aChangedRoot. Any plugin that overlaps the overflow > area of aChangedRoot could have its clip region affected because it might be > covered (or uncovered) by changes to the subtree. Plugins in the subtree might > have changed position and/or size, and they might not be in aChangedRoot's > overflow area (because they're being clipped by an ancestor in the subtree). > I'll add this as a comment. Sounds good.

Comment on attachment 388191 [details] [diff] [review] Part 4 v2: Remove nsViewManager::AddCoveringWidgetsToOpaqueRegion r+=me on GTK (and nsViewManager) parts. >+ kid->GetBounds(bounds); nsWindow::Scroll() currently doesn't necessarily update mBounds but it looks like that will be fixed in bug 352093.

Comment on attachment 388192 [details] [diff] [review] Part 3 v4: create a native widget clip region API + // XXX using GTK_PRIVATE_SET_FLAG sucks, but we have to do it + // until GTK gets a gtk_window_set_shape_region API. I don't think gtk_widget_set_shape_region is going to be what we want because we only want to set the shape on one of the windows in the widget, so I'd prefer to leave out this part of the comments. r+=me on the GTK impl.

Comment on attachment 386134 [details] [diff] [review] Part 9 v2: fix nsViewManager::UpdateWidgetArea to handle plugin widgets correctly per-platform ...continued from comment 83. nsObjectFrame.cpp: + dx->SupportsNativeWidgets(usewidgets); As far as I can tell, there's one implementation of SupportsNativeWidgets in the tree (in nsThebesDeviceContext), and it always returns true. Were you expecting something that returned false on Mac? see also http://mxr.mozilla.org/mozilla1.8/source/gfx/src/mac/nsDeviceContextMac.cpp#155 + nsIWidget* parentWidget = + rpc->PresShell()->FrameManager()->GetRootFrame()->GetWindow(); Is that really the cleanest way? +nsDisplayPlugin::GetBounds(nsDisplayListBuilder* aBuilder) { Brace on its own line, please? (Though it is consistent with the nsDisplayList.cpp code, I suppose; but not with the 3 methods just below.) +void +nsDisplayPlugin::Paint(nsDisplayListBuilder* aBuilder, nsIRenderingContext* aCtx, + const nsRect& aDirtyRect) +PRBool +nsDisplayPlugin::OptimizeVisibility(nsDisplayListBuilder* aBuilder, + nsRegion* aVisibleRegion) +{ Weird indentation, and please wrap at less than 80 characters for the first one. + nscoord appUnitsPerDevPixel = presContext->AppUnitsPerDevPixel(); How about PRInt32 ? + nsRect bounds = GetContentRect() + GetParent()->GetOffsetTo(rootFrame); It might be nice to document in nsIFrame.h that GetOffsetTo works across frames; I had to read the source code to verify that. + // Snap *r to pixels while its relative to the painted widget, to it's (maybe a . at the end too) nsObjectFrame::ComputeWidgetGeometry could perhaps use a comment that aRegion and aPluginOrigin are in the same coordinate space (which can be anything, as long as they're the same). Please document AttachWidgetEventHandler/DetachWidgetEventHandler in nsIView.h - if (view && view->GetVisibility() == nsViewVisibility_kShow) { + PRBool visible; + childWidget->IsVisible(visible); + if (view && visible && !IsWidgetDrawnByPlugin(childWidget, view)) { What are the implications of dropping the check for view visibility? (I'm sort of hoping view visibility goes away sometime soon, since it's never really made sense to me.) r=dbaron

Comment on attachment 385579 [details] [diff] [review] Part 10: simplify widget parameter passed to UpdateWidgetArea Oh, good. I got a little confused about why those were needed while reading patch 9. Good to see them go away. r=dbaron

Comment on attachment 385582 [details] [diff] [review] Part 12: export nsDisplayBackground::IsOpaque + * will be used. It does not check nsStyleVisibility::mVisible, the s/mVisible,/mVisible;/ The static version doesn't need the aBuilder parameter; seems like you may as well drop it. (Do any IsOpaque implementations need it?) r=dbaron (though given my comments on the next patch I'm not sure we'll need it)

Comment on attachment 385583 [details] [diff] [review] Part 13: make display lists aware of opaque table backgrounds This doesn't seem right. nsDisplayTableBorderBackground's bounds are the overflow rect of the table, and it's not opaque over that area. Now, the bounds don't actually need to extend that far, but with border collapsing I think half the border does extend past the edge of the background. You probably could come up with a patch here that reported nsDisplayTableBorderBackground to be opaque in a bunch of cases, but you'd need a whole bunch more checks. However, I think it might actually make more sense to move the drawing of the table's background (and nothing else; no sub-element backgrounds) into its own display item (which I think could probably just be an nsDisplayBackground) and out of nsTablePainter. But we should probably loop fantasai in on this discussion. review-

(In reply to comment #88) > nsObjectFrame.cpp: > > + dx->SupportsNativeWidgets(usewidgets); > > As far as I can tell, there's one implementation of > SupportsNativeWidgets in the tree (in nsThebesDeviceContext), and it > always returns true. Were you expecting something that returned false > on Mac? I basically just inlined a call to nsIView::CreateWidget here. OK if I just file a followup bug to remove SupportsNativeWidgets? > + nsIWidget* parentWidget = > + rpc->PresShell()->FrameManager()->GetRootFrame()->GetWindow(); > > Is that really the cleanest way? Yes, I think it actually is the cleanest way that doesn't addref. rpc->PresShell()->GetViewManager()->GetRootWidget(getter_AddRefs(parentWidget)) is one step shorter but isn't really simpler. Probably I should add a GetWidget method to nsRootPresContext and start converting users to it. > +nsDisplayPlugin::GetBounds(nsDisplayListBuilder* aBuilder) { > > Brace on its own line, please? (Though it is consistent with the > nsDisplayList.cpp code, I suppose; but not with the 3 methods just > below.) OK > +void > +nsDisplayPlugin::Paint(nsDisplayListBuilder* aBuilder, nsIRenderingContext* > aCtx, > + const nsRect& aDirtyRect) > > +PRBool > +nsDisplayPlugin::OptimizeVisibility(nsDisplayListBuilder* aBuilder, > + nsRegion* aVisibleRegion) > +{ > > Weird indentation, and please wrap at less than 80 characters for the > first one. OK > + nscoord appUnitsPerDevPixel = presContext->AppUnitsPerDevPixel(); > > How about PRInt32 ? OK. > + nsRect bounds = GetContentRect() + GetParent()->GetOffsetTo(rootFrame); > > It might be nice to document in nsIFrame.h that GetOffsetTo works across > frames; I had to read the source code to verify that. OK. > + // Snap *r to pixels while its relative to the painted widget, to > > it's > > (maybe a . at the end too) OK > nsObjectFrame::ComputeWidgetGeometry could perhaps use a comment that > aRegion and aPluginOrigin are in the same coordinate space (which can be > anything, as long as they're the same). OK > Please document AttachWidgetEventHandler/DetachWidgetEventHandler in > nsIView.h OK > - if (view && view->GetVisibility() == nsViewVisibility_kShow) { > + PRBool visible; > + childWidget->IsVisible(visible); > + if (view && visible && !IsWidgetDrawnByPlugin(childWidget, view)) { > > What are the implications of dropping the check for view visibility? > (I'm sort of hoping view visibility goes away sometime soon, since it's > never really made sense to me.) Widget visibility tracks view visibility for views with widgets. Here we actually care about widget visibility; visible widgets need to be repainted no matter what their view says, and hidden widgets don't (and don't cover their parent), no matter what their view says.

(In reply to comment #91) > This doesn't seem right. nsDisplayTableBorderBackground's bounds are > the overflow rect of the table, and it's not opaque over that area. Good catch. > However, I think it might actually make more sense to move the drawing > of the table's background (and nothing else; no sub-element backgrounds) > into its own display item (which I think could probably just be an > nsDisplayBackground) and out of nsTablePainter. But we should probably > loop fantasai in on this discussion. That would work. We need to report opaqueness for table backgrounds somehow or we'll break sites (e.g., Youtube) that position opaque-background tables over plugins (with a transparent IFRAME behind them) and expect the content to appear over the plugin.

(In reply to comment #92) > > As far as I can tell, there's one implementation of > > SupportsNativeWidgets in the tree (in nsThebesDeviceContext), and it > > always returns true. Were you expecting something that returned false > > on Mac? > > I basically just inlined a call to nsIView::CreateWidget here. OK if I just > file a followup bug to remove SupportsNativeWidgets? Sure. > Probably I should add a GetWidget method to nsRootPresContext and start > converting users to it. Yeah, that would be good; doesn't need to happen immediately.

I filed bug 505184 to deal with the table background issue. It blocks this one.

This just gives us a flag in nsDisplayListBuilder to detect when we're in a -moz-transform or SVG foreignObject context. We hide windowed plugins in all such contexts.

Builds on the code in this bug to test bug 505184 --- tables with opaque backgrounds should be made visible by excluding them from the plugin clip region.

Comment on attachment 389444 [details] [diff] [review] Part 14: hide plugins in CSS-transformed or SVG foreignObject contexts >+ nsDisplayListBuilder::AutoInTransformSetter >+ setInTransform(aBuilder, inTransform); Maybe call this "inTransformSetter" instead of "setInTransform", since the latter sounds too much like the name of a function? r=dbaron

http://hg.mozilla.org/mozilla-central/rev/a93636d14be6 http://hg.mozilla.org/mozilla-central/rev/71c896fcd12e http://hg.mozilla.org/mozilla-central/rev/8d7d268bd181 http://hg.mozilla.org/mozilla-central/rev/a1161f2b4595 http://hg.mozilla.org/mozilla-central/rev/eb98a28b69a3 http://hg.mozilla.org/mozilla-central/rev/fce2b34ac030 http://hg.mozilla.org/mozilla-central/rev/c2b46638b84a http://hg.mozilla.org/mozilla-central/rev/d54c8b8b2ecc http://hg.mozilla.org/mozilla-central/rev/6d131711f67f http://hg.mozilla.org/mozilla-central/rev/273af2d98747 http://hg.mozilla.org/mozilla-central/rev/ff08730973b9 http://hg.mozilla.org/mozilla-central/rev/3568b5e81a50 http://hg.mozilla.org/mozilla-central/rev/d3156c132bc1 http://hg.mozilla.org/mozilla-central/rev/5b0d9f36c0b3

(In reply to comment #27) > Created an attachment (id=385567) [details] > Part 4: Remove nsViewManager::AddCoveringWidgetsToOpaqueRegion > > nsViewManager::AddCoveringWidgetsToOpaqueRegion tries to exclude from the paint > region areas which are covered by child widgets. It's a pain to maintain, and > completely unnecessary on Windows since Windows already does it for us. Now > that all platforms pass down a paint region in the NS_PAINT event, we can just > make this the platform's responsibility. This is also unnecessary for gtk2, which subtracts child window regions from the region areas, since at least version 2.10 (the minimum required version for building mozilla).

I seem to recall running tests that indicated that wasn't happening. Maybe Karl could look into it.

(In reply to comment #100) > This is also unnecessary for gtk2, which subtracts child window regions from > the region areas, since at least version 2.10 (the minimum required version for > building mozilla). I had a look at gtk+-2.16.5, and gdk_window_process_updates_internal() doesn't look like it considers child windows. So I checked _gdk_window_move_resize_child() and gdk_window_x11_show() but neither of them seem to remove the visible child region from the update area. That would mean that gdk_window_process_updates_internal() could/would request that regions are redrawn even though they are covered by opaque regions of windows. If I've missed something, can you point me at the function that subtracts child windows, please?

The methods mentioned will (possibly indirectly) call gdk_window_invalidate_maybe_recurse(), which subtracts child window regions.

Thanks. In gtk+-2.16.5, gdk_window_invalidate_maybe_recurse() only subtracts child window regions when !child->shaped. Also, that is happening at invalidation time; once a region is in the update_area it does not look like it gets removed (before the expose) if it is covered by child windows after invalidate but before the expose.