Move plugin widgets to the top of the widget hierarchy

RESOLVED FIXED in mozilla1.9.2a1

Status

()

defect
RESOLVED FIXED
13 years ago
9 years ago

People

(Reporter: roc, Assigned: roc)

Tracking

(Depends on 1 bug)

Trunk
mozilla1.9.2a1
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(16 attachments, 18 obsolete attachments)

41.16 KB, application/octet-stream
Details
1.04 KB, text/html
Details
10.88 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
9.14 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
4.94 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
3.83 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
2.06 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
1.33 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
2.43 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
17.44 KB, patch
Details | Diff | Splinter Review
48.26 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
8.96 KB, patch
karlt
: review+
Details | Diff | Splinter Review
29.71 KB, patch
karlt
: review+
jimm
: review+
Details | Diff | Splinter Review
6.20 KB, patch
jaas
: review+
Details | Diff | Splinter Review
15.83 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
6.81 KB, patch
Details | Diff | Splinter Review
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.
Posted patch patch version zero (obsolete) — Splinter Review
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.
Posted file testcase (obsolete) —
testcase for "DIV over plugin" and scrolling
Posted patch improved patch (obsolete) — Splinter Review
Cleaned up a bit, plus some entirely untested code for Windows support.
Attachment #223652 - Attachment is obsolete: true
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 :-).
Posted patch full patch (obsolete) — Splinter Review
Sorry, the previous patch had some parts missing (gfx and modules/plugin changes). This is the complete patch.
Attachment #223735 - Attachment is obsolete: true
Posted patch view bits (obsolete) — Splinter Review
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.
Attachment #224484 - Attachment is obsolete: true
Attachment #226085 - Attachment is obsolete: true

Comment 11

13 years ago
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.
Posted patch patch merged to TIP (obsolete) — Splinter Review
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. ;-)
Attachment #245728 - Attachment is obsolete: true
Attachment #245889 - Attachment is obsolete: true

Comment 21

12 years ago
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.
Attachment #385564 - Flags: review?(bzbarsky)
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.
Attachment #385564 - Flags: review?(bzbarsky)
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.
Attachment #385567 - Flags: review?(vladimir)
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.
Attachment #385567 - Flags: review?(vladimir)
Attachment #385567 - Flags: review?(mozbugz)
Attachment #385567 - Flags: review?(joshmoz)
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.
Attachment #385568 - Flags: review?(dbaron)
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.
Attachment #385569 - Flags: review?(dbaron)
We need a method like nsLayoutUtils::IsProperAncestorFrameCrossDoc, but that returns true when the specified ancestor is the frame itself.
Attachment #385570 - Flags: review?(dbaron)
We need to be able to snap an appunits point to the nearest pixel corner.
Attachment #385571 - Flags: review?(dbaron)
Posted patch Part 9: the main patch (obsolete) — Splinter Review
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.
Posted patch Part 3 v2 (obsolete) — Splinter Review
Oops, I found part of another patch that logically belongs here. We need to change nsChildView::GetPluginClipRect to account for the clip region.
Attachment #385566 - Attachment is obsolete: true
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.
Attachment #385582 - Flags: review?(dbaron)
Attachment #385582 - Attachment description: Part 12: → Part 12: export nsDisplayBackground::IsOpaque
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.
Attachment #385583 - Flags: review?(dbaron)

Comment 40

10 years ago
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.

Updated

10 years ago
Depends on: 500933
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.

Comment 43

10 years ago
(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 44

10 years ago
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.
Attachment #385567 - Flags: review?(joshmoz) → review+
Make Kimura-san happy by avoiding lots of CombineRegion calls to build up regions.
Attachment #385578 - Attachment is obsolete: true
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.
Attachment #226363 - Attachment is obsolete: true
Attachment #254566 - Attachment is obsolete: true
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
Attachment #385957 - Flags: review?(joshmoz)
Comment on attachment 385957 [details] [diff] [review]
Part 3 v3: Use ExtCreateRegion

Karl for GTK2
Attachment #385957 - Flags: review?(mozbugz)
Comment on attachment 385957 [details] [diff] [review]
Part 3 v3: Use ExtCreateRegion

Jim for Windows

Comment 55

10 years ago
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 59

10 years ago
Comment on attachment 385957 [details] [diff] [review]
Part 3 v3: Use ExtCreateRegion

Mac code looks fine.
Attachment #385957 - Flags: review?(joshmoz) → review+

Updated

10 years ago
Attachment #385958 - Flags: review?(joshmoz) → review+

Comment 60

10 years ago
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.
Attachment #385564 - Flags: superreview+
Attachment #385564 - Flags: review?(bzbarsky)
Attachment #385564 - Flags: review+
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
Attachment #385565 - Flags: superreview+
Attachment #385565 - Flags: review?(bzbarsky)
Attachment #385565 - Flags: review+
(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];
Attachment #385957 - Flags: review?(bzbarsky) → review+
(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.
Attachment #385567 - Attachment is obsolete: true
Attachment #388191 - Flags: review?(mozbugz)
Attachment #385567 - Flags: review?(mozbugz)
(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.
Attachment #385957 - Attachment is obsolete: true
Attachment #385957 - Flags: review?(mozbugz)
Attachment #385957 - Flags: review?(jmathies)
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).
Attachment #385958 - Attachment is obsolete: true
Attachment #388195 - Flags: review?(joshmoz)

Updated

10 years ago
Attachment #388192 - Flags: review?(jmathies) → review+
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
Attachment #385568 - Flags: review?(dbaron) → review+
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
Attachment #385570 - Flags: review?(dbaron) → review+
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
Attachment #385571 - Flags: review?(dbaron) → review+
(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.
Attachment #388191 - Flags: review?(mozbugz) → review+
Attachment #388192 - Flags: review?(mozbugz) → review+
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
Attachment #386134 - Flags: review?(dbaron) → review+
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
Attachment #385579 - Flags: review?(dbaron) → review+
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)
Attachment #385582 - Flags: review?(dbaron) → review+
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-
Attachment #385583 - Flags: review?(dbaron) → 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.

Updated

10 years ago
Attachment #388195 - Flags: review?(joshmoz) → review+
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.
Attachment #389444 - Flags: review?(dbaron)
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
Attachment #389444 - Flags: review?(dbaron) → review+
Depends on: 505896

Updated

10 years ago
Depends on: 505912
Depends on: 507926

Updated

10 years ago
Depends on: 508908

Updated

10 years ago
Depends on: 510082

Comment 100

10 years ago
(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?

Comment 103

10 years ago
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.
Depends on: 517949
Target Milestone: --- → mozilla1.9.2a1
Depends on: 518506
Depends on: 530605
You need to log in before you can comment on or make changes to this bug.